-
Notifications
You must be signed in to change notification settings - Fork 2
Fix unclear error message when connecting cluster #281
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
Conversation
@@ -146,7 +146,8 @@ func RegisterCluster(db *gorm.DB, validate *validator.Validate, unmarshalFn Unma | |||
err = validate.Struct(crr) | |||
if err != nil { | |||
log.Errorf("Failed to validate payload: %v", err) | |||
common.WriteError(w, ErrInvalidPayload, http.StatusBadRequest) | |||
combinedError := fmt.Errorf("%v, %v", ErrInvalidPayload, "Error parsing 'url' in field 'Ingress URL'") |
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! It be interesting to test the output from the validator too. From:
https://github.com/go-playground/validator/#error-return-value
it says we should cast to validation errors after the nil check to get the actual validation error which might be useful too 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.
Makes sense! Thank you so much 🙏 I tried this at first but thought it was too long so I used a shorter error message but it's better to pass the validation error of course 👍 🙂
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.
Ah hah! What does it look like? A bit crazy?
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.
This is for the url string: Key: 'ClusterUpdateRequest.IngressURL' Error:Field validation for 'IngressURL' failed on the 'url' tag
👌
And this is for the ID 🤔: strconv.ParseUint: parsing \"\": invalid syntax
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.
Ooft! Alright I take it back that error is a bit rough. Your shorter one is much better. Maybe we go back to that one?
Thanks for testing!
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.
Sure! Thank you very much for reviewing Simon 🙏 🙂
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.
Looking good! <3 tests
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.
👌
Fixes: #150