5 ms·

Whatever test runs during pre-commit must also run during normal CI/CD run. In case of Python: black formatter. It must run during normal CI/CD because pre-com

Whatever test runs during pre-commit must also run during normal CI/CD run. In case of Python: black formatter. It must run during normal CI/CD because pre-commit hooks can be skipped. So now I have two different black calls: in the pre-commit hook and in the CI/CD. And they must be of the same version. Ad infinitum for all other tests. This is the reason I don't use pre-commit framework. It leads to "double accounting". I have to sync the pre-commit tests and the CI/CD tests. Or am I missing something? Can the framework run off the venv dir?

woodruffw 5y ago
This is why I stuff everything into a top-level Makefile with `.PHONY` rules instead. Nearly every developer knows how to invoke `make` and already has tab completion for `make` rules, to boot. The CI in turn calls `make` as well, so there's no duplication. For example: https://github.com/trailofbits/pip-audit/blob/main/Makefile https://github.com/trailofbits/pip-audit/blob/main/Makefile
- joshuamorton 5y ago
  It seems like you're still assuming the hard part, ensuring that isort etc. Are installed and have the same configuration (.rc files).
  - woodruffw 5y ago
    Nope. The `make dev` target does that, and the configuration is checked in. You might have missed it, since most of it is embedded in the "pyproject.toml" file.
OakNinja 5y ago
The no-brainer basic usage of any pre-commit hook is to make sure you don’t waste minutes while waiting for your build to finish and then end up with a failure because you don’t have a new line at the end of the file. So, adding lint(ers). Making sure every commit have a ticket reference (and auto add it from the branch name if possible) is a fantastic convenience that also avoids commits referencing the wrong ticket. You can also have a sanity check on what branches you don’t want to cowboy commit on unless you deliberately want to, like the main branch. Black is a tool that is better to run on save. The key to a good pre-commit hook is that it has to be _fast_ otherwise parts of the team will stop using it.
TestTester1 5y ago
We solve this by doing `pre-commit run --all-files` in CI and if that fails, direct the author to docs on how to install pre-commit locally. Black versions are maintained/updated using renovate in the `.pre-commit-config.yaml` https://docs.renovatebot.com/modules/manager/pre-commit/ https://docs.renovatebot.com/modules/manager/pre-commit/
WorldMaker 5y ago
Black is an interesting example because you want two different commands in your pre-commit versus CI/CD, at least in my estimation. In pre-commit you want to run Black in full formatter mode on the staged files. (VS Code's "Format After Save" does most of the work already, but every now and then it slips on something.) In CI/CD you want the check that formatting occurred and block it as a failed test. Those are different commands. For what it is worth, I was evaluating lint-staged [1] and husky [2] for my pre-commit hooks. [1] https://github.com/okonet/lint-staged https://github.com/okonet/lint-staged [2] https://github.com/typicode/husky https://github.com/typicode/husky
ryan_lane 5y ago
I use pre-commit for all of my lint. I also run lint as a test in CI/CD. In the case of github actions, it also auto-fixes the PR, if it didn't run the pre-commit, prior to commit.
quentindemetz 5y ago
Just have your CI run "pre-commit run —all-files"?
- LittlePeter 5y ago
  CI/CD runs from a virtual environment. I guess I have to install pre-commit framework into same virtual environment. Granted, I haven't really looked into it too deeply, so I am a bit embarrassed with my low-effort comment above :-).
  - evanpurkhiser 5y ago
    Here's what we do at sentry.io :) https://github.com/getsentry/sentry/blob/4cf958535563a6b7443b961eac02ea161b3df095/.github/workflows/backend-lint.yml#L50-L79 https://github.com/getsentry/sentry/blob/4cf958535563a6b7443...
Nextgrid 5y ago
My strategy is to put pre-commit in the dev requirements (which includes test-specific packages that would be irrelevant for production) and then CI can run the same hooks that pre-commit runs locally. The objective is that ultimately no non-compliant code can be merged to master; the pre-commit hook (along with all other tests or whatever your CI enforces) is a mandatory step before a PR can be merged, and nobody is allowed to push to master directly.
chriswarbo 5y ago
That's only a problem if your CI/CD is able to have different dependency versions than a dev machine; in which case, you may end up with bigger problems too (e.g. if your Black versions are different, what about your Python interpreter versions?)
ivirshup 5y ago
Pre-commit has it's own CI that does this for you. But if you're in a private repository, you can just have a job that runs pre-commit. The versions of the pre-commit tests are hard-coded into the config. There's also a bot that keeps your pre-commit config up to date with the newest released versions of the checks.
waynesonfire 5y ago
CICD verifies but pre commit applies. Therefore, regardless of whether you use pre commit, your cicd should be verifying that the software, including source code is satisfying the specification. As a collorary, pre commit doesn't need to validate. Eg tell you where exactly the errors are. These are subtle but important distinctions as it nullifies your double accounting concern. The only doubling that exist is that the pre commit and cicd are both made aware of the specification. Maybe that is your concern.
- matharmin 5y ago
  I like seeing exactly what I commit, so I never let a pre-commit script make any changes. It's just a single command to format any changed files in the repo. But some developers in my team do like that convenience of just being able to commit, so that's an option that can be configured in our internal tooling.
  - misnome 5y ago
    Pre-commit doesn’t commit if it makes any changes, it just leaves the changed files for you to either add to the index or re-commit skipping the checks.
matharmin 5y ago
The CI check is the important one: It's what ensures that bad formatting doesn't make it into master. The pre-commit is just a convenience for the developer. It gives faster feedback - immediately when trying to commit, instead of a couple of minutes later. And if a developer on my team wants to disable it, it doesn't affect others. And yes, it's important that you use the same version in dev and on CI/CD. The same applies for practically any other tool you use in your project. I see the CI/CD config as the "master" version - it's a known-working setup, that any developer should be able to use.
- fivea 5y ago
  > The CI check is the important one: It's what ensures that bad formatting doesn't make it into master. I'd say that running linters in precommit hooks is the ideal use case, as linters do not belong in CICD pipelines (i.e., it would make absolutely no sense to block a release just because a whitespace didn't matched an arbitrary rule) and linting stuff before committing also eliminates iterations in code reviews to handle inane stuff like where to break a line or how many spaces come before and after a bracket.
  - matharmin 5y ago
    IMO linters definitely belong into the CI/CD pipeline. I do block a release if there are whitespace issues - it's trivial for the developer to fix, and part of basic code hygiene standards that we enforce in every repository. And if there is an emergency hotfix that needs to go out as soon as possible, we do have processes in place to bypass the CI/CD checks.
  - jen20 5y ago
    That’s a pretty reductive attitude towards linters. If writing Go (for example), linters will tell you if you have ineffective assignments, or have neglected to check an error value - both things that may well block releases. While one could argue these are the job of the compiler, they are not today, and so linting during CI is not only appropriate, but practically required.
    - 5y ago
oauea 5y ago
Just run the same script from both places?
aequitas 5y ago
I have the same issue but the other way around with things like Github Actions. It's really easy to pull actions from the marketplace and apply them in your project. But now you have your linting and testing locked into Github with no easy way to do it locally. So your dev iteration cycle gets bigger. Generic linting configuration files get you a long way, but often enough there are small differences in the way test environments are setup that still causes issues.
- kortex 5y ago
  You can use `act` to run actions locally. Most functionality is supported. Works a treat better than sending it repeatedly just to test how things behave in CI. https://github.com/nektos/act https://github.com/nektos/act
  - aequitas 5y ago
    Have there been any changes to improve it recently? I checked act years back with the old Actions system and a few months back with the new one, but both times ran into issues with supporting most of the actions I was using.
    - kortex 5y ago
      Seems like it's getting more features, slowly but surely.
fivea 5y ago
> So now I have two different black calls: in the pre-commit hook and in the CI/CD. And they must be of the same version. What's the problem, exactly? I worked on projects which enforced precommit hooks running unit tests, and all the precommit and CICD test stages did was run the tests that were already in the repo. Also, I don't understand what would be the problem of calling out-of-synch tests in precommit hooks and the CICD, given that precommit tests are used as a failsafe to not block the pipeline with broken commits.
- claytonjy 5y ago
  The problem is if you use the recommended hooks, pulling from the tool's GitHub repo, that runs off a different venv than the rest of your project. If you leave e.g. black out of your dependencies, and only ever run it with pre-commit, there's not really an issue. I prefer to have everything managed in one place, with black as a dev dependency (I use poetry) so I never use the upstream-maintained hooks, I have to write my own.
kortex 5y ago
Here's our setup, which is the result of several iterations and ergonomics refinements. Note: our stack is 90% python, with TS for frontend. Also 95% devs use mac (there's one data scientist on windows, he uses WSL). We install enough utilities with `brew` to get pyenv working, use that to build all python versions. Then iirc `brew install pipx`, maybe it's `pip3 install --user pipx`. Anyway, that's the only python library binary installed outside a venv. Pipx installs isort, black, dvc, and pre-commit. Every repo has a Makefile. This drives all the common operations. Pyproject.toml (/eslint.json?) set the config for isort and black (or eslint). `make format` runs isort and black on python, eslint on js. `make lint` just verifies. Pre-commit only runs the lint, it doesn't format. It also runs some scripts to ensure you aren't accidentally committing large files. Pre-commit also runs several DVC actions (the default dvc hooks) on commit, push, and checkout. These run in a venv managed by pre-commit. We just pin the version. Github actions has a dedicated lint.yaml which runs a python linter action. We use the black version here to define which black pipx installs. We use `act` if we wanna see how an action runs without sending a commit just to trigger jobs. As an aside, I'm still fiddling with the dvc `pre-commit` post-checkout hooks. They don't always pull the files when they ought to. Most of the actual unit/integration tests run in containers, but they can run in a venv with the same logic, thanks to makefile. We use a dvc action to sync files in CI. So yeah there's technically 2 copies of black and dvc, but we just use pinning. In practice, we've only had one issue with discrepancies in behavior locally vs CI, which was local black not catching a rule to avoid ''' for docstrings; using """ fixed it. On the whole, pre-commit saves against a lot of annoying goofs, but CI system is law, so we largely harmonize against that. IMHO, this is the least egregious "double accounting" we have in local vs staging ci vs production ci (I lost that battle, manager would rather keep staing.yaml and production.yaml, rather than parameterize. Shrug.gif). Other knowledge nuggets: - pre-commit manages its own dependencies. This leads to surprising behavior if you aren't expecting it. Eg you need a special line to specify dvc[s3]. - black has yet to release a non-beta semver, which messes with solvers. This is super annoying. They might as well use 0ver if they don't want to commit to stability. Don't expect any kind of stability of formatting between versions. Hope they settle down soon. - git-lfs is a nightmare. Two projects at $lastco used it. It's more trouble than it's worth. Just use DVC for yucky files. I have no affiliation with dvc, other than a few bug reports. - makefiles are great and IMHO underrated. But they have their limits. More complex logic should be broken out into scripts. - python dependency management is still a kafkaesque nightmare. I say this with over a decade of python experience and it's my favorite language despite this. - Suggestions welcome! Technologies referenced: https://dvc.org/ https://dvc.org/ https://github.com/iterative/setup-dvc https://github.com/iterative/setup-dvc https://github.com/marketplace/actions/python-linter https://github.com/marketplace/actions/python-linter https://github.com/nektos/act https://github.com/nektos/act
l3s2d 5y ago
Is there a reason you can't run pre-commit in CI/CD? That seems to get around your version mismatch issues.
claytonjy 5y ago
> So now I have two different black calls: in the pre-commit hook and in the CI/CD. And they must be of the same version. > Can the framework run off the venv dir? It can, but it means not using the hooks from the tool's repo and writing your own. This blog post [0] shows the pipenv version of some common hooks; mine look the same but s/pipenv/poetry. If there's a way to use the upstream hooks with your local venv, so I can trust the tool maintainers for proper configuration, I'd love to know about it. [0] https://sourcery.ai/blog/python-best-practices/ https://sourcery.ai/blog/python-best-practices/
BasilPH 5y ago
Pre-commit supports reading in multiple types of config files, like pyproject.toml for example. It obviously depends on the tools, but in our case we can use the same configs for pre-commit and CI. It's taken some time to set up, but IMO worth it.
- graton 5y ago
  Curious how you used pyproject.toml. The documentation is large and searching through it didn't give me much information on how to do it, but it sounds interesting!

woodruffw 5y ago

This is why I stuff everything into a top-level Makefile with `.PHONY` rules instead. Nearly every developer knows how to invoke `make` and already has tab completion for `make` rules, to boot. The CI in turn calls `make` as well, so there's no duplication. For example: https://github.com/trailofbits/pip-audit/blob/main/Makefile https://github.com/trailofbits/pip-audit/blob/main/Makefile

joshuamorton 5y ago
It seems like you're still assuming the hard part, ensuring that isort etc. Are installed and have the same configuration (.rc files).
- woodruffw 5y ago
  Nope. The `make dev` target does that, and the configuration is checked in. You might have missed it, since most of it is embedded in the "pyproject.toml" file.

OakNinja 5y ago

The no-brainer basic usage of any pre-commit hook is to make sure you don’t waste minutes while waiting for your build to finish and then end up with a failure because you don’t have a new line at the end of the file. So, adding lint(ers). Making sure every commit have a ticket reference (and auto add it from the branch name if possible) is a fantastic convenience that also avoids commits referencing the wrong ticket. You can also have a sanity check on what branches you don’t want to cowboy commit on unless you deliberately want to, like the main branch. Black is a tool that is better to run on save. The key to a good pre-commit hook is that it has to be _fast_ otherwise parts of the team will stop using it.

TestTester1 5y ago

We solve this by doing `pre-commit run --all-files` in CI and if that fails, direct the author to docs on how to install pre-commit locally. Black versions are maintained/updated using renovate in the `.pre-commit-config.yaml` https://docs.renovatebot.com/modules/manager/pre-commit/ https://docs.renovatebot.com/modules/manager/pre-commit/

WorldMaker 5y ago

Black is an interesting example because you want two different commands in your pre-commit versus CI/CD, at least in my estimation. In pre-commit you want to run Black in full formatter mode on the staged files. (VS Code's "Format After Save" does most of the work already, but every now and then it slips on something.) In CI/CD you want the check that formatting occurred and block it as a failed test. Those are different commands. For what it is worth, I was evaluating lint-staged [1] and husky [2] for my pre-commit hooks. [1] https://github.com/okonet/lint-staged https://github.com/okonet/lint-staged [2] https://github.com/typicode/husky https://github.com/typicode/husky

ryan_lane 5y ago

I use pre-commit for all of my lint. I also run lint as a test in CI/CD. In the case of github actions, it also auto-fixes the PR, if it didn't run the pre-commit, prior to commit.

quentindemetz 5y ago

Just have your CI run "pre-commit run —all-files"?

LittlePeter 5y ago
CI/CD runs from a virtual environment. I guess I have to install pre-commit framework into same virtual environment. Granted, I haven't really looked into it too deeply, so I am a bit embarrassed with my low-effort comment above :-).
- evanpurkhiser 5y ago
  Here's what we do at sentry.io :) https://github.com/getsentry/sentry/blob/4cf958535563a6b7443b961eac02ea161b3df095/.github/workflows/backend-lint.yml#L50-L79 https://github.com/getsentry/sentry/blob/4cf958535563a6b7443...

Nextgrid 5y ago

My strategy is to put pre-commit in the dev requirements (which includes test-specific packages that would be irrelevant for production) and then CI can run the same hooks that pre-commit runs locally. The objective is that ultimately no non-compliant code can be merged to master; the pre-commit hook (along with all other tests or whatever your CI enforces) is a mandatory step before a PR can be merged, and nobody is allowed to push to master directly.

chriswarbo 5y ago

That's only a problem if your CI/CD is able to have different dependency versions than a dev machine; in which case, you may end up with bigger problems too (e.g. if your Black versions are different, what about your Python interpreter versions?)

ivirshup 5y ago

Pre-commit has it's own CI that does this for you. But if you're in a private repository, you can just have a job that runs pre-commit. The versions of the pre-commit tests are hard-coded into the config. There's also a bot that keeps your pre-commit config up to date with the newest released versions of the checks.

waynesonfire 5y ago

CICD verifies but pre commit applies. Therefore, regardless of whether you use pre commit, your cicd should be verifying that the software, including source code is satisfying the specification. As a collorary, pre commit doesn't need to validate. Eg tell you where exactly the errors are. These are subtle but important distinctions as it nullifies your double accounting concern. The only doubling that exist is that the pre commit and cicd are both made aware of the specification. Maybe that is your concern.

matharmin 5y ago
I like seeing exactly what I commit, so I never let a pre-commit script make any changes. It's just a single command to format any changed files in the repo. But some developers in my team do like that convenience of just being able to commit, so that's an option that can be configured in our internal tooling.
- misnome 5y ago
  Pre-commit doesn’t commit if it makes any changes, it just leaves the changed files for you to either add to the index or re-commit skipping the checks.

matharmin 5y ago

The CI check is the important one: It's what ensures that bad formatting doesn't make it into master. The pre-commit is just a convenience for the developer. It gives faster feedback - immediately when trying to commit, instead of a couple of minutes later. And if a developer on my team wants to disable it, it doesn't affect others. And yes, it's important that you use the same version in dev and on CI/CD. The same applies for practically any other tool you use in your project. I see the CI/CD config as the "master" version - it's a known-working setup, that any developer should be able to use.

fivea 5y ago
> The CI check is the important one: It's what ensures that bad formatting doesn't make it into master. I'd say that running linters in precommit hooks is the ideal use case, as linters do not belong in CICD pipelines (i.e., it would make absolutely no sense to block a release just because a whitespace didn't matched an arbitrary rule) and linting stuff before committing also eliminates iterations in code reviews to handle inane stuff like where to break a line or how many spaces come before and after a bracket.
- matharmin 5y ago
  IMO linters definitely belong into the CI/CD pipeline. I do block a release if there are whitespace issues - it's trivial for the developer to fix, and part of basic code hygiene standards that we enforce in every repository. And if there is an emergency hotfix that needs to go out as soon as possible, we do have processes in place to bypass the CI/CD checks.
- jen20 5y ago
  That’s a pretty reductive attitude towards linters. If writing Go (for example), linters will tell you if you have ineffective assignments, or have neglected to check an error value - both things that may well block releases. While one could argue these are the job of the compiler, they are not today, and so linting during CI is not only appropriate, but practically required.
  - 5y ago

oauea 5y ago

Just run the same script from both places?

aequitas 5y ago

I have the same issue but the other way around with things like Github Actions. It's really easy to pull actions from the marketplace and apply them in your project. But now you have your linting and testing locked into Github with no easy way to do it locally. So your dev iteration cycle gets bigger. Generic linting configuration files get you a long way, but often enough there are small differences in the way test environments are setup that still causes issues.

kortex 5y ago
You can use `act` to run actions locally. Most functionality is supported. Works a treat better than sending it repeatedly just to test how things behave in CI. https://github.com/nektos/act https://github.com/nektos/act
- aequitas 5y ago
  Have there been any changes to improve it recently? I checked act years back with the old Actions system and a few months back with the new one, but both times ran into issues with supporting most of the actions I was using.
  - kortex 5y ago
    Seems like it's getting more features, slowly but surely.

fivea 5y ago

> So now I have two different black calls: in the pre-commit hook and in the CI/CD. And they must be of the same version. What's the problem, exactly? I worked on projects which enforced precommit hooks running unit tests, and all the precommit and CICD test stages did was run the tests that were already in the repo. Also, I don't understand what would be the problem of calling out-of-synch tests in precommit hooks and the CICD, given that precommit tests are used as a failsafe to not block the pipeline with broken commits.

claytonjy 5y ago
The problem is if you use the recommended hooks, pulling from the tool's GitHub repo, that runs off a different venv than the rest of your project. If you leave e.g. black out of your dependencies, and only ever run it with pre-commit, there's not really an issue. I prefer to have everything managed in one place, with black as a dev dependency (I use poetry) so I never use the upstream-maintained hooks, I have to write my own.

kortex 5y ago

Here's our setup, which is the result of several iterations and ergonomics refinements. Note: our stack is 90% python, with TS for frontend. Also 95% devs use mac (there's one data scientist on windows, he uses WSL). We install enough utilities with `brew` to get pyenv working, use that to build all python versions. Then iirc `brew install pipx`, maybe it's `pip3 install --user pipx`. Anyway, that's the only python library binary installed outside a venv. Pipx installs isort, black, dvc, and pre-commit. Every repo has a Makefile. This drives all the common operations. Pyproject.toml (/eslint.json?) set the config for isort and black (or eslint). `make format` runs isort and black on python, eslint on js. `make lint` just verifies. Pre-commit only runs the lint, it doesn't format. It also runs some scripts to ensure you aren't accidentally committing large files. Pre-commit also runs several DVC actions (the default dvc hooks) on commit, push, and checkout. These run in a venv managed by pre-commit. We just pin the version. Github actions has a dedicated lint.yaml which runs a python linter action. We use the black version here to define which black pipx installs. We use `act` if we wanna see how an action runs without sending a commit just to trigger jobs. As an aside, I'm still fiddling with the dvc `pre-commit` post-checkout hooks. They don't always pull the files when they ought to. Most of the actual unit/integration tests run in containers, but they can run in a venv with the same logic, thanks to makefile. We use a dvc action to sync files in CI. So yeah there's technically 2 copies of black and dvc, but we just use pinning. In practice, we've only had one issue with discrepancies in behavior locally vs CI, which was local black not catching a rule to avoid ''' for docstrings; using """ fixed it. On the whole, pre-commit saves against a lot of annoying goofs, but CI system is law, so we largely harmonize against that. IMHO, this is the least egregious "double accounting" we have in local vs staging ci vs production ci (I lost that battle, manager would rather keep staing.yaml and production.yaml, rather than parameterize. Shrug.gif). Other knowledge nuggets: - pre-commit manages its own dependencies. This leads to surprising behavior if you aren't expecting it. Eg you need a special line to specify dvc[s3]. - black has yet to release a non-beta semver, which messes with solvers. This is super annoying. They might as well use 0ver if they don't want to commit to stability. Don't expect any kind of stability of formatting between versions. Hope they settle down soon. - git-lfs is a nightmare. Two projects at $lastco used it. It's more trouble than it's worth. Just use DVC for yucky files. I have no affiliation with dvc, other than a few bug reports. - makefiles are great and IMHO underrated. But they have their limits. More complex logic should be broken out into scripts. - python dependency management is still a kafkaesque nightmare. I say this with over a decade of python experience and it's my favorite language despite this. - Suggestions welcome! Technologies referenced: https://dvc.org/ https://dvc.org/ https://github.com/iterative/setup-dvc https://github.com/iterative/setup-dvc https://github.com/marketplace/actions/python-linter https://github.com/marketplace/actions/python-linter https://github.com/nektos/act https://github.com/nektos/act

l3s2d 5y ago

Is there a reason you can't run pre-commit in CI/CD? That seems to get around your version mismatch issues.

claytonjy 5y ago

> So now I have two different black calls: in the pre-commit hook and in the CI/CD. And they must be of the same version. > Can the framework run off the venv dir? It can, but it means not using the hooks from the tool's repo and writing your own. This blog post [0] shows the pipenv version of some common hooks; mine look the same but s/pipenv/poetry. If there's a way to use the upstream hooks with your local venv, so I can trust the tool maintainers for proper configuration, I'd love to know about it. [0] https://sourcery.ai/blog/python-best-practices/ https://sourcery.ai/blog/python-best-practices/

BasilPH 5y ago

Pre-commit supports reading in multiple types of config files, like pyproject.toml for example. It obviously depends on the tools, but in our case we can use the same configs for pre-commit and CI. It's taken some time to set up, but IMO worth it.

graton 5y ago
Curious how you used pyproject.toml. The documentation is large and searching through it didn't give me much information on how to do it, but it sounds interesting!