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

JENKINS-46278: workaround code for providing environment variables to… #204

Merged
merged 1 commit into from Aug 20, 2017

Conversation

Projects
None yet
5 participants
@MattLud

MattLud commented Aug 18, 2017

This fix allows a work around where you can use environment variables to be set in the container.

@scoheb

This comment has been minimized.

Show comment
Hide comment
@scoheb

scoheb Aug 18, 2017

Works well!
Tried with both withEnv and env.SOME_VAR in a standard pipeline (non-declarative).

scoheb commented Aug 18, 2017

Works well!
Tried with both withEnv and env.SOME_VAR in a standard pipeline (non-declarative).

@MattLud

This comment has been minimized.

Show comment
Hide comment
@MattLud

MattLud Aug 18, 2017

Should also work with withCredentials as well!

MattLud commented Aug 18, 2017

Should also work with withCredentials as well!

@ruebenramirez

This comment has been minimized.

Show comment
Hide comment
@ruebenramirez

ruebenramirez commented Aug 18, 2017

+1

@carlossg carlossg merged commit e323ad9 into jenkinsci:master Aug 20, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

scoheb added a commit to scoheb/kubernetes-plugin that referenced this pull request Aug 21, 2017

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Aug 22, 2017

@MattLud @carlossg: If I am not mistaken, this will result in build pods inheriting the env vars of the jenkins master, which can be problematic in many cases (e.g. screwing up PATH, JAVA_HOME etc).

wdyt?

iocanel commented Aug 22, 2017

@MattLud @carlossg: If I am not mistaken, this will result in build pods inheriting the env vars of the jenkins master, which can be problematic in many cases (e.g. screwing up PATH, JAVA_HOME etc).

wdyt?

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Aug 22, 2017

I think that implicitly inheriting from jenkins master can be really dangerous. So, I'd like to counter propose to just take withEnv into consideration. I think we could easily implement that if we picked up the EnvExpander instead of the EnvVars from the context.

iocanel commented Aug 22, 2017

I think that implicitly inheriting from jenkins master can be really dangerous. So, I'd like to counter propose to just take withEnv into consideration. I think we could easily implement that if we picked up the EnvExpander instead of the EnvVars from the context.

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel commented Aug 22, 2017

See #206

@MattLud

This comment has been minimized.

Show comment
Hide comment
@MattLud

MattLud Aug 22, 2017

Agreed - #206 is better - Are there any docs on EnvironmentExpander somewhere?

MattLud commented Aug 22, 2017

Agreed - #206 is better - Are there any docs on EnvironmentExpander somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment