Skip to content
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

Consider trying pre-commit.ci for autoupdate PRs + faster CI #687

Closed
sirosen opened this issue Feb 4, 2022 · 9 comments
Closed

Consider trying pre-commit.ci for autoupdate PRs + faster CI #687

sirosen opened this issue Feb 4, 2022 · 9 comments

Comments

@sirosen
Copy link
Collaborator

sirosen commented Feb 4, 2022

I recently noticed that typeshed has started using pre-commit.ci: https://results.pre-commit.ci/repo/github/31696383

@sloria, @lafrech, I'm wondering about webargs as the first repo to try this, and then rollout to other repos if we like the result.

I'm starting to experiment with this on some personal repos today, and, if that works well, I'd like to try this out here.
It doesn't entirely eliminate manual updates -- e.g. additional_dependencies need to be updated manually when pinned.

@sloria
Copy link
Member

sloria commented Feb 6, 2022

Yes please! I've been meaning to try out pre-commit.ci but haven't had the time. Would be great if you could evaluate it in webargs then roll it out to the rest of marshmallow-code. 🙏

@lafrech
Copy link
Member

lafrech commented Feb 6, 2022

I'd be glad to drop Azure.

I've been using Github CI for various repos and I'm happy with it (and its integration in GH).

What does pre-commit CI brings that isn't in GH CI already?

@sirosen
Copy link
Collaborator Author

sirosen commented Feb 6, 2022

I also want to move other parts of the build to GitHub actions. The UI in GitHub works much better when builds are done in Actions -- perhaps unfortunate, but true. However, I'd still consider doing linting in pre-commit.ci

There are three major reasons for that:

  • it will push autofix commits on PRs (easier for new contributors)
  • autoupdate PRs for precommit config
  • very, very fast runs (I've had it complete under 10s after a commit), as it is not a generalized CI system

I'll try setting it up today. I think someone with admin on the org or repo may need to allow the pre-commit.ci app access, but I don't quite remember. I may need to follow up on that later.

@lafrech
Copy link
Member

lafrech commented Feb 6, 2022

OK so that's just the linting. Anyway, I'm happy to see how it goes once you set it up here.

I think someone with admin on the org or repo may need to allow the pre-commit.ci app access

Congratulations! You've just been promoted repo admin.

@sloria
Copy link
Member

sloria commented Feb 6, 2022

yeah i'm not opposed to migrating to GH actions. the major blocker was Azure Pipeline's reusable templates feature, but GHA has similar functionality now.

@sirosen
Copy link
Collaborator Author

sirosen commented Feb 6, 2022

I configured the pre-commit.ci integration for the webargs repo. I'm a little bit hazy on how the auth details for the process work, but I think there's now a webargs<->pre-commit.ci setup which is not tied to my GitHub user.

In other repos, it looked to me like pre-commit.ci picks up on the first pull request event to start builds, so I put in #688 .

I can see a build success here:
image

and it's in the status checks for that PR.


I'll open a separate issue to discuss using GH Actions.

@lafrech
Copy link
Member

lafrech commented Feb 6, 2022

In flask-smorest, the release tasks depends on the lint and tests to be successful.

https://github.com/marshmallow-code/flask-smorest/blob/master/.github/workflows/build-release.yml

Can this be achieved while doing the linting in pre-commit CI rather in GHA itself?

@sirosen
Copy link
Collaborator Author

sirosen commented Feb 6, 2022

Can this be achieved while doing the linting in pre-commit CI rather in GHA itself?

I don't think so, at least not directly. I wish needs could specify a check, rather than a job, but I don't think it's possible. There are actions out there for waiting on checks, but they're not sufficiently mainstream to consider stable/usable.

My approach to this would be to make a different workflow for tag pushes, and have that workflow read almost the same as your build-release workflow, but without the if check on the ref.
Linting with pre-commit would run in GitHub Actions at release time but not on PRs. We could allow a tox -e lint to run on pushes to the dev branch too, to give us some extra peace of mind.

Another option is to keep the lint job in main CI, and rely on pre-commit.ci only for its autoupdate and autofixing capabilities. That would work totally fine, but it seems a little wasteful to me. The chances of pre-commit.ci passing, but tox -e lint failing, are slim. But it is possible. Our approach depends a bit on how worried we are about that situation.

@sirosen
Copy link
Collaborator Author

sirosen commented Feb 10, 2022

As we now have pre-commit.ci up and running, I'm going to close this. Let's use #689 to discuss moving other builds from Azure to other platforms.

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

No branches or pull requests

3 participants