[JENKINS-46124] - support map with EnvStep by jetersen · Pull Request #105 · jenkinsci/workflow-basic-steps-plugin

jetersen · 2020-01-27T20:28:40Z

src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java

MindaugasLaganeckas

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

MindaugasLaganeckas · 2020-01-29T08:11:42Z

src/main/java/org/jenkinsci/plugins/workflow/steps/EnvStep.java

+        @Override
+        public Map<String, Object> customInstantiate(@NonNull Map<String, Object> arguments) {
+            Map<String, Object> r = new HashMap<>(arguments);
+            Object overrides = r.get("overrides");


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 · 2020-02-03T20:35:31Z

src/main/java/org/jenkinsci/plugins/workflow/steps/EnvStep.java

                    }
                }
                return b.toString();
+            } else if (overrides instanceof Map) {


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.

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

Not easily. Just noting some background.

src/main/java/org/jenkinsci/plugins/workflow/steps/EnvStep.java

src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java

src/main/java/org/jenkinsci/plugins/workflow/steps/EnvStep.java

jetersen · 2020-02-03T22:16:39Z

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 🤯

src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java

jglick · 2020-02-18T22:35:40Z

src/main/java/org/jenkinsci/plugins/workflow/steps/EnvStep.java

-                .collect(Collectors.toMap(e -> (String)e.getKey(), e -> e.getValue() == null ? "" : e.getValue().toString()))
-                .entrySet().stream()
-                .map(m -> m.getKey() + "=" + m.getValue())
+                .map(m -> m.getKey() + "=" + (m.getValue() == null ? "" : m.getValue().toString()))


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

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

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']) {}

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']) {

Maybe

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

then, whatever the syntax is. At worst

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

I'd argue your asking for trouble

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

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

Then you get into java.lang.ClassCastException

Exactly what I was requesting with 8d385a2.

src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java

jetersen · 2020-02-27T06:34:40Z

@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 · 2020-03-02T18:01:31Z

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

mss · 2024-09-03T10:35:51Z

@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?

support map with EnvStep

21ab92d

jetersen commented Jan 27, 2020

View reviewed changes

src/test/java/org/jenkinsci/plugins/workflow/steps/EnvStepTest.java Outdated Show resolved Hide resolved

jetersen requested review from dwnusbaum and jglick January 27, 2020 20:29

jetersen added 2 commits January 27, 2020 22:03

map arguments to string

898b05e

call toString on value

b89ccbc

MindaugasLaganeckas approved these changes Jan 29, 2020

View reviewed changes

DRY

34178ae

jglick reviewed Feb 3, 2020

View reviewed changes

jetersen added 2 commits February 3, 2020 23:10

simplify map tests and conversion based on review

2b151a0

add missing env from assertion

1aa3c3f

jglick self-requested a review February 5, 2020 02:01

jglick approved these changes Feb 18, 2020

View reviewed changes

jetersen added 3 commits February 19, 2020 06:05

check null values

20d8cf7

a comma too much

4f52a0f

Cast key to String

8d385a2

dwnusbaum requested changes Feb 27, 2020

View reviewed changes

stuartrowe mentioned this pull request Jun 15, 2025

[JENKINS-46124] - support map with EnvStep #348

Open

5 tasks

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

Uh oh!

Conversation

jetersen commented Jan 27, 2020

Uh oh!

Uh oh!

MindaugasLaganeckas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jetersen commented Feb 3, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jetersen commented Feb 27, 2020

Uh oh!

dwnusbaum left a comment

Choose a reason for hiding this comment

Uh oh!

jetersen commented Mar 2, 2020

Uh oh!

mss commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants