refactor(cmd): migrate CLI commands from gRPC to ConnectRPC clients#1388
refactor(cmd): migrate CLI commands from gRPC to ConnectRPC clients#1388whoAbhishekSah merged 5 commits intomainfrom
Conversation
Switch all CLI commands to use ConnectRPC clients instead of gRPC clients. This decouples the CLI from the legacy gRPC server, preparing for its removal. Key changes: - cmd/client.go: Replace gRPC dial with ConnectRPC HTTP client factory - cmd/context.go: Replace gRPC metadata with generic newRequest[T] helper - All command files: Use connect.NewRequest() / newRequest() patterns - Response access: res.GetField() -> res.Msg.GetField() - Remove gRPC connection lifecycle (cancel, defer cancel) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces gRPC dialing with HTTP Connect clients, adds host normalization, simplifies client constructors (removes context/cancel returns), moves header handling into request wrappers, updates all RPC calls to use connect request wrappers and accesses responses via res.Msg, and updates tests to expect error presence where applicable. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/user.go (1)
194-201:⚠️ Potential issue | 🟡 MinorColumn header mismatch: "SLUG" vs
GetTitle().The table header at line 194 says "SLUG" but the value displayed at line 199 is
user.GetTitle(). This appears to be a pre-existing issue, but worth noting for consistency.Proposed fix
- report = append(report, []string{"ID", "NAME", "EMAIL", "SLUG"}) + report = append(report, []string{"ID", "NAME", "EMAIL", "TITLE"})cmd/seed.go (1)
35-37:⚠️ Potential issue | 🟡 MinorGlobal mutable state may cause issues if seed is called multiple times.
The package-level variables
resourceNamespacesandroleIDsare appended to during execution without being reset. IfSeedCommandis invoked multiple times in the same process (e.g., tests or interactive CLI sessions), stale data from previous runs could accumulate.Proposed fix: Reset or scope variables locally
- resourceNamespaces []string - roleIDs []string ) func SeedCommand(cliConfig *Config) *cli.Command { var header, configFile string + var resourceNamespaces []string + var roleIDs []stringThen pass these as parameters or return values from
createCustomRolesAndPermissions.
🧹 Nitpick comments (2)
cmd/context.go (1)
20-27: Consider trimming whitespace from parsed header key and value.Headers like
"Authorization: Bearer token"will result in a value with a leading space (" Bearer token"). Most HTTP clients trim optional whitespace after the colon. Consider usingstrings.TrimSpacefor robustness.Proposed fix
func parseHeader(header string) (string, string, bool) { parts := strings.SplitN(header, ":", 2) if len(parts) != 2 { return "", "", false } - return parts[0], parts[1], true + return strings.TrimSpace(parts[0]), strings.TrimSpace(parts[1]), true }cmd/client.go (1)
12-24: Consider configuring HTTP client timeouts.Using
http.DefaultClientmeans no request timeouts are enforced. If the server is unresponsive, CLI commands could hang indefinitely. Consider creating a client with reasonable timeouts.Optional: Configure HTTP client with timeouts
+import "time" + +var httpClient = &http.Client{ + Timeout: 30 * time.Second, +} + func createClient(host string) (frontierv1beta1connect.FrontierServiceClient, error) { if host == "" { return nil, ErrClientConfigHostNotFound } - return frontierv1beta1connect.NewFrontierServiceClient(http.DefaultClient, ensureHTTPScheme(host)), nil + return frontierv1beta1connect.NewFrontierServiceClient(httpClient, ensureHTTPScheme(host)), nil } func createAdminClient(host string) (frontierv1beta1connect.AdminServiceClient, error) { if host == "" { return nil, ErrClientConfigHostNotFound } - return frontierv1beta1connect.NewAdminServiceClient(http.DefaultClient, ensureHTTPScheme(host)), nil + return frontierv1beta1connect.NewAdminServiceClient(httpClient, ensureHTTPScheme(host)), nil }
ConnectRPC wraps connection errors in *connect.Error instead of returning context.DeadlineExceeded directly. Update test assertions to use wantErr flag for connection-failure cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pull Request Test Coverage Report for Build 22133478615Details
💛 - Coveralls |
Issues found during testingWhile manually testing the CLI after the ConnectRPC migration, the following pre-existing issues were discovered:
These are not regressions from this PR — they existed before the migration. |
Avoids sending auth headers over unencrypted HTTP by default. Users can still explicitly use http:// for local development. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
parseHeader and newRequest now return errors when the header string is invalid (missing colon separator), preventing silent failures when users pass malformed --header values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/client.go (1)
12-23: Set a bounded HTTP timeout instead ofhttp.DefaultClient.
http.DefaultClienthas no request timeout, so CLI calls can hang indefinitely if the server stalls. The codebase already uses this pattern elsewhere (e.g.,cmd/serve.gocreates a client with an 80-second timeout). Apply the same approach here with a reasonable timeout value.♻️ Suggested update with a bounded timeout
import ( "fmt" "net/http" "strings" + "time" frontierv1beta1connect "github.com/raystack/frontier/proto/v1beta1/frontierv1beta1connect" "github.com/spf13/cobra" ) +var defaultHTTPClient = &http.Client{ + Timeout: 30 * time.Second, +} + func createClient(host string) (frontierv1beta1connect.FrontierServiceClient, error) { if host == "" { return nil, ErrClientConfigHostNotFound } - return frontierv1beta1connect.NewFrontierServiceClient(http.DefaultClient, ensureHTTPScheme(host)), nil + return frontierv1beta1connect.NewFrontierServiceClient(defaultHTTPClient, ensureHTTPScheme(host)), nil } func createAdminClient(host string) (frontierv1beta1connect.AdminServiceClient, error) { if host == "" { return nil, ErrClientConfigHostNotFound } - return frontierv1beta1connect.NewAdminServiceClient(http.DefaultClient, ensureHTTPScheme(host)), nil + return frontierv1beta1connect.NewAdminServiceClient(defaultHTTPClient, ensureHTTPScheme(host)), nil }
Replace http.DefaultClient (no timeout) with a client configured with a 30-second timeout to prevent CLI commands from hanging indefinitely if the server stalls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/client.go (1)
31-36: Trim whitespace before scheme normalization.If
hostcomes from config/flags with leading/trailing spaces, it will bypass the scheme check and produce malformed URLs. A quick trim makes the helper more robust.🔧 Suggested tweak
func ensureHTTPScheme(host string) string { + host = strings.TrimSpace(host) if strings.HasPrefix(host, "http://") || strings.HasPrefix(host, "https://") { return host } return fmt.Sprintf("https://%s", host) }
|
Issues discovered during testing and review:
|
3 participants
Summary
Migrate all CLI commands from gRPC clients to ConnectRPC clients, decoupling the CLI from the legacy gRPC server.
Changes
newRequest[T]helper for header propagationres.GetField()tores.Msg.GetField()--headervalues instead of silently ignoring*connect.Errorinstead ofcontext.DeadlineExceeded)Notes
createConnection()had acaCertFileparameter for custom CA certificates, but it was never wired up — bothcreateClientandcreateAdminClientalways passed"". No TLS support was lost in this migration.frontier seedcommand does not work against the ConnectRPC server. It relied on the deprecated identity proxy header (X-Frontier-Email) which the ConnectRPC server intentionally does not support. Tracked in Seed command fails with ConnectRPC server - needs proper authentication #1393.Test plan
go build ./...passesgo test ./cmd/...passesfrontier preferences list/frontier preferences getwith session cookie🤖 Generated with Claude Code