Skip to content
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.

Sign up for GitHub

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

Fixing global parameters for siterecovery and resourcehealth #1753

Closed
wants to merge 5 commits into from
Closed

Fixing global parameters for siterecovery and resourcehealth #1753

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a49385
siterecovery should have resourcegroupName and resourcename as method…
amarzavery Sep 27, 2017
7b69658
resourceType should be a method parameter and not a client property. …
amarzavery Sep 27, 2017
8983664
adding "x-ms-parameter-location": "method" extension to global parame…
amarzavery Sep 28, 2017
024fd4f
replacing special character � with ' as this causes jshint issues in …
amarzavery Sep 28, 2017
a80c2da
Updating documentation as per review feedback.
amarzavery Sep 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...yservicessiterecovery/resource-manager/Microsoft.RecoveryServices/2016-08-10/service.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14122,14 +14122,16 @@
"in": "path",
"description": "The name of the resource group where the recovery services vault is present.",
"required": true,
"type": "string"
"type": "string",
"x-ms-parameter-location": "method"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what issue was not having this causing? what's the breaking change on generated code due to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new SiteRecoveryManagementClient(credentials, subscriptionId, resourceGroupName, resourceName);
This makes the client require resourceGroupName and resourceName in the client constructor. Both of these become client properties and that is incorrect. I had filed an issue in the linter repo about this. If it is management-plane swagger then the linter should warn if it sees anything else apart from subscriptionId and apiVersion and does not have x-ms-parameter-location set to method. Azure/azure-openapi-validator#84

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As responded over the mail thread:

As per our scenarios, customers are more likely to work in the context of a single ResourceName, so, It makes more sense to have ResourceGroupName and ResourceName in global parameters section for recoveryservicessiterecovery specification. For more context we have:

  1. https://github.com/Azure/azure-rest-api-specs/blob/current/specification/recoveryservices/resource-manager/Microsoft.RecoveryServices/2016-06-01/vaults.json which defines CRUD on ResourceName and hence, ResourceGroupName and ResourceName are already method parameters.
  2. However, specification at https://github.com/Azure/azure-rest-api-specs/tree/current/specification/recoveryservicessiterecovery/resource-manager is mostly sused by customer in the context of a single ResourceName. So, initializing ResourceGroupName and ResourceName at the time of client initialization aligns better to our customer scenario.

Moreover, any change as suggested in the thread would be a breaking change for our existing clients like PowerShell and Ibiza (requires passing ResourceGroupName and ResourceName explicitly as input to every call rather than just once at sdk client initialization) as I can understand, which we would not be able to take at this point.
Let us know if we are missing some point to understand here.

Thanks,
Avneesh

},
"ResourceName": {
"name": "resourceName",
"in": "path",
"description": "The name of the recovery services vault.",
"required": true,
"type": "string"
"type": "string",
"x-ms-parameter-location": "method"
},
"ApiVersion": {
"name": "api-version",
Expand Down