Address the issues with the redis API spec/examples found by new validation#883
Conversation
|
(note: pending agreement on whether exposing accessKeys this way is the best approach) |
vishrutshah
left a comment
vishrutshah
left a comment
There was a problem hiding this comment.
@TimLovellSmith I've reviewed the changes & left some minor comments.
There are some other errors, I've discovered apart from the changes you have made using AutoRest Linter but I am just going to list here for your reference. We'd strongly recommend fixing them as well for better SDK experiences.
WARNING: NonAppJsonTypeWarning - Media types other than 'application/json' has limited support
Path: #/Consumes[2]
WARNING: NonAppJsonTypeWarning - Media types other than 'application/json' has limited support
Path: #/Produces[2]
WARNING: UniqueResourcePaths - More than one resource path is not allowed in a single spec
Path: #/Paths
WARNING: LongRunningResponseValidation - An operation with x-ms-long-running-operation extension must have a valid terminal success status code. 200 or 201 for Put/Patch. 200, 201 or 204 for Post. 200 or 204 or both for Delete.
Path: #/Paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Cache/Redis/{name}/import/post/Extensions/x-ms-long-running-operation
WARNING: LongRunningResponseValidation - An operation with x-ms-long-running-operation extension must have a valid terminal success status code. 200 or 201 for Put/Patch. 200, 201 or 204 for Post. 200 or 204 or both for Delete.
Path: #/Paths/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Cache/Redis/{name}/export/post/Extensions/x-ms-long-running-operation
Secondly, based on email thread regarding accessKeys I am going to wait on approval of this PR until we have talk/decision with @hovsepm.
Thanks!
There was a problem hiding this comment.
Do you really need this (Message) to be PascalCase?
There was a problem hiding this comment.
I added this in the example due to an unfortunate fact that it is returning pascal case on the wire today.
We did not formally document this member before the exercise with examples. In fact when we reviewed this REST API we did not document any response body. Perhaps the best thing to do here is document the API as returning an empty response, and treat the fact that RP returns a Message property as a bug?
There was a problem hiding this comment.
@vishrutshah my take, revised: it should never have been PascalCase but now that it is, it can't be fixed without it being breaking, so its stuck that way til next api-version.
There was a problem hiding this comment.
@TimLovellSmith I understand your concerns and makes sense to wait until next api-version. It's be great if you could please open and issue on that here and from your side as well, we do not miss this on next api-version.
There was a problem hiding this comment.
Do you really need this (Message) to be PascalCase?
TimLovellSmith
force-pushed
the
redis-example-fixes
branch
from
c678274 to
7072a5d
Compare
…dation. Fix some linter errors from redis swagger, mainly around the redis Import/Export actions.
TimLovellSmith
force-pushed
the
redis-example-fixes
branch
from
7072a5d to
d43750c
Compare
|
@vishrutshah any further changes required? |
|
@TimLovellSmith Changes looks good to me. @amarzavery If you have time to skim through, please skim or I can merge it by tomorrow :) |
|
ping @amarzavery @vishrutshah Ready to take this one? |
|
looking at this PR right now |
|
LGTM |
6 participants
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger