Skip to content
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

using chartpress with pre-commit #38

Closed
rokroskar opened this issue Jun 18, 2019 · 9 comments
Closed

using chartpress with pre-commit #38

rokroskar opened this issue Jun 18, 2019 · 9 comments
Labels
documentation help wanted Extra attention is needed

Comments

@rokroskar
Copy link
Contributor

I've started using chartpress --reset with the pre-commit package and it works very nicely to prevent accidental commits of modified chart values. It would be great to have the .pre-commit.hooks.yaml definition as a part of chartpress so one could simply include it like any other pre-commit hook. The user of chartpress would then have a .pre-commit.config.yaml in their repo's base directory with something like:

repos:
-   repo: https://github.com/jupyterhub/chartpress
    rev: master
    hooks:
    - id: chartpress

If then you try to commit a change to the chartpress-managed values you get something like:

git commit -am 'commiting something'
black................................................(no files to check)Skipped
Flake8...............................................(no files to check)Skipped
chartpress reset.........................................................Failed
hookid: chartpress

If you want to include something like this I'll happily contribute a PR.

@minrk
Copy link
Member

minrk commented Oct 17, 2019

chartpress is often pretty expensive to run, which can be quite frustrating for pre-commit hooks, which need to be very quick to avoid causing friction. But it would at least be interesting to see an example

@rokroskar
Copy link
Contributor Author

Sure, my suggestion here is to only run it with the reset flag to avoid committing rendered charts - that should be very cheap to run.

@rokroskar
Copy link
Contributor Author

@minrk here's a full example of my .pre-commit-config.yaml file:

repos:
-   repo: local
    hooks:
    - id: chartpress
      name: chartpress reset
      files: helm-chart/*
      description: Run `chartpress --reset` to clean up helm charts before committing.
      entry: pipenv run chartpress --reset
      language: system
      pass_filenames: false

The result is something like this:

# rendering the chart
$ chartpress

# trying to commit
git commit -am 'bla'
chartpress reset.........................................................Failed
hookid: chartpress

Files were modified by this hook. Additional output:

$> git log -n 1 --pretty=format:%h -- . . jupyterhub singleuser
$> git log -n 1 --pretty=format:%h -- . chartpress.yaml
$> git log -n 1 --pretty=format:%h -- jupyterhub jupyterhub chartpress.yaml
$> git log -n 1 --pretty=format:%h -- git-clone git-clone chartpress.yaml
Updating helm-chart/renku-notebooks/values.yaml: image: {'repository': 'renku/renku-notebooks', 'tag': 'latest'}
Updating helm-chart/renku-notebooks/values.yaml: jupyterhub.hub.image: {'repository': 'renku/jupyterhub-k8s', 'tag': 'latest'}
Updating helm-chart/renku-notebooks/values.yaml: git_clone.image: {'repository': 'renku/git-clone', 'tag': 'latest'}

@consideRatio
Copy link
Member

chartpress --reset is or at least should have very high performance. It will probably be about as easy to setup a local hook as it would be to declare one in this repo though right, since it is just a one liner. From the example you provided, it seems like it makes a lot of sense to use a local hook, as there are some local chart configuration matters as well.

@rokroskar perhaps we could have this local hook example documented in the readme? What do you think?

repos:
-   repo: local
    hooks:
    - id: chartpress
      name: chartpress reset
      files: helm-chart/*
      description: Run `chartpress --reset` to clean up helm charts before committing.
      entry: pipenv run chartpress --reset
      language: system
      pass_filenames: false

@rokroskar
Copy link
Contributor Author

rokroskar commented Mar 26, 2020

@consideRatio yes I think it could be useful for some - has definitely saved me from having to rebase/force-push my commits all the time :)

But you might want to omit the pipenv parts, since those are presumably user-specific.

@consideRatio consideRatio added documentation help wanted Extra attention is needed labels Nov 16, 2020
@consideRatio
Copy link
Member

consideRatio commented Dec 26, 2020

I explored pre-commit a bit further and were happy to learn it really isn't disruptive at all, it works on a lines of code level and will not modify anything except whats conflicting etc.

I added the following .pre-commit-config.yaml entry to the jupyterhub/zero-to-jupyterhub-k8s helm chart in jupyterhub/zero-to-jupyterhub-k8s#1970

A problem for people may be that chartpress can only be run from the folder where chartpress.yaml is located, but I assume pre-commit will invoke it from the git repo root folder which may not be the same, so the entry will possibly need to include a directory change first.

I've no experience with chartpress hooks definitions etc, but if we can do something like...

  1. search for a chartpress.yaml file in the repo
  2. execute chartpress from there
  3. override default files trigger to /Chart.yaml|/values.yaml changes (or whatever the syntax will be).

Then I'd be up for defining a .pre-commit-hook.yaml as part of this repo and not only provide an example. But, in the linked PR, it wouldn't just work for the jupyterhub/binderhub repo I think for example, because of the difference of having chartpress.yaml defined in non-root folder

@rokroskar
Copy link
Contributor Author

Thanks for following up on this @consideRatio - I've been using this setup for some time and it works well. A question though:

A problem for people may be that chartpress can only be run from the folder where chartpress.yaml is located

I've always wished for a -f flag with chartpress 😄 - afaik all the paths are defined relative to the chartpress.yaml file so in principle there is no hard-coded dependency on cwd that couldn't be overcome.

@manics
Copy link
Member

manics commented Apr 1, 2021

Does #118 Allow chartpress to be run from a different folder cover the last feature request? Is there anything else required here or can we close this?

@consideRatio
Copy link
Member

Let's close this and not maintain a chartpress pre-commit hook that is very easy and plausible to define locally to do whats wanted anyhow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants