add params to New-Azvm

[grizzlytheodore]

adding '-SshKeyName' and 'GenerateSshKey' parameters to New-AzVm
and fixing small bug in New-AzSshKey

Description

Checklist

• I have read the Submitting Changes section of CONTRIBUTING.md
• The title of the PR is clear and informative
• The appropriate ChangeLog.md file(s) has been updated:
  • 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 -- no new version header should be added
• 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:
  • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • {Please put the link here}
  • the markdown help files have been regenerated using the commands listed here

[VeryEarly]

if so, please add a new parameter set where -Linux, -SshKeyName, are required And remove this if block

[grizzlytheodore]

indentations are fixed in this file

[grizzlytheodore]

@VeryEarly thank you for all your comments, I really appreciate it looking at it thoroughly.
I've addressed most of your comments except for the ones advising new parameter sets.

So you want me to add two new parameter sets branch off of DefaultParameterSet and SimpleParameter set that has '-Linux', '-SshKeyName' and '-GenerateSshKey'?

Could you elaborate on why that would be better?

I am hesitant because

1. Current way of checking the OS is sufficient and works fine.
2. wouldn't that add more room for error like: '-Linux' is given but '-Image' or OSRprofile is not Linux.
3. we were to add two new parameter sets for this occasion to such a commonly used cmdlet with so many parameters, someday we will end up with countless parameter sets for this cmdlet.

Should I consult a PM?

[VeryEarly]

@VeryEarly thank you for all your comments, I really appreciate it looking at it thoroughly.
I've addressed most of your comments except for the ones advising new parameter sets.

So you want me to add two new parameter sets branch off of DefaultParameterSet and SimpleParameter set that has '-Linux', '-SshKeyName' and '-GenerateSshKey'?

Could you elaborate on why that would be better?

I am hesitant because

1. Current way of checking the OS is sufficient and works fine.
2. wouldn't that add more room for error like: '-Linux' is given but '-Image' or OSRprofile is not Linux.
3. we were to add two new parameter sets for this occasion to such a commonly used cmdlet with so many parameters, someday we will end up with countless parameter sets for this cmdlet.

Should I consult a PM?

please correct me if I'm wrong, the exception thrown at line 452 (NewAzureVMCommand.cs), won't be executed until line 625 (NewAzureVMCommand.cs) was reached:

result = await client.RunAsync(client.SubscriptionId, parameters, asyncCmdlet);

At this point, some other resources should be already created (storage/network? need your confirmation).

This is too heavy for a parameter level behavior, that is to say, this exception can be discovered at the very beginning when cmdlet was invoked (using parameter set) while not to call any actual business logic.

Please let me know if you have any concern.

[grizzlytheodore]

@VeryEarly thank you for all your comments, I really appreciate it looking at it thoroughly.
I've addressed most of your comments except for the ones advising new parameter sets.
So you want me to add two new parameter sets branch off of DefaultParameterSet and SimpleParameter set that has '-Linux', '-SshKeyName' and '-GenerateSshKey'?
Could you elaborate on why that would be better?
I am hesitant because

1. Current way of checking the OS is sufficient and works fine.
2. wouldn't that add more room for error like: '-Linux' is given but '-Image' or OSRprofile is not Linux.
3. we were to add two new parameter sets for this occasion to such a commonly used cmdlet with so many parameters, someday we will end up with countless parameter sets for this cmdlet.

Should I consult a PM?

please correct me if I'm wrong, the exception thrown at line 452 (NewAzureVMCommand.cs), won't be executed until line 625 (NewAzureVMCommand.cs) was reached:

result = await client.RunAsync(client.SubscriptionId, parameters, asyncCmdlet);

this part is right

At this point, some other resources should be already created (storage/network? need your confirmation).

but here no. Other resources are not created. the PowerShell sends one payload to Azure on this line 625, and in the backend, the storage resources, network resources, and VM are all created together. Many of the storage and network properties will default to a value in the backend because the simple parameter set provides very little bit of that.

This is too heavy for a parameter level behavior, that is to say, this exception can be discovered at the very beginning when cmdlet was invoked (using parameter set) while not to call any actual business logic.

Please let me know if you have any concern.

[VeryEarly]

@VeryEarly thank you for all your comments, I really appreciate it looking at it thoroughly.
I've addressed most of your comments except for the ones advising new parameter sets.
So you want me to add two new parameter sets branch off of DefaultParameterSet and SimpleParameter set that has '-Linux', '-SshKeyName' and '-GenerateSshKey'?
Could you elaborate on why that would be better?
I am hesitant because

1. Current way of checking the OS is sufficient and works fine.
2. wouldn't that add more room for error like: '-Linux' is given but '-Image' or OSRprofile is not Linux.
3. we were to add two new parameter sets for this occasion to such a commonly used cmdlet with so many parameters, someday we will end up with countless parameter sets for this cmdlet.

Should I consult a PM?

please correct me if I'm wrong, the exception thrown at line 452 (NewAzureVMCommand.cs), won't be executed until line 625 (NewAzureVMCommand.cs) was reached:

