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: cli allow retry successful workflow if nodeFieldSelector is set. Fixes #11020) #11409

Merged

Conversation

or-shachar
Copy link
Contributor

@or-shachar or-shachar commented Jul 20, 2023

Fixes #11020

Motivation

As described in #11020 - I want to be able to retry successful workflow, given I specify explicitly retrySuccessful and node selector.

Modifications

I added a condition to only fail the FormulateRetryWorkflow method if the original workflow is not Error/Failed AND either restartSuccessful==False or nodeFieldSelector is empty.

Verification

I verified it with local deployment + added tests for both cases (retry should fail for a successful run without those extra flags, retry should work for a successful workflow with those flags).

Notes

  • I'm not completely sure about the error message ("To retry either: (1) workflow must be Failed/Error (2) Provide also restartSuccessful and nodeFieldSelector value"). Too long? understandable? changed it according to @terrytangyuan 's advise 🙏
  • I think that to complement this feature - we should also re-enable the "Retry" button from the UI and require the user to select which node to retry. I don't think this is a stopper for this PR as allowing to do this from CLI is already super helpful.

@or-shachar or-shachar force-pushed the 11020-retry-successful-workflows-cli branch 8 times, most recently from 3d27393 to 9f9732e Compare July 21, 2023 09:41
@or-shachar or-shachar marked this pull request as ready for review July 21, 2023 10:13
@or-shachar or-shachar force-pushed the 11020-retry-successful-workflows-cli branch from 9f9732e to 9c8f14c Compare July 26, 2023 13:59
workflow/util/util.go Outdated Show resolved Hide resolved
workflow/util/util.go Show resolved Hide resolved
@or-shachar or-shachar force-pushed the 11020-retry-successful-workflows-cli branch 2 times, most recently from 8066658 to 897fcc7 Compare July 26, 2023 15:38
@or-shachar or-shachar requested a review from Joibel July 26, 2023 15:42
Signed-off-by: or-shachar <or.shachar@wiz.io>
@or-shachar or-shachar force-pushed the 11020-retry-successful-workflows-cli branch from 897fcc7 to da46ab1 Compare July 26, 2023 17:40
@Joibel
Copy link
Member

Joibel commented Jul 26, 2023

Looks good to me. Thanks @or-shachar

@terrytangyuan terrytangyuan changed the title feat: cli allow retry successful workflow if nodeFieldSelector is set (Fixed #11020) feat: cli allow retry successful workflow if nodeFieldSelector is set. Fixes #11020) Jul 27, 2023
@terrytangyuan terrytangyuan enabled auto-merge (squash) July 27, 2023 03:13
@terrytangyuan terrytangyuan enabled auto-merge (squash) July 27, 2023 03:13
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

@terrytangyuan terrytangyuan merged commit 90930ab into argoproj:master Jul 27, 2023
23 checks passed
@agilgur5 agilgur5 added the area/cli The `argo` CLI label Sep 3, 2023
@agilgur5 agilgur5 added the area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries label Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli The `argo` CLI area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Retry (rerun) successful workflows from certain steps
5 participants