provider name enhancements#615
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds provider-name validation and multi-source resolution (positional or DEVPOD_PROVIDER[_]_SOURCE) to provider add; separates clone-vs-add flows; relaxes provider delete arg validation; updates ToEnvironment signature and callers to accept provider name; introduces per-command environment variable prefixes; punctuation/docs tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as devpod CLI (cmd/provider/add)
participant Env as OS Env
participant Store as ProviderStore
participant Service as ProviderService
User->>CLI: run "devpod provider add [name?] [source?]"
CLI->>CLI: validate providerName if provided (regex + length)
CLI->>Env: read DEVPOD_PROVIDER_SOURCE
CLI->>Env: if name provided read DEVPOD_PROVIDER_<NAME>_SOURCE
CLI->>CLI: resolve providerSource (arg or env)
alt FromExisting set
CLI->>Store: check referenced provider exists
Store-->>CLI: exists / not found
alt exists
CLI->>Service: CloneProvider(existingRef, providerName)
Service-->>Store: create cloned provider
Service-->>CLI: success
else not found
CLI-->>User: error "provider not found"
end
else adding new provider
alt providerSource present
CLI->>Service: AddProvider(providerName, providerSource)
Service-->>Store: create provider
Service-->>CLI: success
else missing source
CLI-->>User: error "must provide provider source"
end
end
CLI-->>User: final status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/provider/add.go`:
- Around line 76-79: The error message returned when
provider.ProviderNameRegEx.MatchString(providerName) fails contains a typo
"smaller case"; update the fmt.Errorf(...) string in that return statement to
read "lowercase" instead of "smaller case" so the message becomes: "provider
name can only include lowercase letters, numbers or dashes".
- Around line 94-96: The constructed environment variable name in the else
branch builds sourceEnvVar without an underscore before "SOURCE"; update the
expression that sets sourceEnvVar (currently using
"DEVPOD_PROVIDER_"+strings.ToUpper(providerName)+"SOURCE") to include the
missing underscore so the result is
"DEVPOD_PROVIDER_"+strings.ToUpper(providerName)+"_SOURCE", ensuring the
environment variable matches the documented DEVPOD_PROVIDER_<provider
name>_SOURCE format.
In `@cmd/provider/update.go`:
- Around line 51-52: The error message returned from the validation in
cmd/provider/update.go currently always states a source is required even though
the one-argument form (update <name>) is valid; update the fmt.Errorf call (the
return that builds the message) to mention the valid single-argument form or
clarify that either "update <name>" is allowed or "update <name> <source>" with
examples (reference the fmt.Errorf invocation in this file to locate and replace
the string).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0236da0f-fc7a-4543-be3d-360ca09d1211
📒 Files selected for processing (15)
cmd/provider/add.gocmd/provider/delete.gocmd/provider/update.gocmd/up.godocs/pages/developing-providers/options.mdxdocs/pages/managing-providers/add-provider.mdxpkg/driver/custom/custom.gopkg/ide/jupyter/jupyter.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/provider/download.gopkg/provider/env.gopkg/provider/parse.gopkg/workspace/workspace.goproviders/kubernetes/provider.yaml
💤 Files with no reviewable changes (1)
- cmd/provider/delete.go
0c2bf10 to
6d9999c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ide/jupyter/jupyter.go (1)
44-45: Fix the "Notebook" typo in the struct name.The
JupyterNotbookServertype (and constructor) misspells "Notebook" as "Notbook" in lines 44, 45, 53, 60, 69, and 109. Since this PR addresses typos, rename it toJupyterNotebookServer. All references are localized to this file.✏️ Proposed fix
func NewJupyterNotebookServer( workspaceFolder string, userName string, values map[string]config.OptionValue, log log.Logger, -) *JupyterNotbookServer { - return &JupyterNotbookServer{ +) *JupyterNotebookServer { + return &JupyterNotebookServer{ values: values, workspaceFolder: workspaceFolder, userName: userName, log: log, } } -type JupyterNotbookServer struct { +type JupyterNotebookServer struct { values map[string]config.OptionValue workspaceFolder string userName string log log.Logger } -func (o *JupyterNotbookServer) Install() error { +func (o *JupyterNotebookServer) Install() error { -func (o *JupyterNotbookServer) installNotebook() error { +func (o *JupyterNotebookServer) installNotebook() error { -func (o *JupyterNotbookServer) Start() error { +func (o *JupyterNotebookServer) Start() error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/jupyter/jupyter.go` around lines 44 - 45, Rename the misspelled type and its constructor from JupyterNotbookServer to JupyterNotebookServer and update all local references in this file accordingly: change the type declaration, the constructor return type (where the function currently returns *JupyterNotbookServer), the constructor name if it matches the type, and any method receivers or variables named JupyterNotbookServer (references at the locations around lines 44, 53, 60, 69, 109). Ensure receiver declarations (e.g., func (s *JupyterNotbookServer) ...) and any instantiations or type assertions are updated to the new JupyterNotebookServer identifier so the file compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ide/jupyter/jupyter.go`:
- Around line 44-45: Rename the misspelled type and its constructor from
JupyterNotbookServer to JupyterNotebookServer and update all local references in
this file accordingly: change the type declaration, the constructor return type
(where the function currently returns *JupyterNotbookServer), the constructor
name if it matches the type, and any method receivers or variables named
JupyterNotbookServer (references at the locations around lines 44, 53, 60, 69,
109). Ensure receiver declarations (e.g., func (s *JupyterNotbookServer) ...)
and any instantiations or type assertions are updated to the new
JupyterNotebookServer identifier so the file compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fac076a7-70be-446f-8d92-a251873b3da1
📒 Files selected for processing (16)
cmd/context/create.gocmd/provider/add.gocmd/provider/delete.gocmd/provider/update.gocmd/up.godocs/pages/developing-providers/options.mdxdocs/pages/managing-providers/add-provider.mdxpkg/driver/custom/custom.gopkg/ide/jupyter/jupyter.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/provider/download.gopkg/provider/env.gopkg/provider/parse.gopkg/workspace/workspace.goproviders/kubernetes/provider.yaml
💤 Files with no reviewable changes (1)
- cmd/provider/delete.go
🚧 Files skipped from review as they are similar to previous changes (7)
- providers/kubernetes/provider.yaml
- cmd/provider/update.go
- pkg/ide/openvscode/openvscode.go
- cmd/up.go
- pkg/ide/rstudio/rstudio.go
- docs/pages/developing-providers/options.mdx
- pkg/workspace/workspace.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/root.go (1)
171-183:⚠️ Potential issue | 🟠 MajorBreaking change: subcommand flags now require prefixed environment variables.
Previously, a flag like
--ideon theupsubcommand could be set viaDEVPOD_IDE. After this change, it requiresDEVPOD_UP_IDE. This affects all subcommand-specific flags across multiple commands includingup,status,build, andssh.Any user scripts or automation relying on the old
DEVPOD_*environment variable names for subcommand flags will break after this update. Ensure this breaking change is documented in release notes or a migration guide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 171 - 183, The change to require DEVPOD_<SUBCOMMAND>_<FLAG> breaks existing DEVPOD_<FLAG> env vars; update inheritFlagsFromEnvironment to preserve backward compatibility by checking for the legacy environment variable name (constructed as suffix, e.g., "DEVPOD_IDE") if the prefixed name ("DEVPOD_"+commandPrefix+suffix) is not set — lookup both names and prefer the more specific prefixed var but fall back to the legacy var; modify the logic where environmentVariable is determined in inheritFlagsFromEnvironment and use both names when calling os.Getenv or equivalent so subcommand flags continue to respect existing DEVPOD_* environment variables while still supporting DEVPOD_<SUBCOMMAND>_*.
🤖 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/root.go`:
- Around line 160-166: The code assumes sub.Use is non-empty and calls
strings.Fields(sub.Use)[0], which can panic; replace that with a defensive
check: split sub.Use via strings.Fields into a slice (e.g., fields :=
strings.Fields(sub.Use)) and if len(fields) > 0 use strings.ToUpper(fields[0]) +
"_" for subCommandPrefix, otherwise fall back to a safe alternative such as
strings.ToUpper(sub.Name()) + "_" (or just use commandPrefix if provided), then
call inheritCommandFlagsFromEnvironment(sub, subCommandPrefix); update the block
that computes subCommandPrefix (references: sub.Use, subCommandPrefix,
inheritCommandFlagsFromEnvironment, sub.Name()) accordingly.
---
Outside diff comments:
In `@cmd/root.go`:
- Around line 171-183: The change to require DEVPOD_<SUBCOMMAND>_<FLAG> breaks
existing DEVPOD_<FLAG> env vars; update inheritFlagsFromEnvironment to preserve
backward compatibility by checking for the legacy environment variable name
(constructed as suffix, e.g., "DEVPOD_IDE") if the prefixed name
("DEVPOD_"+commandPrefix+suffix) is not set — lookup both names and prefer the
more specific prefixed var but fall back to the legacy var; modify the logic
where environmentVariable is determined in inheritFlagsFromEnvironment and use
both names when calling os.Getenv or equivalent so subcommand flags continue to
respect existing DEVPOD_* environment variables while still supporting
DEVPOD_<SUBCOMMAND>_*.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/provider/add.go`:
- Around line 103-106: The code currently only errors if devPodConfig.Current()
!= nil && devPodConfig.Current().Providers[cmd.FromExisting] == nil, which lets
execution continue when devPodConfig.Current() is nil; update the logic to
explicitly check for a nil current context (devPodConfig.Current() == nil) and
return a clear error (e.g., "no current devpod context") before attempting to
access Providers or calling CloneProvider, and keep the existing check that
returns "provider %s does not exist" when Current() is non-nil but
Providers[cmd.FromExisting] is nil; ensure these checks occur prior to invoking
CloneProvider or any access to Current().Providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06b22b7a-c2e5-42e1-a349-56b802031854
📒 Files selected for processing (17)
cmd/context/create.gocmd/provider/add.gocmd/provider/delete.gocmd/provider/update.gocmd/root.gocmd/up.godocs/pages/developing-providers/options.mdxdocs/pages/managing-providers/add-provider.mdxpkg/driver/custom/custom.gopkg/ide/jupyter/jupyter.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/provider/download.gopkg/provider/env.gopkg/provider/parse.gopkg/workspace/workspace.goproviders/kubernetes/provider.yaml
💤 Files with no reviewable changes (1)
- cmd/provider/delete.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/provider/env.go
- pkg/driver/custom/custom.go
- cmd/up.go
- docs/pages/developing-providers/options.mdx
- pkg/workspace/workspace.go
- providers/kubernetes/provider.yaml
- cmd/provider/update.go
- pkg/ide/rstudio/rstudio.go
- cmd/context/create.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/provider/add.go (1)
37-47:⚠️ Potential issue | 🟡 MinorReject extra positional args explicitly.
At line 87, only the single-arg case is handled; extra arguments are silently ignored. Add an argument validator to the Cobra command to enforce this explicitly, consistent with other commands in the codebase.
Proposed fix
addCmd := &cobra.Command{ Use: "add [name, GitHub link, URL or path]", Short: "Adds a new provider to DevPod", + Args: cobra.MaximumNArgs(1), PreRunE: func(cobraCommand *cobra.Command, args []string) error { if cmd.FromExisting != "" { return cobraCommand.MarkFlagRequired("name") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/provider/add.go` around lines 37 - 47, The command currently handles only a single positional argument in RunE but silently ignores extras; update the Cobra command definition in cmd/provider/add.go (the command struct with Use/Short/PreRunE/RunE) to validate argument count explicitly by adding an Args validator (e.g., cobra.MaximumNArgs(1) or a custom Args function) so any extra positional args are rejected; ensure this validation is applied alongside the existing PreRunE that checks cmd.FromExisting so the command fails fast when more than one positional argument is provided.
♻️ Duplicate comments (1)
cmd/provider/add.go (1)
103-106:⚠️ Potential issue | 🟠 MajorHandle nil current context before checking provider existence.
At Line 103,
devPodConfig.Current() == nilskips this guard and falls through to clone logic. This should fail fast with a clear error before attempting clone operations.🐛 Proposed fix
if cmd.FromExisting != "" { - if devPodConfig.Current() != nil && - devPodConfig.Current().Providers[cmd.FromExisting] == nil { + current := devPodConfig.Current() + if current == nil { + return fmt.Errorf("no current devpod context") + } + if current.Providers[cmd.FromExisting] == nil { return fmt.Errorf("provider %s does not exist", cmd.FromExisting) } providerWithOptions, err := workspace.CloneProvider(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/provider/add.go` around lines 103 - 106, The current guard only checks provider existence when devPodConfig.Current() != nil, which lets execution continue when Current() is nil; add an explicit fast-fail check for a nil current context before any clone/provider lookups (e.g., if devPodConfig.Current() == nil return fmt.Errorf("no current context set") ) so that the code referencing devPodConfig.Current().Providers and cmd.FromExisting never runs against a nil context; update the logic around the devPodConfig.Current(), Providers and cmd.FromExisting checks to first validate the current context is non-nil and then validate the requested provider exists.
🧹 Nitpick comments (1)
cmd/provider/add.go (1)
92-97: Make named source env vars shell-friendly for dashed provider names.Provider names allow dashes, but Line 95 builds
DEVPOD_PROVIDER_<NAME>_SOURCEdirectly from name. For names likemy-provider, this yields env keys that are awkward/impossible to set via commonexportsyntax. Consider normalizing-to_(and optionally falling back to the raw form for compatibility).♻️ Suggested refactor
if providerSource == "" { var sourceEnvVar string if providerName == "" { sourceEnvVar = "DEVPOD_PROVIDER_SOURCE" } else { - sourceEnvVar = "DEVPOD_PROVIDER_" + strings.ToUpper(providerName) + "_SOURCE" + normalized := strings.ToUpper(strings.ReplaceAll(providerName, "-", "_")) + sourceEnvVar = "DEVPOD_PROVIDER_" + normalized + "_SOURCE" } providerSource = os.Getenv(sourceEnvVar) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/provider/add.go` around lines 92 - 97, The env var key built for provider source uses the raw providerName (DEVPOD_PROVIDER_<NAME>_SOURCE), which breaks common shell export syntax for dashed names; change the construction so you first normalize providerName by replacing '-' with '_' (e.g., normalized := strings.ReplaceAll(providerName, "-", "_")), then build sourceEnvVar using strings.ToUpper(normalized) and read os.Getenv(sourceEnvVar); if that lookup is empty, fall back to the original raw-uppercased form (DEVPOD_PROVIDER_ + strings.ToUpper(providerName) + _SOURCE) for compatibility. Update the code around the variables providerName, sourceEnvVar and providerSource to perform the normalize-then-fallback lookup while preserving the existing special-case when providerName == "".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@providers/kubernetes/provider.yaml`:
- Around line 68-70: Fix the subject-verb agreement in the PVC_ACCESS_MODE
description string: change "the storage class support the given access mode" to
"the storage class supports the given access mode" so the singular subject
"storage class" matches the verb; locate the PVC_ACCESS_MODE description in
provider.yaml and update the text accordingly.
---
Outside diff comments:
In `@cmd/provider/add.go`:
- Around line 37-47: The command currently handles only a single positional
argument in RunE but silently ignores extras; update the Cobra command
definition in cmd/provider/add.go (the command struct with
Use/Short/PreRunE/RunE) to validate argument count explicitly by adding an Args
validator (e.g., cobra.MaximumNArgs(1) or a custom Args function) so any extra
positional args are rejected; ensure this validation is applied alongside the
existing PreRunE that checks cmd.FromExisting so the command fails fast when
more than one positional argument is provided.
---
Duplicate comments:
In `@cmd/provider/add.go`:
- Around line 103-106: The current guard only checks provider existence when
devPodConfig.Current() != nil, which lets execution continue when Current() is
nil; add an explicit fast-fail check for a nil current context before any
clone/provider lookups (e.g., if devPodConfig.Current() == nil return
fmt.Errorf("no current context set") ) so that the code referencing
devPodConfig.Current().Providers and cmd.FromExisting never runs against a nil
context; update the logic around the devPodConfig.Current(), Providers and
cmd.FromExisting checks to first validate the current context is non-nil and
then validate the requested provider exists.
---
Nitpick comments:
In `@cmd/provider/add.go`:
- Around line 92-97: The env var key built for provider source uses the raw
providerName (DEVPOD_PROVIDER_<NAME>_SOURCE), which breaks common shell export
syntax for dashed names; change the construction so you first normalize
providerName by replacing '-' with '_' (e.g., normalized :=
strings.ReplaceAll(providerName, "-", "_")), then build sourceEnvVar using
strings.ToUpper(normalized) and read os.Getenv(sourceEnvVar); if that lookup is
empty, fall back to the original raw-uppercased form (DEVPOD_PROVIDER_ +
strings.ToUpper(providerName) + _SOURCE) for compatibility. Update the code
around the variables providerName, sourceEnvVar and providerSource to perform
the normalize-then-fallback lookup while preserving the existing special-case
when providerName == "".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de115631-5f85-4084-8ea3-249c0d8a9cbc
📒 Files selected for processing (17)
cmd/context/create.gocmd/provider/add.gocmd/provider/delete.gocmd/provider/update.gocmd/root.gocmd/up.godocs/pages/developing-providers/options.mdxdocs/pages/managing-providers/add-provider.mdxpkg/driver/custom/custom.gopkg/ide/jupyter/jupyter.gopkg/ide/openvscode/openvscode.gopkg/ide/rstudio/rstudio.gopkg/provider/download.gopkg/provider/env.gopkg/provider/parse.gopkg/workspace/workspace.goproviders/kubernetes/provider.yaml
💤 Files with no reviewable changes (1)
- cmd/provider/delete.go
🚧 Files skipped from review as they are similar to previous changes (12)
- cmd/up.go
- cmd/provider/update.go
- docs/pages/managing-providers/add-provider.mdx
- pkg/provider/parse.go
- pkg/ide/rstudio/rstudio.go
- cmd/context/create.go
- pkg/workspace/workspace.go
- pkg/driver/custom/custom.go
- cmd/root.go
- docs/pages/developing-providers/options.mdx
- pkg/provider/env.go
- pkg/ide/jupyter/jupyter.go
- chore: typos and minor cleanup - feat: convey to the provider the name it is running under - feat: use `DEVPOD_PROVIDER_<provider name>_SOURCE` environment variable - feat: scope command flags environment variables by the first-level subcommand name
Merge conflicts: - cmd/provider/update.go: keep main's config.ProviderPrefix with PR's punctuation style - cmd/root.go: use config.EnvPrefix constant with PR's commandPrefix scoping Review fixes: - cmd/provider/add.go: fix "lower case" -> "lowercase" typo, add cobra.MaximumNArgs(1) to reject extra args, fix nil context guard (use || not &&) so missing context returns an error instead of falling through - cmd/root.go: add defensive check for empty sub.Use field, add backward-compatible fallback to legacy DEVPOD_<FLAG> env vars when DEVPOD_<SUBCMD>_<FLAG> is not set - providers/kubernetes/provider.yaml: fix "support" -> "supports" grammar
The backward-compat fallback for legacy DEVPOD_<FLAG> env vars would compute DEVPOD_DEVPOD_HOME for the devpod-home flag, since the prefix guard already resolves it to DEVPOD_HOME. Skip the fallback when the flag name already starts with the env prefix.
The PR added a providerName parameter to ToEnvironment to set DEVPOD_PROVIDER in the provider's environment, but the custom driver call site passed an empty string. Use workspace.Workspace.Provider.Name so custom driver commands also receive the provider name.
This reverts commit a201a0d.
The custom driver call site passes an empty string for providerName, so DEVPOD_PROVIDER is never set in the environment for custom driver commands. Use workspace.Workspace.Provider.Name to match the behavior of other call sites. Depends on #615
Revert the commandPrefix changes to inheritCommandFlagsFromEnvironment and inheritFlagsFromEnvironment back to main's behavior. The scoping feature (DEVPOD_UP_IDE instead of DEVPOD_IDE) is a breaking change that requires a legacy fallback adding unnecessary complexity. This can be revisited as a separate, properly documented change.
Prevents silently ignoring extra arguments when calling `devpod provider delete`. Mirrors the same constraint already present in add.go.
Avoids collision with the --provider CLI flag which maps to DEVPOD_PROVIDER via inheritFlagsFromEnvironment in cmd/root.go. Child devpod processes were having their --provider flag overridden by the subprocess env injection.
PROVIDER_ID (set by GetBaseEnvironment) already carries the provider name into every subprocess environment. The additional providerName parameter and DEVPOD_PROVIDER_NAME env var were redundant and risked confusion with the DEVPOD_PROVIDER flag-mapped env var.
The provider source can be passed directly as a CLI argument. The env var fallback added unnecessary complexity and namespace collision concerns without solving a real problem.
DEVPOD_PROVIDER_<provider name>_SOURCEenvironment variableSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Style