Update AzureStack.Compute.Admin powershell quota object to support two additional managed disk parameters #6903

cheguv · 2018-08-11T01:09:52Z

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

bganapa · 2018-08-11T17:19:01Z

@cheguv build is failing.. could you pls take a look https://azuresdkci.westus2.cloudapp.azure.com/job/powershell/6543/console

bganapa · 2018-08-11T17:25:49Z

...Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Get-AzsPlatformImage.ps1

            $Location = (Get-AzureRMLocation).Location
        }

-        $filterInfos = @(


Why this is being removed?

@cheguv We have to leave this in, I forgot that this was for client side filters.

added back in.

I still see it as removed.

reverted all changes to this file. should now have no differeence from what was in azure\azure-powershell

bganapa · 2018-08-11T17:29:02Z

@cheguv Can you please update the Examples and markdown help files? To update the markdown files, you could use tools\UpdatePlaytyps.ps1 file

deathly809 · 2018-08-12T17:53:57Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1


+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationStandardManagedDisksAndSnapshots = 2048,


Please shorten these.

This has been discussed and PM has opened a bug to track this and get done.

deathly809 · 2018-08-12T17:54:03Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1

+
+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationPremiumManagedDisksAndSnapshots = 2048,


Please shorten these.

This has been discussed and PM has opened a bug to track this and get done.

deathly809 · 2018-08-12T17:54:12Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1

 .PARAMETER VirtualMachineCount
    Maximum number of virtual machines allowed.

+.PARAMETER MaxAllocationStandardManagedDisksAndSnapshots


Same as above.

This has been discussed and PM has opened a bug to track this and get done.

deathly809 · 2018-08-12T17:54:18Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1

+.PARAMETER MaxAllocationStandardManagedDisksAndSnapshots
+	Maximum number of standard managed disks and snapshots allowed
+
+.PARAMETER MaxAllocationPremiumManagedDisksAndSnapshots


Same as above.

This has been discussed and PM has opened a bug to track this and get done.

deathly809 · 2018-08-12T17:54:33Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1


+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationStandardManagedDisksAndSnapshots,


Same as above.

This has been discussed and PM has opened a bug to track this and get done.

deathly809 · 2018-08-12T17:54:37Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1

+
+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationPremiumManagedDisksAndSnapshots,


Same as above.

This has been discussed and PM has opened a bug to track this and get done.

We will just have to make a breaking change in the future and notify clients with a warning before we do.

@cheguv Do you want to print a warning saying the parameter name MaxAllocationStandardManagedDisksAndSnapshots will be changed in future kind of message when this param is being used?

Please rename the PowerShell parameters to the shortened name - they don't need to match the backend, and better to not match now than to make breaking changes in the future.

deathly809 · 2018-08-12T17:56:19Z

...zs.Compute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaObject.ps1


+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationStandardManagedDisksAndSnapshots,


Please shorten these.

This has been discussed and PM has opened a bug to track this and get done.

You can change it here, it won't affect server side. Please consult with PM to change it now in PowerShell so we don't have a breaking change later.

Ping on this comment

StandardManagedDiskAndSnapshotSize

this is is shortened. Check ConvertTo-ComputeQuota.ps1.
new ComputeQuotaObject type is defined in this file. This is what is exposed to customers.

…ig for both compute.admin and tests

bganapa · 2018-08-14T18:43:47Z

ps-sign https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/ps-sign/1043/

maddieclayton · 2018-08-14T21:14:54Z

src/StackAdmin/Azs.Compute.Admin/Help/New-AzsComputeQuota.md

-### -CoresLimit
-Maximum number of cores allowed.
+### -Location
+{{Fill Location Description}}


Fill out description - apply everywhere in this file.

maddieclayton · 2018-08-14T21:15:52Z

src/StackAdmin/Azs.Compute.Admin/Help/Remove-AzsVMExtension.md

-### -Type
-Type of extension.
+### -Location
+{{Fill Location Description}}


maddieclayton · 2018-08-14T21:16:50Z

...zs.Compute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaObject.ps1


+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationStandardManagedDisksAndSnapshots,


Ping on this comment

maddieclayton · 2018-08-14T21:19:52Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1

+
+        [Parameter(Mandatory = $false)]
+        [int32]
+		$MaxAllocationPremiumManagedDisksAndSnapshots,


Please rename the PowerShell parameters to the shortened name - they don't need to match the backend, and better to not match now than to make breaking changes in the future.

deathly809 · 2018-08-17T17:27:01Z

src/StackAdmin/Azs.Compute.Admin/Help/New-DataDiskObject.md

@@ -1,72 +0,0 @@
---


Why was this deleted?

deathly809 · 2018-08-17T17:27:06Z

src/StackAdmin/Azs.Compute.Admin/Help/Remove-AzsComputeQuota.md

@@ -1,147 +0,0 @@
---


Why was this deleted?

deathly809 · 2018-08-17T17:27:15Z

src/StackAdmin/Azs.Compute.Admin/Help/Remove-AzsPlatformImage.md

@@ -1,185 +0,0 @@
---


Why was this deleted?

deathly809 · 2018-08-17T17:28:02Z

...pute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaCustomObject.ps1

+        [string]
+        $Type
+
+        [int32]


This will be a BREAKING change that will break all the BVTs along with clients.

deathly809 · 2018-08-17T17:32:22Z

src/StackAdmin/Azs.Compute.Admin/Tests/src/Quota.Tests.ps1

+                $quota.CoresCount           | Should be $_[1]
                $quota.VmScaleSetCount      | Should be $_[2]
                $quota.VirtualMachineCount  | Should be $_[3]
+				$quota.StandardManagedDiskAndSnapshotSize  | Should be $_[4]


deathly809 · 2018-08-17T17:34:41Z

src/StackAdmin/Azs.Compute.Admin/Tests/src/Quota.Tests.ps1

-                $Quota.CoresLimit           | Should Not Be $null
+                $Quota.CoresCount           | Should Not Be $null
                $Quota.VirtualMachineCount  | Should Not Be $null
                $Quota.VmScaleSetCount      | Should Not Be $null


You are not validating the new Quota properties. Add validation for those.

deathly809 · 2018-08-17T17:36:01Z

src/StackAdmin/Azs.Compute.Admin/Tests/src/Quota.Tests.ps1

+
+			#
+			#	if running against the actual environment enable the following to create first.
+			#	New-AzsComputeQuota -Location $global:Location -Name "testQuotaCreateUpdateDelete" -AvailabilitySetCount 1 -CoresCount 1 -VmScaleSetCount 1 -VirtualMachineCount 1 -StandardManagedDiskAndSnapshotSize 1 -PremiumManagedDiskAndSnapshotSize 1


You should have recorded the create.

for some reason recording with powershell is not working where as with sdk did work well.

bganapa · 2018-08-17T18:57:21Z

src/StackAdmin/Azs.Compute.Admin/Help/Add-AzsPlatformImage.md

 ```

 Add a new platform image.



There should not be a need to change this file as there is no change in the cmdlet Add-AzsPlatformImage?

yes, there might be soem update in platyps script, that caused this change, i will remove.

deathly809 · 2018-08-17T20:11:21Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/Set-AzsComputeQuota.ps1

            if ('Update' -eq $PsCmdlet.ParameterSetName -or 'InputObject' -eq $PsCmdlet.ParameterSetName -or 'ResourceId' -eq $PsCmdlet.ParameterSetName) {

                if ($NewQuota -eq $null) {
                    $NewQuota = Get-AzsComputeQuota -Location $Location -Name $Name


Change this to

ConvertTo-SDKQuota -Quota (Get-AzsComputeQuota -Location $Location -Name $Name)

deathly809 · 2018-08-17T20:12:06Z

...pute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaCustomObject.ps1

+    return $result
+}
+
+function Convert-CustomQuotaToSDKObject


Rename to function to

ConvertTo-SDKQuota

deathly809 · 2018-08-17T20:13:47Z

...pute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaCustomObject.ps1

@@ -0,0 +1,115 @@
+<#


Split into two files, ConvertTo-CustomQuotaObject.ps1 and ConvertTo-SDKQuota.ps1

deathly809 · 2018-08-17T20:19:14Z

...pute.Admin/Generated.PowerShell.Commands/SwaggerDefinitionCommands/New-QuotaCustomObject.ps1

+        $Location
+}
+
+function New-QuotaCustomObject


rename to

ConvertTo-CustomQuotaObject

…bject to be used, add back deleted md files, review comments

deathly809 · 2018-08-20T14:59:10Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1


        if ($PSCmdlet.ShouldProcess("$Name", "Create a new compute quota.")) {

+            <#


Uncomment this and move to first line of Process block.

Also, add this and alias to Set-AzsComputeQuota

deathly809 · 2018-08-20T15:00:53Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1


                $quotaObj = Get-TaskResult @GetTaskResult_params
-				[QuotaCustomObject]$script:result = New-QuotaCustomObject -Quota $quotaObj
+                [ComputeQuotaObject]$script:result = ConvertTo-ComputeQuota -Quota $quotaObj


You don't need to save to variable. Just have this single line at the end of your function

ConvertTo-ComputeQuota -Quota (Get-TaskResult @GetTaskResult_params)

deathly809 · 2018-08-20T15:03:25Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1

        [int32]
        $AvailabilitySetCount = 10,

+        #[Alias("CoresLimit")]


Why is this commented out?

deathly809 · 2018-08-21T00:15:01Z

.../Azs.Compute.Admin/Generated.PowerShell.Commands/SwaggerPathCommands/New-AzsComputeQuota.ps1

-                $utilityCmdParams[$_] = Get-Variable -Name $_ -ValueOnly
-            }
-            $NewQuota = New-QuotaObject @utilityCmdParams
+            $NewQuota = ConvertTo-SdkQuota $PSBoundParameters


Why did you do this? My last comment said to just revert to what was there originally and creating 2 variables for the expected C# SDK quota object. To make it work with alias just set $CoresLimit = $CoresCount if $CoresLimit is not set.

…e alias is used for two methods New-AzsComputeQuota and Set-AzsComputeQuota

… alias

cheguv · 2018-08-21T22:00:50Z

@bganapa @deathly809 Can you please provide your approval for ASDK team to merge the changes.

cheguv · 2018-08-23T05:21:58Z

@maddieclayton can you please help merge the PR.

deathly809 · 2018-08-23T14:54:00Z

@cheguv As we said, we are waiting for the Azure team to finish some work. They have a PR that is outstanding and we cannot merge until that has finished.

deathly809 · 2018-08-28T16:19:47Z

@maddieclayton Can you merge this so I can do my PR?

maddieclayton · 2018-08-28T17:33:29Z

@deathly809 Can you give your approval on the PR just so we have record of it? Since you release the Admin modules, I'm going to require approval from either you or @bganapa before merging PRs to StackAdmin. After the Travis job passes I'll merge.

deathly809 · 2018-08-28T17:39:06Z

@maddieclayton I approve.

Update AzureStack.Compute.Admin powershell quota object to support two additional managed disk parameters

cheguv added 2 commits August 3, 2018 16:58

fix for get-azslocation failure

849ee03

Support Compute Quota with two additional parameters

dbf8d37

cheguv requested review from bganapa and deathly809 August 11, 2018 01:10

azuresdkci added the needs-review label Aug 11, 2018

bganapa reviewed Aug 11, 2018

View reviewed changes

deathly809 reviewed Aug 12, 2018

View reviewed changes

maddieclayton added needs-revision and removed needs-review labels Aug 13, 2018

maddieclayton assigned cheguv Aug 13, 2018

update markdown files for Azs.Compute.Admin

0f98751

deathly809 added the Azure Stack label Aug 13, 2018

cheguv force-pushed the preview branch from 5ff68b3 to 813c7eb Compare August 14, 2018 00:38

update the sdk version from 0.2.0 to 0.3.0, also update packages.conf…

0755a6c

…ig for both compute.admin and tests

cheguv force-pushed the preview branch from 813c7eb to 0755a6c Compare August 14, 2018 00:54

maddieclayton added needs-review and removed needs-revision labels Aug 14, 2018

maddieclayton assigned maddieclayton and unassigned cheguv Aug 14, 2018

maddieclayton suggested changes Aug 14, 2018

View reviewed changes

maddieclayton added the needs-revision label Aug 14, 2018

deathly809 reviewed Aug 17, 2018

View reviewed changes

src/StackAdmin/Azs.Compute.Admin/Help/New-DataDiskObject.md

@@ -1,72 +0,0 @@

---

Copy link

Member

deathly809 Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

deathly809 reviewed Aug 17, 2018

View reviewed changes

src/StackAdmin/Azs.Compute.Admin/Help/Remove-AzsComputeQuota.md

@@ -1,147 +0,0 @@

---

Copy link

Member

deathly809 Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

deathly809 reviewed Aug 17, 2018

View reviewed changes

src/StackAdmin/Azs.Compute.Admin/Help/Remove-AzsPlatformImage.md

@@ -1,185 +0,0 @@

---

Copy link

Member

deathly809 Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this deleted?

deathly809 reviewed Aug 17, 2018

View reviewed changes

bganapa reviewed Aug 17, 2018

View reviewed changes

deathly809 reviewed Aug 17, 2018

View reviewed changes

Reduce characters in two new parameter names, needs an intermediate o…

151453f

…bject to be used, add back deleted md files, review comments

deathly809 reviewed Aug 20, 2018

View reviewed changes

deathly809 reviewed Aug 21, 2018

View reviewed changes

Update CoresList as an alias for CoresCount and warning message incas…

03fa1d7

…e alias is used for two methods New-AzsComputeQuota and Set-AzsComputeQuota

cheguv force-pushed the preview branch from 365c52f to 03fa1d7 Compare August 21, 2018 06:16

Regenerate md files for CoreCount paramter that now has CoresLimit as…

de0f43a

… alias

Merge branch 'preview' into preview

2b8c7c1

maddieclayton approved these changes Aug 28, 2018

View reviewed changes

maddieclayton merged commit 4434f13 into Azure:preview Aug 28, 2018

ionuttulai pushed a commit to ionuttulai/azure-powershell that referenced this pull request Mar 4, 2020

Merge pull request Azure#6903 from cheguv/preview

8dbadd2

Update AzureStack.Compute.Admin powershell quota object to support two additional managed disk parameters


		if ($PSCmdlet.ShouldProcess("$Name", "Create a new compute quota.")) {

		<#

Update AzureStack.Compute.Admin powershell quota object to support two additional managed disk parameters #6903

Update AzureStack.Compute.Admin powershell quota object to support two additional managed disk parameters #6903

Uh oh!

Conversation

cheguv commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

bganapa commented Aug 11, 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!

bganapa commented Aug 11, 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!

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!

bganapa commented Aug 14, 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

cheguv commented Aug 11, 2018 •

edited

Loading