Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-29144

SimpleBuildStep to receive EnvVars

    Details

    • Similar Issues:
    • Released As:
      Jenkins 2.241

      Description

      SimpleBuildStep.perform needs to be given an EnvVars argument. Otherwise there is no way for an implementation to observe any local environment variable settings.

        Attachments

          Issue Links

            Activity

            jglick Jesse Glick created issue -
            jglick Jesse Glick made changes -
            Field Original Value New Value
            Link This issue is blocking JENKINS-23713 [ JENKINS-23713 ]
            Hide
            jglick Jesse Glick added a comment -

            A way for the step to set variables has also been requested. In a freestyle build I suppose this would simply set build variables. In a Pipeline build, not sure. Return a Map<String,String> from step?

            Show
            jglick Jesse Glick added a comment - A way for the step to set variables has also been requested. In a freestyle build I suppose this would simply set build variables. In a Pipeline build, not sure. Return a Map<String,String> from step ?
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 164012 ] JNJira + In-Review [ 181474 ]
            abayer Andrew Bayer made changes -
            Labels workflow pipeline workflow
            abayer Andrew Bayer made changes -
            Labels pipeline workflow pipeline
            Hide
            jglick Jesse Glick added a comment -

            In a freestyle build I suppose this would simply set build variables.

            Or via EnvironmentContributingAction, which is also not an option for a SimpleBuildStep due to JENKINS-29537.

            Show
            jglick Jesse Glick added a comment - In a freestyle build I suppose this would simply set build variables. Or via EnvironmentContributingAction , which is also not an option for a SimpleBuildStep due to JENKINS-29537 .
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-29537 [ JENKINS-29537 ]
            Hide
            jglick Jesse Glick added a comment -

            Without JENKINS-40070Run.getEnvironment cannot be used as a workaround.

            Show
            jglick Jesse Glick added a comment - Without  JENKINS-40070 ,  Run.getEnvironment cannot be used as a workaround.
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-40070 [ JENKINS-40070 ]
            jglick Jesse Glick made changes -
            Labels pipeline api pipeline
            Hide
            dbrooks David Brooks added a comment -

            Has there been any progress on this issue? This is blocking our company from updating our jobs to from Freestyle to Pipelines.

            Could the solution be as easy as doing what the old

            AbstractBuild getEnvironment()

            did which was holding onto an array that contained the RunListener's environments and NodeProperty's environments? And then in

            Run getEnvironment()

            we could add the environments from the array we gathered via 

            buildEnvironmentFor(this,env,listener);

            ?

            Show
            dbrooks David Brooks added a comment - Has there been any progress on this issue? This is blocking our company from updating our jobs to from Freestyle to Pipelines. Could the solution be as easy as doing what the old AbstractBuild getEnvironment() did which was holding onto an array that contained the RunListener's environments and NodeProperty's environments? And then in Run getEnvironment() we could add the environments from the array we gathered via  buildEnvironmentFor( this ,env,listener); ?
            Hide
            jglick Jesse Glick added a comment -

            This is blocking our company from …

            If you implement Step directly there are no restrictions.

            Could the solution be as easy as …

            No, that is not how it would work.

            Show
            jglick Jesse Glick added a comment - This is blocking our company from … If you implement Step directly there are no restrictions. Could the solution be as easy as … No, that is not how it would work.
            Hide
            dbrooks David Brooks added a comment -

            Jesse Glick The header of our class is

            public class IntrepidBuilder extends Builder implements SimpleBuildStep

            since we want our plugin to be compatible with both Freestyle and Pipelines. How might we update this to still allow the end user to use the "pipeline syntax generator" to call our build step?

            Show
            dbrooks David Brooks added a comment - Jesse Glick The header of our class is public class IntrepidBuilder extends Builder implements SimpleBuildStep since we want our plugin to be compatible with both Freestyle and Pipelines. How might we update this to still allow the end user to use the "pipeline syntax generator" to call our build step?
            Hide
            jglick Jesse Glick added a comment -

            Currently if you want to access environment variables, you cannot; you must implement Step instead.

            Show
            jglick Jesse Glick added a comment - Currently if you want to access environment variables, you cannot; you must implement Step instead.
            Hide
            jeremym Jeremy Marshall added a comment -

            How come the CHANGE_URL env variable is defined in multi-branch pipeline jobs (PRs) and this is accessible by adding to a publish/post step

            public boolean prepare(final Run<?, ?> run) {
            Map<String, String> env = null;
            try {
                env = getEnvironment(run, new LogTaskListener(LOGGER, Level.INFO));
                env.forEach((k, v) -> log("key: " + k + " value:" + v)); 
                .
                .
                .

            but GIT_URL and GIT_BRANCH is not?

            I'm trying to push from the warnings post step to the PR in Github. So far I can do it from a freestyle job and a PR in a multi branch pipeline but not in a standard pipeline, mainly because I can't derive the github url or github branch

             

            Show
            jeremym Jeremy Marshall added a comment - How come the CHANGE_URL env variable is defined in multi-branch pipeline jobs (PRs) and this is accessible by adding to a publish/post step public boolean prepare( final Run<?, ?> run) { Map< String , String > env = null ; try { env = getEnvironment(run, new LogTaskListener(LOGGER, Level.INFO)); env.forEach((k, v) -> log( "key: " + k + " value:" + v)); . . . but GIT_URL and GIT_BRANCH is not? I'm trying to push from the warnings post step to the PR in Github. So far I can do it from a freestyle job and a PR in a multi branch pipeline but not in a standard pipeline, mainly because I can't derive the github url or github branch  
            Hide
            jglick Jesse Glick added a comment -

            Jeremy Marshall CHANGE_URL is defined in branch-api. GIT_URL etc. are map keys in the return value of git / checkout, done in JENKINS-26100.

            Show
            jglick Jesse Glick added a comment - Jeremy Marshall CHANGE_URL is defined in branch-api . GIT_URL etc. are map keys in the return value of git / checkout , done in JENKINS-26100 .
            Hide
            zastai Tim Van Holder added a comment -

            Everywhere it's "you must implement Step instead".

            However, from the looks of things, implementing Step does not make it available in a Freestyle project.
            It seems completely ridiculous to have to duplicate all the processing (and UI jellies) just to be able to access the context environment in a pipeline.

            It also means any SimpleBuildSteps using the Launcher are potentially broken, as they will not honor any PATH set (whether via tools/environment/withEnv/...).

            My initial thought would be "can't the pipeline set the environment on the Run object for the duration of the step execution?"; but I can see how there might be issues with that (especially with async steps in the mix).

            But is there anything problematic about the following approach?

            1. add a new method to SimpleBuildStep
                default void perform(@Nonnull Run<?,?> run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher,
                               @Nonnull TaskListener listener) throws InterruptedException, IOException {
                  // by default, ignore the environment
                  this.perform(run, workspace, launcher, listener);
                }
                
            2. adjust BuildStepCompatibilityLayer to use it:
                ...
                     if (this instanceof SimpleBuildStep) {
                          // delegate to the overloaded version defined in SimpleBuildStep
                          FilePath workspace = build.getWorkspace();
                          if (workspace == null) {
                              throw new AbortException("no workspace for " + build);
                          }
                          ((SimpleBuildStep) this).perform(build, workspace, build.getEnvironment(), launcher, listener);
                          return true;
                      }
                ...
                
            3. so far, that fully supports existing code, there's just an additional call that has its return value ignored
            4. extend CoreStep's descriptor to require EnvVars.class too, and adjust its Execution to pass it:
                      @Override protected Void run() throws Exception {
                          FilePath workspace = getContext().get(FilePath.class);
                          workspace.mkdirs();
                          delegate.perform(getContext().get(Run.class), workspace, getContext().get(EnvVars.class), getContext().get(Launcher.class), getContext().get(TaskListener.class));
                          return null;
                      }
                

            I mean, it would also be possible to add SimpleBuildStepWithEnvironment, special-case it in BuildStepCompatibilityLayer, and add a CoreStepWithEnvironment that duplicates most of CoreStep.
            That would keep all current code completely unchanged, with plugins able to switch interface to opt-in to receiving the environment too.

            Show
            zastai Tim Van Holder added a comment - Everywhere it's "you must implement Step instead". However, from the looks of things, implementing Step does not make it available in a Freestyle project. It seems completely ridiculous to have to duplicate all the processing (and UI jellies) just to be able to access the context environment in a pipeline. It also means any SimpleBuildSteps using the Launcher are potentially broken, as they will not honor any PATH set (whether via tools/environment/withEnv/...). My initial thought would be "can't the pipeline set the environment on the Run object for the duration of the step execution?"; but I can see how there might be issues with that (especially with async steps in the mix). But is there anything problematic about the following approach? add a new method to SimpleBuildStep default void perform(@Nonnull Run<?,?> run, @Nonnull FilePath workspace, @Nonnull EnvVars env, @Nonnull Launcher launcher, @Nonnull TaskListener listener) throws InterruptedException, IOException { // by default , ignore the environment this .perform(run, workspace, launcher, listener); } adjust BuildStepCompatibilityLayer to use it: ... if ( this instanceof SimpleBuildStep) { // delegate to the overloaded version defined in SimpleBuildStep FilePath workspace = build.getWorkspace(); if (workspace == null ) { throw new AbortException( "no workspace for " + build); } ((SimpleBuildStep) this ).perform(build, workspace, build.getEnvironment(), launcher, listener); return true ; } ... so far, that fully supports existing code, there's just an additional call that has its return value ignored extend CoreStep 's descriptor to require EnvVars.class too, and adjust its Execution to pass it: @Override protected Void run() throws Exception { FilePath workspace = getContext().get(FilePath.class); workspace.mkdirs(); delegate.perform(getContext().get(Run.class), workspace, getContext().get(EnvVars.class), getContext().get(Launcher.class), getContext().get(TaskListener.class)); return null ; } I mean, it would also be possible to add SimpleBuildStepWithEnvironment, special-case it in BuildStepCompatibilityLayer , and add a CoreStepWithEnvironment that duplicates most of CoreStep . That would keep all current code completely unchanged, with plugins able to switch interface to opt-in to receiving the environment too.
            Hide
            jglick Jesse Glick added a comment -

            Tim Van Holder If you have an API proposal you believe would address the gap without breaking backwards compatibility, it is easiest to discuss details in PRs:

            • a PR in jenkins with
              • a patch to SimpleBuildStep and BuldStepCompatibilityLayer
              • a patch to something in test/src/main/java/ proving that a sample step (@TestExtension) can implement the new method and receive actual build environment variables (e.g., values from ParametersAction)
            • a (draft) PR in workflow-basic-steps with jenkins.version set to the incremental version produced by the first PR, with
              • a patch to CoreStep to consume the new API
              • a patch to CoreStepTest using @TestExtension to define a sample step like the one in core, but this time tested in a Pipeline project inside a withEnv block demonstrating access to contextual environment variables
            • optionally also a (draft) PR in some OSS plugin which consumes the new API to improve an existing SimpleBuildStep

            As to the proposed API, a default method in SimpleBuildStep can be made to work but it is awkward since we would prefer to mark the old overload @Deprecated and a new implementation would want to only implement the new overload, so you would have to make the original method also default and do some tricks with Util.isOverridden and AbstractMethodError. We do this kind of dance in various places. Introducing a SimpleBuildStep2 is also a possibility though the constructor of CoreStep would pose a bit of an obstacle.

            Show
            jglick Jesse Glick added a comment - Tim Van Holder If you have an API proposal you believe would address the gap without breaking backwards compatibility, it is easiest to discuss details in PRs: a PR in jenkins with a patch to SimpleBuildStep and BuldStepCompatibilityLayer a patch to something in test/src/main/java/ proving that a sample step ( @TestExtension ) can implement the new method and receive actual build environment variables (e.g., values from ParametersAction ) a (draft) PR in workflow-basic-steps with jenkins.version set to the incremental version produced by the first PR, with a patch to CoreStep to consume the new API a patch to CoreStepTest using @TestExtension to define a sample step like the one in core, but this time tested in a Pipeline project inside a withEnv block demonstrating access to contextual environment variables optionally also a (draft) PR in some OSS plugin which consumes the new API to improve an existing SimpleBuildStep As to the proposed API, a default method in SimpleBuildStep can be made to work but it is awkward since we would prefer to mark the old overload @Deprecated and a new implementation would want to only implement the new overload, so you would have to make the original method also default and do some tricks with Util.isOverridden and AbstractMethodError . We do this kind of dance in various places. Introducing a SimpleBuildStep2 is also a possibility though the constructor of CoreStep would pose a bit of an obstacle.
            Hide
            zastai Tim Van Holder added a comment - - edited

            Part of the trickiness of doing this with a PR is that two repositories are involved.

            But I'm glad to hear that there isn't a more basic problem with changing CoreStep to require envvars while it did not before (are there failure conditions from such changes?)

             

            I think what I'll try is to make a SimpleBuildStepWithEnv and CoreStepWithEnv (names TBD) locally in my plugin. If that works, I'll see what I can do about PRs. I might look at 2 or three alternative approaches and make PRs for each.

            Show
            zastai Tim Van Holder added a comment - - edited Part of the trickiness of doing this with a PR is that two repositories are involved. But I'm glad to hear that there isn't a more basic problem with changing CoreStep to require envvars while it did not before (are there failure conditions from such changes?)   I think what I'll try is to make a SimpleBuildStepWithEnv and CoreStepWithEnv (names TBD) locally in my plugin. If that works, I'll see what I can do about PRs. I might look at 2 or three alternative approaches and make PRs for each.
            Hide
            jglick Jesse Glick added a comment -

            Part of the trickiness of doing this with a PR is that two repositories are involved.

            This is pretty routine actually. Dev guide coming soon in https://github.com/jenkins-infra/jenkins.io/pull/3343 but https://github.com/jenkinsci/incrementals-tools has some more technical docs already.

            there isn't a more basic problem with changing CoreStep to require envvars while it did not before

            Not that I can think of offhand. But that is why you file the PRs—to see if CI says there are test failures you did not anticipate!

            Introducing CoreStepWithEnv is technically possible but sounds like a mess from a user perspective, and might not interoperate with some plugins which analyze the build graph. I would definitely recommend some approach that involves updating the existing CoreStep.

            That said, there is absolutely nothing wrong with filing alternative PRs and letting reviewers compare the impact, so long as you make the situation very clear in the PR description. (Remember to refer to one PR from another by pasting in a naked URL, not a Markdown link, so that GitHub displays a hover tip and more importantly creates an automatic backlink.) Also recommended:

            • assign Jira to yourself, mark In Progress while working and In Review when you believe you have something ready to merge (not counting draft status due to an incremental version in a dependency—up to repository maintainers to deal with that)
            • link to Jira from each PR (we do not currently have a bot to do this, shame on us)
            • link to PRs from Jira (ditto)
            Show
            jglick Jesse Glick added a comment - Part of the trickiness of doing this with a PR is that two repositories are involved. This is pretty routine actually. Dev guide coming soon in https://github.com/jenkins-infra/jenkins.io/pull/3343 but https://github.com/jenkinsci/incrementals-tools has some more technical docs already. there isn't a more basic problem with changing CoreStep to require envvars while it did not before Not that I can think of offhand. But that is why you file the PRs—to see if CI says there are test failures you did not anticipate! Introducing CoreStepWithEnv is technically possible but sounds like a mess from a user perspective, and might not interoperate with some plugins which analyze the build graph. I would definitely recommend some approach that involves updating the existing CoreStep . That said, there is absolutely nothing wrong with filing alternative PRs and letting reviewers compare the impact, so long as you make the situation very clear in the PR description. (Remember to refer to one PR from another by pasting in a naked URL, not a Markdown link, so that GitHub displays a hover tip and more importantly creates an automatic backlink.) Also recommended: assign Jira to yourself, mark In Progress while working and In Review when you believe you have something ready to merge (not counting draft status due to an incremental version in a dependency—up to repository maintainers to deal with that) link to Jira from each PR (we do not currently have a bot to do this, shame on us) link to PRs from Jira (ditto)
            zastai Tim Van Holder made changes -
            Assignee Tim Van Holder [ zastai ]
            Hide
            zastai Tim Van Holder added a comment -

            Turns out that for my immediate purpose in my plugin, it was actually very easy to add support for getting environment variables by implementing a metastep (all my builders share a base class that implements the perform() anyway, so no SimpleBuildStep needed at all).

            The trick is to know that metasteps exist, which I didn't until I read the javadoc for isMetaStep() while going over what CoreStep does.
             

            Anyway, I've assigned this to myself and will look at setting up clones of jenkins and workflow-basic-steps-plugin to see what I can make work without breaking tests.

            And thanks for the pointer to the incrementals; that's useful info.

            Show
            zastai Tim Van Holder added a comment - Turns out that for my immediate purpose in my plugin, it was actually very easy to add support for getting environment variables by implementing a metastep (all my builders share a base class that implements the perform() anyway, so no SimpleBuildStep needed at all). The trick is to know that metasteps exist, which I didn't until I read the javadoc for isMetaStep() while going over what CoreStep does.   Anyway, I've assigned this to myself and will look at setting up clones of jenkins and workflow-basic-steps-plugin to see what I can make work without breaking tests. And thanks for the pointer to the incrementals; that's useful info.
            Hide
            jglick Jesse Glick added a comment -

            The metastep system was not intended for use by domain-specific plugins, i.e. for anything other than core, wrap, and in the future perhaps checkout.

            Show
            jglick Jesse Glick added a comment - The metastep system was not intended for use by domain-specific plugins, i.e. for anything other than core , wrap , and in the future perhaps checkout .
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-35671 [ JENKINS-35671 ]
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-46175 [ JENKINS-46175 ]
            zastai Tim Van Holder made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            zastai Tim Van Holder made changes -
            Remote Link This issue links to "Jenkins PR 4766 (Web Link)" [ 24959 ]
            Hide
            zastai Tim Van Holder added a comment -

            Add link to draft plugin PR.

            Show
            zastai Tim Van Holder added a comment - Add link to draft plugin PR.
            zastai Tim Van Holder made changes -
            Remote Link This issue links to "workflow-basic-steps draft PR 116 (Web Link)" [ 25003 ]
            jglick Jesse Glick made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]
            oleg_nenashev Oleg Nenashev made changes -
            Status In Review [ 10005 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            Released As Jenkins 2.241
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            The Jenkins core patch was released in Jenkins 2.241

            Show
            oleg_nenashev Oleg Nenashev added a comment - The Jenkins core patch was released in Jenkins 2.241
            Hide
            zastai Tim Van Holder added a comment -

            Technically this issue won't be solved until workflow-basic-steps is updated too.

            Show
            zastai Tim Van Holder added a comment - Technically this issue won't be solved until workflow-basic-steps is updated too.
            jglick Jesse Glick made changes -
            Link This issue causes JENKINS-62723 [ JENKINS-62723 ]
            jglick Jesse Glick made changes -
            Link This issue causes JENKINS-63754 [ JENKINS-63754 ]

              People

              • Assignee:
                zastai Tim Van Holder
                Reporter:
                jglick Jesse Glick
              • Votes:
                3 Vote for this issue
                Watchers:
                14 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: