[JENKINS-29537] EnvironmentContributingAction compatible with Workflow #2975

Jimilian · 2017-08-10T06:57:59Z

Adds GenericEnvironmentContributingAction
Deprecates EnvironmentContributingAction

See JENKINS-29537.

Internal: JENKINS-29537, Add GenericEnvironmentContributingAction that supports WorkflowJobs as well as AbstractProjects.

Downstream PR:

Provide the data about parent builds as environment variable to the dependent build gerrit-trigger-plugin#319

@jglick, @rsandell

daniel-beck · 2017-08-10T10:38:41Z

Can't we keep the existing interface around and do something with Java 8 default methods?

oleg-nenashev · 2017-08-10T10:52:19Z

Yes, new interface is not required for Java 8. You can even make the default method to call the obsolete Abstract Project one if implemented (see Util.isOverridden()). And then you can make the old implementation optional as well by offering the default method.

I was considering doing similar patch for https://issues.jenkins-ci.org/browse/JENKINS-42614 (Pipeline Support in EnvInject). According to that experience I would definitely recommend adding a an optional @CheckForNull Node node parameter to the method. Otherwise it would complicated to integrate plugins depending on the Node state

rsandell · 2017-08-10T12:16:30Z

Yes, if this becomes just an addition to the old interface I believe we can use it in plugins without needing to bump dependency to a newer core.

Jimilian · 2017-08-10T12:47:04Z

Sounds reasonable, will try to do.

+ Adds new method with default implementation in EnvironmentContributingAction to support Runs + Marks AbstractBuild as deprecated + Adds default implementation for deprecated method for backward compatiblity.

Jimilian · 2017-08-14T06:50:11Z

@daniel-beck, @oleg-nenashev, @rsandell, done. Please, take a look.

oleg-nenashev

LGTM 👍 , just minor Javadoc comments. Also created https://issues.jenkins-ci.org/browse/JENKINS-46169 as a follow-up

oleg-nenashev · 2017-08-14T07:08:24Z

core/src/main/java/hudson/model/Run.java

@@ -2302,6 +2302,9 @@ public EnvVars getEnvironment() throws IOException, InterruptedException {
        for (EnvironmentContributor ec : EnvironmentContributor.all().reverseView())
            ec.buildEnvironmentFor(this,env,listener);

+        for (EnvironmentContributingAction a : getActions(EnvironmentContributingAction.class))
+            a.buildEnvVars(this,env,n);


Nice 2 have for new extension invocations: try/catch unexpected runtime exceptions. NIT in this case since there is same old code above

From my personal perspective some more generic way to do it is needed. I.e. some decorator that wraps all calls by try/catch. Or at least forces developers to do it.

I agree. I was thinking about adding a wrapper for it using the Java 8 magic.
The only concern is about performance requirements for particular extensions.

oleg-nenashev · 2017-08-14T07:08:48Z

core/src/main/java/hudson/model/EnvironmentContributingAction.java

+     *      The node execute on. Can be null.
+     * @param env
+     *      Environment variables should be added to this map.
+     */


@since TODO

oleg-nenashev · 2017-08-14T07:09:39Z

core/src/main/java/hudson/model/EnvironmentContributingAction.java

+     * @param run
+     *      The calling build. Never null.
+     * @param node
+     *      The node execute on. Can be null.


Can be {@code null} when the Run is not binded to the node (e.g. in Pipeline outside the {@code node() step})

oleg-nenashev · 2017-08-14T07:10:15Z

core/src/main/java/hudson/model/EnvironmentContributingAction.java

+     * Called by {@link Run} or {@link AbstractBuild} to allow plugins to contribute environment variables.
+     *
+     * @param run
+     *      The calling build. Never null.


Instead of "Never null" it's better to add the @Nonnull annotation to the method argument

rsandell · 2017-08-14T09:21:11Z

👍

oleg-nenashev · 2017-08-14T09:21:22Z

@jenkinsci/code-reviewers

oleg-nenashev · 2017-08-22T08:32:53Z

No negative feedback after 1 week, merging

jglick · 2017-08-24T15:38:15Z

core/src/main/java/hudson/model/EnvironmentContributingAction.java

+     * @param run
+     *      The calling build. Never null.
+     * @param node
+     *      The node execute on. Can be {@code null} when the Run is not binded to the node,


Seems inappropriate. What is this argument for?

jglick · 2017-08-24T15:39:35Z

Should not have been merged without downstream PRs demonstrating the API in action. In fact I think the signature is wrong; the Node will always be null when used from Pipeline—the supposed purpose of the change.

daniel-beck · 2017-08-24T15:42:46Z

@jglick There's still time to revert.

jglick · 2017-08-24T15:46:19Z

According to that experience I would definitely recommend adding a an optional @CheckForNull Node node parameter to the method. Otherwise it would complicated to integrate plugins depending on the Node state

No, this should not be here. It will not work anyway.

for an AbstractBuild, you can already call getBuildOn
for a WorkflowRun, there is no specific Node without a StepContext, which is not going to be here

oleg-nenashev · 2017-08-24T15:46:39Z

I propose to discuss it first before reverting. I confirm this API is useful, and the nullable Node is also useful. I had pretty same PoC for EnvInject. Even Pipeline can contribute node when it is available in the context.

@jenkinsci/code-reviewers were pinged and definitely had enough time to provide feedback

daniel-beck · 2017-08-24T15:56:31Z

It's much easier to properly re-do an addition that was backed out, than to fix something with conceptual problems once it's in.

jglick · 2017-08-24T16:04:52Z

I confirm this API is useful, and the nullable Node is also useful.

Confirmed how? “Confirmation” to me means a downstream PR using a timestamped snapshot dep on the core PR showing an EnvironmentContributingAction implementation implementing the new method, and using the Node parameter, with a functional test using createSlave or similar to demonstrate that the correct node is in fact selected.

Even Pipeline can contribute node when it is available in the context.

There is no context here. Only a Run, which can have any number of active contexts.

@jenkinsci/code-reviewers were pinged and definitely had enough time to provide feedback

Sure, but unfortunately I just cannot keep up with all those messages, nor @-mentions. For the future, best to “request a review” from me, which at least shows up in my personal board.

It's much easier to properly re-do an addition that was backed out, than to fix something with conceptual problems once it's in.

Yes, an API is forever I am afraid.

jglick · 2017-08-24T16:21:58Z

core/src/main/java/hudson/model/AbstractBuild.java

@@ -881,7 +881,7 @@ public EnvVars getEnvironment(TaskListener log) throws IOException, InterruptedE
                e.buildEnvVars(env);

        for (EnvironmentContributingAction a : getActions(EnvironmentContributingAction.class))
-            a.buildEnvVars(this,env);
+            a.buildEnvVars(this,env,getBuiltOn());


AFAICT this loop should just be deleted since it is already being called in the super; otherwise we would be calling each EnvironmentContributingAction twice on a single AbstractBuild.

jglick · 2017-08-24T16:24:26Z

BTW jenkinsci/gerrit-trigger-plugin#319 may be related, but it is not downstream—does not demonstrate that this works.

jglick · 2017-08-24T16:31:07Z

I propose to revert before 2.76 gets cut on Sunday or Monday and the API is set in stone.

oleg-nenashev · 2017-08-24T16:32:48Z

I propose to give @Jimilian an opportunity to respond first. I will process the feedback above. If we have no agreement, let's revert it on Friday's evening in EU

jglick · 2017-08-24T16:33:47Z

the Node will always be null when used from Pipeline

Or perhaps, worse, always Jenkins, i.e. master, even when the build is in fact running some node block on an agent. Not sure which it is offhand.

jglick · 2017-08-24T16:40:03Z

core/src/main/java/hudson/model/ParametersAction.java

@@ -138,10 +138,11 @@ public void createBuildWrappers(AbstractBuild<?,?> build, Collection<? super Bui
        }
    }

-    public void buildEnvVars(AbstractBuild<?,?> build, EnvVars env) {
+    @Override
+    public void buildEnvVars(Run<?,?> run, EnvVars env, Node node) {


This is going to cause this code to process Pipeline build parameters twice, until workflow-job is updated to disable its own copy of the logic when running on a sufficiently new core version. Probably harmless but needs to be investigated.

jglick · 2017-08-24T17:13:15Z

Not sure which it is offhand.

Just checked. It is null in all cases. So the node parameter is clearly useless and misleading: for WorkflowRun will always be null; for AbstractBuild will be the same as getBuiltOn.

jglick · 2017-08-24T21:27:19Z

Rather than reverting per se, I filed #2993 to correct the problems I identified in this PR, and jenkinsci/workflow-job-plugin#67 to demonstrate its usefulness in a plugin (though a PR showing a plugin implementation of the new method would be valuable too).

jglick · 2018-01-05T22:36:26Z

core/src/main/java/hudson/model/EnvironmentContributingAction.java

     * @param build
     *      The calling build. Never null.
     * @param env
     *      Environment variables should be added to this map.
     */
-    void buildEnvVars(AbstractBuild<?, ?> build, EnvVars env);
+    @Deprecated
+    @Restricted(ProtectedExternally.class)


Causing me problems in jenkinsci/maven-plugin#109. Why was this necessary?

It was supposed to make upgrading plugin maintainers to migrate to new APIs. But I agree that such approach creates issues in Jenkinsfiles testing against newer cores. PCT is not my concern, it should probably ignore @Restricted annotations and just report them in the output without failing the entrire build

…2975.

Jimilian force-pushed the JENKINS-29537 branch from 0e87833 to b6d7109 Compare August 10, 2017 07:14

Jimilian mentioned this pull request Aug 10, 2017

Provide the data about parent builds as environment variable to the dependent build jenkinsci/gerrit-trigger-plugin#319

Merged

oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Aug 11, 2017

Jimilian force-pushed the JENKINS-29537 branch from b6d7109 to b178836 Compare August 12, 2017 11:59

[JENKINS-29537] EnvironmentContributingAction compatible with Workflow

33cc3cf

+ Adds new method with default implementation in EnvironmentContributingAction to support Runs + Marks AbstractBuild as deprecated + Adds default implementation for deprecated method for backward compatiblity.

Jimilian force-pushed the JENKINS-29537 branch from b178836 to 33cc3cf Compare August 12, 2017 12:01

oleg-nenashev approved these changes Aug 14, 2017

View reviewed changes

Tiny improvements in javadoc

519627e

oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes on-hold This pull request depends on another event/release, and it cannot be merged right now and removed work-in-progress The PR is under active development, not ready to the final review labels Aug 14, 2017

oleg-nenashev merged commit c60735a into jenkinsci:master Aug 22, 2017

Jimilian deleted the JENKINS-29537 branch August 22, 2017 08:35

jglick reviewed Aug 24, 2017

View reviewed changes

jglick mentioned this pull request Aug 24, 2017

[JENKINS-29537] Amended EnvironmentContributingAction signature #2993

Closed

jglick mentioned this pull request Aug 25, 2017

[JENKINS-42499] Removing caching from EnvActionImpl jenkinsci/workflow-cps-plugin#106

Merged

2 tasks

jglick reviewed Jan 5, 2018

View reviewed changes

jglick added a commit to jglick/maven-plugin that referenced this pull request Jan 5, 2018

Make AbstractMavenBuild compilable before or after jenkinsci/jenkins#…

984f69c

…2975.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-29537] EnvironmentContributingAction compatible with Workflow #2975

[JENKINS-29537] EnvironmentContributingAction compatible with Workflow #2975

Jimilian commented Aug 10, 2017 •

edited by oleg-nenashev

daniel-beck commented Aug 10, 2017

oleg-nenashev commented Aug 10, 2017 •

edited

rsandell commented Aug 10, 2017 •

edited

Jimilian commented Aug 10, 2017

Jimilian commented Aug 14, 2017

oleg-nenashev left a comment

oleg-nenashev Aug 14, 2017

Jimilian Aug 14, 2017

oleg-nenashev Aug 14, 2017

oleg-nenashev Aug 14, 2017

oleg-nenashev Aug 14, 2017

oleg-nenashev Aug 14, 2017

rsandell commented Aug 14, 2017

oleg-nenashev commented Aug 14, 2017

oleg-nenashev commented Aug 22, 2017

jglick Aug 24, 2017

jglick commented Aug 24, 2017

daniel-beck commented Aug 24, 2017

jglick commented Aug 24, 2017

oleg-nenashev commented Aug 24, 2017

daniel-beck commented Aug 24, 2017

jglick commented Aug 24, 2017

jglick Aug 24, 2017

jglick commented Aug 24, 2017

jglick commented Aug 24, 2017

oleg-nenashev commented Aug 24, 2017

jglick commented Aug 24, 2017

jglick Aug 24, 2017

jglick commented Aug 24, 2017

jglick commented Aug 24, 2017

jglick Jan 5, 2018

oleg-nenashev Jan 9, 2018

[JENKINS-29537] EnvironmentContributingAction compatible with Workflow #2975

[JENKINS-29537] EnvironmentContributingAction compatible with Workflow #2975

Conversation

Jimilian commented Aug 10, 2017 • edited by oleg-nenashev

daniel-beck commented Aug 10, 2017

oleg-nenashev commented Aug 10, 2017 • edited

rsandell commented Aug 10, 2017 • edited

Jimilian commented Aug 10, 2017

Jimilian commented Aug 14, 2017

oleg-nenashev 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

rsandell commented Aug 14, 2017

oleg-nenashev commented Aug 14, 2017

oleg-nenashev commented Aug 22, 2017

Choose a reason for hiding this comment

jglick commented Aug 24, 2017

daniel-beck commented Aug 24, 2017

jglick commented Aug 24, 2017

oleg-nenashev commented Aug 24, 2017

daniel-beck commented Aug 24, 2017

jglick commented Aug 24, 2017

Choose a reason for hiding this comment

jglick commented Aug 24, 2017

jglick commented Aug 24, 2017

oleg-nenashev commented Aug 24, 2017

jglick commented Aug 24, 2017

Choose a reason for hiding this comment

jglick commented Aug 24, 2017

jglick commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimilian commented Aug 10, 2017 •

edited by oleg-nenashev

oleg-nenashev commented Aug 10, 2017 •

edited

rsandell commented Aug 10, 2017 •

edited