Add parameters for alternate extensions and encrypt-format #4848

vermashi · 2017-10-24T23:34:49Z

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

cormacpayne

@vermashi a few comments that need to be addressed. Also,

Please update the Compute change log to reflect the changes made in this PR
Please add test coverage to verify the changes you are making in this PR are working as intended

cormacpayne · 2017-10-25T17:12:25Z

...Manager/Compute/Commands.Compute/Extension/AzureDiskEncryption/DisableAzureDiskEncryption.cs

+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "The extension type. Specify this parameter to override its default value of \"AzureDiskEncryption\" for Windows VMs and \"AzureDiskEncryptionForLinux\" for Linux VMs.")]
+        [ValidateNotNullOrEmpty]
+        public string ExtensionType { get; set; }


@vermashi is there a closed set of values that can be provided to this parameter? If so, please use a ValidateSet to define the possible set of values to allow a user to tab through them

No, users are not limited to a closed set of values for this parameter.

cormacpayne · 2017-10-25T17:12:42Z

...Manager/Compute/Commands.Compute/Extension/AzureDiskEncryption/DisableAzureDiskEncryption.cs

+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "The extension publisher name. Specify this parameter only to override the default value of \"Microsoft.Azure.Security\".")]
+        [ValidateNotNullOrEmpty]
+        public string ExtensionPublisherName { get; set; }


@vermashi same comment about possibly using a ValidateSet

For background, the defaults won't change, and the user can get those if they don't specify this optional parameter.

cormacpayne · 2017-10-25T17:14:14Z

...er/Compute/Commands.Compute/Extension/AzureDiskEncryption/SetAzureDiskEncryptionExtension.cs

+            Position = 18,
+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "Encrypt-Format all data drives that are not already encrypted")]
+        public SwitchParameter EncryptFormatAll { get; set; }


@vermashi a SwitchParameter should not have a position nor should it accept a value from the pipeline

cormacpayne · 2017-10-25T17:15:31Z

src/ResourceManager/Compute/Commands.Compute/help/Disable-AzureRmVMDiskEncryption.md

@@ -76,6 +78,32 @@ Accept pipeline input: True (ByPropertyName)
 Accept wildcard characters: False
 ```

+### -ExtensionPublisherName
+The extension publisher name. Specify this parameter only to override the default value of "Microsoft.Azure.Security".```yaml


@vermashi this is a bug in platyPS - please make sure the ```yaml string is on a line by itself. See the other parameters in this file for guidance.

cormacpayne · 2017-10-25T17:15:38Z

src/ResourceManager/Compute/Commands.Compute/help/Disable-AzureRmVMDiskEncryption.md

+```
+
+### -ExtensionType
+The extension type. Specify this parameter to override its default value of "AzureDiskEncryption" for Windows VMs and "AzureDiskEncryptionForLinux" for Linux VMs.```yaml


@vermashi same comment

cormacpayne · 2017-10-25T17:15:53Z

src/ResourceManager/Compute/Commands.Compute/help/Get-AzureRmVMDiskEncryptionStatus.md

@@ -48,6 +50,32 @@ Accept pipeline input: False
 Accept wildcard characters: False
 ```

+### -ExtensionPublisherName
+The extension publisher name. Specify this parameter only to override the default value of "Microsoft.Azure.Security".```yaml


@vermashi same comment in this file for the added parameters

cormacpayne · 2017-10-25T17:16:04Z

src/ResourceManager/Compute/Commands.Compute/help/Set-AzureRmVMDiskEncryptionExtension.md

@@ -293,6 +296,45 @@ Accept pipeline input: True (ByPropertyName)
 Accept wildcard characters: False
 ```

+### -EncryptFormatAll
+Encrypt-Format all data drives that are not already encrypted```yaml


@vermashi same comment in this file for the added parameters

ejarvi

It looks like the platyPS yaml strings need to be changed as per feedback from @cormacpayne but after that, it looks good to me.

ejarvi · 2017-10-26T00:18:56Z

...ompute/Commands.Compute/Extension/AzureDiskEncryption/AzureDiskEncryptionExtensionContext.cs

        public const string LinuxExtensionDefaultVersion = "0.1";

        public const string ExtensionDefaultPublisher = "Microsoft.Azure.Security";
        public const string ExtensionDefaultName = "AzureDiskEncryption";
+        public const string ExtensionDefaultType = "AzureDiskEncryption";


👍 ... nice to see this getting disambiguated :)

ejarvi · 2017-10-26T00:26:53Z

...Manager/Compute/Commands.Compute/Extension/AzureDiskEncryption/DisableAzureDiskEncryption.cs

+            ValueFromPipelineByPropertyName = true,
+            HelpMessage = "The extension publisher name. Specify this parameter only to override the default value of \"Microsoft.Azure.Security\".")]
+        [ValidateNotNullOrEmpty]
+        public string ExtensionPublisherName { get; set; }


For background, the defaults won't change, and the user can get those if they don't specify this optional parameter.

vermashi · 2017-10-26T22:57:12Z

Have addressed most of the comments. Working on the tests.

vermashi · 2017-10-30T17:38:42Z

@cormacpayne

I have been trying to fix the tests but I'm running into an issue that when I try to create KeyVaults while running in Record Mode, I get the exception "TestManagementClientHelper class wasn't initialized with the ResourceManagementClient client". I'm not confident I can fix this issue before the snap date.
Our team has been considering that since the primary purpose of "ExtensionPublisherName" parameter will be to redirect the cmdlet to a private build of the extension, if its ok to pass this information, when required, via an EnvironmentVariable.

TL;DR: 1- The tests might not get fixed by snap date, possible to ship without? Is it ok if we replace some of the newly added parameters in this PR with env-variables?

maddieclayton · 2017-11-02T00:17:03Z

@vermashi Please fix the merge conflicts and push.

This reverts commit 9b1b045.

This reverts commit 507717b.

cormacpayne

@vermashi a few minor comments, otherwise LGTM

cormacpayne · 2017-11-02T03:42:28Z

...Manager/Compute/Commands.Compute/Extension/AzureDiskEncryption/DisableAzureDiskEncryption.cs

@@ -89,6 +89,22 @@ public class DisableAzureDiskEncryptionCommand : VirtualMachineExtensionBaseCmdl
            HelpMessage = "Disable auto-upgrade of minor version")]
        public SwitchParameter DisableAutoUpgradeMinorVersion { get; set; }

+        [Parameter(
+            Mandatory = false,
+            Position = 6,


@vermashi nit: the number of positional parameters in a single set should be capped at four or five. After this, it can be difficult for the user to remember the order in which they should provide the values, so our recommendation would be to remove the positions from the parameters you added (I would also include the DisableAutoUpgradeMinorVersion parameter above since switch parameters are only named ☺️ )

cormacpayne · 2017-11-02T03:43:26Z

...nager/Compute/Commands.Compute/Extension/AzureDiskEncryption/GetAzureDiskEncryptionStatus.cs

@@ -58,6 +58,22 @@ public class GetAzureDiskEncryptionStatusCommand : VirtualMachineExtensionBaseCm
        [ValidateNotNullOrEmpty]
        public string Name { get; set; }

+        [Parameter(
+            Mandatory = false,
+            Position = 3,


@vermashi nit: you can decide whether or not the positions are needed for the new parameters ☺️

cormacpayne · 2017-11-02T03:45:53Z

...er/Compute/Commands.Compute/Extension/AzureDiskEncryption/SetAzureDiskEncryptionExtension.cs

            publicSettings.Add(AzureDiskEncryptionExtensionConstants.sequenceVersionKey, SequenceVersion ?? String.Empty);

+            if (EncryptFormatAll)


@vermashi nit: EncryptFormatAll.IsPresent for consistency with other cmdlets

cormacpayne · 2017-11-02T03:46:26Z

src/ResourceManager/Compute/Commands.Compute/help/Set-AzureRmVMDiskEncryptionExtension.md

+Required: False
+Position: Named
+Default value: None
+Accept pipeline input: True (ByPropertyName)


@vermashi it looks like the help for these cmdlets needs to be regenerated

cormacpayne · 2017-11-02T03:48:12Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/116/

cormacpayne · 2017-11-06T06:12:29Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/141/

vermashi requested review from ejarvi, hyonholee and mayank88mahajan October 24, 2017 23:35

azuresdkci added the needs-review label Oct 25, 2017

azuresdkci assigned shahabhijeet Oct 25, 2017

cormacpayne suggested changes Oct 25, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Oct 25, 2017

cormacpayne assigned vermashi and unassigned shahabhijeet Oct 25, 2017

ejarvi suggested changes Oct 26, 2017

View reviewed changes

maddieclayton added the Merge Conflicts label Nov 2, 2017

vermashi added 9 commits November 1, 2017 17:37

allow test switch in code

04f2d9e

update help documentation

f4980aa

add code for encrypt format all

2840af4

add documentation for encrypt format

f3b0390

fix help docs and cmdlet parameter

a2bcb50

switch to env variables

507717b

update help

9b1b045

Revert "update help"

a37fecc

This reverts commit 9b1b045.

Revert "switch to env variables"

200edf5

This reverts commit 507717b.

vermashi force-pushed the shiv-test-switch branch from c2e57ec to 200edf5 Compare November 2, 2017 00:43

cormacpayne previously approved these changes Nov 2, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed Merge Conflicts labels Nov 2, 2017

markcowl removed the Waiting for On Demand label Nov 2, 2017

address pr comments

b0f7b84

vermashi dismissed cormacpayne’s stale review via b0f7b84 November 2, 2017 23:16

markcowl changed the base branch from preview to release-5.0.0 November 2, 2017 23:24

Update BreakingChangeIssues.csv

6724c4a

ejarvi previously approved these changes Nov 3, 2017

View reviewed changes

Merge branch 'release-5.0.0' into shiv-test-switch

caaf87c

cormacpayne dismissed ejarvi’s stale review via caaf87c November 6, 2017 06:11

cormacpayne approved these changes Nov 6, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-revision labels Nov 6, 2017

cormacpayne unassigned vermashi Nov 6, 2017

cormacpayne merged commit fca0a14 into Azure:release-5.0.0 Nov 6, 2017

		publicSettings.Add(AzureDiskEncryptionExtensionConstants.sequenceVersionKey, SequenceVersion ?? String.Empty);

		if (EncryptFormatAll)

Add parameters for alternate extensions and encrypt-format #4848

Add parameters for alternate extensions and encrypt-format #4848

Uh oh!

Conversation

vermashi commented Oct 24, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

cormacpayne 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!

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!

ejarvi 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!

vermashi commented Oct 26, 2017

Uh oh!

vermashi commented Oct 30, 2017

Uh oh!

maddieclayton commented Nov 2, 2017

Uh oh!

cormacpayne 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!

Choose a reason for hiding this comment

Uh oh!

cormacpayne commented Nov 2, 2017

Uh oh!

cormacpayne commented Nov 6, 2017

Uh oh!

Uh oh!

vermashi commented Oct 24, 2017 •

edited by azuresdkci

Loading