On Thu, 2024-09-12 at 11:17 -0700, Clark Boylan wrote:
On Thu, Sep 12, 2024, at 10:43 AM, Stephen Finucane wrote:
o/
Trimming this down as I'm mostly interested in the use of pre-commit.
In recent times, we have been slowly introducing use of the pre-commit tool [1]. Initially this was purely additive, as it's a nice help for users working in a terminal (like me!) and mostly duplicated what was defined in the `pep8` target. This duplication led to issues where pre-commit was doing slightly different things from the `pep8` target, so we have evolved this so that the `pep8` target now simply calls pre-commit. This is pretty easy to do now since most if not all of the linters in common use across OpenStack - flake8, hacking, doc8, bandit, etc. - provides pre-commit hooks. I recommend anyone using pre-commit does the same thing otherwise these thing can drift. For example, your pep8 target can become:
[testenv:pep8] description = Run style checks. skip_install = true deps = pre-commit commands = pre-commit run --all-files --show-diff-on-failure
One of my main concerns with pre-commit is that it didn't operate on tool packages (for example wheels from pypi) but instead with tooling source code and then built them all from source. This has a couple of downsides in a CI environment: it makes caching more difficult (we can't rely on our pypi cache) and you need to ensure you've got build toolchains for all the tools invoked by pre-commit. This is less of a concern when you can install these things once (or infrequently) on your local laptop. ya so locally this is effectively a non issue as there is some caching invovled
we have been using pre-commit in our ci now for a number of years. it has not been problemaitc to the stability or speed of the ci form what i have seen but it may have increased the total bandwith consumed. i expect that we are benefiting form the provider mirrors. we refence the repos by https urls to allow caching proxies to cache them if configured and use git tags to pin the version so it changes very in frequently.
Has pre-commit improved on this at all? Quickly skimming the docs it seems that python hook targets must still "hook repository must be installable via pip install . (usually by either setup.py or pyproject.toml)".
i dont think this has change but this has not been a painpoint based on my expirnce with using it both in openstack and in the recent work i have been doing in https://github.com/openstack-k8s-operators/ i proposed the introduction of precommit there 2 years ago https://github.com/openstack-k8s-operators/nova-operator/commit/78a8a7348f44... and we have never had any isssue with using hooks form git in ci or locally as a result of how it functions. it has been very stable for us and very fast. those repos are all written in go so the precise hooks we use are alittle different but it has been a very positive experience. https://github.com/openstack-k8s-operators/nova-operator/blob/main/.pre-comm...
This is potentially extra problematic when you start adding in tools like Ruff that require a full rust toolchain to build. Suddenly we're replacing python slowness with language runtime bootstrapping and tool compilation time. In the ruff case it looks like they've worked around that not by fixing pre-commit but instead by creating a shim repo that will install the prebuilt wheels: https://github.com/astral-sh/ruff-pre-commit. Whatever works I guess. Just be aware that this is sort of a worst case scenario from an Internet is unreliable standpoint as now we need both GitHub and PyPI to be up and accessible rather than one or the other.
on the rust point we already need the rust toolcain in some context because of pycryptography we normlly can avoid that by installign the prebuilt wheel in that case but not alwasy. for the most port we have provided partity in nova so that the existing tox envs can be used in the goglan operators we also provided a dual interface via either a make taget or precommit hook. in this case we had pre-commit local hook just run make if the ruff rust toolcahin was a concern we coudl have pre-commit call a tox target that ran it using a wheel or write our own hook. i will not that there are two general patterns for pre-commit, standalone repos that just provide the hook and in repo hooks. so the approch taken by https://github.com/astral-sh/ruff-pre-commit is actully one of the preferd ways to use pre-commit rather then somethign novel. hacking uses the in tree apprcoh https://github.com/openstack/hacking/blob/master/.pre-commit-hooks.yaml but i belive ```- .[pep257]``` could be repalced with ```- -c https://releases.openstack.org/constraints/upper/master .[pep257]``` or ```- -c https://releases.openstack.org/constraints/upper/master hacking[pep257]``` i have not actully tried that but i belive the additional_dependencies is appened to the pip install line directly so i think we can pass extra flags. https://github.com/pre-commit/pre-commit/blob/504149d2ca57f3115ccd4fa9c6ae69... if that does not work that would be a nice enhancement.
This also has the downside of sidestepping constraints, but in the case of linters we've already largely done that so that projects don't have to update in lockstep. This doesn't really change the status quo there.
i agree this largely does not change that aspect.