Added ReachabilityProviderList and ReachabilityReport cmdlets + tests #4880
Conversation
There was a problem hiding this comment.
@irrogozh a couple of minor comments. In addition:
-
Please update the Network change log to reflect the changes that are being made in this cmdlet.
-
Please generate and commit the markdown files associated with the new cmdlets you are adding so there is help content available for users.
| Mandatory = false, | ||
| HelpMessage = "Optional Azure regions to scope the query to.", | ||
| ValueFromPipelineByPropertyName = true)] | ||
| public List<string> AzureLocations { get; set; } |
There was a problem hiding this comment.
@irrogozh please rename this parameter to AzureLocation to avoid using a plural noun. I would go ahead and just rename this parameter to Location to be consistent with all other cmdlets that have a parameter that accepts Azure locations.
| Mandatory = true, | ||
| HelpMessage = "The name of the network watcher resource group.", | ||
| ParameterSetName = "SetByName", | ||
| ValueFromPipelineByPropertyName = true)] |
There was a problem hiding this comment.
@irrogozh please remove the ValueFromPipelineByPropertyName attribute from this parameter- ResourceId should be the only parameter that has this attribute
| HelpMessage = "The name of network watcher.", | ||
| ParameterSetName = "SetByName", | ||
| ValueFromPipeline = true, | ||
| ValueFromPipelineByPropertyName = true)] |
There was a problem hiding this comment.
@irrogozh please remove both the ValueFromPipeline and ValueFromPipelineByPropertyName attributes from this parameter - ResourceId should be the only parameter that has the ValueFromPipelineByPropertyName attribute
|
|
||
| namespace Microsoft.Azure.Commands.Network.Automation | ||
| { | ||
| [Cmdlet(VerbsCommon.Get, "AzureRmNetworkWatcherReachabilityProvidersList", DefaultParameterSetName = "SetByResource"), OutputType(typeof(PSAvailableProvidersList))] |
There was a problem hiding this comment.
@irrogozh I think the default parameter set should be SetByName, so we prompt the user to provide a NetworkWatcherName and ResourceGroupName rather than the NetworkWatcher object
|
|
||
| namespace Microsoft.Azure.Commands.Network.Automation | ||
| { | ||
| [Cmdlet(VerbsCommon.Get, "AzureRMNetworkWatcherReachabilityReport", DefaultParameterSetName = "SetByResource"), OutputType(typeof(PSAzureReachabilityReport))] |
There was a problem hiding this comment.
@irrogozh same comment about using the SetByName parameter set as the default parameter set for this cmdlet
| HelpMessage = "The name of network watcher.", | ||
| ParameterSetName = "SetByName", | ||
| ValueFromPipeline = true, | ||
| ValueFromPipelineByPropertyName = true)] |
There was a problem hiding this comment.
@irrogozh same comment about removing ValueFromPipeline and ValueFromPipelineByPropertyName attributes from this parameter
| Mandatory = true, | ||
| HelpMessage = "The name of the network watcher resource group.", | ||
| ParameterSetName = "SetByName", | ||
| ValueFromPipelineByPropertyName = true)] |
There was a problem hiding this comment.
@irrogozh same comment about removing the ValueFromPipelineByPropertyName attribute from this parameter
| Mandatory = false, | ||
| HelpMessage = "List of Internet service providers.", | ||
| ValueFromPipelineByPropertyName = true)] | ||
| public List<string> Providers { get; set; } |
There was a problem hiding this comment.
@irrogozh please rename this parameter to Provider to avoid using a plural noun
| Mandatory = false, | ||
| HelpMessage = "Optional Azure regions to scope the query to.", | ||
| ValueFromPipelineByPropertyName = true)] | ||
| public List<string> AzureLocations { get; set; } |
There was a problem hiding this comment.
@irrogozh same comment about renaming this parameter to Location
|
@irrogozh please make sure to pull in the latest changes from the |
| @@ -20,6 +20,10 @@ | |||
| ## Current Release | |||
|
|
|||
| ## Version 4.4.1 | |||
| * Added cmdlet to list available internet service providers for a specified Azure region | |||
There was a problem hiding this comment.
@irrogozh please move this snippet you added to be under the ## Current Release header
| [Parameter( | ||
| Mandatory = false, | ||
| HelpMessage = "The name of the state.", | ||
| ValueFromPipelineByPropertyName = true)] |
There was a problem hiding this comment.
@irrogozh please remove ValueFromPipelineByPropertyName from this parameter for consistency with other parameters.
| # Get-AzureRMNetworkWatcherReachabilityReport | ||
|
|
||
| ## SYNOPSIS | ||
| {{Fill in the Synopsis}} |
There was a problem hiding this comment.
@irrogozh please fill in any templates in the markdown files you've added
| ] | ||
| } | ||
| ] | ||
|
|
There was a problem hiding this comment.
@irrogozh it looks like you're missing ending ``` in this example
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
ShouldProcessand haveSupportShouldProcess=truespecified in the cmdlet attribute. You can find more information onShouldProcesshere.OutputTypeattribute if any output is produced - if the cmdlet produces no output, it should implement aPassThruparameter.Cmdlet Parameter Guidelines