fedora-ci / general

#24 Applying the PR patch is weird, maybe bogus
Closed by churchyard. Opened by churchyard.

Closed
Reopen Issue

I've noticed that the way PR package is built is handled by applying the pagure PR patch file and building the srpm. I consider the patch part a bit problematic, as it can result in strange behaviors. This is one of them:

https://jenkins-continuous-infra.apps.ci.centos.org/blue/organizations/jenkins/fedora-rawhide-pr-pipeline/detail/fedora-rawhide-pr-pipeline/714/pipeline/

Your branch is up-to-date with 'origin/master'.
+ curl -L https://src.fedoraproject.org/rpms/python3/pull-request/84.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100   957  100   957    0     0    957      0  0:00:01 --:--:--  0:00:01  2278
+ git apply pr_84.patch
error: patch failed: python3.spec:14
error: python3.spec: patch does not apply
script returned exit code 1

What probably happened here is that the PR got merged before this and the patch didn't apply because it was already applied. (Side note: printing out the HEAD commit hash would be helpful for debugging such problems.)

I think we should use git checkout / merge for this instead. First a decision needs to be made:

Are we testing the head of a PR?

In that case, don't apply patch for master, but apply it on the base of the PR when made. In fact, just fetch the PR and checkout to it:

$ git fetch origin refs/pull/84/head
$ git checkout FETCH_HEAD

Or...

Are we testing what happens once the PR is merged?

In that case, don't apply patch but merge the branch:

$ git fetch origin refs/pull/84/head
$ git merge FETCH_HEAD

Or in fact:

$ git pull origin refs/pull/84/head

This can generate a merge conflict, but that would happen with git apply pr_84.patch anyway.

git pull origin refs/pull/84/head

While nice, the head is created by an async task leading to potential race-condition between the CI system and pagure depending on the load on the pagure instance.

One way around may be to make the CI system sleep and retry a few times if the ref is undefined, but then what about the case where the PR is updated (so the ref exists but hasn't been updated yet).

Fetch the commit hash from API?

I also had a problem with patch application: I had a series of commits which created a file and renamed it afterwards (I would squash before merging, trust me; this was really just a test).

When the patch was applied, the file I renamed was present in the checkout and CI used it which resulted into borked test run.

+1 on using checkout instead of applying patches

The problem with the approach of checkout is that you are basically not testing what you would merge if that would go through. Afaik that is why we used patch application to the latest head of the target branch ...

@bgoncalv do you remember some details or some reference please?

@mvadkert I don't remember the details now, but you are right.
We were fetching the PR and then checkout to it, but we were asked to change it to apply to master.

Well, you can check out the PR and rebase it against master.

Just merge. Either it fails (and patch would fail as well) or it merges cleanly. Just applying a patch file leads to errors as described above.

@churchyard thanks, we can try that. @bgoncalv maybe you can experiment on the pipeline

and @mkluson can try to fix it on our generic pagure building module :)

proposed fix: https://github.com/CentOS-PaaS-SIG/ci-pipeline/pull/782/files

This issue should been fixed by PR above.

It seems that this valid concern was ignored:

the head is created by an async task leading to potential race-condition between the CI system and pagure depending on the load on the pagure instance.

Please, let's not forget this problem.

I would say let's open for that a new issue maybe, as it seems to me quite a corner case, we should not forget about it, but I do not think it has very high priority ...

@churchyard is that so or has this higher impact?

Only time will tell. My concern is that the CI is an async job triggered by opening the PR. And so is the head update. The CI will usually take longer to set up before it comes to fetching the head, however this sounds very fragile. Rebellions are built on hope, not CIs.

BTW I'm OK if another issue is opened. Shall I do it?

@churchyard that would help! thanks

https://pagure.io/fedora-ci/general/issue/35

Metadata Update from @churchyard:
- Issue status updated to: Closed (was: Open)
Metadata
None
None
None
None
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.