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

Add loops walkthrough (with_item/with_param, fanout/fanin) #660

Merged
merged 5 commits into from
Jun 1, 2023

Conversation

elliotgunton
Copy link
Collaborator

Pull Request Checklist

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
@elliotgunton elliotgunton changed the title Walkthrough/loops Add loops walkthrough (with_item/with_param, fanout/fanin) May 31, 2023
@elliotgunton elliotgunton added semver:patch A change requiring a patch version bump type:documentation A documentation update labels May 31, 2023
Copy link
Collaborator

@flaviuvadan flaviuvadan left a comment

Choose a reason for hiding this comment

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

I think things like "{{item.name}}" are set automatically for Task arguments. Should we expose that to Step as well?

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #660 (00dc608) into main (7a584be) will decrease coverage by 0.3%.
The diff coverage is n/a.

❗ Current head 00dc608 differs from pull request most recent head 85ddd42. Consider uploading reports for the commit 85ddd42 to get more accurate results

@@           Coverage Diff           @@
##            main    #660     +/-   ##
=======================================
- Coverage   74.6%   74.3%   -0.3%     
=======================================
  Files         43      43             
  Lines       3017    3017             
  Branches     575     575             
=======================================
- Hits        2252    2244      -8     
- Misses       591     602     +11     
+ Partials     174     171      -3     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@flaviuvadan
Copy link
Collaborator

flaviuvadan commented May 31, 2023

I think things like "{{item.name}}" are set automatically for Task arguments. Should we expose that to Step as well?

@elliotgunton can confirm these are set automatically for tasks based on arguments inference. So, Task invocation does not need the arguments field when with_param is passed, or with_item, but Step does for now. We should add the same logic to Step as well: https://github.com/argoproj-labs/hera/blob/main/src/hera/workflows/_mixins.py#L469 Motivation here is to not confuse users with these differences of "sometimes I can set a thing on Task but not on Step"

@elliotgunton
Copy link
Collaborator Author

We should add the same logic to Step as well

@flaviuvadan 100% agree - we made this change in the source-kwargs branch here https://github.com/argoproj-labs/hera/blob/d0f117f82616c00592346932fcfcc7c9e0d57677/src/hera/workflows/_mixins.py#LL468C27-L468C27, think it's fine to move it out of the Task try branch in the same way

elliotgunton added a commit that referenced this pull request Jun 1, 2023
**Description of PR**
_Copied from
[comment](#660 (comment)
>>I think things like "{{item.name}}" are set automatically for Task
arguments. Should we expose that to Step as well?

>@elliotgunton can confirm these are set automatically for tasks based
on arguments inference. So, Task invocation does not need the arguments
field when with_param is passed, or with_item, but Step does for now. We
should add the same logic to Step as well:
https://github.com/argoproj-labs/hera/blob/main/src/hera/workflows/_mixins.py#L469
Motivation here is to not confuse users with these differences of
"sometimes I can set a thing on Task but not on Step"

---------

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
@elliotgunton elliotgunton enabled auto-merge (squash) June 1, 2023 08:20
@elliotgunton elliotgunton merged commit 0f9cda6 into main Jun 1, 2023
@elliotgunton elliotgunton deleted the walkthrough/loops branch June 1, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:documentation A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants