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

docs: add basic README for each Helm chart on their own directory #167

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

fmuyassarov
Copy link
Collaborator

This commit adds basic documentation for each Helm chart so that basic information is available to the user who are trying to install any of those plugins via artifacthub.io registry.
An example: https://artifacthub.io/packages/helm/node-feature-discovery/node-feature-discovery

@klihub
Copy link
Collaborator

klihub commented Oct 19, 2023

You need to link to those new bits of documentation from somewhere which is reachable from the top entry point. Otherwise the doc build check/verification refuses to accept it.

@fmuyassarov fmuyassarov force-pushed the add-helm-readmes branch 2 times, most recently from a628dcb to 60a07b1 Compare October 19, 2023 10:34
@fmuyassarov
Copy link
Collaborator Author

You need to link to those new bits of documentation from somewhere which is reachable from the top entry point. Otherwise the doc build check/verification refuses to accept it.

Fixed and moved out some bits from installation.md. PTAL.

@fmuyassarov
Copy link
Collaborator Author

failed again...I will check

@klihub
Copy link
Collaborator

klihub commented Oct 19, 2023

@fmuyassarov I played a bit around with how we could include the helm chart docs among our generated documentation while keeping them in their current (very logical and natural) location in deployment/helm/*/README.md. I ended up having a helm directory under docs with its own ToC which links to each helm chart doc (I had to slightly adjust the Helm chart docs to make the end result look more sensible). The actual helm chart docs are stitched into the the main doc tree using includes. And the main doc includes the helm ToC, like we do for the rest of the documentation.

Here is the whole shebang, with two additional commits on top of your tree: https://github.com/klihub/nri-plugins/tree/docs/add-helm-readme

@fmuyassarov
Copy link
Collaborator Author

@fmuyassarov I played a bit around with how we could include the helm chart docs among our generated documentation while keeping them in their current (very logical and natural) location in deployment/helm/*/README.md. I ended up having a helm directory under docs with its own ToC which links to each helm chart doc (I had to slightly adjust the Helm chart docs to make the end result look more sensible). The actual helm chart docs are stitched into the the main doc tree using includes. And the main doc includes the helm ToC, like we do for the rest of the documentation.

Here is the whole shebang, with two additional commits on top of your tree: https://github.com/klihub/nri-plugins/tree/docs/add-helm-readme

Thanks @klihub. I think those changes you have made in the other two commits are good to have. Because first they fix the issue with the doc building and second reorganize the docs to have a separate section describing each chart parameters and installation/un-installation procedure. I have now cherry-picked your commits. PTAL.
/cc @kad

@fmuyassarov
Copy link
Collaborator Author

Actually I might need to rename the commit docs: experiment with Helm chart doc inclusion.

@klihub
Copy link
Collaborator

klihub commented Oct 20, 2023

Actually I might need to rename the commit docs: experiment with Helm chart doc inclusion.

Sure, that should be reworded.

@klihub
Copy link
Collaborator

klihub commented Oct 20, 2023

@fmuyassarov I would also squash at least the second fixup+headings commit to the first one. There is no benefit in keeping it separate.

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Hmm, make site-build fails with

...
/bin/bash: -c: line 1: unexpected EOF while looking for matching `''
/bin/bash: -c: line 3: syntax error: unexpected end of file
make: *** [Makefile:412: html] Error 2
make: *** [Makefile:426: site-build] Error 2

@klihub
Copy link
Collaborator

klihub commented Oct 20, 2023

Hmm, make site-build fails with

...
/bin/bash: -c: line 1: unexpected EOF while looking for matching `''
/bin/bash: -c: line 3: syntax error: unexpected end of file
make: *** [Makefile:412: html] Error 2
make: *** [Makefile:426: site-build] Error 2

@marquiz You are in a git worktree and lack commit 97a3e65.

@klihub
Copy link
Collaborator

klihub commented Oct 20, 2023

@fmuyassarov Can you rebase this on latest main/HEAD ? Then @marquiz should be able to make site-[build|serve] from a git worktree.

@klihub klihub self-requested a review October 20, 2023 11:13
Copy link
Collaborator

@klihub klihub left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, but could you rebase this on top of latest main/HEAD. Helps review{ers,ing} locally...

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @fmuyassarov for the update. I think this makes sense and is definitely a good improvement 👍 One comment below about naming.

Also, I think you should remove all the helm stuff from docs/resource-policy/installation.md. Just add a link to the new location or smth?

docs/helm/index.md Outdated Show resolved Hide resolved
This commit adds basic documentation for each Helm chart so that
basic information is available to the user who are trying to install
any of those plugins via artifacthub.io registry.

Co-authored-by: Krisztian Litkey <[email protected]>
Signed-off-by: Feruzjon Muyassarov <[email protected]>
Include link to Helm chart-specific documentation stored in
deployment/helm/*/README.md among the normal documentation
rooted under /docs.

Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Feruzjon Muyassarov <[email protected]>
@fmuyassarov
Copy link
Collaborator Author

fmuyassarov commented Oct 20, 2023

Also, I think you should remove all the helm stuff from docs/resource-policy/installation.md. Just add a link to the new location or smth?

@marquiz Yes, I think that's a good point. Because all the prerequisites would also appear in the plugin's corresponding README.md. I have now updated the PR. PTAL. And, you should be able to run make site-serve .

@fmuyassarov fmuyassarov requested review from marquiz and klihub October 20, 2023 12:29
Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

LGTM

@marquiz marquiz merged commit c96ed9b into containers:main Oct 20, 2023
3 checks passed
@fmuyassarov fmuyassarov deleted the add-helm-readmes branch October 20, 2023 13:16
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.

3 participants