Skip to content

Conversation

@zzh8829
Copy link
Contributor

@zzh8829 zzh8829 commented May 5, 2023

example

> zeet cluster list team-zeet-example -o json | jq '.[].Name' 
"coreweave"
"zeet-example-gcp"

@zzh8829 zzh8829 requested a review from jereynolds May 5, 2023 21:13
output, err := json.MarshalIndent(clusters, "", " ")
if err != nil {
fmt.Fprintln(os.Stderr, err)
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't returning the error out to the cobra command cause it to print to Stdout? I think this will cause the error to print to both stdout and stderr. If we want to print the error only to stderr we should print here and return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is a interesting question, since we are expecting user to run this thru other utils like pipe and jq. what is the standard way to handle error happens somewhere in the upstream util like zeet cli? do we return exit code 1 or empty json list or print error message to stdout/stderr. is there some standard i can reference. to me it feels like all these options are equally valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should always send errors to stderr; this way there's no chance of a user accidentally piping error messages into downstream programs. I think this is not currently implemented basically anywhere in our CLI (certainly not for errors that are simply returned out to the Cobra command, but that is hopefully a very small change)

beyond that, i've seen wildly divergent behavior. but i think a reasonable rule for us is that for any given step (either a full command or a step in some interactive workflow within a command) we should either return data to stdout, OR an error to stderr plus a non-zero exit code, but not both.

For this particular PR I propose that we skip the printing to stderr, simply return err to allow cobra to handle this. Then we should make a ticket to improve error handling and make it consistent

@zzh8829 zzh8829 force-pushed the zz/list-cluster branch from c018f18 to cb8093a Compare May 9, 2023 23:15
@zzh8829 zzh8829 merged commit 1d65ad6 into main May 15, 2023
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.

3 participants