Skip to content

StashBuilds.java : try again before asking to "PLEASE SET JENKINS ROOT URL… #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 24, 2019
Merged

StashBuilds.java : try again before asking to "PLEASE SET JENKINS ROOT URL… #26

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
422eab9
StashBuilds.java : try again before asking to "PLEASE SET JENKINS ROO…
jimklimov Sep 19, 2018
ee35b3f
StashBuilds.java : first try to load() the global config into our obj…
jimklimov Sep 19, 2018
2b229e5
Just use JenkinsLocationConfiguration.get() per recommendations
jimklimov Sep 24, 2018
1d1b45b
fix nullness
jakub-bochenski Jan 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuilds.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,10 @@ public void onCompleted(AbstractBuild build, TaskListener listener) {
return;
}
Result result = build.getResult();
JenkinsLocationConfiguration globalConfig = new JenkinsLocationConfiguration();
String rootUrl = globalConfig.getUrl();
// Note: current code should no longer use "new JenkinsLocationConfiguration()"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend removing this comment seems like it adds no value, the reasoning behind the change can be in the commit message or the PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, to many newcomers (like myself not so long ago) the code is the documentation. A removed line carries no visibility to git blame, not that many people dig in that at all, and a checked-out workspace or a fork has no data from PR discussions. For this particular piece of code, I found too many examples collecting dust on the internet over many years proposing the new way to get the config, that without the comment someone might be eager to make this particular mistake again.

// as only one instance per runtime is really supported by the current core.
JenkinsLocationConfiguration globalConfig = JenkinsLocationConfiguration.get();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a null check or upgrade to newer jenkins dependency.

My vote is for the former, since the later would reduce compatibility.

String rootUrl = globalConfig == null ? null : globalConfig.getUrl();
String buildUrl = "";
if (rootUrl == null) {
buildUrl = " PLEASE SET JENKINS ROOT URL FROM GLOBAL CONFIGURATION " + build.getUrl();
Expand Down