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.

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

Enterprise Knowledge Graph Documentation in one swagger json file #6743

Closed
wants to merge 103 commits into from

Conversation

tengr
Copy link
Contributor

@tengr tengr commented Jul 26, 2019

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

@anuchandy
Copy link
Member

@tengr Did you get a chance to get it reviewed by AzureAPIBoard?

\ cc @johanste

@tengr
Copy link
Contributor Author

tengr commented Aug 20, 2019

@tengr Did you get a chance to get it reviewed by AzureAPIBoard?

\ cc @johanste

Yes. We had a meeting with the AzureAPIBoard. I'm still working on updating this PR.

@yungezz
Copy link
Member

yungezz commented Aug 28, 2019

hi @tengr any update?

@tengr
Copy link
Contributor Author

tengr commented Sep 10, 2019

hi @tengr any update?

Just updated.

@anuchandy
Copy link
Member

@tengr Let me know once you've final approval from API review board.

@yungezz
Copy link
Member

yungezz commented Sep 27, 2019

HI @anuchandy , could you pls continue reviewing and push to end?

"parameters": [
{
"in": "query",
"name": "service",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't this an internal-only parameter? Or am I misremembering?

"type": "string"
},
"isAggregate": {
"type": "boolean",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is malformed JSON. I'm not really sure why this is not being flagged by our tools. But please fix this up.

@johanste
Copy link
Member

johanste commented Oct 1, 2019

The swagger document is malformed. Not sure why our tools are not flagging it. But it needs to be fixed up.

@tengr
Copy link
Contributor Author

tengr commented Oct 1, 2019

The swagger document is malformed. Not sure why our tools are not flagging it. But it needs to be fixed up.

just fixed.

@johanste
Copy link
Member

johanste commented Oct 2, 2019

The semantic validation is now failing due to the document not being a valid swagger/openapi document.

],
"responses": {
"200": {
"description": "Success",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the number of items/the size of the data ever require the service to do paging?

}
},
"definitions": {
"ConverseRequest": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are missing descriptions for almost all attributes. Understanding what values are allowed for attributes like entityType will be very challenging for users of the API.

}
}
},
"EntityType": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you have a model item with only a single property? Is the intent for the number of attributes to grow over time? If so, should attributes named entityType be of type "/definitions/EntityTypeor should the attribute name beentityTypeName`?

}
}
},
"SparqlQueryDbResponse": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest naming the type in a way that makes it clear that this is a SPARQL response object and add a description that indicates that this is a standard format, not something that is unique to this service.

@anuchandy
Copy link
Member

@tengr could you take a look at review comments?

@tengr tengr closed this Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants