Skip to content

main.go: leftovers from PR #242 #289

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

Draft
stefanb wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

main.go: leftovers from PR #242 #289

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
493ef99
main.go: leftovers from PR #242
erazemk Aug 13, 2025
21bf459
Merge branch 'master' into PR-242-leftovers
stefanb Mar 1, 2026
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
38 changes: 19 additions & 19 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/alts"
"google.golang.org/grpc/credentials/insecure"
healthpb "google.golang.org/grpc/health/grpc_health_v1"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -99,7 +100,7 @@ func init() {
os.Exit(StatusInvalidArguments)
}

argError := func(s string, v ...interface{}) {
argError := func(s string, v ...any) {
log.Printf("error: "+s, v...)
os.Exit(StatusInvalidArguments)
}
Expand Down Expand Up @@ -259,7 +260,6 @@ func main() {

opts := []grpc.DialOption{
grpc.WithUserAgent(flUserAgent),
grpc.WithBlock(),
}
if flTLS && flSPIFFE {
log.Printf("-tls and -spiffe are mutually incompatible")
Expand All @@ -278,7 +278,8 @@ func main() {
creds := alts.NewServerCreds(alts.DefaultServerOptions())
opts = append(opts, grpc.WithTransportCredentials(creds))
} else if flSPIFFE {
spiffeCtx, _ := context.WithTimeout(ctx, flRPCTimeout)
spiffeCtx, spiffeCancel := context.WithTimeout(ctx, flRPCTimeout)
defer spiffeCancel()
source, err := workloadapi.NewX509Source(spiffeCtx)
if err != nil {
log.Printf("failed to initialize tls credentials with spiffe. error=%v", err)
Expand All @@ -295,34 +296,33 @@ func main() {
creds := credentials.NewTLS(tlsconfig.MTLSClientConfig(source, source, tlsconfig.AuthorizeAny()))
opts = append(opts, grpc.WithTransportCredentials(creds))
} else {
opts = append(opts, grpc.WithInsecure())
opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials()))
}

if flGZIP {
opts = append(opts,
grpc.WithCompressor(grpc.NewGZIPCompressor()),
grpc.WithDecompressor(grpc.NewGZIPDecompressor()),
)
Comment on lines -303 to -306
Copy link
Collaborator Author

@stefanb stefanb Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly related:

Copy link
Collaborator Author

@stefanb stefanb Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported to

}
// Note: GZIP compression is now handled automatically by gRPC when available
// The deprecated WithCompressor/WithDecompressor options are no longer needed
// The -gzip flag is kept for backward compatibility but has no effect
_ = flGZIP // Suppress unused variable warning
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flGZIP flag is now silently ignored, which could confuse users who expect it to enable compression. Consider either: (1) logging a deprecation warning when the flag is used, or (2) documenting this behavior change in the PR description or changelog so users understand the flag no longer has an effect.

Suggested change
_ = flGZIP // Suppress unused variable warning
if flGZIP {
log.Printf("warning: the -gzip flag is deprecated and no longer has any effect; gRPC compression is negotiated automatically")
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@stefanb stefanb Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ported over to


if flVerbose {
log.Print("establishing connection")
}
connStart := time.Now()
dialCtx, dialCancel := context.WithTimeout(ctx, flConnTimeout)
defer dialCancel()
conn, err := grpc.DialContext(dialCtx, flAddr, opts...)

// Use the new grpc.NewClient instead of deprecated DialContext
conn, err := grpc.NewClient(flAddr, opts...)
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc.NewClient creates a client without immediately establishing a connection, but the code measures connDuration and logs 'connection established'. This is misleading since the actual connection happens lazily on the first RPC. The connection timing logic should be removed or updated to reflect that NewClient doesn't block to establish a connection.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@ahmetb ahmetb Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still thinking we do some form of blocking dial and still let people configure dial timeout. What do you think?

Copy link
Collaborator Author

@stefanb stefanb Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, current (on master) blocking while immediately establishing the connection may be deprecated for real-world applications, but for explicitly testing the grpc services health it may still be ok to do so. We just need to find how to elegantly do similar with the new NewClient() instead of deprecated grpc.DialContext().

Copy link
Collaborator

@ahmetb ahmetb Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

highly doubt this method will be removed anytime soon since it's a major breaking change for a lot of legacy code out there.

Copy link
Collaborator Author

@stefanb stefanb Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deprecation notices of both DialContext and WithBlock say:

Will be supported throughout 1.x.

(this is also clearly shown by linter)

Copy link
Collaborator Author

@stefanb stefanb Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, perhaps we should just add a comment why is this still being used and instruct linter to ignore it?

if err != nil {
if err == context.DeadlineExceeded {
log.Printf("timeout: failed to connect service %q within %v", flAddr, flConnTimeout)
} else {
log.Printf("error: failed to connect service at %q: %+v", flAddr, err)
}
log.Printf("error: failed to create client for service at %q: %+v", flAddr, err)
retcode = StatusConnectionFailure
return
}

connDuration := time.Since(connStart)
defer conn.Close()
defer func() {
if closeErr := conn.Close(); closeErr != nil {
log.Printf("warning: failed to close connection: %v", closeErr)
}
}()
Comment on lines +321 to +325
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling for Close() is now more verbose than necessary. Since conn.Close() errors are typically not actionable and the program is already exiting, the simpler 'defer conn.Close()' pattern would be sufficient and more readable.

Suggested change
defer func() {
if closeErr := conn.Close(); closeErr != nil {
log.Printf("warning: failed to close connection: %v", closeErr)
}
}()
defer conn.Close()

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@stefanb stefanb Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion reverts this defer block addition and re-introduces the linter waning.

if flVerbose {
log.Printf("connection established (took %v)", connDuration)
}
Expand Down
Loading