-
Notifications
You must be signed in to change notification settings - Fork 69
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
Return all validation errors from configuration #1561
Conversation
96bffe6
to
d45556f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
d45556f
to
923f358
Compare
Signed-off-by: Mark Sagi-Kazar <[email protected]>
Signed-off-by: Mark Sagi-Kazar <[email protected]>
3f6ddd9
to
ede9550
Compare
if err := c.ConsumerConfiguration.Validate(); err != nil { | ||
return err | ||
errs = append(errs, errorsx.WithPrefix(err, "consumer")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it would be better if the prefixes used the full name of the configuration object, it would make things 100% unambiguous (e.g. consumer => ConsumerConfiguration)
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm...maybe
Right now it follows the name as it appears in the config file which probably makes it easier to debug a problem.
Ultimately, this should be a "path" to the configuration (whether it's file or not), but people rarely look at the code to figure out configuration.
Overview
Instead of returning on error at a time, config now returns all validation errors.
Notes for reviewer