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.

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

New rule: verify the parameters defined in the global parameters section #84

Closed
amarzavery opened this issue Aug 4, 2017 · 17 comments
Closed
Assignees
Milestone

Comments

@amarzavery
Copy link

amarzavery commented Aug 4, 2017

Autorest treats the parameters defined in the global parameters section as client properties. Hence one needs to be super sure when they are adding a parameter over there.

90% scenario is that subscriptionId and api-version are parameters that should be defined in the lobal parameters section.

However, one can define a parameter that is being referenced in multiple operations (example: resourceGroupName) in the global parameters section and apply the extension "x-ms-parameter-location": "method". This will then not be a client property.

It would be nice to have a linter rule that validates this aspect and warns the user.

CognitiveSerivices api has location as a global parameter and SDKs have shipped with this. It seems odd to pass location while instantiating the client.

@amarzavery
Copy link
Author

amarzavery commented Aug 10, 2017

Another instance that proves that this should definitely be a linter rule.
someone added resourceGroupName and expressRouteConnectorName as global parameters and the reviewer merged it. By having this linter rule we will make it easier for reviewers.

@amarzavery
Copy link
Author

And one more PR in the management plane was merged incorrectly Azure/azure-rest-api-specs@bc175db#diff-e895aa4ceab43a039f4baf6e7b2bf7c0R3146

amarzavery referenced this issue in Azure/azure-rest-api-specs Sep 28, 2017
* Release ServiceFabric 2017-07-01-preview swagger specification

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.

* Release ServiceFabric 2017-07-01-preview swagger specification

This change introduces applicationType, version, application, and service ARM resources for the ServiceFabric resource provider.
@lmazuel
Copy link
Member

lmazuel commented Oct 12, 2017

Another example: Azure/azure-rest-api-specs#1829

@lmazuel
Copy link
Member

lmazuel commented Oct 13, 2017

New example: Azure/azure-rest-api-specs#1860

@lmazuel
Copy link
Member

lmazuel commented Oct 24, 2017

@veronicagg veronicagg changed the title A linter rule to verify the parameters defined in the global parameters section New rule: verify the parameters defined in the global parameters section Nov 15, 2017
@mboersma
Copy link
Member

Apparently I got bitten by this issue as well in Azure/azure-cli#5086 but luckily @lmazuel is watching out for this.

Adding a linter to rule to catch this in the future would be 💯.

@lmazuel
Copy link
Member

lmazuel commented Jan 10, 2018

Again: Azure/azure-rest-api-specs#2052

@lmazuel
Copy link
Member

lmazuel commented Jan 10, 2018

@salameer At some point we need to prioritize this, this is really time-consuming... :(

@salameer
Copy link
Member

@veronicagg @sarangan12 @mcardosos

THis seems to be a rule that benefits our group and it's efficiency can we please prioritize this?

@veronicagg
Copy link
Collaborator

@salameer I've just tagged it with Sprint-113. We haven't been updating rules recently or scheduling any work for them, it'd be good to triage and select which ones we'd like to tackle next and how it prioritizes with other work.

@salameer
Copy link
Member

salameer commented Jan 10, 2018 via email

@lmazuel
Copy link
Member

lmazuel commented Jan 25, 2018

Again Azure/azure-rest-api-specs#2289

@amarzavery
Copy link
Author

For Heaven's sake, please implement this linter rule.
This Consumption API PR was merged incorrectly. It has resourceGroupName and budgetName as client properties. They should be method parameters.
Assign this issue on priority to someone and implement this linter rule.
/cc @salameer

@lmazuel
Copy link
Member

lmazuel commented Feb 6, 2018

Again Azure/azure-rest-api-specs#2433

@lmazuel
Copy link
Member

lmazuel commented Feb 6, 2018

Seriously, now when I type "validator" in my Chrome bar, it suggest me directly this issue....

@lmazuel
Copy link
Member

lmazuel commented Feb 6, 2018

Again Azure/azure-rest-api-specs#2391

@sarangan12
Copy link
Member

The new Rule XmsParameterLocation has been implemented. Refer PR #149 for details.

The dotnet classic open api validator is scheduled for release on Monday, March 19, 2018. The code changes are complete. Refer [PR #150][https://github.com//pull/150] for further details.

All the existing errors have already been fixed in the specs repository. Refer PR #2649 for details.

There is no pending action items (of course the actual release is pending until 19th) in this issue. Resolving it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants