[AzureRT] December release #3263

hyonholee · 2016-12-07T23:42:09Z

Add "Force" parameter to Remove cmdlets (Breaking change)
Remove unnecessary (redundant) properties from VM object.
Add DisplayHint property to VM object to enable Compact and Expand display modes
Add "Remove-AzureRmVMSecret" cmdlet

Description

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 PassThrough 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.

…nto dev

Add VmId to PS VM object. Remove-AzureRmVMNetworkInterface and Remove-AzureRmVMDataDisk removes all if NetworkInterfaceIDs and DataDiskNames parameters are not given, respectively.

cormacpayne · 2016-12-07T23:51:04Z

...te/Generated/ContainerService/Config/RemoveAzureRmContainerServiceAgentPoolProfileCommand.cs

@@ -42,6 +42,7 @@ public class RemoveAzureRmContainerServiceAgentPoolProfileCommand : Microsoft.Az
        [Parameter(
            Mandatory = true,
            Position = 1,
+            ParameterSetName = "NameParameterSet",


@hyonholee why was this parameter set added?

We generated these cmdlets. For Removing there are can be more than one way to removing (for example, removing with a name and removing with a id), so we added parameter set name for each removing way.

If this is concerned, I will update our generator and remove parameter set when there is only one way to remove.

I was just wondering why it was added. As long as the scenario where only the ContainerService parameter is provided is covered, this will be fine

cormacpayne · 2016-12-07T23:52:32Z

...ommands.Compute/Generated/VirtualMachineScaleSet/Config/RemoveAzureRmVmssExtensionCommand.cs

@@ -42,9 +42,17 @@ public class RemoveAzureRmVmssExtensionCommand : Microsoft.Azure.Commands.Resour
        [Parameter(
            Mandatory = true,
            Position = 1,
+            ParameterSetName = "NameParameterSet",


@hyonholee same comment about parameter set

cormacpayne · 2016-12-07T23:52:54Z

...rated/VirtualMachineScaleSet/Config/RemoveAzureRmVmssNetworkInterfaceConfigurationCommand.cs

@@ -42,9 +42,17 @@ public class RemoveAzureRmVmssNetworkInterfaceConfigurationCommand : Microsoft.A
        [Parameter(
            Mandatory = true,
            Position = 1,
+            ParameterSetName = "NameParameterSet",


@hyonholee same question about parameter set

cormacpayne · 2016-12-07T23:53:35Z

src/ResourceManager/Compute/Commands.Compute/Models/PSVirtualMachine.cs

@@ -88,34 +91,7 @@ public string ResourceGroupName
        // Gets or sets the storage profile.
        public StorageProfile StorageProfile { get; set; }

-        [JsonIgnore]


@hyonholee why were these properties removed?

This is unnecessary. This property is just a copy of a nested property, which was used for piping scenario, but now the piping scenario works without these properties. There was a complain about these redundant property.

There are numerous cmdlets that output an object of type PSVirtualMachine, and existing scripts that use this object will no longer be able to use these properties, which means it's a breaking change.

Can we announce the breaking change? Do you think it is serious? These properties are not included in the VM response at the top level. They are nested properties and we added these redundant properties at the top level just for pipeline.

@singhkays

@hyonholee Not sure this release is setup to be a breaking change. Can we leave this in the object until next time we have a breaking change release?

@cormacpayne What do we need to do for a breaking change release?

cormacpayne · 2016-12-07T23:54:12Z

...sourceManager/Compute/Commands.Compute/VirtualMachine/Config/RemoveAzureVMDataDiskCommand.cs

            Position = 1,
-            ValueFromPipelineByPropertyName = true,


@hyonholee why did you remove this attribute? Scripts that get DataDiskNames from the pipeline will no longer work

Pipeline works because I also updated Remove-AzureRmVMDataDisk. Please look at Test-VirtualMachineProfile test.

Actually there is an issue with pipeline with Remove-AzureRmVMDataDisk (and other Remove command that removes a property from VM object.) Since this parameteter (DataDiskNames) have an alias "Name" and VM object has "Name" property for VM Name, in piping scenario (e.g. Get-AzureRmVM | Remove-AzureRmVMDataDisk ), VM name is piped to Remove-AzureRmVMDataDisk as a data disk name, and it tries to remove a data disk named VM name. Setting ValueFromPipelineByPropertyName to false removes this issue. Also we remove "DataDiskNames" (which is redundant property) from VM object, DataDiskNames are not piped from VM object to Remove-AzureRmVM. Remove-AzureRmVM is also modified when DataDiskNames are not provided, it interprets it as removing all existing data disks. So the command does not change the behavior of Get-AzureRmVM | Remove-AzureRmVM .

cormacpayne · 2016-12-07T23:54:34Z

...nager/Compute/Commands.Compute/VirtualMachine/Config/RemoveAzureVMNetworkInterfaceCommand.cs

            Position = 1,
-            ValueFromPipelineByPropertyName = true,


@hyonholee same comment about removing this attribute

Same issue with Remove-AzureRmVMDataDisk

cormacpayne · 2016-12-07T23:55:08Z

...ResourceManager/Compute/Commands.Compute/VirtualMachine/Config/RemoveAzureVMSecretCommand.cs

+        ProfileNouns.VaultSecretGroup),
+    OutputType(
+        typeof(PSVirtualMachine))]
+    public class RemoveAzureVMSecretCommand : Microsoft.Azure.Commands.ResourceManager.Common.AzureRMCmdlet


@hyonholee since this cmdlet makes changes, it should implement SupportsShouldProcess

This cmdlet only updates a local VM object and does not actually remove the secret from VM, until Update-AzureRmVM is performed with the updated VM object. Other Remove cmdlets that change only local object do not have ShouldProcess or Force parameter

cormacpayne · 2016-12-08T00:22:19Z

@hyonholee the build is failing because there is no help for the new cmdlet Remove-AzureRmVMSecret, and the same cmdlet does not implement SupportsShouldProcess

hyonholee · 2016-12-08T01:04:23Z

I will update the help file. Thanks for comments.

…NetworkInterface and Remove-AzureRmVMDataDisk cmdlets

…hell into dev

…g change.

cormacpayne · 2016-12-09T19:14:17Z

LGTM once on demand build passes

http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1343/

hyonholee added 6 commits November 18, 2016 17:08

Generate cmdlets with adding -Force parameter for Remove cmdlets

967ab2f

Merge branch 'dev' of https://github.com/hyonholee/azure-powershell i…

b329e1c

…nto dev

Remove NetworkInterfaceIDs and DataDiskNames from PS VM object.

1abc9d8

Add VmId to PS VM object. Remove-AzureRmVMNetworkInterface and Remove-AzureRmVMDataDisk removes all if NetworkInterfaceIDs and DataDiskNames parameters are not given, respectively.

Merge branch 'dev' of https://github.com/Azure/azure-powershell into dev

c9861cf

Add Remove-AzureRmVMSecret command

5e5d0b4

Add DisplayHint property to VM object

7cb9746

azurecla added the cla-not-required label Dec 7, 2016

cormacpayne suggested changes Dec 7, 2016

View reviewed changes

hyonholee added 3 commits December 7, 2016 20:04

Update output format for array

42a26a9

Update Compute help file

4298129

Add ShouldProcess support to Remove-AzureRmVMSecret, Remove-AzureRmVM…

f6305d2

…NetworkInterface and Remove-AzureRmVMDataDisk cmdlets

hyonholee changed the base branch from dev to release-3.3.0 December 9, 2016 01:08

hyonholee added 3 commits December 8, 2016 17:09

Merge branch 'release-3.3.0' of https://github.com/Azure/azure-powers…

eec5d00

…hell into dev

Revert breaking changes and add warning messages for upcoming breakin…

4262678

…g change.

Update change log of Compute

40c778b

cormacpayne approved these changes Dec 9, 2016

View reviewed changes

cormacpayne merged commit b841e45 into Azure:release-3.3.0 Dec 9, 2016

[AzureRT] December release #3263

[AzureRT] December release #3263

Uh oh!

Conversation

hyonholee commented Dec 7, 2016

Description

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!

singhkays Dec 8, 2016 • 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!

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!

cormacpayne commented Dec 8, 2016

Uh oh!

hyonholee commented Dec 8, 2016

Uh oh!

cormacpayne commented Dec 9, 2016

Uh oh!

Uh oh!

singhkays Dec 8, 2016 •

edited

Loading