Skip to content

Comments

add Get-AzVmssVmRunCommand on SDK based cmdlet #19840

Merged
VeryEarly merged 4 commits intomainfrom
addVmRunCommand
Oct 21, 2022
Merged

add Get-AzVmssVmRunCommand on SDK based cmdlet #19840
VeryEarly merged 4 commits intomainfrom
addVmRunCommand

Conversation

@grizzlytheodore
Copy link
Contributor

@grizzlytheodore grizzlytheodore commented Oct 18, 2022

Description

This is mirroring what we already did to Get-AzVMRunCommand in the last release: #19272

Get-AzVmssVMRunCommand was originally autorest.powershell generated. But I am removing that from compute.autorest in this pr:
#19826

We are moving it to cmdlet built on SDK because we wanted to achieve two things:
Have all the fields show as a list format when we get back list of RunCommands
Have instanceView only show when -Expand InstanceView

So to achieve these customizations I moved it to cmdlet built on SDK here.

There is no breaking change here because currently from the documentation https://learn.microsoft.com/en-us/powershell/module/az.compute/get-azvmssvmruncommand?view=azps-9.0.0, 1st, 2nd, and 5th(last) parameter sets arent working

And the one parameter set we are adding in this PR is a combination of 3rd and 4th parameter sets by having -RunCommandName as optional.

Checklist

  • SHOULD select appropriate branch. Cmdlets from Autorest.PowerShell should go to generation branch.
  • SHOULD make the title of PR clear and informative, and in the present imperative tense.
  • SHOULD update ChangeLog.md file(s) appropriately
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense. Add changelog in description section if PR goes into generation branch.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD have approved design review for the changes in this repository (Microsoft internal only) with following situations
    • Create new module from scratch
    • Create new resource types which are not easy to conform to Azure PowerShell Design Guidelines
    • Create new resource type which name doesn't use module name as prefix
    • Have design question before implementation
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT introduce breaking changes in Az minor release except preview version.
  • SHOULD NOT adjust version of module manually in pull request

"Az.Compute","Get-AzVmssVMRunCommand","Get-AzVmssVMRunCommand","0","1060","The parameter set 'List' for cmdlet 'Get-AzVmssVMRunCommand' is no longer the default parameter set.","Change the default parameter for cmdlet 'Get-AzVmssVMRunCommand' back to 'List'."
"Az.Compute","Get-AzVmssVMRunCommand","Get-AzVmssVMRunCommand","0","1060","The parameter set 'List' for cmdlet 'Get-AzVmssVMRunCommand' is no longer the default parameter set.","Change the default parameter for cmdlet 'Get-AzVmssVMRunCommand' back to 'List'."
"Az.Compute","Get-AzVmssVMRunCommand","Get-AzVmssVMRunCommand","0","1060","The parameter set 'List' for cmdlet 'Get-AzVmssVMRunCommand' is no longer the default parameter set.","Change the default parameter for cmdlet 'Get-AzVmssVMRunCommand' back to 'List'."
"Az.Compute","Get-AzVmssVMRunCommand","Get-AzVmssVMRunCommand","0","2000","The cmdlet 'Get-AzVmssVMRunCommand' no longer supports the parameter 'SubscriptionId' and no alias was found for the original parameter name.","Add the parameter 'SubscriptionId' back to the cmdlet 'Get-AzVmssVMRunCommand', or add an alias to the original parameter name."
Copy link
Collaborator

@VeryEarly VeryEarly Oct 19, 2022

Choose a reason for hiding this comment

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

line 10 is breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrote in the description above, but that parameter set was not working to begin with

Copy link
Collaborator

Choose a reason for hiding this comment

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

please try set VirtualMachineScaleSetVMRunCommandsClient.SubscriptionId
before this line:

VirtualMachineRunCommand vmssVmRc = VirtualMachineScaleSetVMRunCommandsClient.Get(this.ResourceGroupName, this.VMScaleSetName, this.InstanceId, this.RunCommandName, this.Expand);

@@ -0,0 +1,23 @@
"AssemblyFileName","ClassName","Target","Severity","ProblemId","Description","Remediation"
"Az.Compute","Get-AzVmssVMRunCommand","Get-AzVmssVMRunCommand","0","1020","The cmdlet 'Get-AzVmssVMRunCommand' no longer has output type 'Microsoft.Azure.PowerShell.Cmdlets.Compute.Models.Api20210701.IVirtualMachineRunCommand'.","Make cmdlet 'Get-AzVmssVMRunCommand' return type 'Microsoft.Azure.PowerShell.Cmdlets.Compute.Models.Api20210701.IVirtualMachineRunCommand'."
Copy link
Collaborator

Choose a reason for hiding this comment

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

please list differences between IVirtualMachineRunCommand and PSVirtualMachineRunCommand to prove there are no breaking changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see IVirtualMachineRunCommand here:

internal partial interface IVirtualMachineRunCommandInternal :

see PSVirtualMachineRunCommand here:

all the properties match

Copy link
Collaborator

@VeryEarly VeryEarly Oct 20, 2022

Choose a reason for hiding this comment

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

these two objects do not match, please check carefully, properties in IVirtualMachineRunCommand are flatten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see. you are right.

It is a little different and the return type will be changing. but the move from IVirtualMachineRunCommandInternal return type to PSVirtualMachineRunCommand has already been done for Get-AzVMRunCommand,

and essentially the current Get-AzVmssVMRunCommand is broken in numerous different places, so we are improving customer experience by making this transition from autorest generated to sdk based. You've already merged the PR that is removing this cmdlet from autorest, please let me know if this PR can be merged to complete the transfer despite the change in return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also @dingmeng-xue is aware of this migration, but here is the reasonings I sent him via email:

  1. The feature owner wanted the returned output as list format.
    a. Even when we specify -RunCommandName to retrieve 1 RunCommand resource, it is returned in table format. The having ~10 properties that the user needed to see, the table format did not provide all the information needed for the users due to the PowerShell window size constraints.
  2. Have instanceView properties only shown when -Expand is provided.
    a. The output was always showing “IntanceView” properties. So when “-Expand InstanceView” was not provided, those properties were always empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and 2a is the result of having a flat hierchy

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it, breaking changes on broken cmdlets won't be considered as blocker.

@VeryEarly VeryEarly self-assigned this Oct 19, 2022
@grizzlytheodore
Copy link
Contributor Author

please see my responses @VeryEarly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants