[JENKINS-29144] Extend CoreStep to also pass EnvVars to SimpleBuildStep.

[Zastai]

See JENKINS-29144

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

[Zastai]

And a spotbugs error in code I have not touched. Yay. Should I fix these as part of this PR?

[Zastai]

The remaining issue now, a test failure in CatchErrorStepTest.optionalMessage. That seems to test whether catchError('foo') works too (as opposed to mentioning the field name catchError(message: 'foo')).
But given that CatchErrorStep has multiple String-typed fields (the others being buildResult and stageResult), I'm not sure why that's expected to work.
As such, I don't know what to do to make that test work.

[dwnusbaum]

Thanks for the PR! The main idea looks good to me, but I added some comments about the unrelated issues you ran into because of the core upgrade.

[Zastai]

So what happens next? Wait for jenkins/PR4766 to be merged, then when the weekly is released, reference that and convert this to non-draft?

[dwnusbaum]

Wait for jenkins/PR4766 to be merged, then when the weekly is released, reference that and convert this to non-draft?

Yes, more or less. One thing though is that this plugin is used widely enough that I do not want it to depend on such a new version of Jenkins core, instead I would prefer to use reflection to call the new overload of perform only when it is available and otherwise call the existing overload. You can drop the @Override annotation in the test so that things still compile against an older version of Jenkins core and I think you can use Jenkins.getVersion to change the assertions depending on what version of Jenkins is being tested against, and I can push a commit to your branch to update the Jenkinsfile to build the plugin against both the current baseline and the weekly release with your change to test the new behavior.

[Zastai]

That's a bit messy, and it'll get messier when adding JENKINS-46175.

I thought to have read something about plugin versioning supporting "branches", so 2.22 and onwards would reference jenkins 2.240 (or whatever the weekly turns out to be), but a later 2.25.1 might reference 2.163 (for a fix for a bug reported for 2.21 or earlier that does not depend on new API), with the update center offering the 2.21 -> 2.25.1 upgrade to users running older Jenkins releases.
But I may have misunderstood.

[jglick]

I thought to have read something about plugin versioning supporting "branches"

Yes, my preference (FWIW) is to keep things simple, declaring whatever Jenkins version you want, and using something like https://gist.github.com/jglick/86a30894446ed38f918050c1180483e2 to create backport branches on demand (typically just for significant bug fixes). But the decision rests on whomever is actually merging PRs and cutting releases, probably @dwnusbaum in this case.

[Zastai]

Now that the core part of JENKINS-21944 is merged, I can start JENKINS-46175.
Given they affect the same code area here, I wonder whether it would make sense to use this PR to apply both sets of changes here, especially if I'd have to go around via reflection to access the new APIs.

[Zastai]

Decided to keep this separate despite the overlap with #117

[Zastai]

Switched to the released 2.241.

Then did as requested and dropped the requirement back to 2.164.3, invoking the new API via reflection when running on 2.241 or later.

Would just need that Jenkinsfile change to build against old & new.

[Zastai]

The build also seems to have failed for no obvious reason.

[Zastai]

Closing - superseded by #121

[jglick]

Note that you do not need to close this: if #121 is merged (without squash or rebase!), this will automatically be marked merged as well.

[Zastai]

I did not know that. No harm done I guess.