Add PS CmdLets for Express Route Circuit Connection Resource #6094
Conversation
|
Tested End to End on NRP Slice : "\scratch2\scratch\dedhar\pstesting.txt" |
|
PS Signed Build also passes : https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/ps-sign/533/ |
|
do I need to update this file since we are adding new commands ? https://github.com/Azure/azure-powershell/blob/preview/src/ResourceManager/Network/ChangeLog.md -- Done fixed in latest commit |
|
@dedhar Yes, please update the changelog. Please also fill out a cmdlet review here: https://github.com/Azure/azure-powershell-cmdlet-review-pr |
|
Started filling out cmdlet review here : https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/66 |
|
Can someone from PS Team please review this https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/66 |
|
@maddieclayton / @markcowl : do you have any comments? |
|
@dedhar You have ci-build failure and merge conflict. |
|
Fix the build failure related to unwanted changes to common.ps1 |
| } | ||
|
|
||
| [Fact(Skip = "Express Route Circuits need to be manually provisioned by Service Providers and the whole setup can't be automated.")] | ||
| [Trait(Category.AcceptanceType, Category.CheckIn)] |
There was a problem hiding this comment.
why include the test and skip it right away?
There was a problem hiding this comment.
Today to enable Azure Private Peering on an Express Route Circuit, a service provider needs to mark the Circuit as provisioned which is a manual step. we are planning to add a check in backend server code to skip this check so that PS tests can be automated. The fix is merged but yet to be rolled. Once the change is rolled we will fix the test for Circuit Connection.
| { | ||
| "Entries": [ | ||
| { | ||
| "RequestUri": "/subscriptions/8c992d64-fce9-426d-b278-85642dfeab03/resourceGroups/dedharpspeer/providers/Microsoft.Network/expressRouteCircuits/dedharcktpeer?api-version=2018-02-01", |
There was a problem hiding this comment.
Why do you need the recorded file if you skip the test?
There was a problem hiding this comment.
I added the recorded file to store the output for circuit connection creation and deletion commands
| #> | ||
| function Test-ExpressRouteCircuitConnectionCRUD | ||
| { | ||
| $circuitName = "dedharcktpeer" |
There was a problem hiding this comment.
why do you need the test if you skip it?
There was a problem hiding this comment.
Adding a test for future reference so that we fix the test case once the backend code is released.
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
Add-AzureRmExpressRouteCircuitConnectionConfig [-Name] [-ExpressRouteCircuit] [-PeerExpressRouteCircuitPeering] [-AddressPrefix] [[-AuthorizationKey] ]
ExpressRouteCircuit parameter is declared as required positional - please add a position.
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
Get-AzureRmExpressRouteCircuitConnectionConfig [-Name] [-ExpressRouteCircuit]
ExpressRouteCircuit parameter is declared as required positional - please add a position.
|
|
||
| ## EXAMPLES | ||
| ### Example 1: Display the circuit connection configuration for an ExpressRoute circuit | ||
|
|
There was a problem hiding this comment.
You may want to add some examples including piping scenario example we discussed in the design review here.
| Remove-AzureRmExpressRouteCircuitConnectionConfig -Name $circuitConnectionName -ExpressRouteCircuit $circuit_init | ||
| Set-AzureRmExpressRouteCircuit -ExpressRouteCircuit $circuit_init | ||
| ``` | ||
|
|
There was a problem hiding this comment.
You may want to add a piping scenario example we discussed in the design review here.
| HelpMessage = "The name of the Circuit Connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public override string Name { get; set; } | ||
|
|
There was a problem hiding this comment.
The Name property - the base class AzureExpressRouteCircuitConnectionConfigBase has already declared the same property
There was a problem hiding this comment.
Yes the property is virtual in the base class. Modeled it on other Express Route Circuit Resources : Circuit, Peering, Route Filter etc.
| HelpMessage = "Private IP Addresses to create VxLAN Tunnels")] | ||
| [ValidateNotNullOrEmpty] | ||
| public string AddressPrefix { get; set; } | ||
|
|
There was a problem hiding this comment.
For all positional required properties (declared like this: CmdletName [ParamName] <ParamType> in the design review/Syntax changes/New Cmdlets section) Position property should be set.
There was a problem hiding this comment.
AddressPrefix is required property. Do all required properties need to have position?
There was a problem hiding this comment.
Resolved based on offline conversation with Vladimir.
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
Remove-AzureRmExpressRouteCircuitConnectionConfig [-Name] [-ExpressRouteCircuit]
ExpressRouteCircuit parameter is declared as a position required - so the Position property should be set.
|
Tested latest changes in Express Route NRP Slice AllowClassicOperations : False AddressPrefix : 90.0.0.0/29 AllowClassicOperations : False |
|
Removed the unnecessary ValueFromPipelineByPropertyName from Set Express Route Circuit command, re tested the changes and updated PR |
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
for the property
public PSExpressRouteCircuit ExpressRouteCircuit { get; set; }
why do you need
ValueFromPipelineByPropertyName = true,
?
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
for the property
public PSExpressRouteCircuit ExpressRouteCircuit { get; set; }
why do you need
ValueFromPipelineByPropertyName = true,
| HelpMessage = "Express Route Circuit Peering intiating connection")] | ||
| [ValidateNotNullOrEmpty] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
for the property
public PSExpressRouteCircuit ExpressRouteCircuit { get; set; }
why do you need
ValueFromPipelineByPropertyName = true,
| ValueFromPipelineByPropertyName = true, | ||
| HelpMessage = "The ExpressRouteCircuit")] | ||
| public PSExpressRouteCircuit ExpressRouteCircuit { get; set; } | ||
|
|
There was a problem hiding this comment.
Why do you need ValueFromPipelineByPropertyName = true, here?
|
Removed Value From pipeline property name from all circuit connection commands and re tested changes |
7 participants
Description
This PR adds support for Add, New, Set, Remove, Get CmdLet for Express Route Circuit Connection Resource. Express Route Circuit Connection would be a child of Express Route Peering resource and grand child of Express Route Circuit Resource. Customers would be required to create both Express Route Circuits and Private Peerings on them before creating a Circuit Connection Resource to peer two Express Route Circuits.
Checklist
CONTRIBUTING.mdplatyPSmodule