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

How to document cnd's HTTP API #38

Merged
merged 20 commits into from
Sep 3, 2019
Merged

Conversation

luckysori
Copy link
Collaborator

@luckysori luckysori commented Aug 29, 2019

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.

Copy link
Member

@da-kami da-kami left a 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 :)

0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
Copy link
Collaborator

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

0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@D4nte D4nte left a 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:https://localhost:<port-where-cnd-is-hosted>
Copy link
Contributor

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.

Copy link
Collaborator Author

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 /
Copy link
Contributor

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?

Copy link
Collaborator Author

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).
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
* 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].
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 descrive a bit more OpenAPI vs JSON schema? I am not clear on the concepts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hopefully clearer now.

Copy link
Contributor

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?

Copy link
Collaborator Author

@luckysori luckysori Sep 5, 2019

Choose a reason for hiding this comment

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

Copy link
Member

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

0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
0015-how-to-document-cnd-http-api.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@thomaseizinger thomaseizinger left a 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 😬

@luckysori luckysori merged commit ed95f96 into master Sep 3, 2019
@luckysori luckysori deleted the 1122-document-cnd-http-api branch September 3, 2019 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to document the cnd HTTP API?
6 participants