feat: Create GC CLI Tool for Authentication#343
Open
adityachopra29 wants to merge 13 commits intocontainer-registry:mainfrom
Open
feat: Create GC CLI Tool for Authentication#343adityachopra29 wants to merge 13 commits intocontainer-registry:mainfrom
adityachopra29 wants to merge 13 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
adityachopra29
changed the title
Contributor
📝 WalkthroughWalkthroughAdds a new gcctl CLI (Cobra-based) with login/logout/whoami commands, YAML config management, an HTTP API client, output formatting utilities, server-side /api/whoami endpoint, and module dependency updates for Cobra and friends. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as gcctl CLI
participant API as API Client
participant Server
User->>CLI: gcctl login --server https://gc.example.com
CLI->>User: prompt username/password (or read stdin)
User->>CLI: provide credentials
CLI->>API: NewClient(server)
CLI->>API: Ping()
API->>Server: GET /api/ping
Server-->>API: 200 OK
CLI->>API: Login(username,password)
API->>Server: POST /login
Server-->>API: {token, expires_at}
API-->>CLI: LoginResponse
CLI->>CLI: Save token/config to ~/.gcctl/config.yaml
CLI-->>User: Login successful
User->>CLI: gcctl whoami
CLI->>CLI: Load config, resolve server
CLI->>API: NewClient(server, token)
CLI->>API: Whoami()
API->>Server: GET /api/whoami (Authorization)
Server->>Server: extract user from context
Server-->>API: {username, role}
API-->>CLI: WhoamiResponse
CLI->>CLI: Format output (json/yaml/table)
CLI-->>User: Display user info
User->>CLI: gcctl logout
CLI->>CLI: Load config
CLI->>API: NewClient(server, token)
CLI->>API: Logout()
API->>Server: POST /api/logout (Authorization)
Server-->>API: 200 OK
CLI->>CLI: Clear local credentials
CLI-->>User: Logout successful
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codacy's Analysis Summary1 new issue (≤ 0 issue) Review Pull Request in Codacy →
|
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
Signed-off-by: Aditya Chopra <adityachopra2912@gmail.com>
adityachopra29
force-pushed
the
satellite-CLI
branch
from
60e16c5 to
5c75258
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 18 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="ground-control/cmd/gcctl/root/logout.go">
<violation number="1" location="ground-control/cmd/gcctl/root/logout.go:38">
P2: Logout ignores SaveConfig errors when ResolveServer fails, so credentials may remain on disk while reporting success.</violation>
</file>
<file name="ground-control/cmd/gcctl/root/cmd.go">
<violation number="1" location="ground-control/cmd/gcctl/root/cmd.go:27">
P2: GetConfigPath returns "" on error, so LoadConfig/SaveConfig operate on an invalid path, masking the real failure and producing confusing read/write behavior.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if err != nil { | ||
| // No server known — just clear local state | ||
| cfg.ClearCredentials() | ||
| _ = SaveConfig(cfg) |
Contributor
There was a problem hiding this comment.
P2: Logout ignores SaveConfig errors when ResolveServer fails, so credentials may remain on disk while reporting success.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ground-control/cmd/gcctl/root/logout.go, line 38:
<comment>Logout ignores SaveConfig errors when ResolveServer fails, so credentials may remain on disk while reporting success.</comment>
<file context>
@@ -0,0 +1,60 @@
+ if err != nil {
+ // No server known — just clear local state
+ cfg.ClearCredentials()
+ _ = SaveConfig(cfg)
+ fmt.Println("Local credentials cleared.")
+ return nil
</file context>
| path, err := gcctlconfig.DefaultConfigPath() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: could not determine default config path: %v\n", err) | ||
| return "" |
Contributor
There was a problem hiding this comment.
P2: GetConfigPath returns "" on error, so LoadConfig/SaveConfig operate on an invalid path, masking the real failure and producing confusing read/write behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ground-control/cmd/gcctl/root/cmd.go, line 27:
<comment>GetConfigPath returns "" on error, so LoadConfig/SaveConfig operate on an invalid path, masking the real failure and producing confusing read/write behavior.</comment>
<file context>
@@ -0,0 +1,109 @@
+ path, err := gcctlconfig.DefaultConfigPath()
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "Warning: could not determine default config path: %v\n", err)
+ return ""
+ }
+ return path
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
ground-control/cmd/gcctl/pkg/config/config.go (1)
35-36: Prefererrors.Is(err, fs.ErrNotExist)overos.IsNotExist.
os.IsNotExistpredates theerrorschain-unwrapping API and can miss wrapped errors. The idiomatic modern form is:🔧 Proposed fix
+ "errors" + "io/fs" ... - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/config/config.go` around lines 35 - 36, Replace the os.IsNotExist(err) check with errors.Is(err, fs.ErrNotExist) to correctly handle wrapped errors in the config loading routine (the branch returning &Config{}, nil), and add the necessary imports ("errors" and "io/fs") to the file so the compiler resolves errors.Is and fs.ErrNotExist.ground-control/cmd/gcctl/root/version.go (1)
13-26: ConsiderRunE+ structured output support for consistency.Two minor inconsistencies with the rest of the CLI:
- All other commands use
RunE; this usesRun, which silently drops anyfmt.Printfwrite errors.- The other commands respect
GetOutputFormat()/--output;VersionCommandignores it. Emitting JSON/YAML version info is handy for scripts and aligns with the stated-osupport.♻️ Suggested refactor
- Run: func(cmd *cobra.Command, args []string) { - fmt.Printf("gcctl version %s\n", version.Version) - fmt.Printf(" Git Commit: %s\n", version.GitCommit) - fmt.Printf(" Build Date: %s\n", version.BuildDate) - fmt.Printf(" Go Version: %s\n", runtime.Version()) - fmt.Printf(" OS/Arch: %s/%s\n", runtime.GOOS, runtime.GOARCH) - }, + RunE: func(cmd *cobra.Command, args []string) error { + type versionInfo struct { + Version string `json:"version" yaml:"version"` + GitCommit string `json:"git_commit" yaml:"git_commit"` + BuildDate string `json:"build_date" yaml:"build_date"` + GoVersion string `json:"go_version" yaml:"go_version"` + OSArch string `json:"os_arch" yaml:"os_arch"` + } + info := versionInfo{ + Version: version.Version, + GitCommit: version.GitCommit, + BuildDate: version.BuildDate, + GoVersion: runtime.Version(), + OSArch: runtime.GOOS + "/" + runtime.GOARCH, + } + if f := GetOutputFormat(); f == "json" || f == "yaml" { + return utils.PrintFormat(info, f) + } + fmt.Printf("gcctl version %s\n", info.Version) + fmt.Printf(" Git Commit: %s\n", info.GitCommit) + fmt.Printf(" Build Date: %s\n", info.BuildDate) + fmt.Printf(" Go Version: %s\n", info.GoVersion) + fmt.Printf(" OS/Arch: %s\n", info.OSArch) + return nil + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/version.go` around lines 13 - 26, The VersionCommand currently uses Run and prints with fmt.Printf (silently ignoring write errors) and ignores the CLI output format; change VersionCommand to use RunE and return errors, build a structured version payload (fields: Version, GitCommit, BuildDate, GoVersion, OS, Arch) and respect GetOutputFormat()/--output to emit human-readable text by default or JSON/YAML when requested; update the command to call GetOutputFormat() inside the RunE handler, marshal/encode the payload accordingly, and use cmd.PrintErr/ cmd.Print/ writers or returned errors for any write/marshal failures so failures surface properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ground-control/cmd/gcctl/pkg/api/client.go`:
- Around line 155-165: The parseAPIError function currently discards the
io.ReadAll error which can produce empty/misleading APIError messages; update
parseAPIError to check the error returned by io.ReadAll(resp.Body) and, if
non-nil, return an APIError containing the HTTP status and the read error
message (or a combined message including the partial body and read error),
otherwise proceed to json.Unmarshal as before and return the parsed error or the
body string; reference the parseAPIError function and the APIError type when
making this change.
- Around line 54-141: The public client methods (Login, Logout, Whoami, Ping)
and the helper newAuthRequest currently create requests with http.NewRequest
(background context) so in-flight HTTP calls can't be cancelled; change each of
those method signatures to accept a context.Context parameter, propagate that
context into newAuthRequest and all http.NewRequestWithContext calls (replace
http.NewRequest with http.NewRequestWithContext(ctx,...)), and ensure
newAuthRequest also accepts and uses the context when creating requests; update
any callers to pass cmd.Context() (or other contexts) through to these methods.
In `@ground-control/cmd/gcctl/pkg/utils/format.go`:
- Around line 29-31: The printYAML function currently creates a yaml.Encoder and
immediately calls Encode without closing it, risking truncated output; change
printYAML (the function) to create a local encoder via
yaml.NewEncoder(os.Stdout), defer or explicitly call encoder.Close() to ensure
the encoder flushes and writes the document end marker, then call
encoder.Encode(data) and return its error (handling any Close error as
appropriate).
In `@ground-control/cmd/gcctl/root/cmd.go`:
- Around line 12-18: The package-level mutable vars cfgFile, outputFormat,
serverURL, and verbose should be removed and scoped inside RootCmd(); create a
small rootOpts struct (e.g. type rootOpts struct { cfgFile, outputFormat,
serverURL string; verbose bool }) or closures returned by RootCmd() that hold
these fields, bind those fields to the persistent flags within RootCmd(), and
change any helpers or subcommand factories to accept *rootOpts (or call opts
getters like opts.GetConfigPath()) instead of relying on the global variables so
each command invocation owns its own state and is testable.
- Around line 20-30: GetConfigPath currently hides errors by returning "" when
gcctlconfig.DefaultConfigPath() fails; change GetConfigPath to return (string,
error) (or otherwise propagate the error) so callers can handle failures
explicitly, update LoadConfig to call GetConfigPath and, if it returns an error,
return that error to the caller rather than calling gcctlconfig.Load(""); ensure
error messages use the original DefaultConfigPath error (e.g., "could not
determine config path: <err>") and update any callers of
GetConfigPath/LoadConfig accordingly so no empty path is passed into
gcctlconfig.Load.
In `@ground-control/cmd/gcctl/root/login.go`:
- Around line 118-120: The CLI currently accepts a --password / -p flag
(variables password and passwordStdin; flags defined on cmd) which exposes
credentials; modify the login command's RunE to detect when the password flag
was explicitly set (check cmd.Flags().Changed("password") or inspect the
password variable) and emit a runtime deprecation/security warning advising
users to use --password-stdin instead; additionally mark the flag deprecated via
cmd.Flags().MarkDeprecated("password", "use --password-stdin instead") so
callers see deprecation metadata (keep username handling unchanged) and ensure
the warning is logged to stderr before proceeding with the existing ping/auth
flow.
- Around line 46-115: The RunE closure in login.go is too long and complex;
extract the credential-gathering and validation logic into a new helper (e.g.,
gatherCredentials or resolveCredentials) that takes cfg and flag-vars (username,
password, passwordStdin) and returns (server, username, password, error); move
calls that resolve server (ResolveServer/promptInput), prompt for username
(promptInput), readStdin (readStdin), prompt for password (promptPassword) and
the final username/password validation into that helper, then replace the big
block in RunE with a call to the helper and retain the remaining
Ping/Login/SaveConfig flow (client.Login, SaveConfig, etc.), propagating errors
unchanged.
In `@ground-control/cmd/gcctl/root/logout.go`:
- Around line 34-41: When ResolveServer(cfg) returns an error and you clear
local creds, don't ignore SaveConfig(cfg) — check its returned error and
propagate or report it instead of discarding it; update the early-exit block in
logout.go (the ResolveServer handling that calls cfg.ClearCredentials and
SaveConfig) to capture err := SaveConfig(cfg) and if non-nil print/return a
clear failure message (or return the error) so the user is not shown "Local
credentials cleared." when the config write actually failed.
In `@ground-control/cmd/gcctl/version/version.go`:
- Around line 8-15: The package-level mutable vars Version, GitCommit, and
BuildDate are intentionally set for ldflags injection but trigger
gochecknoglobals; add a nolint directive to document the exception by annotating
the var block with a comment such as //nolint:gochecknoglobals (placed
immediately above the var (...) block or on the same line) so the linter skips
this specific rule while keeping the existing variable names Version, GitCommit,
and BuildDate and their default values.
---
Nitpick comments:
In `@ground-control/cmd/gcctl/pkg/config/config.go`:
- Around line 35-36: Replace the os.IsNotExist(err) check with errors.Is(err,
fs.ErrNotExist) to correctly handle wrapped errors in the config loading routine
(the branch returning &Config{}, nil), and add the necessary imports ("errors"
and "io/fs") to the file so the compiler resolves errors.Is and fs.ErrNotExist.
In `@ground-control/cmd/gcctl/root/version.go`:
- Around line 13-26: The VersionCommand currently uses Run and prints with
fmt.Printf (silently ignoring write errors) and ignores the CLI output format;
change VersionCommand to use RunE and return errors, build a structured version
payload (fields: Version, GitCommit, BuildDate, GoVersion, OS, Arch) and respect
GetOutputFormat()/--output to emit human-readable text by default or JSON/YAML
when requested; update the command to call GetOutputFormat() inside the RunE
handler, marshal/encode the payload accordingly, and use cmd.PrintErr/
cmd.Print/ writers or returned errors for any write/marshal failures so failures
surface properly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ground-control/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
ground-control/cmd/gcctl/main.goground-control/cmd/gcctl/pkg/api/client.goground-control/cmd/gcctl/pkg/config/config.goground-control/cmd/gcctl/pkg/utils/format.goground-control/cmd/gcctl/root/cmd.goground-control/cmd/gcctl/root/login.goground-control/cmd/gcctl/root/logout.goground-control/cmd/gcctl/root/version.goground-control/cmd/gcctl/root/whoami.goground-control/cmd/gcctl/version/version.goground-control/go.modground-control/internal/middleware/certwatcher.goground-control/internal/server/config_handlers.goground-control/internal/server/routes.goground-control/internal/server/satellite_handlers_test.goground-control/internal/server/server.goground-control/internal/server/whoami_handler.go
💤 Files with no reviewable changes (3)
- ground-control/internal/server/satellite_handlers_test.go
- ground-control/internal/server/server.go
- ground-control/internal/middleware/certwatcher.go
Comment on lines
+54
to
+141
| func (c *Client) Login(username, password string) (*LoginResponse, error) { | ||
| payload := LoginRequest{Username: username, Password: password} | ||
| body, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal login request: %w", err) | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodPost, c.BaseURL+"/login", bytes.NewReader(body)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create login request: %w", err) | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := c.HTTP.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to server: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, parseAPIError(resp) | ||
| } | ||
|
|
||
| var result LoginResponse | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| return nil, fmt.Errorf("failed to decode login response: %w", err) | ||
| } | ||
| return &result, nil | ||
| } | ||
|
|
||
| func (c *Client) Logout() error { | ||
| req, err := c.newAuthRequest(http.MethodPost, "/api/logout", nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| resp, err := c.HTTP.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to connect to server: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusNoContent && resp.StatusCode != http.StatusOK { | ||
| return parseAPIError(resp) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (c *Client) Whoami() (*WhoamiResponse, error) { | ||
| req, err := c.newAuthRequest(http.MethodGet, "/api/whoami", nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resp, err := c.HTTP.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to connect to server: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, parseAPIError(resp) | ||
| } | ||
|
|
||
| var result WhoamiResponse | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| return nil, fmt.Errorf("failed to decode whoami response: %w", err) | ||
| } | ||
| return &result, nil | ||
| } | ||
|
|
||
| func (c *Client) Ping() error { | ||
| req, err := http.NewRequest(http.MethodGet, c.BaseURL+"/ping", nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create ping request: %w", err) | ||
| } | ||
|
|
||
| resp, err := c.HTTP.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("server unreachable: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("server returned status %d", resp.StatusCode) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
All HTTP methods lack context propagation — in-flight requests can't be cancelled.
Every method (Login, Logout, Whoami, Ping, newAuthRequest) uses http.NewRequest, which creates a background-context request. When the user presses Ctrl+C, Cobra cancels cmd.Context(), but the underlying HTTP call continues running until the 30-second client timeout fires. Pass context.Context through each public method and use http.NewRequestWithContext.
🔧 Proposed fix (representative excerpt)
-func (c *Client) Login(username, password string) (*LoginResponse, error) {
+func (c *Client) Login(ctx context.Context, username, password string) (*LoginResponse, error) {
// ...
- req, err := http.NewRequest(http.MethodPost, c.BaseURL+"/login", bytes.NewReader(body))
+ req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.BaseURL+"/login", bytes.NewReader(body))
-func (c *Client) Logout() error {
+func (c *Client) Logout(ctx context.Context) error {
- req, err := c.newAuthRequest(http.MethodPost, "/api/logout", nil)
+ req, err := c.newAuthRequest(ctx, http.MethodPost, "/api/logout", nil)
-func (c *Client) Whoami() (*WhoamiResponse, error) {
+func (c *Client) Whoami(ctx context.Context) (*WhoamiResponse, error) {
- req, err := c.newAuthRequest(http.MethodGet, "/api/whoami", nil)
+ req, err := c.newAuthRequest(ctx, http.MethodGet, "/api/whoami", nil)
-func (c *Client) Ping() error {
+func (c *Client) Ping(ctx context.Context) error {
- req, err := http.NewRequest(http.MethodGet, c.BaseURL+"/ping", nil)
+ req, err := http.NewRequestWithContext(ctx, http.MethodGet, c.BaseURL+"/ping", nil)
-func (c *Client) newAuthRequest(method, path string, body io.Reader) (*http.Request, error) {
+func (c *Client) newAuthRequest(ctx context.Context, method, path string, body io.Reader) (*http.Request, error) {
- req, err := http.NewRequest(method, c.BaseURL+path, body)
+ req, err := http.NewRequestWithContext(ctx, method, c.BaseURL+path, body)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 54 - 141, The public
client methods (Login, Logout, Whoami, Ping) and the helper newAuthRequest
currently create requests with http.NewRequest (background context) so in-flight
HTTP calls can't be cancelled; change each of those method signatures to accept
a context.Context parameter, propagate that context into newAuthRequest and all
http.NewRequestWithContext calls (replace http.NewRequest with
http.NewRequestWithContext(ctx,...)), and ensure newAuthRequest also accepts and
uses the context when creating requests; update any callers to pass
cmd.Context() (or other contexts) through to these methods.
Comment on lines
+155
to
+165
| func parseAPIError(resp *http.Response) error { | ||
| body, _ := io.ReadAll(resp.Body) | ||
|
|
||
| var errResp struct { | ||
| Error string `json:"error"` | ||
| } | ||
| if err := json.Unmarshal(body, &errResp); err == nil && errResp.Error != "" { | ||
| return &APIError{StatusCode: resp.StatusCode, Message: errResp.Error} | ||
| } | ||
|
|
||
| return &APIError{StatusCode: resp.StatusCode, Message: string(body)} |
Contributor
There was a problem hiding this comment.
Silently discarding io.ReadAll error produces empty/misleading error messages.
body, _ := io.ReadAll(resp.Body) suppresses the read error. If the read fails, body is empty, and the resulting APIError carries an empty Message, making error diagnosis impossible.
🔧 Proposed fix
func parseAPIError(resp *http.Response) error {
- body, _ := io.ReadAll(resp.Body)
+ body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return &APIError{StatusCode: resp.StatusCode, Message: fmt.Sprintf("failed to read error body: %v", err)}
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func parseAPIError(resp *http.Response) error { | |
| body, _ := io.ReadAll(resp.Body) | |
| var errResp struct { | |
| Error string `json:"error"` | |
| } | |
| if err := json.Unmarshal(body, &errResp); err == nil && errResp.Error != "" { | |
| return &APIError{StatusCode: resp.StatusCode, Message: errResp.Error} | |
| } | |
| return &APIError{StatusCode: resp.StatusCode, Message: string(body)} | |
| func parseAPIError(resp *http.Response) error { | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return &APIError{StatusCode: resp.StatusCode, Message: fmt.Sprintf("failed to read error body: %v", err)} | |
| } | |
| var errResp struct { | |
| Error string `json:"error"` | |
| } | |
| if err := json.Unmarshal(body, &errResp); err == nil && errResp.Error != "" { | |
| return &APIError{StatusCode: resp.StatusCode, Message: errResp.Error} | |
| } | |
| return &APIError{StatusCode: resp.StatusCode, Message: string(body)} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 155 - 165, The
parseAPIError function currently discards the io.ReadAll error which can produce
empty/misleading APIError messages; update parseAPIError to check the error
returned by io.ReadAll(resp.Body) and, if non-nil, return an APIError containing
the HTTP status and the read error message (or a combined message including the
partial body and read error), otherwise proceed to json.Unmarshal as before and
return the parsed error or the body string; reference the parseAPIError function
and the APIError type when making this change.
Comment on lines
+29
to
+31
| func printYAML(data any) error { | ||
| return yaml.NewEncoder(os.Stdout).Encode(data) | ||
| } |
Contributor
There was a problem hiding this comment.
yaml.Encoder is never closed — potential incomplete YAML output.
yaml.NewEncoder(os.Stdout).Encode(data) discards the encoder immediately; Close() is never called. The encoder uses Close() to flush and write the final ... document-end marker. For certain complex data shapes the output can be silently truncated.
🔧 Proposed fix
func printYAML(data any) error {
- return yaml.NewEncoder(os.Stdout).Encode(data)
+ enc := yaml.NewEncoder(os.Stdout)
+ if err := enc.Encode(data); err != nil {
+ return err
+ }
+ return enc.Close()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func printYAML(data any) error { | |
| return yaml.NewEncoder(os.Stdout).Encode(data) | |
| } | |
| func printYAML(data any) error { | |
| enc := yaml.NewEncoder(os.Stdout) | |
| if err := enc.Encode(data); err != nil { | |
| return err | |
| } | |
| return enc.Close() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/pkg/utils/format.go` around lines 29 - 31, The
printYAML function currently creates a yaml.Encoder and immediately calls Encode
without closing it, risking truncated output; change printYAML (the function) to
create a local encoder via yaml.NewEncoder(os.Stdout), defer or explicitly call
encoder.Close() to ensure the encoder flushes and writes the document end
marker, then call encoder.Encode(data) and return its error (handling any Close
error as appropriate).
Comment on lines
+12
to
+18
| var ( | ||
| // Global flags bound to persistent flags on the root command. | ||
| cfgFile string | ||
| outputFormat string | ||
| serverURL string | ||
| verbose bool | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Package-level mutable variables violate gochecknoglobals.
cfgFile, outputFormat, serverURL, and verbose are package-level globals. The idiomatic fix is to scope them inside RootCmd() and expose the getters as closures — or as methods on a small options struct — so that each command invocation owns its own state (which also makes commands unit-testable without global side-effects).
♻️ Suggested refactor — options struct approach
-var (
- cfgFile string
- outputFormat string
- serverURL string
- verbose bool
-)
-
-func GetConfigPath() string { ... }
-func GetOutputFormat() string { return outputFormat }
-func GetServerURL() string { return serverURL }
+// rootOpts holds flag values for the root command and its children.
+type rootOpts struct {
+ cfgFile string
+ outputFormat string
+ serverURL string
+ verbose bool
+}
+
+func (o *rootOpts) GetConfigPath() string {
+ if o.cfgFile != "" {
+ return o.cfgFile
+ }
+ path, err := gcctlconfig.DefaultConfigPath()
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "Warning: could not determine default config path: %v\n", err)
+ return ""
+ }
+ return path
+}Then inside RootCmd():
func RootCmd() *cobra.Command {
+ opts := &rootOpts{}
root := &cobra.Command{ ... }
- root.PersistentFlags().StringVarP(&cfgFile, "config", ...)
+ root.PersistentFlags().StringVarP(&opts.cfgFile, "config", ...)
// ... wire remaining flags to opts fields ...
- root.AddCommand(LoginCommand())
+ root.AddCommand(LoginCommand(opts))
// ... pass opts to all subcommand factories ...
return root
}Each subcommand factory accepts *rootOpts and uses opts.GetConfigPath(), opts.serverURL, etc., instead of the package-level helpers.
As per coding guidelines, "Enforce strict linting standards via golangci-lint with 50+ linters including no global variables (gochecknoglobals)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/cmd.go` around lines 12 - 18, The package-level
mutable vars cfgFile, outputFormat, serverURL, and verbose should be removed and
scoped inside RootCmd(); create a small rootOpts struct (e.g. type rootOpts
struct { cfgFile, outputFormat, serverURL string; verbose bool }) or closures
returned by RootCmd() that hold these fields, bind those fields to the
persistent flags within RootCmd(), and change any helpers or subcommand
factories to accept *rootOpts (or call opts getters like opts.GetConfigPath())
instead of relying on the global variables so each command invocation owns its
own state and is testable.
Comment on lines
+20
to
+30
| func GetConfigPath() string { | ||
| if cfgFile != "" { | ||
| return cfgFile | ||
| } | ||
| path, err := gcctlconfig.DefaultConfigPath() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Warning: could not determine default config path: %v\n", err) | ||
| return "" | ||
| } | ||
| return path | ||
| } |
Contributor
There was a problem hiding this comment.
GetConfigPath() silently returns "", producing a misleading downstream error.
When gcctlconfig.DefaultConfigPath() fails, GetConfigPath() prints a warning and returns "". The caller — LoadConfig() — then invokes gcctlconfig.Load(""), which will itself fail with a path-related error. The user sees "failed to load config: <open : …>" instead of the clearer "could not determine config path". Propagating the error through LoadConfig avoids this mismatch.
🐛 Proposed fix
-func GetConfigPath() string {
- if cfgFile != "" {
- return cfgFile
- }
- path, err := gcctlconfig.DefaultConfigPath()
- if err != nil {
- fmt.Fprintf(os.Stderr, "Warning: could not determine default config path: %v\n", err)
- return ""
- }
- return path
-}
// LoadConfig is a helper used by subcommands to load the CLI config.
func LoadConfig() (*gcctlconfig.Config, error) {
- return gcctlconfig.Load(GetConfigPath())
+ if cfgFile != "" {
+ return gcctlconfig.Load(cfgFile)
+ }
+ path, err := gcctlconfig.DefaultConfigPath()
+ if err != nil {
+ return nil, fmt.Errorf("could not determine config path: %w", err)
+ }
+ return gcctlconfig.Load(path)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/cmd.go` around lines 20 - 30, GetConfigPath
currently hides errors by returning "" when gcctlconfig.DefaultConfigPath()
fails; change GetConfigPath to return (string, error) (or otherwise propagate
the error) so callers can handle failures explicitly, update LoadConfig to call
GetConfigPath and, if it returns an error, return that error to the caller
rather than calling gcctlconfig.Load(""); ensure error messages use the original
DefaultConfigPath error (e.g., "could not determine config path: <err>") and
update any callers of GetConfigPath/LoadConfig accordingly so no empty path is
passed into gcctlconfig.Load.
Comment on lines
+46
to
+115
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| cfg, err := LoadConfig() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load config: %w", err) | ||
| } | ||
|
|
||
| // Resolve server URL | ||
| server, err := ResolveServer(cfg) | ||
| if err != nil { | ||
| // Neither flag nor config has a server; prompt interactively | ||
| server, err = promptInput("Ground Control Server URL") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Prompt for username if not provided via flag | ||
| if username == "" { | ||
| username, err = promptInput("Username") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Read password from stdin if --password-stdin is set | ||
| if passwordStdin { | ||
| password, err = readStdin() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read password from stdin: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // Prompt for password if still empty | ||
| if password == "" { | ||
| password, err = promptPassword("Password") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| if username == "" || password == "" { | ||
| return fmt.Errorf("username and password are required") | ||
| } | ||
|
|
||
| // Ping the server first to check connectivity | ||
| client := api.NewClient(server, "") | ||
| if err := client.Ping(); err != nil { | ||
| return fmt.Errorf("cannot reach server %s: %w", server, err) | ||
| } | ||
|
|
||
| // Authenticate | ||
| resp, err := client.Login(username, password) | ||
| if err != nil { | ||
| return fmt.Errorf("login failed: %w", err) | ||
| } | ||
|
|
||
| // Save credentials to config file | ||
| cfg.Server = server | ||
| cfg.Token = resp.Token | ||
| cfg.ExpiresAt = resp.ExpiresAt | ||
| cfg.Username = username | ||
|
|
||
| if err := SaveConfig(cfg); err != nil { | ||
| return fmt.Errorf("login succeeded but failed to save config: %w", err) | ||
| } | ||
|
|
||
| fmt.Printf("Logged in as %s to %s\n", username, server) | ||
| fmt.Printf(" Token expires: %s\n", resp.ExpiresAt) | ||
| return nil | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
RunE closure violates function-length and cyclomatic-complexity limits.
Static analysis flags it at 52 lines (limit 50) and cyclomatic complexity 15 (limit 8). Extract the credential-gathering logic into a dedicated helper to bring both metrics inside the allowed bounds.
♻️ Suggested extraction
+// resolveCredentials collects and validates server URL, username, and password
+// from flags, config, stdin, or interactive prompts.
+func resolveCredentials(
+ cfg *gcctlconfig.Config,
+ username, password string,
+ passwordStdin bool,
+) (server, user, pass string, err error) {
+ server, err = ResolveServer(cfg)
+ if err != nil {
+ server, err = promptInput("Ground Control Server URL")
+ if err != nil {
+ return
+ }
+ }
+ if username == "" {
+ username, err = promptInput("Username")
+ if err != nil {
+ return
+ }
+ }
+ if passwordStdin {
+ password, err = readStdin()
+ if err != nil {
+ err = fmt.Errorf("failed to read password from stdin: %w", err)
+ return
+ }
+ }
+ if password == "" {
+ password, err = promptPassword("Password")
+ if err != nil {
+ return
+ }
+ }
+ if username == "" || password == "" {
+ err = fmt.Errorf("username and password are required")
+ return
+ }
+ return server, username, password, nil
+}Then the RunE body becomes:
RunE: func(cmd *cobra.Command, args []string) error {
cfg, err := LoadConfig()
if err != nil {
return fmt.Errorf("failed to load config: %w", err)
}
- // ... (all credential-gathering branches)
+ server, username, password, err := resolveCredentials(cfg, username, password, passwordStdin)
+ if err != nil {
+ return err
+ }
client := api.NewClient(server, "")
if err := client.Ping(); err != nil {
return fmt.Errorf("cannot reach server %s: %w", server, err)
}
resp, err := client.Login(username, password)
if err != nil {
return fmt.Errorf("login failed: %w", err)
}
cfg.Server = server
cfg.Token = resp.Token
cfg.ExpiresAt = resp.ExpiresAt
cfg.Username = username
if err := SaveConfig(cfg); err != nil {
return fmt.Errorf("login succeeded but failed to save config: %w", err)
}
fmt.Printf("Logged in as %s to %s\n", username, server)
fmt.Printf(" Token expires: %s\n", resp.ExpiresAt)
return nil
},🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 46-46: ground-control/cmd/gcctl/root/login.go#L46
Method has 52 lines of code (limit is 50)
[warning] 46-46: ground-control/cmd/gcctl/root/login.go#L46
Method has a cyclomatic complexity of 15 (limit is 8)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/login.go` around lines 46 - 115, The RunE
closure in login.go is too long and complex; extract the credential-gathering
and validation logic into a new helper (e.g., gatherCredentials or
resolveCredentials) that takes cfg and flag-vars (username, password,
passwordStdin) and returns (server, username, password, error); move calls that
resolve server (ResolveServer/promptInput), prompt for username (promptInput),
readStdin (readStdin), prompt for password (promptPassword) and the final
username/password validation into that helper, then replace the big block in
RunE with a call to the helper and retain the remaining Ping/Login/SaveConfig
flow (client.Login, SaveConfig, etc.), propagating errors unchanged.
Comment on lines
+118
to
+120
| cmd.Flags().StringVarP(&username, "username", "u", "", "username for authentication") | ||
| cmd.Flags().StringVarP(&password, "password", "p", "", "password (prefer --password-stdin)") | ||
| cmd.Flags().BoolVar(&passwordStdin, "password-stdin", false, "read password from stdin") |
Contributor
There was a problem hiding this comment.
--password / -p flag exposes credentials in the process list and shell history.
Passing a password as a CLI flag value makes it visible in ps aux output, shell history files, and system audit logs — a well-known security risk in CLI tools. The comment "prefer --password-stdin" acknowledges this but provides no enforcement. At minimum, emit a visible warning when -p is used; ideally, deprecate the flag so scripts migrate to --password-stdin.
🔒️ Suggested fix — runtime deprecation warning
- cmd.Flags().StringVarP(&password, "password", "p", "", "password (prefer --password-stdin)")
+ cmd.Flags().StringVarP(&password, "password", "p", "", "password (DEPRECATED: use --password-stdin to avoid exposing credentials in your shell history)")And inside RunE, before the ping call:
+ if cmd.Flags().Changed("password") {
+ fmt.Fprintf(cmd.ErrOrStderr(),
+ "WARNING: Using --password via CLI is insecure. Use --password-stdin instead.\n")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/login.go` around lines 118 - 120, The CLI
currently accepts a --password / -p flag (variables password and passwordStdin;
flags defined on cmd) which exposes credentials; modify the login command's RunE
to detect when the password flag was explicitly set (check
cmd.Flags().Changed("password") or inspect the password variable) and emit a
runtime deprecation/security warning advising users to use --password-stdin
instead; additionally mark the flag deprecated via
cmd.Flags().MarkDeprecated("password", "use --password-stdin instead") so
callers see deprecation metadata (keep username handling unchanged) and ensure
the warning is logged to stderr before proceeding with the existing ping/auth
flow.
Comment on lines
+34
to
+41
| server, err := ResolveServer(cfg) | ||
| if err != nil { | ||
| // No server known — just clear local state | ||
| cfg.ClearCredentials() | ||
| _ = SaveConfig(cfg) | ||
| fmt.Println("Local credentials cleared.") | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Silent SaveConfig error gives a false "cleared" message.
When ResolveServer fails and the code takes this early-exit path, a failed config write is silently swallowed (_ = SaveConfig(cfg)). The user sees "Local credentials cleared." but the stale token may still be on disk — inconsistent with the proper error handling on lines 52–54.
🐛 Proposed fix
- cfg.ClearCredentials()
- _ = SaveConfig(cfg)
- fmt.Println("Local credentials cleared.")
- return nil
+ cfg.ClearCredentials()
+ if err := SaveConfig(cfg); err != nil {
+ return fmt.Errorf("failed to save config: %w", err)
+ }
+ fmt.Println("Local credentials cleared.")
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| server, err := ResolveServer(cfg) | |
| if err != nil { | |
| // No server known — just clear local state | |
| cfg.ClearCredentials() | |
| _ = SaveConfig(cfg) | |
| fmt.Println("Local credentials cleared.") | |
| return nil | |
| } | |
| server, err := ResolveServer(cfg) | |
| if err != nil { | |
| // No server known — just clear local state | |
| cfg.ClearCredentials() | |
| if err := SaveConfig(cfg); err != nil { | |
| return fmt.Errorf("failed to save config: %w", err) | |
| } | |
| fmt.Println("Local credentials cleared.") | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/logout.go` around lines 34 - 41, When
ResolveServer(cfg) returns an error and you clear local creds, don't ignore
SaveConfig(cfg) — check its returned error and propagate or report it instead of
discarding it; update the early-exit block in logout.go (the ResolveServer
handling that calls cfg.ClearCredentials and SaveConfig) to capture err :=
SaveConfig(cfg) and if non-nil print/return a clear failure message (or return
the error) so the user is not shown "Local credentials cleared." when the config
write actually failed.
Comment on lines
+8
to
+15
| var ( | ||
| // Version is the semantic version (set via ldflags). | ||
| Version = "dev" | ||
| // GitCommit is the git commit hash (set via ldflags). | ||
| GitCommit = "unknown" | ||
| // BuildDate is the build timestamp (set via ldflags). | ||
| BuildDate = "unknown" | ||
| ) |
Contributor
There was a problem hiding this comment.
Package-level vars will be flagged by gochecknoglobals.
Version, GitCommit, and BuildDate are package-level mutable variables. While -ldflags injection requires this pattern, the enforced gochecknoglobals linter will still reject them. Add a //nolint directive to document the intentional exception.
🔧 Proposed fix
-var (
+//nolint:gochecknoglobals // set via -ldflags at build time
+var (
// Version is the semantic version (set via ldflags).
Version = "dev"
// GitCommit is the git commit hash (set via ldflags).
GitCommit = "unknown"
// BuildDate is the build timestamp (set via ldflags).
BuildDate = "unknown"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var ( | |
| // Version is the semantic version (set via ldflags). | |
| Version = "dev" | |
| // GitCommit is the git commit hash (set via ldflags). | |
| GitCommit = "unknown" | |
| // BuildDate is the build timestamp (set via ldflags). | |
| BuildDate = "unknown" | |
| ) | |
| //nolint:gochecknoglobals // set via -ldflags at build time | |
| var ( | |
| // Version is the semantic version (set via ldflags). | |
| Version = "dev" | |
| // GitCommit is the git commit hash (set via ldflags). | |
| GitCommit = "unknown" | |
| // BuildDate is the build timestamp (set via ldflags). | |
| BuildDate = "unknown" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/version/version.go` around lines 8 - 15, The
package-level mutable vars Version, GitCommit, and BuildDate are intentionally
set for ldflags injection but trigger gochecknoglobals; add a nolint directive
to document the exception by annotating the var block with a comment such as
//nolint:gochecknoglobals (placed immediately above the var (...) block or on
the same line) so the linter skips this specific rule while keeping the existing
variable names Version, GitCommit, and BuildDate and their default values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
ground-control/cmd/gcctl/root/logout.go (1)
34-41:SaveConfigerror still silently discarded in the early-exit path.The previous review flagged that
_ = SaveConfig(cfg)on Line 38 gives a false "Local credentials cleared." message when the config write actually fails. This remains unfixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/logout.go` around lines 34 - 41, The early-exit branch in logout.go currently clears credentials and discards SaveConfig(cfg) errors; change it so after calling cfg.ClearCredentials() you check the error returned by SaveConfig(cfg) and handle it (e.g. log/print an error and return the error) instead of ignoring it; update the block around ResolveServer, cfg.ClearCredentials and SaveConfig to capture SaveConfig's error, only print "Local credentials cleared." on success, and return the SaveConfig error when it fails.ground-control/cmd/gcctl/pkg/utils/format.go (1)
29-31:yaml.Encoderstill not closed — potential truncated output.The previous review flagged that
yaml.NewEncoder(os.Stdout).Encode(data)never callsClose(), which is needed to flush and write the document-end marker. This remains unfixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/utils/format.go` around lines 29 - 31, The printYAML function uses yaml.NewEncoder(os.Stdout) but never closes the encoder; change printYAML to create the encoder in a local variable (via yaml.NewEncoder(os.Stdout)), defer encoder.Close() so Close() is always called to flush and write the document-end marker, then call encoder.Encode(data) and return its error; reference yaml.NewEncoder, encoder.Close, and printYAML to locate the change.ground-control/cmd/gcctl/version/version.go (1)
8-15: Package-level vars still missing//nolint:gochecknoglobalsdirective.The previous review flagged that these
vardeclarations will be rejected by the enforcedgochecknoglobalslinter. The//nolintdirective to document the intentional exception for-ldflagsinjection is still missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/version/version.go` around lines 8 - 15, Add a package-level nolint to document the intentional global vars used for ldflags: annotate the var block containing Version, GitCommit, and BuildDate with a "//nolint:gochecknoglobals" directive so the linter knows these globals are intentional; update the comment above the Version/GitCommit/BuildDate var block to include that directive and keep the existing LDFlags explanation.ground-control/cmd/gcctl/root/login.go (2)
46-115:RunEclosure still exceeds complexity and length limits.The previous review flagged that static analysis reports cyclomatic complexity of 15 (limit 8) and 52 lines (limit 50). The suggestion to extract credential-gathering into a
resolveCredentialshelper remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/login.go` around lines 46 - 115, The RunE closure is too long/complex; extract all credential- and server-resolution logic into a new helper resolveCredentials that takes the loaded cfg plus the current flags (username, password, passwordStdin) and returns (server, username, password, error). Move the flow that calls ResolveServer, prompts via promptInput/promptPassword, handles --password-stdin via readStdin, and validates non-empty username/password into resolveCredentials, leaving RunE to just LoadConfig, call resolveCredentials, then Ping and Login and SaveConfig; update RunE to call resolveCredentials(cfg, username, password, passwordStdin) and handle the returned error.
118-120:--passwordflag still exposes credentials without a deprecation warning.The previous review flagged that passing a password via
-pflag exposes it in process lists and shell history. The suggestion to emit a runtime warning or mark the flag as deprecated remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/login.go` around lines 118 - 120, Mark the --password/-p flag as deprecated and emit a runtime warning when it is used: call cmd.Flags().MarkDeprecated("password", "use --password-stdin instead") after the existing cmd.Flags().StringVarP(...) and, in the login command execution path (where username, password, and passwordStdin are used), check if password != "" and passwordStdin == false and write a clear warning to stderr (e.g., via fmt.Fprintln(os.Stderr, ...)) advising to use --password-stdin; this ensures both compile-time deprecation metadata and a runtime warning when the unsafe flag is actually provided.ground-control/cmd/gcctl/pkg/api/client.go (2)
155-165:io.ReadAllerror still silently discarded inparseAPIError.The previous review flagged that
body, _ := io.ReadAll(resp.Body)on Line 156 swallows the read error, producing empty/misleadingAPIErrormessages. This remains unfixed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 155 - 165, In parseAPIError, stop discarding the io.ReadAll error: read the response body with body, err := io.ReadAll(resp.Body) and if err != nil return an APIError that includes the read error (e.g., "failed to read response body: <err>") so callers see the real failure; then continue unmarshalling into the existing errResp and return the parsed error string or the raw body as before (referencing parseAPIError, resp.Body, and APIError).
54-141: All HTTP methods still lackcontext.Contextpropagation.The previous review flagged that
Login,Logout,Whoami,Ping, andnewAuthRequestall usehttp.NewRequestinstead ofhttp.NewRequestWithContext. This remains unfixed — in-flight requests cannot be cancelled when the user presses Ctrl+C.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 54 - 141, Change the affected methods to accept and propagate a context.Context and use http.NewRequestWithContext: update Login, Logout, Whoami, Ping (and the helper newAuthRequest) to add a ctx parameter, replace http.NewRequest calls with http.NewRequestWithContext(ctx, ...), and pass ctx through when calling newAuthRequest and any callers of these methods so in-flight requests can be cancelled (adjust call sites to supply the caller's context).ground-control/cmd/gcctl/root/cmd.go (2)
20-30:GetConfigPath()silently swallows errors — see prior review for the propagation fix viaLoadConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/cmd.go` around lines 20 - 30, GetConfigPath is swallowing errors by printing a warning and returning an empty string; change it to propagate the error instead (match the pattern used by LoadConfig) by updating GetConfigPath to return (string, error), keep the cfgFile fast-path, call gcctlconfig.DefaultConfigPath() and return its (path, err) directly when cfgFile is empty, and update all callers to handle the returned error accordingly; reference symbols: GetConfigPath, cfgFile, gcctlconfig.DefaultConfigPath, and LoadConfig.
12-18: Package-level mutable variables violategochecknoglobals— see prior review for therootOptsstruct refactor suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/cmd.go` around lines 12 - 18, The package-level mutable vars cfgFile, outputFormat, serverURL, and verbose should be removed and folded into a rootOpts struct instance; create a rootOpts type (if not present) and add fields for CfgFile, OutputFormat, ServerURL, and Verbose, then instantiate that struct in the root command constructor (e.g., NewRootCmd or where the root cobra.Command is built) and bind persistent flags to the struct fields instead of the package vars; finally delete the package-level variables and update any references to use the rootOpts instance.
🧹 Nitpick comments (4)
ground-control/cmd/gcctl/root/whoami.go (1)
60-73: Thedefaultcase implicitly accepts any format string as "table" output.If
GetOutputFormat()returns an invalid string (e.g.,"xml"), thedefaultbranch silently renders table output instead of reporting an error. This is inconsistent withPrintFormat, which returns an error for unsupported formats. Consider explicitly matching"table", ""and erroring on the rest.♻️ Proposed fix
switch format { case "json", "yaml": return utils.PrintFormat(display, format) - default: + case "table", "": // Default table/key-value output utils.PrintKeyValue([][]string{ {"Username", display.Username}, {"Role", display.Role}, {"Server", display.Server}, {"Token Expires", display.ExpiresAt}, }) return nil + default: + return fmt.Errorf("unsupported output format: %q (use table, json, or yaml)", format) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/root/whoami.go` around lines 60 - 73, The switch on GetOutputFormat() in whoami.go currently treats any unknown format as table output; change it to explicitly handle "json"/"yaml" (call utils.PrintFormat(display, format)), "table" and "" (call utils.PrintKeyValue(...) and return nil), and add a default branch that returns an error for unsupported formats (e.g., return fmt.Errorf("unsupported output format: %s", format)) so invalid strings like "xml" are reported; update references in the function accordingly (GetOutputFormat, utils.PrintFormat, utils.PrintKeyValue).ground-control/cmd/gcctl/pkg/utils/format.go (1)
33-41:tabwriter.Flush()error is discarded.
w.Flush()returns an error that is silently ignored. If writing to stdout fails (e.g., broken pipe), the caller won't know.♻️ Proposed fix
-func PrintKeyValue(pairs [][]string) { +func PrintKeyValue(pairs [][]string) error { w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) for _, pair := range pairs { if len(pair) == 2 { - fmt.Fprintf(w, "%s:\t%s\n", pair[0], pair[1]) + if _, err := fmt.Fprintf(w, "%s:\t%s\n", pair[0], pair[1]); err != nil { + return err + } } } - w.Flush() + return w.Flush() }Note: This would require updating the caller in
whoami.go(Line 66) to handle the returned error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/utils/format.go` around lines 33 - 41, PrintKeyValue currently ignores the error from w.Flush(); change PrintKeyValue to propagate that error (update its signature to return error), call w.Flush() and return any non-nil error, and update all callers (notably the caller in whoami.go that invokes PrintKeyValue) to handle the returned error so write/flush failures (e.g., broken pipe) are not silently dropped.ground-control/cmd/gcctl/pkg/api/client.go (1)
13-17: Consider restrictingClientfield visibility.All three fields (
BaseURL,Token,HTTP) are exported.TokenandHTTPbeing public means callers can freely mutate them after construction, which could lead to subtle bugs (e.g., replacing the HTTP client mid-flight). If there's no external need, makeTokenandHTTPunexported.♻️ Proposed change
type Client struct { BaseURL string - Token string - HTTP *http.Client + token string + http *http.Client }Then update internal references accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 13 - 17, The Client struct currently exports BaseURL, Token, and HTTP; make Token and HTTP unexported to prevent external mutation by renaming them (e.g., Token -> token, HTTP -> httpClient) while keeping BaseURL exported if needed, then update all internal references to Client.token and Client.httpClient across functions/methods (constructors, request builders, and tests) and add accessor/setter functions if external read/write is required (e.g., Token() string or SetToken(string)). Ensure any uses of Client.HTTP in methods like Do/Request or in tests are updated to the new field name.ground-control/cmd/gcctl/pkg/config/config.go (1)
16-22: Token stored as plaintext in config file — acceptable but worth documenting.The token is persisted as plaintext YAML at
~/.gcctl/config.yaml. While the file permissions (0600) mitigate casual exposure and this pattern is standard for CLI tools (Docker, kubectl, gh), consider adding a brief comment in the config file or docs noting that the token is stored unencrypted, so users on shared machines are aware.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ground-control/cmd/gcctl/pkg/config/config.go` around lines 16 - 22, The Config struct persists the auth token in plaintext; update the code that serializes/writes the Config (the function responsible for saving the YAML) to prepend a short human-readable comment to the top of the generated config file stating that the token is stored unencrypted and to secure the file (e.g., permissions 0600), and also add the same note to the package/CLI documentation or README so users are aware; reference the Config type when locating the serializer/save function to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ground-control/cmd/gcctl/pkg/api/client.go`:
- Around line 19-27: NewClient currently accepts any baseURL and may send
credentials over plain HTTP; update NewClient (and/or a helper like
validateBaseURL) to parse baseURL, require scheme "https" (allow "http" only for
localhost/127.0.0.1 and optional explicit dev flag), and return an error or
panic when non-HTTPS is provided; trim and normalize the URL as before (using
strings.TrimRight) only after validation, and include references to
Client.BaseURL and NewClient in the check so callers cannot instantiate a Client
that will send credentials over plain HTTP.
In `@ground-control/cmd/gcctl/root/cmd.go`:
- Around line 101-108: ResolveServer can panic when cfg is nil because it
accesses cfg.Server; add a nil check before referencing cfg.Server. Modify
ResolveServer to first call GetServerURL() as it does, then check if cfg != nil
and cfg.Server != "" before returning cfg.Server; otherwise return the existing
error. Ensure you update only the ResolveServer function (referenced symbols:
ResolveServer, GetServerURL, cfg.Server) so callers that pass nil configs no
longer dereference cfg.
In `@ground-control/cmd/gcctl/root/login.go`:
- Around line 125-134: promptInput and readStdin each create their own
bufio.NewReader(os.Stdin), which can cause buffered bytes to be consumed by one
reader and lost to the other; update the code to use a single shared reader or
scanner for the command (e.g., create one bufio.Reader or bufio.Scanner in the
login command setup and pass it into promptInput and readStdin) so both
functions read from the same stream; modify promptInput(label string) and
readStdin(...) signatures to accept the shared reader/scanner (or close over it)
and ensure all line reads use that shared instance.
---
Duplicate comments:
In `@ground-control/cmd/gcctl/pkg/api/client.go`:
- Around line 155-165: In parseAPIError, stop discarding the io.ReadAll error:
read the response body with body, err := io.ReadAll(resp.Body) and if err != nil
return an APIError that includes the read error (e.g., "failed to read response
body: <err>") so callers see the real failure; then continue unmarshalling into
the existing errResp and return the parsed error string or the raw body as
before (referencing parseAPIError, resp.Body, and APIError).
- Around line 54-141: Change the affected methods to accept and propagate a
context.Context and use http.NewRequestWithContext: update Login, Logout,
Whoami, Ping (and the helper newAuthRequest) to add a ctx parameter, replace
http.NewRequest calls with http.NewRequestWithContext(ctx, ...), and pass ctx
through when calling newAuthRequest and any callers of these methods so
in-flight requests can be cancelled (adjust call sites to supply the caller's
context).
In `@ground-control/cmd/gcctl/pkg/utils/format.go`:
- Around line 29-31: The printYAML function uses yaml.NewEncoder(os.Stdout) but
never closes the encoder; change printYAML to create the encoder in a local
variable (via yaml.NewEncoder(os.Stdout)), defer encoder.Close() so Close() is
always called to flush and write the document-end marker, then call
encoder.Encode(data) and return its error; reference yaml.NewEncoder,
encoder.Close, and printYAML to locate the change.
In `@ground-control/cmd/gcctl/root/cmd.go`:
- Around line 20-30: GetConfigPath is swallowing errors by printing a warning
and returning an empty string; change it to propagate the error instead (match
the pattern used by LoadConfig) by updating GetConfigPath to return (string,
error), keep the cfgFile fast-path, call gcctlconfig.DefaultConfigPath() and
return its (path, err) directly when cfgFile is empty, and update all callers to
handle the returned error accordingly; reference symbols: GetConfigPath,
cfgFile, gcctlconfig.DefaultConfigPath, and LoadConfig.
- Around line 12-18: The package-level mutable vars cfgFile, outputFormat,
serverURL, and verbose should be removed and folded into a rootOpts struct
instance; create a rootOpts type (if not present) and add fields for CfgFile,
OutputFormat, ServerURL, and Verbose, then instantiate that struct in the root
command constructor (e.g., NewRootCmd or where the root cobra.Command is built)
and bind persistent flags to the struct fields instead of the package vars;
finally delete the package-level variables and update any references to use the
rootOpts instance.
In `@ground-control/cmd/gcctl/root/login.go`:
- Around line 46-115: The RunE closure is too long/complex; extract all
credential- and server-resolution logic into a new helper resolveCredentials
that takes the loaded cfg plus the current flags (username, password,
passwordStdin) and returns (server, username, password, error). Move the flow
that calls ResolveServer, prompts via promptInput/promptPassword, handles
--password-stdin via readStdin, and validates non-empty username/password into
resolveCredentials, leaving RunE to just LoadConfig, call resolveCredentials,
then Ping and Login and SaveConfig; update RunE to call resolveCredentials(cfg,
username, password, passwordStdin) and handle the returned error.
- Around line 118-120: Mark the --password/-p flag as deprecated and emit a
runtime warning when it is used: call cmd.Flags().MarkDeprecated("password",
"use --password-stdin instead") after the existing cmd.Flags().StringVarP(...)
and, in the login command execution path (where username, password, and
passwordStdin are used), check if password != "" and passwordStdin == false and
write a clear warning to stderr (e.g., via fmt.Fprintln(os.Stderr, ...))
advising to use --password-stdin; this ensures both compile-time deprecation
metadata and a runtime warning when the unsafe flag is actually provided.
In `@ground-control/cmd/gcctl/root/logout.go`:
- Around line 34-41: The early-exit branch in logout.go currently clears
credentials and discards SaveConfig(cfg) errors; change it so after calling
cfg.ClearCredentials() you check the error returned by SaveConfig(cfg) and
handle it (e.g. log/print an error and return the error) instead of ignoring it;
update the block around ResolveServer, cfg.ClearCredentials and SaveConfig to
capture SaveConfig's error, only print "Local credentials cleared." on success,
and return the SaveConfig error when it fails.
In `@ground-control/cmd/gcctl/version/version.go`:
- Around line 8-15: Add a package-level nolint to document the intentional
global vars used for ldflags: annotate the var block containing Version,
GitCommit, and BuildDate with a "//nolint:gochecknoglobals" directive so the
linter knows these globals are intentional; update the comment above the
Version/GitCommit/BuildDate var block to include that directive and keep the
existing LDFlags explanation.
---
Nitpick comments:
In `@ground-control/cmd/gcctl/pkg/api/client.go`:
- Around line 13-17: The Client struct currently exports BaseURL, Token, and
HTTP; make Token and HTTP unexported to prevent external mutation by renaming
them (e.g., Token -> token, HTTP -> httpClient) while keeping BaseURL exported
if needed, then update all internal references to Client.token and
Client.httpClient across functions/methods (constructors, request builders, and
tests) and add accessor/setter functions if external read/write is required
(e.g., Token() string or SetToken(string)). Ensure any uses of Client.HTTP in
methods like Do/Request or in tests are updated to the new field name.
In `@ground-control/cmd/gcctl/pkg/config/config.go`:
- Around line 16-22: The Config struct persists the auth token in plaintext;
update the code that serializes/writes the Config (the function responsible for
saving the YAML) to prepend a short human-readable comment to the top of the
generated config file stating that the token is stored unencrypted and to secure
the file (e.g., permissions 0600), and also add the same note to the package/CLI
documentation or README so users are aware; reference the Config type when
locating the serializer/save function to modify.
In `@ground-control/cmd/gcctl/pkg/utils/format.go`:
- Around line 33-41: PrintKeyValue currently ignores the error from w.Flush();
change PrintKeyValue to propagate that error (update its signature to return
error), call w.Flush() and return any non-nil error, and update all callers
(notably the caller in whoami.go that invokes PrintKeyValue) to handle the
returned error so write/flush failures (e.g., broken pipe) are not silently
dropped.
In `@ground-control/cmd/gcctl/root/whoami.go`:
- Around line 60-73: The switch on GetOutputFormat() in whoami.go currently
treats any unknown format as table output; change it to explicitly handle
"json"/"yaml" (call utils.PrintFormat(display, format)), "table" and "" (call
utils.PrintKeyValue(...) and return nil), and add a default branch that returns
an error for unsupported formats (e.g., return fmt.Errorf("unsupported output
format: %s", format)) so invalid strings like "xml" are reported; update
references in the function accordingly (GetOutputFormat, utils.PrintFormat,
utils.PrintKeyValue).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ground-control/cmd/gcctl/pkg/api/client.goground-control/cmd/gcctl/pkg/config/config.goground-control/cmd/gcctl/pkg/utils/format.goground-control/cmd/gcctl/root/cmd.goground-control/cmd/gcctl/root/login.goground-control/cmd/gcctl/root/logout.goground-control/cmd/gcctl/root/version.goground-control/cmd/gcctl/root/whoami.goground-control/cmd/gcctl/version/version.goground-control/internal/middleware/certwatcher.goground-control/internal/server/config_handlers.goground-control/internal/server/satellite_handlers_test.goground-control/internal/server/server.goground-control/internal/server/whoami_handler.go
💤 Files with no reviewable changes (3)
- ground-control/internal/server/server.go
- ground-control/internal/middleware/certwatcher.go
- ground-control/internal/server/satellite_handlers_test.go
✅ Files skipped from review due to trivial changes (1)
- ground-control/internal/server/config_handlers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ground-control/internal/server/whoami_handler.go
Comment on lines
+19
to
+27
| func NewClient(baseURL, token string) *Client { | ||
| return &Client{ | ||
| BaseURL: strings.TrimRight(baseURL, "/"), | ||
| Token: token, | ||
| HTTP: &http.Client{ | ||
| Timeout: 30 * time.Second, | ||
| }, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
No scheme validation on baseURL — credentials may be sent over plain HTTP.
NewClient trims trailing slashes but never validates the URL scheme. If a user configures http://... instead of https://..., the login password and bearer token are transmitted in cleartext. Consider warning or rejecting non-HTTPS URLs (at least outside localhost).
🔒️ Proposed validation
func NewClient(baseURL, token string) *Client {
+ if !strings.HasPrefix(baseURL, "https://") &&
+ !strings.HasPrefix(baseURL, "http://localhost") &&
+ !strings.HasPrefix(baseURL, "http://127.0.0.1") {
+ fmt.Fprintf(os.Stderr, "WARNING: server URL %q is not HTTPS; credentials may be transmitted in cleartext\n", baseURL)
+ }
return &Client{
BaseURL: strings.TrimRight(baseURL, "/"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/pkg/api/client.go` around lines 19 - 27, NewClient
currently accepts any baseURL and may send credentials over plain HTTP; update
NewClient (and/or a helper like validateBaseURL) to parse baseURL, require
scheme "https" (allow "http" only for localhost/127.0.0.1 and optional explicit
dev flag), and return an error or panic when non-HTTPS is provided; trim and
normalize the URL as before (using strings.TrimRight) only after validation, and
include references to Client.BaseURL and NewClient in the check so callers
cannot instantiate a Client that will send credentials over plain HTTP.
Comment on lines
+101
to
+108
| func ResolveServer(cfg *gcctlconfig.Config) (string, error) { | ||
| if s := GetServerURL(); s != "" { | ||
| return s, nil | ||
| } | ||
| if cfg.Server != "" { | ||
| return cfg.Server, nil | ||
| } | ||
| return "", fmt.Errorf("no server configured; use --server flag or run 'gcctl login'") |
Contributor
There was a problem hiding this comment.
Nil pointer dereference in ResolveServer when cfg is nil.
If a caller passes nil for cfg (e.g., when LoadConfig() returns an error on a fresh machine but the user supplied --server), line 105 accesses cfg.Server and panics. The --server flag path on line 102-103 correctly short-circuits, but the nil guard is missing for the fallthrough.
🛡️ Proposed fix
func ResolveServer(cfg *gcctlconfig.Config) (string, error) {
if s := GetServerURL(); s != "" {
return s, nil
}
- if cfg.Server != "" {
+ if cfg != nil && cfg.Server != "" {
return cfg.Server, nil
}
return "", fmt.Errorf("no server configured; use --server flag or run 'gcctl login'")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func ResolveServer(cfg *gcctlconfig.Config) (string, error) { | |
| if s := GetServerURL(); s != "" { | |
| return s, nil | |
| } | |
| if cfg.Server != "" { | |
| return cfg.Server, nil | |
| } | |
| return "", fmt.Errorf("no server configured; use --server flag or run 'gcctl login'") | |
| func ResolveServer(cfg *gcctlconfig.Config) (string, error) { | |
| if s := GetServerURL(); s != "" { | |
| return s, nil | |
| } | |
| if cfg != nil && cfg.Server != "" { | |
| return cfg.Server, nil | |
| } | |
| return "", fmt.Errorf("no server configured; use --server flag or run 'gcctl login'") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/cmd.go` around lines 101 - 108, ResolveServer
can panic when cfg is nil because it accesses cfg.Server; add a nil check before
referencing cfg.Server. Modify ResolveServer to first call GetServerURL() as it
does, then check if cfg != nil and cfg.Server != "" before returning cfg.Server;
otherwise return the existing error. Ensure you update only the ResolveServer
function (referenced symbols: ResolveServer, GetServerURL, cfg.Server) so
callers that pass nil configs no longer dereference cfg.
Comment on lines
+125
to
+134
| // promptInput reads a line of text from stdin with the given prompt label. | ||
| func promptInput(label string) (string, error) { | ||
| fmt.Printf("%s: ", label) | ||
| reader := bufio.NewReader(os.Stdin) | ||
| input, err := reader.ReadString('\n') | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read input: %w", err) | ||
| } | ||
| return strings.TrimSpace(input), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Repeated bufio.NewReader(os.Stdin) could silently consume buffered input.
promptInput and readStdin each create a fresh bufio.NewReader(os.Stdin). If promptInput is called for the server URL and then again for the username, the first reader may buffer bytes beyond the newline that the second reader will never see. In practice this is unlikely with line-at-a-time terminal input, but in piped/scripted scenarios (e.g., printf "url\nuser\n" | gcctl login --password-stdin) it can silently lose input.
Consider sharing a single bufio.Reader across the command's lifetime or using bufio.Scanner line-by-line on a shared instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ground-control/cmd/gcctl/root/login.go` around lines 125 - 134, promptInput
and readStdin each create their own bufio.NewReader(os.Stdin), which can cause
buffered bytes to be consumed by one reader and lost to the other; update the
code to use a single shared reader or scanner for the command (e.g., create one
bufio.Reader or bufio.Scanner in the login command setup and pass it into
promptInput and readStdin) so both functions read from the same stream; modify
promptInput(label string) and readStdin(...) signatures to accept the shared
reader/scanner (or close over it) and ensure all line reads use that shared
instance.
Author
|
Hi @bupd . This PR is working completely right now. PTAL once :) |
Author
|
Fixing the coderabbit suggestions.... |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
gcctlstructure for GCgcctl login,gcctl logout,gcctl whoami,gcctl versioncommands.whoamiapi handler for GC~/.gcctl/config.yaml:Screenshots of Working
Testing the
logincommand :usersapi, which requires authentication .TOKENto verify that user is logged in.Testing
whoamicommandTesting the
logoutcommandTesting the
versioncommandSummary by cubic
Adds gcctl, a Ground Control CLI for authentication with login, logout, whoami, and version commands. Stores local config and supports JSON/YAML/table outputs, and adds /api/whoami to support the CLI (addresses #247).
New Features
Dependencies
Written for commit 5c75258. Summary will update on new commits.
Summary by CodeRabbit