Skip to content
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.

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

Stop using deprecated Util#join #855

Closed
wants to merge 1 commit into from
Closed

Conversation

basil
Copy link
Member

@basil basil commented Sep 11, 2021

This method was deprecated in jenkinsci/jenkins#5467, so migrate away from it in favor of native Java Platform functionality.

Copy link
Member

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

Can't merge as-is as it seems to break the unit-tests.

@@ -193,7 +193,7 @@ private void invokeBody(DockerTransientNode node, TaskListener listener) {
env.overrideExpandingAll(computer.buildEnvironment(listener));
env.put("NODE_NAME", computer.getName());
env.put("EXECUTOR_NUMBER", "0");
env.put("NODE_LABELS", Util.join(node.getAssignedLabels(), " "));
env.put("NODE_LABELS", node.getAssignedLabels().stream().map(Object::toString).collect(Collectors.joining(" ")));
Copy link
Member

Choose a reason for hiding this comment

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

While I'm unsure of the cause, it looks like this change breaks the unit-tests.

...also, I'm not a big fan of having so many method calls daisy-chained on the same line - it makes debugging a NPE nigh-on impossible.
I'd suggest implementing a join method (possibly in this plugin's utils package, maybe as a new class unless it fits well in another), and adding a unit-test for it (to prove it'll cope with a null input, or an input containing nulls, as well as normal non-null stuff), and then call that instead of Util.join.
I suspect that such a unit-test would then reveal that Util.join was more null-safe than this new code and fixing that would then fix the failing unit-tests.

@basil
Copy link
Member Author

basil commented Sep 21, 2021

it looks like this change breaks the unit-tests.

The same unit tests that are broken in the PR build of #857, which does nothing but change a comment in README.md? Get your CI own build in order before you request changes of others.

@basil basil closed this Sep 21, 2021
@basil basil deleted the util-join branch September 21, 2021 14:57
@pjdarton
Copy link
Member

Thank you for bringing that to my attention.
Unfortunately, this is old code (long since abandoned by its original authors) and (as you've demonstrated) it seems to be somewhat fragile. It's not an ideal situation.
I would, of course, welcome a PR that makes it work again (or even educated guesses as to where the problem(s) are as I'd be temped to have a go myself) but, until someone cares enough to fix it (usually when someone wants some non-trivial enhancement merged), it'll remain broken.

@pjdarton
Copy link
Member

FYI I found one issue which I fixed in #858 (the official docker image had changed, which broke the unit tests that used it).
The unit tests now pass ... most of the time.
Sometimes they fail because they weren't able to download a Windows JRE, sometimes because of a ClassNotFound exception, sometimes because a cloud resource ran out of memory etc - the reasons seem to vary from one run to the next but, if one spams the "re-run" button, it's possible to get a clear run. I don't have access to the jenkinsci server internals so I can't investigate why this happens (it works when I run things locally and the jenkinsci service has never been 100% reliable IME)

@basil I've (re)implemented your suggested changes in #862 (which is now merged), so you can cross this plugin off the TODO list in jenkinsci/jenkins#5467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants