chore: uniform context usage in commands; some unused parameters removed#82
Merged
skevetter merged 7 commits intoskevetter:mainfrom Mar 12, 2026
Merged
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors CLI commands to use cobra command contexts and remove env-based machine provider retrieval; adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "Cobra CLI\n(NewDescribeCmd)"
participant Cmd as "DescribeCmd.Run"
participant Provider as "AwsProvider"
participant EC2 as "EC2 API"
participant Stdout as "stdout"
CLI->>Cmd: user runs `devpod describe <name>`
Cmd->>Provider: Describe(ctx, provider, name)
Provider->>EC2: DescribeInstances / GetInstance (state filter)
EC2-->>Provider: instance data
Provider->>Cmd: JSON description (string)
Cmd->>Stdout: print JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
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 (1)
pkg/aws/aws.go (1)
794-833:⚠️ Potential issue | 🟠 MajorPreserve “not found” as a non-error status.
After
GetInstancestarted returningfmt.Errorf("instance %s not found", name),Status()now treats an absent machine as a fatal error and exits non-zero instead of returningclient.StatusNotFound. That breaks the normal provider flow for missing machines.Proposed fix
+var ErrInstanceNotFound = errors.New("instance not found") + func GetInstance( ctx context.Context, cfg aws.Config, name string, states []string, ) (types.Instance, error) { @@ if len(result.Reservations) == 0 || len(result.Reservations[0].Instances) == 0 { - return types.Instance{}, fmt.Errorf("instance %s not found", name) + return types.Instance{}, fmt.Errorf("%w: %s", ErrInstanceNotFound, name) } return result.Reservations[0].Instances[0], nil }func Status(ctx context.Context, provider *AwsProvider, name string) (string, error) { @@ result, err := GetDevpodInstance(ctx, provider.AwsConfig, name) if err != nil { + if errors.Is(err, ErrInstanceNotFound) { + return client.StatusNotFound, nil + } return client.StatusNotFound, fmt.Errorf("get instance: %w", err) }Also applies to: 1037-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` around lines 794 - 833, GetInstance currently returns fmt.Errorf("instance %s not found", name) which makes absence appear as a generic error; instead return a sentinel non-fatal not-found error that callers can check (so Status() can map it to client.StatusNotFound). Add a package-level var ErrInstanceNotFound = errors.New("instance not found") (or reuse an existing exported sentinel if one exists), change the two places in GetInstance (the fmt.Errorf at the end and the similar case at lines ~1037-1043) to return types.Instance{} and ErrInstanceNotFound (or wrap with fmt.Errorf("%w: %s", ErrInstanceNotFound, name) if you want name included), and update callers/tests to compare errors using errors.Is(err, ErrInstanceNotFound).
🧹 Nitpick comments (1)
pkg/aws/aws.go (1)
1081-1084: Avoid dumping the full instance JSON into logs.This duplicates the stdout payload and pushes private IPs, tags, and instance-profile metadata into debug logs. A short summary is safer and easier to read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` around lines 1081 - 1084, The current code logs the full JSON payload in description (built from instanceBytes) via provider.Log.Debugf("Describe: machine %s is %s", name, description), which leaks sensitive data; change the logging to output a short safe summary instead: parse or extract only non-sensitive fields (e.g., instance ID, state/Status, and a redacted/private-IP-free indicator) from instanceBytes and log those, or truncate the description to a fixed short string (e.g., first N chars) and omit IPs/tags; update the provider.Log.Debugf call to reference the new summary variable instead of description while still returning the full description value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/describe.go`:
- Around line 22-29: The RunE in cmd/describe.go currently calls
aws.NewProvider(cobraCmd.Context(), true, log.Default) which forces
options.FromEnv(..., true) and incorrectly makes MACHINE_FOLDER required; change
the second argument to false so NewProvider is invoked as
aws.NewProvider(cobraCmd.Context(), false, log.Default), ensuring
DescribeCmd.Run only requires providerAws.Config.MachineID and no longer fails
when MACHINE_FOLDER is unset.
In `@cmd/status.go`:
- Around line 35-41: The status command makes MACHINE_FOLDER mandatory by
calling aws.NewProvider with the second arg true in RunE; change RunE to create
the AWS provider without requiring MACHINE_FOLDER (pass false or the non-strict
flag) so aws.NewProvider(cobraCmd.Context(), false, log.Default) is used, since
StatusCmd.Run only needs providerAws.Config.MachineID and should not fail when
the machine folder is absent; update the RunE invocation that currently calls
aws.NewProvider(...) and ensure StatusCmd.Run still receives the provider
instance.
In `@pkg/aws/aws.go`:
- Around line 43-48: The IMDS probe in configureDefaults()/NewProvider uses
httpClient := &http.Client{} with no timeout causing hangs; change the probe to
use a bounded request by creating the HTTP client with a timeout (or use
context.WithTimeout for the request) and ensure you close the response body;
update the code around http.NewRequest and httpClient.Do to use the timed
client/context so the probe fails fast on non-EC2 hosts and does not block
provider startup.
---
Outside diff comments:
In `@pkg/aws/aws.go`:
- Around line 794-833: GetInstance currently returns fmt.Errorf("instance %s not
found", name) which makes absence appear as a generic error; instead return a
sentinel non-fatal not-found error that callers can check (so Status() can map
it to client.StatusNotFound). Add a package-level var ErrInstanceNotFound =
errors.New("instance not found") (or reuse an existing exported sentinel if one
exists), change the two places in GetInstance (the fmt.Errorf at the end and the
similar case at lines ~1037-1043) to return types.Instance{} and
ErrInstanceNotFound (or wrap with fmt.Errorf("%w: %s", ErrInstanceNotFound,
name) if you want name included), and update callers/tests to compare errors
using errors.Is(err, ErrInstanceNotFound).
---
Nitpick comments:
In `@pkg/aws/aws.go`:
- Around line 1081-1084: The current code logs the full JSON payload in
description (built from instanceBytes) via provider.Log.Debugf("Describe:
machine %s is %s", name, description), which leaks sensitive data; change the
logging to output a short safe summary instead: parse or extract only
non-sensitive fields (e.g., instance ID, state/Status, and a
redacted/private-IP-free indicator) from instanceBytes and log those, or
truncate the description to a fixed short string (e.g., first N chars) and omit
IPs/tags; update the provider.Log.Debugf call to reference the new summary
variable instead of description while still returning the full description
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aea498aa-82e8-47ec-9989-f94b9e393b2f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignoreREADME.mdcmd/command.gocmd/create.gocmd/delete.gocmd/describe.gocmd/init.gocmd/root.gocmd/start.gocmd/status.gocmd/stop.gogo.modpkg/aws/aws.go
Comment on lines
+22
to
+29
| RunE: func(cobraCmd *cobra.Command, args []string) error { | ||
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cmd.Run(cobraCmd.Context(), awsProvider) | ||
| }, |
There was a problem hiding this comment.
Don’t require MACHINE_FOLDER for describe.
aws.NewProvider(..., true, ...) flows into options.FromEnv(false, true), so this command now fails when MACHINE_FOLDER is unset even though DescribeCmd.Run only needs providerAws.Config.MachineID. Please pass false here.
Proposed fix
- awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default)
+ awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default)📝 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
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run(cobraCmd.Context(), awsProvider) | |
| }, | |
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run(cobraCmd.Context(), awsProvider) | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/describe.go` around lines 22 - 29, The RunE in cmd/describe.go currently
calls aws.NewProvider(cobraCmd.Context(), true, log.Default) which forces
options.FromEnv(..., true) and incorrectly makes MACHINE_FOLDER required; change
the second argument to false so NewProvider is invoked as
aws.NewProvider(cobraCmd.Context(), false, log.Default), ensuring
DescribeCmd.Run only requires providerAws.Config.MachineID and no longer fails
when MACHINE_FOLDER is unset.
Comment on lines
+35
to
+41
| RunE: func(cobraCmd *cobra.Command, args []string) error { | ||
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cmd.Run( | ||
| context.Background(), | ||
| awsProvider, | ||
| getMachineProviderFromEnv(), | ||
| ) | ||
| return cmd.Run(cobraCmd.Context(), awsProvider) |
There was a problem hiding this comment.
Don’t require MACHINE_FOLDER for status.
This uses aws.NewProvider(..., true, ...), which makes MACHINE_FOLDER mandatory even though StatusCmd.Run only reads providerAws.Config.MachineID. That turns a simple status check into a config error when the folder isn’t present.
Proposed fix
- awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default)
+ awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default)📝 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
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run( | |
| context.Background(), | |
| awsProvider, | |
| getMachineProviderFromEnv(), | |
| ) | |
| return cmd.Run(cobraCmd.Context(), awsProvider) | |
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run(cobraCmd.Context(), awsProvider) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/status.go` around lines 35 - 41, The status command makes MACHINE_FOLDER
mandatory by calling aws.NewProvider with the second arg true in RunE; change
RunE to create the AWS provider without requiring MACHINE_FOLDER (pass false or
the non-strict flag) so aws.NewProvider(cobraCmd.Context(), false, log.Default)
is used, since StatusCmd.Run only needs providerAws.Config.MachineID and should
not fail when the machine folder is absent; update the RunE invocation that
currently calls aws.NewProvider(...) and ensure StatusCmd.Run still receives the
provider instance.
skevetter
approved these changes
Mar 12, 2026
- chore: track changes to DevPod - chore: use cobra.Command.Context - refactor: surface AWS instance structure - feat: add `machine describe` - chore(cli): remove some spurious types and parameters
7d31429 to
9bfad46
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cmd/status.go (1)
35-41:⚠️ Potential issue | 🟠 MajorDon't make
MACHINE_FOLDERa prerequisite forstatus.
StatusCmd.Runonly needsproviderAws.Config.MachineID, soaws.NewProvider(..., true, ...)turns a read-only status check into a config error when the local machine folder is absent.Suggested fix
- awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) + awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/status.go` around lines 35 - 41, The status command is forcing a local MACHINE_FOLDER prerequisite by calling aws.NewProvider with the second arg true; change the NewProvider invocation in cmd/status.go's RunE so it does not require the local machine folder (pass false instead of true) and ensure StatusCmd.Run still uses providerAws.Config.MachineID as before; this removes the unnecessary config error for read-only status checks while preserving access to MachineID from the provider.pkg/aws/aws.go (1)
42-48:⚠️ Potential issue | 🟠 MajorBound the EC2 metadata probe.
This probe still uses an unbounded
http.Client, soNewProvider()can hang indefinitely on non-EC2 hosts or broken DNS/network paths becauseconfigureDefaults()runs it during startup.Suggested fix
-func isEC2Instance() bool { - httpClient := &http.Client{} - req, err := http.NewRequest("GET", "http://instance-data.ec2.internal", nil) +func isEC2Instance(ctx context.Context) bool { + ctx, cancel := context.WithTimeout(ctx, time.Second) + defer cancel() + + httpClient := &http.Client{Timeout: time.Second} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://instance-data.ec2.internal", nil) if err != nil { return false } resp, err := httpClient.Do(req)-func configureDefaults(ctx context.Context, cfg aws.Config, config *options.Options, log log.Logger) error { - isEC2 := isEC2Instance() +func configureDefaults(ctx context.Context, cfg aws.Config, config *options.Options, log log.Logger) error { + isEC2 := isEC2Instance(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` around lines 42 - 48, The isEC2Instance probe uses an unbounded http.Client causing NewProvider()/configureDefaults() to potentially hang; fix by giving the request a bounded deadline (e.g., create an http.Client with a short Timeout or use context.WithTimeout on the request) and handle timeout errors gracefully so isEC2Instance returns false on timeout/transport errors; update the isEC2Instance function to use the timed client/context and ensure callers (NewProvider, configureDefaults) rely on the probe's bounded behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/delete.go`:
- Around line 21-27: The delete command currently calls
aws.NewProvider(cobraCmd.Context(), true, log.Default) which forces a
MACHINE_FOLDER requirement; change the provider initialization to not require
the machine folder (pass false for the strict/machine-folder-required parameter)
so Delete can run as a recovery/cleanup path even when local state is
missing—update the call in the RunE closure that creates awsProvider (and keep
using cmd.Run(cobraCmd.Context(), awsProvider) as before).
In `@cmd/start.go`:
- Around line 21-27: The start command currently forces a local machine folder
by calling aws.NewProvider(cobraCmd.Context(), true, log.Default) in the RunE
closure; change that so the provider is created without requiring MACHINE_FOLDER
(pass false or use the non-folder variant of NewProvider) so start only needs
machine ID and AWS config, and ensure cmd.Run(cobraCmd.Context(), awsProvider)
can operate when local workspace state is absent (adjust aws.NewProvider and any
provider methods if they assume a machine folder).
In `@pkg/aws/aws.go`:
- Around line 829-830: Replace the generic fmt.Errorf("instance %s not found",
name) in GetInstance with a canonical sentinel error so callers can distinguish
“not found” from real lookup failures: add a package-level var
ErrInstanceNotFound = errors.New("instance not found") and return that (wrapped
with context if desired) from GetInstance (and the similar occurrence around the
1040-1043 block). Update callers like Status to check errors.Is(err,
ErrInstanceNotFound) instead of string-matching so the CLI can report NotFound
vs real errors.
---
Duplicate comments:
In `@cmd/status.go`:
- Around line 35-41: The status command is forcing a local MACHINE_FOLDER
prerequisite by calling aws.NewProvider with the second arg true; change the
NewProvider invocation in cmd/status.go's RunE so it does not require the local
machine folder (pass false instead of true) and ensure StatusCmd.Run still uses
providerAws.Config.MachineID as before; this removes the unnecessary config
error for read-only status checks while preserving access to MachineID from the
provider.
In `@pkg/aws/aws.go`:
- Around line 42-48: The isEC2Instance probe uses an unbounded http.Client
causing NewProvider()/configureDefaults() to potentially hang; fix by giving the
request a bounded deadline (e.g., create an http.Client with a short Timeout or
use context.WithTimeout on the request) and handle timeout errors gracefully so
isEC2Instance returns false on timeout/transport errors; update the
isEC2Instance function to use the timed client/context and ensure callers
(NewProvider, configureDefaults) rely on the probe's bounded behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8da5745-0a53-4c63-942f-0bc6670a1569
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.gitignoreREADME.mdcmd/command.gocmd/create.gocmd/delete.gocmd/describe.gocmd/init.gocmd/root.gocmd/start.gocmd/status.gocmd/stop.gogo.modpkg/aws/aws.go
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- cmd/root.go
- README.md
- cmd/command.go
- cmd/describe.go
Comment on lines
+21
to
+27
| RunE: func(cobraCmd *cobra.Command, args []string) error { | ||
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cmd.Run( | ||
| context.Background(), | ||
| awsProvider, | ||
| getMachineProviderFromEnv(), | ||
| ) | ||
| return cmd.Run(cobraCmd.Context(), awsProvider) |
There was a problem hiding this comment.
Don't require MACHINE_FOLDER for delete.
delete is often the recovery path after local state is gone. Making aws.NewProvider strict here prevents cleanup of orphaned instances even though this command never uses MachineFolder.
Suggested fix
- awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default)
+ awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/delete.go` around lines 21 - 27, The delete command currently calls
aws.NewProvider(cobraCmd.Context(), true, log.Default) which forces a
MACHINE_FOLDER requirement; change the provider initialization to not require
the machine folder (pass false for the strict/machine-folder-required parameter)
so Delete can run as a recovery/cleanup path even when local state is
missing—update the call in the RunE closure that creates awsProvider (and keep
using cmd.Run(cobraCmd.Context(), awsProvider) as before).
Comment on lines
+21
to
+27
| RunE: func(cobraCmd *cobra.Command, args []string) error { | ||
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return cmd.Run( | ||
| context.Background(), | ||
| awsProvider, | ||
| getMachineProviderFromEnv(), | ||
| ) | ||
| return cmd.Run(cobraCmd.Context(), awsProvider) |
There was a problem hiding this comment.
Don't require MACHINE_FOLDER for start.
This path only needs the machine ID plus AWS config. Requiring the local machine folder here blocks restarting an existing instance after local workspace state is removed.
Suggested fix
- awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default)
+ awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default)📝 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
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), true, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run( | |
| context.Background(), | |
| awsProvider, | |
| getMachineProviderFromEnv(), | |
| ) | |
| return cmd.Run(cobraCmd.Context(), awsProvider) | |
| RunE: func(cobraCmd *cobra.Command, args []string) error { | |
| awsProvider, err := aws.NewProvider(cobraCmd.Context(), false, log.Default) | |
| if err != nil { | |
| return err | |
| } | |
| return cmd.Run(cobraCmd.Context(), awsProvider) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/start.go` around lines 21 - 27, The start command currently forces a
local machine folder by calling aws.NewProvider(cobraCmd.Context(), true,
log.Default) in the RunE closure; change that so the provider is created without
requiring MACHINE_FOLDER (pass false or use the non-folder variant of
NewProvider) so start only needs machine ID and AWS config, and ensure
cmd.Run(cobraCmd.Context(), awsProvider) can operate when local workspace state
is absent (adjust aws.NewProvider and any provider methods if they assume a
machine folder).
Author
|
@skevetter please approve 3 workflows ;) |
Owner
Please try to push followup commits instead of force pushing so that I can see what are the differences in the latest change. |
Author
Sorry, got carried away. Will do. |
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/aws/aws.go (1)
829-830:⚠️ Potential issue | 🟠 MajorInstance not-found still indistinguishable from real lookup failures.
The previous review flagged this issue and it remains unaddressed.
GetInstance()returns a plain formatted error for absence, so callers cannot distinguish between "instance doesn't exist" (a valid state) and actual API/network failures.This causes downstream issues:
Status()at lines 1045-1048 has unreachable dead code checkingresult.Status == ""- Callers in
cmd/delete.goandcmd/start.gothat checkinstance.Status != ""now have unreachable branchesSuggested fix using sentinel error
+var ErrInstanceNotFound = errors.New("instance not found") + func GetInstance( ctx context.Context, cfg aws.Config, name string, states []string, ) (types.Instance, error) { // ... existing code ... if len(result.Reservations) == 0 || len(result.Reservations[0].Instances) == 0 { - return types.Instance{}, fmt.Errorf("instance %s not found", name) + return types.Instance{}, fmt.Errorf("%w: %s", ErrInstanceNotFound, name) } return result.Reservations[0].Instances[0], nil }Then update
Status()to useerrors.Is():func Status(ctx context.Context, provider *AwsProvider, name string) (client.Status, error) { provider.Log.Debugf("checking status for machine %s", name) result, err := GetDevpodInstance(ctx, provider.AwsConfig, name) if err != nil { + if errors.Is(err, ErrInstanceNotFound) { + provider.Log.Debugf("machine %s not found", name) + return client.StatusNotFound, nil + } return client.StatusNotFound, fmt.Errorf("get instance: %w", err) } - - if result.Status == "" { - provider.Log.Debugf("machine %s not found", name) - return client.StatusNotFound, nil - } // ... rest of status mapping ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` around lines 829 - 830, Replace the opaque formatted error in GetInstance (function GetInstance) with a sentinel error value (e.g., ErrInstanceNotFound declared at package scope) and return that when the AWS lookup yields no reservations/instances; then update callers—specifically the Status method (Status()) and the code paths in cmd/delete.go and cmd/start.go—to detect the not-found case using errors.Is(err, ErrInstanceNotFound) instead of string comparison or empty Status checks so the absence case is distinguishable from real API/network errors.
🧹 Nitpick comments (1)
pkg/aws/aws.go (1)
1076-1076: Misplaced#nosecdirective.The
#nosec G117comment references a gosec rule for integer overflow in type conversion, butjson.MarshalIndentdoesn't involve type conversion that would trigger this rule. This appears to be a copy-paste error or incorrect annotation.If you intended to suppress a specific linter warning, verify the correct rule ID. Otherwise, remove the directive.
- instanceBytes, err := json.MarshalIndent(instance, "", " ") // `#nosec` G117 + instanceBytes, err := json.MarshalIndent(instance, "", " ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` at line 1076, The trailing "// `#nosec` G117" on the json.MarshalIndent call is incorrect (G117 is unrelated to json.MarshalIndent); remove the misplaced directive from the instanceBytes, err := json.MarshalIndent(instance, "", " ") line (or replace it with the correct gosec rule if you actually meant to suppress a different finding) so the comment no longer mislabels that statement; reference the json.MarshalIndent call and the instanceBytes/err variables to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/aws/aws.go`:
- Around line 1045-1048: The check for result.Status == "" in the
GetDevpodInstance call is dead/unreachable because GetDevpodInstance (via
GetInstance) now returns an error when no instance is found; remove the
unreachable branch (the provider.Log.Debugf("machine %s not found", name) and
the return client.StatusNotFound, nil) from the function, rely on the error
returned by GetDevpodInstance/GetInstance to signal “not found”, and ensure any
callers handle that sentinel error instead of expecting an empty result.Status.
- Around line 1077-1079: The code incorrectly returns client.DescriptionNotFound
when json.MarshalIndent fails; instead propagate the actual marshal error and
return the proper zero-value for the description result. Replace the return that
uses client.DescriptionNotFound with a return of the function's zero-value for
the description (e.g., nil or ""/empty struct/empty byte slice depending on the
function signature) and err, so the caller can distinguish a serialization error
from a missing description; locate the failing call to json.MarshalIndent and
the return that references client.DescriptionNotFound and update that return
accordingly.
---
Duplicate comments:
In `@pkg/aws/aws.go`:
- Around line 829-830: Replace the opaque formatted error in GetInstance
(function GetInstance) with a sentinel error value (e.g., ErrInstanceNotFound
declared at package scope) and return that when the AWS lookup yields no
reservations/instances; then update callers—specifically the Status method
(Status()) and the code paths in cmd/delete.go and cmd/start.go—to detect the
not-found case using errors.Is(err, ErrInstanceNotFound) instead of string
comparison or empty Status checks so the absence case is distinguishable from
real API/network errors.
---
Nitpick comments:
In `@pkg/aws/aws.go`:
- Line 1076: The trailing "// `#nosec` G117" on the json.MarshalIndent call is
incorrect (G117 is unrelated to json.MarshalIndent); remove the misplaced
directive from the instanceBytes, err := json.MarshalIndent(instance, "", " ")
line (or replace it with the correct gosec rule if you actually meant to
suppress a different finding) so the comment no longer mislabels that statement;
reference the json.MarshalIndent call and the instanceBytes/err variables to
locate the fix.
Signed-off-by: Samuel K <skevetter@pm.me>
Author
|
What is the reason for keeping |
dubinsky
commented
Mar 12, 2026
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/aws/aws.go (1)
1000-1017: Use transition-requested wording here.Lines 1016 and 1036 log the final state even though
StartInstances/StopInstancesonly initiate the transition. That makes debugging harder when the instance is stillpendingorstopping.Proposed tweak
func Start(ctx context.Context, provider *AwsProvider, instanceID string) error { provider.Log.Debugf("starting instance %s", instanceID) @@ - provider.Log.Debugf("instance %s started", instanceID) + provider.Log.Debugf("start requested for instance %s", instanceID) return nil } func Stop(ctx context.Context, provider *AwsProvider, instanceID string) error { provider.Log.Debugf("stopping instance %s", instanceID) @@ - provider.Log.Debugf("instance %s stopped", instanceID) + provider.Log.Debugf("stop requested for instance %s", instanceID) return nil }Also applies to: 1020-1037
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/aws/aws.go` around lines 1000 - 1017, The log messages in Start (and the analogous Stop function) incorrectly claim the instance is "started"/"stopped" even though ec2.StartInstances/StopInstances only request a state transition; update the final provider.Log.Debugf calls in Start (and Stop) to use transition-requested wording such as "start transition requested for instance %s" / "stop transition requested for instance %s" (or similar) so the logs reflect the requested transition rather than a completed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/aws/aws.go`:
- Around line 1069-1079: Describe currently treats all GetInstance errors as
"not found"; update Describe to distinguish ErrInstanceNotFound from other
errors by using errors.Is(err, ErrInstanceNotFound) (or the package-qualified
ErrInstanceNotFound) after calling GetInstance(ctx, provider.AwsConfig, name,
anyState()). If the error is ErrInstanceNotFound return
client.DescriptionNotFound, nil; for any other non-nil error return "", err.
Keep the rest of Describe (json.MarshalIndent etc.) unchanged and reference the
Describe and GetInstance functions and ErrInstanceNotFound constant in your
change.
---
Nitpick comments:
In `@pkg/aws/aws.go`:
- Around line 1000-1017: The log messages in Start (and the analogous Stop
function) incorrectly claim the instance is "started"/"stopped" even though
ec2.StartInstances/StopInstances only request a state transition; update the
final provider.Log.Debugf calls in Start (and Stop) to use transition-requested
wording such as "start transition requested for instance %s" / "stop transition
requested for instance %s" (or similar) so the logs reflect the requested
transition rather than a completed state.
Signed-off-by: Samuel K <skevetter@pm.me>
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
2 participants
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.
machine describeSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Chores