Fix for https://github.com/Azure/azure-powershell/issues/5506 #5689

yolocs · 2018-03-07T00:41:24Z

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

maddieclayton · 2018-03-07T22:53:11Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

Please add a snippet about this change to your changelog to ensure it is picked up in the next release: https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/ContainerInstance/ChangeLog.md

maddieclayton · 2018-03-07T22:58:51Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs

Please change this into two parameter sets (it is better to do validation over parameter sets rather than throwing at runtime). You should have:
New-AzureRmContainerGroup
[-ResourceGroupName]
[-Name]
[-Image]
[-RegistryCredential ]
[-Location ]
[-OsType ]
[-RestartPolicy ]
[-Cpu ]
[-MemoryInGB ]
[-IpAddressType ]
[-Port <Int32[]>]
[-Command ]
[-EnvironmentVariable ]
[-RegistryServerDomain ]
[-Tag ]
[-DefaultProfile ]
[-WhatIf]
[-Confirm]
[]

and

New-AzureRmContainerGroup
[-ResourceGroupName]
[-Name]
[-Image]
[ -RegistryCredential ]
-AzureFileVolumeShareName
-AzureFileVolumeAccountCredential
-AzureFileVolumeMountPath
[-Location ]
[-OsType ]
[-RestartPolicy ]
[-Cpu ]
[-MemoryInGB ]
[-IpAddressType ]
[-Port <Int32[]>]
[-Command ]
[-EnvironmentVariable ]
[-Tag ]
[-DefaultProfile ]
[-WhatIf]
[-Confirm]
[]

@maddieclayton I have doubts here. What if we have another set of parameters that require validation but those parameters can also be used along with these 'AzureFile' parameters? That's exactly what happened for the issue associated.

As long as we can reasonable validate with parameter sets that is the best way to do this validation - in your proposed version, a user looking at the docs would have no idea that a set of parameters are required if one is provided, which a bad experience, especially while scripting. If we have an issue with this in the future we can look at it again then.

But in this case, the bad experience is already happening for registry related parameters. When a registry server is specified, the registry credential should also be specified. And now I can only validate that in runtime. What's the guideline here to determine which parameters to be prioritized to put into a parameter set? e.g. azure file parameters vs. registry paramters

A couple of notes, as I had not seen this code:

You should make Location mandatory instead of throwing an exception at runtime.
You should have these parameter sets to get rid of runtime validation over RegistryServer:
New-AzureRmContainerGroup
[-ResourceGroupName]
[-Name]
[-Image]
[-RegistryCredential ]
[-Location ]
[-OsType ]
[-RestartPolicy ]
[-Cpu ]
[-MemoryInGB ]
[-IpAddressType ]
[-Port <Int32[]>]
[-Command ]
[-EnvironmentVariable ]
[-Tag ]
[-DefaultProfile ]
[-WhatIf]
[-Confirm]

New-AzureRmContainerGroup
[-ResourceGroupName]
[-Name]
[-Image]
-RegistryServerDomain
-RegistryCredential
[-Location ]
[-OsType ]
[-RestartPolicy ]
[-Cpu ]
[-MemoryInGB ]
[-IpAddressType ]
[-Port <Int32[]>]
[-Command ]
[-EnvironmentVariable ]
[-Tag ]
[-DefaultProfile ]
[-WhatIf]
[-Confirm]

The third validation you should keep, as it cannot be done through parameter set validation. The fourth validation could be moved into parameter validation using validatescript, but this is not required. The whole purpose of parameter sets is to ensure users can figure out how to run cmdlets without having to use trial and error.

Yes, that should eliminate all of the parameter validation that can be done on the parameter set level.

I can't find any example using ValidateScript in c#. Do you have any example?

The syntax should be pretty much the same for both PowerShell and C# syntax: https://powershell.org/2013/05/21/validatescript-for-beginners/. Basically, you want the script to return true if the parameter is valid, and false otherwise. Error Message can be given like this: https://docs.microsoft.com/en-us/dotnet/api/system.management.automation.validatescriptattribute.errormessage?view=pscore-6.0.0&viewFallbackFrom=powershellsdk-1.1.0

This is not a blocking change for your PR, so it is up to you if you want to make this change.

Good to know. It seems it requires a recent version of powershell sdk.
Anyway, I add back parameter set for azure file.

maddieclayton · 2018-03-07T23:01:34Z

...ager/ContainerInstance/Commands.ContainerInstance/Models/ContainerGroupCreationParameters.cs

remove new validation, should be fixed by parameter sets.

maddieclayton · 2018-03-07T23:01:57Z

...sourceManager/ContainerInstance/Commands.ContainerInstance/help/New-AzureRmContainerGroup.md

regenerate this file once parameter sets are fixed.

yolocs · 2018-03-12T17:47:33Z

@maddieclayton ping

cormacpayne

@yolo3301 can we add test coverage to prevent the linked issue from resurfacing in the future?

cormacpayne · 2018-03-12T18:21:02Z

src/ResourceManager/ContainerInstance/ChangeLog.md

@yolo3301 you don't need this version header -- you can just put this content under the ## Current Release header

maddieclayton · 2018-03-12T21:23:37Z

...nager/ContainerInstance/Commands.ContainerInstance/Commands/NewAzureContainerGroupCommand.cs


        [Parameter(
-            Mandatory = true,
-            ParameterSetName = CreateContainerGroupWithRegistryParamSet,


Can you add the third parameter set we discussed in preview comments? Looks like you only have two at the moment.

I tried to but I don't think it can work. The problem is there is no unique mandatory parameter for Registry. You might think RegistryServer should be the unique mandatory one. But we actually support omitting it if the Image is from azure container registry, which means RegistryServer is not mandatory. Am I right?

You might also argue I can use RegistryCredential as the unique mandatory parameter. However, if that's present, there is nothing else to validate because RegistryServer is optional as described above.

Your new parameter set would have both RegistryServer and RegistryCredential mandatory. Any case where RegistryServer is given, RegistryCredential must be also given, and this parameter set covers this case. Any other case where RegistryServer is not specified will be covered by the first parameter set (which will not contain RegistryServer and RegistryCredential will be optional).

In my test, it doesn't validate Registry parameters at all. Here are the parameter sets:

SYNTAX New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-RegistryServerDomain <string>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -RegistryCredential <pscredential> -RegistryServerDomain <string> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> -AzureFileVolumeMountPath <string> [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-RegistryServerDomain <string>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]

With above parameter sets, if I have:

New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryCredential $registryCredential

It won't fail while I expect it asks me to give -RegistryServer. On the other hand, if I have:

New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryServer "registry.io"

It gives me runtime error while I expect a parameter set check.

Any idea?

It is supported. Registry parameters can be used along with FileVolume ones.
Back to my first comment, in our case, almost every parameter can be used with other parameters. The validation needs happen in a way like: when parameter A is present, parameter B must be present too. But at the same time A & B can be used with every other parameter.

Okay, these should be your parameter sets then:

PS1: All parameters that don't need validation (don't include FileVolume or Registry) - all parameters are optional and this is the default
PS2: All parameters that don't need validation, and the two registry parameters as required.
PS3: All parameters that don't need validation, and the FileVolume parameters as required.
PS4: All parameters that don't need validation, and both the registry and FileVolume parameters as required.

OK. Confirming the following is what you suggested:

SYNTAX New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -RegistryServerDomain <string> -RegistryCredential <pscredential> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -RegistryServerDomain <string> -RegistryCredential <pscredential> -AzureFileVolumeMountPath <string> -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -AzureFileVolumeMountPath <string> -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]

With these parameter sets, the following valid commands won't work as expected:

New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryCredential $registryCredential # Parameter set cannot be resolved using the specified named parameters. New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryServerDomain "abc.io" # Parameter set cannot be resolved using the specified named parameters. New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryCredential $registryCredential -AzureFileVolumeShareName $shareName -AzureFileVolumeAccountCredential $accountCredential -AzureFileVolumeMountPath $mountPath # Cannot process command because of one or more missing mandatory parameters: RegistryServerDomain.

The problem is -RegistryServer is not always required. And -RegistryCredential is mandatory if and only if -RegistryServer is present. When -RegistryServer is not present, -RegistryCredential is optional. So I also tried the following parameter sets:

SYNTAX New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -RegistryServerDomain <string> -RegistryCredential <pscredential> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -RegistryServerDomain <string> -RegistryCredential <pscredential> -AzureFileVolumeMountPath <string> -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -AzureFileVolumeMountPath <string> -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]

It can make the above 2 commands work as expected. But there is still one command doesn't work as expected:

New-AzureRmContainerGroup -ResourceGroupName $resourceGroupName -Name $containerGroupName -Image $image -RegistryServerDomain "abc.io" # Parameter set cannot be resolved using the specified named parameters. # This one doesn't give the right error message, it should complain -RegistryCredential can't be null

Thoughts?

Okay, as PowerShell can't resolve the parameter set, you should keep the validation for registryserverdomain/registrycredential in the executecmdlet statement. Please keep the individual sets for the Volume share cmdlets:

New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> [-RegistryServerDomain <string>] ` [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] ` [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] ` [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>] New-AzureRmContainerGroup [-ResourceGroupName] <string> [-Name] <string> [-Image] <string> -AzureFileVolumeMountPath <string> ` -AzureFileVolumeShareName <string> -AzureFileVolumeAccountCredential <pscredential> [-RegistryServerDomain <string>] ` [-RegistryCredential <pscredential>] [-Location <string>] [-OsType {Linux | Windows}] [-RestartPolicy {Always | Never | OnFailure}] [-Cpu <int>] ` [-MemoryInGB <double>] [-IpAddressType {Public}] [-DnsNameLabel <string>] [-Port <int[]>] [-Command <string>] [-EnvironmentVariable <hashtable>] ` [-Tag <hashtable>] [-DefaultProfile <IAzureContextContainer>] [-WhatIf] [-Confirm] [<CommonParameters>]

I think that's what's already in the PR.

maddieclayton · 2018-03-19T23:50:55Z

on demand here: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/632/ (and 633)

azuresdkci added the needs-review label Mar 7, 2018

maddieclayton suggested changes Mar 7, 2018

View reviewed changes

maddieclayton assigned yolocs Mar 7, 2018

maddieclayton added needs-revision and removed needs-review labels Mar 7, 2018

cormacpayne added needs-review and removed needs-revision labels Mar 12, 2018

cormacpayne assigned maddieclayton and unassigned yolocs Mar 12, 2018

cormacpayne reviewed Mar 12, 2018

View reviewed changes

maddieclayton assigned yolocs and unassigned maddieclayton Mar 12, 2018

maddieclayton added needs-revision and removed needs-review labels Mar 12, 2018

yolocs added 3 commits March 12, 2018 12:23

remove parameter sets

edd2a32

add back azure file vol para set

eadca31

add test

920e5ba

yolocs force-pushed the fix_reg branch from af08796 to 920e5ba Compare March 12, 2018 21:19

maddieclayton suggested changes Mar 12, 2018

View reviewed changes

Merge branch 'preview' into fix_reg

b68ce7d

maddieclayton previously approved these changes Mar 19, 2018

View reviewed changes

maddieclayton removed the needs-revision label Mar 19, 2018

maddieclayton unassigned yolocs Mar 19, 2018

maddieclayton added the Waiting for On Demand label Mar 19, 2018

Merge branch 'preview' into fix_reg

3961eba

cormacpayne dismissed maddieclayton’s stale review via 3961eba March 20, 2018 15:43

cormacpayne approved these changes Mar 20, 2018

View reviewed changes

maddieclayton merged commit 6a893ee into Azure:preview Mar 20, 2018

Fix for https://github.com/Azure/azure-powershell/issues/5506 #5689

Fix for https://github.com/Azure/azure-powershell/issues/5506 #5689

Uh oh!

Conversation

yolocs commented Mar 7, 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!

maddieclayton Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yolocs commented Mar 12, 2018

Uh oh!

cormacpayne left a comment

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!

yolocs Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

maddieclayton commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yolocs commented Mar 7, 2018 •

edited

Loading

maddieclayton Mar 8, 2018 •

edited

Loading

yolocs Mar 12, 2018 •

edited

Loading