JENKINS-29144: Enable proper environment access for build steps #4766
Conversation
Provides a default implementation for both overloads:
- if the old method is called, it forwards to the new method, passing
the environment specified for the Run
- this means that when a new implementer (who has overridden the new
overload) gets called by an old client, they get the run
environment only (fine for freestyle, only the initial environment
in a pipeline)
- if the new method is called, it forwards to the old method, ignoring
the passed environment
- in this case, the old method MUST be overridden
- this handles the case of an old implementer getting called by a
new client
- no change in functionality; implementer still gets the same
environment to work with (via the Run)
BuildStepCompatibilityLayer has _not_ been changed yet to call the new
overload, mainly because it would just get the environment to pass the
same way the default implementation in SimpleBuildStep does (i.e.
passing the result of calling getEnvironment() on the Run).
Implementers of SimpleBuildStep (like ArtifactArchiver and
Fingerprinter) similarly have _not_ been changed. This is mainly
because they get Run's environment to apply substitution, and that
should not be done using an environment passed in from a pipeline
step. So if they switch to the new signature, their code should also
be adjusted to only expand environment variables when the Run is an
AbstractBuild.
With this change, a PR can be made to workflow-basic-steps-plugin,
to extend CoreStep to pass in the environment variables from the
step context. The combination of both PRs should then fix
JENKINS-29144 and allow the creation of Builders that are
automatically available as pipeline steps with proper honoring of
things like withEnv and global tools.
This currently just checks for a slave-level environment variable; not sure if that is enough.
There was a problem hiding this comment.
I did NOT make a change to
BuildStepCompatibilityLayeror any other users ofSimpleBuildStep.
This is mainly because they would then need to be made more step-aware (specifically: no expansions should be done when the environment comes from a pipeline step).
Not sure I follow. All you need to do is patch BuildStepCompatibilityLayer to call the nondeprecated overload, passing in Run.getEnvironment(TaskListener)---same behavior as now, just being explicit and avoiding an internal use of a deprecated API. This will only be used for traditional job types. CoreStep would directly call the new overload using its contextual EnvVars.
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
|
If we are patching |
…new SimpleBuildStep API Both will only use the environment for substitution when the Run being passed is an AbstractBuild, following the guidelines that for pipelines, the user does their own substitution in Groovy. This causes a small behaviour change; before, even in pipelines, substitution would be done by these steps using the (tiny) initial environment.
I was more talking about Fingerprinter & co. For BSCL it is indeed a small patch, but given it would just be doing exactly what the default does, I left it out. But I've now pushed a commit that adds that change. |
Was just suggesting this on the principle that when you deprecate a method you should attempt to also replace all implementations and call sites within the same source tree, so you do not leave deprecation warnings around. |
Hmm. What would be best - adding overloads without the FilePath? Adding a needsWorkspace() and/or needsEnvVars() to the Descriptor is great, but that would be tricky to check from the interface, for example. |
|
I was thinking to make the I guess this could be done at another time. I am just trying to think through whether we would need to change the Java binary signature of |
jglick
added
developer
That's so weird - in my plugin I got deprecation warnings for them. |
|
Changing the signature of the old API from NonNull to Nullable/CheckForNull feels like a breaking change. |
Because you are using an older |
Changing from |
|
|
I've left out the nullability changes for now. Mainly because the two steps in core both need the workspace, so I would have to also add the descriptor portion. Because SimpleBuildStep is just an interface, there is no SimpleBuildStepDescriptor in the hierarchy to hang anything on. And I'm not sure that adding a SimpleBuildStepDescriptor interface would be ideal. Feels like a separate PR, albeit one I'd be willing to look at after this one. |
|
Approved but waiting for the downstream in workflow-basic-steps-api. |
As I understand it, to create the downstream PR, I need to set its pom.xml to refer to the incremental produced by this PR. I can get around that locally, but without the incremental deployed to the repository, any PR I make would fail to build, would it not? |
jglick
added
the
on-hold
There was a problem hiding this comment.
Looks good to me. Indeed a reference implementation in a plugin would be nice, but IMHO internal implementations already justify the change. E.g. Archiver's support for variable expansion can be considered a minor improvement on its own.
BTW, is there a risk that the introduced variable expansion leads to regressions when variable macros are present in the strings but not supposed to be resolved? Not a blocker even in this case IMHO
Actually, ArtifactArchiver and Fingerprinter have almost no functional change with this PR. Essentially it causes them to stop expanding the (very small set of) initial environment variables when used in pipelines. In freestyle jobs they retain the full expansion they already had. But to close the actual ticket, the change to CoreStep in workflow-basic-steps-plugin is required. Because it will be CoreStep that actually passes the EnvVars in when the SimpleBuildStep is used in a pipeline. |
Builds on jenkinsci/jenkins#4766 (incremental version 2.240-rc30032.5a9c198e2a33). Note: I'm getting test failures from CatchErrorStepTest.optionalMessage(), but that doesn't involve CoreStep, so is probably caused by something else in current Jenkins master.
jglick
removed
the
on-hold
oleg-nenashev
added
the
ready-for-merge
It would be great to document it in the changelog Would be great to get confirmation from @dwnusbaum
Do we also need to release workflow-basic-steps-plugin before this change lands in Weekly? |
|
Changelog entry added. |
If a change is done in a compatible way, we can. If I understand the required fix correctly, |
|
Well no, I don't think so - |
The behavioral change to
Currently it actually is possible to release the plugin with an incremental core dep (pending a new Enforcer rule), but anyway I think there is no rush—the plugin release is only needed when someone first writes and publishes a |
|
Based on the feedback, we may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process |
|
jenkinsci/workflow-basic-steps-plugin#116 seems to be the downstream PR. This should have been in the PR comment. Per PR template:
|
timja
added
the
squash-merge-me
You're right - at the time of submission that did not exist yet, but I did indeed fail to go back and add it. |
| // If this is called, this must be an implementer of the previous API, in which case we call that, discarding | ||
| // the environment we were given. | ||
| // But for that to work, that API method must have been implemented. | ||
| if (Util.isOverridden(SimpleBuildStep.class, this.getClass(), |
There was a problem hiding this comment.
@jglick @oleg-nenashev this requires perform method not to be final and breaks backward compatibility with uber-archive/phabricator-jenkins-plugin#344
There was a problem hiding this comment.
@Zastai certainly not intentional. JENKINS-62723
See JENKINS-29144.
This extends
SimpleBuildStepwith an overload ofperform()that takes anEnvVars.Uses default implementation of both (as is done in other places) to deal with both old-implementer-called-by-new-client and new-implemented-called-by-old-client.
This change on its own does very little, by design.
The main fix for the ticket would come from a corresponding change in
CoreStep, in theworkflow-basic-steps-plugin, to make that pass theEnvVarsfrom the step context.In particular, I did NOT make a change to
BuildStepCompatibilityLayeror any other users ofSimpleBuildStep.This is mainly because they would then need to be made more step-aware (specifically: no expansions should be done when the environment comes from a pipeline step).
If desired, I could add such changes to this PR, but leaving things as they are preserves the current behaviour of those steps just fine.
I added a small test case, to check whether a step implementing the new API receives the expected environment; but that isn't much of a test (in a FreeStyle build it would have had access to it via the
Runobject already). The real test is in the pipeline context.Downstream PR jenkinsci/workflow-basic-steps-plugin#116
Proposed changelog entries
fingerprintandarchiveArtifactspipeline steps will no longer apply any environment substitution.Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entriessection only if there are breaking changes or other changes which may require extra steps from users during the upgrade Shouldn't that be theProposed upgrade guidelinessection?Desired reviewers
@jglick
Maintainer checklist
Before the changes are marked as
ready-for-merge:Proposed changelog entriesare correctupgrade-guide-neededlabel is set and there is aProposed upgrade guidelinessection in the PR title. (example)lts-candidateto be considered (see query).