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: fix Helm documentation and doc checks #16737

Merged
merged 2 commits into from Jul 1, 2021

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jul 1, 2021

First commit is an update of the Helm reference, so far omitted after some recent changes on the Helm values.

Second commit is a fix to the GitHub workflow conditions to run the Documentation action when Helm values are updated, in order to effectively catch similar omissions in the future.

Only the second commit should be backported, so I won't mark this PR for backports but will do it manually.

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. labels Jul 1, 2021
@qmonnet qmonnet requested review from a team as code owners July 1, 2021 15:42
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

The first commit checked in a binary file that's not needed, otherwise 👍

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks fine to me, just probably don't need to include the compiled python binary like Chris mentioned above (maybe this should be in a .gitignore somewhere?).

Will be ready-to-merge after addressing the above.

@@ -838,7 +842,7 @@
- object
- ``{}``
* - monitor
- Specify the CIDR for native routing (ie to avoid IP masquerade for). This value corresponds to the configured cluster-cidr. nativeRoutingCIDR:
- Specify the IPv4 CIDR for native routing (ie to avoid IP masquerade for). This value corresponds to the configured cluster-cidr. ipv4NativeRoutingCIDR:
Copy link
Member

Choose a reason for hiding this comment

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

The placement of this option seems a bit odd like it's in the middle of the documentation for the monitor option , but that's not a new problem introduced by this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this should probably be addressed independently. The file is generated from the comments in install/kubernetes/cilium/values.yaml:

# -- Specify the IPv4 CIDR for native routing (ie to avoid IP masquerade for).
# This value corresponds to the configured cluster-cidr.
# ipv4NativeRoutingCIDR:

@bmcustodio do you know if there is a way to avoid appending the name of the variable from this comment to the generated reference? Do you think removing the space between the # and the variable name would work?

Apologies for the python binary, that was obviously a mistake that I did not notice. I should probably add it to a .gitignore, I think it's generated from the custom filter for spelling I added for WireGuard (but I need to check first that it's still used and not some old artefact from a development version of that filter).

Copy link
Member Author

Choose a reason for hiding this comment

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

Now updated to remove the binary.

Fix Helm documentation after a recent update of the Helm values. Update
the spelling file accordingly.

Fixes: 792ed5a ("daemon: rename native-routing-cidr option to ipv4-native-routing-cidr")
Fixes: cilium#16646
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
A spurious slash at the end of the pattern for files to watch for changes
in the description of the GitHub workflow prevented the action to run
on all changes of the Helm values. Fix it.

Fixes: 4e5272b ("docs: run GitHub action when Charts are touched to check Helm values ref")
Fixes: cilium#16577
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@joestringer joestringer merged commit b89118b into cilium:master Jul 1, 2021
@qmonnet qmonnet deleted the pr/ci-run-helm-doc branch July 1, 2021 21:27
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport pending to v1.10 in 1.10.2 Jul 1, 2021
@aanm aanm added this to Backport pending to v1.10 in 1.10.3 Jul 2, 2021
@aanm aanm removed this from Backport pending to v1.10 in 1.10.2 Jul 2, 2021
@aanm aanm added this to Backport pending to v1.10 in 1.10.4 Jul 15, 2021
@aanm aanm removed this from Backport pending to v1.10 in 1.10.3 Jul 15, 2021
@joestringer joestringer moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.4
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

3 participants