Refactor template deployment cmdlets. #10798

Tiano2017 · 2019-12-28T21:20:57Z

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

…actor # Conflicts: # src/Resources/ResourceManager/SdkClient/ResourceManagerSdkClient.cs

…actor

…actor # Conflicts: # src/Blueprint/Blueprint.Test/Blueprint.Test.csproj # src/Resources/ResourceManager/ResourceManager.csproj # src/Resources/ResourceManager/SdkClient/ResourceManagerSdkClient.cs # src/Resources/Resources.Test/SessionRecords/Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests/TestPolicyDefinitionCRUDAtManagementGroup.json # src/Resources/Resources.Test/SessionRecords/Microsoft.Azure.Commands.Resources.Test.ScenarioTests.PolicyTests/TestPolicyDefinitionMode.json

isra-fel · 2020-01-10T11:47:52Z

For PR big like this, a cmdlet design review is required.

As for the online-version, you can add them manually.

Tiano2017 · 2020-01-10T20:34:01Z

For PR big like this, a cmdlet design review is required.

As for the online-version, you can add them manually.

Created an issue here: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/462

…actor # Conflicts: # src/Resources/Resources/ChangeLog.md

isra-fel · 2020-01-25T11:30:25Z

...Resources/ResourceManager/Implementation/Deployments/GetAzureSubscriptionDeploymentCmdlet.cs

            HelpMessage = "The name of deployment.")]
        [ValidateNotNullOrEmpty]
        public string Name { get; set; }

        [Alias("DeploymentId", "ResourceId")]
-        [Parameter(ParameterSetName = GetAzureDeploymentCmdlet.DeploymentIdParameterSet, Mandatory = false,
+        [Parameter(ParameterSetName = GetAzureSubscriptionDeploymentCmdlet.DeploymentIdParameterSet, Mandatory = true,


Changing an optional parameter to mandatory is a breaking change, and it cannot be checked in until next breaking change release which is ~May.
I agree that it should be mandatory, but my suggestion is to add a breaking change attribute and make the change later.

This is also the cause to CI failure

This shouldn't be breaking. "Id" is the only parameter in this parameter set. If you don't specify the "id" parameter, you'll land with the default parameter set which is "GetByDeploymentName".

I can change it back to "mandatory" to false if that's the right thing..

Right.. then it's a false positive. Please read Debugging StaticAnalysis Errors to suppress the warning.

isra-fel · 2020-01-25T11:32:48Z

src/Resources/Resources/help/Get-AzManagementGroupDeploymentOperation.md

+
+Gets deployment operations with name "Deploy01" at the management group "myMG".
+
+### Example 2: Get a deployment and get its deployment operations


Nice examples!

Tiano2017 · 2020-01-31T07:14:00Z

@isra-fel I've made the changes. can you help take another look?

…actor # Conflicts: # src/Resources/Resources/ChangeLog.md

isra-fel · 2020-02-03T05:56:36Z

@isra-fel I've made the changes. can you help take another look?

Sorry for the late response. I believe you are right about it not being a breaking change. If you could revert the last commit and suppress the warning I would be able to merge. Thanks

Tiano2017 · 2020-02-03T07:24:42Z

@isra-fel so the only issue is the CI run gave an error if I make that change. Is that an bug in the CI run?

isra-fel · 2020-02-03T08:21:47Z

@isra-fel so the only issue is the CI run gave an error if I make that change. Is that an bug in the CI run?

Our CI runs static analysis on the code. One of the checks is for breaking changes. The error was that it detected one (potential) breaking change.

According to your comment it did not break anything after all, so we can suppress the error by following these instructions: https://github.com/Azure/azure-powershell/blob/master/documentation/Debugging-StaticAnalysis-Errors.md#breaking-changes

Tiano2017 · 2020-02-04T02:22:15Z

@isra-fel so the only issue is the CI run gave an error if I make that change. Is that an bug in the CI run?

Our CI runs static analysis on the code. One of the checks is for breaking changes. The error was that it detected one (potential) breaking change.

According to your comment it did not break anything after all, so we can suppress the error by following these instructions: https://github.com/Azure/azure-powershell/blob/master/documentation/Debugging-StaticAnalysis-Errors.md#breaking-changes

updated. can you help take a look and merge?

isra-fel · 2020-02-04T03:17:42Z

/azp run azure-powershell - powershell-core

azure-pipelines · 2020-02-04T03:17:55Z

Azure Pipelines successfully started running 1 pipeline(s).

Refactor template deployment cmdlets.

Tiano2017 added 2 commits December 28, 2019 21:18

Refactor template deployment cmdlets.

7949a24

Merge remote-tracking branch 'Azure/master' into tiano-deployment-ref…

c61d977

…actor # Conflicts: # src/Resources/ResourceManager/SdkClient/ResourceManagerSdkClient.cs

azuresdkci added the needs-review label Dec 29, 2019

isra-fel added needs-revision and removed needs-review labels Dec 30, 2019

isra-fel assigned Tiano2017 Dec 30, 2019

Tiano2017 added 6 commits December 31, 2019 00:30

Fix test failures.

c8bd082

Merge remote-tracking branch 'Azure/master' into tiano-deployment-ref…

bd60d14

…actor

help files.

06ec2f4

more changes.

057e381

update help files.

d841c38

isra-fel added Cmdlet Review Required ⚠️ and removed needs-revision labels Jan 10, 2020

isra-fel added Cmdlet Review Complete needs-review and removed Cmdlet Review Required ⚠️ labels Jan 22, 2020

isra-fel assigned isra-fel and unassigned Tiano2017 Jan 22, 2020

Tiano2017 added 2 commits January 23, 2020 22:43

Merge remote-tracking branch 'Azure/master' into tiano-deployment-ref…

4d279d2

…actor # Conflicts: # src/Resources/Resources/ChangeLog.md

add help links.

87da776

isra-fel requested changes Jan 25, 2020

View reviewed changes

isra-fel added needs-revision and removed needs-review labels Jan 25, 2020

isra-fel assigned Tiano2017 and unassigned isra-fel Jan 25, 2020

change "id" parameter to be mandatory.

fbdfd7e

Tiano2017 removed the needs-revision label Jan 31, 2020

Tiano2017 added the needs-review label Jan 31, 2020

Merge remote-tracking branch 'Azure/master' into tiano-deployment-ref…

5ce0d86

…actor # Conflicts: # src/Resources/Resources/ChangeLog.md

isra-fel added needs-revision and removed needs-review labels Feb 3, 2020

change "id" parameter to be mandatory.

2f6f657

isra-fel approved these changes Feb 4, 2020

View reviewed changes

isra-fel added Waiting for CI and removed needs-revision labels Feb 4, 2020

isra-fel merged commit f904d89 into Azure:master Feb 4, 2020

dingmeng-xue pushed a commit to dingmeng-xue/azure-powershell that referenced this pull request Mar 29, 2020

Merge pull request Azure#10798 from Tiano2017/tiano-deployment-refactor

8ce80f5

Refactor template deployment cmdlets.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor template deployment cmdlets. #10798

Refactor template deployment cmdlets. #10798

Uh oh!

Tiano2017 commented Dec 28, 2019

Uh oh!

isra-fel commented Jan 10, 2020

Uh oh!

Tiano2017 commented Jan 10, 2020

Uh oh!

isra-fel Jan 25, 2020

Uh oh!

isra-fel Jan 25, 2020

Uh oh!

Tiano2017 Jan 26, 2020

Uh oh!

Tiano2017 Jan 26, 2020

Uh oh!

isra-fel Feb 3, 2020

Uh oh!

isra-fel Jan 25, 2020

Uh oh!

Tiano2017 commented Jan 31, 2020

Uh oh!

isra-fel commented Feb 3, 2020

Uh oh!

Tiano2017 commented Feb 3, 2020 •

edited

Loading

Uh oh!

isra-fel commented Feb 3, 2020

Uh oh!

Tiano2017 commented Feb 4, 2020

Uh oh!

isra-fel commented Feb 4, 2020

Uh oh!

azure-pipelines bot commented Feb 4, 2020

Uh oh!

Uh oh!


		Gets deployment operations with name "Deploy01" at the management group "myMG".

		### Example 2: Get a deployment and get its deployment operations

Refactor template deployment cmdlets. #10798

Refactor template deployment cmdlets. #10798

Uh oh!

Conversation

Tiano2017 commented Dec 28, 2019

Description

Checklist

Uh oh!

isra-fel commented Jan 10, 2020

Uh oh!

Tiano2017 commented Jan 10, 2020

Uh oh!

isra-fel Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

isra-fel Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Tiano2017 Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Tiano2017 Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

isra-fel Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

isra-fel Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Tiano2017 commented Jan 31, 2020

Uh oh!

isra-fel commented Feb 3, 2020

Uh oh!

Tiano2017 commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isra-fel commented Feb 3, 2020

Uh oh!

Tiano2017 commented Feb 4, 2020

Uh oh!

isra-fel commented Feb 4, 2020

Uh oh!

azure-pipelines bot commented Feb 4, 2020

Uh oh!

Uh oh!

Tiano2017 commented Feb 3, 2020 •

edited

Loading