[FIXED JENKINS-42858] Process credentials before rest of environment #141

abayer · 2017-03-17T23:07:25Z

JENKINS issue(s):
- JENKINS-42858
Description:
- I don't think there's a reasonable use case for an environment variable being in the credentials ID, so there's really no reason not to do credentials first, is there? This allows credentials to be referenced by other environment variables.
- Needs jenkinsci/credentials-binding-plugin#34 to be merged/released before we can actually move forward with this, due to withCredentials currently requiring a workspace.
Documentation changes:
- n/a
Users/aliases to notify:
- @reviewbybees
- @rsandell


        [FIXED JENKINS-42858] Process credentials before rest of environment

I don't think there's a reasonable use case for an environment variable being in the credentials ID, so there's really no reason *not* to do credentials first, is there? This allows credentials to be referenced by other environment variables.

reviewbybees · 2017-03-17T23:16:58Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

rsandell · 2017-03-21T13:45:15Z

Since we switched to expanding the environment to after the agent has been allocated there is no reason any more. It used to be needed because the withCredentials step requires a workspace.

rsandell · 2017-03-21T13:48:56Z

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

+                                                    withCredentialsBlock(thisStage.getEnvCredentials()) {
+                                                        withEnvBlock(thisStage.getEnvVars(root, script)) {
+                                                            if (evaluateWhen(thisStage.when)) {
+                                                                inDeclarativeAgent(thisStage, root, thisStage.agent) {


🐛 withCredentials step requires a workspace so this will only work when there is a global agent allocated.

So it won't work with agent none even without this change?

Yup, I see it now. I've opened https://issues.jenkins-ci.org/browse/JENKINS-42999 (with a PR at jenkinsci/credentials-binding-plugin#34) to get withCredentials to stop requiring a workspace unless one or more of the MultiBindings passed to it requires a workspace itself.

That's in now, so yay.

bitwiseman · 2017-03-22T00:40:50Z

@abayer

I don't think there's a reasonable use case for an environment variable being in the credentials ID

No, I can honestly say this not a safe assumption. People will have different credentials depending on possibly branch or other env var - "my-creds-for-${EXECUTION_ENVIRONMENT_NAME}".

This would a place where env var magical ordering is not as good as simple sequential evaluation.

abayer · 2017-03-22T01:20:11Z

@bitwiseman Ugggggh. Well, that's a pain.


        Bump credentials-binding to 1.11 for JENKINS-42999

abayer · 2017-03-30T21:29:38Z

Closed in favor the omnibus at #147

abayer added this to the 1.1.2 milestone Mar 17, 2017

rsandell requested changes Mar 21, 2017

View changes

Bump credentials-binding to 1.11 for JENKINS-42999

Loading status checks…

07f68eb

abayer closed this Mar 30, 2017

jenkinsci / pipeline-model-definition-plugin

[FIXED JENKINS-42858] Process credentials before rest of environment #141

[FIXED JENKINS-42858] Process credentials before rest of environment #141

abayer commented Mar 17, 2017 •

edited

reviewbybees commented Mar 17, 2017

rsandell commented Mar 21, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bitwiseman commented Mar 22, 2017 •

edited

abayer commented Mar 22, 2017

abayer commented Mar 30, 2017

jenkinsci / pipeline-model-definition-plugin

Join GitHub today

[FIXED JENKINS-42858] Process credentials before rest of environment #141

[FIXED JENKINS-42858] Process credentials before rest of environment #141

Conversation

abayer commented Mar 17, 2017 • edited

reviewbybees commented Mar 17, 2017

rsandell commented Mar 21, 2017

This comment has been minimized.

rsandell Mar 21, 2017

This comment has been minimized.

abayer Mar 21, 2017

This comment has been minimized.

abayer Mar 21, 2017

This comment has been minimized.

abayer Mar 30, 2017

bitwiseman commented Mar 22, 2017 • edited

abayer commented Mar 22, 2017

abayer commented Mar 30, 2017

abayer commented Mar 17, 2017 •

edited

bitwiseman commented Mar 22, 2017 •

edited