dask / community

For general discussion and community planning. Discussion issues welcome.
20 stars 3 forks source link

Piloting pre-commit.ci - a service for use with repo's with a `.pre-commit-config.yaml` #222

Open consideRatio opened 2 years ago

consideRatio commented 2 years ago

In https://github.com/dask/dask-gateway/issues/476, we have now started piloting the use of https://pre-commit.ci - a service that helps open source projects having a .pre-commit-config.yaml file.

Why pilot pre-commit.ci?

I wanted to champion it because I wanted the quality of life improvements it gives me as a maintainer. I've grown very fond of them by using the service in various JupyterHub projects. While I initially felt uneasy about relying on another service, it has been great:

What pre-commit.ci does

  1. Verifies pre-commit is happy based on the config - a test
  2. Regularly updates the .pre-commit-config.yaml configured hooks with more modern versions, once a week.
  3. Automatically pushes a commit to PRs that have a failing pre-commit check with autoformatting changes made by pre-commit.

To pilot pre-commit.ci use in another project

To pilot the use in another project as well, what you should do is to enable the pre-commit.ci service for that repo via https://github.com/organizations/dask/settings/installations and optionally remove previous configuration to run pre-commit in GitHub workflows etc that will no longer be needed.

I could recommend documenting the .pre-commit-config.yaml file quite well, helping anyone trying to understand what that is about by inline comments. For that you could also look to https://github.com/dask/dask-gateway/blob/main/.pre-commit-config.yaml for inspiration. PS: I really appreciate having inline comments on top of misc files in repos to help maintainers understand what they are about etc, this is an example of that practice.

jacobtomlinson commented 2 years ago

Currently, we use the pre-commit GitHub Action to run pre-commit checks in CI.

I too was initially sceptical of setting up another external CI service, but keeping the config up to date and automatically pushing commits to PRs to fix linting issues sounds valuable enough to make the transition. So piloting it on dask-gateway feels like a good approach to try things out.

I do wonder how much effort it would be to achieve the same thing via GitHub Actions rather than introducing another external service. Keeping the config up to date could be done in the same way we update the Docker image with a cron based action. And we could get the "push commits to PRs" functionality by adding an extra step to our current action. Increasing the complexity of our existing Actions would mean we wouldn't need another third party CI, but I'm not sure which trade off is better.

consideRatio commented 2 years ago

I do wonder how much effort it would be to achieve the same thing via GitHub Actions rather than introducing another external service.

I think the proper solution would be to develop a GitHub App that is enabled for the repo's, in practice, doing exactly what pre-commit.ci does. One could also do it by defining a workflow in each repo that referenced a centrally located github action - that would require some code duplication in each repo though besides re-implementing the logic itself.

Currently, we use the pre-commit GitHub Action to run pre-commit checks in CI:

I recall the reason we piloted this initially in JupyterHub was because the pre-commit action that we also used, had been deprecated.

DEPRECATED this action is in maintenance-only mode and will not be accepting new features.

Please switch to using pre-commit.ci which is faster and has more features.

consideRatio commented 2 years ago

I just ran into a situation where I appreciated pre-commit.ci again! Even though I have experience and knowledge about the need to apply autoformatting, the pre-commit.ci's help to automatically add a commit helped me out as well.

I just applied some suggestions to a PR via GitHub's UI, but, they broke the formatting... So, pre-commit.ci simply added a commit fixing them again - I didn't need to go back and fix it locally etc and push back a commit after applying the GitHub UI code suggestions.

image

jsignell commented 2 years ago

Considering what we are using is deprecated, I am definitely open to trying pre-commit.ci.

consideRatio commented 2 years ago

The first automated PR to update a pre-commit config file: https://github.com/dask/dask-gateway/pull/481

Note how pre-commit.ci also pushed a commit - this was unrelated to pre-commit being the author of the PR. It just observed a need to apply some autoformatting that hadn't been applied based on the PRs pre-commit-config, which in this case had just bumped the autoformatter black to a new version that made a slight change.

consideRatio commented 2 years ago

I've suggested going for pre-commit.ci as a service also in https://github.com/dask/helm-chart/pull/244 - another project i invest time in