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

Add Microsoft.Web ResourceHealthMetadata swagger #2588

Merged
merged 6 commits into from
Mar 19, 2018
Merged

Add Microsoft.Web ResourceHealthMetadata swagger #2588

merged 6 commits into from
Mar 19, 2018

Conversation

pavelzel
Copy link
Contributor

@pavelzel pavelzel commented Mar 3, 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

pavelzel added 2 commits March 2, 2018 17:08
Adding a new set of Resource Health Metadata API swagger files for Microsoft.Web
…wagger file

Update the Microsoft.Web autorest config to test validity of the ResourceHealthMetadata swagger file
@AutorestCI
Copy link

AutorestCI commented Mar 3, 2018

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#2065

@AutorestCI
Copy link

AutorestCI commented Mar 3, 2018

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#1336

@fearthecowboy fearthecowboy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Mar 5, 2018
@fearthecowboy
Copy link
Member

Greetings @pavelzel !

These look like new APIs, so I'm going to ask @ravbhatnagar to do a quick review as well.

Straight off they look pretty good; were you going to add x-ms-examples for these APIs?

Adding examples for the Microsoft.Web ResourceHealthMetdata APIs
@AutorestCI
Copy link

AutorestCI commented Mar 7, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@pavelzel
Copy link
Contributor Author

pavelzel commented Mar 7, 2018

@fearthecowboy Added x-ms-examples as requested

@pavelzel
Copy link
Contributor Author

pavelzel commented Mar 8, 2018

@fearthecowboy @ravbhatnagar , is there anything else you need from me for this PR?

@fearthecowboy
Copy link
Member

Your examples are failing validation: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/350123757

"$ref": "#/parameters/apiVersionParameter"
}
],
"responses": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add "default" response to all APIs to capture error cases. Schema complies with the ARM RPC error contract. example from Batch RP swagger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ravbhatnagar This looks to be a newer requirement for swagger files. Since we (Microsoft.Web) auto-generate our swagger files from code, is it okay to not have the "default" response for this file while we work on adding support for this in the auto-generation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed up the responses to have a default error response

"properties": {
"description": "ResourceHealthMetadata resource specific properties",
"properties": {
"category": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a candidate for an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a string as per spec provided by the RHC team, relevant part of spec:
Category (string) - this is the category of the sku that you have communicated to RHC. RHC Rp will select the policy based on this value. If the sku's category does not match existing categories, we will fall back to the default category

"description": "The category that the resource matches in the RHC Policy File",
"type": "string"
},
"signalAvailability": {
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally this should be an enum with multiple states. Currently you might just require true or false for this but in future as scenario evolves you might want to indicate more states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as category, this is a boolean as per the spec I was given by RHC team:
SignalAvailability (boolean) - This field is to notify us if the signal for a resource is reaching us or not. This can come into picture when you are aware that a particular set of resources (possibly belonging a test region or a legacy region) are not sending RHC signals. When this is false, we will put the corresponding resource into unknown events bucket.

@ravbhatnagar
Copy link
Contributor

@fearthecowboy - no blocking feedback from my side. Feel free to merge based on response to above comments.

@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 Mar 9, 2018
The location should not have been populated for the Microsoft.Web ResourceHealthMetadata API examples. The single ResourceHealthMetadata GET call should return ResourceHealthMetadata and not a collection of them
@AutorestCI
Copy link

AutorestCI commented Mar 13, 2018

Automation for azure-libraries-for-java

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-libraries-for-java#327

Adding the default errror responses to the Microsoft.Web ResourceHealthMetadata API swagger
For Microsoft.Web ResourceHealthMetadata APIs renaming the default error response from ErrorResponse to DefaultErrorResponse to fix the python SDK generation. The DefaultErrorResponse will be the default error response for our other APIs in the future as well
@jhendrixMSFT
Copy link
Member

@AutorestCI rebuild azure-sdk-for-go

@pavelzel
Copy link
Contributor Author

@fearthecowboy is there anything else you need from me for this PR?

@fearthecowboy
Copy link
Member

Looks good to me @pavelzel -- you want me to merge this now?

@fearthecowboy fearthecowboy added the Approved-OkToMerge <valid label in PR review process>add this label when assignee approve to merge the updates label Mar 19, 2018
@pavelzel
Copy link
Contributor Author

@fearthecowboy Yes, please go ahead and merge. Thank you!

@azuresdkciprbot
Copy link

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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

File: specification/web/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 22
After the PR: Warning(s): 0 Error(s): 26

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@jhendrixMSFT
Copy link
Member

@AutorestCI rebuild azure-sdk-for-go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved-OkToMerge <valid label in PR review process>add this label when assignee approve to merge the updates 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.

6 participants