-
Notifications
You must be signed in to change notification settings - Fork 0
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
How to document cnd's HTTP API #38
Conversation
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.
Nice work, don't have anything to add :)
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.
Approve with these few minor suggestions. Nice work.
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.
Awesome job, few questions please and maybe some link issues?
* Example: | ||
[source,sh] | ||
---- | ||
dredd cnd-http-api-description.yml http://localhost:<port-where-cnd-is-hosted> |
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.
So the API documentation would be enough for dredd
to generate random-yet-valid queries. That's cool.
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.
Yeah. But since you mention the word "random", it's important to point out that this is not fuzz-testing.
Dredd, because it's well documented and has all the features we need. It looks like the main effort will be https://dredd.org/en/latest/how-it-works.html#making-your-api-description-ready-for-testing[preparing the specification] for testing. | ||
|
||
==== JSON Schema integration ==== | ||
We currently use JSON Schema to validate the shape of the body of the response to `GET / |
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.
Wouldn't you expect to replace the JSON schema with our Open API doc?
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.
My understanding is that JSON Schema is more expressive and can be used for things such as server-defined client-side validation. OpenAPI supports a subset of JSON Schema and in the near future if will be fully supported.
In any case, using the $ref
keyword we can reference .schema.json
files in OpenAPI files. Until OpenAPI supports JSON Schema in full there might be some pain points, but there are ways to work around them.
If we go for the recommended option of using custom-id:redoc[ReDoc], it's https://www.npmjs.com/package/redoc#tldr[very easy]: | ||
|
||
. Include API specification in repository with cnd. | ||
. Host a website on GitHub Pages (for example). |
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.
Would that allow developers to access the API specification of various versions or just master?
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.
Talking to @thomaseizinger, we see the publicly hosted API specification only supporting master.
For someone developing against the API locally, we recommend adding an endpoint which returns the specification to the HTTP API itself.
* Use in comit-i to prove that it works. | ||
|
||
==== References ==== | ||
* https://apisyouwonthate.com/blog/the-many-amazing-uses-of-json-schema-client-side-validation[Blog on client-side validation based on JSON Schema]. |
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 descrive a bit more OpenAPI vs JSON schema? I am not clear on the concepts.
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.
Hopefully clearer now.
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.
What commit made it clearer?
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 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.
Good job.
Can you summarize all sub recommendations in one Recommendation section please (with these subsections it's a bit hard to read)?
So that if we refer back to this spike, we can see on one sight:
- Use OpenAPI for the spec
- Use Dredd to ensure up2dateness
- ...
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.
Awesome, good work! :)
The resource (/http-spec
) on that hosts the documentation of cnd
is probably debatable, but that is a minor nit 😬
Fixes comit-network/comit-rs#1122.
I wrote this in emacs' org-mode and then automatically converted to
.adoc
so please also let me know if there's anything weird in terms of formatting.