Skip to content

Adjust pre-commit step in CI to run every time, but fast fail if red #5041

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

Closed
michaelosthege opened this issue Oct 5, 2021 · 6 comments
Closed

Comments

@michaelosthege
Copy link
Member

In a glossary PR I merged yesterday the docs step ran, but the pre-commit step did not.
We can easily run the pre-commit also for PRs of new contributors.

Also we should probably fast-fail the CI if the pre-commit is red.

@michaelosthege
Copy link
Member Author

Looks like this is really hard when pre-commit and tests are separate GitHub Actions workflows: pre-commit-ci/issues#106

@ricardoV94
Copy link
Member

ricardoV94 commented Jan 26, 2022

Could we have the pre-commit run in each job? Would that make things easier?

@michaelosthege
Copy link
Member Author

It looks like we're getting a switch to mamba with #5387, but to solve this issue, I'd prefer to pursue this:

[...] combine pre-commit.yml, arviz_compat.yml and pytest.yml into the same workflow.
Then we could fail-fast all jobs when the pre-commit doesn't pass (#5041)

quoted my comment in #5375

That'll be less steps in total and cancel/rerun would be a single click.

@michaelosthege
Copy link
Member Author

I'm not sure how to interpret the docs about the fail-fast setting. (Asked a question in github/docs#16401)

The trigger filters are another hurdle: They apply to the workflow but we want to run pre-commit every time. So if we include a pre-commit job in the tests.yml workflow, we'll have to keep the other pre-commit workflow around too.

All in all not elegant either way.

@michaelosthege
Copy link
Member Author

Update: the fail-fast only applies to the same matrix. So we'd have to restructure everything into the same matrix, or make all jobs depend on the pre-commit job (delays all jobs by a few minutes).

@ricardoV94
Copy link
Member

Second option doesn't sound so bad

@ricardoV94 ricardoV94 closed this as not planned Won't fix, can't repro, duplicate, stale May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants