Adding support for configuring when matrix projects notify. #547

kcboschert · 2019-04-02T19:19:45Z

This adds support for users to configure whether a matrix project notifies:

Upon the completion of the parent job.
Upon the completion of each child job.
Both of the above.

This implementation solves #198 by pulling in similar logic from the hipchat and email-ext plugin.

While this does add the matrix-project as a dependency, this seems to be the pattern for how other publishers handle this, as seen by the plugins mentioned above. If this is acceptable, I'll add tests and request final review for merging.

Fixes #198

timja · 2019-04-02T19:42:41Z

Hey, I'll take a look at this soon, could you add the needed tests please?

timja

looking good

timja · 2019-04-02T19:47:43Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -168,6 +176,14 @@ public CommitInfoChoice getCommitInfoChoice() {
        return commitInfoChoice;
    }

+    public MatrixTriggerMode getMatrixTriggerMode() {
+        if (matrixTriggerMode == null) {
+            return MatrixTriggerMode.ONLY_CONFIGURATIONS;


can you initialise it to this and have the getter just be dumb?

timja · 2019-04-02T19:49:03Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -341,20 +362,20 @@ public SlackNotifier(CommitInfoChoice commitInfoChoice) {
    public SlackNotifier(final String baseUrl, final String teamDomain, final String authToken, final boolean botUser, final String room, final String tokenCredentialId,
                         final String sendAs, final boolean startNotification, final boolean notifyAborted, final boolean notifyFailure,
                         final boolean notifyNotBuilt, final boolean notifySuccess, final boolean notifyUnstable, final boolean notifyRegression, final boolean notifyBackToNormal,
-                         final boolean notifyRepeatedFailure, final boolean includeTestSummary, final boolean includeFailedTests,
+                         final boolean notifyRepeatedFailure, final boolean includeTestSummary, final boolean includeFailedTests, MatrixTriggerMode matrixTriggerMode,


need to deprecate the old constructor and add a new one, modifying the constructor breaks compatibility

timja

I noticed an unused import when I was reviewing this so I've added a checkstyle rule for it,

Resolved conflicts for you but can you take a look please

timja · 2019-04-03T07:15:39Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

+    public MatrixAggregator createAggregator(MatrixBuild matrixBuild, Launcher launcher, BuildListener buildListener) {
+        return new MatrixAggregator(matrixBuild, launcher, buildListener) {
+            @Override
+            public boolean startBuild() throws InterruptedException, IOException {


IntelliJ says these exception declarations are unneeded?

Suggested change

public boolean startBuild() throws InterruptedException, IOException {

public boolean startBuild() {

timja · 2019-04-03T07:15:45Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

+            }
+
+            @Override
+            public boolean endBuild() throws InterruptedException, IOException {


IntelliJ says these exception declarations are unneeded?

Suggested change

public boolean endBuild() throws InterruptedException, IOException {

public boolean startBuild() {

…ing compatibility and removing unnecessary invocations/exceptions.

timja · 2019-04-04T06:47:30Z

The build is currently failing for findbug violations:


[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.FailedToParseSlackResponse(Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 40] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.MigratedCredentialDescription() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 108] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.NotificationFailed() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 220] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.NotificationFailedWithException(Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 176] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.SlackSendStepDisplayName() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 87] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.SlackSendStepValues(Object, Object, Object, Object, Object, Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 257] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.720Z] [INFO] The method name jenkins.plugins.slack.Messages.SlackSendStepValuesEmptyMessage() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 199] NM_METHOD_NAMING_CONVENTION

[2019-04-04T01:49:59.721Z] [INFO] 

[2019-04-04T01:49:59.721Z] 

[2019-04-04T01:49:59.721Z] 

[2019-04-04T01:49:59.721Z] To see bug detail using the Findbugs GUI, use the following command "mvn findbugs:gui"

[2019-04-04T01:49:59.721Z] 

[2019-04-04T01:49:59.721Z] 

[2019-04-04T01:49:59.721Z] 

[2019-04-04T01:49:59.721Z] [INFO] ------------------------------------------------------------------------

[2019-04-04T01:49:59.721Z] [INFO] BUILD FAILURE

[2019-04-04T01:49:59.721Z] [INFO] ------------------------------------------------------------------------

[2019-04-04T01:49:59.721Z] [INFO] Total time: 03:21 min

[2019-04-04T01:49:59.721Z] [INFO] Finished at: 2019-04-04T01:49:59Z

[2019-04-04T01:49:59.721Z] [INFO] ------------------------------------------------------------------------

[2019-04-04T01:49:59.721Z] [ERROR] Failed to execute goal org.codehaus.mojo:findbugs-maven-plugin:3.0.5:check (findbugs) on project slack: failed with 7 bugs and 0 errors -> [Help 1]

[2019-04-04T01:49:59.721Z] org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.mojo:findbugs-maven-plugin:3.0.5:check (findbugs) on project slack: failed with 7 bugs and 0 errors

Honestly not sure why it's picking those up, all the other methods in there seem to start with a capital.

Any idea @Casz?

jetersen · 2019-04-04T06:58:42Z

no idea why findbugs is looking at messages 😆 I'd suggest updating parent pom and getting spotbugs instead.

Should be setup to ignore them.

jetersen · 2019-04-08T19:33:54Z

findbugs-exclude.xml

+<FindBugsFilter>
+	<Match>
+    <Class name="~.*\.Messages" />
+    <Bug pattern="NM_METHOD_NAMING_CONVENTION" />


This shouldn't be needed...

jetersen · 2019-04-08T19:36:00Z

Please update parent POM to the least instead, should solve your issue
https://github.com/jenkinsci/slack-plugin/blob/master/pom.xml#L9

kcboschert · 2019-04-08T19:36:56Z

@Casz Forgive my ignorance, but I'm not a Java developer. Can you give me more detail on what you're asking?

jetersen · 2019-04-08T19:38:58Z

Revert last addition and change the parent pom to the latest version: https://github.com/jenkinsci/plugin-pom/blob/master/CHANGELOG.md

slack-plugin/pom.xml

Lines 6 to 10 in a68a8b0

    
           <parent> 
        
               <groupId>org.jenkins-ci.plugins</groupId> 
        
               <artifactId>plugin</artifactId> 
        
               <version>3.33</version> 
        
           </parent>

3.33 is already quite old please bump to latest, which is 3.42

kcboschert · 2019-04-08T19:51:18Z

Updating to 3.42 gives the same errors with spotbugs:

[INFO] --- spotbugs-maven-plugin:3.1.11:check (default-cli) @ slack ---
[INFO] BugInstance size is 7
[INFO] Error size is 0
[INFO] Total bugs: 7
[ERROR] The method name jenkins.plugins.slack.Messages.FailedToParseSlackResponse(Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 40] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.MigratedCredentialDescription() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 108] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.NotificationFailed() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 220] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.NotificationFailedWithException(Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 176] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.SlackSendStepDisplayName() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 87] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.SlackSendStepValues(Object, Object, Object, Object, Object, Object) doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 257] NM_METHOD_NAMING_CONVENTION
[ERROR] The method name jenkins.plugins.slack.Messages.SlackSendStepValuesEmptyMessage() doesn't start with a lower case letter [jenkins.plugins.slack.Messages] At Messages.java:[line 199] NM_METHOD_NAMING_CONVENTION
[INFO]

jetersen · 2019-04-08T20:27:44Z

I think it is mocking that is pulling them into view

Could you try fixing them to lower case?

This reverts commit 7c5802b.

jetersen · 2019-04-08T21:32:16Z

@timja I think you would benefit from adding Travis. Jenkins CI is on the slow side...

Build #8 (Apr 8, 2019 9:20:05 PM) unacceptable waiting time.

timja · 2019-04-09T06:40:52Z

Tested regression, and light touch on the new feature (I don't know anything about matrix, but clicking through and running a build worked)

Adding support for configuring when matrix projects notify.

588426a

timja reviewed Apr 2, 2019

View reviewed changes

timja added the fix label Apr 2, 2019

Merge branch 'master' into matrix_config

03654d7

timja reviewed Apr 3, 2019

View reviewed changes

Adding tests around matrix build notification options. Properly handl…

ea0e271

…ing compatibility and removing unnecessary invocations/exceptions.

kcboschert and others added 2 commits April 8, 2019 14:25

Ignore findbugs method naming conventions for Messages.

7c5802b

Merge branch 'master' into matrix_config

85b0b2c

jetersen requested changes Apr 8, 2019

View reviewed changes

kcboschert added 2 commits April 8, 2019 16:19

Revert "Ignore findbugs method naming conventions for Messages."

afc0a79

This reverts commit 7c5802b.

Update parent pom version to latest. Fix casing with Messages.

198e9da

timja approved these changes Apr 9, 2019

View reviewed changes

timja merged commit 12bfda1 into jenkinsci:master Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for configuring when matrix projects notify. #547

Adding support for configuring when matrix projects notify. #547

kcboschert commented Apr 2, 2019 •

edited by timja

Loading

timja commented Apr 2, 2019

timja left a comment

timja Apr 2, 2019

timja Apr 2, 2019

timja left a comment

timja Apr 3, 2019

timja Apr 3, 2019

timja commented Apr 4, 2019

jetersen commented Apr 4, 2019

jetersen Apr 8, 2019

jetersen commented Apr 8, 2019 •

edited

Loading

kcboschert commented Apr 8, 2019

jetersen commented Apr 8, 2019 •

edited

Loading

kcboschert commented Apr 8, 2019

jetersen commented Apr 8, 2019

jetersen commented Apr 8, 2019 •

edited

Loading

timja commented Apr 9, 2019

	public boolean startBuild() throws InterruptedException, IOException {
	public boolean startBuild() {

	public boolean endBuild() throws InterruptedException, IOException {
	public boolean startBuild() {

Adding support for configuring when matrix projects notify. #547

Adding support for configuring when matrix projects notify. #547

Conversation

kcboschert commented Apr 2, 2019 • edited by timja Loading

timja commented Apr 2, 2019

timja left a comment

Choose a reason for hiding this comment

timja Apr 2, 2019

Choose a reason for hiding this comment

timja Apr 2, 2019

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

timja Apr 3, 2019

Choose a reason for hiding this comment

timja Apr 3, 2019

Choose a reason for hiding this comment

timja commented Apr 4, 2019

jetersen commented Apr 4, 2019

jetersen Apr 8, 2019

Choose a reason for hiding this comment

jetersen commented Apr 8, 2019 • edited Loading

kcboschert commented Apr 8, 2019

jetersen commented Apr 8, 2019 • edited Loading

kcboschert commented Apr 8, 2019

jetersen commented Apr 8, 2019

jetersen commented Apr 8, 2019 • edited Loading

timja commented Apr 9, 2019

kcboschert commented Apr 2, 2019 •

edited by timja

Loading

jetersen commented Apr 8, 2019 •

edited

Loading

jetersen commented Apr 8, 2019 •

edited

Loading

jetersen commented Apr 8, 2019 •

edited

Loading