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

Add CI section (autoupdate) to pre-commit config #95

Merged
merged 4 commits into from
Dec 26, 2023

Conversation

egeakman
Copy link
Contributor

@egeakman egeakman commented Dec 25, 2023

If someone can enable pre-commit.ci, we could get monthly or weekly auto updates. I don't have write access here, so I can't enable it.

See also: #90 (review)


📚 Documentation preview 📚: https://docs-community--95.org.readthedocs.build/en/95/

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone can enable pre-commit.ci

I've requested access, please could @python/organization-owners approve it?

image

@ewdurbin
Copy link
Member

Done

@hugovk hugovk merged commit b21d543 into python:main Dec 26, 2023
2 checks passed
@hugovk
Copy link
Member

hugovk commented Dec 26, 2023

Thanks!

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 26, 2023

Maybe I'm missing something, but I don't really understand why we need to rely on a third-party service and grant it privileged access our repository and organization, when this is trivial to do with a simple cron-triggered GitHub Actions workflow?

@egeakman
Copy link
Contributor Author

egeakman commented Dec 27, 2023

@CAM-Gerlach This just seemed like the easiest solution. Are you suggesting to run pre-commit autoupdate then open a pull request using a GitHub Actions workflow? Or something completely manual (e.g. parse the config file, get the latest release for each repo, update the revisions...)? I am not against using these methods, but pre-commit.ci seems like the easiest to maintain and to deal with, even though we're relying on a third party service.

@CAM-Gerlach
Copy link
Member

I mean a drop-in workflow like this (adapted from this reusable workflow):

on:
  schedule:
    - cron: "0 0 1 */3 *"  # Quarterly
  workflow_dispatch:

jobs:
  auto-update:
    permissions:
      contents: write # To create branch
      pull-requests: write # To create a PR
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-python@v5
        with:
          python-version: "3.x"
      - run: pip install pre-commit
      - run: pre-commit autoupdate
      - run: pre-commit run --all-files
      - uses: peter-evans/create-pull-request@v5
        with:
          branch: pre-commit-autoupdate
          title: Auto-update pre-commit hooks
          commit-message: Auto-update pre-commit hooks

That way it runs on our existing GitHub Actions infra, fully under our control and configured entirely via our repo, rather than requiring relying on and granting sensitive permissions to a third-party service and account thereof.

I could submit a PR adding it in lieu of this external approach, if others agree.

@hugovk
Copy link
Member

hugovk commented Dec 27, 2023

That does scheduled autoupdate, how about fixing PRs?

@CAM-Gerlach
Copy link
Member

The other reasons to avoid pre-commit.ci is that it only supports a hardcoded list pre-commit hooks, only specific types of hooks and also lacks other capabilities it supports vis a vis the pre-commit action and just running pre-commit directly, which the maintainer has no interest in addressing except possibly as a paid enterprise feature at some point in the future; see e.g. pre-commit/action#118 for a listing of some of these. Relying on pre-commit.ci would also lock us in to all of these limitations, which would be incompatible with what we're doing already on a number of other Python-org repos.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 27, 2023

That does scheduled autoupdate, how about fixing PRs?

Hmm, that's true, though I'm not really seeing where that capability is requested or discussed in this PR or the linked issue. However, that can also be achieved via a GitHub Action, e.g. using the approach proposed in python/cpython#104903 , which I've developed and used successfully in other projects I maintain.

In general, I at least don't often find it that necessary, since much of the design goal of pre-commit is to make it as easy, painless and reliable as possible to run hooks locally, either on-demand or automatically with each commit, and many if not most hooks don't support autofix anyway. And ISTM that seems unlikely to actually come into play here, since basically all the changes here are to the RST, and the only autofix-enabled hooks that affect those are just the trivial trailing space/EOL fixers (which most IDEs support automatically).

@hugovk
Copy link
Member

hugovk commented Dec 27, 2023

That way it runs on our existing GitHub Actions infra, fully under our control and configured entirely via our repo, rather than requiring relying on and granting sensitive permissions to a third-party service and account thereof.

The example YAML gives write permissions to a third-party action.

The other reasons to avoid pre-commit.ci is that it only supports a hardcoded list pre-commit hooks, only specific types of hooks and also lacks other capabilities it supports vis a vis the pre-commit action and just running pre-commit directly, which the maintainer has no interest in addressing except possibly as a paid enterprise feature at some point in the future; see e.g. pre-commit/action#118 for a listing of some of these.

Is the hardcoded list available somewhere?

For this repo, we only use three hooks, and don't use mypy, pylint or system hooks, so I think we're fine here.

Relying on pre-commit.ci would also lock us in to all of these limitations, which would be incompatible with what we're doing already on a number of other Python-org repos.

How would it lock ud in? We can still update config manually from the command line, and there are always some lint failures pre-commit might find that need manual fixes.

And that's unlikely to even come into play here, since by far the great majority of changes here are to the RST, and the only autofix-enabled hooks that affect those are just the trivial trailing space/EOL fixers (which most IDEs support automatically).

And yet these are common issues from first time contributors.

Furthermore, I generally don't find it that necessary, since much of the design goal of pre-commit is to make it as easy, painless and reliable as possible to run hooks locally, either on-demand or automatically with each commit.

Personally, I find PR autofixing much more valuable than scheduled autoupdates.

  • Autofix PRs: twice today it's helped or could have helped me in PRs.
  • Autoupdate config: I generally update at the same time making other pre-commit updates, which may be on the scale of months.

And I prefer maintaining less YAML where feasible!

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.

5 participants