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

Jump to bottom

added files for new api version 2017-03-31 (#1) #1054

Merged
merged 6 commits into from
Mar 27, 2017
Merged

added files for new api version 2017-03-31 (#1) #1054

merged 6 commits into from
Mar 27, 2017

Conversation

kejiun
Copy link
Contributor

@kejiun kejiun commented Mar 21, 2017 •
edited
Loading

  • added files for new api version 2017-03-31
  • only releasing new version number, no api/signature changes

@msftclas
Copy link

@kejiun,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

sarangan12
sarangan12 requested changes Mar 22, 2017
View reviewed changes
arm-advisor/2017-03-31/swagger/advisor.json
}
],
"responses": {
"202": {
Copy link
Member

Choose a reason for hiding this comment

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

Here you are giving the status 202 which is accepted. But what is the final status? 200? If so, please include it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The design of this API will be revisited after GA.

arm-advisor/2017-03-31/swagger/advisor.json Outdated
"GenerateRecommendations"
],
"description": "Retrieves the status of the recommendation computation or generation process. Invoke this API after calling the generation recommendation. The URI of this API is returned in the Location field of the response header.",
"operationId": "Recommendations_GetGenerateRecommendationsStatus",
Copy link
Member

Choose a reason for hiding this comment

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

Are you particular in this name? Can it be names GenerateRecommendationsStatus_Get which is what we usually recommend

arm-advisor/2017-03-31/swagger/advisor.json
}
],
"responses": {
"202": {
Copy link
Member

Choose a reason for hiding this comment

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

Here you mention 202 which is accepted. Now, how will I poll back to get the response? Which URI to hit? Usually it is obtained from the response of the 202. But there is no response here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The design of this API will be revisited after GA.

arm-advisor/2017-03-31/swagger/advisor.json
"Suppressions"
],
"description": "Obtains the details of a suppression.",
"operationId": "Suppressions_Get",
Copy link
Member

Choose a reason for hiding this comment

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

Does this operation provide a list of suppressions for one recommendations? In that case, the recommendation is to use the id Suppressions_ListByRecommendations

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 one returns only one suppression

arm-advisor/2017-03-31/swagger/advisor.json Outdated
],
"responses": {
"200": {
"description": "OK.",
Copy link
Member

Choose a reason for hiding this comment

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

Provide a valid description

arm-advisor/2017-03-31/swagger/advisor.json Outdated
],
"responses": {
"200": {
"description": "OK.",
Copy link
Member

Choose a reason for hiding this comment

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

Provide a vaid description

arm-advisor/2017-03-31/swagger/advisor.json
"description": "The list of snoozed and dismissed rules for the recommendation.",
"type": "array",
"items": {
"format": "uuid",
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to avoid usage of uuids?

Copy link
Contributor Author

@kejiun kejiun Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

may I have the reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hank and I had a conversation on this. The design of this API will be revisited after GA. There may not be a need for suppressionIds. But lets not block GA due to this. This is read-only for now and service generates it. So user does not have to pass in the GUID.

arm-advisor/2017-03-31/swagger/advisor.json
}
}
},
"ResourceRecommendationBase": {
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm if this is a resource? If so, it should have an allOf of a resource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@sarangan12
Copy link
Member

Running the swagger against linter, I get the following errors:

ERROR: Per the Noun_Verb convention for Operation Ids, the noun 'Recommendations' should not appear after the underscore.
Path: ..\advisor.json#/paths/subscriptions/{subscriptionId}/providers/Microsoft.Advisor/generateRecommendations/{operationId}/get/operationId
ERROR: Top level properties should be one of name, type, id, location, properties, tags, plan, sku, etag, managedBy, identity. Extra properties found: "SuppressionContract/suppressionId, SuppressionContract/ttl".
Path: ..\advisor.json#/paths/{resourceUri}/providers/Microsoft.Advisor/recommendations/{recommendationId}/suppressions/{name}
ERROR: Tracked Resource failing validation is: "SuppressionContract". Validation Failed: 2.
A Tracked Resource must have:
1. A Get Operation
2. A ListByResourceGroup operation with x-ms-pageable extension and
3. A ListBySubscriptionId operation with x-ms-pageable extension.
4. "type","location","tags" should not be used in the RP property bag named "properties".
Path: ..\advisor.json#/definitions
ERROR: Tracked resource 'SuppressionContract' must have patch operation that at least supports the update of tags.
Path: ..\advisor.json#/definitions

Please fix them

@sarangan12
Copy link
Member

Based on the offline communications, I see some of the API design issues will be handled after GA release. The documentation issues seem to be resolved now. Approving these

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@sarangan12 sarangan12 merged commit f431986 into Azure:master Mar 27, 2017
mccleanp pushed a commit that referenced this pull request Mar 23, 2022
@jlautman @sanjaiganesh
Remove hostnameprefix from properties and make checknameavailability … (
a85c88f 
#1054)

* Remove hostnameprefix from properties and make checknameavailability regional

* Remove 201 response declaration from the swagger spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ravbhatnagar ravbhatnagar ravbhatnagar left review comments

@sarangan12 sarangan12 sarangan12 approved these changes

Assignees

@sarangan12 sarangan12

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kejiun @msftclas @sarangan12 @azuresdkci @ravbhatnagar