Run pre-commit only on the files modified by the PR

[codificat]

Related Issues and Dependencies

This is a redux of #27401

It should help with thoth-station/prescriptions-refresh-job#52

This introduces a breaking change

• No

This should yield a new module release

• No

This Pull Request implements

Modify the pre-commit configuration so that it runs only on the changed files.

Doc on this: https://pre-commit.com/#usage-in-continuous-integration

Description

The previous attempt in #27401 had e.g. "--to-ref HEAD" as a single argument (instead of 2), which caused an error, leading to it being reverted in #27402.

Also taking the opportunity to update the pre-commit image version

[codificat]

/retest
(trying to catch the relevant tide log here that precedes the execution of the pre-commit job)

[codificat]

So, this is failing with:

An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/git', 'diff', '--name-only', '--no-ext-diff', '-z', 'origin/HEAD..HEAD')
return code: 128
expected return code: 0
stdout: (none)
stderr:
    fatal: ambiguous argument 'origin/HEAD..HEAD': unknown revision or path not in the working tree.

Taking a look at what the clonerefs init container does, this was the command sequence:

$ jq -r .command < 128059a6-4415-11ed-a2c6-0a580a820598-clonerefs*
null
null
null
mkdir -p /home/prow/go/src/github.com/thoth-station/prescriptions
git init
git config user.name ci-robot
git config user.email [email protected]
git fetch https://github.com/thoth-station/prescriptions.git --tags --prune
git fetch https://github.com/thoth-station/prescriptions.git master
git checkout eef45cdf91e83d981d2268ba8918a94aaefb304c
git branch --force master eef45cdf91e83d981d2268ba8918a94aaefb304c
git checkout master
git fetch https://github.com/thoth-station/prescriptions.git 9cc6e8b3fa342b40ef140d14cb15780e26765c3c
git merge --no-ff 9cc6e8b3fa342b40ef140d14cb15780e26765c3c
git submodule update --init --recursive

Running this locally on a clone of the repo I get something that works, though:

$ git reflog
e2a5c17c05 (HEAD -> master) HEAD@{0}: merge 9cc6e8b3fa342b40ef140d14cb15780e26765c3c: Merge made by the 'ort' strategy.
eef45cdf91 (origin/master, origin/HEAD) HEAD@{1}: checkout: moving from eef45cdf91e83d981d2268ba8918a94aaefb304c to master
eef45cdf91 (origin/master, origin/HEAD) HEAD@{2}: checkout: moving from master to eef45cdf91e83d981d2268ba8918a94aaefb304c
eef45cdf91 (origin/master, origin/HEAD) HEAD@{3}: clone: from https://github.com/thoth-station/prescriptions.git

$ git diff --name-only --no-ext-diff origin/HEAD..HEAD
.pre-commit-config.yaml
.prow.yaml

Hmmm... not sure why this isn't working as expected

[codificat]

Ah, of course - there is no clone in the prow job, so no origin.

Updating to use HEAD@{1} instead

[codificat]

/retest

[VannTen]

Good catch. My previous PR was indeed flawed since command is not a shell and does not do word splitting ^.

Regarding using the reflog ref, is that stable ? (I think prow exposes from and to branch as env vars, at least it seems to me I read that in the doc somewhere) (EDIT: PULL_BASE_SHA + PULL_PULL_SHA) Using that would not be convenient for a command though.

[codificat]

Regarding using the reflog ref, is that stable ?

I am wondering the same thing. But I think it is, based on the logic that the clonerefs container implements: it checks out the base ref and then merges the PR's ref. I believe this should always lead to HEAD@{1} pointing to the base ref, no?

I started a debug pod from the last pre-commit job and checked the reflog there. Here is how it looks like:

$ oc debug pod/9c55f2c0-447e-11ed-8b46-0a580a820599
Defaulting container name to test.
Use 'oc describe pod/9c55f2c0-447e-11ed-8b46-0a580a820599-debug -n opf-ci-prow' to see all of the containers in this pod.
Starting pod/9c55f2c0-447e-11ed-8b46-0a580a820599-debug, command was: /tools/entrypoint
Pod IP: 10.128.6.220
If you don't see a command prompt, try pressing enter.
(app-root) sh-4.4$ pwd
/home/prow/go/src/github.com/thoth-station/prescriptions
(app-root) sh-4.4$ git reflog
e8d3245e9c (HEAD -> master) HEAD@{0}: merge 6d8f1aede55e364203ba434eafd2eb39d6dfa819: Merge made by the 'recursive' strategy.
eef45cdf91 HEAD@{1}: checkout: moving from eef45cdf91e83d981d2268ba8918a94aaefb304c to master
eef45cdf91 HEAD@{2}: checkout: moving from master to eef45cdf91e83d981d2268ba8918a94aaefb304c

(I think prow exposes from and to branch as env vars, at least it seems to me I read that in the doc somewhere) (EDIT: PULL_BASE_SHA + PULL_PULL_SHA) Using that would not be convenient for a command though.

Right... I don't think we can use that, unfortunately. And of course we can't hardcode the base ref either (i.e. in the example above, we can't use master)

[VannTen]

I am wondering the same thing. But I think it is, based on the logic that the `clonerefs` container implements: it checks out the base ref and then merges the PR's ref. I believe this should always lead to `HEAD@{1}` pointing to the base ref, no?

Yeah indeed it should. /lgtm

[harshad16]

/lgtm
/approve
thanks 👍

[sesheta]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [harshad16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment