-
Notifications
You must be signed in to change notification settings - Fork 4k
[Compute] Update-AzureRmVmss update for patch operation #4912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not at all what we discussed. Adding a complex parameter that containspatch properties is precisely what should not be done to make this cmdlet usable.
Please add in the simple parameters we agreed to in the review.
Mandatory = false, | ||
ValueFromPipelineByPropertyName = false, | ||
ValueFromPipeline = true)] | ||
[AllowNull] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you allowing null for all of these parmeters? This allows someone to set the parameter to $null. What is the reasoning behind this? Are you trying to enable removing this setting in the patch scenario?
ParameterSetName = "DefaultParameter", | ||
Mandatory = false)] | ||
[ValidateNotNullOrEmpty] | ||
public int? SkuCapacity { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the parameter that is used for scaling? What is the reason for using a nullable type with the 'ValidateNotNullOrEmpty' attribute.. We should just use the non-nullable type, if this value cannot be null.
ValueFromPipeline = true)] | ||
[AllowNull] | ||
public VirtualMachineScaleSetUpdate VirtualMachineScaleSetUpdate { get; set; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These parameter sets are incorrect.
The intention shoudl be to
- Allow identifying the VMSS by resource group + resource name
- Allow piping in a VMSS via the pipeline and updating the parameters.
In both of these cases, ideally you would allow either patching individual properties (if they provide additional parameters), or simply writing the VMSS object.
Firther, It is not clear how the user creates a VisrtualMachienScaleSetUpdate parameter,a s this doesn't appear to be the ouput of any other cmdlet.
string resourceGroupName = this.ResourceGroupName; | ||
string vmScaleSetName = this.VMScaleSetName; | ||
BuildPatchObject(); | ||
if (this.VirtualMachineScaleSet == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this execution logic. This prevenets users from piping scale sets into this cmdlet for PATCH.
The logic we discussed was that if any of the PATCH propertires were set, we would use PATCH, otherwise PUT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
piping scenario works as expected even it calls PUT. (There is no difference between PUT and PATCH for CRP.)
The piping scenario is included in the test.
$vmss2 = $vmss | Update-AzureRmVmss -ResourceGroupName $rgname -Name $vmssName -SkuCapacity 4;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not the piping scenario. Why would I pass in resource group name and VMscaleset name when piping? The piping scenario would be:
$vmss2 | Update-AzureRmVmss -SkuCapacity 4
Also, I thought part of thsi was that the PATCH was faster than the put.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is failing with the following Static Analysis issue:
Parameter PauseTimeBetweenBatches of cmdlet Update-AzureRmVmss does not follow the enforced naming convention of using a singular noun for a parameter name.
You can copy the line from the report into the suppressions file here: https://github.com/Azure/azure-powershell/blob/preview/tools/StaticAnalysis/Exceptions/SignatureIssues.csv
@hyonholee There is a static cnalysis failure, you can update the suppressions file here: https://github.com/Azure/azure-powershell/blob/preview/tools/StaticAnalysis/Exceptions/SignatureIssues.csv |
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.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThru
parameter.Cmdlet Parameter Guidelines