[JENKINS-46124] - support map with EnvStep

[jetersen]

fixes JENKINS-46124

[MindaugasLaganeckas]

A nice feature and a nice set of tests! :)

[MindaugasLaganeckas]

DRY for "overrides" :)

[jglick]

Do you want to implement customUninstantiate to guide users of Pipeline Syntax to use the new form, or leave them with the List<String> syntax?

[jglick]

BTW I lobbied for ArgumentsAction[Impl] to persist only the internal, resolved form of arguments, but the design we went with persists the surface form, necessitating extra work like this.

[bitwiseman]

@jglick Okay, is there some action to be taken about that now?

[jglick]

Not easily. Just noting some background.

[jetersen]

please take another look @jglick I think I listen to your feedback, I honestly don't know what I was thinking about when I wrote that stream collector 🤯

[jglick]

Suggested change

	.map(m -> m.getKey() + "=" + (m.getValue() == null ? "" : m.getValue().toString()))
	.map(m -> (String) m.getKey() + "=" + (m.getValue() == null ? "" : m.getValue().toString()))

since non-String keys would be bogus

[jetersen]

IDE consider this a redundant cast and wants to remove it.

[jglick]

Then your IDE is wrong and you should ignore it. You should be able to have a test demonstrating a failure from some bogus thing like

withEnv([new Object(): 'x']) {}

[jetersen]

This case is not supported:

WorkflowScript: 2: illegal colon after argument expression;
  solution: a complex label expression before a colon must be parenthesized @ line 2, column 72.
  D: true, E: null, new Object(): 'x']) {

[jglick]

Maybe

withEnv([(new Object()): 'x']) {}

then, whatever the syntax is. At worst

def m = [:]
m.put(new Object(), 'x')
withEnv(m) {}

[jetersen]

I'd argue your asking for trouble

[jglick]

OK forget new Object(), just some arbitrary object key which is not a String. new URL('http://nowhere.net/') for example is whitelisted.

[jetersen]

Than you get into java.lang.ClassCastException: java.net.URL cannot be cast to java.lang.String 🤔

Properly better to call toString instead of trying to cast the object

[jglick]

Then you get into java.lang.ClassCastException

Exactly what I was requesting with 8d385a2.

[jetersen]

[jetersen]

@bitwiseman @jglick 🙏 Would be a great addition to get merged.

[dwnusbaum]

Thanks for the PR! The implementation looks good to me (thanks for adding tests!). I am a little hesitant to add complexity since we will have to support both syntaxes going forward, but I think the trade-off is worth it in this case.

Can you please update EnvStep/help-overrides.html (and maybe also EnvStep/help.html) to mention the new syntax and give an example? Once that is done I am happy to merge the PR.

[jetersen]

I'll see what I can do :)
However somewhat busy in this week. Starting at a new workplace 😁

[mss]

@dwnusbaum I was looking for this exact feature for years and finally found this PR but it got stalled. I'd be happy to provide the requested documentation but since this code does not apply in the meantime I wondered who would have to adapt it to the changed codebase?