Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-33161

Allow merge commits to be built from origin PRs

    XMLWordPrintable

Details

    Description

      There should be an option by which the normal suppression of Multibranch projects for origin PRs would be disabled, so you would get two projects:

      1. one building the head of the branch
      2. one building the speculative merge commit.

      There could also be an option to suppress the origin branch project when a PR is filed from the branch. This would cause the origin branch project to be deleted once the PR is filed. (It could be resurrected if the PR is closed but the branch not deleted.) This option would necessitate a new API in github-branch-source to filter branches.

      Attachments

        Issue Links

          Activity

            jglick Jesse Glick added a comment -

            Well, they are related in that all reasonable options about what to build and based on what reference ought to be offered.

            Tricky because the GitHub API documentation is frustratingly vague and incomplete. When getting a pull request, there is an undocumented .base.sha which seems to be the common ancestor between the PR branch and the base branch—very different from pull/*/merge and merge_commit_sha, which both seem to represent a virtual merge of the PR head onto the current base branch head (or, rather, a time-delayed snapshot thereof, and as noted in JENKINS-33237 not a very reliable one).

            jglick Jesse Glick added a comment - Well, they are related in that all reasonable options about what to build and based on what reference ought to be offered. Tricky because the GitHub API documentation is frustratingly vague and incomplete. When getting a pull request , there is an undocumented .base.sha which seems to be the common ancestor between the PR branch and the base branch—very different from pull/*/merge and merge_commit_sha , which both seem to represent a virtual merge of the PR head onto the current base branch head (or, rather, a time-delayed snapshot thereof, and as noted in JENKINS-33237 not a very reliable one).
            jglick Jesse Glick added a comment -

            Rough outline of changes:

            • UntrustedPullRequestSCMRevision needs to be generalized to a PullRequestSCMRevision which always records a baseHash, plus a trusted field, so that all PullRequestSCMHead will use this kind of revision. Implies a readResolve for settings compatibility.
            • GitHubBranchSource (and, by extension, GitHubSCMNavigator) needs to be able to be configured as to whether it will create jobs for:
              • an origin branch with no corresponding PR
              • an origin branch with a corresponding PR (but built as a branch: named by branch name, not sending status notifications, not offering to merge with a base branch)
              • a PR from an origin branch
              • a PR from a fork
            • For the latter two, we additionally need to be able to decide whether to build:
              • The PR head. Means the trusted revision, if applicable, is the .base.sha, and no merge is required.
              • The merge into the current base branch. Means base branch updates need to trigger fresh PR builds (SCMRevision.equals should be false when the base branch has moved forward). The trusted revision is the current base branch head. merge_commit_sha is deprecated and might be deleted in the future; pull/*/merge is buggy; so the plugin rather needs to explicitly look up and record the current base branch head, and perform an explicit merge (probably via decorateRevisionToBuild/PreBuildMerge, or less obviously via getCandidateRevisions).
            • getRefSpecs need not refer to PR refs, since build would anyway be overridden for PRs.
            • The two overloads of doRetrieve need to produce consistent SCMRevision implementations for PRs. Currently we return the PR head during branch indexing, but the virtual merge commit later, which violates the API contract and intuition.

            TBD whether it is necessary to allow a PR to be built both as head and as merge. I think I have heard this request. In such a case, two jobs need to be created for it with different names, perhaps PR-123 vs. PR-123-merge.

            jglick Jesse Glick added a comment - Rough outline of changes: UntrustedPullRequestSCMRevision needs to be generalized to a PullRequestSCMRevision which always records a baseHash , plus a trusted field, so that all PullRequestSCMHead will use this kind of revision. Implies a readResolve for settings compatibility. GitHubBranchSource (and, by extension, GitHubSCMNavigator ) needs to be able to be configured as to whether it will create jobs for: an origin branch with no corresponding PR an origin branch with a corresponding PR (but built as a branch: named by branch name, not sending status notifications, not offering to merge with a base branch) a PR from an origin branch a PR from a fork For the latter two, we additionally need to be able to decide whether to build: The PR head. Means the trusted revision, if applicable, is the .base.sha , and no merge is required. The merge into the current base branch. Means base branch updates need to trigger fresh PR builds ( SCMRevision.equals should be false when the base branch has moved forward). The trusted revision is the current base branch head. merge_commit_sha is deprecated and might be deleted in the future; pull/*/merge is buggy; so the plugin rather needs to explicitly look up and record the current base branch head, and perform an explicit merge (probably via decorateRevisionToBuild / PreBuildMerge , or less obviously via getCandidateRevisions ). getRefSpecs need not refer to PR refs, since build would anyway be overridden for PRs. The two overloads of doRetrieve need to produce consistent SCMRevision implementations for PRs. Currently we return the PR head during branch indexing, but the virtual merge commit later, which violates the API contract and intuition. TBD whether it is necessary to allow a PR to be built both as head and as merge. I think I have heard this request. In such a case, two jobs need to be created for it with different names, perhaps PR-123 vs. PR-123-merge .

            jglick thanks for the writeup. I'd just like to confirm the request that it should be possible to build the PR head or merge – that is, I know some have a preference for the merge (current behavior) but it should also be possible to do the head instead. I don't know that a user would want to build both refs, but definitely being able to choose one or the other is valuable.

            I'd also like to say that it's still valuable to provide github status notifications on any ref, not just pull requests. The information is accessible through the API and very useful. On a related note, being able to control the status context is also useful.

            mikedougherty Mike Dougherty added a comment - jglick thanks for the writeup. I'd just like to confirm the request that it should be possible to build the PR head or merge – that is, I know some have a preference for the merge (current behavior) but it should also be possible to do the head instead. I don't know that a user would want to build both refs, but definitely being able to choose one or the other is valuable. I'd also like to say that it's still valuable to provide github status notifications on any ref, not just pull requests. The information is accessible through the API and very useful. On a related note, being able to control the status context is also useful.
            michaelneale Michael Neale added a comment -

            Very thorough writeup.
            I think the default of building merge for PRs is consistent with forked pull requests.

            michaelneale Michael Neale added a comment - Very thorough writeup. I think the default of building merge for PRs is consistent with forked pull requests.
            jglick Jesse Glick added a comment -

            confirm the request that it should be possible to build the PR head or merge

            Yes, I would consider that a requirement, and making it possible to build both does not seem like any extra work. What the best way to present all these options in the UI would be, I am not sure, but I will leave that to someone with a good eye for it.

            valuable to provide github status notifications on any ref, not just pull requests

            True, currently most notifications do get sent on all refs. GitHub will only display status on commits in the PR UI, but there is no harm in always sending it.

            being able to control the status context is also useful

            There are certainly use cases for displaying customized statuses but it seems off topic here.

            jglick Jesse Glick added a comment - confirm the request that it should be possible to build the PR head or merge Yes, I would consider that a requirement, and making it possible to build both does not seem like any extra work. What the best way to present all these options in the UI would be, I am not sure, but I will leave that to someone with a good eye for it. valuable to provide github status notifications on any ref, not just pull requests True, currently most notifications do get sent on all refs. GitHub will only display status on commits in the PR UI, but there is no harm in always sending it. being able to control the status context is also useful There are certainly use cases for displaying customized statuses but it seems off topic here.

            People

              jglick Jesse Glick
              recena Manuel Recena Soto
              Votes:
              12 Vote for this issue
              Watchers:
              26 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: