Skip to content

Added ProviderOperations endpoint and fixed description warnings#2220

Merged
marstr merged 4 commits intoAzure:masterfrom
ajay-arora:master
Jan 9, 2018
Merged

Added ProviderOperations endpoint and fixed description warnings#2220
marstr merged 4 commits intoAzure:masterfrom
ajay-arora:master

Conversation

@ajay-arora
Copy link
Copy Markdown
Member

@ajay-arora ajay-arora commented Jan 5, 2018

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@azuresdkciprbot
Copy link
Copy Markdown

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/devtestlabs/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 5
After the PR: Warning(s): 0 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link
Copy Markdown

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/devtestlabs/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 5
After the PR: Warning(s): 0 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@marstr marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 9, 2018
}
},
"security": [ { "oauth2": [ "user_impersonation" ] } ],
"security": [ { "azure_auth": [ "user_impersonation" ] } ],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Taking a look at the Redis swagger, I believe that in addition to renaming the security line from oauth2 to azure_auth, one must define a "securityDefintions" clause.

More info here from the spec: https://swagger.io/specification/#securitySchemeObject

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, i already have that in line 8717

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Excellent, thanks for the link. Sorry I didn't see it!

"paths": {
"/providers/Microsoft.DevTestLab/operations": {
"get": {
"tags": [ "ProviderOperations" ],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In all of the other specs I'm looking at, the tag is just Operations not ProviderOperations.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Operations are already used in the end point for polling long running operations. I think ProviderOperations makes it clear.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ravbhatnagar, do you have thoughts on the name of the Operations group? It sounds like we're sorta flexible here, which is fine with me. I just want to make sure that's true though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@marstr - Do you mean the operationId? or the tags value. The operationId should be consistent between teams and I am not sure how tags is used. So I dont have a recommendation on that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think its used by the autorest tool to segregate the generated code

Copy link
Copy Markdown
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

Looks good from my perspective, I'll approve one @ravbhatnagar or @simongdavies have approved.

}
},
"security": [ { "oauth2": [ "user_impersonation" ] } ],
"security": [ { "azure_auth": [ "user_impersonation" ] } ],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Excellent, thanks for the link. Sorry I didn't see it!

@ravbhatnagar
Copy link
Copy Markdown
Contributor

Looks good.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jan 9, 2018
@AutorestCI
Copy link
Copy Markdown

@AutorestCI
Copy link
Copy Markdown

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

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants