JENKINS-47376: Fix global env variables #245
Conversation
MattLud
force-pushed
the
JENKINS-47376
branch
from
8d44527
to
f16fdf6
Compare
|
The alternate spot to inject these would be here - As part of the final setup before scheduling the Pod and as part of the Pod EnvVars. |
|
|
||
| public ContainerExecDecorator(KubernetesClient client, String podName, String containerName, String namespace, EnvironmentExpander environmentExpander) { | ||
| public ContainerExecDecorator(KubernetesClient client, String podName, String containerName, String namespace, EnvironmentExpander environmentExpander, EnvVars globalVars) { |
There was a problem hiding this comment.
can we keep binary compatibility ?
There was a problem hiding this comment.
Should be done in the latest iteration.
| DescribableList<NodeProperty<?>, NodePropertyDescriptor>globalNodeProperties = instance.getGlobalNodeProperties(); | ||
| List<EnvironmentVariablesNodeProperty> envVarsNodePropertyList = globalNodeProperties.getAll(EnvironmentVariablesNodeProperty.class); | ||
| if ( envVarsNodePropertyList != null && envVarsNodePropertyList.size() != 0 ) { | ||
| globalVars = envVarsNodePropertyList.get(0).getEnvVars(); |
There was a problem hiding this comment.
I assume you need to iterate through envVarsNodePropertyList not just take the first item
There was a problem hiding this comment.
You shouldn't unless there are multiple GlobalNodeProperties in a config.
MattLud
force-pushed
the
JENKINS-47376
branch
2 times, most recently
from
5d3587c
to
c0ee807
Compare
MattLud
force-pushed
the
JENKINS-47376
branch
from
c0ee807
to
7001f17
Compare
| boolean javaHome_detected = false; | ||
| for(String env: procStarter) | ||
| { | ||
| if(env.contains("JAVA_HOME")) |
There was a problem hiding this comment.
I don't understand what does it mean if JAVA_HOME is there, what does that mean?
There was a problem hiding this comment.
and due to the break any variable after JAVA_HOME is ignored, so the order of vars matter
| @@ -96,6 +102,14 @@ public void configureCloud() throws Exception { | |||
| JenkinsLocationConfiguration.get().setUrl(nonLocalhostUrl.toString()); | |||
|
|
|||
| r.jenkins.clouds.add(cloud); | |||
|
|
|||
| DescribableList<NodeProperty<?>, NodePropertyDescriptor> list = r.jenkins.getGlobalNodeProperties(); | |||
| list.getAll(hudson.slaves.EnvironmentVariablesNodeProperty.class); | |||
| // built-in ones. | ||
| boolean javaHome_detected = false; | ||
| for (String env : procStarter) { | ||
| if (env.contains("JAVA_HOME")) { |
There was a problem hiding this comment.
I don't understand what does it mean if JAVA_HOME is there?
and due to the break any variable after JAVA_HOME is ignored, so the order of vars matter
There was a problem hiding this comment.
Removed that break...The JAVA_HOME comes from not being able to tell if the execProc was dispatched by another plugin, thus necessitating the entire provided environment variables to be injected or from the master, thus overwriting whatever local environment variables there were.
At least, that the best I can understand from the various ways Jenkins envVars are provided.
MattLud
force-pushed
the
JENKINS-47376
branch
from
524d452
to
79b6d42
Compare
|
I implemented a similiar fix (before seeing your PR, unfortunately), however, I took a slightly different approach (taking a cue from the pipeline plugin), which might be somewhat more generic. When injecting the variables from the proc starter, I simply removed all variables from computer.environment (key AND value). So anything that is not already there must have come from another plugin (like globalVar, configFile provider, Maven Pipeline Integration etc.). Makes for shorter code and works. The only caveat (which should be documented) is the workings of PATH+X like environment expansions, which must only be done inside container statements to work. If needed, I can adapt my changes to the current state, but this would mostly overshadow this PR. |
|
@pauxus - Feel free to open it and rework/remove items contributed by this PR. This code makes some naive assumptions about envvars and there is probably a better means of handling proc-injected variables though this is mostly for globals. 👍 |
https://issues.jenkins-ci.org/browse/JENKINS-47376
This fixes the missing global environment variables that are missing when a container runs.
As an alternative fix, this could instead be included as part of the POD level environment variables rather than injected and I can move it there if desired.