-
Notifications
You must be signed in to change notification settings - Fork 288
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
internal/proto: improvements to OpenAPIv2 spec #4555
Conversation
internal/proto/controller/api/services/v1/account_service.proto
Outdated
Show resolved
Hide resolved
url: "https://github.com/hashicorp/boundary/blob/main/LICENSE" | ||
}; | ||
} | ||
host: "your-boundary-hostname.com" |
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.
We could have something like localhost:9200
here as well, not sure what makes this the most obvious that it depends on their Boundary deployment?
5f0b8aa
to
187ccbb
Compare
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.
Very cool! Thank you!
internal/proto/controller/api/services/v1/account_service.proto
Outdated
Show resolved
Hide resolved
internal/proto/controller/api/resources/accounts/v1/account.proto
Outdated
Show resolved
Hide resolved
187ccbb
to
24e3dbd
Compare
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 looks great. Thank you!
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.
Great work on this! This was a huge effort, many thanks to the whole API Docs Days team that worked on it. Our customers will greatly benefit from the more consistent presentation of resource names and other information. And I love that you included links to external docs where appropriate!
I added suggestions for style and standards. Please let me know if you have any questions about my feedback. Also, if you're short on time to follow up, I'd be happy to apply the updates. Just let me know how I can help.
Thank you!
Add various pieces of metadata that can be used by the API docs renderer. Improve the service labels by specifying explicit tags. Specify error responses. Specify version at generation time.
The default error entry is now used to cover all errors, as adding a new error for every possible status code was a bit much. This also properly includes the real error type now!
…rer. Improve the service labels by specifying explicit tags.
Replace Output Only comments with a field behavior annotation in accounts. Mark some fields as required. Add elaborate description for attributes.
Copy parts of the API docs into the description
Unfortunately, while these help with the creating of resources, they still appear required in contexts where they are not, such as when updating a resource.
24e3dbd
to
2c40770
Compare
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.
Amazing work on this Johan and team!! Thanks so much for all your hard work. This is a significant improvement for our users! 🎉
This PR attempts to refactor much of the patterns used in our protobuf documentation to improve the experience of using the API documentation. Among the general changes:
This PR also contains the beginnings of a refactor of all the service files. So far,
account_service.proto
andalias_service.proto
(and dependencies) have been updated with the following:// Output only
with the(google.api.field_behavior) = OUTPUT_ONLY
annotation option. The old string confused users as it was part of the field descriptionAdd thethis turned out to be the wrong call as it causes confusion when updating a resource.(google.api.field_behavior) = REQUIRED
annotation option to fields that are required in requestsAccount
->account
) in accordance with the Boundary docs style guideattribute
fields. Previously,attribute
fields were completely opaque to users. We now have example inputs for attributes.version
field is not required when creating a resourceI intend to go through the rest of the service files as and when I find time in the next few weeks. If anyone is interested in helping with this effort, please reach out to me 😄.
Reviewers: copy the contents of
controller.swagger.json
and upload to https://developer.hashicorp.services/open-api-docs-preview to review the look of the changes. Feel free to also try uploading it to https://editor-next.swagger.io/ to compare against a more capable OpenAPIv2 renderer. Note that the developer preview OpenAPI docs page suffers a few issues. If you see anything that isn't already mentioned in the feedback doc, let me know.