[JENKINS-51170] Enable the usage of the StepEnvironmentContributor extensionpoint

[t-8ch]

Support the new StepEnvironmentContributor hook introduced in
jenkinsci/workflow-step-api-plugin#36.

This depends on jenkinsci/workflow-step-api-plugin#36.
The dependency version number used depends on the incrementalifyiation (JEP 305).
If requested I can perform this and submit another PR.

[jglick]

The dependency version number used depends on the incrementalifyiation (JEP 305).
If requested I can perform this

Yes should suffice to update the parent (currently to 3.12) and

mvn incrementals:incrementalify

Could be done in this PR so far as I am concerned.

[jglick]

(Somehow the CI build seems to be running even without the consume-incrementals profile, not sure why. Might be an artifact of mirror setup on the CI builder.)

[t-8ch]

I got an error with access restrictions (DoNotUse).
Will try again next week, just left for the weekend.

[t-8ch]

The incrementalization is blocked on jenkinsci/lib-access-modifier#12
(Or we work around by removing some access modifiers)

[jglick]

I got an error with access restrictions (DoNotUse).

Just switch to NoExternalUse.

PR seems to have a bunch of test failures.

[t-8ch]

The hooks work so far for steps. The logic in jenkinsci/workflow-support-plugin#62 is actually sufficient. This PR only contains tests.

I am not sure about also calling the hooks for EnvActionImpl.
This would require to get the current interpreter state from groovy, getting the enclosing scope and somehow using this to fetch the parents scope StepContext from somewhere inside the CPS engine.

Any opinions if this part is required/wanted/superfluous/etc?

[jglick]

Minor comments, but looks OK given the upstreams.

[t-8ch]

@jglick I implemented your comments. Thanks for the review!
EDIT: this depends on #230 now.

[dwnusbaum]

@t-8ch Here is the change you need to fix the enforcer issue:

diff --git a/pom.xml b/pom.xml
index 97bd8cc5..1b796251 100644
--- a/pom.xml
+++ b/pom.xml
@@ -28,7 +28,7 @@
     <parent>
         <groupId>org.jenkins-ci.plugins</groupId>
         <artifactId>plugin</artifactId>
-        <version>3.28</version>
+        <version>3.32</version>
         <relativePath />
     </parent>
     <groupId>org.jenkins-ci.plugins.workflow</groupId>

Unfortunately, after fixing that, I get a lot of test failures that appear to be caused by the workflow-support patch (for example, try ArgumentsActionTestImpl#testArgumentDescriptions). It looks like the addition of get(TaskListener.class) is causing failures in some scenarios where the node is not active or has completed. Here is an example stack trace:

java.io.IOException: cannot start writing logs to a finished node org.jenkinsci.plugins.workflow.cps.nodes.StepAtomNode[id=3] echo in CpsFlowExecution[Owner[argumentDescription/1:argumentDescription #1]]
	at org.jenkinsci.plugins.workflow.support.DefaultStepContext.getListener(DefaultStepContext.java:107)
	at org.jenkinsci.plugins.workflow.support.DefaultStepContext.get(DefaultStepContext.java:78)
	at org.jenkinsci.plugins.workflow.support.DefaultStepContext.get(DefaultStepContext.java:72)
	at org.jenkinsci.plugins.workflow.cps.DSL.invokeStep(DSL.java:239)
	at org.jenkinsci.plugins.workflow.cps.DSL.invokeMethod(DSL.java:176)

Maybe workflow-support should call the old API in the cases where TaskListener.class is not available, or pass TaskListener.NULL or something, but I'm not sure what the best approach would be.

[t-8ch]

I see the following alternatives to aquire a listener:

1. getExecution().getOwner().getListener()
   • Is used just above
2. Remove the check that is throwing the exception (only for our internal call)
   • May have side-effects
3. Do the same as EnvActionImpl.getListener()
   • Looks to be similar to 1), but falls back to plain logging

Personally I think 3) seems to be the best.

[t-8ch]

I implemented prototypes for all three scenarios and all of them pass the testsuites of workflow-support and workflow-cps.

[dwnusbaum]

Thanks for looking into all of the options! Given that the line immediately above the new line in workflow-support uses getExecution().getOwner().getListener(), option 1 seems the best to me for consistency with that behavior. Option 2 seems a little worrying because of potential side effects like you noted, and it seems like if we used option 3, anything sent to the system logs would probably just end up being unhelpful log spam since it wouldn't be associated with the particular build it came from. So my preference would be 1 over 3 over 2.

[t-8ch]

Will do. I won't get around until tomorrow though. If you want feel free to take a shot at it.

[jglick]

getExecution().getOwner().getListener()

Sounds good but better do it in DefaultStepContext.getListener: jenkinsci/workflow-support-plugin#62 (comment)

Do the same as EnvActionImpl.getListener()

No, do not do that.

[t-8ch]

I went with 2 with a fallback to 1, as recommended by @jglick . The tests seem to be happy.