-
Notifications
You must be signed in to change notification settings - Fork 21
Fix linting ct lint
issue in helm chart
#487
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
Conversation
MCK 1.5.0 Release NotesNew Features
Bug Fixes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this will fix the helm linting issue definitely. More likely we are observing flakiness, because of running dependant tasks in parallel. lint_helm_chart
func depends on the helm values generated by update_values_yaml_files
func, but they are not running sequentially.
pre_commit() {
run_job_in_background "update_jobs" # <- this rebuilds helm values
run_job_in_background "update_licenses"
run_job_in_background "lint_code"
run_job_in_background "start_shellcheck"
run_job_in_background "regenerate_public_rbac_multi_cluster"
run_job_in_background "python_formatting"
run_job_in_background "check_erroneous_kubebuilder_annotations"
run_job_in_background "validate_snippets"
run_job_in_background "lint_helm_chart" # <- this requires helm values ready
if wait_for_all_background_jobs; then
echo -e "${GREEN}pre-commit: All checks passed!${NO_COLOR}"
return 0
else
return 1
fi
}
update_jobs() {
# Update release.json first in case there is a newer version
time update_release_json
# We need to generate the values files first
time update_values_yaml_files # <- this rebuilds helm values
# The values files are used for generating the standalone yaml
time generate_standalone_yaml
}
I think the solution should be to run lint_helm_chart
in update_jobs
function as the last step. Or maybe run it after all other jobs have finished?
Hi @MaciejKaras, |
@m1kola can you please have another look/approve? |
Summary
Update:
Thanks to @MaciejKaras for the comment here and it looks like we were trying to lint the helm chart in parallel to other jobs that were trying to update the helm chart. This PR tries to change that and runs the helm chart linting after the updates to helm chart are made by
update_jobs
job.Old Desc:
The chart linting command that we run as part of lint_repo task
seems to be flakey. It doesn't seem to fail regularly but we have seen issues that are reported here. If we look into the error
It looks like
.Values
object that is passed to the template file doesn't have the field.Values.operator.vaultSecretBackend
. And the only places where the values.yaml doesn't havevaultSecretBackend
defined are the filesvalues-openshift.yaml
andvalues-multi-cluster.yaml
.And I suspect that
helm template
, and merge doesn't work properly and eventually.Values.operator
has the value that is defined in eithervalues-openshift.yaml
or invalues-multi-cluster.yaml
(and it doesn't havevaultSecretBackend
).helm template
is run using the other values.yaml files which doesn't haveoperator.vaultSecretBackend
. This is less likely because this could have caused consistent failure in CI.One option that I considered to fix this is explicitly passing just the main values.yaml file to the
ct lint
command, using--helm-extra-args
flag, but that didn't resolve the issues. I was able to reproduce it once if I ran 15 manual patches.The other option was to have the
vaultSecretBackend
field in the other values files as well. And this is what I have done in this PR. I am not really sure if this actually fixed the problem but I was not able to reproduce it in 20 manual patches. The other small change that this PR does is, runninghelm template
before runningct lint
so that if the test fails ever again we can check and see ifvaultSecretBackend
is eventually generated in the template.Proof of Work
Output of successful run of
make precommit
: https://gist.github.com/viveksinghggits/45a6f80f2b5b85d26d7d21dcc3dfb56cOutput of failed run of
make precommit
: https://gist.github.com/viveksinghggits/7bb5a35e8bfe3a9be93e668ee8e734b9Ran 20 manual evg patches and all of them are successful.

Checklist
skip-changelog
label if not needed