-
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
[API Management] Revert Cross Referencing for Doc Team #1064
Conversation
@solankisamir Please fix the model validator error
|
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.
please fix the model validation error.
@vishrutshah updated the PR. |
@solankisamir Thanks for updating it. I've a question for you. Can you please let us know the purpose of this swagger i.e Is it only for documentation or are you also planning to have SDKs out of this? |
@vishrutshah currently this swagger is only for documentation purposes, but we want to start generating SDKs out of this. Problem currently is the OpenAPI Spec 2.0 currently doesn't fully represent our API interface. We are waiting for Open Api Spec 3.0 to be incorporated into AutoRest for complete SDK generation. |
@solankisamir Could you please fix following: apimdeployment.json file references |
@vishrutshah apimdeployment.json is another file, which we need not to have any cross-references. You can check here 2016-07-07 |
@solankisamir Cool I see that you are removing that referencing into this PR changes. that's great. 👍 |
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 were RPCViolations detected from the linter.
BodyTopLevelProperties | M3006 | ApiContract | Extra Properties : "ApiContract/serviceUrl, ApiContract/path, ApiContract/protocols, ApiUpdateContract/serviceUrl, ApiUpdateContract/path, ApiUpdateContract/protocols"
BodyTopLevelProperties | M3006 | OperationContract| Extra Properties: “OperationContract/method, OperationContract/urlTemplate, OperationUpdateContract/method, OperationUpdateContract/urlTemplate”
OperationsAPIImplementationValidation | M3023| Operations API must be implemented for '/providers/Microsoft.ApiManagement/operations’
ProvidersPathValidation | Violating Path - /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ApiManagement/service/{serviceName}/tenant/configuration/syncState
ProvidersPathValidation | Violating Path - /subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ApiManagement/service/{serviceName}/tenant/configuration/validate
. . .
@solankisamir Did you receive any approved exceptions on these?
"operationId": "Apis_Get", | ||
"description": "Gets the details of the API specified by its identifier.", | ||
"produces": [ | ||
"application/json", |
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.
Does the service really support all these application formats ?
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.
@vishrutshah yes the api returns these formats, when making a request with Accept header specified for these format.
"schema": { | ||
"$ref": "#/definitions/ApiContract" | ||
}, | ||
"headers": { |
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.
could you please help me understand why are we defining headers
in response
section? What is purpose on documentation and what is expected from SDK?
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.
@vishrutshah The ETag response-header field provides the current value of the entity tag for the requested variant 14.19. The headers used with entity tags are described in sections 14.24, 14.26 and 14.44
{ | ||
"$ref": "#/parameters/ServiceNameParameter" | ||
}, | ||
|
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.
@ravbhatnagar Is this url
top level property allowed?
Rules: M3006 | BodyTopLevelProperties | RPCViolation
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.
This violates Resource Provider Contract (RPC). @solankisamir - Were your APIs reviewed and approved by the ARM team/Azure API review board.
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.
@ravbhatnagar we will update our contracts for the next version of the API. Can you provide us exception for this API version?
{ | ||
"$ref": "#/parameters/ServiceNameParameter" | ||
}, | ||
|
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.
@ravbhatnagar Is this url
top level property allowed?
Rules: M3006 | BodyTopLevelProperties | RPCViolation
{ | ||
"$ref": "#/parameters/ServiceNameParameter" | ||
}, | ||
|
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.
This property is missing description
{ | ||
"$ref": "#/parameters/ServiceNameParameter" | ||
}, | ||
|
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.
description is missing for property for requestId
{ | ||
"$ref": "#/parameters/ServiceNameParameter" | ||
}, | ||
|
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.
id
, name
& type
properties must be refactored into Resource
model.
@vishrutshah - had an email conversation with @solankisamir and they will address the RPC violations in the next API version update. You can ignore those for now. |
Issue #1060 for reference |
#sign-off |
No modification for NodeJS |
No modification for Ruby |
No modification for Python |
This is just adding a complete spec to what was merged earlier #1049. The Docs team don't support cross-referencing yet. It is just meant for them.
Added file apimanagement-all.json which is an aggregation of all contracts
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger