StashBuilds.java : try again before asking to "PLEASE SET JENKINS ROOT URL… #26
Conversation
|
In site-local testing, this patch did fix our issue. We see links to particular builds in |
|
In the end, the solution recommended with discussion in jenkinsci/jenkins#3570 (comment) was to just access the data using |
|
Technically, nemccarthy#149 is almost identical, but with a null-ness guard seems to be one line better :) |
@jimklimov it's pointless to do a null check on a |
|
@jakub-bochenski : Just in case, did you verify since when is the value marked as My concern here is if the older Jenkins core (as in "compatible-since" metadata of the plugin) already has that, or the double-check is warranted there? |
|
@jimklimov good catch, apparently this has only been changed in April jenkinsci/jenkins@79ec930#diff-1439884ddd002c6ca6463d9657412b15 |
|
Can someone merge it and make a release, please! Even the stable Jenkins branch is at 2.150.1, which requires this fix. |
|
@proski no, nobody can Make noise there if you want to make a difference. |
|
Done (the message is pending approval). Thank you for stepping up! |
|
Hopefully soon @jakub-bochenski will be a new project maintainer 👍 |
|
Processing the request to become maintainer, BTW, I'm thinking: given you apparently submitted many PRs here @jimklimov, would be interested too to become co-maintainer with @jakub-bochenski ? |
| @@ -49,7 +49,9 @@ public void onCompleted(AbstractBuild build, TaskListener listener) { | |||
| return; | |||
| } | |||
| Result result = build.getResult(); | |||
| JenkinsLocationConfiguration globalConfig = new JenkinsLocationConfiguration(); | |||
| // Note: current code should no longer use "new JenkinsLocationConfiguration()" | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
That would be great. As mentioned I don't plan to actively develop the plugin, only fix the most annoying defects. |
|
What @jakub-bochenski said :) +1 |
|
You've both been granted permissions. Thanks again for stepping up! |
instead of new JenkinsLocationConfiguration() Trying to fix: https://issues.jenkins-ci.org/browse/JENKINS-54054 Inspired by: jenkinsci/stash-pullrequest-builder-plugin#26
|
I guess we need to rebase the branch vs. master for the build to happen |
…T URL FROM GLOBAL CONFIGURATION"
…ect... and do try/catch for cores that do not have this API
jakub-bochenski
force-pushed
the
getRootUrl-fix
branch
from
96c993e to
2b229e5
Compare
| JenkinsLocationConfiguration globalConfig = new JenkinsLocationConfiguration(); | ||
| // Note: current code should no longer use "new JenkinsLocationConfiguration()" | ||
| // as only one instance per runtime is really supported by the current core. | ||
| JenkinsLocationConfiguration globalConfig = JenkinsLocationConfiguration.get(); |
There was a problem hiding this comment.
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.
|
@jakub-bochenski Are you guys planning on releasing a new version of the stash-pullrequest-builder-plugin now that this PR has been merged? |
|
Yes, you can expect a release soon. @jimklimov no pressure, but could you take a look at #36 #37 #38 ? Those are simple changes, I'd prefer to release with a clean project descriptor. |
|
Hope I can, but currently preoccupied with other topics on fosdem. So likely next week. |
…FROM GLOBAL CONFIGURATION"
https://stackoverflow.com/questions/30355079/jenkins-setting-root-url-via-groovy-api was helpful :)
Note: apparently, jenkinsci/jenkins@c3988aa#diff-1439884ddd002c6ca6463d9657412b15 from Aug 18 2018 broke the earlier working setups, since the constructors doing
load()were removed with it. Which sort of contradicts some discussion threads from much earlier ages, that the constructor makes a new empty object so aget()of existing config should be done. But not being much of a Java developer, I might read it wrong... still, it matches our server's mis-behavior starting a few weeks ago after an update.