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

fix(cli): Check mutual exclusivity for argo CLI flags #3493

Merged
merged 5 commits into from Jul 17, 2020
Merged

fix(cli): Check mutual exclusivity for argo CLI flags #3493

merged 5 commits into from Jul 17, 2020

Conversation

terrytangyuan
Copy link
Member

This sets the right expectations to users. --resubmitted can be used jointly while --completed and --running are mutually exclusive.

Signed-off-by: terrytangyuan terrytangyuan@gmail.com

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".
  • 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.
  • My organization is added to USERS.md.

@simster7
Copy link
Member

Could I ask that you also go over the rest of the flags and commands and do all mutex checks that are applicable?

@terrytangyuan
Copy link
Member Author

@simster7 Absolutely. I'll include all changes in this PR.

@terrytangyuan terrytangyuan changed the title fix(cli): Check mutual exclusivity for argo list args [WIP] fix(cli): Check mutual exclusivity for argo list args Jul 16, 2020
@terrytangyuan terrytangyuan changed the title [WIP] fix(cli): Check mutual exclusivity for argo list args fix(cli): Check mutual exclusivity for argo CLI flags Jul 16, 2020
@terrytangyuan
Copy link
Member Author

terrytangyuan commented Jul 16, 2020

@simster7 Just went over and there doesn't seem to be many of them left. I updated the PR with the ones I found. The build failure is unrelated.

@alexec
Copy link
Contributor

alexec commented Jul 16, 2020

Can you please investigate failing tests?

@terrytangyuan
Copy link
Member Author

I am seeing the following error that's unrelated to this PR. It also happens in other PRs as well, e.g. #3488.

##[error]level=info msg=... condition="to finish" message="OCI runtime create failed: container_linux.go:349: starting container process caused \"exec: \\\"sh\\\": executable file not found in $PATH\": unknown" phase=Failed timeout=30s type=MODIFIED workflow=exit-1

@alexec
Copy link
Contributor

alexec commented Jul 17, 2020

The failing test is:

    --- FAIL: TestCLISuite/TestWorkflowDeleteAll (3.98s)
        cli_test.go:395: 
            	Error Trace:	cli_test.go:395
            	            				given.go:179
            	            				cli_test.go:394
            	Error:      	Received unexpected error:
            	            	exit status 1
            	Test:       	TestCLISuite/TestWorkflowDeleteAll

Tip: search for FAIL: in the logs.

Could this be related to this PR?

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@terrytangyuan
Copy link
Member Author

@alexec Thanks! That's good tip. It's fixed now.

@@ -391,7 +391,7 @@ func (s *CLISuite) TestWorkflowDeleteAll() {
SubmitWorkflow().
WaitForWorkflow(30*time.Second).
Given().
RunCli([]string{"delete", "--all", "-l", "argo-e2e"}, func(t *testing.T, output string, err error) {
RunCli([]string{"delete", "--all"}, func(t *testing.T, output string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can't be correct - all workflows created by e2e test must get this label - so this probably means there is a bug

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you clarify where is incorrect? There is only one workflow called basic at this point so deleting all would just means only this workflow gets deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

All test workflows must be annotated with argo-e2e - if it fails to delete, then it indicates that --all is not deleting all workflows labelled with argo-e2e. --all should work with -l, but does not appear to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I see. I've reverted the flag check in delete command. Please take another look.

Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
@alexec alexec merged commit 19e700a into argoproj:master Jul 17, 2020
@terrytangyuan terrytangyuan deleted the list-doc-check branch July 17, 2020 19:48
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.

None yet

3 participants