Skip to content

Add a pre-commit workflow #11

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add a pre-commit workflow #11

wants to merge 2 commits into from

Conversation

jmcarcell
Copy link
Member

This uses an image with alma9 and is faster, around 1:30 compared to at least 2
minutes that is the minimum it takes in some of our repos. We have fancier ones
like the one in EDM4hep that even build podio (https://github.com/key4hep/EDM4hep/blob/main/.github/workflows/pre-commit.yml)

I don't think there is any exception to this, any generated code will not be
tracked so I don't think we need to do anything than run pre-commit.

@tmadlener
Copy link
Contributor

Should we consider using a different configuration for the CI? That would make it possible to have a .pre-commit-config.yaml that uses pre-built actions (and potentially skip some of the checks locally) and have a .pre-commit-config-ci.yaml that we use for CI, via pre-commit run --config .pre-commit-config-ci.yaml.

C.f. key4hep/EDM4hep#402

@jmcarcell
Copy link
Member Author

With the approach that I propose in key4hep/key4hep-dev-utils#12 that is not trivial. So there would be one global pre-commit configuration file plus possibly one local for the current repository - that would be run in CI. Now we would want some hooks to not run if locally. I think something could be done in the script that parses the configurations to check if running in CI or not via, for example, an env variable. Then we can maybe encode in the id or the name that a hook should only be run in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants