Support new cmdlets for subscription level deployment #6516

Tiano2017 · 2018-06-22T04:26:22Z

Description

This PR adds new cmdlets for subscription level deployment.

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review
- the cmdlet markdown files were generated using the platyPS module

cormacpayne · 2018-07-03T18:07:12Z

@Tiano2017 when you're ready for someone from our side to review this PR, please remove the Do Not Merge and needs-revision labels and add the needs-review label to the PR

cormacpayne · 2018-07-09T21:13:21Z

Corresponding cmdlet design review: https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/102

vladimir-shcherbakov · 2018-07-19T21:28:58Z

@Tiano2017
Is it possible for you to re-base you commits to have only essential ones in the PR?
A possible way to do this:

git remote add upstream https://github.com/Azure/azure-powershell.git
git fetch upstream
git merge upstream/preview
git reset --soft upstream/preview
<commit your changes>
git push --force

Tiano2017 · 2018-07-19T22:20:54Z

@vladimir-shcherbakov sure I can do that, but should I do that after all issues are resolved? right now I still have CI failures which I'm trying to resolve.

vladimir-shcherbakov · 2018-07-19T22:57:14Z

@Tiano2017
I think this would help you in resolving of ci-build issues.

Tiano2017 · 2018-07-20T06:11:57Z

did the rebase. seems still got the failure.

vladimir-shcherbakov · 2018-07-20T06:44:24Z

https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/6023/artifact/src/Package/ValidateHelpIssues.csv/*view*/

Tiano2017 · 2018-07-20T19:11:47Z

@vladimir-shcherbakov I've fixed the error on help files. Can you help take another look?

vladimir-shcherbakov · 2018-07-20T21:06:18Z

@Tiano2017
You have a bunch of failing tests: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/6026/artifact/src/Publish/TestResults/FailingTests/

Reason to fail for most of them is Unable to find a matching HTTP request for URL .. . That means these tests need to be re-recorded

Tiano2017 · 2018-08-03T05:26:00Z

@cormacpayne can you please take another look?

cormacpayne · 2018-08-03T23:26:53Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/GetAzureDeploymentCmdlet.cs

+
+        [Alias("DeploymentName")]
+        [Parameter(Position = 0, ParameterSetName = GetAzureDeploymentCmdlet.DeploymentNameParameterSet, Mandatory = false,
+            ValueFromPipelineByPropertyName = true, HelpMessage = "The name of deployment.")]


@Tiano2017 please remove the ValueFromPipelineByPropertyName property from the -Name parameter and move it to the -ResourceId parameter so users can do the following:

Get-AzureRmResource -ResourceType Microsoft.Resources/deployments | Get-AzureRmDeployment

we need it on the Name parameter for some other pipeline scenarios. As for Get-AzureRmResource, I don't think it returns "deployments" as resources. Strictly speaking, "deployment" is not a resource, although we implement the APIs to be consistent with resource APIs.

@Tiano2017 this is deprecated - Piping by value is now the recommendation - if you can pipe the object associated with the cmdlet noun into the cmdlet no by-property-name piping (other than piping resourceid by property name) should be used.

Sorry Mark.. I'm not quite getting the point. Are you recommending just removing this piping support?

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

As replied in the other comment, Get-AzureRmResource doesn't return deployments, as deployment is not a resource..

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

The ValueFromPipelineByPropertyName setting is still here. Remove. If you want to support piping from something, use a parameter set with a PSDeployment INputObject. In this case, you may not need piping.

Also note - you should consider having a single parameter that would accept either Name or Id - then the user could pass in a fully qualified name or short name and you wouldn't need any parameter set.

removed.

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

We have these two parameters for Get-AzureRmResourceGroupDeployment. I think we can keep them to be consistent.

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

cormacpayne · 2018-08-03T23:29:16Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/NewAzureDeploymentCmdlet.cs

+        public string Location { get; set; }
+
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The deployment debug log level.")]
+        [ValidateSet("RequestContent", "ResponseContent", "All", "None", IgnoreCase = true)]


@Tiano2017 we recommend using PSArgumentCompleter since it allows for potential additions in the future and users don't notice a difference with the tab-completion

