JENKINS-41748, JENKINS-41890 - Fix env var references and WORKSPACE #110

abayer · 2017-02-09T19:48:30Z

JENKINS issue(s):
- JENKINS-41748
- JENKINS-41890
Description:
- Fixes env var references in environment section so that referring to existing env vars (either from the overall build context or, when in a stage, in the top-level environment actually works right.
- Switches ordering of evaluation of environment and agent at top-level so that we already have a WORKSPACE (if not agent none) before evaluating the environment - this is JENKINS-41890, which depended on the other change and didn't seem to merit a separate PR.
Documentation changes:
- Adding info on referencing other vars in environment directive jenkins-infra/jenkins.io#626
Users/aliases to notify:
- @reviewbybees
- @rsandell
- @aheritier

bitwiseman

This is going to be super confusing. Everywhere else when we want to substitute values in strings we put them in double quotes, but in this one area we need to put them in single quotes? I understand the underlying design/implementation issue, but this implementation exposes and highlights that to the user, making their lives harder instead handling it internally.

Can we take GStrings and do the work internally instead?

abayer · 2017-02-09T20:12:21Z

@bitwiseman I don't believe we can, no, or at least not with the current runtime translation tech. The GStrings get evaluated before I can stop them from doing so. I'll keep experimenting, but I think we're stuck with this.

staffanf · 2017-02-09T20:12:38Z

Would this fix allow me to write something like this?

environment {
    BUILD_VERSION = '${params.RELVERSION?: "0.0.0.1"}'
}

abayer · 2017-02-09T20:18:55Z

@staffanf I...am not honestly sure, but I don't think so. I'll dig into that.

aheritier · 2017-02-09T21:54:50Z

I did a quick test and I have an issue

pipeline {
    agent any

    environment {
        FOO = 'bar'
        OTHER = '${FOO}baz'
    }

    stages {
        stage('Build') {
            steps {
                sh 'echo ${FOO}'
                sh 'echo ${OTHER}'
            }
        }
    }
}

dies with

Started by user admin
[Pipeline] node
Running on master in /Users/arnaud/CloudBees/Support/cases/44820/027-cloudbees-support_2017-02-02_08.11.32/jenkins-home/workspace/declarative-pipeline
[Pipeline] {
[Pipeline] withEnv
[Pipeline] {
[Pipeline] stage
[Pipeline] { (Build)
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // withEnv
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
java.lang.NullPointerException: Cannot invoke method resolveEnvVars() on null object
	at org.codehaus.groovy.runtime.NullObject.invokeMethod(NullObject.java:91)
	at org.codehaus.groovy.runtime.callsite.PogoMetaClassSite.call(PogoMetaClassSite.java:48)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.NullCallSite.call(NullCallSite.java:35)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:141)
	at org.jenkinsci.plugins.pipeline.modeldefinition.model.Stage.getEnvVars(Stage.groovy:100)
	at org.jenkinsci.plugins.pipeline.modeldefinition.model.Stage$getEnvVars.call(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
	at com.cloudbees.groovy.cps.sandbox.DefaultInvoker.methodCall(DefaultInvoker.java:18)
	at org.jenkinsci.plugins.pipeline.modeldefinition.ModelInterpreter.call(jar:file:/Users/arnaud/CloudBees/Support/cases/44820/027-cloudbees-support_2017-02-02_08.11.32/jenkins-home/plugins/pipeline-model-definition/WEB-INF/lib/pipeline-model-definition.jar!/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy:86)
	at ___cps.transform___(Native Method)
	at com.cloudbees.groovy.cps.impl.ContinuationGroup.methodCall(ContinuationGroup.java:57)
	at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.dispatchOrArg(FunctionCallBlock.java:109)
	at com.cloudbees.groovy.cps.impl.FunctionCallBlock$ContinuationImpl.fixArg(FunctionCallBlock.java:82)
	at sun.reflect.GeneratedMethodAccessor235.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
	at com.cloudbees.groovy.cps.impl.PropertyishBlock$ContinuationImpl.get(PropertyishBlock.java:76)
	at com.cloudbees.groovy.cps.LValueBlock$GetAdapter.receive(LValueBlock.java:30)
	at com.cloudbees.groovy.cps.impl.PropertyishBlock$ContinuationImpl.fixName(PropertyishBlock.java:66)
	at sun.reflect.GeneratedMethodAccessor239.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
	at com.cloudbees.groovy.cps.impl.ConstantBlock.eval(ConstantBlock.java:21)
	at com.cloudbees.groovy.cps.Next.step(Next.java:74)
	at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:154)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.access$001(SandboxContinuable.java:18)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:33)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:30)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.runInSandbox(GroovySandbox.java:108)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.run0(SandboxContinuable.java:30)
	at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:163)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:328)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$100(CpsThreadGroup.java:80)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:240)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:228)
	at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:63)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Finished: FAILURE

Maybe I didn't installed correctly the plugin and its dependencies ...

aheritier · 2017-02-09T22:03:23Z

Same with mvn hpi:run

abayer · 2017-02-09T22:29:03Z

@aheritier Fixed. And from my most recent commit message:

Now we can do "${FOO}bar" and "${params.SOMETHING ?: 'something-else'}",
but we can only do this thanks to some weird black magic, no longer
doing runtime translation for environment blocks and instead pulling
it from the ModelAST directly.

I don't love it. It's hackish. There's additional hackery involved in
script.evaluateing to get the more complicated expressions to
resolve as well.

bitwiseman

This is so much better. Thanks!

Some added tests needed.

Also need a test for a validation error message for any unsupported GStrings.

bitwiseman · 2017-02-10T01:10:43Z

pipeline-model-definition/src/test/resources/environmentCrossReferences.groovy

+        stage("foo") {
+            environment {
+                BAZ = "${FOO}BAZ"
+                SPLODE = "${params.WUT ?: 'banana'}"


Can we add a version of this working?
BAZA = "${env.BAR ?: 'nope'}"

Also, in this second env block would this work?
"BAR = "${BAR}FOO"

And would BAR return to the original value outside this particular stage?

So...I should probably rename the variable from SPLODE, since, well, it doesn't 'splode. It works. =)

Yes, BAR = "${BAR}FOO" would result in echoing BAR is FOOBARFOO, and yes, the stage overrides would "reset" - they're only scoped in the context of the withEnv block within the stage they're specified in.

Cool, just wanted to make sure there are tests that ensure that behavior. 👍

abayer · 2017-02-10T01:27:35Z

@rsandell @kzantow - 4fab096 is of interest to both of you, I think. I may yank it from here and just do it over in https://github.com/jenkinsci/pipeline-model-definition-plugin/tree/JENKINS-41759, but I wanted to make sure it worked. =)

bitwiseman · 2017-02-10T01:44:19Z

...nition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties

-JSONParser.MethArgsMissing=Method arguments missing or not an array
-JSONParser.MethCallMustBeObj=Method call definition must be a JSON object
+JSONParser.MethArgsMustBeObj=Individual method or function arguments must be a JSON object
+JSONParser.MethArgsMissing=Method orfunction arguments missing or not an array


or function

rsandell

No likey the inheritance model. And some other questionable things.

And ouch this is gonna be a merge from hell with JENKINS-41759 :)

rsandell · 2017-02-10T11:05:50Z

...odel-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.java

@@ -31,7 +31,7 @@
 * @author Andrew Bayer
 * @author Kohsuke Kawaguchi
 */
-public abstract class ModelASTValue extends ModelASTElement implements ModelASTMethodArg {
+public abstract class ModelASTValue extends ModelASTElement implements ModelASTMethodArg, ModelASTEnvironmentValue {


Throwing me off again with strange domain model hierarchies eh? I do not see the logic in that every ModelASTValue is a ModelASTEnvironmentValue

It means that every ModelASTValue could be used as a ModelASTEnvironmentValue.

So why have a ModelASTEnvironmentValue at all then?

Sorta agree with @rsandell here, if every ModelASTValue is a ModelASTEnvironmentValue what's the point in differentiating them?

@rsandell and I talked about this last week - ModelASTEnvironmentValue means "This element type can be used as a value for an environment variable".

So why not the reverse inheritance? (mostly irrelevant to this PR, and more of a question than anything else)

ModelASTEnvironmentValue (and friends like ModelASTWhenContent and ModelASTMethodArg) are just marker interfaces - saying "any type implementing this interface can be used as a value where this interface is specified as the type". So ModelASTValue, for example, implements both ModelASTEnvironmentValue and ModelASTMethodArg because it can be used in both places (as well as any place that explicitly calls out ModelASTValue), but ModelASTInternalFunctionCall only implements ModelASTEnvironmentValue - so it can be used in contexts looking for ModelASTEnvironmentValues but not those looking for ModelASTMethodArg, or any context that's explicitly looking for a ModelASTValue.

Thinking out loud, albeit a bit pedantically, as I try to make sure I understand what I'm saying... =) Imagine Human, Lizard, and Cat classes, with interfaces HasTail and Mammal. Lizard and Cat (ignore Manx cats for the moment!) would have the HasTail interface, while Human and Cat would have the Mammal interface (yes, not a great example since Human and Cat could just inherit from a Mammal class, but roll with me here).

Also, yes, ModelASTInternalFunctionCall does overlap to some extent with ModelASTMethodCall. The main difference is that ModelASTInternalFunctionCall doesn't allow recursive nesting of instances of itself as arguments, while ModelASTMethodCall does. Might want to rename/reorganize that at some point.

rsandell · 2017-02-10T11:20:14Z

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

-                        // We save the caught error, if any, for throwing at the end of the build.
-                        inDeclarativeAgent(root, root.agent) {
+                // Entire build, including notifications, runs in the agent.
+                inDeclarativeAgent(root, root.agent) {


So you can no longer use environment variables to declare agent? Sort of hinders one of the use cases of JENKINS-41759 where the properties would be the marker file and declare what docker image to run in.

It's one or the other - either you get WORKSPACE available when we evaluate the environment or you get the environment available when we evaluate the agent.

rsandell · 2017-02-10T11:22:45Z

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

+            List<String> evaledEnv = new ArrayList<>()
+            for (int i = 0; i < envVars.size(); i++) {
+                // Evaluate to deal with any as-of-yet unresolved expressions.
+                evaledEnv.add((String)script.evaluate('"' + envVars.get(i) + '"'))


Are we jumping out of the sandbox here?

Shouldn't be, no - it still gets run through the CpsScript's GroovyShell.

abayer · 2017-02-10T22:18:24Z

FYI, gonna table this until #112 lands - it'll be easier to do this once that's in rather than the other way around.

owenhaynes · 2017-02-16T10:34:25Z

This is very much needed to get go-lang builds to work. As you need to set the GOPATH to the workspace as changing dir to /go does not work.

jglick · 2017-02-16T17:04:45Z

Does this make use of jenkinsci/workflow-durable-task-step-plugin#29 or does PMD set WORKSPACE on its own somehow?

abayer · 2017-02-16T17:51:25Z

@jglick Interesting question. It seems WORKSPACE is getting set somehow, but it's not by Declarative.

abayer · 2017-02-17T15:52:31Z

Marking this as aimed at 1.1.

abayer · 2017-03-06T23:22:26Z

Un-tabling this - it'll go in 1.1, while #112 will wait for 1.2. @bitwiseman @rsandell - please re-review ASAP.

ghost · 2017-03-06T23:25:38Z

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-13T10:33:27Z

...c/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTEnvironmentValue.java

+ *
+ * @author Andrew Bayer
+ */
+public interface ModelASTEnvironmentValue {


extends ModelASTMarkerInterface

rsandell · 2017-03-13T10:43:24Z

...on/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/steps/CredentialWrapper.java

@@ -45,7 +45,7 @@
    private final String credentialId;
    private final List<Map<String, Object>> withCredentialsParameters;

-    CredentialWrapper(String credentialId, List<Map<String, Object>> withCredentialsParameters) {
+    public CredentialWrapper(String credentialId, List<Map<String, Object>> withCredentialsParameters) {


@Restricted(NoExternalUse.class) since this will be refactored a bit later and you now made it public.

rsandell · 2017-03-13T10:44:01Z

...nition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties

-JSONParser.MethArgsMissing=Method arguments missing or not an array
-JSONParser.MethCallMustBeObj=Method call definition must be a JSON object
+JSONParser.MethArgsMustBeObj=Individual method or function arguments must be a JSON object
+JSONParser.MethArgsMissing=Method orfunction arguments missing or not an array


or function

kzantow

👍 AFAICT

rsandell

🐝

This changes things to now properly expand environment variable names referred to in `environment` values, building from the job context's environment to the parent context (if per-stage `environment`) and lastly to the immediate `environment` context. This is not exactly *perfect*, per se - you still need to either single quote or escape the dollar sign due to the `GString` getting resolved before we can populate anything. May revisit that in the future to see if there's a workaround we can use.

This is to make sure we have `WORKSPACE` in the environment. Note that this won't quite work right for per-stage agent/environment, though - `WORKSPACE` there will still refer to the top-level `WORKSPACE` (if there is one) or be null if `agent none` is used at the top level. This is so that we have the per-stage environment populated before evaluating any when condition, while not launching in a per-stage agent until said when condition has already been evaluated. Might reconsider this in the future. Also added `.logMatches(String... patterns)` to `AbstractModelDefTest.ExpectationsBuilder` so that we can do regular expression matching against logs, which we needed to verify this.

Now we can do `"${FOO}bar"` and `"${params.SOMETHING ?: 'something-else'}"`, but we can only do this thanks to some weird black magic, no longer doing runtime translation for environment blocks and instead pulling it from the ModelAST directly. I don't love it. It's hackish. There's additional hackery involved in `script.evaluate`ing to get the more complicated expressions to resolve as well.

Still gotta add negative-case tests, and will probably end up folding JENKINS-41759 into this effort at some point, since that's where this will *really* start to be valuable.

bitwiseman · 2017-03-13T17:42:03Z

🐝 Looks good now.

DavidA2014 · 2017-09-21T16:00:19Z

In a declarative script I have tried:
environment {
SCRIPTS_PATH="${WORKSPACE}/Tools/Jenkins/PythonScripts"
}

When the script runs I get exception:

groovy.lang.MissingPropertyException: No such property: WORKSPACE for class: groovy.lang.Binding
    at groovy.lang.Binding.getVariable(Binding.java:63)

Is my syntax incorrect?

(Pipeline: Declarative Agent API v1.1.1, Jenkins 2.79)

abayer · 2017-09-21T16:45:04Z

Assuming you have a top level agent, that should theoretically work, but you'll probably find it more reliable with 1.2, which releases late today or first thing tomorrow.

DavidA2014 · 2017-09-22T07:27:47Z

Thanks Andrew, the problem was that I had no top level agent, as you hinted. Works ok now.

abayer mentioned this pull request Feb 9, 2017

Adding info on referencing other vars in environment directive jenkins-infra/jenkins.io#626

Closed

bitwiseman requested changes Feb 9, 2017

View reviewed changes

bitwiseman requested changes Feb 10, 2017

View reviewed changes

bitwiseman reviewed Feb 10, 2017

View reviewed changes

rsandell requested changes Feb 10, 2017

View reviewed changes

abayer mentioned this pull request Feb 10, 2017

JENKINS-41759 read properties into environment #112

Closed

abayer force-pushed the jenkins-41748 branch from 34ea445 to 9b63c9f Compare February 16, 2017 18:35

bitwiseman mentioned this pull request Feb 17, 2017

[JENKINS-41759] read properties into environment #116

Closed

abayer added the work-in-progress label Feb 17, 2017

abayer added this to the for 1.1 milestone Feb 17, 2017

abayer force-pushed the jenkins-41748 branch 3 times, most recently from fa29e40 to 2ba0be7 Compare February 23, 2017 20:20

davehunt mentioned this pull request Feb 27, 2017

Declarative pipeline and ActiveData submission mozilla/mozillians-tests#293

Merged

abayer force-pushed the jenkins-41748 branch 2 times, most recently from cf4498f to 0eb5073 Compare March 4, 2017 01:24

abayer removed the work-in-progress label Mar 6, 2017

abayer mentioned this pull request Mar 6, 2017

[FIXED JENKINS-42498] Stop evaluating when conditions at runtime #132

Merged

abayer force-pushed the jenkins-41748 branch 2 times, most recently from 8fd7387 to 2d85905 Compare March 10, 2017 15:51

rsandell requested changes Mar 13, 2017

View reviewed changes

kzantow approved these changes Mar 13, 2017

View reviewed changes

rsandell approved these changes Mar 13, 2017

View reviewed changes

abayer added 9 commits March 13, 2017 08:52

Had to special case credentials further. Joy.

ae73b00

Internal function call handled separately in AST

7ed9ce6

Still gotta add negative-case tests, and will probably end up folding JENKINS-41759 into this effort at some point, since that's where this will *really* start to be valuable.

Begone, foul debug statement!

d2622fc

Fix tests.

7c075e6

Make sure JSON conversion works for env cross references

dc31b06

Review comments

ad916f4

abayer force-pushed the jenkins-41748 branch from c400526 to ad916f4 Compare March 13, 2017 15:59

bitwiseman approved these changes Mar 13, 2017

View reviewed changes

abayer merged commit f337baa into jenkinsci:master Mar 13, 2017

JENKINS-41748, JENKINS-41890 - Fix env var references and WORKSPACE #110

JENKINS-41748, JENKINS-41890 - Fix env var references and WORKSPACE #110

Conversation

abayer commented Feb 9, 2017 • edited Loading

bitwiseman left a comment

Choose a reason for hiding this comment

abayer commented Feb 9, 2017

staffanf commented Feb 9, 2017 • edited Loading

abayer commented Feb 9, 2017

aheritier commented Feb 9, 2017

aheritier commented Feb 9, 2017

abayer commented Feb 9, 2017

bitwiseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Feb 10, 2017

owenhaynes commented Feb 16, 2017

jglick commented Feb 16, 2017

abayer commented Feb 16, 2017

abayer commented Feb 17, 2017

abayer commented Mar 6, 2017

ghost commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzantow left a comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

bitwiseman commented Mar 13, 2017

DavidA2014 commented Sep 21, 2017

abayer commented Sep 21, 2017

DavidA2014 commented Sep 22, 2017

abayer commented Feb 9, 2017 •

edited

Loading

staffanf commented Feb 9, 2017 •

edited

Loading