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

feat(controller): Support per-output parameter aggregation. Fixes #2393 #4374

Merged

Conversation

Ark-kun
Copy link
Member

@Ark-kun Ark-kun commented Oct 26, 2020

Currently Argo only supports the {{tasks.<TASKNAME>.outputs.parameters.<NAME>}} placeholder for the outputs of normal nodes. (For the outputs of the loop nodes there is the {{tasks.<TASKNAME>.outputs.parameters}} placeholder, which is replaced with JSON-serialized list of parameter value maps.

This PR adds support for the {{tasks.<TASKNAME>.outputs.parameters.<NAME>}} placeholder for the outputs of the loop nodes. The placeholder is replaced with JSON-serialized list of parameter values.

Fixes #2393

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • My organization is added to USERS.md.
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.

@Ark-kun Ark-kun force-pushed the Support-per-output-parameter-aggregation branch 2 times, most recently from fd0551e to e7fd44c Compare October 26, 2020 09:23
@Ark-kun Ark-kun changed the title feat: Support per-output parameter aggregation feat: Support per-output parameter aggregation. Fixes #2393 Oct 26, 2020
@simster7 simster7 self-assigned this Oct 26, 2020
@Ark-kun Ark-kun force-pushed the Support-per-output-parameter-aggregation branch from e7fd44c to 055058f Compare November 1, 2020 00:39
@simster7
Copy link
Member

@Ark-kun please mark this as "Ready for Review" when it's so and I'll take a look

@alexec
Copy link
Contributor

alexec commented Dec 1, 2020

@Ark-kun we're going to review some unfinished PRs to determine if we should finish them. Do you plan to continue on this, please?

@alexec alexec changed the title feat: Support per-output parameter aggregation. Fixes #2393 feat(controller): Support per-output parameter aggregation. Fixes #2393 Dec 1, 2020
@Ark-kun Ark-kun force-pushed the Support-per-output-parameter-aggregation branch 4 times, most recently from 38638d2 to f1bef2d Compare December 2, 2020 08:16
@Ark-kun Ark-kun marked this pull request as ready for review December 3, 2020 05:33
Currently Argo only supports the `{{tasks.<TASKNAME>.outputs.parameters.<NAME>}}` placeholder for the outputs of normal nodes. (For the outputs of the loop nodes there is the `{{tasks.<TASKNAME>.outputs.parameters}}` placeholder, which is replaced with JSON-serialized list of parameter value maps.

This PR adds support for the `{{tasks.<TASKNAME>.outputs.parameters.<NAME>}}` placeholder for the outputs of the loop nodes. The placeholder is replaced with JSON-serialized list of parameter values.

Signed-off-by: Alexey Volkov <alexey.volkov@ark-kun.com>
@Ark-kun Ark-kun force-pushed the Support-per-output-parameter-aggregation branch from f1bef2d to 252257a Compare December 3, 2020 05:34
@Ark-kun
Copy link
Member Author

Ark-kun commented Dec 3, 2020

@Ark-kun we're going to review some unfinished PRs to determine if we should finish them. Do you plan to continue on this, please?

@alexec
Hello. I think this PR should be ready.

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

This LGTM. Can we add a test please?

@Ark-kun
Copy link
Member Author

Ark-kun commented Dec 8, 2020

Can we add a test please?

This would be great, but I have not found the tests for the main feature (parameter aggregation), so it looks like this will require non-trivial amount of work.

Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

I'll add tests for both in a separate PR

@simster7 simster7 merged commit e8cc2fb into argoproj:master Dec 9, 2020
@fvdnabee
Copy link
Contributor

Will we see this PR released in argo v2.13?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make aggregated output parameters available by name
4 participants