[FIXED JENKINS-44456] Add STAGE_NAME to environment #10

abayer · 2017-05-23T16:37:20Z

cc @reviewbybees

ghost · 2017-05-23T16:47:49Z

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.

jglick · 2017-05-23T16:48:16Z

src/main/java/org/jenkinsci/plugins/workflow/support/steps/StageStepExecution.java

+            this.stageName = stageName;
+        }
+        @Override public void expand(EnvVars env) throws IOException, InterruptedException {
+            env.override("STAGE_NAME", stageName);


Eek. It is time to introduce an API

public static EnvironmentExpander constant(Map<String, String> env);

Yeaaaaah. On it.

jenkinsci/workflow-step-api-plugin#25 FTR

jglick · 2017-05-23T16:49:25Z

src/test/java/org/jenkinsci/plugins/workflow/support/steps/StageStepTest.java

+            @Override public void evaluate() throws Throwable {
+                WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+                p.setDefinition(new CpsFlowDefinition("stage('foo-stage') {echo \"STAGE_NAME is ${STAGE_NAME}\"}", true));
+                WorkflowRun b = story.j.assertBuildStatusSuccess(p.scheduleBuild2(0));


Could add another step to prove that the serialization works, but probably overkill here.

abayer · 2017-05-23T17:10:30Z

Gonna leave this for a day to see if anyone else has any feedback before merging.

jglick · 2017-05-25T19:36:39Z

Feel free to release—just be sure to update the changelog in the wiki if so.

abayer · 2017-05-25T20:42:15Z

Can't yet since I don't have the rights, but opening a PR for that now.

svanoort · 2017-05-25T21:02:17Z

Um. I didn't see this in time, but really REALLY don't encourage this approach -- better to use Context variables. @jglick @abayer

jglick · 2017-05-25T21:07:09Z

@svanoort as in, StepContext.get? How would that address the user request here, which IIUC was to be able to do something like

pipeline {
  stages {
    stage('build') {…}
    stage('test') {…}
  }
  post {
    failure {
      mail …, subject: "Failure in $STAGE_NAME"
    }
  }
}

? I am not following.

svanoort · 2017-05-25T21:27:41Z

🐛 Because I feel we're generally following the wrong path here.

Need to explain more fully, but I do not consider this appropriate to release until we can have a more full discussion.

abayer · 2017-05-25T21:46:45Z

As I've said offline, I am very confused as to how adding an environment variable is the wrong path for, well, adding an environment variable. If you've got a better approach for delivering the end result of having STAGE_NAME available in the running Pipeline's environment a la WORKSPACE, NODE_NAME, JOB_NAME, etc, I'm totally open to it, since that's the entirety of what we're trying to do here - no additional complications, etc, just adding an environment variable for the sake of there being such an environment variable.

svanoort · 2017-06-02T14:44:35Z

My issue here is that there is not another good way provided to obtain the Stage information for APIs (as opposed to within pipeline code). This encourages people to (mis) use environment variables within plugins if we need stage info, rather than replying on a proper API.

If we do one without the other, then you're going to get some horrifying code.

jglick · 2017-06-02T15:30:31Z

there is not another good way provided to obtain the Stage information for APIs

Huh? You just scan up for the nearest enclosing stage block and grab its label, using fetchEnclosingBlocks. Probably easier than accessing the environment variable.

abayer · 2017-06-12T13:32:00Z

So...can we release this? I know it's not optimal for @svanoort but I really don't think it's a risk in terms of plugin devs using it instead of other methodologies.

svanoort · 2017-06-12T14:31:18Z

Huh? You just scan up for the nearest enclosing stage block and grab its label, using fetchEnclosingBlocks. Probably easier than accessing the environment variable.

This is actually both very inefficient and nonintuitive for people. It's fine from time to time but can potentially trigger iteration to the first flownode every time it is run.

If we're binding the stage name into the EnvVars, why not bind it in the StepContext for in-progress flows as well? If you expose it there (via something like a StageName class), I'm satisfied.

Otherwise my original concerns still have not been addressed.

abayer · 2017-06-12T15:02:06Z

@svanoort Only hassle I'd see with that is that we'd need a distinct class for it, since String would not exactly be distinctive. =) But I'll see if I can come up with something.

jglick · 2017-06-12T15:30:28Z

This is actually both very inefficient

Well then optimize it please. There is no inherent reason for this to be anything but instantaneous.

and nonintuitive

How so? Seems very intuitive to me that if you want to know what stage something is running in, you call the APIs which list enclosing blocks and, well, search for a stage block.

trigger iteration to the first flownode

OK, so?

why not bind it in the StepContext for in-progress flows as well

Because it is redundant. The flow graph already contains this information, so we would be providing an API for accessing something that you can already access.

I still see no reason why release of this enhancement should be held up.

svanoort · 2017-06-12T16:27:56Z

Well then optimize it please. There is no inherent reason for this to be anything but instantaneous.

I've tried, but you felt the FlowStorage engine is the only place to retain the needed info -- which means a much deeper change, with additional testing needs. The arguments for this were reasonable from an architectural standpoint; however this means that it is not solvable by the easier solution I'd proposed such as caching or retaining a sketch of the flow structure to satisfy block/parallel structural queries.

Because it is redundant. The flow graph already contains this information, so we would be providing an API for accessing something that you can already access.

The flow graph should be consider the system of record for completed runs, but for in-progress flows it is completely fair to store some runtime information (as we already do).

Implementing it that way is a fairly trivial change that follows the same pattern laid down here -- and I see no reason why StageAction shouldn't be able to simplify our lives by offering something like getCurrentStage(FlowExection) and piggybacking on the same mechanism here.

abayer · 2017-06-12T20:12:43Z

Wait, so StageAction or StepContext? I'm confused.

svanoort · 2017-06-12T20:14:00Z

@abayer Stored in the StepContext, StageAction just returns it from context if the FlowExecution is not completed (or null if absent).

abayer · 2017-06-12T20:15:30Z

@svanoort But StageAction doesn't have the StepContext. Like I said, I'm confused.

abayer · 2017-06-12T20:16:03Z

Or do you mean a static method?

abayer · 2017-06-12T20:17:34Z

...all told, while I do understand that there could be value in easier API access to the current stage's name, I'm with @jglick in the end - I don't see how that is a requirement for adding an environment variable. @svanoort, if it's something you think should be there, I'd happily review and +1 it, but it's orthogonal to the need that drove this change.

svanoort · 2017-06-12T20:37:36Z

Static method. The method is probably a 3-5 liner once you add the object to the StepContext.

abayer · 2017-07-24T14:19:29Z

@svanoort Can we release this? Getting some pressure on JIRA to get this rolled out.

zmeggyesi · 2017-07-24T14:25:30Z

I'm probably that pressure :)
Feel free not to rush this, for now, I compiled master and I'm using that, but I wouldn't complain if I could get a proper release instead of using a self-built version.

svanoort · 2017-10-05T14:42:53Z

@abayer I still feel that it is inappropriate to have this as an environment variable. I feel that the instant we have users writing pipeline code that changes depending on what stage they are in, we're opening up a nasty can of worms just for limited value add.

Printing stage name on the other hand, I think is pretty okay (for notifications and so forth), but unfortunately we can't gain one without the other.

abayer · 2017-11-02T16:00:28Z

@svanoort There are valid use cases for, say, being able to have shell scripts detect what stage they're on via said env var, etc... I really honestly don't see how this is problematic.

svanoort · 2017-11-02T16:30:58Z

@abayer To capture out of band discussion -- now that there's a pretty easy way for plugin authors to fetch enclosing stages (jenkinsci/workflow-api-plugin#53) I think it's safe to release this -- If you can add a warning in the Javadocs NOT to use the Env Var for plugin code (and a pointer to the workflow-api use to do) -- my main concern was people abusing this API in plugins (no no no no).

That takes care of my big objections (pending a PR adding that note) and once done I'm happy to see it released and all.

abayer · 2017-11-02T17:03:10Z

And for everyone else's benefit, I've released this plugin's version 2.3, which includes this change. =)

[FIXED JENKINS-44456] Add STAGE_NAME to environment

2e7f175

abayer requested a review from jglick May 23, 2017 16:37

jglick approved these changes May 23, 2017

View reviewed changes

abayer mentioned this pull request May 23, 2017

Provide an API for an EnvironmentExpander of constant string->string map jenkinsci/workflow-step-api-plugin#25

Merged

abayer closed this May 24, 2017

abayer reopened this May 24, 2017

Bump structs and scm-api to fix InjectedTest

59df6d1

abayer merged commit 850947f into jenkinsci:master May 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-44456] Add STAGE_NAME to environment #10

[FIXED JENKINS-44456] Add STAGE_NAME to environment #10

abayer commented May 23, 2017

ghost commented May 23, 2017

jglick May 23, 2017

abayer May 23, 2017

jglick May 25, 2017

jglick May 23, 2017

abayer commented May 23, 2017

jglick commented May 25, 2017

abayer commented May 25, 2017

svanoort commented May 25, 2017

jglick commented May 25, 2017

svanoort commented May 25, 2017

abayer commented May 25, 2017

svanoort commented Jun 2, 2017

jglick commented Jun 2, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

jglick commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

abayer commented Jun 12, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jul 24, 2017

zmeggyesi commented Jul 24, 2017

svanoort commented Oct 5, 2017

abayer commented Nov 2, 2017

svanoort commented Nov 2, 2017

abayer commented Nov 2, 2017

[FIXED JENKINS-44456] Add STAGE_NAME to environment #10

[FIXED JENKINS-44456] Add STAGE_NAME to environment #10

Conversation

abayer commented May 23, 2017

ghost commented May 23, 2017

jglick May 23, 2017

Choose a reason for hiding this comment

abayer May 23, 2017

Choose a reason for hiding this comment

jglick May 25, 2017

Choose a reason for hiding this comment

jglick May 23, 2017

Choose a reason for hiding this comment

abayer commented May 23, 2017

jglick commented May 25, 2017

abayer commented May 25, 2017

svanoort commented May 25, 2017

jglick commented May 25, 2017

svanoort commented May 25, 2017

abayer commented May 25, 2017

svanoort commented Jun 2, 2017

jglick commented Jun 2, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

jglick commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jun 12, 2017

abayer commented Jun 12, 2017

abayer commented Jun 12, 2017

svanoort commented Jun 12, 2017

abayer commented Jul 24, 2017

zmeggyesi commented Jul 24, 2017

svanoort commented Oct 5, 2017

abayer commented Nov 2, 2017

svanoort commented Nov 2, 2017

abayer commented Nov 2, 2017