[GitHub] Support using the `merge` ref instead of the `head` ref #584

mrc0mmand · 2021-05-16T15:09:29Z

Hello!

During our experience with Packit in systemd we've encountered multiple occurrences of a a scenario where a change in a pull request breaks the Packit CI when combined with a commit pushed into the main branch after the PR was created - in other words: had the PR been rebased before running the CI the issue would have been detected.

For this reason, our other CIs use the test/merge ref GitHub provides for each PR (refs/pull/XXX/merge instead of refs/pull/XXX/head) which provides a transient merge commit combining the current base branch with the PR changes .

In the GH API the SHA of the merge commit is provided via the merge_commit_sha field:

The value of the merge_commit_sha attribute changes depending on the state of the pull request. Before merging a pull request, the merge_commit_sha attribute holds the SHA of the test merge commit. After merging a pull request, the merge_commit_sha attribute changes depending on how you merged the pull request

(taken from https://docs.github.com/en/rest/reference/pulls#get-a-pull-request).

The PyGithub counterpart is github.PullRequest.merge_commit_sha (https://pygithub.readthedocs.io/en/latest/github_objects/PullRequest.html#github.PullRequest.PullRequest.merge_commit_sha)

GitLab seems to support the merge_commit_sha field as well (https://docs.gitlab.com/ee/api/merge_requests.html), but I have a very limited experience with GitLab, so I'm not sure about the details.

Would it be possible to implement this in a similar manner as the GithubPullRequest.head_commit property (since it's "just" github.PullRequest.merge_commit_sha instead of github.PullRequest.head.sha), and then make it somehow configurable via Packit?

Thank you!

The text was updated successfully, but these errors were encountered:

mfocko · 2021-05-16T15:27:58Z

Hi!

So if I understand correctly, you propose two following changes:

merge_commit_sha to be added to ogr API for pull/merge requests, and
ability to configure running Packit-as-a-Service on the "merge commit"
- I recall some discussion regarding this previously
- I can see one smallish issue in case of merge conflicts, but I guess that could be definitely resolved by falling back to the head of the PR/MR
- this seems to be related too: Make copr-builds from the merged result not the contributor's branch packit-service#245

It seems to be more relevant for the service, so if you don't have any objections for those 2 points, I will move transfer the issue there. :)

(Or in the light of packit/packit-service#245, maybe just keep this for the ogr part)

mrc0mmand · 2021-05-16T15:46:01Z

So if I understand correctly, you propose two following changes:

Exactly!

I can see one smallish issue in case of merge conflicts, but I guess that could be definitely resolved by falling back to the head of the PR/MR

In this case it might help checking the mergeable field (github.PullRequest.mergeable in PyGithub):

The value of the mergeable attribute can be true, false, or null. If the value is null, then GitHub has started a background job to compute the mergeability. After giving the job time to complete, resubmit the request. When the job finishes, you will see a non-null value for the mergeable attribute in the response. If mergeable is true, then merge_commit_sha will be the SHA of the test merge commit.

But I suspect it might not be the universal solution for all git forges.

this seems to be related too: packit/packit-service#245

Hah, I completely missed this one when I was trying to find if this was already requested, thanks!

It seems to be more relevant for the service, so if you don't have any objections for those 2 points, I will move transfer the issue there. :)

(Or in the light of packit/packit-service#245, maybe just keep this for the ogr part)

Feel free to keep/transfer/closed this one then, I've subscribed to packit/packit-service#245 since it's basically what I was trying to request.

Thanks!

TomasTomecek · 2021-05-17T15:35:19Z

Let's keep this one to track the ask for ogr and then continue the discussion for the whole change in the packit-service's issues :)

I think this is a good suggestion, I'm wondering what happens when the PR cannot be merged to the target branch - this is the reason why I think we should support both workflows: building from contributor's branch and building from a merged result. I'd say both are valid workflows.

mfocko · 2021-05-17T15:54:50Z

I think this is a good suggestion, I'm wondering what happens when the PR cannot be merged to the target branch - this is the reason why I think we should support both workflows: building from contributor's branch and building from a merged result. I'd say both are valid workflows.

At least for GitHub you get true, false, null, true means "merged", false should be conflict, null not finished.
In case of GitLab I just checked on of the opened MRs, it seems they don't do it... The merge commit SHA is populated with the actual merge commit after the MR is merged.

For further discussion:

If it is only supported by GitHub, it would probably make sense to do the merge locally on our side (positive is no need to keep GitHub jobs in queue till the "merge" is finished, negative is additional cost on our side)

They have this though: https://docs.gitlab.com/ee/api/merge_requests.html#merge-to-default-merge-ref-path
Still if we decide to support Pagure, it could be a problem.

TomasTomecek · 2021-05-18T08:23:10Z

* If it is only supported by GitHub, it would probably make sense to do the merge locally on our side (positive is no need to keep GitHub jobs in queue till the "merge" is finished, negative is additional cost on our side)

This should be a fairly cheap operation, right? Unless we are merging 5y old PR.

The main problem I can see from my side is if the merge fails with some obscure error message - though we can check how others are doing this, especially zuul and get inspired.

martinpitt · 2021-05-20T04:42:47Z

@TomasTomecek , @mvollmer: FYI, this is the issue we talked about yesterday. We ran into that in cockpit-project/cockpit#15823 as well -- the packit test failure happens because it takes outdated tests: that PR was posted from a slightly out of date fork, while origin/master got some string changes ("Never lock account" → "Never expire account"). Our code and tests got adjusted, and our create-archive: command correctly runs from the merge -- but packit runs the test directly from the proposed branch, not the merge commit. So the tests are out of date.

IMHO the default should be to test the merge commit, as that's what will actually happen if you merge the PR. At least it should be possible to configure.

I'm wondering what happens when the PR cannot be merged to the target branch

Then the test should fail early. The PR has conflicts and needs to be rebased anyway. It is not acceptable to test something outdated with conflicts and then manually merge it with conflict resolution without testing the merged result. At least CI systems should not encourage it.

martinpitt · 2021-05-20T04:47:16Z

we can check how others are doing this, especially zuul and get inspired.

There should be nothing magic to it -- see e.g. cockpit CI (--rebase is given for all PRs). GitHub workflow's standard actions/checkout also grabs the PR "merge commit" by default, not the fork's HEAD (you can opt into using the latter).

martinpitt · 2021-05-20T04:48:34Z

... in other words, packit's code doesn't need to do anything explicitly. It should just clone the merge branch as the description says, that one is already rebased.

TomasTomecek · 2021-06-04T12:06:15Z

... in other words, packit's code doesn't need to do anything explicitly. It should just clone the merge branch as the description says, that one is already rebased.

I didn't realize it's this simple. It also sounds like it's "the right thing" to do and would also solve some weird behaviour (e.g. a PR is not rebased and contains very outdated packit.yaml or other configs).

Bumping the prio here: @lachmanfrantisek shall we plan this one for the next sprint? Sounds like a trivial thing to implement.

Martin, sorry for such a late response, GitHub's notifications are my bane lately -_-

Add merge_commit_sha and mergeable properties to GithubPullRequest class (and PullRequest superclass). OGR issue packit#584. Signed-off-by: Ben Crocker <[email protected]>

Add merge_commit_sha and merge_commit_status properties to GithubPullRequest class (and PullRequest superclass); merge_commit_status returns a CommitStatus, which is expanded with two new values, mergeable and notmergeable. GitHub's 'mergeable' boolean maps to CommitStatus.mergeable (true) or CommitStatus.notmergeable (false). GitLab's 'merge_status' string values map to CommitStatus.mergeable ("can_be_merged") or CommitStatus.notmergeable (everything else). OGR issue packit#584. Signed-off-by: Ben Crocker <[email protected]>

Add merge_commit_sha and merge_commit_status properties to GithubPullRequest class (and PullRequest superclass); merge_commit_status returns a CommitStatus, which is expanded with two new values, mergeable and notmergeable. GitHub's 'mergeable' boolean maps to CommitStatus.mergeable (true) or CommitStatus.notmergeable (false). GitLab's 'merge_status' string values map to CommitStatus.mergeable ("can_be_merged") or CommitStatus.notmergeable (everything else). New test_pull_requests.py:test_merge_commit_sha methods for GitHub and GitLab. OGR issue packit#584. Signed-off-by: Ben Crocker <[email protected]>

Add merge_commit_sha and merge_commit_status properties to GithubPullRequest class (and PullRequest superclass); merge_commit_status returns a MergeCommitStatus. GitHub's 'mergeable' boolean maps to MergeCommitStatus.can_be_merged (true) or MergeCommitStatus.cannot_be_merged (false). GitLab's 'merge_status' string values map to MergeCommitStatus values verbatim. New test_pull_requests.py:test_merge_commit_sha methods for GitHub and GitLab. OGR issue packit#584. Signed-off-by: Ben Crocker <[email protected]>

Add merge_commit_sha and merge_commit_status properties to GithubPullRequest class (and PullRequest superclass); merge_commit_status returns a MergeCommitStatus. GitHub's 'mergeable' boolean maps to MergeCommitStatus.can_be_merged (true) or MergeCommitStatus.cannot_be_merged (false). GitLab's 'merge_status' string values map to MergeCommitStatus values verbatim. New test_pull_requests.py:test_merge_commit_sha methods for GitHub and GitLab. In Makefile, pass GITHUB_TOKEN, GITLAB_TOKEN in check-in-container OGR issue packit#584. Signed-off-by: Ben Crocker <[email protected]>

Support using the merge ref instead of the head ref in a pull request Add merge_commit_sha and mergeable properties to GithubPullRequest class (and PullRequest superclass). OGR issue #584. Signed-off-by: Ben Crocker [email protected] Reviewed-by: Tomas Tomecek <[email protected]> Reviewed-by: None <None> Reviewed-by: None <None> Reviewed-by: Matej Focko <None>

allisonkarlitskaya · 2021-07-12T10:05:17Z

Yay! Thanks for landing this! ❤️

TomasTomecek · 2021-07-12T10:49:35Z

@allisonkarlitskaya just to be clear, we still need to actually use this functionality in packit, this is only the first part where we implemented the plumbing :) please watch packit/packit-service#245

mrc0mmand changed the title ~~[GitHub] Support using merge ref instead of the head ref~~ [GitHub] Support using the merge ref instead of the head ref May 16, 2021

martinpitt mentioned this issue May 20, 2021

package.json: Update @patternfly/react-table, @patternfly/react-core, @patternfly/patternfly, @patternfly/react-styles cockpit-project/cockpit-podman#723

Merged

bcrocker15 mentioned this issue Jun 13, 2021

Support using the merge ref instead of the head ref in a pull request #601

Merged

bcrocker15 linked a pull request Jun 13, 2021 that will close this issue

Support using the merge ref instead of the head ref in a pull request #601

Merged

allisonkarlitskaya mentioned this issue Jun 27, 2021

jumpstart: use sha256sums file for consistency checking cockpit-project/cockpit#16005

Closed

1 task

softwarefactory-project-zuul bot closed this as completed in #601 Jul 12, 2021

mfocko mentioned this issue Jul 16, 2021

feat(api): Add merge_ref for merge requests python-gitlab/python-gitlab#1549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GitHub] Support using the `merge` ref instead of the `head` ref #584

[GitHub] Support using the `merge` ref instead of the `head` ref #584

mrc0mmand commented May 16, 2021

mfocko commented May 16, 2021

mrc0mmand commented May 16, 2021

TomasTomecek commented May 17, 2021

mfocko commented May 17, 2021 •

edited

Loading

TomasTomecek commented May 18, 2021

martinpitt commented May 20, 2021

martinpitt commented May 20, 2021

martinpitt commented May 20, 2021

TomasTomecek commented Jun 4, 2021 •

edited

Loading

allisonkarlitskaya commented Jul 12, 2021

TomasTomecek commented Jul 12, 2021

[GitHub] Support using the merge ref instead of the head ref #584

[GitHub] Support using the merge ref instead of the head ref #584

Comments

mrc0mmand commented May 16, 2021

mfocko commented May 16, 2021

mrc0mmand commented May 16, 2021

TomasTomecek commented May 17, 2021

mfocko commented May 17, 2021 • edited Loading

TomasTomecek commented May 18, 2021

martinpitt commented May 20, 2021

martinpitt commented May 20, 2021

martinpitt commented May 20, 2021

TomasTomecek commented Jun 4, 2021 • edited Loading

allisonkarlitskaya commented Jul 12, 2021

TomasTomecek commented Jul 12, 2021

[GitHub] Support using the `merge` ref instead of the `head` ref #584

[GitHub] Support using the `merge` ref instead of the `head` ref #584

mfocko commented May 17, 2021 •

edited

Loading

TomasTomecek commented Jun 4, 2021 •

edited

Loading