-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Removed the publishingcredentials API and re-added the publishingUsers API #925
Conversation
…. Make SCM type read/write. Add Linux App Fx version.
…Node CLI SDK. Add config snapshots API. Add valid values for SCMType.
Please adjust all |
Fixed the folder structure. |
@@ -207,76 +271,6 @@ | |||
} | |||
} | |||
}, | |||
"/subscriptions/{subscriptionId}/providers/Microsoft.Web/publishingCredentials": { |
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.
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.
Intentional. This API is not per ARM spec and ARM does not let it through. So its not breaking anyone.
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.
@salameer @ravbhatnagar - what is the ARM guideline for removing an API from an existing api-version ?
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.
FYI: The resourceType 'publishingcredentials' is not in the ARM manifest for Microsoft.Web. So this is not an API that anyone can actually call into.
Here is the url.
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.
Ah I see. I remember seeing an issue posted by a node sdk customer. Azure/azure-sdk-for-node#1909 (comment) . Ruslan recommended using this GET /providers/Microsoft.Web/publishingUsers/web?api-version=2015-08-01
instead.
Btw, is the above api present for the new api-version (Just confirming)?
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.
Not yet. These APIs need to be redesigned properly.
], | ||
"parameters": [ | ||
{ | ||
"name": "requestMessage", |
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.
If the parameter is named "userDetails" then this would be better. It gives better developer experience.
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.
Fixed.
No modification for NodeJS |
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