Upload git patch when pre-commit fails

[RazvanLiviuVarzaru]

When pre-commit modify files but is not verbose about the changes, one case use git apply pre-commit.patch (download the patch from GitHub Actions).

[RazvanLiviuVarzaru]

@fauust

This commit is just for demonstration, I will remove it soon.
d4a9217

The patch is at:
Artifact download URL: https://github.com/MariaDB/buildbot/actions/runs/11957626874/artifacts/2219913716

with contents:

diff --git a/utils.py b/utils.py
index d7719c2..b2e7834 100644
--- a/utils.py
+++ b/utils.py
@@ -4,6 +4,7 @@ import re
 import sys
 from datetime import datetime, timedelta
 
+import docker
 from pyzabbix import ZabbixAPI
 from twisted.internet import defer
 
@@ -27,7 +28,6 @@ from constants import (
     releaseBranches,
     savedPackageBranches,
 )
-import docker
 
 private_config = {"private": {}}
 exec(open("/srv/buildbot/master/master-private.cfg").read(), private_config, {})

[cvicentiu]

@fauust after discussing with @RazvanLiviuVarzaru I suggest the following approach:

1. The pre-commit git hook should not modify the developer's commit, rather it should just warn of problems, but not even prevent "a commit from happening" either.
2. Răzvan has noticed that sometimes different isort or different black versioning can have different suggestions on how to fix things. So we should:
3. Modify the pre-commit-config.yml to invoke isort and black with --diff and --check flags. This will allow us to catch such 'isort' and 'black' failures during CI, with proper logging.

Let me know if this is clear enough.

[RazvanLiviuVarzaru]

@fauust after discussing with @RazvanLiviuVarzaru I suggest the following approach:

1. The pre-commit git hook should not modify the developer's commit, rather it should just warn of problems, but not even prevent "a commit from happening" either.
2. Răzvan has noticed that sometimes different isort or different black versioning can have different suggestions on how to fix things. So we should:
3. Modify the pre-commit-config.yml to invoke isort and black with --diff and --check flags. This will allow us to catch such 'isort' and 'black' failures during CI, with proper logging.

Let me know if this is clear enough.

Applied @cvicentiu idea on 7014f98

You can see in the log that isort let the files intact and wrote the diff's to the output:
https://github.com/MariaDB/buildbot/actions/runs/11959895369/job/33342704197

I prefer this over letting the hooks modify the files and then upload the patch.

[fauust]

@fauust after discussing with @RazvanLiviuVarzaru I suggest the following approach:

1. The pre-commit git hook should not modify the developer's commit, rather it should just warn of problems, but not even prevent "a commit from happening" either.

Yes, definitively, but please keep in mind that regular contributor should use pre-commit locally, see https://github.com/MariaDB/buildbot?tab=readme-ov-file#pre-commit. So, I am not sure what you mean by:

but not even prevent "a commit from happening" either.

It should locally prevent a commit from being done, this way contributor would not have to wait for the CI to be failing to do a correct commit.

2. Răzvan has noticed that sometimes different isort or different black versioning can have different suggestions on how to fix things. So we should:

3. Modify the pre-commit-config.yml to invoke `isort` and `black` with `--diff` and `--check` flags. This will allow us to catch such 'isort' and 'black' failures during CI, with proper logging.

Let me know if this is clear enough.

Yep appart from 1/ and also @RazvanLiviuVarzaru the trailing white space check is doing modification too, so we might want to put it in check mode only.

[RazvanLiviuVarzaru]

@fauust after discussing with @RazvanLiviuVarzaru I suggest the following approach:

1. The pre-commit git hook should not modify the developer's commit, rather it should just warn of problems, but not even prevent "a commit from happening" either.

Yes, definitively, but please keep in mind that regular contributor should use pre-commit locally, see https://github.com/MariaDB/buildbot?tab=readme-ov-file#pre-commit. So, I am not sure what you mean by:

but not even prevent "a commit from happening" either.

It should locally prevent a commit from being done, this way contributor would not have to wait for the CI to be failing to do a correct commit.

2. Răzvan has noticed that sometimes different isort or different black versioning can have different suggestions on how to fix things. So we should:

3. Modify the pre-commit-config.yml to invoke `isort` and `black` with `--diff` and `--check` flags. This will allow us to catch such 'isort' and 'black' failures during CI, with proper logging.

Let me know if this is clear enough.

Yep appart from 1/ and also @RazvanLiviuVarzaru the trailing white space check is doing modification too, so we might want to put it in check mode only.

@fauust , @cvicentiu
It seems not possible for whitespace hook.
See an old unresolved issue: pre-commit/pre-commit-hooks#108
I can't find any useful configurable parameters for that hook to prevent it to modify the files. See: https://github.com/pre-commit/pre-commit-hooks/blob/main/pre_commit_hooks/trailing_whitespace_fixer.py

Do you want to keep a hybrid approach? Meaning, let isort/black only check and for whitespace upload a patch.

[fauust]

Yep appart from 1/ and also @RazvanLiviuVarzaru the trailing white space check is doing modification too, so we might want to put it in check mode only.

@fauust , @cvicentiu It seems not possible for whitespace hook. See an old unresolved issue: pre-commit/pre-commit-hooks#108 I can't find any useful configurable parameters for that hook to prevent it to modify the files. See: https://github.com/pre-commit/pre-commit-hooks/blob/main/pre_commit_hooks/trailing_whitespace_fixer.py

pre-commit/pre-commit-hooks#108 makes sense to me. I have the feeling that we are trying to implement something that nobody will use (well at least I personally don't need it since I have pre-commit installed locally).

Do you want to keep a hybrid approach? Meaning, let isort/black only check and for whitespace upload a patch.

Let's not waste time on the trailing spaces. If this PR helps you working with the project, I am ok with it. What you could do is to document this so that new contributors know how to use patches from GH actions.

[cvicentiu]

@fauust after discussing with @RazvanLiviuVarzaru I suggest the following approach:

1. The pre-commit git hook should not modify the developer's commit, rather it should just warn of problems, but not even prevent "a commit from happening" either.

Yes, definitively, but please keep in mind that regular contributor should use pre-commit locally, see https://github.com/MariaDB/buildbot?tab=readme-ov-file#pre-commit. So, I am not sure what you mean by:

but not even prevent "a commit from happening" either.

It should locally prevent a commit from being done, this way contributor would not have to wait for the CI to be failing to do a correct commit.

In typical development this is not desirable. There are many cases when one wants to do WIP commits, without having to worry about isort and black validity.

[fauust]

It should locally prevent a commit from being done, this way contributor would not have to wait for the CI to be failing to do a correct commit.

In typical development this is not desirable. There are many cases when one wants to do WIP commits, without having to worry about isort and black validity.

In that case, that's of course possible locally https://pre-commit.com/#temporarily-disabling-hooks but pushing something knowing that the CI will be red is a bit strange IMO (appart from backup purpose of course!).