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

Use pull request target branch for reference build by default

      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.

          [JENKINS-66672] Use pull request target branch for reference build by default

          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.

          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.

          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"

          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"

          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.

          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.

          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 cetlan. So what do you think?

          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 cetlan . So what do you think?

          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.

          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.

          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.

          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.

          cetlan 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.)

          Kalle Niemitalo added a comment - cetlan 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.)

          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".

          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".

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

              Created:
              Updated:
              Resolved: