Add pre commit hooks support

[lhriley]

What this PR does / why we need it:

This PR adds support for pre-commit hooks in a variety of options.

• The generic hooks support the local ct binary
• The golang hooks build the ct package in a virtual environment managed by pre-commit
• The container hooks depend on Docker and use the latest official container image

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes # n/a

Special notes for your reviewer:

This can be tested with a configuration similar to the following:

---
repos:
  - repo: https://github.com/Labelbox/chart-testing
    rev: add-pre-commit-hooks-support
    hooks:
      - id: ct
        entry: ct version

      - id: ct-lint-golang
        args:
          - --config=.ct.yaml
          - --lint-conf=.yamllint

      - id: ct-lint-container
        args:
            - --config=.ct.yaml

Additionally, it should be noted that the container variations require a bash script (included) to work properly. Using the native docker_image pre-commit "language" is not enough due to the requirements of ct. The basic configuration is based on the information in the README.md and includes some tweaks to work with pre-commit.

Of special note, I have included a hack in the script to mount the gcloud SDK into the container as well, since we use this for IAM access to our OCI repository in Google Artifact Registry. I assume similar functionality could be added to support other providers. The alternative is to build containers FROM the official chart-testing image or include dead weight in the official container. I opted for the hack to avoid the alternative.

Related:
pre-commit/pre-commit#2968

[lhriley]

In testing over the last week I've come across a weird issue when using the golang version of the pre-commit hooks. It is unable to find the chart_schema.yaml file in any of the locations it is looking for it.

In my case, I had a copy in /etc/ct/chart_schema.yaml from previously installing the ct binary. For another user, they had never run ct before, and got the following error:

❯ pre-commit run -av ct-lint-golang
chart testing lint (please wait)....................................................Failed
- hook id: ct-lint-golang
- duration: 0.01s
- exit code: 1

Linting charts...
Error: failed loading configuration: 'chart_schema.yaml' neither specified nor found in default locations
failed loading configuration: 'chart_schema.yaml' neither specified nor found in default locations

These files should be available in the hook, as they are provided in the git repo under etc. I believe that this issue would be resolved by renaming the etc folder to .ct amd updating the documentation.

Update: This did not work as expected. Adding the expected files into our git repo under .ct did fix the issue, however.

Is there any option in the config files to specify where to look for the schema / linting files?

[cpanato]

maybe we can move this to a directory, myabe called tools or something else?

[lhriley]

@cpanato I'm not sure that I understand your ask, Do you mean move the bash script to a different directory? i.e. not .pre-commit/bin/pre-commit-ct-container.sh ?

Instead:

tools/pre-commit-ct-container.sh

[cpanato]

@cpanato I'm not sure that I understand your ask, Do you mean move the bash script to a different directory? i.e. not .pre-commit/bin/pre-commit-ct-container.sh ?

Instead:

tools/pre-commit-ct-container.sh

yes, something like that, move all those scripts to a specific directory, this is more a support tool for users using the chart-testing for the charts and not much for this repository and flow here.
or am I missing something?

[lhriley]

You are correct. These scripts are used by pre-commit locally, and not related to repo maintenance.

[lhriley]

pushed a commit moving the script as requested

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[cpanato]

The dot file should be in the same directory as well

[cpanato]

I would add all files the dot file and the .sh under 'tools/pre-commit'

[lhriley]

@cpanato pre-commit expects the .pre-commit-hooks.yaml file to be in the root of the host git repo (this one). I'm not sure that there is a way to accommodate moving it to an arbitrary folder at least nothing in the documentation mentions a way to do so: https://pre-commit.com/#new-hooks

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions]

This PR was closed because it has been stalled for 10 days with no activity.

[lucacome]

Is there any way that we can bring this back to life? 🙏

[cpanato]

yes, sure, but this hook have nothing to do in this repository, so that why i asked to add in a specific directory, and the users can download that and install the hook in the repo they want

[znd4]

I don't think there's any way to implement this without /.pre-commit-hooks.yaml.

There used to be a practice of people creating mirrors / forks of popular projects and putting pre-commit configuration in them, but it's fallen out of favor for obvious reasons.

If you don't want to add a dotfile to the root project, it is what it is 🤷

[cpanato]

My main issue with accepting this PR is that this pre-commit hook is to be used when dealing with helm charts, not here in this repo, which is the tool that is used in ci.

People do not need to clone this repo to build their helm charts, so that should be an example of how to implement the hook as a document.

Do you have any thoughts @davidkarlsen?

[lucacome]

@cpanato people don't need to clone this repo to use the pre-commit hook, at least not directly, pre-commit takes care of that

[cpanato]

@cpanato people don't need to clone this repo to use the pre-commit hook, at least not directly, pre-commit takes care of that

I am dumb. Can you explain how this will work when people are working in another repository for the helm charts? Let's say I am updating my chart in this repo: https://github.com/falcosecurity/charts/tree/master/charts/falco-exporter. How will this pre-commit hook work? and i don't have https://github.com/helm/chart-testing cloned in my machine

[znd4]

To add context, in addition to orchestrating command execution, pre-commit is a package manager.

The main benefit of using pre-commit is that it provides reproducibility by installing a version specific to the current git repo. When I get back to my keyboard I'll share an example configuration + GitHub action run

[znd4]

this is a pre-commit config on one of my projects.

If you check the build step in this pre-commit.ci job, you can see that pre-commit handles installing the targeted CLI tools that have been configured in my project's .pre-commit-config.yaml.

An aside to call out a possible alternative route:

ruff-pre-commit is a repo that exists solely for pre-commit to point to when installing ruff. It might be more complicated to set up and maintain than what's in this PR, but a similar helm/chart-testing-pre-commit repo could probably be created. (Ruff calls out that they use a standalone repo to enable installing binaries, which is not really a concern for a go project)

[aiell0]

Just piping in to say I would love this feature. Would help make our pipelines fail faster, as right now people don't become aware of linting errors until the github action runs after a commit, which is frustrating (especially when it's simple things like bumping the version of the chart, for example).

[davidkarlsen]

this is a pre-commit config on one of my projects.

If you check the build step in this pre-commit.ci job, you can see that pre-commit handles installing the targeted CLI tools that have been configured in my project's .pre-commit-config.yaml.

And here we get to the core. My view aligns with @cpanato that the pre-commit hooks should reside in the repos that host the charts not this repo which hosts tooling. It would not benefit consumers of the ct tool - in fact it would influence any committers to chart-testing, as it would trigger the hooks and probably fail as it is not a chart repo.

What we could do however, is to offer a gist in this repo to show how one can integrate other 3rd party tooling around your chart development - that could make sense.

[davidkarlsen]

hang on... researching a bit... I'll get back

[davidkarlsen]

yes, sorry, I was jumping to conclusions a bit fast.

It seems patterns are like:

• https://github.com/gruntwork-io/pre-commit
• https://github.com/antonbabenko/pre-commit-terraform/

perhaps we should rather create a separate repo for offering these hooks. That seems like a more widespread pattern - and also more clearly separates the tool itself from usage of the tool through a hook. But I agree that it provides value to offer a central definition of the hooks for folks to be able to consume/hook into (pun intended)

[cpanato]

yes, sorry, I was jumping to conclusions a bit fast.

It seems patterns are like:

• https://github.com/gruntwork-io/pre-commit
• https://github.com/antonbabenko/pre-commit-terraform/

perhaps we should rather create a separate repo for offering these hooks. That seems like a more widespread pattern - and also more clearly separates the tool itself from usage of the tool through a hook. But I agree that it provides value to offer a central definition of the hooks for folks to be able to consume/hook into (pun intended)

agree in having a new repo and put that there, what will be the name helm-chart-pre-commit-check?

or pre-commit-charts

[lucacome]

Those two repos are not very representative of the ecosystem. It's far more common to have the hook config in the same repo as the tool.
You would have to mirror this repo just to add one yaml file .pre-commit-hooks.yaml.

[sabre1041]

First and foremost @lhriley thanks for not only starting the conversation, but for the integration that will enable ct in the pre-commit ecosystem.

Regarding the key question within this issue (where hooks for ct should be defined [coexisting in the same repository or within a dedicated repository]), I would suggest the option of a separate repository for the purpose of pre-commit hooks. From what I have seen in my cursory searching of existing usage, I can certainly see there being a need when the associated tool uses a scripting language or other assets within the repository.

However, for at least ct, there is no dependance on any of the files included in the repository as it is merely executing a binary already present on the users' machine. Separating into its own repository also allows for the creation of additional hooks for other aspects within the helm ecosystem that might be of use to end users

[lhriley]

Hi all, first let me say thanks to everyone who has participated in this discussion. I'm not able to contribute any code changes at the moment, so I would appreciate it if someone could handle any changes requested or moving the code to a different repository if that's the decision.

I'd like to add some comments, however, since there is an incomplete picture of how pre-commit would work in this context.

However, for at least ct, there is no dependance on any of the files included in the repository as it is merely executing a binary already present on the users' machine. Separating into its own repository also allows for the creation of additional hooks for other aspects within the helm ecosystem that might be of use to end users

While the statement above could be true, one of the features of pre-commit is that it will handle the downloading or building of the binary (or Dockerfile, etc) as long as the hook is defined properly and the repo is a language pre-commit can manage. This means the user never has to manually download / install the binary for a language like golang since pre-commit will checkout the code and build the binary automatically.

The nice part about this is that the version of the tool can be pinned to a unique git commit, and then when pre-commit runs, it builds a binary from that specific commit. This reduces the likelihood that two different users or systems would run ct at two different versions and get two different results. By default, pre-commit prefers to work with pinned (immutable) git commits rather than references like branches or tags which are mutable.

One benefit is that this can speed up CI dramatically when cache is configured properly as multiple versions of ct can be in the cache at any given time, whereas work would need to be done to isolate and specify binaries with explicit versions.

So, even though there is a pattern where some tools use a separate repo for hooks, that's not the intended / ideal use case for a tool such as ct (or something written in python, ruby, etc). External hooks repos are generally used when third parties are providing hooks for tools that don't provide them, the codebase crosses several git repos, or the code isn't available while a binary is public (eg: a container image).

I honestly don't have a strong opinion for which pattern to adopt. There are a few hacks to make the hooks work as is, and if the maintainers would prefer a separate repo there are a few limitations that would arise that would need to be considered:

1. users couldn't pin to an arbitrary git commit of ct (i.e. only releases, binaries)
2. pre-commit won't be able to manage the application as it would if the hooks were in the same repo; meaning that the user would likely have to provide the binary themselves (via a package manager, etc)
3. the hooks would need to be written in such a way that they handle the above issues (expect lots of hacks, support scripts)
4. additional maintenance required to maintain versions/releases in the external hooks repo

Ideally, the hooks should remain in this repo for full functionality, but if that isn't "allowed", then I would ask that someone pick up the work from this PR and run with it. 🖖🏼

[scottrigby]

hi @lhriley, echoing Andy's thanks for the PR and convo. Also thanks for your last thoughtful reply.

A few clarifications:

1. release immutability: Helm and helm sub-projects hold a strong backwards compatibility promise, and therefore treat tags as immutable
2. managing the application: Chart maintainers don't build ct locally, but rather always use the pre-build packages. Only people developing ct would be working with branches/commit SHAs
3. hacks and/or support scripts: Can you please clarify the which of these you have in mind that we'd need if hooks were added to a separate repo as opposed to this one?
4. managing versions/releases in the separate git repo: This one I could see being an issue for some folks, however we already manage this for GitHub actions repos. I'm also not sure we need or even want to manage which version of ct users are using. For example, ct itself doesn't manage the user's version of Helm - this is on purpose - as it's meant to allow users to also test any version of helm if they desire, including their own modifications before submitting a PR.

So in summary, apart for something you might be able to clarify in point 3, I think the downsides don't apply to this specific tool. But please correct me if there are some major unsavory hacks or duplicated code needed for some reason?

A few more thoughts:

• I do however think we should link to the new pre-commit hooks repo from the README in all other applicable repos (for now, just this one, since there's only ct lint, but in future for any of the other applicable repos for future Helm-related hooks), and from the docs at https://helm.sh
• I also can imagine Helm having other pre-commit hooks than for chart-testing. ct lint is for chart developers, but we have other user profiles as well (https://github.com/helm/community/blob/main/user-profiles.md).
• One other thing about moving to a new repo. I woudn't want to loose your commit history that credits you. In cases like these, I recommend whenever possible bringing over the original author's commits (and when not possible, add co-authored-by commit trailers) so that you are properly thanked ❤️

[lhriley]

Regarding:

hacks and/or support scripts: Can you please clarify the which of these you have in mind that we'd need if hooks were added to a separate repo as opposed to this one?

There are weird edge cases that I would expect anyone working across repos might encounter. I'm not clear on what would be required, and it would need to come out of testing.

I simply mentioned the possibility as I've had to write several scripts to work around design limitations with pre-commit itself after having some discussion with the creator. This is mostly based on the fact that pre-commit is highly opinionated, and while it has some flexibility, you must work around many limitations that are by design by providing wrapper scripts.

I would refer you to the scripts that I wrote to make the containerized releases compatible with pre-commit as part of this PR. They replicate and extend some of the internal logic that pre-commit uses to mount files into the running container.

Furthermore, there are some odd issues with ct itself, from what I recall. There are files the binary seems to require (or used to) which it couldn't find under certain circumstances, and I believe there's a script to deal with this in the PR as well.

Additionally, the biggest annoyance I've experienced is that pre-commit is very cache heavy. Meaning it will go out of its way to prefer cache and never check the same reference for an update. In my experience this can be pretty unclear for containers specifically if the hook depends on a floating (mutable) reference/tag. This can cause two users who init their hooks at different times to end up with different versions, and pre-commit will never tell them a new version is available. So, every release will need a corresponding commit that bumps the tag, regardless of whether this or a new repo is used.

Again, I'm not able to contribute any code at the moment for personal reasons, despite the fact that I would love to get first commit credit in one of this organization's repos. It could be possible at some point in the future, but I'm not clear when that might be. I'm happy to give guidance when possible, in whatever limited capacity that takes.

[github-actions]

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions]

This PR was closed because it has been stalled for 10 days with no activity.