docs: check updates for the Helm reference #17613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qmonnet
added
area/documentation
qmonnet
force-pushed
the
pr/helm-values-check
branch
from
00e8d48
to
ea9bca2
Compare
|
Travis error is related, it happens while we try to regenerate the Helm reference after building Cilium, as part of the My understanding from the logs and error message is that the I'll update tomorrow, and probably disable the check if on arm64. I don't suppose it is worth building a second |
Cilium's documentation has a reference for the Helm values supported in its Charts. The reference is auto-generated, and is supposed to be updated each time the Charts are modified. To help keep the reference up-to-date, Cilium's CI should warn when developers forgot to regenerate and commit the document. Because the CI reported missing updates in the past, we thought that this was covered. But it turns out that the CI would only complain when it _fails_ to update the Helm reference - typically when some words need to be added to the spelling list. If the update goes fine, there is no check in place to validate that the regenerated file is identical to the one currently in the repository. This has led to multiple PRs missing the update for the Helm reference in the past. Address the issue by adding a check-helmvalues.sh script to validate that the current file is identical to the version in Git's HEAD. Run this script from the Makefile, as part of the "check" target. We also create a "update-helm-values" target, which looks cleaner to add as a prerequisite for "check" instead of passing the name of a .rst file. We also introduce a "FORCE" phony target to explicitly mark that we want the file regenerated each time. Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Update the reference for Helm values as a follow-up to a recent change in the Charts. Fixes: cilium#17509 Fixes: 105e1ab ("Allowed to set labels to ServiceMonitors") Signed-off-by: Quentin Monnet <quentin@isovalent.com>
qmonnet
force-pushed
the
pr/helm-values-check
branch
from
ea9bca2
to
e40b676
Compare
|
Travis build for arm64 went fine and it has the message about the skipped action 👍 |
bmcustodio
approved these changes
Oct 20, 2021
bmcustodio
approved these changes
Oct 20, 2021
qmonnet
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cilium's documentation has a reference for the Helm values supported in its Charts. The reference is auto-generated, and is supposed to be updated each time the Charts are modified. To help keep the reference up-to-date, Cilium's CI should warn when developers forgot to regenerate and commit the document.
Because the CI reported missing updates in the past, we thought that this was covered. But it turns out that the CI would only complain when it fails to update the Helm reference - typically when some words need to be added to the spelling list. If the update goes fine, there is no check in place to validate that the regenerated file is identical to the one currently in the repository. This has led to multiple PRs missing the update for the Helm reference in the past.
Address the issue by adding a check-helmvalues.sh script to validate that the current file is identical to the version in Git's HEAD. Run this script from the Makefile, as part of the
checktarget.We also create a
update-helm-valuestarget, which looks cleaner to add as a prerequisite forcheckinstead of passing the name of a .rst file. We also introduce aFORCEphony target to explicitly mark that we want the file regenerated each time.