-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: report the real error when newSocketMount() fails #252
Conversation
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.
Thanks for fixing this! We probably needed a smarter approach to port selection as is.
internal/proxy/proxy.go
Outdated
@@ -406,8 +406,8 @@ func NewClient(ctx context.Context, d alloydb.Dialer, l alloydb.Logger, conf *Co | |||
l.Errorf("failed to close mount: %v", mErr) | |||
} | |||
} | |||
i, err := ShortInstURI(inst.Name) | |||
if err != nil { | |||
i, instUriErr := ShortInstURI(inst.Name) |
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.
Why this change? Isn't err
enough here?
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.
The variable err
is initially declared on line 401: m, err := newSocketMount(ctx, conf, pc, inst)
. Then if that err is not nil, we need to report why not.
On line 409, we are attempting to format the name of the instance that caused newSocketMount()
to return an error. Redeclaring the variable err
on line 409 hides err
from line 401.
Then, on line 414, we are attempting to return an error from this function, including err
in the message.
If we redeclare err
on 409, then the statement on line 414 uses the err
from line 409 instead of what we wanted: the error from line 401.
Formatting the URI into an instance name when reporting an error should not hide the real error. Also,
avoid port conflict in root_test.go.