-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix overriding HOME env var and remove duplicated env vars #224
Conversation
]) { | ||
|
||
node ('mypod') { | ||
sh """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use single quotes, you don't have to escape the $
..
stage('Run busybox') { | ||
container('busybox') { | ||
sh 'echo inside container' | ||
sh """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dito
.map(TemplateEnvVar::buildEnvVar) | ||
.collect(Collectors.toList())); | ||
containerTemplate.getEnvVars().forEach(item -> | ||
envVarsMap.computeIfAbsent(item.getKey(), k -> item.buildEnvVar()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means "first one wins", correct?
Is this really what we want?
} | ||
|
||
List<EnvVar> defaultEnvVars = env.entrySet().stream() | ||
.map(entry -> new EnvVar(entry.getKey(), entry.getValue(), null)) | ||
.collect(Collectors.toList()); | ||
envVarsList.addAll(defaultEnvVars); | ||
EnvVar[] envVars = envVarsList.stream().toArray(EnvVar[]::new); | ||
defaultEnvVars.forEach( item -> envVarsMap.putIfAbsent(item.getName(), item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency (and because it is shorter), isn't this the same as:
env.entrySet().forEach(item ->
envVarsMap.computeIfAbsent(item.getKey(), k -> new EnvVar(item.getKey(), item.getValue(), null))
);
@marvinthepa thank you for your review. Is it ok now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now.
I remember @carlossg saying that KubernetesPipelineTest is already to large, so maybe that test could be split out (maybe with others concerning environment variable handling).
This PR enables redefining default env variables like HOME and removes duplicated env vars for created containers.
It fixes regression introduced in https://github.com/jenkinsci/kubernetes-plugin/pull/162/files#diff-14316beac1f1489a56a52e21cf8e0ee6R371
After bumping kubernetes-plugin to version 1.0 redefining HOME env variable doesn't work.
One of our pipeline where we need to redefine HOME variable for a container in a podTemplate stopped working.
On kubernetes-plugin version 1.0 mongodb container starts with following setup:
And the pod terminates with an error from mongodb container:
Environment variables should look like here for mongodb container from the example: