-
Notifications
You must be signed in to change notification settings - Fork 47
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
Publish /docs to github pages #265
Conversation
Ran the GitHub action from my main branch: https://anik120.github.io/operator-controller/ |
b7d469c
to
5dbce81
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.
Overall this looks awesome and is IMO definitely the right direction to go for making OLMv1 more easily usable. Great work @anik120 !
I do have some nits/comments:
* [deppy](https://github.com/operator-framework/deppy): Deppy is a Kubernetes API that runs on- or off-cluster for resolving constraints over catalogs of RukPak bundles. Deppy is part of the next iteration of OLM and was first introduced here. The initial goal of the project is to remove the dependency manager from the Operator Lifecycle Manager (OLM) and make it its own generic component. | ||
|
||
|
||
* [catalogD](https://github.com/operator-framework/catalogd): Catalogd is a Kubernetes extension that unpacks [file-based catalog (FBC)](https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs) content that is packaged and shipped in container images, for consumption by clients on-clusters (unpacking from other sources, like git repos, OCI artifacts etc, are in the roadmap for catalogD). As component of the Operator Lifecycle Manager (OLM) v1 microservices architecture, catalogD hosts metadata for Kubernetes extensions packaged by the authors of the extensions, as a result helping customers discover installable content. |
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.
* [catalogD](https://github.com/operator-framework/catalogd): Catalogd is a Kubernetes extension that unpacks [file-based catalog (FBC)](https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs) content that is packaged and shipped in container images, for consumption by clients on-clusters (unpacking from other sources, like git repos, OCI artifacts etc, are in the roadmap for catalogD). As component of the Operator Lifecycle Manager (OLM) v1 microservices architecture, catalogD hosts metadata for Kubernetes extensions packaged by the authors of the extensions, as a result helping customers discover installable content. | |
* [catalogd](https://github.com/operator-framework/catalogd): Catalogd is a Kubernetes extension that unpacks [file-based catalog (FBC)](https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs) content that is packaged and shipped in container images, for consumption by clients on-clusters (unpacking from other sources, like git repos, OCI artifacts etc, are in the roadmap for catalogD). As component of the Operator Lifecycle Manager (OLM) v1 microservices architecture, catalogD hosts metadata for Kubernetes extensions packaged by the authors of the extensions, as a result helping customers discover installable content. |
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 is a good highlight. Have we landed on a concrete way we want to use that yet? catalogD? catalogd? CatalogD? Catalogd?
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.
I would typically recommend using the repository name but to be fair I have gone against that myself and used most if not all of these variations. I definitely think we need to pick one and make sure it is consistent throughout these docs though. My personal preferences are either catalogd or Catalogd
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.
For comparison, we use "containerd" in most instances in the downstream docs, unless we are specifically referring to containerd
as an object or command. If the product name is at the beginning of a sentence or title, we use "Containerd"
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.
The information is there, but it needs to be re-organized. One thing I'm seeing a lot of is "OLM also...". That phrase appears a number of times, and is definitely repetitive. It sounds as though something was left out, and it's just "another thing".
For organization, I would suggest:
- Extension/operator lifecycle
- Describe OLM v0 (i.e. History)
- Describe OLM v1
- Differences between OLM v0 and v1
Section headers and separation would be good too (along the lines of the above).
docs/components.md
Outdated
|
||
* [catalogD](https://github.com/operator-framework/catalogd): Catalogd is a Kubernetes extension that unpacks [file-based catalog (FBC)](https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs) content that is packaged and shipped in container images, for consumption by clients on-clusters (unpacking from other sources, like git repos, OCI artifacts etc, are in the roadmap for catalogD). As component of the Operator Lifecycle Manager (OLM) v1 microservices architecture, catalogD hosts metadata for Kubernetes extensions packaged by the authors of the extensions, as a result helping customers discover installable content. | ||
|
||
* [operator-controller](https://github.com/operator-framework/operator-controller): Finally, operator-controller is the central component of OLM v1, that consumes all of the components above to extend Kubernetes to allows users to install, and manage the lifecycle of other extensions |
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.
I would actually put this first, ad is the main OLMv1 component. And the description seems very short considering how important this component is.
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.
I've moved the list point to be on top, but great point about the description being inadequate. I was hoping to get some inspiration from the README of the project to expand on the description, but looks like the README itself needs a little bit of work: https://github.com/operator-framework/operator-controller/blob/4d6757a2b0fe7dda779d03d44f37d20872218711/README.md
Leaving this conversation as unresolved so that we can create a follow up ticket to update the README and expand the description here
Codecov Report
@@ Coverage Diff @@
## main #265 +/- ##
=======================================
Coverage 77.00% 77.00%
=======================================
Files 15 15
Lines 687 687
=======================================
Hits 529 529
Misses 129 129
Partials 29 29 |
@tmshort @everettraven thank you for the detailed feedback! I've addressed most of the feedback that isn't around reorganization of content.
Fyi, Michael had the same feedback, and has volunteered to help in reorganizing the content (I.e this will also be our first time working in the proposed framework: provide the content, so that the content can be organized in separate PRs. That way, "provide content" PRs can stay scoped to providing the content without having to worry about architecting the content in the right way, and "organize content" PRs can architect the content in the right way without needing to worry about constantly fetching info to generate the content) |
598be7c
to
dcdcdbc
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.
Overall looks good! Noticed a couple spelling mistakes and left a response to another comment:
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.
A few more tiny nits after circling back around with fresh eyes:
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.
Overall this looks good to me. Found a couple spelling mistakes and have a nit. Also noticed there are some spelling mistakes from a previous review I did that have not been resolved.
docs/Releases/v0.2.0-release.md
Outdated
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.
If at all possible, I would really like to avoid putting release notes into the repo itself. It causes some significant problems:
- If you want the notes to exist prior to pushing a tag, you have to close the main branch for merging, push a tag, open the main branch. That suddenly means your release process has multiple steps and isn't simply "push a tag". I think we should ensure that the release process is always only "push a tag".
- If it is okay for notes to exist after making a release, then we just won't ever actually follow-up and do it, so things get out of date.
My advice for now for in-repo docs, make them reflect the current state of the world. If you want docs for v0.2.0, you would go do the v0.2.0 tag and look at docs. If you want v0.3.0 docs, go to the v0.3.0 tag and look at docs there, etc. This also means that as features go in, docs go in at the same time. Therefore, in theory, the docs on a commit are always docs for that commit.
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.
Release note should be more like CHANGES.md
or similar. This is more of a HOWTO.md
.
The release announcement should have the friendly text ("Hello, again, world!"), with a pointer to the CHANGES.md@v0.2.0
(or proper syntax).
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.
@joelanford looks like the popular option for versioning documentation sites out there is mike. @ncdc brought it to our attention that kcp documentation site is using a different theme, mkdocs-material, and looks like mkdocs has native mike integration (or at least puts some effort in integrating with mike, as opposed to other theme, like the one the PR was using before).
I was looking into that theme, but looks like mike integration isn't as straightforward as the doc projects it to be .
In the interest of getting the first brick laid out and then iterating on that, I've switched the theme to mkdocs-material from Jekyll, so that we can do a follow up PR "Release versioned documentation" that does the mike integration.
Also @tmshort I've updated the release-notes.md, but instead of trying to come to a decision about what the release notes (for this and all upcoming future releases) should contain in this PR, we should do that as a follow up.
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.
looks like mike integration isn't as straightforward
This linked issue reported isn't super relevant / is a red herring.
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.
I'd suggest first looking at including release notes as part of the GitHub releases, and not in the docs.
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.
@ncdc I followed the doc and included those lines in the yaml, and then did a docker run --rm -it -p 8000:8000 -v ${PWD}:/docs squidfunk/mkdocs-material
to run the website, and the version tab never showed up. That's when I tumbled onto that issue, and did a mike deploy
+ mike serve
, and the version tab showed up.
Based on that, is it fair to say that if we do need versioning to be plugged into this website at some point, we'll have to write scripts/add Makefile targets to download and use mike in CI to deploy the website? I.e The GitHub actions I copied from the mkdocs-material that is the equivalent of docker run --rm -it -p 8000:8000 -v ${PWD}:/docs squidfunk/mkdocs-material
for the GitHub-pages needs to be switched out for the scripts/makefile target, similar to what kcp does.
I also had to install python in my machine just for mike.
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.
You don't need to add anything to download mike by itself. The links below handle it with venv
and requirements.txt
.
See:
- https://github.com/kcp-dev/kcp/blob/8dd1a3f4e240ae0e1ec81df526afb200321df37d/Makefile#L163-L174
- https://github.com/kcp-dev/kcp/blob/main/Makefile.venv
- https://github.com/kcp-dev/kcp/blob/8dd1a3f4e240ae0e1ec81df526afb200321df37d/.github/workflows/docs-gen-and-push.yaml#L39-L42
- https://github.com/kcp-dev/kcp/blob/8dd1a3f4e240ae0e1ec81df526afb200321df37d/docs/requirements.txt
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.
Right, it needs configuration beyond what's mentioned in the documentation site. Keeping this here for record if we do need versioning for the website at some point.
@@ -0,0 +1,85 @@ | |||
--- | |||
layout: default | |||
title: Adding a catalog of operators to the cluster |
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 this be in the catalogd
repo? I thought our plan was to co-locate docs and code for technical details / reference information.
And then we'd have a separate place to collect the higher level concepts/tasks/tutorials/etc.
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.
That is the general plan yes, this one's just to get us started, for us to have a place to keep putting things until all the sites for all the repos are ready. Hopefully that'll be soon and we won't have to do a surgical procedure to separate things, but it's at least better than having no where to put docs until then. Catching up to a docs backlog is much harder than reorganizing docs from my perspective.
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.
Fyi @michaelryanpeter and I plan on working on a Brief that'll capture all of these decisions, I.e put docs effort in the Briefs/RFC path too
ebcd3af
to
2d4ed17
Compare
I'm still not to keep on the name/contents of |
This comment was marked as outdated.
This comment was marked as outdated.
Just wanted to remove the "Changes Requested"
Closes operator-framework#291 Signed-off-by: Anik <anikbhattacharya93@gmail.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.
Thanks.
Description
Reviewer Checklist