[JENKINS-33161] Customized PR build behavior #60

jglick · 2016-06-16T20:48:17Z

JENKINS-33161 and probably also fixes JENKINS-33237 by not relying on more exotic details of the GitHub API. Supersedes #36.

basic structure of selection of build types
fix representation of PR SCMRevisions
run an explicit merge when requested
basic UI
help files
full interactive testing (pending Test case for scanning a GitHub repo #55 this will be pretty laborious)
organization folder support
review indexing log appearance

@reviewbybees

jglick · 2016-06-16T20:49:01Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubBuildStatusNotification.java

+        } else if (revision instanceof PullRequestSCMRevision) {
+            return ((PullRequestSCMRevision) revision).getPullHash();
+        } else {
+            throw new IllegalArgumentException("did not recognize " + revision);


By keeping a reasonable representation of the SCMRevisions to begin with, we avoid the need to try to reconstruct what we just built.

jglick · 2016-06-16T21:03:09Z

src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMHead.java

    private static final long serialVersionUID = 1;

    private final PullRequestAction metadata;
+    private Boolean merge;
+    private final boolean trusted;


Defaulting to false for old settings to be on the safe side. Only practical impact should be that PRs filed before the upgrade will start using Jenkinsfile from the base branch if you upgrade and then push more commits, even when the PR should have been trusted. I think this is an acceptable price to pay for fixing up the metadata to store this field where it makes more sense, so that we do not need to recheck trusted status on every commit.

…ash.

…ion." This reverts commit 6ce16b1. FindBugs is not happy with my simplification!

jglick · 2016-06-16T22:02:46Z

Combinations to be tested include:

organization vs. account
branch vs. origin PR vs. fork PR
merging vs. not vs. both
trusted vs. not
PR up to date w/ base branch vs. needs merge vs. has merge conflict

…and unmerged job results.

amuniz · 2016-06-21T10:09:04Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

+                            continue; // not doing this combination
+                        }
+                        if (buildForkPRUnmerged) {
+                            branchName += "-merged"; // make sure they are distinct


The contrary seems more natural (as most users will want the merged commit to be built). PR-23 for merged, and PR-23-head for unmerged, for example.

I have no strong opinion about naming (in particular whether -head is more or less intuitive than -unmerged), but felt that having one job mention the merge flag and the other not mention it would be confusing.

Don't invent custom termins like ghprb did, just name as it is in API, head merge

head vs. merge would be OK too. I am not particularly moved by the argument of consistency with the terminology used in the API because (AFAICT) there is no mention in user documentation of virtual merge commits or the like.

“Head branch” vs. “base branch” are used (briefly), so users could probably guess that merge refers to a merge of the two, whereas head does not.

amuniz · 2016-06-21T10:40:09Z

Log messages seems misleading when using merged and unmerged PRs:

Checking pull request #3
    ‘Jenkinsfile’ does not exist in this pull request
Does not meet criteria
    ‘Jenkinsfile’ exists in this pull request
Met criteria

This is for this PR: amuniz/maven-helloworld#3

I understand that the first log is for the unmerged state and the second one for the merged state. But Adding some additional information would be nice.

LGTM so far.

jglick · 2016-06-21T13:49:10Z

I understand that the first log is for the unmerged state and the second one for the merged state. But Adding some additional information would be nice.

Yeah the indexing log needs some fine-tuning after this change.

I take it you tried running this? And did not find any serious bugs so far? Unfortunately it is time-consuming to try different combinations by hand. For example, IIRC prior to this PR, if a PR were unmergeable Jenkins would actually build a merge commit—between the branch head and the last base branch commit it merged cleanly with! Which certainly smells like a bug, albeit arguably in the GitHub API. But without an automated test suite (like #55) these kinds of corner cases are likely to regress and stay broken for months.

amuniz · 2016-06-21T14:55:21Z

I take it you tried running this?

Yes, I did.

And did not find any serious bugs so far?

Nop, it worked fine, however I didn't test all corner cases, only the ones that came to my mind in that moment.

…d as any.

…ean; null is only in the field type for the benefit of readResolve.

KostyaSha · 2016-06-21T21:35:07Z

@jglick how you got such number imports?

chinshr · 2016-06-21T21:41:40Z

Looks promising /cc @rguevara84

tfennelly · 2016-06-23T11:59:36Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

+    /** Whether to build PRs filed from a fork, where the build is of the merge with the base branch. */
+    private @Nonnull Boolean buildForkPRMerge = DescriptorImpl.defaultBuildForkPRMerge;
+    /** Whether to build PRs filed from a fork, where the build is of the branch head. */
+    private @Nonnull Boolean buildForkPRHead = DescriptorImpl.defaultBuildForkPRHead;


NIT ... I'm after seeing these build* flags together in a few places in the PR. Seems like they are all closely related and could be encapsulated into a type of some sort.

See the TODO comment in GitHubSCMNavigator.

tfennelly · 2016-06-23T12:43:10Z

🐝 insofar as I get it

Made a few minor comments.

erik-am · 2016-06-24T13:05:46Z

Nice! Do you have any estimate when this will be in the stable release?
We've just upgraded to Jenkins 2 and wanted to build pipelines, but then found out that we can't build origin Pull Requests.
Good to hear that that functionality is on its way! 😄

jglick · 2016-06-24T13:49:24Z

Do you have any estimate when this will be in the stable release?

Well technically nothing is blocking me from releasing it in the next hour, though @recena expressed interest in doing some manual testing and I suspect he has no time to do it today because of other obligations.

@tfennelly saw your comments, some would be good topics for cleanups, though I am hesitant to do nonessential code changes now since it would require another round of sanity checks.

recena · 2016-06-24T16:00:15Z

@jglick Is it urgent to merge this PR?

shauvik · 2016-06-24T16:19:52Z

@recena This PR is very important. Especially because pipeline is not compatible with the ghprb plugin. This will allow people to build PRs with pipelines enabled without essentially building all the branches all the time, which is a waste of resources (for most non-trivial repos).

@jglick Thanks for all the awesome work!

recena · 2016-06-24T16:33:39Z

@shauvik I know what the PR means 😄 I'm co-maintainer of this plugin.

KostyaSha · 2016-06-24T16:37:08Z

Especially because pipeline is not compatible with the ghprb plugin.

JFYI https://wiki.jenkins-ci.org/display/JENKINS/GitHub+Integration+Plugin compatible long time ago even when there no branch-source-plugin.

shauvik · 2016-06-24T20:08:48Z

@recena Sorry, I didn't mean to offend you. Just that I'm very excited for this PR to make it in :-)

recena · 2016-06-25T09:02:56Z

@shauvik Don't worry. What I meant is this change is important and needs to be tested and documented.

kloarubeek · 2016-06-30T10:01:55Z

Great work! @recena did you have time to test? We also want to build origin Pull Requests so it would be great if this moves to the stable release 😎

jglick · 2016-07-05T18:37:30Z

This seems to also fix JENKINS-34728.

kevin-brown · 2016-07-20T14:48:46Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubBuildStatusNotification.java

        LOGGER.log(Level.FINE, "{0}/commit/{1} {2} from {3}", new Object[] {repo.getHtmlUrl(), revision, state, url});
-        repo.createCommitStatus(revision, state, url, message, "Jenkins");
+        repo.createCommitStatus(revision, state, url, message, "Jenkins job " + job.getName());


This causes issues right now as it's not possible to require Jenkins status checks to pass anymore without GitHub. It relies on the name being consistent across checks, which isn't possible when the check name is always changing.

I see this issue has already been filed as JENKINS-36574.

eamonnfaherty · 2016-12-06T12:06:11Z

Can this be released? It looks like the work has been completed.

* Log warnings for caught GitLabApiExceptions * Switch class loggers from org.slf4j to java.util.logging * Fix more exception logging

[JENKINS-33161] Draft of support for customized PR build behavior.

996cd93

jglick added the work-in-progress label Jun 16, 2016

jglick reviewed Jun 16, 2016
View reviewed changes

jglick added 2 commits June 16, 2016 16:52

Oops, forgot to record entries in originBranchesWithPR.

ce5f941

Warning about another anomalous combination.

63a6006

jglick reviewed Jun 16, 2016
View reviewed changes

jglick added 5 commits June 16, 2016 17:13

Fixing a few warnings and a test failure.

ed48654

We are converting this anyway, can strip out more implementation.

6ce16b1

Comments.

870125b

We need to be sure to feed SpecificRevisionBuildChooser the PR head h…

32a5de3

…ash.

Revert "We are converting this anyway, can strip out more implementat…

7dc37d0

…ion." This reverts commit 6ce16b1. FindBugs is not happy with my simplification!

jglick added 2 commits June 16, 2016 18:04

typo

56f47c5

Noting when job names will distinguish kinds of PRs.

413f0b0

jglick mentioned this pull request Jun 20, 2016

The original GitException from a failed merge was good enough, no need to rethrow it jenkinsci/git-client-plugin#208

Merged

jglick added 3 commits June 20, 2016 16:59

Specify a notification per job, so that we can distinctly see merged …

5a98d2a

…and unmerged job results.

Override toString for debugging.

cdfe0b3

Building merged PRs seems to work now.

7353a02

amuniz reviewed Jun 21, 2016
View reviewed changes

jglick added 6 commits June 21, 2016 13:42

@KostyaSha’s suggestion of *-head vs. *-merge job naming seems as goo…

bff827f

…d as any.

The getters and setters were intended to be of type boolean, not Bool…

d067c0c

…ean; null is only in the field type for the benefit of readResolve.

Did my IDE just go nuts?

e003af9

Indentation fix.

c1feec5

Typo.

e889802

Improved form validation at @amuniz’s suggestion.

f7e5157

tfennelly reviewed Jun 23, 2016
View reviewed changes

amuniz mentioned this pull request Jun 28, 2016

Option to skip pull requests during branch & pullrequests discovery jenkinsci/bitbucket-branch-source-plugin#9

Closed

jglick mentioned this pull request Jul 5, 2016

Added logging on when and why PR events are delivered #61

Merged

jglick merged commit c976a4c into jenkinsci:master Jul 5, 2016

jglick deleted the PR-repn-JENKINS-33161 branch July 5, 2016 19:17

jglick mentioned this pull request Jul 5, 2016

Create a ChangeRequestAction so the CHANGE_ID is filled when building jenkinsci/bitbucket-branch-source-plugin#13

Closed

kevin-brown reviewed Jul 20, 2016
View reviewed changes

Eernie mentioned this pull request Nov 22, 2016

[JENKINS-36283] custom pr behavior jenkinsci/bitbucket-branch-source-plugin#25

Closed

6 tasks

jglick mentioned this pull request Mar 1, 2019

[JENKINS-43194] Use merge_commit_sha for master #209

Merged

dubrsl pushed a commit to dubrsl/github-branch-source-plugin that referenced this pull request Oct 17, 2020

Fix changes in jenkinsci#60

9bf37ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-33161] Customized PR build behavior #60

[JENKINS-33161] Customized PR build behavior #60

jglick commented Jun 16, 2016 •

edited

Loading

jglick Jun 16, 2016

jglick Jun 16, 2016

jglick commented Jun 16, 2016

amuniz Jun 21, 2016

jglick Jun 21, 2016

KostyaSha Jun 21, 2016 •

edited

Loading

jglick Jun 21, 2016

amuniz commented Jun 21, 2016

jglick commented Jun 21, 2016

amuniz commented Jun 21, 2016 •

edited

Loading

KostyaSha commented Jun 21, 2016

chinshr commented Jun 21, 2016

tfennelly Jun 23, 2016

jglick Jun 24, 2016

tfennelly commented Jun 23, 2016

erik-am commented Jun 24, 2016

jglick commented Jun 24, 2016

recena commented Jun 24, 2016

shauvik commented Jun 24, 2016

recena commented Jun 24, 2016

KostyaSha commented Jun 24, 2016

shauvik commented Jun 24, 2016

recena commented Jun 25, 2016

kloarubeek commented Jun 30, 2016

jglick commented Jul 5, 2016

kevin-brown Jul 20, 2016

kevin-brown Jul 20, 2016

eamonnfaherty commented Dec 6, 2016

[JENKINS-33161] Customized PR build behavior #60

[JENKINS-33161] Customized PR build behavior #60

Conversation

jglick commented Jun 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jun 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KostyaSha Jun 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amuniz commented Jun 21, 2016

jglick commented Jun 21, 2016

amuniz commented Jun 21, 2016 • edited Loading

KostyaSha commented Jun 21, 2016

chinshr commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfennelly commented Jun 23, 2016

erik-am commented Jun 24, 2016

jglick commented Jun 24, 2016

recena commented Jun 24, 2016

shauvik commented Jun 24, 2016

recena commented Jun 24, 2016

KostyaSha commented Jun 24, 2016

shauvik commented Jun 24, 2016

recena commented Jun 25, 2016

kloarubeek commented Jun 30, 2016

jglick commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eamonnfaherty commented Dec 6, 2016

jglick commented Jun 16, 2016 •

edited

Loading

KostyaSha Jun 21, 2016 •

edited

Loading

amuniz commented Jun 21, 2016 •

edited

Loading