Change default PR build strategy from merge to head

[jglick]

When multibranch was first written, using PR merge build strategy seemed like a neat way to ensure that trunk was never broken. But this strategy comes with downsides:

• It can be expensive: as the number of open PRs grow, the time spent testing them can grow quadratically rather than linearly: every time one is merged, all the remaining ones are rebuilt.
• It violates the principle of least surprise: gh pr checkout 123 && make can produce different results than CI.
• This plugin as well as github-checks submit a commit status or check but then need to account for the fact that the built commit might be an ephemeral merge commit and the submission actually needs to use the first parent—though it might not, even in merge strategy, if the PR head is already up to date.
  • You can then have a PR which is initially green but then goes red without being touched, or vice-versa; this is in some sense the point of the strategy to begin with, but GitHub retains no historical record of why a status/check was overwritten for a given PR branch commit. (You can create a permalink to a particular check run, even if outdated—but after it is superseded for that commit, you cannot readily look it up if you did not remember to save the permalink before.) The same issue of course applies to using Replay (Jenkins) or Re-run (Checks tab) but at least that scenario involves a deliberate gesture.

In practice, it is not very common that two pull requests are filed which individually pass CI, fail CI in combination, and yet have no textual merge conflicts (edits within ~3 lines of one another). It can certainly happen (jenkinsci/script-security-plugin#468 is an example) but not very often, and it is even less common that the failure is so weird that is difficult to fix trunk retroactively.

Furthermore, since this feature was conceived, GitHub itself has evolved to include branch protections such as the rule that a PR must be up to date before merging (which Dependabot will automatically honor by rebasing); as well as the introduction of a merge queue feature that supersedes this Jenkins trick.

This patch simply changes the default for multibranch projects or organization folders newly created via the GUI, to the simpler and more comprehensible head mode: build whatever was last pushed to the PR branch. Example result in config.xml:

<traits>
  <org.jenkinsci.plugins.github__branch__source.BranchDiscoveryTrait>
    <strategyId>1</strategyId>
  </org.jenkinsci.plugins.github__branch__source.BranchDiscoveryTrait>
  <org.jenkinsci.plugins.github__branch__source.OriginPullRequestDiscoveryTrait>
    <strategyId>2</strategyId>
  </org.jenkinsci.plugins.github__branch__source.OriginPullRequestDiscoveryTrait>
  <org.jenkinsci.plugins.github__branch__source.ForkPullRequestDiscoveryTrait>
    <strategyId>2</strategyId>
    <trust class="org.jenkinsci.plugins.github_branch_source.ForkPullRequestDiscoveryTrait$TrustPermission"/>
  </org.jenkinsci.plugins.github__branch__source.ForkPullRequestDiscoveryTrait>
</traits>

For what I suppose are historical reasons, strategyId is an int rather than a more legible enum:

github-branch-source-plugin/src/main/java/org/jenkinsci/plugins/github_branch_source/OriginPullRequestDiscoveryTrait.java

Lines 57 to 67 in 3a76035

	/** None strategy. */
	public static final int NONE = 0;
	/** Merging the pull request with the current target branch revision. */
	public static final int MERGE = 1;
	/** The current pull request revision. */
	public static final int HEAD = 2;
	/**
	* Both the current pull request revision and the pull request merged with the current target
	* branch revision.
	*/
	public static final int HEAD_AND_MERGE = 3;

[car-roll]

LGTM. Does it change the behavior of existing projects? I just see it being called for newInstance, but I know this is all nested heavily with other repos.

[jglick]

Does it change the behavior of existing projects?

Should not; these are just the defaults for new projects.

[raul-arabaolaza]

I have suffered the issues @jglick mentions and I totally agree with the arguments