GH-820: Strictly lint helm files #63
Conversation
The benefit of being a separate action is we get to very easily filter this to only run on applicable PRs/pushes. We also get parallelization too, which we could get just by using separate jobs in the same action. Accomplishing the filtering, though, would require more complex logic to do at step-time rather than action-time as done here.
This issue with --strict has been documented online: helm/helm#5417 helm/helm#3766 The Helm developers essentially said they don't care, but some other folks found they had a similar issue with template files and came up with workarounds: VirtusLab/render#11 One option is to use weird template syntax to do some sort of non-checked existence test: VirtusLab/render#11 (comment) The cleaner solution I use here is to put empty overrides in the values file since they'll get actually overridden if needed: VirtusLab/render#11 (comment)
This container is the same one used by the chart-test-action itself. The three commands in sequence: - helm lint: using --strict to fail on warning using --namespace to emulate our ct.yaml config - yamale NOT using --strict because the schema doesn't include all optional fields, so --strict fails - yamllint using --strict to fail on warning NOT recursively searching because templates don't pass
This file can be safely deleted: - Functionality encoded in pr-helm-lint.yml - The fetch here isn't something we need to replicate in pr-helm-lint.yml, since we are controlling via path triggers and don't need the git history anymore during the job (we actually never needed the fetch here, since this action also used a PR path trigger) - I summarized the "hyphenated name" comment in the new action so we don't lose that knowledge
This file can be safely deleted: - Functionality encoded in pr-helm-lint.yml - I summarized the "hyphenated name" comment in the new action so we don't lose that knowledge
…t' into jack-r-warren/GH-820-pr-helm-lint
My new linting complains that there's no version bump, and I suppose it is right
|
Since the PR is coming from a fork, it is using my fork's GitHub Actions. This means:
|
| version: 0.2.7 | ||
|
|
||
| home: https://github.com/broadinstitute/wfl | ||
| appVersion: 0.2.6 | ||
| appVersion: 0.2.7 |
There was a problem hiding this comment.
Even though my change to values.yaml is a no-op, the linting wants a version bump here. If we want, we can revert afa751a to get rid of this version bump (linting will complain but it won't affect anything).
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| paths: | ||
| - helm/wfl/** | ||
| pull_request: | ||
| paths: | ||
| - helm/wfl/** |
There was a problem hiding this comment.
I'm targeting helm/wfl specifically because we'd have to reconfigure this action a bit if we added other charts. Basic linting (just errors) will work on everything because chart-testing has logic to find charts based on ct.yaml, but the strict linting is just done via command line and uses a working directory of helm/wfl.
There was a problem hiding this comment.
LGTM! Thanks for improving this! I have confirmed https://github.com/rexwangcc/wfl/runs/859149051?check_suite_focus=true this works on my fork. It would be great to let helm be more tolerant on version bumps, but that's trivial to me and this PR already does a lot good.
That’s something I have a bit of control over, actually. The version check and a “is there a maintainer” check both come from running |
|
Discussed with folks after standup, determined that we like the version check. Merging. |
Purpose
Changes
--strictnilis "empty"/falsyReview Instructions
fullnameOverride,nameOverride, andserviceAccount.namethen it worksI'll do that, I just figured I could get this in front of other folks' eyes since I'm pretty confident the code here is done.I've now done this, there's no change in behavior I can find (output is the same, arguments still work).