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

Allow merge commits to be built from origin PRs

    XMLWordPrintable

    Details

    • Similar Issues:

      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

            recena Manuel Recena Soto created issue -
            recena Manuel Recena Soto made changes -
            Field Original Value New Value
            Description There should be an option by which the normal suppression of branch projects for origin PRs would be disabled, so you would get two projects:

            # one building the head of the branch
            # 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.
            There should be an option by which the normal suppression of branch projects for origin PRs would be disabled, so you would get two projects:

            # one building the head of the branch
            # 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.
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-33237 [ JENKINS-33237 ]
            hrmpw Patrick Wolf made changes -
            Labels 2.0
            jglick Jesse Glick made changes -
            Labels 2.0 2.0-planned
            jglick Jesse Glick made changes -
            Labels 2.0-planned 2.0
            Hide
            hrmpw Patrick Wolf added a comment -

            This should be as consistent as possible across all branch source plugins that work with PRs. Unless we want to track all of them separately.

            Show
            hrmpw Patrick Wolf added a comment - This should be as consistent as possible across all branch source plugins that work with PRs. Unless we want to track all of them separately.
            Hide
            michaelneale Michael Neale added a comment -

            If in the happy case of the branch being update with the master when a pull request is opened, ideally a new build would not be triggered, instead it could just use the build previously done on the branch (but it could show up as a new build?? perhaps??).

            Show
            michaelneale Michael Neale added a comment - If in the happy case of the branch being update with the master when a pull request is opened, ideally a new build would not be triggered, instead it could just use the build previously done on the branch (but it could show up as a new build?? perhaps??).
            jamesdumay James Dumay made changes -
            Labels 2.0 2.0 blueocean
            michaelneale Michael Neale made changes -
            Priority Minor [ 4 ] Major [ 3 ]
            Hide
            michaelneale Michael Neale added a comment -

            I think this is becoming more important to bring it to parity with other PR builders people are used to

            Show
            michaelneale Michael Neale added a comment - I think this is becoming more important to bring it to parity with other PR builders people are used to
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-34120 [ JENKINS-34120 ]
            jglick Jesse Glick made changes -
            Link This issue is duplicated by JENKINS-33530 [ JENKINS-33530 ]
            recena Manuel Recena Soto made changes -
            Description There should be an option by which the normal suppression of branch projects for origin PRs would be disabled, so you would get two projects:

            # one building the head of the branch
            # 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.
            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:

            # one building the head of the branch
            # 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.
            recena Manuel Recena Soto made changes -
            Assignee Jesse Glick [ jglick ] Manuel Jesús Recena Soto [ recena ]
            Hide
            recena Manuel Recena Soto added a comment -

            Michael Neale Sure, something is going forward to

            Show
            recena Manuel Recena Soto added a comment - Michael Neale Sure, something is going forward to
            recena Manuel Recena Soto made changes -
            Link This issue duplicates JENKINS-33739 [ JENKINS-33739 ]
            amuniz Antonio Muñiz made changes -
            Link This issue duplicates JENKINS-33739 [ JENKINS-33739 ]
            Hide
            nickbrown Nicholas Brown added a comment -

            It would be great to see this also added to the bitbucket-branch-source plugin.

            Show
            nickbrown Nicholas Brown added a comment - It would be great to see this also added to the bitbucket-branch-source plugin.
            Hide
            hrmpw Patrick Wolf added a comment -

            Yes, Nicholas Brown. We will make sure to include this in BB as well. Our goal is to have as much parity between GH and BB as possible. My first comment was to that effect. Antonio Muñiz

            Show
            hrmpw Patrick Wolf added a comment - Yes, Nicholas Brown . We will make sure to include this in BB as well. Our goal is to have as much parity between GH and BB as possible. My first comment was to that effect. Antonio Muñiz
            Hide
            mikedougherty Mike Dougherty added a comment - - edited

            Are there any plans to allow users to opt out of building PR merge refs? I don't want to be doing builds for revisions that will never exist. This is a serious problem for me, to the point that I'm about to make a custom build with +refs/pull/*/head instead of +refs/pull/*/merge in GitHubSCMSource.java.

            Show
            mikedougherty Mike Dougherty added a comment - - edited Are there any plans to allow users to opt out of building PR merge refs? I don't want to be doing builds for revisions that will never exist. This is a serious problem for me, to the point that I'm about to make a custom build with +refs/pull/*/head instead of +refs/pull/*/merge in GitHubSCMSource.java.
            jglick Jesse Glick made changes -
            Epic Link JENKINS-35386 [ 171179 ]
            recena Manuel Recena Soto made changes -
            Assignee Manuel Recena Soto [ recena ] Jesse Glick [ jglick ]
            Hide
            hamiltb Bill Hamilton added a comment -

            The original description of this seems to mix the issues at hand. One issue is that origin PRs should be allowed, via an option. The second issue, completely unrelated, is that any PR (from remote or origin) should expose options to build either or both of HEAD and MERGE.

            Show
            hamiltb Bill Hamilton added a comment - The original description of this seems to mix the issues at hand. One issue is that origin PRs should be allowed, via an option. The second issue, completely unrelated, is that any PR (from remote or origin) should expose options to build either or both of HEAD and MERGE.
            Hide
            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).

            Show
            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).
            Hide
            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.

            Show
            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 .
            Hide
            mikedougherty Mike Dougherty added a comment -

            Jesse Glick 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.

            Show
            mikedougherty Mike Dougherty added a comment - Jesse Glick 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.
            Hide
            michaelneale Michael Neale added a comment -

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

            Show
            michaelneale Michael Neale added a comment - Very thorough writeup. I think the default of building merge for PRs is consistent with forked pull requests.
            Hide
            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.

            Show
            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 made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            nickbrown Nicholas Brown made changes -
            Link This issue is related to JENKINS-34931 [ JENKINS-34931 ]
            jglick Jesse Glick made changes -
            Remote Link This issue links to "PR 60 (Web Link)" [ 14516 ]
            jamesdumay James Dumay made changes -
            Link This issue blocks JENKINS-35843 [ JENKINS-35843 ]
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-35991 [ JENKINS-35991 ]
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-36283 [ JENKINS-36283 ]
            jglick Jesse Glick made changes -
            Link This issue blocks JENKINS-34728 [ JENKINS-34728 ]
            jglick Jesse Glick made changes -
            Resolution Fixed [ 1 ]
            Status In Progress [ 3 ] Resolved [ 5 ]
            jglick Jesse Glick made changes -
            Link This issue is blocked by JENKINS-36574 [ JENKINS-36574 ]
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 169034 ] JNJira + In-Review [ 198517 ]

              People

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

                Dates

                Created:
                Updated:
                Resolved: