-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Adding new API version for Microsoft.Web RP which fixes swagger errors #2802
Conversation
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-goThe initial PR has been merged into your service PR: |
Automation for azure-libraries-for-javaThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-nodeThe initial PR has been merged into your service PR: |
@AutorestCI rebuild azure-sdk-for-python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany generally cool, see comments. Anything starting with [informational]
is more of a statement, not a request for action on your side, but please review real quick if those changes are expected 🙂
Note that github (STILL!!!???) has this annoying bug of showing comments on any line >2048 on line 2048... so for WebApps
, I also tried annotating which type/operation I meant.
"summary": "Renew a domain.", | ||
"description": "Renew a domain.", | ||
"operationId": "Domains_Renew", | ||
"parameters": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[informational] example removed!?
"204": { | ||
"description": "Domain does not exist." | ||
}, | ||
"400": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[informational] changed from default
to explicit status codes (note that this changes the SDK's behavior!)
} | ||
} | ||
}, | ||
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/hostingEnvironments/{name}/changeVirtualNetwork": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note to self] new operation
], | ||
"summary": "Resume an App Service Environment.", | ||
"description": "Resume an App Service Environment.", | ||
"operationId": "AppServiceEnvironments_SyncVirtualNetworkInfo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[note to self] new operation
"200": { | ||
"description": "Succesfully deleted web app backup item.", | ||
"schema": { | ||
"$ref": "#/definitions/BackupItem" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[informational] added response schema (so you return the deleted item? cool!)
"$ref": "./CommonDefinitions.json#/definitions/ProxyOnlyResource" | ||
} | ||
], | ||
"properties": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[informational] geoRegion
property removed
"sourceWebAppId" | ||
], | ||
"type": "object", | ||
"properties": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[informational] ignoreQuotas
property removed
"SystemAssigned", | ||
"UserAssigned" | ||
], | ||
"x-ms-enum": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany type: string
removed! On purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olydis ManagedServiceIdentity is now a flag enumeration. What is the correct swagger representation of such an enumeration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naveedaz how would that enumeration be sent over the wire? I suspect there is no good Swagger representation, but assuming that you want something like "SystemAssigned|UserAssigned"
to be sent, that's still a string so it must be modeled like that 🙂
}, | ||
"x-ms-client-flatten": true | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany isLinux
? 😄 I don't know the details of this, but sounds like a year from now there could be an isMacOS
or similar - wouldn't it make more sense to introduce an enum property that just says platform
or something? May well be that that makes no sense here, just dumping my brain 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olydis - while I agree that enum probably would be better, for now it is a bool. Should we add another platform later then we'll rework the API to be enum. As far as I know there are currently no plans for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me 🙂
@@ -0,0 +1,264 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany is this the first version of this Swagger? Or is there a previous API version? Examples for this presumably new API are missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany We should not expose the ClassicMobileServices.json. It is not really useful for customers and was only used during transition (Zumo -> ARM web apps).
@ravbhatnagar new API version, mostly resolved linter errors |
@olydis The "underscore_separated_words" is for the KUDU APIs (https://github.com/projectkudu/kudu/wiki/REST-API). Due to a bug in the swagger generator, they came out as pascalCasing earlier, but the actual API follows the "underscore_separated_words" convention. So the swagger was incorrect before. |
Signing off from ARM side since these are all existing APis already deployed. |
"id": "/subscriptions/34adfa4f-cedf-4dc0-ba29-b6d1a69ab345/providers/Microsoft.DomainRegistration/topLevelDomains/com", | ||
"name": "com", | ||
"type": "Microsoft.DomainRegistration/topLevelDomains", | ||
"location": "global", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruslany @naveedaz This example (and that for TopLevelDomains_Get
) is failing validation right now since the type here is modeled as a ProxyOnlyResource
which doesn't have a location
field. It this intended or do you think that
a) maybe there is a slight modeling bug or
b) these examples should not have location
on them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from that, please acknowledge whether all changes I point out in my first review (removed example, property changes, etc.) are on purpose. Note that this is a real issue, please fix it 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olydis are you referring to the "ManagedServiceIdentity is now a flag enumeration"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct - just responded to the comment, depending on how that flag enum is supposed to be sent over the wire, it has to be documented accordingly. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Outstanding discussion: Adding new API version for Microsoft.Web RP which fixes swagger errors #2802 (comment)
-
Also, there seems to be a slight issue with example
ListTopLevelDomainAgreements.json
: The operation requires aagreementOption
parameter, but none is provided in the example.
@olydis - fixed the ListTopLevelDomainAgreements example |
@ruslany Looking good, thanks! :) #2802 (comment) remains - note that this is critical, no user-friendly code is generated for this as it is right now. In C#: /// <summary>
/// Gets or sets type of managed service identity.
/// </summary>
[JsonProperty(PropertyName = "type")]
public object Type { get; set; } Cool. What can I set this to now? |
@olydis - fixed the type for "ManagedServiceIdentityType", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your efforts everyone! 🙂
@olydis - thanks for the thorough review and feedback! |
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: AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
1 similar comment
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: AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback Thanks for your co-operation. |
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
api-version
in the path should match theapi-version
in the spec).Quality of Swagger