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

CLI: Rework to use RunE #1935

Closed
simster7 opened this issue Jan 10, 2020 · 2 comments · Fixed by #13656
Closed

CLI: Rework to use RunE #1935

simster7 opened this issue Jan 10, 2020 · 2 comments · Fixed by #13656
Labels
area/cli The `argo` CLI type/feature Feature request type/tech-debt

Comments

@simster7
Copy link
Member

Rework the CLI to use cobra.RunE instead of cobra.Run. This will provide some benefits such as unit testing for CLI commands, ability to export CLI as a package, etc.

Follow the model used by Argo Rollouts.
@simster7 simster7 added type/feature Feature request tech debt labels Jan 10, 2020
@simster7 simster7 mentioned this issue Apr 17, 2020
Closed
@alexec
Copy link
Contributor

alexec commented Apr 24, 2020

@sarabala1979 did you raise a more general ticket for testing the CLI.

@simster7 simster7 mentioned this issue Apr 27, 2020
Merged
@alexec alexec added area/cli The `argo` CLI type/tech-debt and removed type/feature Feature request labels Feb 7, 2022
@stale

This comment was marked as resolved.

Sign in to view
@stale stale bot added the problem/stale This has not had a response in some time label Mar 2, 2022
@alexec alexec removed the problem/stale This has not had a response in some time label Mar 2, 2022
@agilgur5 agilgur5 added the type/feature Feature request label Sep 21, 2023
MasonM added a commit to MasonM/argo-workflows that referenced this issue Sep 24, 2024
@MasonM
refactor(cli): improve error handling. Fixes argoproj#1935
d14141c 
It's tricky to write unit tests for the CLI code for three main reasons:
exiting on errors, lack of dependency injection, and insufficient mocks.
argoproj#3917 tried to solve all
of these issues, but appears to have been abandoned. I thought about
trying to revive that PR, but it's been over 4 years and there's
certainly going to be a ton of conflicts. I figure it's better to take a
more focused, incremental approach.

This PR is focused on improving error handling in the following ways:
1. Do argument validation using [cobra
   validators](https://cobra.dev/#positional-and-custom-arguments)
   instead of custom logic in the `Run` function. Besides being
   simpler, this leads to better error messages.
2. Replace all `Run` functions with `RunE`, which allows returning
   errors. This is the same approach that Argo Rollouts is taking
   (example:
   https://github.com/argoproj/argo-rollouts/blob/00e39b114849010dd96221a8db441d58e860d4d0/pkg/kubectl-argo-rollouts/cmd/abort/abort.go#L41)
3. Replace nearly all calls to `log.Fatal()`/`errors.CheckError()`/etc
   with error propagation.

Signed-off-by: Mason Malone <mmalone@adobe.com>
@MasonM MasonM mentioned this issue Sep 24, 2024
Merged
MasonM added a commit to MasonM/argo-workflows that referenced this issue Sep 24, 2024
@MasonM
refactor(cli): improve error handling. Fixes argoproj#1935
1a40f65 
It's tricky to write unit tests for the CLI code for three main reasons:
exiting on errors, lack of dependency injection, and insufficient mocks.
argoproj#3917 tried to solve all
of these issues, but appears to have been abandoned. I thought about
trying to revive that PR, but it's been over 4 years and there's
certainly going to be a ton of conflicts. I figure it's better to take a
more focused, incremental approach.

This PR is focused on improving error handling in the following ways:
1. Do argument validation using [cobra
   validators](https://cobra.dev/#positional-and-custom-arguments)
   instead of custom logic in the `Run` function. Besides being
   simpler, this leads to better error messages.
2. Replace all `Run` functions with `RunE`, which allows returning
   errors. This is the same approach that Argo Rollouts is taking
   (example:
   https://github.com/argoproj/argo-rollouts/blob/00e39b114849010dd96221a8db441d58e860d4d0/pkg/kubectl-argo-rollouts/cmd/abort/abort.go#L41)
3. Replace nearly all calls to `log.Fatal()`/`errors.CheckError()`/etc
   with error propagation.

Signed-off-by: Mason Malone <mmalone@adobe.com>
@agilgur5 agilgur5 changed the title Rework CLI to use RunE CLI: Rework to use RunE Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/cli The `argo` CLI type/feature Feature request type/tech-debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor(cli): improve CLI error handling. Fixes #1935 MasonM/argo-workflows
3 participants
@simster7 @alexec @agilgur5