cckellogg commented on a change in pull request #635:
URL: https://github.com/apache/pulsar-client-go/pull/635#discussion_r723751698
##########
File path: oauth2/oidc_endpoint_provider.go
##########
@@ -36,10 +36,16 @@ type OIDCWellKnownEndpoints struct {
// GetOIDCWellKnownEndpointsFromIssuerURL gets the well known endpoints for the
// passed in issuer url
func GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL string)
(*OIDCWellKnownEndpoints, error) {
+ if issuerURL == "" {
+ return nil, errors.New("required: issuer url")
+ }
u, err := url.Parse(issuerURL)
if err != nil {
return nil, errors.Wrap(err, "could not parse issuer url to
build well known endpoints")
}
+ if !u.IsAbs() {
+ return nil, errors.New("invalid: issuer url")
Review comment:
Is it possible to get a reason why the issue url is invalid and/or
include the invalid url within the error message?
##########
File path: oauth2/oidc_endpoint_provider.go
##########
@@ -36,10 +36,16 @@ type OIDCWellKnownEndpoints struct {
// GetOIDCWellKnownEndpointsFromIssuerURL gets the well known endpoints for the
// passed in issuer url
func GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL string)
(*OIDCWellKnownEndpoints, error) {
+ if issuerURL == "" {
+ return nil, errors.New("required: issuer url")
Review comment:
maybe add a little more context "issuer url not provided" or "required
issuer url missing"?
##########
File path: oauth2/oidc_endpoint_provider.go
##########
@@ -36,10 +36,16 @@ type OIDCWellKnownEndpoints struct {
// GetOIDCWellKnownEndpointsFromIssuerURL gets the well known endpoints for the
// passed in issuer url
func GetOIDCWellKnownEndpointsFromIssuerURL(issuerURL string)
(*OIDCWellKnownEndpoints, error) {
+ if issuerURL == "" {
+ return nil, errors.New("required: issuer url")
+ }
u, err := url.Parse(issuerURL)
if err != nil {
return nil, errors.Wrap(err, "could not parse issuer url to
build well known endpoints")
}
+ if !u.IsAbs() {
+ return nil, errors.New("invalid: issuer url")
Review comment:
I'm not going to hold it up. However, just because other things don't do
that in the code does not mean we can't make things better for users.
```
invalid issuer url expected an absolute url got=<invalid-url>
or
invalid issuer_url=<invalid-url> expected an absolute url
```
To me the above error messages are way more efficient to troubleshoot since
I can see the error and the value of the suspect url. All the context is in one
place. Without this type of information someone has to look at application code
or search logs for configuration if those are even logged. What if the
application dynamically looks up this url, how will a user easily figure out
the suspect url?
I'll admit I'm a little biased having spent years debugging and
troubleshooting pulsar related issues. The more context that can be provided in
error and logs messages the easier it is for users and saves lots of time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
