[Compute] Update VMSS VM feature #6170

hyonholee · 2018-05-09T02:23:08Z

This PR adds Update-AzureRmVmssVM and New-AzureRmVMDataDisk cmdlets.
Add-AzureRmVMDataDisk is also updated to support VMSS VM update feature.

https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/8

Description

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

praries880 · 2018-05-09T22:34:48Z

src/ResourceManager/Common/Commands.ResourceManager.Common/AzureRMCmdlet.cs

+                    null));
+        }
+
+        protected string GetDiskNameFromId(string Id)


Is this utility function going to be user by cmdlets outside compute as well? If n ot, then I think a better place to put it would be under compute common, more specifically the compute base cmdlet

praries880 · 2018-05-09T22:44:51Z

src/ResourceManager/Compute/Commands.Compute.Test/ScenarioTests/VirtualMachineScaleSetTests.ps1

+        $vmssType = 'Microsoft.Compute/virtualMachineScaleSets';
+
+        $adminUsername = 'Foo12';
+        $adminPassword = $PLACEHOLDER;


Use this method to generate the passwords instead of the Placeholder

praries880 · 2018-05-09T22:48:46Z

src/ResourceManager/Compute/Commands.Compute/Generated/ComputeAutomationBaseCmdlet.cs

        {
            if (string.IsNullOrEmpty(resourceId)) { return null; }
-            Regex r = new Regex(@"(.*?)/" + resourceName + @"/(?<rgname>\S+)", RegexOptions.IgnoreCase);
+            Regex r = (instanceName == null)
+                      ? new Regex(@"(.*?)/" + resourceName + @"/(?<rgname>\S+)", RegexOptions.IgnoreCase)


it seems the three reg exes here share the same prefix, i.e. :

@"(.*?)/" + resourceName + @"/(?<rgname>\S+)"

Can you move it to a common const that gets consumed in all the three locations??

resourceName is a variable.

praries880 · 2018-05-09T22:52:03Z

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

+
+namespace Microsoft.Azure.Commands.Compute.Models
+{
+    public class PSVirtualMachineDataDisk : DataDisk


Is this class ever going to add any new functionality over the class DataDisk?

OneSDK asked to convert any client library object (in this case, Management.Compute.Models) to a powershell object (in this case, Commands.Compute.Models) for a return object, so that it is easy to hide a possible breaking change (in the future) caused by a client library.

praries880 · 2018-05-09T22:56:51Z

src/ResourceManager/Compute/Commands.Compute/VirtualMachine/Config/NewAzureVMDataDiskCommand.cs

+
+        [Parameter(
+            Mandatory = true,
+            Position = 1,


Is there something expected to be at Position 0?

cormacpayne · 2018-05-11T15:49:54Z

src/ResourceManager/Compute/Commands.Compute/VirtualMachine/Config/NewAzureVMDataDiskCommand.cs

+                    WriteAcceleratorEnabled = this.WriteAccelerator.IsPresent
+                };
+
+            WriteObject(dataDisk);


@hyonholee since Name and ManagedDiskId are optional parameters, there are scenarios where the disk that is being returned doesn't have a name or managed disk -- how is this handled when you try to add the disk to a VMSS VM and update it on the server?

@cormacpayne For VMSS VM, ManagedDiskId is a mandatory. It only accepts an existing managed disk. But I wrote this cmdlet for a general case, which can create an unmanaged disk, managed disk without referencing existing id, and managed disk with referencing existing id.

cormacpayne · 2018-05-11T15:59:32Z

src/ResourceManager/Compute/Commands.Compute/VirtualMachine/Config/AddAzureVMDataDiskCommand.cs

            ValueFromPipelineByPropertyName = true,
            HelpMessage = HelpMessages.VMDataDiskVhdUri)]
        [ValidateNotNullOrEmpty]
        public string VhdUri { get; set; }

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


@hyonholee any reason that positions are being removed from parameters in this file? This would be a breaking change

@cormacpayne this is optional parameter, so I removed the position value

@hyonholee I agree that this is correct pattern, but it's still a breaking change. We should look at making this kind of change across all Compute cmdlets in the next breaking change release

MiYanni · 2018-05-15T21:57:41Z

@hyonholee The requested changes need to be made before this PR can be accepted.

hyonholee · 2018-05-17T00:06:27Z

@cormacpayne @praries880 I addressed the comments and updated PR. Please review it. Thanks.

cormacpayne · 2018-05-21T22:53:38Z

.../Commands.Compute/Generated/VirtualMachineScaleSetVM/VirtualMachineScaleSetVMUpdateMethod.cs

+                    {
+                        foreach (var d in this.DataDisk)
+                        {
+                            parameters.StorageProfile.DataDisks.Add(d);


@hyonholee do we need to do any type of null-check for StorageProfile and/or DataDisks, or they instantiated when creating the new VirtualMachineScaleSetVM object above?

@cormacpayne Thanks, I updated the PR.

…nto vmssvm # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv # tools/StaticAnalysis/Exceptions/SignatureIssues.csv

hyonholee · 2018-05-23T21:14:46Z

@cormacpayne @praries880 Could you approve this PR, if no more concern? Thanks.

cormacpayne · 2018-05-23T21:19:51Z

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

cormacpayne · 2018-05-23T21:20:17Z

@praries880 would you mind signing off if you feel everything has been resolved?

cormacpayne · 2018-05-24T16:41:31Z

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

* #1443089: New Powershell commandlets for feature:P2S Ipsec custom policy set on Gateway. and fix build merge errors. * #1443089: Update change log * fix in AzureRM.Network.psd1 * Fix for issue #5365 Moved the Websites dependency of LogicApp into commons * Add websites dll to required assemblies part of Profile * Fic sln files to include the dependencies * Fic dependency paths and projects * Fix dll name * Add/update help files for PS command lets * Incorporate code review comments * Add csproj files for the new common module to Profile * Fix build * Fix dll name (WebSites -> Websites) in the pofile psd1 * Record failing tests * [Maps] Rename folders and files from LocationBasedServices * [Maps] Rename help file names * [Maps] Renaming variables, strings, descriptions, etc. * Recorded and enabled skipped tests * Add StaticAnalysis checks for - No output type - Optional positional parameters - Parameter set names containing a space - Cmdlets with no default parameter set name and multiple sets * Add tests for new static analysis checks * [Maps] Use Maps SDK over LocationBasedServices SDK * Record and enable skipped Brooklyn tests to have test coverage * fix test * move from on demand to regular build * #Fix help file error * Fix static analysis errors * Fix cred scan errors * Fix cred scan errors * Add dependence in the ServiceManagement sln to fix the sign job * Update SignatureIssues.csv * Incorporate code review comments * Updating Networking tests with owner attributes * Fix command output * Revert "Fix command output" This reverts commit a4bace3. * Suppress signature issues as they are approved by Powershell team * Strategies library: Version 4, new WriteWarning method. * Websites.Strategies are replaced by Common.Strategies library. * Added comments with full team names * Cleanup dependencies and assembly references * remove one more reference * Fix for empty examples. * [Maps] Add legacy ResourceManager & update test recordings * [Maps] Add legacy ResourceManager & update test recordings * Add example to Stop-AzureRmResourceGroupDeployment.md * [Maps] Resolving merge revision requests * [Maps] Resolving merge revision requests (2) * Remove spaces in location before comparision * Revert default role assignment in New-AzureRmADServicePrincipal, update help with examples * Update Stack.sln * [Maps] Add PassThru to Remove method, resolve build failure * Update azure-powershell-developer-guide.md * [Maps] Update help files to reflect -PassThru param addition * [Maps] Fix trailing yaml string for PassThru * address comments * Fix sign job, add tests for service principal creation with role or scope * Fix local build issues because of a netcore sln file introduced accidentally * Update azure-powershell-developer-guide.md * Update test for test-ConnectivityTroubleshoot * Fix AKS doc * Update Get-AzureRmAks.md * Dependency fix : ServiceFabric (#5971) * Fix for #535 Dependency fixes for ServiceFabric in this commit * Add signing stuff and netcode projects to fix netcore build. * Added exception for breaking changes * Update all the sln projects with the newest Keyvault common project * Took care of Cormac's comments * Fix bad merge * Fix breaking build as oer comments from Cormac * Fix file name * Fix sln files for new dependencies * Fix netcore csproj * Fic netcode proj for storage.management * Fix dependency paths and project names * Fix assembly name in assembluInfo * Fix KEyVault guid * Add csproj fils for the new common modules to profile * Fix names of csproj files * Netcore projects are case sensitive, and when i changed Keyvault to KeyVault, none of the folder names or file names reflected the change in git... git still sees them as Keyvault * Fix caseing in one more netcore project * Fix casing on the namespace... * Fix the folder name mixup. Step 1 of 2 * Fix the foldername mixup, Step 2 of 2 * Fix project references to point to the correct location * Fix namespace casing in test projet * Fix bad merge * Fix mad merge * Fix project dependencies * As poer feedback, add 2018_04_01 version for compute * Update sevice fabric dependency on the common compute code * Suppress warnings about passwords in generated code. * Credscan can not hanndle secrets in comments.. just removing the offending comments * Fix tests to be able to record them and other small changes * Fix bad merge * Remove UTF8 BOM encoding from file * Fix bad merge in profile psd1 file * Took care of Maddie's comment * iFix assembly name in profile csproj * Add common projects to the stack solution as well * Fix Issue 5545 - Set Auth Values on UpdateAzureSchedulerServiceBusQueueJobCommand (#6196) * 5545 - Set Auth to value from PSServiceBusAuthenticationParams * Change log updates * Update versions for release of AzureRM.Resources * Fix issue with Properties property of PSResource (#6218) * Fix issue with Properties property of PSResource * Add test coverage for Properties property * Release Maps (#6287) * Update VMSS VM feature (#6170) * Add test coverage for no role assignment in default SP creation scenario * Update AzureRM.psm1 * adding right powershell command Add-AzureApplicationGatewayRequestRoutingRule <-- incorrect Add-AzureRmApplicationGatewayRequestRoutingRule <--corrected command * removing adding space causing powershell error in example $ AppGw is not a valid powershell variable $AppGw has been proposed instead * Typo in powershell command Get-AzureRmApplicationGatewayFrontendIPort is no a valid command Get-AzureRmApplicationGatewayFrontendPort proposed as the corrected powershell command * Fixes inconsistent slot name in example The example and description's slot names should match. * Merging RecoveryServices.Backup code into RecoveryServices (#6263) * Merged RecoveryServices.Backup code in to RecoveryServices * Updating change log * Adding ignored file * Updating test targets and mappings * Moving changelog and documentation files of RS and RS.Backup to their respective module folders * Add ManagementGroups to AzureRM.Resources and Update to use latest SDK for GA (#6271) * Initial migration commit * Move ManagementGroups to AzureRM.Resources * Update powershell to use the latest SDK and API, minor changes to responses and help files * Add installer file * Revert "Add installer file" This reverts commit 0d9b92c. * Update tests and run tests again * Update Help * Update helpfiles * more changes to help files * Changes to Netcore.psd1 * Changes to Netcore.csproj * Update help file * Regenerate all help files * Move help files to AzureRM.Resource help files * Update package version * Update local feed and rerun tests * Remove local feed

azuresdkci added the needs-review label May 9, 2018

hyonholee force-pushed the vmssvm branch from 8bc5492 to 6c08208 Compare May 9, 2018 03:26

hyonholee requested review from cormacpayne, praries880 and markcowl May 9, 2018 17:42

praries880 suggested changes May 9, 2018

View reviewed changes

hyonholee force-pushed the vmssvm branch from 6c08208 to f9f9f3d Compare May 10, 2018 03:50

cormacpayne suggested changes May 11, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels May 11, 2018

cormacpayne assigned hyonholee May 11, 2018

hyonholee force-pushed the vmssvm branch from f9f9f3d to 2a4f444 Compare May 17, 2018 00:05

maddieclayton assigned cormacpayne and praries880 and unassigned hyonholee May 21, 2018

maddieclayton added needs-review and removed needs-revision labels May 21, 2018

cormacpayne reviewed May 21, 2018

View reviewed changes

Update VMSS VM feature

ee974cd

hyonholee force-pushed the vmssvm branch from 2a4f444 to ee974cd Compare May 22, 2018 18:35

Merge branch 'preview' of https://github.com/Azure/azure-powershell i…

7f3902d

…nto vmssvm # Conflicts: # tools/StaticAnalysis/Exceptions/BreakingChangeIssues.csv # tools/StaticAnalysis/Exceptions/SignatureIssues.csv

cormacpayne previously approved these changes May 23, 2018

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-review labels May 23, 2018

cormacpayne unassigned cormacpayne and praries880 May 23, 2018

praries880 previously approved these changes May 23, 2018

View reviewed changes

Merge branch 'preview' into vmssvm

363ad97

cormacpayne dismissed stale reviews from praries880 and themself via 363ad97 May 24, 2018 16:40

maddieclayton approved these changes May 24, 2018

View reviewed changes

maddieclayton merged commit 8c83c74 into Azure:preview May 24, 2018

hyonholee deleted the vmssvm branch May 29, 2018 21:48

[Compute] Update VMSS VM feature #6170

[Compute] Update VMSS VM feature #6170

Uh oh!

Conversation

hyonholee commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

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!

MiYanni commented May 15, 2018

Uh oh!

hyonholee commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyonholee commented May 23, 2018

Uh oh!

cormacpayne commented May 23, 2018

Uh oh!

cormacpayne commented May 23, 2018

Uh oh!

cormacpayne commented May 24, 2018

Uh oh!

Uh oh!

hyonholee commented May 9, 2018 •

edited

Loading