-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
[Feat] Use pre-commit.ci workflow #3255
Comments
This looks good. I can press the button at pre-commit.ci. Their pricing: "pre-commit.ci will always be free for open source repositories."
I agree that this is the main drawback. In the past, I would be very concerned about contributors being very confused by that, but now I think you are more likely to deal with the extra commit than get all the linters right locally. I myself have a mixed experience using this feature with MegaLinter, so I'm curious how this will work. Alternatives: We could set it up ourselves in GitHub Actions in some way (e.g. with |
The main advantage I think that made it a good idea, was that it ran on another service, so we don't get bogged down when there is a lot of activity here, or like when gdal prepares a release. That meant faster cycles, and most of the time, the fix would come back cancelling the first jobs that wouldn't have started yet. If we were to use github actions directly, so we stay in full control, then since they don't want to support or maintain anymore this way of doing (and their action), we would be better off with something more like megalinter (correctly tuned), or using super-linter at its full potential, if we want to stick with it. |
I'm all in for at least have a trial period with pre-commit.ci, we won't loose anything but precious CI time. |
Enabled. "no recent runs. push or open a PR to trigger a run" From the documentation:
This may be a problem for release branches.
Isn't this a problem?
|
We'll see.. It may not be bad depending if they file a PR for it. |
Disabled. It fails if any files are not compliant, so we need to fix all files first to comply with pre-commit settings. The autofix resulted in "unknown error". My search for that only resulted in unrelated, but relevant comments about being confused by the auto-fixes. I must say I didn't like the experience with the interface which is partially (completely?) related to GitHub rather than pre-commit.ci. It defaults to "apply to all repos under org", so it is easy to accidentally enable it for all your or org's repos. This might be again a GitHub thing, but to disable, you need go through GitHub, not pre-commit.ci and it finishes again with apply to all orgs. I think we at least need to fix all the files before enabling it again. Good old |
The positive point was that it was fast to start and finish! |
We have tried it previously in the repo, and wasn’t suitable for our needs, it didn’t work the way we hoped to. |
Is your feature request related to a problem? Please describe.
It is obvious that not everyone/everything(!) is using pre-commit, which is OK as it was never a requirement. We have been aware of the discrepancies between the checks of pre-commit (as per the config file) and the checks we use in CI (in particular the super-linter). Unsurprisingly, this have now lead to issues to update some files without making unrelated changes (because pre-commit will not allow it). I recently experienced it first-hand making changes to a yaml file with modifications made by Renovate bot update.
Describe the solution you'd like
This situation can be mitigated by using pre-commit.ci for this repository.
For this to work the following pull requests need to be merged:
To then enable pre-commit.ci on this repo, an admin with enough access rights, need to sign into GitHub via https://pre-commit.ci and give it repository access. That's all there is to do. The config file
.pre-commit-config.yaml
does the rest.Now, how does this work in real life?
Every commit is checked by pre-commit.ci as if pre-commit were executed locally: the hooks that are formatters, e.g. 'trailing-whitespace', 'black', clang-format' actually do the format and the changed files are added with a new commit to the PR. The other hooks only fails the workflow.
As we always squash the PRs, the automatically added commit do not matter much for the commit history of main. But the commit need to be pulled before adding any additional commits by the PR submitter. This is the main "drawback" with this, but as long as we are aware of this it may be acceptable.
You can see how it works/look like in my test-field at nilason#5.
Describe alternatives you've considered
There are no good alternatives that come to my mind.
The text was updated successfully, but these errors were encountered: