returning errors back to the client. #990
returning errors back to the client. #990LennyAndersen wants to merge 1 commit intoapache:masterfrom
Conversation
…tching errors in the oauth config
shibd
left a comment
shibd
left a comment
There was a problem hiding this comment.
@LennyAndersen Please fix the go lint related failed.
| oauth, _ := auth.NewAuthenticationOAuth2WithParams(authParams) | ||
| return oauth | ||
| func NewAuthenticationOAuth2(authParams map[string]string) (Authentication, error) { | ||
| oauth, err := auth.NewAuthenticationOAuth2WithParams(authParams) |
There was a problem hiding this comment.
I don't think you should change the return value.
You can add a new method or use panic here.
There was a problem hiding this comment.
Okay. I dont think I will do a panic/fatal or in any other way exit with an error in a function like that. But I guess that could be a matter of personal preference or coding style.
So just to be clear we're talking about the same; your suggestion would be to make another function called something like NewAuthenticationOAuth2withErrorHandling, correct?
There was a problem hiding this comment.
In principal, the modifications that modify the public APIs should not be accepted. /cc @shibd
5 participants
Motivation
When using the pulsar go client with oauth2, setting up the auth with the function NewAuthenticationOAuth2 doesn't return an error. In case of errors in the authentication flow nothing happens and the user/developer can only assume that the authentication went well.
It turns out there is a pretty decent error handling in the entire flow, but for unknown reasons this error is discarded right before the connection is returned to the client.
Modifications
Instead of just discarding the error, it is now returned. Note that the function now returns two parameters instead of just one and should be handled accordingly.
Refer to the changelog of the file
pulsar/client.goto see the actual changes.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation