Revisit using pre-commit.ci

dstansby commented 1 month ago

If I'm makng a simple change to markdown, such as in https://github.com/UCL-ARC/python-tooling/pull/462, I use the GitHub web interface for speed. This can often result in pre-commit failing however, because the markdown isn't correctly linted. This then breaks the benefit of using the GitHub website for speed, because I have no way of automatically fixing the linting and have to go back to local development on my laptop.

To improve this I'd like to propose we revisit using pre-commit.ci to lint this repo. This would provide a mechanism of automatically making pre-commit fixes on demand, by posting a "pre-commit.ci autofix" comment in a PR.

In https://github.com/UCL-ARC/python-tooling/pull/196 we very briefly discussed and decided not to use it because it's not available for private repositories. One mitigation to this is we can still have a GitHub actions linting job, so when someone sets up a private repo from this template there's still a linting CI job that is run.

So I'd like to advocate for using pre-commit.ci for this repo. Thoughts?

samcunliffe commented 1 month ago

In https://github.com/UCL-ARC/python-tooling/pull/196 we very briefly discussed and decided not to use it because it's not available for private repositories. One mitigation to this is we can still have a GitHub actions linting job, so when someone sets up a private repo from this template there's still a linting CI job that is run.

Briefly from memory the decision to turn it off was:

We didn't want to have different tools in the main tooling repo than those in the template.
pre-commit.ci is not free for closed-source: we want to encourage maximum takeup of the template amongst UCL researchers, and a good portion of projects either start out closed until some research outcome is published, or stay closed-source.

I'm actually OK with violating 1. We could have pre-commit.ci on the repo, but not the template.

If I read you correctly, you're suggesting that we make the template a bit smarter and have it choose whether to lint based on a cookiecutter option?

paddyroddy commented 1 month ago

I'm happy to have it on the repo, but not the template

dstansby commented 1 month ago

I'm definitely only proposing for the repo here, not the template.

I'm not suggesting we alter the template - just that we add pre-commit.ci to this repo. In order to make sure we keep eating our own dog food, my suggestion is we could keep the GitHub actions linting running on this repo too. But that would duplicate linting CI on this repo 🤔 I'm not sure which way round is best...

paddyroddy commented 1 month ago

Installed and tested here https://github.com/UCL-ARC/python-tooling/pull/464#issuecomment-2429008801

paddyroddy commented 3 weeks ago

Sorry I'm turning this off. It is impossible to turn off autoupdates https://github.com/pre-commit-ci/issues/issues/83. See the issues/concerns: #479, #482

dstansby commented 3 weeks ago

@paddyroddy could you post the issue you've found on this thread? I think they're spread around several PRs/issues at the moment, and I'm struggling to find them all 😄

samcunliffe commented 3 weeks ago

Why not taking alphas?

paddyroddy commented 3 weeks ago

Sorry I'm turning this off. It is impossible to turn off autoupdates pre-commit-ci/issues#83. See the issues/concerns: #479, #482

It's here @dstansby

paddyroddy commented 3 weeks ago

Also doesn't update the template

dstansby commented 3 weeks ago

I can follow links, but I'm asking if you could repeat the problems here so it's easier for us to see what they are all in one place 🙏

samcunliffe commented 3 weeks ago

Sorry I'm turning this off.

Hmm. I'm also a bit wary of adding and removing bots and making decisions so unilaterally.

🗳 I think we should add it, keep it on, and accept that autoupdates will come from both bots. I'm fine with alpha versions. Presumably, renovate will update the template at a slower cycle. Which is less of an issue if we are using versioning and latest.

paddyroddy commented 3 weeks ago

Hmm. I'm also a bit wary of adding and removing bots and making decisions so unilaterally.

I can understand that. But the bot did more than was advertised in #463. IMO behaviour that we don't want. The maintainer is a 🍑 and has not added the ability to turn off autoupdates.

🗳 I think we should add it, keep it on, and accept that autoupdates will come from both bots. I'm fine with alpha versions. Presumably, renovate will update the template at a slower cycle. Which is less of an issue if we are using versioning and latest.

I disagree. Reasons:

Both the repo and template currently update at the same frequency
We have complete control over which updates we want: major, minor, patch etc.
We can control the frequency (I'm aware you can do this with the pre-commit bot, but ours currently aligns with GitHub Actions)
Reduce dependency on a bot which is duplicating behaviour

If we could turn off autoupdate then I'm not opposed to having it. But it's really not hard to run pre-commit run -a on the repo.

paddyroddy commented 3 weeks ago

I can follow links, but I'm asking if you could repeat the problems here so it's easier for us to see what they are all in one place 🙏

In this thread https://github.com/pre-commit-ci/issues/issues/83 there are the maintainer's reasons for not wanting the ability to turn off autoupdates.

samcunliffe commented 3 weeks ago

I disagree. Reasons:

I see.

Both the repo and template currently update at the same frequency

I'm actually not so opposed to having the repo and template out of sync. What's the worse that could happen?

Alternatives:

Would the lite action work instead?
Could we keep full-bodied bot and just have an action to automatically decline update PRs from pre-commit.ci?

paddyroddy commented 3 weeks ago

Not seen lite before. I think that's better than the other.

The worst I guess would be:

Someone checks out repo (ignoring latest tag) with pre-commit out of sync
They look into pre-commit and see the ones they have downloaded aren't the same as the one on the repo
They update their versions to match the repo ones (potentially running the pre-commit autoupdate command they learnt about in their search)
Their linting now fails and they don't know why

dstansby commented 3 weeks ago

I've added a new rule at https://github.com/UCL-ARC/python-tooling/settings/rules/2461978 which (I think...) bans branches called pre-commit-ci-update-config. I'm hoping this will prevent the pre-commit bot from opening auto update PRs?

@paddyroddy did you disable the bot? If so, could you re-enable it, and we can see if this works? If it does it seems like a good solution.

samcunliffe commented 3 weeks ago

@dstansby

I think we should:

Re-enable the pre-commit.ci bot today (now).
(Then) merge @paddyroddy's pre-commit autoupdate revert pull request (so that the pre-commit-config is back to being outdated...)
Hold off on merging the @renovate PRs until Monday.

.... If your branch rule has worked, then there should be no autoupdate PR from pre-commit.ci despite that one is needed. On Tuesday we can merge @renovate's PRs.

paddyroddy commented 3 weeks ago

@samcunliffe I didn't do PRs in your desired order, as didn't see this. Can install bad bot now.

paddyroddy commented 3 weeks ago

Will close this. We can reopen if issues arise.

UCL-ARC / python-tooling

Revisit using pre-commit.ci #463