use JenkinsLocationConfiguration.get() by dodgex · Pull Request #9 · jenkinsci/mattermost-plugin

dodgex · 2019-01-08T07:44:51Z

instead of new JenkinsLocationConfiguration()

Trying to fix: https://issues.jenkins-ci.org/browse/JENKINS-54054

Inspired by: jenkinsci/stash-pullrequest-builder-plugin#26

jovandeginste · 2019-01-09T10:23:08Z

@dodgex thanks for the PR - I will be running your PR in my setup for a few days

furai · 2019-01-13T18:21:41Z

Nicely done. I'll probably use it as well, unless it gets merged upstream pretty soon.

jovandeginste · 2019-01-13T19:13:48Z

@furai Feel free to run it! I will try to merge and publish on Monday

jovandeginste · 2019-01-14T09:05:31Z

@dodgex While building the release, firebug complained about this:

    [INFO] <<< findbugs-maven-plugin:3.0.3:check (findbugs) < :findbugs @ mattermost <<<
    [INFO] 
    [INFO] --- findbugs-maven-plugin:3.0.3:check (findbugs) @ mattermost ---
    [INFO] BugInstance size is 2
    [INFO] Error size is 0
    [INFO] Total bugs: 2
    [INFO] Possible null pointer dereference in jenkins.plugins.mattermost.MattermostNotifier.getBuildServerUrl() due to return value of called method [jenkins.plugins.mattermost.MattermostNotifier, jenkins.plugins.mattermost.MattermostNotifier] Dereferenced at MattermostNotifier.java:[line 84]Known null at MattermostNotifier.java:[line 83] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE
    [INFO] Possible null pointer dereference in jenkins.plugins.mattermost.MattermostNotifier$DescriptorImpl.getBuildServerUrl() due to return value of called method [jenkins.plugins.mattermost.MattermostNotifier$DescriptorImpl, jenkins.plugins.mattermost.MattermostNotifier$DescriptorImpl] Dereferenced at MattermostNotifier.java:[line 383]Known null at MattermostNotifier.java:[line 382] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE
    [INFO]

Would you mind adding a Null check?

dodgex · 2019-01-14T13:36:04Z

I have added a new commit.

jovandeginste · 2019-01-14T15:33:27Z

@dodgex I added two comments as review

jovandeginste

see the inline comments

jovandeginste · 2019-01-14T15:32:43Z

src/main/java/jenkins/plugins/mattermost/MattermostNotifier.java

 			JenkinsLocationConfiguration jenkinsConfig = JenkinsLocationConfiguration.get();
-			return jenkinsConfig.getUrl();
+			String url = jenkinsConfig.getUrl();
+			if (url == null) {


@dodgex I think the test should be whether jenkinsConfig is null (and thus one line higher)

return (jenkinsConfig == null) ? null : jenkinsConfig.getUrl();

/ from the peanut gallery :)

sorry for the false check, i have checked the docs and it didnt seem that get() would return null and for getUrl() it has @CheckForNull...

jovandeginste · 2019-01-14T15:33:05Z

src/main/java/jenkins/plugins/mattermost/MattermostNotifier.java

 				JenkinsLocationConfiguration jenkinsConfig = JenkinsLocationConfiguration.get();
-				return jenkinsConfig.getUrl();
+				String url = jenkinsConfig.getUrl();
+				if (url == null) {


@dodgex same as my other comment

jovandeginste · 2019-01-14T17:03:37Z

FYI, you can test this by running mvn findbugs:check

instead of new JenkinsLocationConfiguration() Trying to fix: https://issues.jenkins-ci.org/browse/JENKINS-54054 Inspired by: jenkinsci/stash-pullrequest-builder-plugin#26

dodgex · 2019-01-15T15:46:02Z

PR is updated and changes have been squashed

jovandeginste · 2019-01-15T20:45:41Z

running the new version in our set-up now

jovandeginste · 2019-01-16T08:58:08Z

New release is coming your way!

dodgex · 2019-01-16T10:47:53Z

Thank you for merging ! :)

jovandeginste requested changes Jan 14, 2019

View reviewed changes

use JenkinsLocationConfiguration.get()

f7b2fac

instead of new JenkinsLocationConfiguration() Trying to fix: https://issues.jenkins-ci.org/browse/JENKINS-54054 Inspired by: jenkinsci/stash-pullrequest-builder-plugin#26

jovandeginste approved these changes Jan 16, 2019

View reviewed changes

jovandeginste merged commit befb619 into jenkinsci:master Jan 16, 2019

dodgex deleted the fix_notification_url branch January 16, 2019 10:47

Conversation

dodgex commented Jan 8, 2019

Uh oh!

jovandeginste commented Jan 9, 2019

Uh oh!

furai commented Jan 13, 2019

Uh oh!

jovandeginste commented Jan 13, 2019

Uh oh!

jovandeginste commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dodgex commented Jan 14, 2019

Uh oh!

jovandeginste commented Jan 14, 2019

Uh oh!

jovandeginste left a comment

Choose a reason for hiding this comment

Uh oh!

jovandeginste Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

akomakom Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

dodgex Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

jovandeginste Jan 14, 2019

Choose a reason for hiding this comment

Uh oh!

jovandeginste commented Jan 14, 2019

Uh oh!

dodgex commented Jan 15, 2019

Uh oh!

jovandeginste commented Jan 15, 2019

Uh oh!

jovandeginste commented Jan 16, 2019

Uh oh!

dodgex commented Jan 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jovandeginste commented Jan 14, 2019 •

edited

Loading