Updated Export-AzResourceGroup cmdlet to use the SDK. by Xynoclafe · Pull Request #13275 · Azure/azure-powershell

Xynoclafe · 2020-10-20T20:28:12Z

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate ChangeLog.md file(s) has been updated:
- For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
- A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
The PR does not introduce breaking changes
If applicable, the changes made in the PR have proper test coverage
For public API changes to cmdlets:
- a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
- the markdown help files have been regenerated using the commands listed here

…to xynoclafe/exportTemplateSDK

Xynoclafe · 2020-10-22T19:41:28Z

/azp run

azure-pipelines · 2020-10-22T19:41:47Z

Azure Pipelines successfully started running 3 pipeline(s).

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs

filizt · 2020-10-23T15:42:26Z

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs

+                        Options = this.GetExportOptions(),
+                    };
+
+                    var exportedTemplate = ResourceManagerSdkClient.ExportResourceGroup(ResourceGroupName, parameters);


Have you had a chance to test export async route? AFAIK, the service side goes into async operation based on the time it takes to export or based on the number of resources (I think it was >= 20 resources). It might be easier to test async route by trying more than 20 resources to export rather than trying to hit the timebound. Do you mind testing this scenario, if you haven't already.

I don't quite understand the scenario.

No worries. We'll chat offline.

In reply to: 510976108 [](ancestors = 510976108)

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs

…clafe/azure-powershell into xynoclafe/exportTemplateSDK

…to xynoclafe/exportTemplateSDK

isra-fel

I'm good with the implmenetation. Please fix the test erorrs.

…to xynoclafe/exportTemplateSDK

Xynoclafe · 2020-10-26T20:34:53Z

I'm good with the implmenetation. Please fix the test erorrs.

Test errors are fixed and all checks seem to be passing

isra-fel

LGTM

filizt · 2020-10-27T13:08:42Z

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs

-                var template = JToken.FromObject(JObject.Parse(resultString)["template"]);
-
-                if (JObject.Parse(resultString)["error"] != null)
+                if (this.IsParameterBound(c => c.ApiVersion))


This logic seems like it should be reversed - if we have an apiVersion provided we should go through the Rest API client, otherwise SDK client.

filizt · 2020-10-27T13:11:23Z

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs

+                        Options = this.GetExportOptions(),
+                    };
+                    var apiVersion = this.ApiVersion ?? DefaultApiVersion;
+                    var operationResult = this.GetResourcesClient()


You also wouldn't need this line after "this.IsParameterBound(c => c.ApiVersion"

filizt · 2020-10-27T13:11:48Z

    private const string DefaultApiVersion = "2020-06-01";

And no need for this const.

Refers to: src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs:44 in fbcb08b. [](commit_id = fbcb08b, deletion_comment = False)

Xynoclafe added 4 commits October 15, 2020 11:19

Export template with SDK #1

83792f9

Export template with SDK #2

89a48cf

Merge branch 'master' of https://github.com/Azure/azure-powershell in…

a734307

…to xynoclafe/exportTemplateSDK

Update changelog

c741602

Xynoclafe requested a review from filizt October 20, 2020 20:28

azuresdkci added the needs-review label Oct 20, 2020

Xynoclafe closed this Oct 20, 2020

Xynoclafe reopened this Oct 20, 2020

Xynoclafe marked this pull request as draft October 20, 2020 20:53

Added back support for -ApiVersion parameter with a deprecation notice

e72e11e

Xynoclafe marked this pull request as ready for review October 22, 2020 15:37

isra-fel self-assigned this Oct 23, 2020

isra-fel requested changes Oct 23, 2020

View reviewed changes

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Outdated Show resolved Hide resolved

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Outdated Show resolved Hide resolved

update changelog to avoid merge issue

4a00dbb

isra-fel reviewed Oct 23, 2020

View reviewed changes

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Outdated Show resolved Hide resolved

filizt reviewed Oct 23, 2020

View reviewed changes

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Outdated Show resolved Hide resolved

filizt reviewed Oct 23, 2020

View reviewed changes

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Show resolved Hide resolved

filizt reviewed Oct 23, 2020

View reviewed changes

src/Resources/ResourceManager/Implementation/ResourceGroups/ExportAzureResourceGroupCmdlet.cs Outdated Show resolved Hide resolved

Xynoclafe added 3 commits October 23, 2020 16:43

Review fixes

cc893be

Merge branch 'xynoclafe/exportTemplateSDK' of https://github.com/Xyno…

0afed3c

…clafe/azure-powershell into xynoclafe/exportTemplateSDK

Merge branch 'master' of https://github.com/Azure/azure-powershell in…

be51822

…to xynoclafe/exportTemplateSDK

isra-fel reviewed Oct 26, 2020

View reviewed changes

isra-fel assigned Xynoclafe Oct 26, 2020

isra-fel added needs-revision and removed needs-review labels Oct 26, 2020

Xynoclafe added 2 commits October 26, 2020 15:11

re-record tests

24e6df4

Merge branch 'master' of https://github.com/Azure/azure-powershell in…

fbcb08b

…to xynoclafe/exportTemplateSDK

isra-fel approved these changes Oct 27, 2020

View reviewed changes

isra-fel merged commit dec1b04 into Azure:master Oct 27, 2020

filizt reviewed Oct 27, 2020

View reviewed changes

Xynoclafe added a commit to Xynoclafe/azure-powershell that referenced this pull request Oct 27, 2020

Follow-up fixes for Azure#13275

7b68dd9

isra-fel pushed a commit that referenced this pull request Oct 29, 2020

Follow-up fixes for #13275 (#13338)

dd2beae

Conversation

Xynoclafe commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Xynoclafe commented Oct 22, 2020

Uh oh!

azure-pipelines bot commented Oct 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filizt Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xynoclafe Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

filizt Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

Xynoclafe commented Oct 26, 2020

Uh oh!

isra-fel left a comment

Choose a reason for hiding this comment

Uh oh!

filizt Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filizt Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filizt commented Oct 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xynoclafe commented Oct 20, 2020 •

edited

Loading

filizt Oct 23, 2020 •

edited

Loading

filizt Oct 27, 2020 •

edited

Loading

filizt Oct 27, 2020 •

edited

Loading