refactor move binaries to provider #435
Conversation
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
📝 WalkthroughWalkthroughRefactors workspace init to use parameter structs; consolidates provider types to the public Changes
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. ✨ 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 |
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/download/download.go (2)
30-41:⚠️ Potential issue | 🟡 MinorClose the HEAD response body to avoid leaking connections.
Line 36 returns the status without closingresp.Body, which can exhaust the HTTP client’s connection pool over time.🛠️ Suggested fix
resp, err := devpodhttp.GetHTTPClient().Do(req) if err != nil { return 0, fmt.Errorf("download file: %w", err) } +defer resp.Body.Close() return resp.StatusCode, nil
108-123:⚠️ Potential issue | 🟡 MinorEnsure the release metadata response body is closed.
Line 125 reads the body but it’s never closed; this can leak connections on repeated calls.🛠️ Suggested fix
resp, err := devpodhttp.GetHTTPClient().Do(req) if err != nil { return nil, err } else if resp.StatusCode >= 400 { _ = resp.Body.Close() return nil, &HTTPStatusError{ StatusCode: resp.StatusCode, URL: releaseURL, } } +defer resp.Body.Close() raw, err := io.ReadAll(resp.Body)pkg/client/clientimplementation/workspace_client.go (1)
509-524:⚠️ Potential issue | 🟠 MajorUnlock on error to avoid deadlocks.
Ifprovider.ToEnvironmentWithBinariesfails, Line 521 returns befores.m.Unlock()runs, leaving the mutex locked.🛠️ Suggested fix
s.m.Lock() environ, err := provider.ToEnvironmentWithBinaries(provider.EnvironmentOptions{ Context: s.workspace.Context, Workspace: s.workspace, Machine: s.machine, Options: s.devPodConfig.ProviderOptions(s.config.Name), Config: s.config, ExtraEnv: map[string]string{ provider.CommandEnv: commandOptions.Command, }, Log: s.log, }) if err != nil { + s.m.Unlock() return err } s.m.Unlock()
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
skevetter
force-pushed
the
refactor-move-binaries-to-provider
branch
from
1e47f88 to
ff38e0b
Compare
Signed-off-by: Samuel K <skevetter@pm.me>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
e2e/tests/integration/integration.go (1)
62-65:⚠️ Potential issue | 🟡 MinorUse the already-defined
authorizedKeysPathvariable for consistency.Line 63 constructs the path using string concatenation while
authorizedKeysPath(defined at line 57) was already created usingfilepath.Joinfor cross-platform compatibility. This inconsistency could cause issues on non-Unix systems.🛠️ Proposed fix
} else { - f, err := os.OpenFile(os.Getenv("HOME")+"/.ssh/authorized_keys", + f, err := os.OpenFile(authorizedKeysPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) framework.ExpectNoError(err)pkg/options/resolve.go (2)
20-154:⚠️ Potential issue | 🟡 MinorExpand nil validation to providerConfig/devConfig.
These functions dereference providerConfig.Name and devConfig.DefaultContext; if callers ever pass nil, this will panic. You already guard originalMachine/originalWorkspace, so consider guarding providerConfig/devConfig similarly.🛡️ Proposed guard (apply in both functions)
func ResolveAndSaveOptionsMachine( ctx context.Context, devConfig *config.Config, providerConfig *provider.ProviderConfig, originalMachine *provider.Machine, userOptions map[string]string, log log.Logger, ) (*provider.Machine, error) { if originalMachine == nil { return nil, fmt.Errorf("originalMachine cannot be nil") } + if providerConfig == nil || devConfig == nil { + return nil, fmt.Errorf("providerConfig and devConfig cannot be nil") + }func ResolveAndSaveOptionsWorkspace( ctx context.Context, devConfig *config.Config, providerConfig *provider.ProviderConfig, originalWorkspace *provider.Workspace, userOptions map[string]string, log log.Logger, options ...resolver.Option, ) (*provider.Workspace, error) { if originalWorkspace == nil { return nil, fmt.Errorf("originalWorkspace cannot be nil") } + if providerConfig == nil || devConfig == nil { + return nil, fmt.Errorf("providerConfig and devConfig cannot be nil") + }
157-221:⚠️ Potential issue | 🟡 MinorGuard ResolveOptions against nil providerConfig/devConfig or document the non-nil contract.
ResolveOptions uses devConfig.DefaultContext and providerConfig.Name before any nil checks. If nil can occur, return a clear error; if not, consider documenting it.🛡️ Proposed guard
func ResolveOptions( ctx context.Context, devConfig *config.Config, providerConfig *provider.ProviderConfig, userOptions map[string]string, skipRequired bool, skipSubOptions bool, singleMachine *bool, log log.Logger, ) (*config.Config, error) { + if providerConfig == nil || devConfig == nil { + return nil, fmt.Errorf("providerConfig and devConfig cannot be nil") + } // get binary paths binaryPaths, err := provider.GetBinaries(devConfig.DefaultContext, providerConfig)
🧹 Nitpick comments (1)
pkg/provider/download.go (1)
196-211: Consider whether allnet.OpErrorcases should be retriable.Currently, all
net.OpErrorinstances are marked as retriable (line 203). While connection errors often benefit from retries, somenet.OpErrorcases like "address already in use" or permission errors may not be transient. Consider checkingopErr.Temporary()oropErr.Timeout()for more precise classification.♻️ Suggested refinement
var opErr *net.OpError if errors.As(err, &opErr) { - return true + return opErr.Temporary() || opErr.Timeout() }
1 participant
Summary by CodeRabbit