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

add script to generate docs _sidebar.md based on the /docs content #11128

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Payback159
Copy link
Contributor

@Payback159 Payback159 commented Apr 26, 2024

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design

/kind documentation

/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

I have noticed in past pull requests that if the PR creator does not take into account that his documentation is also linked in the _sidebar.md then it can be found in the search of kubespray.io but not in the menu shown on the left.

The script should make it easier to automatically generate the _sidebar.md from the content of the /docs folder in the future.

In further steps it could perhaps even be started as part of the pipeline to keep the kubespray website up to date.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I have moved the existing documents in the docs folder to the various subfolders using the existing sidebar structure. I use the subfolders to create the "root structure" of the sidebar.

For the Markdown files that were located directly in /docs, I tried my best to divide them into the right categories.

The _sidebar.md file in this PR has already been created with this script, as an example of what the result would look like.

Does this PR introduce a user-facing change?:

BREAKING CHANGE: This script is introduced to facilitate living documentation and its administration. This leads to a restructuring in the documentation at https://kubespray.io/#/ to simplify the automatic creation of links, as the structure in the sidebar changes.

@k8s-ci-robot k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Apr 26, 2024
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 26, 2024
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 26, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @Payback159. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 26, 2024
@yankay
Copy link
Member

yankay commented Apr 29, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2024
@yankay
Copy link
Member

yankay commented May 6, 2024

Thanks @Payback159
Amazoning work! 👍

It will make some links broken in https://kubespray.io/#/ , so it's a broken change.
We should add it to the notes :-)

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Payback159, yankay

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 6, 2024
@Payback159
Copy link
Contributor Author

Hello @yankay,

thanks for the feedback. I have adjusted the notes to make it more obvious that there is a break in the documentation here.
I have also fixed the merge conflict with master branch.

Let's see what the CI has to say :D

@VannTen
Copy link
Contributor

VannTen commented May 6, 2024 via email

@Payback159
Copy link
Contributor Author

I also noticed that those two files are recognized as new. Both files were already there.

The only new content is the script. Everything else is just an adaption of existing documentation and scripts to work with the new structure.

Except in cgroups.md I recognized that there are some new lines which are coming form another PR of mine. (But the file also was already there I only added some new lines)

I am going to fix those lines. So that the documentation get's updated with the PR. It doesn't make sense to document the feature before PR is getting merged.

@VannTen
Copy link
Contributor

VannTen commented May 6, 2024 via email

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 6, 2024
@Payback159
Copy link
Contributor Author

Thanks for the hint. I have now deleted the original cilium.md file and moved the original cgroup.md file to the operations folder, now the git diff also fits and the new lines from another PR in the cgroup.md file are also cleaned up.

@VannTen
Copy link
Contributor

VannTen commented May 13, 2024

End result looks good.

I have three more things:

  • could you mark the sidebar as generated so the diff does not show up in PRs ? This is helpful for future docs review. (see https://docs.github.com/en/repositories/working-with-files/managing-files/customizing-how-changed-files-appear-on-github )
  • could you also add a pre-commit hook (using the check-readme-versions as an example, for instance) to block PR which did not run a script and ease running it for everyone. This should ensure the sidebar does not go out of sync.
  • Once that's done, it would be helpful to reorder your commits, grouping something like this:
    1. Modify the doc structures and the internal links, so there is not an intermediate commit with dead links
    2. add scripts and automation
    3. add result of script (so the generated sidebar).

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024
@Payback159
Copy link
Contributor Author

Hi @VannTen,

thanks for the feedback. Especially the hint about the .gitattribute, I didn't know that yet... nice feature.

I restructured the commits and added a pre-commit-hook that checks if a file was added in the docs folder, if that is the case it checks if the _sidebar.md was also modified as part of the commit.

If not, there is a recommendation for the developer to execute the gen_docs_sidebar.sh.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024
@VannTen
Copy link
Contributor

VannTen commented May 16, 2024 via email

@Payback159
Copy link
Contributor Author

Payback159 commented May 16, 2024

Yes I thought about it but after my research about pre-commit hooks I had the feeling that it is possible but not recommended/best-practice if a pre-commit hook itself changes something, so I went the way of implementing an additional check.

But for me it is also ok if I execute the generation script and pre-commit then aborts. I had seen this behavior because the "end of file check" also modifies data and therefore the pre-commit run failed for me.

Hopefully I'll get around to it tonight, then I'll remove the check-script from the PR and add the sidebar generation script as a pre-commit hook instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants