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

Use pull request target branch for reference build by default

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      When Jenkins is building pull request (either the source branch or the result of merging), the discoverReferenceBuild and discoverGitReferenceBuild steps could choose a reference build from the job corresponding to the target branch of the pull request, by default. This way, the pipeline would not need to pass in defaultBranch: env.CHANGE_TARGET explicitly.

      The precedence could be:

      1. referenceJob: argument
      2. target branch of the pull request, from ChangeRequestSCMHead.getTarget()
      3. defaultBranch: argument
      4. PrimaryInstanceMetadataAction, if JENKINS-66689 is implemented
      5. hardcoded DEFAULT_BRANCH = "master"

      This could be implemented by making ReferenceRecorder in forensics-api-plugin call ChangeRequestSCMHead.getTarget() if available, and use the result as the reference branch, instead of the default branch (which can be set with ReferenceRecorder.setDefaultBranch or defaults to "master"). SimpleReferenceRecorder.setReferenceJob would still bypass the multibranch support altogether, and use exactly the job that you specify.

        Attachments

          Issue Links

            Activity

            Hide
            kon Kalle Niemitalo added a comment -

            I am not sure which of these should take precedence:

            If the pipeline computes defaultBranch with a complex expression but the step then ignores it for pull requests, I think the developer will be surprised. However, the name of the parameter has “default” in it, which implies that it should not take effect if there is more specific information available.

            Show
            kon Kalle Niemitalo added a comment - I am not sure which of these should take precedence: target branch of the pull request, from ChangeRequestSCMHead.getTarget() defaultBranch: argument If the pipeline computes defaultBranch with a complex expression but the step then ignores it for pull requests, I think the developer will be surprised. However, the name of the parameter has “default” in it, which implies that it should not take effect if there is more specific information available.
            Hide
            drulli Ulli Hafner added a comment - - edited

            If the pipeline computes defaultBranch with a complex expression but the step then ignores it for pull requests, I think the developer will be surprised.

            I changed the name now to targetBranch. I'm not sure which way is better but I think the user preference should be more important?

            So we would get:

            1. referenceJob: argument
            2. targetBranch: argument
            3. target branch of the pull request, from ChangeRequestSCMHead.getTarget()
            4. PrimaryInstanceMetadataAction
            5. hardcoded DEFAULT_BRANCH = "master"
            Show
            drulli Ulli Hafner added a comment - - edited If the pipeline computes defaultBranch with a complex expression but the step then ignores it for pull requests, I think the developer will be surprised. I changed the name now to targetBranch . I'm not sure which way is better but I think the user preference should be more important? So we would get: referenceJob: argument targetBranch: argument target branch of the pull request, from ChangeRequestSCMHead.getTarget() PrimaryInstanceMetadataAction hardcoded DEFAULT_BRANCH = "master"
            Hide
            kon Kalle Niemitalo added a comment -

            Did you implement the user preference precedence in PR #333 yet? It doesn't seem so; in ReferenceRecorder, private String defaultBranch still defaults to "master", so it cannot tell the difference between targetBranch: "master" and omitting the parameter.

            Show
            kon Kalle Niemitalo added a comment - Did you implement the user preference precedence in PR #333 yet? It doesn't seem so; in ReferenceRecorder, private String defaultBranch still defaults to "master", so it cannot tell the difference between targetBranch: "master" and omitting the parameter.
            Hide
            drulli Ulli Hafner added a comment -

            No I did not. I implemented

            1. referenceJob: argument
            2. target branch of the pull request, from ChangeRequestSCMHead.getTarget()
            3. PrimaryInstanceMetadataAction
            4. targetBranch: argument
            5. hardcoded DEFAULT_BRANCH = "master"

            But then I read your comment and thought it made be better to change that to:

            1. referenceJob: argument
            2. targetBranch: argument
            3. target branch of the pull request, from ChangeRequestSCMHead.getTarget()
            4. PrimaryInstanceMetadataAction
            5. hardcoded DEFAULT_BRANCH = "master"

            Before implementing this order I wanted to have some comments from you and Christopher. So what do you think?

            Show
            drulli Ulli Hafner added a comment - No I did not. I implemented referenceJob: argument target branch of the pull request, from ChangeRequestSCMHead.getTarget() PrimaryInstanceMetadataAction targetBranch: argument hardcoded DEFAULT_BRANCH = "master" But then I read your comment and thought it made be better to change that to: referenceJob: argument targetBranch: argument target branch of the pull request, from ChangeRequestSCMHead.getTarget() PrimaryInstanceMetadataAction hardcoded DEFAULT_BRANCH = "master" Before implementing this order I wanted to have some comments from you and Christopher . So what do you think?
            Hide
            cetlan Christopher added a comment -

            I think you're right about the second order. I'm still a bit fuzzy on some of the corner cases, having mostly worked with the multibranch pipelines (and I know there's still a lot I haven't tried with them). But I can imagine some oddball case where someone could need to override the "default" target branch, and omitting it is easy (assuming one of a null or an empty string is valid of course - otherwise it's a bit tricky in a declarative pipeline). I suppose in such a case, they could also just use a manually set referenceJob, but branch name is easier.

            I'm not sure what the PrimaryInstanceMetadataAction is, so I can't comment on whether target branch argument makes sense before or after it to me.

            Show
            cetlan Christopher added a comment - I think you're right about the second order. I'm still a bit fuzzy on some of the corner cases, having mostly worked with the multibranch pipelines (and I know there's still a lot I haven't tried with them). But I can imagine some oddball case where someone could need to override the "default" target branch, and omitting it is easy (assuming one of a null or an empty string is valid of course - otherwise it's a bit tricky in a declarative pipeline). I suppose in such a case, they could also just use a manually set referenceJob, but branch name is easier. I'm not sure what the PrimaryInstanceMetadataAction is, so I can't comment on whether target branch argument makes sense before or after it to me.
            Hide
            kon Kalle Niemitalo added a comment -
            1. referenceJob: argument
            2. targetBranch: argument
            3. target branch of the pull request, from ChangeRequestSCMHead.getTarget()
            4. PrimaryInstanceMetadataAction
            5. hardcoded DEFAULT_BRANCH = "master"

            That makes sense to me.

            Show
            kon Kalle Niemitalo added a comment - referenceJob: argument targetBranch: argument target branch of the pull request, from ChangeRequestSCMHead.getTarget() PrimaryInstanceMetadataAction hardcoded DEFAULT_BRANCH = "master" That makes sense to me.
            Hide
            kon Kalle Niemitalo added a comment -

            Christopher please see PrimaryInstanceMetadataAction javadoc. Here, the plugin would search the multibranch project for an SCMHead marked with PrimaryInstanceMetadataAction. The result would be the same regardless of which branch of the multibranch project is being built, so it makes sense to allow overriding this with targetBranch:, which can vary by branch. (For example, the pipeline can guess that a branch named “bugfix/2.6/hive” is intended to be merged to the “release/2.6” branch, and pass that in as targetBranch even without needing a pull request.)

            Show
            kon Kalle Niemitalo added a comment - Christopher please see PrimaryInstanceMetadataAction javadoc . Here, the plugin would search the multibranch project for an SCMHead marked with PrimaryInstanceMetadataAction. The result would be the same regardless of which branch of the multibranch project is being built, so it makes sense to allow overriding this with targetBranch: , which can vary by branch. (For example, the pipeline can guess that a branch named “bugfix/2.6/hive” is intended to be merged to the “release/2.6” branch, and pass that in as targetBranch even without needing a pull request.)
            Hide
            cetlan Christopher added a comment -

            Oh, I see. That's how it's identifying the "primary" branch (such as the protected trunk in a github repository). Thanks! Then yes, that definitely needs to be possible to override trivially for the case of merging into other branches. And then if the primary branch isn't available from the SCM, fall back to assuming "master", because we can't know anything better. Makes sense for that to be the last fallback before finally giving up and saying "master it is".

            Show
            cetlan Christopher added a comment - Oh, I see. That's how it's identifying the "primary" branch (such as the protected trunk in a github repository). Thanks! Then yes, that definitely needs to be possible to override trivially for the case of merging into other branches. And then if the primary branch isn't available from the SCM, fall back to assuming "master", because we can't know anything better. Makes sense for that to be the last fallback before finally giving up and saying "master it is".

              People

              Assignee:
              drulli Ulli Hafner
              Reporter:
              kon Kalle Niemitalo
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: