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

Simplify docs-check workflow by extracting functionality #12816

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

obnoxxx
Copy link
Contributor

@obnoxxx obnoxxx commented Aug 30, 2023

Description of your changes:

This simplifies the docs-check workflow by extracting hard-coded functionality into new make targets check-docs and check-helm-docs that can be called locally by the developer to conveniently check if they forgot to commit some generated docs or the multus validation job manifest file

Which issue is resolved by this Pull Request:

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
@obnoxxx obnoxxx force-pushed the simplify-workflow branch 2 times, most recently from 9be98b7 to 8865277 Compare August 30, 2023 11:19
# This manifest contains a Kubernetes Job and supporting definitions for running Rook's Multus
# This manifest contains a Kubernetes job and supporting definitions for running Rook's Multus
Copy link
Member

Choose a reason for hiding this comment

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

This is intentional. Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BlaineEXE wrote:

This is intentional. Please revert.

done. commit removed

Copy link
Member

@parth-gr parth-gr left a comment

Choose a reason for hiding this comment

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

lgtm

[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, you need to run helm-docs manually, and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, you need to run helm-docs manually, and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`. We have made running helm-docs very easy for you by creating a `make` target for it: you only need to run `make helm-docs`. an additional `make` target exists to help you check if there are uncommitted generated helm chart docs: just run `make check-helm-docs`
Copy link
Member

Choose a reason for hiding this comment

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

Rook has a new initiative to make docs more imperative (notably avoiding "we" and "you" in docs), so this is a good time to update this paragraph.

Suggested change
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, you need to run helm-docs manually, and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, you need to run helm-docs manually, and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`. We have made running helm-docs very easy for you by creating a `make` target for it: you only need to run `make helm-docs`. an additional `make` target exists to help you check if there are uncommitted generated helm chart docs: just run `make check-helm-docs`
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, run `make helm-docs`, and check in the resulting autogenerated markdown files at the path `/Documentation/Helm-Charts`.

After thinking about this, if I imagine myself to be a new contributor, I think I would be confused how running make helm-docs is improved by also running make check-helm-docs. I think it might end up being less confusing new contributors to have this target be something that is not officially documented.

Improvements for new contributors is certainly valuable, and reducing the barrier to entry is worthwhile. In service of that and in a related vein as this PR, something that I think would be most impactful and useful would be to have a make target that runs all auto-generation targets including docs, helm-docs, crds, and any I forgot. The only exception I would make would be make codegen which takes too long to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BlaineEXE - do I get it right that you are requesting to remove the new mention of the new make target from this section altogether? I can also try to make the wording clearer and avoid using "we" and "you" ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@obnoxxx wrote:

@BlaineEXE - do I get it right that you are requesting to remove the new mention of the new make target from this section altogether?

Alternatively, I can also try to make the wording clearer and avoid using "we" and "you" ...

I have done that and updated the PR

Copy link
Member

Choose a reason for hiding this comment

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

@BlaineEXE - do I get it right that you are requesting to remove the new mention of the new make target from this section altogether? I can also try to make the wording clearer and avoid using "we" and "you" ...

Yes. When I read this while imagining standing in the shoes of a new contributor, I think I would feel overwhelmed by the length of the text and confused about what make targets to run. Short and sweet is best, and I've especially noticed this to be true for users/contributors who are in Asia and don't have the same mastery of English.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

make docs also makes the helm docs. How about if we just document make docs and no need to document a separate make helm-docs? This will keep the commands simpler. If make helm-docs still exists, that's just an implementation detail that we don't need to document.

@obnoxxx
Copy link
Contributor Author

obnoxxx commented Aug 31, 2023

@travisn wrote:

make docs also makes the helm docs. How about if we just document make docs and no need to document a separate make helm-docs? This will keep the commands simpler. If make helm-docs still exists, that's just an implementation detail that we don't need to document.

done: updated accordingly.

@@ -36,7 +36,7 @@ When previewing, now you can navigate your browser to [http://127.0.0.1:8000/](h
Please make sure that your Python binary path is included in your `PATH`.

## Running helm-docs
Copy link
Member

Choose a reason for hiding this comment

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

Can we just change this whole section to only talk about running make docs? I still think it's nice to keep the developer docs simple. Did you push the changes from when I asked about this previously? Maybe I'm missing the update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Can we just change this whole section to only talk about running make docs? I still think it's nice to keep the developer docs simple.

I fully agree!

Did you push the changes from when I asked about this previously? Maybe I'm missing the update.

yes, I did that. here is the updated commit: abcc79e

Copy link
Member

@travisn travisn Aug 31, 2023

Choose a reason for hiding this comment

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

I'm suggesting to not even mention make helm-docs. Something short like this?

## Making docs

[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm
chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` 
and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

I'm suggesting to not even mention make helm-docs. Something short like this?

## Making docs

[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm
chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` 
and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`. 

I just updated it with very similar wording, not mentioning make helm-docs at all.

.github/workflows/docs-check.yml Show resolved Hide resolved
@BlaineEXE BlaineEXE requested a review from travisn August 31, 2023 15:16
@obnoxxx obnoxxx force-pushed the simplify-workflow branch 7 times, most recently from 2f8b49f to 80de65d Compare August 31, 2023 17:35
## Running helm-docs
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, you need to run helm-docs manually, and check in the resulting autogenerated md files at the path `/Documentation/Helm-Charts`
## making docs
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` and check in the resulting autogenerated md files under the path `/Documentation/Helm-Charts`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` and check in the resulting autogenerated md files under the path `/Documentation/Helm-Charts`.
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` and check in the resulting autogenerated md files under the path `/Documentation/Helm-Charts`.

Documentation/Contributing/documentation.md Outdated Show resolved Hide resolved
## making docs
[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` and check in the resulting autogenerated md files under the path `/Documentation/Helm-Charts`.

running `make docs` will do everything that is needed including invocation of helm-docs.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need lines 41-43? They seem like details that only someone looking at makefiles needs to know. IMO the developer guide doesn't need these details for normal usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Do we need lines 41-43? They seem like details that only someone looking at makefiles needs to know. IMO the developer guide doesn't need these details for normal usage.

I fully agree. removed

@obnoxxx obnoxxx force-pushed the simplify-workflow branch 2 times, most recently from 07068d7 to 6e1b47a Compare August 31, 2023 18:08
## Making docs

[helm-docs](https://github.com/norwoodj/helm-docs) is a tool that generates the documentation for a helm chart automatically. If there are changes in the helm chart, the developer needs to run `make docs` (to run helm-docs) and check in the resulting autogenerated md files under the path `/Documentation/Helm-Charts`.
The CI will check on a PR if helm-docs modifies any checked-in file that the developer forgot to commit and complain in that case. To make it easy to check locally for uncommitted changes generated by helm-docs, an additional `make` target exists: just running `make check-helm-docs` will run `helm-docs` and complain if this results in uncommitted generated helm chart doc files. So it is a goog habit to run `make check-helm-docs` locally before creating or updating a PR.
Copy link
Member

Choose a reason for hiding this comment

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

Even this line 41 can be removed IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Even this line 41 can be removed IMO

I didn't remove it completely, but simplified it, removing the "CI" part ...

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, I don't understand the paragraph. There are some sentence formatting issues and partial sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Honestly, I don't understand the paragraph. There are some sentence formatting issues and partial sentences.

I updated the PR once more to clarify the paragraph. I hope it is clearer now!

Additonally, I am wondering if we should mention that make docs generates more than just helm chart docs

@@ -214,6 +214,20 @@ helm-docs: $(HELM_DOCS) ## Use helm-docs to generate documentation from helm cha
-t ../../../Documentation/Helm-Charts/ceph-cluster-chart.gotmpl.md \
-t ../../../Documentation/Helm-Charts/_templates.gotmpl

check-helm-docs:
Copy link
Member

Choose a reason for hiding this comment

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

Since make docs also builds the helm docs, do we still need this separate check-helm-docs? Seems like it's already tested by make check-docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travisn wrote:

Since make docs also builds the helm docs, do we still need this separate check-helm-docs? Seems like it's already tested by make check-docs?

good point. I thought about it but decided to keep the separate targets because the check-helm-docs target has a more specific error message, pointing to a helm-docs specific section in the guide.

@obnoxxx obnoxxx force-pushed the simplify-workflow branch 3 times, most recently from 5c607ea to b28f659 Compare September 1, 2023 15:27
obnoxxx and others added 2 commits September 1, 2023 18:17
This simplifies the docs-check workflow definition by extracting  functionality
directly coded into the workflow file into new make targets 'check-docs'
and 'check-helm-docs' and replacing the code in the workflow file by the simple invocation of
new make targets. As a side effect, it makes it easily possible for a
developer to check locally if they forgot to commit any autogenerated
docs.

Signed-off-by: Michael Adam <obnox@samba.org>
…ing guide

Signed-off-by: Michael Adam <obnox@samba.org>
Co-authored-by: : Michael Adam <obnox@samba.org>
signed-off-by: Travis Nielsen <tnielsen@redhat.com>
Co-authored-by: Travis Nielsen <tnielsen@redhat.com>
@travisn travisn merged commit 3468a4a into rook:master Sep 1, 2023
48 of 50 checks passed
travisn added a commit that referenced this pull request Sep 5, 2023
Simplify docs-check workflow by extracting functionality  (backport #12816)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants