Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: aggregate JSON output parameters correctly #13513

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Aug 27, 2024

Motivation

Fixes #13510

Modifications

outputs.result of a withItems/withParams will be conditionally json.Marshaled.

Other output parameters are always json.Marshalled, which leads to inconsitency and a complete inability to pass JSON out of withItems outputs straight back in as withItems

Conditionally Marshal the outputs.params

  • avoiding it if the parameter isn't enclosed in { or [, primarily to avoid bare numbers as they're valid JSON but need quoting here
  • falling back to old behaviour if this fails

Verification

Additional e2e test tests the example case.

@Joibel Joibel added the area/looping `withParams`, `withItems`, and `withSequence` label Aug 27, 2024
@Joibel Joibel marked this pull request as ready for review August 28, 2024 07:33
Joibel added 2 commits August 30, 2024 11:29
Fixes argoproj#13510

`outputs.result` of a `withItems`/`withParams` will be conditionally
`json.Unmarshal`ed.

Other output parameters are always json.Unmarshalled, which leads to
inconsitency and a complete inability to pass JSON out of withItems
outputs straight back in as `withItems`

Conditionally unmarshal the outputs.params
* avoiding it if the parameter isn't enclosed in `{` or `[`, primarily
to avoid bare numbers as they're valid JSON but need quoting here
* falling back to old behaviour if this fails

Additional e2e test tests the example case.

Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
@Joibel Joibel force-pushed the aggregate-json-output-params branch from 7a93ed3 to 11282dd Compare August 30, 2024 10:29
@agilgur5
Copy link

@Joibel is this at all impacted by #12909? Since that creates a diff between main and 3.5 with JSON in templates

Briefly looking at the code, I don't think it does since it only impacts output parameters, but wanted to double check, especially since #12909 (comment) is still incomplete

@Joibel
Copy link
Member Author

Joibel commented Aug 30, 2024

No, this isn't overlapping, and wouldn't be if #12909 had the remaining changes which are mostly around docs.

#13532 also doesn't appear to overlap, but does seem to have a similar cause. I haven't had a chance to see what's happening there though.

@agilgur5
Copy link

agilgur5 commented Aug 30, 2024

No, this isn't overlapping

Thanks just wanted to make sure!

and wouldn't be if #12909 had the remaining changes which are mostly around docs.

There were some edge cases there that hadn't been tested (that I had asked tests to be added for), and if those edge cases are impacted, we might want to change the logic. A variable already being JSON or already converted to JSON was one of those, hence my concern here

Do you want me to write up an issue to handle the remaining changes to track? I was going to do so if it hadn't been handled soon so that we don't forget about it

@agilgur5 agilgur5 added this to the v3.5.x patches milestone Aug 30, 2024
@agilgur5
Copy link

Do you want me to write up an issue to handle the remaining changes to track? I was going to do so if it hadn't been handled soon so that we don't forget about it

Went ahead and created an issue for it: #13541

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Joibel Joibel merged commit c29c630 into argoproj:main Sep 3, 2024
28 checks passed
@Joibel Joibel deleted the aggregate-json-output-params branch September 3, 2024 14:04
Joibel added a commit to pipekit/argo-workflows that referenced this pull request Sep 19, 2024
Joibel added a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/looping `withParams`, `withItems`, and `withSequence`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON script result vs JSON output parameters are treated differently for withParam item aggregation
3 participants