Machine Learning Compute: Add -IncludeAllResources Paramter for Remove cmdlet #5206

shutchings · 2018-01-02T17:09:27Z

Description

Added a parameter to include all resources when removing an operationalization cluster.

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

shutchings · 2018-01-02T17:10:51Z

@maddieclayton I've started this PR so that you can take a look at the code changes while I'm waiting for the .NET SDK PR to be completed and the new package to be created. Once that's done I'll remove the local nuget package I added and make sure the build works.

maddieclayton

A few initial comments. Please let me know when the SDK changes have been merged and the PR is updated. Also, please be sure to resolve the merge conflict that has come up.

maddieclayton · 2018-01-02T19:23:22Z

...r/MachineLearningCompute/Commands.MachineLearningCompute/Cmdlets/RemoveAzureRmMlOpCluster.cs

+                var clusterToDelete = MachineLearningComputeManagementClient.OperationalizationClusters.Get(ResourceGroupName, Name);
+                var managedByResourceGroup = new ResourceIdentifier(clusterToDelete.ContainerRegistry.ResourceId).ResourceGroupName;
+
+                shouldProcessMessage += $" and supporting resource group {managedByResourceGroup}";


Is the supporting resource group the only additional resource that will be deleted when -IncludeAllResources is specified?

Yes it deletes that cluster and the supporting resource group only. The supporting resource group has a bunch of resources in it, I could change it to list them all out but it will include things like network security groups, nics, vms, disks, etc. It seemed too verbose to list all those resources individually so I think just warning that that resource group will be deleted should be enough.

I see, I agree that this message would make the most sense. Maybe it would be useful to also add in the message that all resources in the supporting resource group will be deleted.

Good suggestion.

maddieclayton · 2018-01-02T19:24:06Z

...achineLearningCompute/Commands.MachineLearningCompute/Commands.MachineLearningCompute.csproj

    </Reference>
+    <Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />


Remove these three references: they are already imported by the common dependencies target.

maddieclayton · 2018-01-02T19:24:31Z

src/ResourceManager/MachineLearningCompute/Commands.MachineLearningCompute/packages.config

@@ -1,4 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <packages>
-  <package id="Microsoft.Azure.Management.MachineLearningCompute" version="0.2.0" targetFramework="net452" />
+  <package id="Microsoft.Azure.Management.MachineLearningCompute" version="0.3.0" targetFramework="net452" />
+  <package id="Microsoft.Rest.ClientRuntime" version="2.3.10" targetFramework="net452" />


Same comment about removing the three duplicate references.

maddieclayton · 2018-01-02T19:25:50Z

...nager/MachineLearningCompute/Commands.MachineLearningCompute.Test/ScenarioTests/MLCTests.ps1

+	Remove-AzureRmMlOpCluster -ResourceGroupName $resourceGroupName -Name $clusterName -IncludeAllResources
+
+    Assert-Throws ( Get-AzureRmResourceGroup -ResourceGroupName $managedByResourceGroupName )


Why does this throw? Shouldn't it complete the operation successfully and delete both the managed resource group and the cluster? And if it is supposed to throw, I would think you'd want another test with the command running successfully.

This throws because after a successful remove call with -IncludeAllResources the resource group I'm trying to get should not exist. So by expecting the throw I can verify that the -IncludeAllResources worked correctly.

Ah, that makes sense - I did not expect that a Get would throw when the resource is not found, but if that is the behavior then this test makes sense. Thanks.

maddieclayton · 2018-01-03T03:03:29Z

@shutchings I'll take a look once the merge conflict has been resolved - please feel free to assign the PR back to me once this is done and you are ready for me to take a look.

shutchings · 2018-01-04T18:50:23Z

@maddieclayton I fixed the merge conflict and removed the local package since I got the latest one published to nuget. From the looks of it it's failing static analysis and should regen the installer. Is that correct?

maddieclayton · 2018-01-04T19:02:23Z

@shutchings The issue is that the parameter you added is plural (which is okay since it is a SwitchParameter). Please copy the message (the issue with severity 1) from SignatureIssues.csv (found in the build) in breakingchanges.csv. This should resolve the issue, please let me know if you have any more questions.

shutchings · 2018-01-04T20:54:23Z

@maddieclayton I added in the exception but it is still failing. The other issues were severity 2 so I assume they aren't failing the build. Is there something else missing?

maddieclayton · 2018-01-04T21:05:04Z

@shutchings No, the issue is still the plural parameter name. Can you try following the format of the lines above Microsoft.Azure.Commands.Compute:

"d:\workspace\powershell\src\Package\Debug\ResourceManager\AzureResourceManager\AzureRM.Sql\Microsoft.Azure.Commands.Sql.dll","Microsoft.Azure.Commands.Sql.Database.Cmdlet.SetAzureSqlDatabase","Set-AzureRmSqlDatabase","0","2100","The parameter 'Tags' in cmdlet 'Set-AzureRmSqlDatabase' is no longer in the parameter set '__AllParameterSets'.","Add parameter 'Tags' back to the parameter set '__AllParameterSets'."

It looks like the format you used only works for compute for reason - if this does not fix the issue I'll look into it further.

shutchings · 2018-01-04T22:24:39Z

@maddieclayton I think I have it in the right format now but it's still giving issues.

maddieclayton · 2018-01-04T22:35:07Z

@shutchings Oh no, this is my fault. It should go in tool/Exception/SignatureIssues.csv, not breakingchanges. I apologize for the confusion, but that should work. Don't use the new format that you switched to, just copy paste again into the csv file.

shutchings · 2018-01-04T23:55:03Z

@maddieclayton Yep, that did it!

maddieclayton · 2018-01-04T23:58:56Z

@shutchings Great! If you are ready for another review please assign the PR back to me.

maddieclayton

Two very small comments then looks good to me.

maddieclayton · 2018-01-05T19:06:21Z

src/ResourceManager/MachineLearningCompute/ChangeLog.md

+* Add IncludeAllResources parameter to Remove-AzureRmMlOpCluster cmdlet
+    - Using this switch parameter will remove all resources that were created with the cluster originally
+
+## Version 0.3.1


Don't modify this - just add your snippet after the snippets about ResourceGroup and Location completers. We will automatically modify this file during release.

That makes sense. I looked at the version number in the .psd1 file and it shows 0.3.1 which is below the current release so that's why I thought the version numbers are off. But maybe I'm misunderstanding? Is it that the bullets below current release are slated for the next version to be released?

"Current release" actually means the upcoming release: it will be displayed as the current release notes in the next release.

maddieclayton · 2018-01-05T19:09:56Z

...r/MachineLearningCompute/Commands.MachineLearningCompute/Cmdlets/RemoveAzureRmMlOpCluster.cs

@@ -58,26 +58,47 @@ public class RemoveAzureRmMlOpCluster : MachineLearningComputeCmdletBase
            HelpMessage = ResourceIdParameterHelpMessage)]
        public string ResourceId { get; set; }

+        [Parameter(ParameterSetName = ObjectParameterSet,


If the parameter is in all of the parameter sets you don't need a parameter attribute for each parameter set, all you need is:

[Parameter(Mandatory = false, HelpMessage = IncludeAllResourcesParameterHelpMessage)] public SwitchParameter IncludeAllResources { get; set; }

Good to know!

maddieclayton · 2018-01-05T19:45:43Z

on demand run here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/323/

shutchings · 2018-01-05T22:38:31Z

@maddieclayton Is it possible to start a package job?

maddieclayton · 2018-01-05T22:41:12Z

Yup, here is it:https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/posh-pack/42/. Let me know when you are ready to merge.

maddieclayton · 2018-01-08T17:26:30Z

@shutchings is this ready to merge? I wasn't sure if you were running testing on the package job or needed it for another reason.

shutchings · 2018-01-08T18:39:21Z

@maddieclayton I found a typo in the parameter name in the help document while testing the package job. Must have been from when I regenerated the help and then made changes to the parameter name. After this build feel free to merge it.

shutchings assigned maddieclayton Jan 2, 2018

azuresdkci added the needs-review label Jan 2, 2018

maddieclayton suggested changes Jan 2, 2018

View reviewed changes

maddieclayton assigned shutchings and unassigned maddieclayton Jan 2, 2018

maddieclayton added Merge Conflicts needs-revision and removed needs-review labels Jan 2, 2018

shutchings added 5 commits January 3, 2018 08:36

Add IncludeAllResources for remove cmdlet

3cf2e88

Add local package while waiting for .net pr to be merged

022d79d

Remove duplicate package references

22d18ba

Make remove should process message more explicit

129f3a0

Update changelog

d051479

shutchings force-pushed the preview branch from 482f216 to d051479 Compare January 3, 2018 16:37

maddieclayton removed the Merge Conflicts label Jan 3, 2018

Remove local package

e08231a

shutchings force-pushed the preview branch from 64fc876 to e08231a Compare January 4, 2018 16:46

shutchings assigned maddieclayton Jan 4, 2018

maddieclayton removed their assignment Jan 4, 2018

shutchings force-pushed the preview branch from f90607c to 3135aca Compare January 4, 2018 21:11

Update breaking change exceptions for plural switch parameter

e683b93

shutchings force-pushed the preview branch from 3135aca to e683b93 Compare January 4, 2018 22:39

shutchings assigned maddieclayton Jan 5, 2018

maddieclayton added needs-review and removed needs-revision labels Jan 5, 2018

maddieclayton unassigned shutchings Jan 5, 2018

maddieclayton reviewed Jan 5, 2018

View reviewed changes

maddieclayton assigned shutchings and unassigned maddieclayton Jan 5, 2018

maddieclayton added needs-revision and removed needs-review labels Jan 5, 2018

Use single parameter set attribute for parameter in all sets

564b1dd

maddieclayton added Waiting for On Demand and removed needs-revision labels Jan 5, 2018

maddieclayton unassigned shutchings Jan 5, 2018

maddieclayton previously approved these changes Jan 5, 2018

View reviewed changes

Fix typo in help document

e0a9dcc

shutchings dismissed maddieclayton’s stale review via e0a9dcc January 8, 2018 18:37

maddieclayton approved these changes Jan 8, 2018

View reviewed changes

maddieclayton merged commit fe1c530 into Azure:preview Jan 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Machine Learning Compute: Add -IncludeAllResources Paramter for Remove cmdlet #5206

Machine Learning Compute: Add -IncludeAllResources Paramter for Remove cmdlet #5206

shutchings commented Jan 2, 2018 •

edited

Loading

shutchings commented Jan 2, 2018

maddieclayton left a comment

maddieclayton Jan 2, 2018

shutchings Jan 2, 2018

maddieclayton Jan 2, 2018

shutchings Jan 2, 2018

maddieclayton Jan 2, 2018

maddieclayton Jan 2, 2018

maddieclayton Jan 2, 2018

shutchings Jan 2, 2018

maddieclayton Jan 2, 2018 •

edited

Loading

maddieclayton commented Jan 3, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

maddieclayton left a comment

maddieclayton Jan 5, 2018

shutchings Jan 5, 2018 •

edited

Loading

maddieclayton Jan 5, 2018

maddieclayton Jan 5, 2018

shutchings Jan 5, 2018

maddieclayton commented Jan 5, 2018 •

edited

Loading

shutchings commented Jan 5, 2018

maddieclayton commented Jan 5, 2018

maddieclayton commented Jan 8, 2018

shutchings commented Jan 8, 2018

		</Reference>
		<Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL" />

		Remove-AzureRmMlOpCluster -ResourceGroupName $resourceGroupName -Name $clusterName -IncludeAllResources

		Assert-Throws ( Get-AzureRmResourceGroup -ResourceGroupName $managedByResourceGroupName )

Machine Learning Compute: Add -IncludeAllResources Paramter for Remove cmdlet #5206

Machine Learning Compute: Add -IncludeAllResources Paramter for Remove cmdlet #5206

Conversation

shutchings commented Jan 2, 2018 • edited Loading

Description

shutchings commented Jan 2, 2018

maddieclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddieclayton Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

maddieclayton commented Jan 3, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

shutchings commented Jan 4, 2018

maddieclayton commented Jan 4, 2018

maddieclayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shutchings Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maddieclayton commented Jan 5, 2018 • edited Loading

shutchings commented Jan 5, 2018

maddieclayton commented Jan 5, 2018

maddieclayton commented Jan 8, 2018

shutchings commented Jan 8, 2018

shutchings commented Jan 2, 2018 •

edited

Loading

maddieclayton Jan 2, 2018 •

edited

Loading

shutchings Jan 5, 2018 •

edited

Loading

maddieclayton commented Jan 5, 2018 •

edited

Loading