cormacpayne · 2018-08-03T23:29:45Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/NewAzureDeploymentCmdlet.cs

+            {
+                Location = Location,
+                DeploymentName = Name,
+                DeploymentMode = DeploymentMode.Incremental,


@Tiano2017 got it, thanks for clarifying

cormacpayne · 2018-08-03T23:30:52Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/NewAzureDeploymentCmdlet.cs

+            {
+                WriteWarning(ProjectResources.WarnOnDeploymentDebugSetting);
+            }
+            WriteObject(ResourceManagerSdkClient.ExecuteDeploymentAtSubscriptionScope(parameters));


@Tiano2017 you do it below for your Save-AzureRmDeployment cmdlet 😀

cormacpayne · 2018-08-03T23:33:24Z

...s/Commands.ResourceManager/Cmdlets/Implementation/Deployments/RemoveAzureDeploymentCmdlet.cs

+        public string Name { get; set; }
+
+        [Alias("DeploymentId", "ResourceId")]
+        [Parameter(ParameterSetName = RemoveAzureDeploymentCmdlet.DeploymentIdParameterSet, Mandatory = true,


@Tiano2017 the result of Get-AzureRmDeployment will be bound to the -InputObject parameter. We want to enable the scenario (as mentioned above for Get-AzureRmDeployment) where the user can pipe the result of Get-AzureRmResource to a cmdlet and have the -ResourceId bound, which is enable when you add the ValueFromPipelineByPropertyName property to this parameter and remove it from the -Name parameter.

cormacpayne · 2018-08-03T23:34:31Z

...s/Commands.ResourceManager/Cmdlets/Implementation/Deployments/RemoveAzureDeploymentCmdlet.cs

+        [Parameter(ParameterSetName = RemoveAzureDeploymentCmdlet.InputObjectParameterSet, Mandatory = true, 
+            ValueFromPipeline = true, HelpMessage = "The deployment object.")]
+        public PSDeployment InputObject { get; set; }
+


@Tiano2017 is the underlying operation long-running? If so, you might as well add the -AsJob parameter to allow users to run this cmdlet in the background (at no cost 😉)

cormacpayne · 2018-08-03T23:36:21Z

...ands.ResourceManager/Cmdlets/Implementation/Deployments/SaveAzureDeploymentTemplateCmdlet.cs

+                        string.IsNullOrEmpty(this.Path)
+                            ? System.IO.Path.Combine(CurrentPath(), deploymentName)
+                            : this.TryResolvePath(this.Path),
+                    overwrite: this.Force,


@Tiano2017 got it, thanks for clarifying

cormacpayne · 2018-08-03T23:46:52Z

...ands.ResourceManager/Cmdlets/Implementation/Deployments/SaveAzureDeploymentTemplateCmdlet.cs

+    /// Saves the deployment template to a file on disk.
+    /// </summary>
+    [Cmdlet(VerbsData.Save, "AzureRmDeploymentTemplate", SupportsShouldProcess = true,
+        DefaultParameterSetName = SaveAzureDeploymentTemplateCmdlet.DeploymentNameParameterSet), OutputType(typeof(PSObject))]


@Tiano2017 even if we are returning the file path, we should still have a concrete type with that path as one of its properties -- we are now generating docs for our types, which lets the user know what properties and methods are exposed on parameter and output types, and it is also best practice to use concrete .NET types in PowerShell for consistency in returned objects

cormacpayne · 2018-08-03T23:47:53Z

...ces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/StopAzureDeploymentCmdlet.cs

+    /// <summary>
+    /// Cancel a running deployment.
+    /// </summary>
+    [Cmdlet(VerbsLifecycle.Stop, "AzureRmDeployment", SupportsShouldProcess = true,


@Tiano2017 got it. Maybe we should think about adding an alias for New-AzureRmDeployment that is Start-AzureRmDeployment? Something to think about

cormacpayne · 2018-08-03T23:48:18Z

...ces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/StopAzureDeploymentCmdlet.cs

+        public string Name { get; set; }
+
+        [Alias("DeploymentId", "ResourceId")]
+        [Parameter(ParameterSetName = StopAzureDeploymentCmdlet.DeploymentIdParameterSet, Mandatory = true,


@Tiano2017 same comment as above 😀

vladimir-shcherbakov · 2018-08-07T19:12:58Z

...sourceManager/Resources/Commands.Resources.Test/Models.ResourceGroups/ResourceClientTests.cs

@@ -1391,13 +1391,13 @@ public void CancelsActiveDeployment()
        public void SerializeHashtableProperlyHandlesAllDataTypes()
        {
            Hashtable hashtable = new Hashtable();
-            var password = "pass";
+            var pass = "pass";
            hashtable.Add("key1", "string");


@Tiano2017
The right way to avoid false positives matches is to use inline-suppressions.

This one should be fine. It's a random variable, and doesn't have to be named "password".

markcowl

I have makred those places where there are still necessary changes.

markcowl · 2018-08-08T00:43:16Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/GetAzureDeploymentCmdlet.cs

+
+        [Alias("DeploymentName")]
+        [Parameter(Position = 0, ParameterSetName = GetAzureDeploymentCmdlet.DeploymentNameParameterSet, Mandatory = false,
+            ValueFromPipelineByPropertyName = true, HelpMessage = "The name of deployment.")]


The ValueFromPipelineByPropertyName setting is still here. Remove. If you want to support piping from something, use a parameter set with a PSDeployment INputObject. In this case, you may not need piping.

markcowl · 2018-08-08T00:45:10Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/GetAzureDeploymentCmdlet.cs

+
+        [Alias("DeploymentName")]
+        [Parameter(Position = 0, ParameterSetName = GetAzureDeploymentCmdlet.DeploymentNameParameterSet, Mandatory = false,
+            ValueFromPipelineByPropertyName = true, HelpMessage = "The name of deployment.")]


Also note - you should consider having a single parameter that would accept either Name or Id - then the user could pass in a fully qualified name or short name and you wouldn't need any parameter set.

markcowl · 2018-08-08T00:45:58Z

...ands.ResourceManager/Cmdlets/Implementation/Deployments/GetAzureDeploymentOperationCmdlet.cs

+        /// <summary>
+        /// Gets or sets the deployment name parameter.
+        /// </summary>
+        [Parameter(Mandatory = true, ValueFromPipelineByPropertyName = true, HelpMessage = "The deployment name.")]


As discussed, these ValueFromPipelineByPropertyName settings should be removed.

Removed. And added DepoymentObject parameter for piping scenario.

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

markcowl · 2018-08-08T00:46:14Z

...rces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/NewAzureDeploymentCmdlet.cs

+    public class NewAzureDeploymentCmdlet : ResourceWithParameterCmdletBase, IDynamicParameters
+    {
+        [Alias("DeploymentName")]
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true,


same comment

markcowl · 2018-08-08T00:49:16Z

...s/Commands.ResourceManager/Cmdlets/Implementation/Deployments/RemoveAzureDeploymentCmdlet.cs

+                        : !string.IsNullOrEmpty(this.Id) ? ResourceIdUtility.GetResourceName(this.Id) : this.InputObject.DeploymentName;
+
+                    ResourceManagerSdkClient.DeleteDeploymentAtSubscriptionScope(deploymentName);
+                    WriteObject(true);


This is not a good reason to continue a non-conforming pattern. You should return nothing on success, unless the user supplies -PassThru

markcowl · 2018-08-08T01:59:43Z

...ands.ResourceManager/Cmdlets/Implementation/Deployments/SaveAzureDeploymentTemplateCmdlet.cs

+        /// <summary>
+        /// Gets or sets the file path.
+        /// </summary>
+        [Parameter(Mandatory = false, ValueFromPipelineByPropertyName = true, HelpMessage = "The output path of the template file.")]


same issue, remove the ValueFromPipelineByPropertyName setting for this parameter

markcowl · 2018-08-08T02:01:26Z

...ces/Commands.ResourceManager/Cmdlets/Implementation/Deployments/StopAzureDeploymentCmdlet.cs

+                this.Name,
+                () => ResourceManagerSdkClient.CancelDeploymentAtSubscriptionScope(deploymentName));
+
+            WriteObject(true);


No, we need to make this meet the design guidelines now.

Marked those places that still need to change

Tiano2017 · 2018-08-08T22:11:07Z

CI job is passing now. @markcowl please let me know if there's more changes required.

azuresdkci added the needs-review label Jun 22, 2018

maddieclayton added the Do Not Merge 🚫 label Jun 25, 2018

cormacpayne assigned Tiano2017 Jul 3, 2018

cormacpayne added needs-revision and removed needs-review labels Jul 3, 2018

Tiano2017 changed the title ~~<Do Not Merge>Support new cmdlets for subscription level deployment~~ Support new cmdlets for subscription level deployment Jul 5, 2018

Tiano2017 added needs-review and removed Do Not Merge 🚫 needs-revision labels Jul 5, 2018

maddieclayton added Merge Conflicts Cmdlet Review Required ⚠️ labels Jul 14, 2018

vladimir-shcherbakov added Cmdlet Review Complete and removed Cmdlet Review Required ⚠️ Merge Conflicts labels Jul 19, 2018

maddieclayton assigned vladimir-shcherbakov Jul 19, 2018

vladimir-shcherbakov added needs-revision and removed needs-review labels Jul 19, 2018

Support subscription level deployment cmdlets.

3f7e84b

Tiano2017 force-pushed the preview branch from 814bd92 to 3f7e84b Compare July 20, 2018 05:38

Fix trailing yaml.

c83acce

vladimir-shcherbakov closed this Jul 20, 2018

vladimir-shcherbakov previously approved these changes Aug 2, 2018

View reviewed changes

vladimir-shcherbakov assigned cormacpayne and unassigned Tiano2017 Aug 2, 2018

vladimir-shcherbakov added needs-review and removed needs-revision labels Aug 2, 2018

cormacpayne previously requested changes Aug 3, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Aug 3, 2018

cormacpayne assigned Tiano2017 and unassigned cormacpayne Aug 3, 2018

Resolve comments.

080880e

Tiano2017 dismissed vladimir-shcherbakov’s stale review via 080880e August 6, 2018 07:37

Tiano2017 added 4 commits August 6, 2018 18:16

Merge remote-tracking branch 'Azure/preview' into preview

ea9a1bf

Remove "ValueFromPipelineByPropertyName" on deploymentName parameters.

fb831ff

Merge remote-tracking branch 'Azure/preview' into preview

578e8c7

CredScan issue.

4709008

vladimir-shcherbakov reviewed Aug 7, 2018

View reviewed changes

markcowl changed the base branch from preview to release-2018-08-10 August 7, 2018 23:10

Merge branch 'release-2018-08-10' into preview

94f193f

markcowl added the Sprint Candidate label Aug 8, 2018

markcowl requested changes Aug 8, 2018

View reviewed changes

Tiano2017 added 2 commits August 8, 2018 05:50

Resolve comments.

97cbc02

set default parameter set for Get-AzureRmDeploymentOperation.

4af0d76

Tiano2017 added 2 commits August 9, 2018 00:13

Add PassThru parameters for stop and remove cmdlets.

bf5b36a

Merge remote-tracking branch 'Azure/release-2018-08-10' into preview

16fcfe8

markcowl approved these changes Aug 9, 2018

View reviewed changes

markcowl merged commit 192c1f0 into Azure:release-2018-08-10 Aug 9, 2018

Support new cmdlets for subscription level deployment #6516

Support new cmdlets for subscription level deployment #6516

Uh oh!

Conversation

Tiano2017 commented Jun 22, 2018

Description

Checklist

Uh oh!

cormacpayne commented Jul 3, 2018

Uh oh!

cormacpayne commented Jul 9, 2018

Uh oh!

vladimir-shcherbakov commented Jul 19, 2018

Uh oh!

Tiano2017 commented Jul 19, 2018

Uh oh!

vladimir-shcherbakov commented Jul 19, 2018

Uh oh!

Tiano2017 commented Jul 20, 2018

Uh oh!

vladimir-shcherbakov commented Jul 20, 2018

Uh oh!

Tiano2017 commented Jul 20, 2018

Uh oh!

vladimir-shcherbakov commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tiano2017 commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimir-shcherbakov Aug 7, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

markcowl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladimir-shcherbakov commented Jul 20, 2018 •

edited

Loading

vladimir-shcherbakov Aug 7, 2018 •

edited

Loading