Skip to content

chore: add validation errors to the cli output #12814

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 4 commits into from
Apr 2, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Mar 28, 2024

Closes #12575

Error:

{
	Response: codersdk.Response{
		Message: "Top level sdk error message.",
		Detail:  "magic dust unavailable, please try again later",
		Validations: []codersdk.ValidationError{
			{
				Field:  "region",
				Detail: "magic dust is not available in your region",
			},
		},
	}
	Helper: "Have you tried turning it off and on again?"
}

Before:

Screenshot from 2024-03-28 14-55-53

After:

Screenshot from 2024-03-28 14-55-09

If you did not know, coder exp example-error <subcommand> has a set of sub commands to see how different cli errors are rendered out. No need to make them occur naturally.

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Looks great!

I think the validation errors could be in yellow though, to make them stand out more?
Also can you please add a couple unit tests?

@Emyrk
Copy link
Member Author

Emyrk commented Mar 29, 2024

Also can you please add a couple unit tests?

Unfortunately the cli golden file code (which is the only unit test that makes sense for formatting output) does not handle errors. This is because all cli tests invoke the command from within Go, skipping the error handler.

You can see this is how we run it in a test:

err := inv.Run()

Refactoring this will likely break a lot of cli tests since the go error is more useful then it's text.

I do not want to add a new golden file test for this formatter in this PR. We can come back to testing if we really have formatting drift issues.

Edit: Just found an old issue I made: #11588. Exactly this.

@Emyrk
Copy link
Member Author

Emyrk commented Mar 29, 2024

Screenshot from 2024-03-29 09-03-05

I made it yellow though!

@Emyrk Emyrk requested a review from dannykopping March 29, 2024 14:19
@dannykopping
Copy link
Contributor

Edit: Just found an old issue I made: #11588. Exactly this.

@Emyrk given this is now in place, should we add a couple tests to it to encompass the new functionality?
I'm not so much worried about formatting errors as regressions; if we change the way we attach validation errors to the codersdk.Error or some other transitive change, this will represent a degradation in UX on the CLI.

I made it yellow though!

Looks great! Thanks

@Emyrk
Copy link
Member Author

Emyrk commented Apr 2, 2024

@Emyrk given this is now in place, should we add a couple tests to it to encompass the new functionality? I'm not so much worried about formatting errors as regressions; if we change the way we attach validation errors to the codersdk.Error or some other transitive change, this will represent a degradation in UX on the CLI.

Tests were added here. These cover all example errors, which covers the validation errors here. I'll rebase on main to get them.

#12698

@Emyrk Emyrk force-pushed the stevenmasley/cli_validation_errors branch from cb1b2ba to 13bd167 Compare April 2, 2024 14:36
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Emyrk Emyrk merged commit 5137433 into main Apr 2, 2024
@Emyrk Emyrk deleted the stevenmasley/cli_validation_errors branch April 2, 2024 15:02
@github-actions github-actions bot locked and limited conversation to collaborators Apr 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(cli): API validation errors are not displayed
2 participants