[CLOUDP-343558] Run helm chart-testing tool ct as part of CI (lint_repo)
#450
Conversation
viveksinghggits
added
the
skip-changelog
MCK 1.4.1 Release Notes |
There was a problem hiding this comment.
Changes look good, still I have one general ask.
When we add or change logic in our linting requirements we should also consider adding them to .githooks/pre-commit. Otherwise there is no way locally to lint the helm charts.
Secondly we should make scripts that are run in EVG CI easy to run locally as well. I wouldn't put them in scripts/evergreen/ directory, but in scripts/helm and make them available for developers. I think also .githooks/pre-commit takes care of installing all prerequisite applications.
Somehow all the efforts to set path env var using below code
```
- command: subprocess.exec
type: test
params:
add_to_path:
- ${workdir}/bin
- ${workdir}/venv/bin
working_dir: src/github.com/mongodb/mongodb-kubernetes
binary: scripts/evergreen/lint_helm_chart.sh
```
I even tried to remove `subprocess.exec` and use `shell.exec` and used
`env:` for params, that didn't work as well. that's why we are finally
settling on to setting path at the lowest level. Otherwise `ct` CLI resulted
into `yamale` and `yamllint` are not found.
.evergreen-functions.yml
Outdated
| - ${workdir}/venv/bin | ||
| working_dir: src/github.com/mongodb/mongodb-kubernetes | ||
| binary: scripts/evergreen/check_precommit.sh | ||
| - command: subprocess.exec |
There was a problem hiding this comment.
I would prefer this to be part of pre-commit script instead
There was a problem hiding this comment.
Since we didn't discuss this in last discuss meeting @lsierant. Is it ok if we handle this comment later after the discussion, so that we can merge this PR? Right now I have added a way to run this script locally.
There was a problem hiding this comment.
I also prefer it to be part of our pre-commit script, otherwise i need to chase-down scripts
There was a problem hiding this comment.
I would prefer:
- To be able easily to run this separately (
make lint-chartis perfect). So I can run it separately from everything else. - Have a separate CI job for it (so not part of
lint_repo) because whenlint_repofails it you need to go through other logs because it is not immediately clear it is an issue with Python linting or Go linting or Bash linting or maybe an issue with licenses?
I'm good for it to be included in a pre-commit if it
There was a problem hiding this comment.
I have made this part of the pre-commit and have kept lint-chart as well.
@m1kola for the second point that you mentioned, when we make this script part of pre-commit it would be automatically be part of the lint_repo task because it eventually just runs the pre-commit script. And I think it makes sense as well considering, we are actually linting the helm chart under lint_repo.
There was a problem hiding this comment.
That is the issue - I think lint_repo is too loaded with checks so the output became overly difficult to comprehend for a linting task. Today when I see lint_repo failed it takes me more time than it should to find out what linting I messed up.
lint_repo checks (probably not the full list):
- Release json/generated yamls
- Licenses
- Go linting
- Python linting
- Bash linting
- Now helm chart as well.
I think each of these things should be a separate task so we can quickly glance at them and understand what has failed instead of going through logs searching for specific failures. It gets worse when you mess up multiple things at the same time (e.g. Python and bash linting).
That is why I was suggesting not to add more to this complexity. I was hoping that the new checks can go into separate jobs and we will eventually split existing linting into separate jobs.
I don't want to block this PR on this - feel free to merge. But I think this is the direction we should be going with our linting jobs.
There was a problem hiding this comment.
I agree that it makes sense to separate those tasks, but setting new evergreen job per each linting tasks seems overkill. I would rather have them separated in bash scripts and then call them in separate evergreen commands/functions. This will help navigate in the task output and make it easier find the offending bash script/linting task.
viveksinghggits
force-pushed
the
lint-chart-in-ci
branch
from
51d2917 to
add22fe
Compare
viveksinghggits
requested review from
MaciejKaras,
fealebenpae and
lsierant
.evergreen-functions.yml
Outdated
| - ${workdir}/venv/bin | ||
| working_dir: src/github.com/mongodb/mongodb-kubernetes | ||
| binary: scripts/evergreen/check_precommit.sh | ||
| - command: subprocess.exec |
There was a problem hiding this comment.
I also prefer it to be part of our pre-commit script, otherwise i need to chase-down scripts
6 participants
Summary
Whenever we raised PR in to helm chart repo CI failed there because we had linting issues in our helm chart. A previous PR that I raised fixed the linting problem. But we should have a linting mechanism in our CI that would consistently check if we have made any mistake in our charts. This PR does that.
The
chart_schema.yamlandlintconf.yamlare the default yaml files using whichctshould be run. Andctis the tool that is used for linting in our helm charts repo.Proof of Work
Passing
lint_repotask here https://spruce.mongodb.com/version/68c43aae1d5d5d000770c624/tasks?sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC and in this PR as well.Checklist
skip-changeloglabel if not needed