-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Add support for Express Route Provider APIs #2532
Add support for Express Route Provider APIs #2532
Conversation
A new top level NRP resource ExpressRouteCrossConnection is being introduced with this change. This resource shadows the customer ExpressRouteCircuit resouce in the customer subscription and enables the provider to perform CRUD operations on some of the sub-resources, such as peerings on the ExpressRoute Circuit * update readme.md to include the complete path for the 2018-02-01 folder * add examples * add ExpressRouteCrossConnection as a top level resource
Automation for azure-sdk-for-pythonNothing to generate for azure-sdk-for-python |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
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.
Posted a few comments. Please check CI jobs and resolve issues if any
"tags": [ | ||
"ExpressRouteCrossConnections" | ||
], | ||
"operationId": "ExpressRouteCrossConnections_ListAll", |
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.
Recommended operationId for list operations is ExpressRouteCrossConnections_List
"tags": [ | ||
"ExpressRouteCrossConnections" | ||
], | ||
"operationId": "ExpressRouteCrossConnectionsByResourceGroup_List", |
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.
recommended operationId is ExpressRouteCrossConnections_ListByResourceGroup
"in": "body", | ||
"required": true, | ||
"schema": { | ||
"$ref": "./network.json#/definitions/TagsObject" |
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.
nit: fix indentation
Shouldn't this parameter be named tags
instead?
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.
Fixed indentation. The naming is consistent with corresponding operation on ExpressRoute Circuit
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.
Thank you for helping with the reviews.
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.
parameters
is still to generic, please name it something more meaningful
} | ||
}, | ||
"x-ms-examples": { | ||
"Update Express Route CrossConnection Tags": { |
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.
Remove spaces (basic Json keys' semantics)
"tags": [ | ||
"ExpressRouteCrossConnectionArpTable" | ||
], | ||
"operationId": "ExpressRouteCrossConnections_ListArpTable", |
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.
Rename as ExpressRouteCircuitsArpTable_list
"description": "The ExpressRouteCircuit" | ||
}, | ||
"serviceProviderProvisioningState": { | ||
"type": "string", |
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.
Provisioning states are usually readonly. Please confirm.
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.
The serviceProviderProvisioning state is writeable by the ExpressRoute Provider subscription.
}, | ||
"provisioningState": { | ||
"type": "string", | ||
"description": "Gets the provisioning state of the public IP resource. Possible values are: 'Updating', 'Deleting', and 'Failed'." |
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.
Definitely readonly
"$ref": "./network.json#/definitions/Resource" | ||
} | ||
], | ||
"description": "ExpressRouteCrossConnection resource" |
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.
nit: indentation
}, | ||
"nextLink": { | ||
"type": "string", | ||
"description": "The URL to get the next set of results." |
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.
mark as readonly
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.
Should the nextLink record be marked readOnly ? A quick browse through some of the other files does not show nextLink marked as readOnly. Please advise and I will update.
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.
Yes, please mark this as readonly
"in": "path", | ||
"required": true, | ||
"type": "string", | ||
"description": "The subscription credentials which uniquely identify the Microsoft Azure subscription. The subscription ID forms part of the URI for every service call." |
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.
nit: indentation
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.
A few more comments.
} | ||
}, | ||
"ExpressRouteCrossConnectionServiceProviderProperties": { | ||
"properties": { |
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.
Similarly, please go through all the models and their properties in the spec and verify their readonly
-ness
}, | ||
"nextLink": { | ||
"type": "string", | ||
"description": "The URL to get the next set of results." |
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.
Yes, please mark this as readonly
"in": "body", | ||
"required": true, | ||
"schema": { | ||
"$ref": "./network.json#/definitions/TagsObject" |
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.
parameters
is still to generic, please name it something more meaningful
"tags": [ | ||
"ExpressRouteCrossConnections" | ||
], | ||
"operationId": "ExpressRouteCrossConnections_UpdateTags", |
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.
Rename this as ExpressRouteCrossConnectionsTags_Update
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.
A minor comment and please delete the .swp file
"properties": { | ||
"properties": { | ||
"x-ms-client-flatten": true, | ||
"$ref": "#/definitions/ExpressRouteCrossConnectionPropertiesFormat" |
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.
nit: please rename this as ExpressRouteCrossConnectionProperties
} | ||
}, | ||
"x-ms-examples": { | ||
"UpdateExpressRouteCrossConnectionTags": { |
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.
nit: fix indentation
"200": { | ||
"description": "Request successful. The operation returns the resulting ExpressRouteCrossConnectionsRouteTableSummary resource.", | ||
"schema": { | ||
"$ref": "./expressRouteCircuit.json#/definitions/ExpressRouteCircuitsRoutesTableSummaryListResult" |
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.
nit: Fix indentation
], | ||
"responses": { | ||
"200": { | ||
"description": "Request successful. The operation returns the resulting ExpressRouteCrossConnectionsRouteTable resource.", |
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.
Indentation
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.
There 2 more violations you need to fix.
- Not all operations have corresponding examples. Please provide them.
- Please verify whether information related to the service appears in the List_Operations API for network
A few examples are still missing, please check here |
How do I verify the following ? |
@gimotwanMSFT disregard that comment |
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.
LGTM
… 2018 (#2642) * Add support for Express Route Circuit Connection resource as a child of Express Route Circuit Peering in 2018-02-01 API (#2358) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * Merge Express Route Circuit Connection to Networking2018-02-01 (#2370) * copy all files from 2018-01-01 to 2018-02-01 * update api version to 2018 in all files * add express route circuit connection as child of express route bgp peering * update readme.md * add examples * fix travis build error * fix travis build error * fix indentation errors * update reference to circuit connections in peering * fix indentation * fix indentation * fix indentation' * get latest file * Fix indentation * fix key * fix indentation and example * fix indentation * fix indentation * make circuit con singular * add fix as PR #2364 (#2376) * Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. (#2521) * 1443089: Network feature: Setting custom ipsec policy for Virtual Network Gateway P2S clients. * 1443089:Fix network ReadMe file. * 1443089:Fix network ReadMe file. * Temporary bug fix * Add support for Express Route Provider APIs (#2532) * Add support for Express Route Provider APIs A new top level NRP resource ExpressRouteCrossConnection is being introduced with this change. This resource shadows the customer ExpressRouteCircuit resouce in the customer subscription and enables the provider to perform CRUD operations on some of the sub-resources, such as peerings on the ExpressRoute Circuit * remove wrong enum values from circuit connection status and fix enum … (#2572) * remove wrong enum values from circuit connection status and fix enum name * Update ExpressRouteCrossConnection Route Table Summary Record (#2574) * Update CrossConnection Route Table Summary record * [Network-2018-02-01] DDoS Protection Plan resource (#2567) * Initial version * Add eol * Add extra property in example * Add default values for fields * Rename operation IDs * Make 'id' read-only for all network resources * Revert "Make 'id' read-only for all network resources" * Fix reference * making peer express route circuit peering and express route circuit peering as references (#2696) * Express Route Cross Connection Swagger changes as per PM requirements (#2644) * Fixed operationIds for non-CRUD operations in ExpressRouteCrossConnections (#2720) * Making travis run > 10 mins (#2739)
A new top level NRP resource ExpressRouteCrossConnection is being
introduced with this change. This resource shadows the customer
ExpressRouteCircuit resouce in the customer subscription and enables
the provider to perform CRUD operations on some of the
sub-resources, such as peerings on the ExpressRoute Circuit