[PULP-1105] Add pre-commit in github templates #1001

pedro-psb · 2026-01-26T23:38:21Z

As proposed here: https://discourse.pulpproject.org/t/proposal-using-pre-commit-for-static-checks/2170/5

This also adds pre-commit to the plugin_template repository itself.

Further improvements are:

Automate having plugin's and plugin-template's tool versions in sync
- Example: the plugin lint requirement is synced with pre-commit because it's templated, but the plugin-template's lint_requirements isn't. Maybe use an scheduled update PR and use templating in plugin-template itself (or just a sed-like script)?
Automate bumping pre-commit hook versions in sync.
- Example: a schduled workflow that uses pre-commit autoupdate and update plugin-template script

mdellweg · 2026-01-29T08:02:51Z

scripts/update_ci.sh

 then
-  pip install -r lint_requirements.txt
-  black .
+  pre-commit run -a black || true  # always zero exit-code


In what conditions does this exit non-zero?

If it modifies files, which means linting failed. But here we don't care, we want auto-format.

Can it still communicate "Failed to reformat"?

Kind of. I see this as an unexpected error:

pre-commit exits with specific codes:

1: a detected / expected error 3: an unexpected error 130: the process was interrupted by ^C

https://pre-commit.com/#command-line-interface

mdellweg · 2026-01-29T08:04:44Z

templates/github/.ci/scripts/check_gettext.sh.j2

 cd "$(dirname "$(realpath -e "$0")")"/../..

-set -uv
+set -u


Can this be "set -eu"?

Actually this makes it harder to have the output we want of, because it will error immediately after failure (e.g, if we invert grep exit code with $(! grep ...)).

ERROR: Detected mix of f-strings and gettext: {RESULT}

There might be one, but I don't know a clever way of achieve this.

I believe this should work:

EXIT_CODE=0 RESULT=$(grep -n "$PATTERN" "$@") || EXIT_CODE=$?

Also I believe this script is lightweight enough to run on sh instead of bash.

That raised an unbound variable error on the error case.
I found other option still using -e.

mdellweg · 2026-01-29T08:07:47Z

plugin-template

-            subprocess.run(["black", "--quiet", "."], cwd=plugin_root_dir)
+            subprocess.run(["pre-commit", "run", "-a", "black"], cwd=plugin_root_dir)


We should probably check the exit code here too.

The intent looks the same as the update_ci part: auto-format if it can, and ignore either way.
Hence the supress error handling if the binary is not found.
Actually, this looks redundant. We'll run auto-format when we apply the template and on the CI update.
Should I drop it here (or on upadate_ci)?

(I dropped it when running plugin-template. If you're running it manually you can apply the template and then run pre-commit to format on a different commit)

mdellweg · 2026-01-29T08:12:24Z

templates/github/.pre-commit-config.yaml.j2

+  - repo: 'https://github.com/pre-commit/pre-commit-hooks'
+    rev: {{ precommit_versions["pre-commit-hooks"] | quote }}


Does this mean these are installed from git? I'd rather have them installed from pypi.

Yes. I believe the reason is that it's supposed to be language agnostic.
If you are concerned about mutability we could use the commit sha.

I guess not. sha values are unmaintainable.
I just had a feeling that this is odd, now you gave me the argument I was lacking.
Thinking more about it, I don't like the fact that precommit needs to "own" the installation in the first place.

What is the benefit over precommit "just" calling make format or make lint?

It does some things, like handling file finding/filtering (non-trivial e.g, recognize shebang scripts), parallelization and owning the installation (python interpreter, packages, caching).

And in some cases, skip checking if there are no changes.

mdellweg · 2026-01-29T08:15:21Z

templates/github/.ci/scripts/check_gettext.sh.j2

+else
+  # original behavior (check all)
+  set -v
+  RESULT=$(grep -n -r --include \*.py "$PATTERN")
+  EXIT_CODE=$?
+fi


Do we still need this then?

I kept it for compatibility, similar to lint_requirements.
Although it's less likely that is actually used manually. Should I drop it?

If that is how we are going to use it, then yes don't keep dead codepaths around.

Added a pre-commit config using the same linting steps as the lint CI job. Also added a layer to keep pre-commit and lint_requirements in sync. Slightly changed some lint scripts to adpat to pre-commit.

mdellweg · 2026-01-30T14:01:55Z

templates/github/.ci/scripts/check_gettext.sh.j2

+fi

-if [ $? -ne 1 ]; then
+# grep returns 1 if it doesn't find a match


"Failure is success" ;)

pedro-psb · 2026-01-30T14:37:42Z

Ok, no more pushes until a new review.

github-actions bot added multi-commit no-changelog no-issue labels Jan 26, 2026

pedro-psb force-pushed the add-pre-commit branch 5 times, most recently from 01fd5eb to 08b2c10 Compare January 28, 2026 14:43

pedro-psb marked this pull request as ready for review January 28, 2026 14:52

pedro-psb mentioned this pull request Jan 28, 2026

[test] Apply plugin-template pulp/pulpcore#7267

Draft

4 tasks

mdellweg reviewed Jan 29, 2026

View reviewed changes

Add pre-commit to plugins and plugin_template itself

80d408e

Added a pre-commit config using the same linting steps as the lint CI job. Also added a layer to keep pre-commit and lint_requirements in sync. Slightly changed some lint scripts to adpat to pre-commit.

pedro-psb force-pushed the add-pre-commit branch from 9dd44f8 to 80d408e Compare January 30, 2026 13:27

github-actions bot removed the multi-commit label Jan 30, 2026

mdellweg reviewed Jan 30, 2026

View reviewed changes

github-actions bot added the multi-commit label Jan 30, 2026

fixup: keep order from lint workflow and re-write pulpcore import check

d278902

pedro-psb force-pushed the add-pre-commit branch from f094a7b to d278902 Compare January 30, 2026 14:34

pedro-psb requested a review from mdellweg January 30, 2026 15:59

pedro-psb mentioned this pull request Jan 30, 2026

Experiment with pre-commit pulp/pulpcore#7181

Closed

		subprocess.run(["black", "--quiet", "."], cwd=plugin_root_dir)
		subprocess.run(["pre-commit", "run", "-a", "black"], cwd=plugin_root_dir)

		- repo: 'https://github.com/pre-commit/pre-commit-hooks'
		rev: {{ precommit_versions["pre-commit-hooks"] \| quote }}

[PULP-1105] Add pre-commit in github templates #1001

Are you sure you want to change the base?

[PULP-1105] Add pre-commit in github templates #1001

Conversation

pedro-psb commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedro-psb commented Jan 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pedro-psb commented Jan 26, 2026 •

edited

Loading