Do not set empty MAVEN_OPTS environment variable #258
Conversation
| turnOnBuildInjectionAndRestart(agent) | ||
|
|
||
| then: | ||
| with(getMavenOptsFromNodeProperties(agent).split(" ").iterator()) { |
There was a problem hiding this comment.
Why not asserting on the list directly ?
There was a problem hiding this comment.
Yes, both approaches are applicable. I find the iterator approach more expressive and more flexible. It enforces the order, without being dependent on a particular collection type.
| return nodeEnvVars.get(MAVEN_OPTS_VAR); | ||
| @Nullable | ||
| private static String getCurrentMavenOpts(Node node) { | ||
| return EnvUtil.getEnv(node, MAVEN_OPTS_VAR); |
There was a problem hiding this comment.
What about changing EnvUtil.getEnv to return an Optional directly ?
There was a problem hiding this comment.
It's possible. But I'm not sure about the value though. This is a private method with a very limited scope. It's quite easy to ensure that null value is properly handled. Not all usages will really benefit from an Optional here. See removeIfNeeded method for example, where we would need to resolve optional straightaway to be able to perform a check.
| return Optional.ofNullable(mavenOpts) | ||
| .map(this::filterMavenOpts) | ||
| .orElse(""); | ||
| .map(this::filterMavenOpts); |
There was a problem hiding this comment.
Same, how about changing the return result of filterMavenOpts by Optional<String> and flatMap here ?
This pull request introduces the following changes:
MAVEN_OPTSonly if it has the changes added by auto-injectionMAVEN_OPTSinstead of setting an empty valueMAVEN_OPTSdefined in the node properties.Fixes https://issues.jenkins.io/browse/JENKINS-70663 and https://issues.jenkins.io/browse/JENKINS-70692