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.

Sign up for GitHub

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

Jump to bottom

fix: filter hook node to find the correct lastNode. Fixes: #12109 #12815

Merged
merged 10 commits into from
Mar 30, 2024
Merged

fix: filter hook node to find the correct lastNode. Fixes: #12109 #12815

merged 10 commits into from
Mar 30, 2024

Conversation

shuangkun
Copy link
Member

@shuangkun shuangkun commented Mar 18, 2024 •
edited

Let retryStrategy AND hooks on a step works well.

Fixes #12109

Motivation

Modifications

Filter hook node to find the correct lastNode when buildLocalScope.
So the error

unable to resolve references: Unable to resolve: "steps.build.outputs.artifacts.result"

won't occur.

Verification

local test and e2e.

@shuangkun
fix: filter hook node to find the correct lastNode. Fixes: argoproj#1…
e6edd28 
…2109

Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun marked this pull request as draft March 18, 2024 08:53
@shuangkun
fix: lint
5262d9c 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun added area/hooks area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries labels Mar 18, 2024
@shuangkun shuangkun closed this Mar 18, 2024
@shuangkun shuangkun reopened this Mar 18, 2024
@shuangkun shuangkun marked this pull request as ready for review March 18, 2024 10:22
tczhao
tczhao requested changes Mar 19, 2024
View reviewed changes
Copy link
Member

@tczhao tczhao 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 the operator changes look good but there are issues with the added test.

test/e2e/hooks_test.go Outdated Show resolved Hide resolved
@tczhao tczhao self-assigned this Mar 19, 2024
@shuangkun
fix: test
3a0dbba 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun
fix: add cli test
671649a 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun
fix: test
3b54927 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun
fix: test
72635e0 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun
fix: test
9b20040 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun
fix: lint
c788103 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun requested a review from tczhao March 19, 2024 14:35
tczhao
tczhao requested changes Mar 20, 2024
View reviewed changes
test/e2e/cli_test.go Outdated Show resolved Hide resolved
test/e2e/cli_test.go Outdated Show resolved Hide resolved
test/e2e/cli_test.go Outdated Show resolved Hide resolved
@shuangkun
fix: test
823dec1 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun requested a review from tczhao March 20, 2024 04:21
tczhao
tczhao approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

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

LGTM

@shuangkun shuangkun added the prioritized-review For members of the Sustainability Effort label Mar 27, 2024
@juliev0
Copy link
Contributor

juliev0 commented Mar 29, 2024

I see possiblyGetRetryChildNode() gets called in 3 places. Want to make sure your change should apply to all 3, right?

@shuangkun
fix: add comments
587d338 
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun requested a review from juliev0 March 30, 2024 00:33
@shuangkun
Copy link
Member Author

I see possiblyGetRetryChildNode() gets called in 3 places. Want to make sure your change should apply to all 3, right?

Yes, all places need to find correct last child node.

@juliev0 juliev0 merged commit 54106f7 into argoproj:main Mar 30, 2024
27 checks passed
@shuangkun shuangkun mentioned this pull request Apr 4, 2024
Closed
6 tasks
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 19, 2024
agilgur5 pushed a commit that referenced this pull request Apr 19, 2024
@shuangkun @agilgur5
fix: filter hook node to find the correct lastNode. Fixes: #12109 (#1…
3d06488 
…2815)

Signed-off-by: shuangkun <tsk2013uestc@163.com>
(cherry picked from commit 54106f7)
@agilgur5
Copy link
Member

Backported cleanly to release-3.5 as 3d06488

@agilgur5 agilgur5 added area/retryStrategy Template-level retryStrategy and removed area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries labels Apr 25, 2024
@tczhao tczhao mentioned this pull request May 1, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliev0 juliev0 juliev0 approved these changes

@tczhao tczhao tczhao approved these changes

Assignees

@tczhao tczhao

Labels
area/hooks area/retryStrategy Template-level retryStrategy prioritized-review For members of the Sustainability Effort
Projects
None yet
Milestone
v3.5.x patches
Development

Successfully merging this pull request may close these issues.

Can't use retryStrategy AND hooks on a step that outputs an artifact with downstream consumers
4 participants
@shuangkun @juliev0 @agilgur5 @tczhao