-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat(cli): add debug logging flag #65
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
base: main
Are you sure you want to change the base?
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/internal/workos/http/client.go (3)
230-249: Close response body after reads
GetJWKsreads body but never closes it. Add a defer to prevent leaks.response, err := h.get(jwkPath) if err != nil { return nil, err } +defer response.Body.Close()
252-281: Close response body in AuthenticateWithCodePrevent resource leak.
response, err := h.post("/user_management/authenticate", body) if err != nil { return nil, err } +defer response.Body.Close()
351-381: Close response body in AuthenticateWithRefreshTokenSame leak pattern here.
response, err := h.post("/auth/refresh", body) if err != nil { return nil, err } +defer response.Body.Close()
🧹 Nitpick comments (5)
cli/internal/config/config.go (1)
19-19: Config.Debug added — OK, but consider not persisting it by defaultIf Debug is meant to be ephemeral (CLI session only), saving the config later may persist it unintentionally.
Apply this small guard in Save to omit Debug from writes:
func (c *Config) Save(global bool) error { var configMap map[string]interface{} err := mapstructure.Decode(c, &configMap) if err != nil { return fmt.Errorf("failed to decode config struct: %w", err) } + // Don't persist ephemeral flags + delete(configMap, "debug")cli/cmd/root.go (1)
25-27: Enable debug from flag — good wiringWorks for all subcommands. Optionally print a one-liner to stderr so users notice debug mode is on.
if debugFlag, _ := cmd.Flags().GetBool("debug"); debugFlag { conf.SetDebug(true) + fmt.Fprintln(os.Stderr, "[DEBUG] Verbose logging enabled (sensitive headers redacted).") }Note: add
osto imports.cli/internal/workos/workos.go (1)
39-41: Avoid shadowing the imported package nameLocal variable
configshadows theconfigpackage; rename tocfgfor clarity.- config := do.MustInvoke[*config.Config](inj) - sugaServerUrl := config.GetSugaServerUrl() + cfg := do.MustInvoke[*config.Config](inj) + sugaServerUrl := cfg.GetSugaServerUrl() ... - httpClient := http.NewHttpClient("", config, opts...) + httpClient := http.NewHttpClient("", cfg, opts...)cli/internal/api/api.go (1)
93-96: Consider using a client with timeouts instead of http.DefaultClientDefault client has no timeout; a hung server can stall the CLI.
Define an
http.Clientwith a sane timeout (e.g., 30s) and reuse it in SugaApiClient.Also applies to: 145-149
cli/internal/workos/http/client.go (1)
289-306: Optional: share a single helper to join base URL + pathSmall consistency nit: you alternate between
JoinPath(path)andJoinPath("/path"). Not wrong, but a helper reduces chances of double slashes.Also applies to: 320-337, 513-534
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/cmd/root.go(1 hunks)cli/internal/api/api.go(5 hunks)cli/internal/config/config.go(2 hunks)cli/internal/workos/http/client.go(5 hunks)cli/internal/workos/workos.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/internal/workos/workos.go (1)
cli/internal/workos/http/client.go (1)
NewHttpClient(109-134)
cli/internal/workos/http/client.go (1)
cli/internal/config/config.go (1)
Config(16-20)
🔇 Additional comments (4)
cli/internal/config/config.go (1)
97-99: Setter looks goodSimple and clear. No issues.
cli/cmd/root.go (1)
34-34: Flag UX — clearHelp text is explicit and matches behavior.
cli/internal/workos/workos.go (1)
55-55: Updated NewHttpClient call — OKSignature change handled correctly and passes config through.
cli/internal/workos/http/client.go (1)
306-317: Debug calls are fine once redaction is in placeWith the redaction changes above, these call sites are safe.
Also applies to: 334-345, 530-541
Adds a debug flag to enable verbose logging of API requests and responses. This feature helps troubleshoot issues by providing detailed information about the communication between the CLI and the backend server. The debug flag is added as a persistent flag to the root command, allowing it to be used with any subcommand. The API client and the WorkOS client are updated to log requests and responses when the debug flag is enabled.
6953f77 to
34340ce
Compare
Adds a debug flag to enable verbose logging of API requests and responses.
This feature helps troubleshoot issues by providing detailed information about the communication between the CLI and the backend server.
The debug flag is added as a persistent flag to the root command, allowing it to be used with any subcommand.
The API client and the WorkOS client are updated to log requests and responses when the debug flag is enabled.