Skip to content
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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 31, 2023
Merged

fix: report the real error when newSocketMount() fails #252

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9866903
fix: assigning err hides the real connection error
hessjcg Jan 31, 2023
f7f9274
fix: avoid port conflicts in test that intermintently cause a failure.
hessjcg Jan 31, 2023
aae69c9
chore: fix lint formatting issue.
hessjcg Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ func TestPrometheusMetricsEndpoint(t *testing.T) {
// Keep the test output quiet
c.SilenceUsage = true
c.SilenceErrors = true
c.SetArgs([]string{"--prometheus", "projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance"})
c.SetArgs([]string{"--prometheus", "projects/my-project/locations/my-region/clusters/my-cluster/instances/my-instance?port=5321"})

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
Expand All @@ -926,7 +926,7 @@ func TestPProfServer(t *testing.T) {
c.SilenceUsage = true
c.SilenceErrors = true
c.SetArgs([]string{"--debug", "--admin-port", "9191",
"projects/proj/locations/region/clusters/clust/instances/inst"})
"projects/proj/locations/region/clusters/clust/instances/inst?port=5323"})
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

Expand Down
4 changes: 2 additions & 2 deletions internal/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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?

Copy link
Contributor Author

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.

if instUriErr != nil {
// this shouldn't happen because the inst uri is already validated by this point
i = inst.Name
}
Expand Down