[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic #155

abayer · 2017-05-04T17:26:20Z

JENKINS-24141

Downstream of jenkinsci/jenkins#2730 and jenkinsci/workflow-job-plugin#35

cc @reviewbybees

jglick · 2017-05-05T17:13:06Z

Set CI to build with JDK 8.

jglick

AFAICT there is no added test coverage for WorkflowRun.getCulprits, or did I just miss it?

jglick · 2017-05-05T17:14:50Z

src/main/java/hudson/plugins/emailext/plugins/recipients/DevelopersRecipientProvider.java

@@ -39,6 +39,7 @@ public void send(final String format, final Object... args) {
            }
        }
        final Debug debug = new Debug();
+


gratuitous, revert

...dangit. =)

davidvanlaatum

While I am happy with these changes If they require features not in the latest LTS Im hesitant to merge until they are as then any other changes won't be available to LTS users (such as my work)

jglick · 2017-05-06T13:27:35Z

Even assuming the upstream PR does get merged and released in a weekly, there is no expectation this PR would get merged in the near future. Probably you would want to wait until the API makes it into an LTS release, at least.

abayer · 2017-05-08T13:28:59Z

@jglick See https://github.com/jenkinsci/email-ext-plugin/pull/155/files#diff-14528565ae064617b0382642b3e14276 - there was previously special-casing for non-AbstractBuilds, which was tested in CulpritsRecipientProviderTest, so we just have to make sure the tests keep passing rather than write new ones.

jglick

I see.

ghost · 2017-05-18T16:26:54Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev · 2017-05-18T17:10:19Z

pom.xml

+        <workflow-step-api.version>2.9</workflow-step-api.version>
+        <workflow-aggregator.version>2.5</workflow-aggregator.version>
+        <workflow-job.version>2.12</workflow-job.version>
+        <jenkins.version>2.60</jenkins.version>


Needs sign-off from the plugin maintainer

oleg-nenashev

Reflection could help to temporary avoid bumping Jenkins core dependency, but I am not a big fan of that either. There are some unrelated updates && missing direct tests for Pipeline, but it is up to the plugin maintainer (as well as the core bump)

🐝 and @reviewbybees done, formally the PR is correct

jglick · 2017-05-19T13:34:06Z

Reflection could help to temporary avoid bumping Jenkins core dependency

Yes, that is definitely an option here, since it is a single method call so the whole idiom would be only a few lines (incl. a comment reminding the maintainer to switch to a direct call when updating the baseline). Would get this in the hands of (2.60+) users sooner, assuming the maintainer is reluctant to use a new baseline, so it might be a reasonable tradeoff.

abayer · 2017-05-19T15:59:22Z

So the reflection thing gets a bit hairier due to there already being a getChangeSets on AbstractBuild pre-2.60, but it's @Restricted(DoNotUse.class). So basically I'll revert to the previous behavior there.

jglick · 2017-05-19T16:22:26Z

src/main/java/hudson/plugins/emailext/plugins/recipients/CulpritsRecipientProvider.java

-            Set<User> users = ((AbstractBuild<?,?>)run).getCulprits();
-            RecipientProviderUtilities.addUsers(users, context, env, to, cc, bcc, debug);
-        } else {
+        // TODO: core 2.60+, workflow-job 2.12+: Switch to checking if run is RunWithSCM and make catch an else block


…or simply delete the else block.

Yeah, I guess we can probably just rule out any other weirdo job types out there.

Actually, I like what I've got here. It's a nice better-safe-than-sorry fallback.

jglick · 2017-05-19T16:22:40Z

src/main/java/hudson/plugins/emailext/plugins/recipients/RecipientProviderUtilities.java

@@ -37,6 +37,7 @@
 import hudson.scm.ChangeLogSet;
 import hudson.tasks.MailSender;

+import javax.mail.MethodNotSupportedException;


jglick · 2017-05-19T16:23:25Z

src/main/java/hudson/plugins/emailext/plugins/recipients/RecipientProviderUtilities.java

@@ -74,6 +76,7 @@ private RecipientProviderUtilities() {
                    addChangeSetUsers(changeLogSet, users, debug);
                }
            } else {
+                // TODO: core 2.60+, workflow-job 2.12+: Decide whether to remove this logic since it won't be needed for Pipelines any more.


Right, in 2.60+ we can just drop the else clause.

jglick · 2017-05-19T16:23:52Z

...dson/plugins/emailext/plugins/recipients/FirstFailingBuildSuspectsRecipientProviderTest.java

-        PowerMockito.when(build5.getResult()).thenReturn(Result.FAILURE);
+        final FreeStyleBuild build5 = PowerMockito.spy(new FreeStyleBuild(p));
+        PowerMockito.doReturn(Result.FAILURE).when(build5).getResult();
+        PowerMockito.doReturn(build4).when(build5).getPreviousCompletedBuild();


This is one reason I loathe mocking tests—they have to be rewritten even if you make compatible changes.

oleg-nenashev

🐝

oleg-nenashev · 2017-05-19T16:23:00Z

pom.xml

        <jenkins.version>2.0</jenkins.version>
+        <workflow-api.version>2.11</workflow-api.version>
+        <workflow-cps.version>2.29</workflow-cps.version>
+        <workflow-durable-task-step.version>2.11</workflow-durable-task-step.version>


make sure that all deps are compatible with Jenkins 2.0after the dep downgrade

They are - none of the core Pipeline plugins have dependencies on core >=2.0 until workflow-job 2.12.

oleg-nenashev · 2017-05-19T16:24:48Z

src/main/java/hudson/plugins/emailext/plugins/recipients/CulpritsRecipientProvider.java

+                }
+            }
+        } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
+            debug.send("Exception getting culprits for %s: %s", run, e);


🐜 this is a valid behavior for 2.60-, no? Maybe it needs more graceful message, but up to you

Good point.

jglick · 2017-06-16T18:48:44Z

Test failures which look real.

[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic

6cce465

abayer added the work-in-progress label May 4, 2017

abayer requested a review from jglick May 4, 2017 17:26

This was referenced May 4, 2017

[JENKINS-24141] Pull ChangeLogSet-related logic out of AbstractBuild jenkinsci/jenkins#2730

Merged

[JENKINS-24141] Implement RunWithSCMMixIn.RunWithSCM jenkinsci/workflow-job-plugin#35

Merged

Update dependencies and tests, fix FindBugs.

2e38d1d

jglick closed this May 5, 2017

jglick reopened this May 5, 2017

jglick approved these changes May 5, 2017

View reviewed changes

davidvanlaatum self-assigned this May 6, 2017

davidvanlaatum self-requested a review May 6, 2017 03:05

davidvanlaatum reviewed May 6, 2017

View reviewed changes

Revert pesky newline, bump mailer version to fix InjectedTest

57a94b4

jglick approved these changes May 8, 2017

View reviewed changes

Use Jenkins 2.60 and workflow-job 2.12.

cd6c1f6

abayer removed the work-in-progress label May 18, 2017

oleg-nenashev reviewed May 18, 2017

View reviewed changes

jglick approved these changes May 19, 2017

View reviewed changes

oleg-nenashev approved these changes May 19, 2017

View reviewed changes

Switch to reflection to keep a lower core.

da47c13

jglick approved these changes May 19, 2017

View reviewed changes

oleg-nenashev approved these changes May 19, 2017

View reviewed changes

abayer added 2 commits May 20, 2017 11:34

Minor code review responses

1ca8b49

Merge remote-tracking branch 'origin/master' into jenkins-24141

facb011

Explicit workflow-support dependency.

cd7d569

Fix broken tests. Sigh, mocking.

b5c0d76

abayer merged commit dfb6bbb into jenkinsci:master Oct 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic #155

[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic #155

abayer commented May 4, 2017

jglick commented May 5, 2017

jglick left a comment

jglick May 5, 2017

abayer May 8, 2017

davidvanlaatum left a comment

jglick commented May 6, 2017

abayer commented May 8, 2017

jglick left a comment

ghost commented May 18, 2017

oleg-nenashev May 18, 2017

oleg-nenashev left a comment

jglick commented May 19, 2017

abayer commented May 19, 2017

jglick May 19, 2017

abayer May 20, 2017

abayer May 20, 2017

jglick May 19, 2017

abayer May 20, 2017

jglick May 19, 2017

jglick May 19, 2017

abayer May 20, 2017

oleg-nenashev left a comment

oleg-nenashev May 19, 2017

abayer May 20, 2017

oleg-nenashev May 19, 2017

abayer May 20, 2017

jglick commented Jun 16, 2017

[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic #155

[JENKINS-24141] Switch to using RunWithSCM for getCulprits logic #155

Conversation

abayer commented May 4, 2017

jglick commented May 5, 2017

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidvanlaatum left a comment

Choose a reason for hiding this comment

jglick commented May 6, 2017

abayer commented May 8, 2017

jglick left a comment

Choose a reason for hiding this comment

ghost commented May 18, 2017

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick commented May 19, 2017

abayer commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented Jun 16, 2017