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): Add a flag status to delete cmd like list cmd of argo cli #11577

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

spencercjh
Copy link
Contributor

Motivation

For #11555. @tooptoop4 wants to filter workflows more accurately and flexibly by status.

There are two supported flags associated with it:

  • --field-selector: metadata.name=foo, metadata.namespace=bar
  • --selector: workflows.argoproj.io/phase=Succeeded

It is not feasible to use the --field-selector to filter workflows by status because of the well-known reasons.

Using -l(--selector) would satisfy the need, but the labels are too long with a common prefix workflows.argoproj.io/. It is not convenient to use.

Luckily, argo list has a directly related flag --status to filter workflows by status like argo list --status Running. It is more convenient to use. I think it could be taken a step further by exempting users from using the shell combination like argo delete $(argo list --status Succeeded | awk '{print $1}')

Modifications

  1. Add a flag --status to argo delete to filter workflows by status. It's an existing field of struct listFlags and can be used directly.

Verification

There was no test code for argo delete before. I run it locally and it works as expected.

image

@spencercjh spencercjh marked this pull request as draft August 15, 2023 03:12
@spencercjh
Copy link
Contributor Author

#11557 does something similar, but I don't think flag pending makes sense, there shouldn't be such a tiny, fine-grained flag.

Also thanks for his/her PR. 🙏

@spencercjh spencercjh marked this pull request as ready for review August 15, 2023 03:38
@agilgur5
Copy link
Member

agilgur5 commented Aug 15, 2023

It is not feasible to use the --field-selector to filter workflows by status because of the well-known reasons.

Using -l(--selector) would satisfy the need, but the labels are too long with a common prefix workflows.argoproj.io/. It is not convenient to use.

Agreed with this rationale. After you brought up the inadequacy of --field-selector in #11555 (comment), I thought label selectors could be a workaround as well, but I would agree that it is quite long to type out. In either case, I did mention selectors as a workaround.

Moreover, argo list already supports --status, so this is just exposing that existing flag to argo delete.

@agilgur5
Copy link
Member

#11557 does something similar, but I don't think flag pending makes sense, there shouldn't be such a tiny, fine-grained flag.

I would normally agree with this, but given that there are already --running and --completed flags, it's not altogether new either. If those didn't exist already, I would definitely prefer to not increase the surface area.

In any case, I think that discussion should be had in that PR. This PR does not technically preclude that PR

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

one tiny comment I need to look at more closely, otherwise LGTM.
Thanks for your contribution!

cmd/argo/commands/delete.go Show resolved Hide resolved
cmd/argo/commands/delete.go Show resolved Hide resolved
cmd/argo/commands/delete.go Show resolved Hide resolved
@spencercjh
Copy link
Contributor Author

Thank's for your review, your analysis is very detailed and careful. 👍 💯

Signed-off-by: spencercjh <spencercjh@gmail.com>
@terrytangyuan terrytangyuan enabled auto-merge (squash) August 17, 2023 01:08
@terrytangyuan terrytangyuan merged commit 27ffa83 into argoproj:master Aug 17, 2023
23 checks passed
@agilgur5 agilgur5 added the area/cli The `argo` CLI label Sep 26, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants