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

Cleanup environment variable mapping, allow overriding HOME env variable #128

Merged

Conversation

drcapulet
Copy link
Contributor

Since the mounting a drive at /home/jenkins wipes out the directory (which has dot files we'd like to keep around), I wanted to mount the workspace in a subdirectory. Unfortunately, by overriding HOME we can't use the dotfiles. This sets a default HOME to be the workspace directory but allows an override from either the pod or container config. Also removes duplicate env vars from being sent to be more explicit.

Wasn't sure how best to test this since createContainer (and its callers) are all private, and no other examples exist in KubernetesCloudTest.

// Running on OpenShift Enterprise, security concerns force use of arbitrary user ID
// As a result, container is running without a home set for user, resulting into using `/` for some tools,
// and `?` for java build tools. So we force HOME to a safe location.
env.put("HOME", containerTemplate.getWorkingDir());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good idea.

env.add(new EnvVar("HOME", containerTemplate.getWorkingDir(), null));

// Convert our env map to an array
EnvVar[] envVars = env.entrySet().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

I also like this

@iocanel iocanel merged commit 84d8caf into jenkinsci:master Feb 13, 2017
@iocanel
Copy link
Contributor

iocanel commented Feb 13, 2017

@drcapulet: Thanks a lot!

@drcapulet drcapulet deleted the alexc-only-set-home-if-not-present branch February 16, 2017 16:05
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