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

[API Management] Revert Cross Referencing for Doc Team #1064

Merged
merged 3 commits into from
Mar 30, 2017
Merged

[API Management] Revert Cross Referencing for Doc Team #1064

merged 3 commits into from
Mar 30, 2017

Conversation

solankisamir
Copy link
Member

@solankisamir solankisamir commented Mar 24, 2017 •
edited by azuresdkci
Loading

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

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

  • I have read the contribution guidelines.
  • My spec meets the review criteria:
    • The spec conforms to the Swagger 2.0 specification.
    • Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
    • The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

@vishrutshah
Copy link
Contributor

@solankisamir Please fix the model validator error

  Azure swagger model validation using x-ms-examples and examples in spec
error: Found errors in validating the response with statusCode "200" for x-ms-example "ApiManagementServiceGetNetworkStatus" in operation "NetworkStatus_GetByService".:
 
{ code: 'RESPONSE_VALIDATION_ERROR',
  message: 'Found errors in validating the response with statusCode "200" for x-ms-example "ApiManagementServiceGetNetworkStatus" in operation "NetworkStatus_GetByService".',
  innerErrors: 
   [ { code: 'INVALID_RESPONSE_BODY',
       errors: 
        [ { code: 'OBJECT_MISSING_REQUIRED_PROPERTY',
            params: [ 'error' ],
            message: 'Missing required property: error',
            path: [ 'connectivityStatus', '1' ],
            description: 'Details about connectivity to a resource.' },
          { code: 'OBJECT_MISSING_REQUIRED_PROPERTY',
            params: [ 'error' ],
            message: 'Missing required property: error',
            path: [ 'connectivityStatus', '0' ],
            description: 'Details about connectivity to a resource.' } ],
       message: 'Invalid body: Value failed JSON Schema validation',
       path: [] } ] }
{ AssertionError: swagger "arm-apimanagement/2016-10-10/swagger/apimanagement-all.json" contains model validation errors.
    at /home/travis/build/Azure/azure-rest-api-specs/test/model.js:21:14
  name: 'AssertionError',
  actual: false,
  expected: true,
  operator: '==',
  message: 'swagger "arm-apimanagement/2016-10-10/swagger/apimanagement-all.json" contains model validation errors.',
  generatedMessage: false }

vishrutshah
vishrutshah suggested changes Mar 24, 2017
View reviewed changes
Copy link
Contributor

@vishrutshah vishrutshah left a 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.

@solankisamir
Copy link
Member Author

@vishrutshah updated the PR.

@vishrutshah
Copy link
Contributor

@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?

@solankisamir
Copy link
Member Author

@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.

@vishrutshah
Copy link
Contributor

@solankisamir Could you please fix following:

apimdeployment.json file references ./apimanagement.json#/parameters/ResourceGroupNameParameter but you still have that parameter defined in the same file here

@solankisamir
Copy link
Member Author

@vishrutshah apimdeployment.json is another file, which we need not to have any cross-references. You can check here 2016-07-07

@vishrutshah
Copy link
Contributor

@solankisamir Cool I see that you are removing that referencing into this PR changes. that's great. 👍

vishrutshah
vishrutshah suggested changes Mar 27, 2017
View reviewed changes
Copy link
Contributor

@vishrutshah vishrutshah left a 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?

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json
"operationId": "Apis_Get",
"description": "Gets the details of the API specified by its identifier.",
"produces": [
"application/json",
Copy link
Contributor

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 ?

Copy link
Member Author

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.

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json
"schema": {
"$ref": "#/definitions/ApiContract"
},
"headers": {
Copy link
Contributor

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?

Copy link
Member Author

@solankisamir solankisamir Mar 27, 2017 •
edited
Loading

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

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json Outdated
{
"$ref": "#/parameters/ServiceNameParameter"
},

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Member Author

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?

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json Outdated
{
"$ref": "#/parameters/ServiceNameParameter"
},

Copy link
Contributor

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

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json Outdated
{
"$ref": "#/parameters/ServiceNameParameter"
},

Copy link
Contributor

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

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json Outdated
{
"$ref": "#/parameters/ServiceNameParameter"
},

Copy link
Contributor

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

arm-apimanagement/2016-10-10/swagger/apimanagement-all.json Outdated
{
"$ref": "#/parameters/ServiceNameParameter"
},

Copy link
Contributor

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.

Reference: https://github.com/Azure/azure-rest-api-specs-pr/pull/40/files#diff-d3a0dcdc5d2939703fa9e32dff2295c1R3

@ravbhatnagar
Copy link
Contributor

@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.

@vishrutshah
Copy link
Contributor

Issue #1060 for reference

@solankisamir
Copy link
Member Author

#sign-off

@vishrutshah vishrutshah merged commit 4f08d63 into Azure:master Mar 30, 2017
@AutorestCI
Copy link

No modification for NodeJS

@AutorestCI
Copy link

No modification for Ruby

@AutorestCI
Copy link

No modification for Python

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

@vishrutshah vishrutshah vishrutshah approved these changes

Assignees

@vishrutshah vishrutshah

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@solankisamir @vishrutshah @ravbhatnagar @AutorestCI @sarangan12 @sergey-shandar @msftclas