Skip to content

feat: add verbose error messaging #3053

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

Merged
merged 6 commits into from
Jul 20, 2022
Merged

feat: add verbose error messaging #3053

merged 6 commits into from
Jul 20, 2022

Conversation

sreya
Copy link
Collaborator

@sreya sreya commented Jul 20, 2022

  • fix an issue where login fatally errored when checking versions.

Regular output:

image

Verbose output:

image

@sreya sreya requested a review from mafredri July 20, 2022 00:08
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

Other than the checkVersions function that could do with a cleanup, this looks great!

The only thing I noticed isn't touched by this PR, so not exactly relevant, but documenting it anyhow:

We probably shouldn't output the version check error when we can't reach the API as it adds verbosity to the output:
image


_, _ = fmt.Fprint(&output, helpErrMsg)

return cliui.Styles.Error.Render(output.String())
}

func checkVersions(cmd *cobra.Command, client *codersdk.Client) error {
Copy link
Member

Choose a reason for hiding this comment

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

Happened to notice this function still uses hard-coded version of varNoVersionCheck and also has a suppression check (which we're doing in the pre-run as well), do we want to keep both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, we don't need to keep both checks but I kept the check at the top of the the pre-run to avoid the unnecessary I/O of instantiating a client that we immediately throw away when someone passes the disable-warning flag.

@sreya
Copy link
Collaborator Author

sreya commented Jul 20, 2022

The only thing I noticed isn't touched by this PR, so not exactly relevant, but documenting it anyhow:
We probably shouldn't output the version check error when we can't reach the API as it adds verbosity to the output:

Great point I'll open up another PR to clean up some of that noise

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.

2 participants