-
Notifications
You must be signed in to change notification settings - Fork 4k
Azure Machine Learning WebServicesRP new API version (Re-submit) #3860
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.
@NonStatic2014 a couple of clarifying comments
VerbsCommon.Add, | ||
"AzureRmMlWebServiceRegionalProperty", | ||
SupportsShouldProcess = true)] | ||
[OutputType(typeof(void))] |
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.
@NonStatic2014 please change the output type to WebService
since that is what you are writing to the output stream
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.
Good catch. Forget change it when addressing another comment before.
public string Name { get; set; } | ||
|
||
[Parameter( | ||
Mandatory = false, |
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.
@NonStatic2014 is there a default region if one is not provided?
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.
Not really. It is a necessary input for this cmdlet.
|
||
[Parameter(Mandatory = true, HelpMessage = "The name of the web service")] | ||
[ValidateNotNullOrEmpty] | ||
public string Name { 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.
@NonStatic2014 I think Name
and ResourceGroupName
should have the ValueFromPipelineByPropertyName
attribute so you are able to pipe the result of Get-AzureRmMlWebService
to this cmdlet. Does that seem like a scenario that a user may use this cmdlet for?
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.
Not always. Name and ResourceGroupName are also necessary input for Get- cmdlet. So I would expect user input them here.
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.
@NonStatic2014 it's a simple piping scenario that would prevent the user from having to type the same set of parameters twice. For example:
Get-AzureRmMlWebService -Name Foo -ResourceGroupName RGFoo | Add-AzureRmMlWebServiceRegionalProperty
@azuresdkci retest this please |
@azuresdkci retest this please |
Description
Azure Machine Learning WebServicesRP new API version
Original PR: #3796
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