-
Notifications
You must be signed in to change notification settings - Fork 53
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
📖 add public api docs #1331
📖 add public api docs #1331
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1331 +/- ##
=======================================
Coverage 76.37% 76.37%
=======================================
Files 41 41
Lines 2438 2438
=======================================
Hits 1862 1862
Misses 402 402
Partials 174 174
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
As per the last discussion HEAD https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD is also a supported method for catalog webserver
Apart from my above comment i.e. #1331 (review) everything else looks good. |
@LalatenduMohanty addressed |
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.
Small nits. I am going to look into getting some linting in the repo to address some of the consistency issues around terminology.
docs/refs/public-api.md
Outdated
- [operator-controller API reference](./api/operator-controller-api-reference.md) | ||
- [catalogd API reference](./api/catalogd-api-reference.md) | ||
- `catalogd` webserver. For more information on what this includes, see the [catalogd webserver documentation](./api/catalogd-webserver.md) |
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.
Since this section is pretty short, I think you could create an Additional resources section at the end of the page for the links. Maybe after line 12?
## Additional resources
- Operator Controller API reference
- Catalogd API reference
- Catalogd web server
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 you mind elaborating on what value this would bring to a reader over the current setup?
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.
To me, the links are a little distracting where they are. The point you are trying to get across is to define the scope of the public API. I think that if you break out the links to the API references at the end of the page, it is a little tidier and your intended meaning is more direct.
That said, this is 100% a nit. Feel free to ignore it doesn't feel relevant to 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.
As a heuristic for determining whether something should be an inline link vs an additional resource, I ask myself if the reader needs to stop and read the link before continuing.
If the answer is yes, then I use an inline link. Typically, this would be in the case of a prerequisite or context information that is vital to understanding a concept or completing a task.
If the reader can follow up on the information later or if the link is provided for helpful context, then I break out the link in an additional resources section.
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.
As long as the content is OK - i'd like to get this merged in so we at least have it and can keep progressing towards our v1.0.0 release. We can always tweak this in the future!
Signed-off-by: everettraven <everettraven@gmail.com>
Signed-off-by: everettraven <everettraven@gmail.com>
de1767e
to
fc29827
Compare
# Catalogd web server | ||
|
||
[Catalogd](https://github.com/operator-framework/catalogd), the OLM v1 component for making catalog contents available on cluster, includes | ||
a web server that serves catalog contents to clients via a HTTP(S) endpoint. |
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 would be "an HTTP(s)" since HTTP is pronounced with a starting vowel sound. You determine the article via the acronym pronunciation.
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.
Should be taken care of in 647a9f7
|
||
### Compression Support | ||
|
||
The `catalogd` web server supports gzip compression of responses, which can significantly reduce associated network traffic. In order to signal to that the client handles compressed responses, the client must include `Accept-Encoding: gzip` as a header in the HTTP request. |
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.
"to signal that the client"
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.
Should be taken care of in 647a9f7
+1 to Tayler's comments. Otherwise, lgtm. |
Signed-off-by: everettraven <everettraven@gmail.com>
Co-authored-by: Joe Lanford <joe.lanford@gmail.com>
Description
Reviewer Checklist