result = await client.RunAsync(client.SubscriptionId, parameters, asyncCmdlet);

this part is right

At this point, some other resources should be already created (storage/network? need your confirmation).

but here no. Other resources are not created. the PowerShell sends one payload to Azure on this line 625, and in the backend, the storage resources, network resources, and VM are all created together. Many of the storage and network properties will default to a value in the backend because the simple parameter set provides very little bit of that.

This is too heavy for a parameter level behavior, that is to say, this exception can be discovered at the very beginning when cmdlet was invoked (using parameter set) while not to call any actual business logic.
Please let me know if you have any concern.

Make sense.

I suggest to

• Based on your implementation I'll say when user want to use ssh key, the parameter "sshkeyname" is required, so for if statement you are using like !String.IsNullOrEmpty(_cmdlet.SshKeyName) || _cmdlet.GenerateSshKey, should all be changed to !String.IsNullOrEmpty(_cmdlet.SshKeyName)
• create a method privatevoid Validate() for class Parameters to handle all these logics (check whether sshkeyname/generated are provided, if linux, and other new parameters in the future)
• put this method in the constructor of Parameters, remove all if blocks that validate parameters
• create another method called cleanup or whatever, to handle all the remove generated ssh key logic (and other clean up jobs in the future), invoke this method in catch exceptions whenever exceptions happened (in your case, if cloudexception, remove generated sshkey)
• make "addSshPublicKey" and "SshKeyForLinux" method of class Parameters

The reason I suggested to do so is because these business logics are not supposed to be blended with the concrete strategy, please let me know if you have any concerns.

[grizzlytheodore]

@VeryEarly thank you for all your comments, I really appreciate it looking at it thoroughly.
I've addressed most of your comments except for the ones advising new parameter sets.
So you want me to add two new parameter sets branch off of DefaultParameterSet and SimpleParameter set that has '-Linux', '-SshKeyName' and '-GenerateSshKey'?
Could you elaborate on why that would be better?
I am hesitant because

1. Current way of checking the OS is sufficient and works fine.
2. wouldn't that add more room for error like: '-Linux' is given but '-Image' or OSRprofile is not Linux.
3. we were to add two new parameter sets for this occasion to such a commonly used cmdlet with so many parameters, someday we will end up with countless parameter sets for this cmdlet.

Should I consult a PM?

please correct me if I'm wrong, the exception thrown at line 452 (NewAzureVMCommand.cs), won't be executed until line 625 (NewAzureVMCommand.cs) was reached:

result = await client.RunAsync(client.SubscriptionId, parameters, asyncCmdlet);

this part is right

At this point, some other resources should be already created (storage/network? need your confirmation).

but here no. Other resources are not created. the PowerShell sends one payload to Azure on this line 625, and in the backend, the storage resources, network resources, and VM are all created together. Many of the storage and network properties will default to a value in the backend because the simple parameter set provides very little bit of that.

This is too heavy for a parameter level behavior, that is to say, this exception can be discovered at the very beginning when cmdlet was invoked (using parameter set) while not to call any actual business logic.
Please let me know if you have any concern.

Make sense.

I suggest to

• Based on your implementation I'll say when user want to use ssh key, the parameter "sshkeyname" is required, so for if statement you are using like !String.IsNullOrEmpty(_cmdlet.SshKeyName) || _cmdlet.GenerateSshKey, should all be changed to !String.IsNullOrEmpty(_cmdlet.SshKeyName)
• create a method privatevoid Validate() for class Parameters to handle all these logics (check whether sshkeyname/generated are provided, if linux, and other new parameters in the future)
• put this method in the constructor of Parameters, remove all if blocks that validate parameters
• create another method called cleanup or whatever, to handle all the remove generated ssh key logic (and other clean up jobs in the future), invoke this method in catch exceptions whenever exceptions happened (in your case, if cloudexception, remove generated sshkey)
• make "addSshPublicKey" and "SshKeyForLinux" method of class Parameters

The reason I suggested to do so is because these business logics are not supposed to be blended with the concrete strategy, please let me know if you have any concerns.

@VeryEarly
Thank you for all the advice. I see how this will be a cleaner design.

I do have a concern about the last bullet point. The default parameter set does not use the Parameters class, and I still need to use "addSshPublicKey" and "SshKeyForLinux" for it. Shown

azure-powershell/src/Compute/Compute/VirtualMachine/Operation/NewAzureVMCommand.cs

Line 739 in 5a488b6

parameters = addSshPublicKey(parameters);

any other suggestion on how I could structure it better?

[grizzlytheodore]

@VeryEarly I've implemented all your suggestions except for the last bullet point. I've made validate() a method of the cmdlet class.

[VeryEarly]

@VeryEarly I've implemented all your suggestions except for the last bullet point. I've made validate() a method of the cmdlet class.

I see, it might need more refactor to this cmdlet then, I'm good with current implementation, thanks for your quick response.
Please check the new comments, it caused test case failures.

[grizzlytheodore]

@VeryEarly pls review again!

[grizzlytheodore]

@VeryEarly pls merge