Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Handle plugin errors properly #3103

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Conversation

marckhouzam
Copy link
Contributor

What this PR does / why we need it

Currently, when a plugin exists with an error the CLI will print ✖ exit status 1.
However, in such a case, we shouldn't print the plugin exit code as a string but instead should use it as our own exit code.

This commit catches the fact that the error is due to a plugin exiting with error and behaves accordingly.

Furthermore, to avoid duplicating error message, this commit tells Cobra not to print final error messages since the CLI prints them itself.

Which issue(s) this PR fixes

Fixes #44

Describe testing done for PR

I first recompile the cluster plugin to return an exit code of 2 just for testing.
Then I ran the following before applying the PR. The first command uses the cluster plugin while the second command uses a native CLI command, so we can compare.
We can see:

  1. for a plugin the exit status 2 string is printed (twice)
  2. the exit code of a plugin is not used as the CLI exit code (see the ===== errcode 1 which should be a value of 2)
  3. for a native command, the error is printed twice
$ tz cluster kubeconfig get; echo "===== errcode $?"
Error: accepts 1 arg(s), received 0
Error: exit status 2

✖  exit status 2
===== errcode 1
$ tz plugin delete
Error: must provide plugin name as positional argument

✖  must provide plugin name as positional argument

Then I ran the same tests with the PR applied. We can see that:

  1. the CLI no longer prints the exit status 1 string
  2. upon error, the exit code of the plugin is also the exit code of the CLI
  3. error strings are only printed once
$ tz cluster kubeconfig get; echo "===== errcode $?"
Error: accepts 1 arg(s), received 0
===== errcode 2
$ tz plugin delete
✖  must provide plugin name as positional argument

Release note

A plugin error code is propagated to the core CLI.
When a command fails the error messages are no longer duplicated.

Additional information

Special notes for your reviewer

When dealing with native commands, it is the CLI that prints the error using "github.com/aunum/log". The format is nicer as it uses colors and icons. However it prints the error to stdout and there does not seem to be a way to print it to stderr. I believe such errors should go to stderr. I will open a separate issue to track this.

@marckhouzam marckhouzam requested a review from a team as a code owner August 9, 2022 15:13
@codecov
Copy link

codecov bot commented Aug 9, 2022

Codecov Report

Merging #3103 (b637a2a) into main (678edc3) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3103      +/-   ##
==========================================
+ Coverage   44.44%   44.48%   +0.03%     
==========================================
  Files         417      417              
  Lines       42340    42340              
==========================================
+ Hits        18820    18835      +15     
+ Misses      21791    21781      -10     
+ Partials     1729     1724       -5     
Impacted Files Coverage Δ
pkg/v1/cli/command/core/root.go 0.00% <ø> (ø)
addons/controllers/clusterbootstrap_controller.go 64.96% <0.00%> (+1.05%) ⬆️
addons/controllers/machine_controller.go 68.68% <0.00%> (+3.03%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

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

Changes looks great. Thanks for fixing this.

Just had a nit comment.

cmd/cli/tanzu/main.go Show resolved Hide resolved
@anujc25 anujc25 added ok-to-merge PRs should be labelled with this before merging kind/bug PR/Issue related to a bug area/core-cli labels Aug 10, 2022
1- when a plugin exists with an error, we don't want to print its exit
   code as a string, but instead want to re-use its exit code as our own
2- the CLI prints the final error to the screen, so we don't want Cobra
   to also print this error

Signed-off-by: Marc Khouzam <kmarc@vmware.com>
@marckhouzam
Copy link
Contributor Author

The last forced-push was simply to re-trigger the code-coverage test which had failed for some reason... It passed now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/core-cli cla-not-required kind/bug PR/Issue related to a bug ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary msg "Error: exit status 1" when a Tanzu CLI command failed
3 participants