Feature #274 Rename provider #358
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 rename and workspace rebind capabilities across CLI, backend, frontend, docs, and E2E tests: new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant ProviderMgr
participant Workspaces
User->>CLI: devpod provider rename oldName newName
CLI->>Config: Load config & validate newName
CLI->>Config: Ensure newName absent
rect rgba(100,150,200,0.5)
CLI->>ProviderMgr: Clone provider oldName -> newName
ProviderMgr->>Config: Persist new provider
end
CLI->>Config: List workspaces bound to oldName
loop For each workspace
CLI->>Workspaces: Load workspace config
CLI->>Workspaces: Update workspace provider -> newName
Workspaces->>Config: Save workspace config
end
rect rgba(100,200,150,0.5)
CLI->>Config: If default == oldName set default -> newName
CLI->>ProviderMgr: Delete oldName provider
CLI->>User: Success
end
rect rgba(200,150,100,0.5)
CLI->>Workspaces: On error revert workspace bindings -> oldName
CLI->>Config: Restore default if changed
CLI->>ProviderMgr: Delete cloned newName provider
CLI->>User: Failure — rolled back
end
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
53-53: Regex bug: double backslash creates incorrect pattern.The regex
/^[a-z0-9\\-]+$/uses a double backslash, which in a regex literal matches a literal backslash character followed by a hyphen—not just a hyphen. This will reject valid names likemy-providerand accept invalid names likemy\-provider.Additionally, this duplicates
ALLOWED_NAMES_REGEXalready exported fromhelpers.ts. Import it instead.🐛 Proposed fix
Remove the local definition and import from helpers:
-import { exists, isError, randomString, useFormErrors } from "../../../lib" +import { exists, isError, randomString, useFormErrors, ALLOWED_NAMES_REGEX } from "../../../lib"Then remove line 53:
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/Alternatively, if
ALLOWED_NAMES_REGEXis not re-exported from../../../lib, import directly from./helpers:-import { FieldName, TFormValues, TSetupProviderResult } from "./types" +import { FieldName, TFormValues, TSetupProviderResult } from "./types" +import { ALLOWED_NAMES_REGEX } from "./helpers"
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 43-47: Update the help/warning text for the "provider rename"
command (the help block in cmd/provider/rename.go used by the provider rename
command) to remove the instruction that users must manually edit workspaces and
instead state that the command will attempt to automatically rebind workspaces
to the new provider name, while advising users to verify workspace configs and
note any potential edge cases/failures that may require manual intervention;
ensure the revised wording replaces the existing WARNING paragraph so it clearly
reflects automatic rebinding and suggests verification steps.
- Around line 86-100: The loop that updates workspaces currently logs failures
from provider.SaveWorkspaceConfig and continues, then calls
provider.RenameProvider which can orphan workspaces; change the logic in the
block iterating workspaces so that any error returned by
provider.SaveWorkspaceConfig for a workspace causes the command to abort (or
accumulate and return an aggregated error) before calling
provider.RenameProvider; specifically, in the loop over workspaces where you
check ws.Provider.Name == oldName, stop and return the error (or collect it and
return after the loop) instead of only logging in the
ws.Provider.SaveWorkspaceConfig error path so provider.RenameProvider is only
called when all workspace rebinds succeeded.
In `@cmd/workspace/rebind.go`:
- Around line 36-46: The code is passing the raw workspace name to
provider.LoadWorkspaceConfig (and later to the save path) but
provider.LoadWorkspaceConfig expects a workspace ID; convert the name to an ID
first using workspace.ToID and use that ID when calling
provider.LoadWorkspaceConfig(devPodConfig.DefaultContext, workspaceID) and when
later saving (e.g., provider.SaveWorkspaceConfig). Replace usages of
workspaceName in LoadWorkspaceConfig/SaveWorkspaceConfig calls with the
normalized workspaceID returned by workspace.ToID, and propagate any conversion
errors appropriately.
In `@desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx`:
- Around line 105-118: The current handleSave sequence renames before
configuring which can leave the UI on the old route if configure fails; change
the sequence to call client.providers.configure(finalProviderID/config) first
(using providerID), then call client.providers.rename(providerID, newName) if
needed, update finalProviderID accordingly, invalidate QueryKeys.PROVIDERS after
both succeed, and call navigate(Routes.toProvider(newName), { replace: true })
only after rename succeeds; ensure you reference handleSave,
client.providers.configure, client.providers.rename, QueryKeys.PROVIDERS and
Routes.toProvider when making the change.
In `@desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx`:
- Around line 377-381: The validation message in SetupProviderSourceForm using
ALLOWED_NAMES_REGEX is misleading because the regex only allows lowercase
letters; update the user-facing error text to accurately reflect the rule (e.g.,
"Name can only contain lowercase letters, numbers and -") or alternatively
broaden ALLOWED_NAMES_REGEX to allow uppercase if uppercase should be permitted;
modify the message string next to the rules.pattern block (the rule inside
SetupProviderSourceForm.tsx) and ensure it matches the actual
ALLOWED_NAMES_REGEX behavior.
In `@e2e/tests/provider/provider.go`:
- Line 21: The test uses an incorrect relative path when calling
framework.CopyToTempDir, so update the string argument from
"testdata/simple-k8s-provider" to "tests/provider/testdata/simple-k8s-provider"
wherever it is used in this file (all four occurrences that call
framework.CopyToTempDir, e.g., the calls that assign tempDir and err); keep the
rest of the call and error handling intact.
In `@e2e/tests/workspace/rebind_test.go`:
- Around line 50-59: The test derives workspaceID with filepath.Base(tempDir)
but DevPod uses workspace.ToID() (see ToID in pkg/workspace) so Stop/Rebind/SSH
may target the wrong workspace; update the test to consistently transform the
derived ID using workspace.ToID() before passing it to f.DevPodStop and
f.DevPodWorkspaceRebind (or alternatively pass tempDir directly like
f.DevPodWorkspaceDelete does), ensuring the same identifier normalization used
by ToID() is applied when calling DevPodStop, DevPodWorkspaceRebind, and any
subsequent workspace operations.
In `@pkg/provider/dir.go`:
- Around line 453-466: Currently the code saves the new provider config then
deletes the old provider directory, which discards binaries/state; instead,
obtain oldProviderDir via GetProviderDir(context, oldName) and move it to the
new provider directory using os.Rename (or shutil-equivalent on cross-filesystem
if needed), then call SaveProviderConfig(context, providerConfig) to write
provider.json into the moved directory (or update provider.json in its new
path). Ensure you replace the final os.RemoveAll(oldProviderDir) with the rename
operation and update any path references so the provider daemon/binaries remain
intact.
- Around line 435-442: The current check treats any os.Stat error other than
IsNotExist as "already exists"; change the logic in the rename/create flow that
uses GetProviderDir, newProviderDir and os.Stat so you explicitly handle three
cases: if os.Stat returns nil then return the "provider with name %s already
exists" error; if os.IsNotExist(err) continue (allowed); otherwise return the
underlying filesystem error (wrap or propagate) so permission/I/O errors are
surfaced instead of being misclassified as "already exists".
♻️ Duplicate comments (1)
e2e/tests/provider/rename_test.go (1)
143-145: Workspace identifier mismatch risk (path vs ID).Line 144 passes
tempDirtoDevPodSSH. If the helper expects a workspace ID (as used in other tests), this will be flaky. Align with the identifier the framework expects.
🧹 Nitpick comments (4)
e2e/framework/helper.go (1)
8-10: Consider differentiating from the existingDevPodDescribein integration tests.A
DevPodDescribefunction already exists ine2e/tests/integration/framework.gothat adds an"[integration] "prefix to the description. This new generic version in the sharedframeworkpackage has no prefix, which could cause confusion when both packages are imported or when developers expect consistent test output formatting.Consider either:
- Adding a distinguishing prefix (e.g.,
"[workspace] ") for workspace tests- Renaming this function to clarify its generic nature
- Removing the integration-specific version and having all tests use this generic one with explicit prefixes
e2e/framework/command.go (1)
391-395: Add context to rebind failures for easier triage.Returning the raw error drops command context in test logs. Consider wrapping the error (and optionally stderr) to match other helpers.
♻️ Suggested tweak
func (f *Framework) DevPodWorkspaceRebind(ctx context.Context, workspaceName, newProvider string, extraArgs ...string) error { args := []string{"workspace", "rebind", workspaceName, newProvider} args = append(args, extraArgs...) - _, _, err := f.ExecCommandCapture(ctx, args) - return err + _, _, err := f.ExecCommandCapture(ctx, args) + if err != nil { + return fmt.Errorf("devpod workspace rebind failed: %s", err.Error()) + } + return nil }desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
150-151: Hide the name field when configuring workspace-specific options.In the workspace flow,
onSaveis a noop, so editing the name currently does nothing. Consider rendering the name field only for provider-level configuration to avoid misleading the user.♻️ Suggested UI guard
- <FormControl> - <FormLabel>Provider Name</FormLabel> - <Input {...formMethods.register(FieldName.NAME)} /> - </FormControl> + {workspace === undefined && ( + <FormControl> + <FormLabel>Provider Name</FormLabel> + <Input {...formMethods.register(FieldName.NAME)} /> + </FormControl> + )}Also applies to: 166-167, 270-273
docs/pages/managing-providers/rename-provider.md (1)
39-39: Consider adding a warning about workspace impact.The CLI help text in
cmd/provider/rename.goincludes an important warning: "WARNING: Renaming a provider will cause all workspaces bound to it to fail. You will have to manually edit the workspace configurations to use the new provider name."This documentation should include a similar warning or note so users are aware of the potential impact on existing workspaces, particularly for CLI users who may not see the help text.
📝 Suggested addition
After renaming, DevPod will automatically update the provider's configuration. + +:::warning +Renaming a provider will cause all workspaces bound to it to fail. You may need to rebind workspaces to use the new provider name using `devpod workspace rebind`. +:::
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx
Show resolved
Hide resolved
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)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
53-53: Bug: Double backslash allows literal backslash characters in names.The regex
/^[a-z0-9\\-]+$/uses\\-which in a JS regex literal matches a literal backslash plus hyphen. This differs from the existingALLOWED_NAMES_REGEXinhelpers.tswhich correctly uses/^[a-z0-9\-]+$/.Import from
helpers.tsinstead of duplicating and introducing this bug:🔧 Suggested fix
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/Then update the import on line 45 to include
ALLOWED_NAMES_REGEX:import { exists, isError, randomString, useFormErrors, ALLOWED_NAMES_REGEX } from "../../../lib"Or import directly from the helpers file if
ALLOWED_NAMES_REGEXisn't re-exported throughlib:import { ALLOWED_NAMES_REGEX } from "./helpers"
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 90-112: The provider is being renamed before workspaces are
rebound which can orphan workspaces if any rebind fails; change the sequence so
you iterate workspacesToRebind and call provider.SaveWorkspaceConfig (updating
ws.Provider.Name = newName) for each workspace first, collecting rebindErrors
and successfulRebinds, and only if all required rebinding succeeds call
provider.RenameProvider(devPodConfig.DefaultContext, oldName, newName);
additionally, if you keep partial-rebind behavior, implement rollback logic to
restore any successfulRebinds back to oldName (using SaveWorkspaceConfig) when
provider.RenameProvider fails so no workspace is left referencing a non-existent
provider.
🧹 Nitpick comments (2)
cmd/provider/rename.go (1)
22-51: Excessive blank lines reduce readability.Multiple unnecessary blank lines between statements (e.g., lines 23, 25, 30, 45, 47) create visual clutter. Consider removing the extra blank lines for a cleaner structure.
♻️ Suggested cleanup
func NewRenameCmd(globalFlags *flags.GlobalFlags) *cobra.Command { - cmd := &RenameCmd{ - GlobalFlags: globalFlags, } return &cobra.Command{ - Use: "rename", - Short: "Rename a provider", - Long: `Renames a provider and automatically rebinds all workspaces that are bound to it to use the new provider name. Example: devpod provider rename my-provider my-new-provider `, - Args: cobra.ExactArgs(2), - RunE: func(cobraCmd *cobra.Command, args []string) error { - return cmd.Run(cobraCmd, args) - }, } - }desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
272-275: Consider defaulting to providerID for better UX.When
nameprop is undefined, the input renders empty. Users editing an existing provider may expect to see the current provider name. Consider defaulting toproviderIDwhennameis not provided.♻️ Suggested default
const formMethods = useForm<TFieldValues>({ defaultValues: { reuseMachine, useAsDefault: isDefault, - name: name, + name: name ?? providerID, }, })
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 145-150: When renaming a provider, if oldName is currently the
default, switch the default to newName before calling DeleteProviderConfig to
avoid clearing the user's default selection; detect this by checking the current
default provider (e.g., compare stored defaultName to oldName) and call the
appropriate setter to update the default to newName, then proceed to call
DeleteProviderConfig(devPodConfig, oldName, true) and handle errors as before
(use DeleteProviderConfig, oldName, newName to locate the logic).
|
Please fix failing checks first |
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 103-116: The rollback function currently returns immediately if
rebindWorkspaces(workspacesTouched, oldName) fails, leaving the cloned provider
(newName) orphaned; change rollback to always attempt
DeleteProviderConfig(devPodConfig, newName, true) even when rebindWorkspaces
returns an error, capture both errors (the rebind error and any delete error),
and return an aggregated error (or combined message) so callers know both
failures; reference the rollback function, the rebindWorkspaces call, and
DeleteProviderConfig invocation when making the change.
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 (2)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
52-52: Regex allows unintended backslash characters in names.The double backslash
\\-in the character class matches a literal backslash character plus a hyphen. This means names likemy\providerwould pass validation, which could cause issues with file paths or command-line operations.🐛 Suggested fix
-const ALLOWED_NAMES_REGEX = /^[a-z0-9\\-]+$/ +const ALLOWED_NAMES_REGEX = /^[a-z0-9-]+$/A hyphen at the end of a character class doesn't require escaping.
desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (1)
150-167: Hide/guard Provider Name in workspace mode to avoid a no‑op rename.In workspace mode,
onSaveis a no‑op, so the new Provider Name field gives the impression a rename will happen when it won’t. Also, passing an empty/whitespace name can lead to confusing UX or backend errors. Consider hiding (or disabling) the field whenworkspaceis set and only forwarding a trimmed non-emptynewName.✅ Suggested fix
const handleSubmit: FormEventHandler<HTMLFormElement> = (event) => { // make sure we don't bubble up the event to the parent event.stopPropagation() event.preventDefault() formMethods.handleSubmit((data) => { const { useAsDefault, reuseMachine, name } = data + const trimmedName = typeof name === "string" ? name.trim() : undefined + const newName = trimmedName && trimmedName !== providerID ? trimmedName : undefined configureProvider({ providerID, config: { reuseMachine: reuseMachine ?? false, useAsDefaultProvider: useAsDefault, options: filterOptions(data, allOptions), }, - newName: name as string, + newName, }) })(event) } @@ - <VStack align="start" spacing={8} position="relative" width="full"> - <FormControl> - <FormLabel>Provider Name</FormLabel> - <Input {...formMethods.register(FieldName.NAME)} /> - </FormControl> + <VStack align="start" spacing={8} position="relative" width="full"> + {!workspace && ( + <FormControl> + <FormLabel>Provider Name</FormLabel> + <Input {...formMethods.register(FieldName.NAME)} /> + </FormControl> + )}Also applies to: 221-236, 270-273
🧹 Nitpick comments (1)
desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
376-389: Consider using*quantifier for optional field pattern validation.The pattern uses
+(one or more), but this field is optional. While react-hook-form typically skips pattern validation for empty non-required fields, using*(zero or more) would make the intent explicit and guard against edge cases where the field value is an empty string.♻️ Suggested refinement
rules={{ pattern: { - value: ALLOWED_NAMES_REGEX, + value: /^[a-z0-9-]*$/, message: "Name can only contain lowercase letters, numbers and hyphens", },Alternatively, update
ALLOWED_NAMES_REGEXto use*if other usages also need to allow empty strings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 119-121: The error message returned in the provider rename
validation (where provider.ProviderNameRegEx.MatchString(newName) is checked)
uses "smaller case letters"; update that error string to use standard
terminology "lowercase letters" (e.g., change "provider name can only include
smaller case letters, numbers or dashes" to "provider name can only include
lowercase letters, numbers, or dashes") so the message is clearer and
consistent.
🧹 Nitpick comments (2)
pkg/workspace/provider.go (1)
443-460: Consider usingdeferfor unlock to prevent resource leaks.The current lock/unlock pattern doesn't use
defer, which could leave the workspace locked if a panic occurs betweenLock()andUnlock(). Additionally, errors fromUnlock()are silently ignored.♻️ Proposed refactor using defer
// Lock the workspace before switching provider err = client.Lock(ctx) if err != nil { return fmt.Errorf("failed to lock workspace %s: %w", workspace.ID, err) } + defer client.Unlock() // Update the provider name in the workspace configuration workspace.Provider.Name = newProviderName // Save the updated configuration err = providerpkg.SaveWorkspaceConfig(workspace) if err != nil { - client.Unlock() // Unlock before returning error return fmt.Errorf("failed to save workspace config: %w", err) } - // Unlock after successful save - client.Unlock() - return nile2e/framework/command.go (1)
391-399: Wrap the error for consistency with other helper methods.Most other methods in this file wrap errors with context (e.g.,
DevPodProviderRenameuses"devpod provider rename failed: %s"). Returning the raw error makes test failures harder to diagnose.♻️ Proposed fix
func (f *Framework) DevPodWorkspaceRebind( ctx context.Context, workspaceName, newProvider string, extraArgs ...string) error { args := []string{"workspace", "rebind", workspaceName, newProvider} args = append(args, extraArgs...) - _, _, err := f.ExecCommandCapture(ctx, args) - return err + _, stderr, err := f.ExecCommandCapture(ctx, args) + if err != nil { + return fmt.Errorf("devpod workspace rebind failed: %s", stderr) + } + return nil }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 424-454: The status check in SwitchProvider is done before
acquiring the client lock, creating a TOCTOU race; acquire the lock first and
then re-check status while holding it. Change the order in SwitchProvider to
call client.Lock() immediately after Get(...), defer client.Unlock() only after
successful Lock(), then call client.Status(...) and validate status
(client2.StatusStopped or StatusNotFound) before modifying
workspace.Provider.Name and calling providerpkg.SaveWorkspaceConfig(workspace);
preserve the same error wrapping for Lock/Status/Save failures.
🧹 Nitpick comments (1)
cmd/provider/rename.go (1)
114-124: Consider extracting the max name length as a constant.The magic number
32on line 122 could be extracted to a named constant (e.g.,MaxProviderNameLength) for improved maintainability and consistency with other validation constants in the codebase.♻️ Optional refactor
+const MaxProviderNameLength = 32 + // Run executes the command func (cmd *RenameCmd) Run(cobraCmd *cobra.Command, args []string) error { - oldName := args[0] newName := args[1] if provider.ProviderNameRegEx.MatchString(newName) { return fmt.Errorf("provider name can only include lowercase letters, numbers or dashes") } - if len(newName) > 32 { - return fmt.Errorf("provider name cannot be longer than 32 characters") + if len(newName) > MaxProviderNameLength { + return fmt.Errorf("provider name cannot be longer than %d characters", MaxProviderNameLength) }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/workspace/rebind.go`:
- Around line 37-66: Before calling workspace.SwitchProvider, validate that the
requested provider exists in the loaded devPodConfig: check devPodConfig for
newProviderName (e.g., lookup devPodConfig.Providers[newProviderName] or call an
existing helper like devPodConfig.FindProvider/HasProvider) and return a clear
error if it does not exist; update RebindCmd.Run to perform this check after
loading devPodConfig and before workspace.SwitchProvider so you never persist a
workspace config pointing to a non-existent provider.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 123-131: The validateProviderName function currently allows empty
strings because ProviderNameRegEx doesn't catch them; add an explicit check at
the start of validateProviderName (the function named validateProviderName) to
return an error when newName == "" (e.g., "provider name cannot be empty")
before running ProviderNameRegEx.MatchString and the length check so empty names
are rejected and downstream filesystem/key issues are avoided.
In `@e2e/tests/workspace/rebind_test.go`:
- Around line 118-127: The test calls f.DevPodUp then immediately attempts
rebind, which can make the test flaky; update the test to wait until the
workspace is confirmed running before calling DevPodWorkspaceRebind. After
computing workspaceID (workspace.ToID(...)), poll the workspace status (e.g.,
via an existing helper like f.WaitForWorkspaceRunning or by repeatedly calling
f.GetWorkspaceStatus/GetWorkspace until status == "running" with a timeout) and
only then call f.DevPodWorkspaceRebind and framework.ExpectError; reference
DevPodUp, workspace.ToID, and DevPodWorkspaceRebind when adding the wait.
🧹 Nitpick comments (1)
e2e/tests/workspace/rebind_test.go (1)
29-36: Consider registering cleanup immediately to avoid resource leaks.Cleanup only happens at the end, so any earlier failure can leave providers/workspaces behind and make later runs flaky. Prefer
ginkgo.DeferCleanup(ordefer) right after creation.Also applies to: 83-89
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 349-358: The test calls DevPodProviderRename immediately after
DevPodUp, causing flakiness because the workspace may not yet be in
State.Running; add an explicit wait for the workspace to reach Running (using
the same helper used in provider_docker.go — e.g., the test helper that waits
for State.Running or WaitForWorkspaceState/WaitForState on f) after DevPodUp and
before DevPodProviderRename so the rename assertion always runs against a fully
Running workspace.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/provider/rename.go`:
- Around line 89-99: The function adjustDefaultProvider mutates
devPodConfig.Current().DefaultProvider to newName before calling
config.SaveConfig; if SaveConfig returns an error this leaves the in-memory
config pointing to newName and can break rollback logic—change the function so
that after a failed config.SaveConfig you restore
devPodConfig.Current().DefaultProvider back to oldName (do the restore before
returning the error), while keeping the existing logging via log.Default.Errorf
and log.Default.Infof; reference adjustDefaultProvider,
devPodConfig.Current().DefaultProvider, and config.SaveConfig to locate and
implement the rollback.
In `@e2e/tests/provider/provider.go`:
- Around line 328-333: After calling f.DevPodUp(ctx, tempDir) but before
f.DevPodSSH(...), add an explicit wait for the workspace to reach the Running
state (the same pattern used in the RENAME-5 test) to avoid flakiness; use the
existing helper that polls the workspace state (e.g., WaitForDevPodState or
whatever pattern is used elsewhere in this file) to wait until the dev pod is
"Running" and only then call f.DevPodSSH(ctx, tempDir, "echo 'hello'"), and make
sure to propagate and check the returned error with framework.ExpectNoError.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 361-364: The code discards the first deletion error and then
asserts a stale err (from CopyToTempDir); update the two calls to
f.DevPodProviderDelete so each return value is assigned (e.g., err :=
f.DevPodProviderDelete(...)) and immediately checked with
framework.ExpectNoError(err) for providerName and renamedProviderName
respectively, ensuring you reference f.DevPodProviderDelete, providerName,
renamedProviderName, and framework.ExpectNoError rather than reusing the old
err.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@e2e/tests/provider/provider.go`:
- Around line 449-455: The cleanup races because DevPodProviderDelete is invoked
before the workspace teardown finishes; update the cleanup sequence around
f.DevPodWorkspaceDelete(ctx, tempDir) and f.DevPodProviderDelete(ctx,
providerName) to wait for the workspace to be fully deleted before deleting the
provider—implement a short retry/poll loop (with timeout) that checks for
workspace absence (mirror the RENAME-4 deletion wait logic used elsewhere) and
only call f.DevPodProviderDelete after the workspace deletion check succeeds;
keep the existing error checks (framework.ExpectNoError) and surface timeout as
a test failure if the workspace does not disappear in time.
- Around line 338-342: After calling DevPodProviderRename, immediately check and
handle its error (ensure the framework.ExpectNoError(err) is placed right after
DevPodProviderRename) before calling DevPodStatus; then call DevPodStatus(ctx,
tempDir), capture its error and only access or log status.State after verifying
the DevPodStatus error is nil (use framework.ExpectNoError for the DevPodStatus
error) so that failures from DevPodStatus are not masked when logging.
In `@pkg/workspace/provider.go`:
- Around line 595-599: The code currently mutates workspace.Provider.Name to
newProviderName and then calls providerpkg.SaveWorkspaceConfig(workspace); if
SaveWorkspaceConfig returns an error, revert workspace.Provider.Name back to its
original value before returning the error to avoid leaving the in-memory
workspace in a mutated state. To fix, capture the original name (e.g., oldName
:= workspace.Provider.Name) before assignment, assign newProviderName, call
providerpkg.SaveWorkspaceConfig(workspace), and on any non-nil err set
workspace.Provider.Name = oldName and then return the wrapped error.
- Around line 584-599: After acquiring the workspace config client (Get) and
successfully calling client.Lock(), validate the runtime state before changing
provider: call the client's status method (e.g., client.Status(ctx) or
client.GetStatus(ctx)) and if the returned state is neither "Stopped" nor
"NotFound" return an error (ensuring the defer client.Unlock() still runs) to
prevent switching providers while the workspace is running; only then set
workspace.Provider.Name = newProviderName and call
providerpkg.SaveWorkspaceConfig(workspace).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 595-598: The error message returned when client.Status(...) fails
contains a typo ("sataus"); update the fmt.Errorf in the Status call handling
(the block that calls client.Status(ctx, client2.StatusOptions{ContainerStatus:
true})) to read "failed to get status for workspace %s: %w" and keep the same
workspace.ID and wrapped err values.
|
It looks like BaseWorkspaceClient Status on windows is not working properly, so, i receive always StatusNotFound instead of real workspace status, though in UI it is displayed correct. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 600-633: The code uses the undefined alias providerpkg (e.g.,
providerpkg.Workspace and providerpkg.SaveWorkspaceConfig) while the package is
already imported as provider; update SwitchProvider and any other symbols to use
the existing provider import: change the parameter type from
*providerpkg.Workspace to *provider.Workspace and replace
providerpkg.SaveWorkspaceConfig with provider.SaveWorkspaceConfig (and any other
providerpkg.* references) so the code builds and goimports no longer tries to
add a conflicting alias.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 612-627: The lock is acquired with client.Lock(...) but defer
client.Unlock() is registered too late, causing a lock leak on early returns;
move the defer client.Unlock() immediately after the successful client.Lock(ctx)
call so the unlock is guaranteed even if subsequent calls (client.Status(ctx,
client2.StatusOptions{ContainerStatus: true}) or the status checks) return
early; update the block around client.Lock, client.Unlock, client.Status and the
workspace state checks (referencing client.Lock, client.Unlock, client.Status,
client2.StatusOptions and workspace.ID) so Unlock is always deferred right after
Lock succeeds.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/workspace/provider.go`:
- Around line 623-627: The multi-line raw backtick error embeds newline and tabs
into the message; replace the backtick literal with a single-line formatted
error that does not include embedded newlines or tabs (e.g. use
fmt.Errorf("workspace %s is in state %s and cannot be switched: only stopped or
non-existent workspaces can be switched", workspace.ID, status)) or build the
message and strings.TrimSpace it before returning; change the return that
references workspace.ID and status (and the client2.StatusStopped/StatusNotFound
check) to use the single-line message instead.
🧹 Nitpick comments (2)
e2e/tests/provider/provider.go (2)
170-175: Inner context re-declaresctx,initialDir, anderr, shadowing the outer scope.Lines 171–174 use
:=to declarectx,initialDir, anderr, which shadow the identically named variables from lines 17–20. Since theginkgo.Orderedcontext runs within the same outerginkgo.Context, the outer variables are already valid and hold the same values. You could reuse the outer variables directly and drop the re-declaration, or rename the inner ones to make the scoping explicit.Option: Remove the redundant re-declarations
ginkgo.Context("renaming providers", ginkgo.Label("rename"), ginkgo.Ordered, func() { - ctx := context.Background() - initialDir, err := os.Getwd() - if err != nil { - panic(err) - } // RENAME-1.This works because the outer
ctx(line 17) andinitialDir(line 18) are captured by the closure and hold the same values.
301-308: Docker/Podman detection is duplicated across RENAME-4 and RENAME-5.The
DOCKER_HOST/ Podman sniffing block is identical in both tests. Consider extracting a small helper (e.g.,addDockerProvider(f, ctx, name)) in the framework or locally in this file to reduce duplication.Example helper
func addDockerProvider(f *framework.Framework, ctx context.Context, name string) error { dockerHost := os.Getenv("DOCKER_HOST") if dockerHost != "" && strings.Contains(dockerHost, "podman") { return f.DevPodProviderAdd(ctx, "docker", "--name", name, "--option=DOCKER_PATH=podman") } return f.DevPodProviderAdd(ctx, "docker", "--name", name) }Also applies to: 384-391
- command to rename provider - command to rebind workspace to another provider - pkg function to switch provider - UI support for provider rename
blstjohnson
force-pushed
the
feature/274-rename
branch
from
09d0f59 to
e932001
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@desktop/src/lib/validation.ts`:
- Line 3: The PROVIDER_NAME_REGEX currently uses an extra backslash in the
character class which allows literal backslashes; update the regex used by
PROVIDER_NAME_REGEX to remove the escaping of the hyphen/backslash so it only
permits lowercase letters, digits and hyphen (e.g. change the pattern to one
that matches /^[a-z0-9-]+$/) ensuring client-side validation matches the
server-side Go regex in pkg/provider/parse.go.
In `@docs/pages/managing-providers/rename-provider.md`:
- Line 37: Update the rollback description to accurately reflect the
implementation: state that rollback reverts workspace configurations and
restores the default provider setting (if changed), and that the cloned provider
(newName) is deleted only if all workspaces were successfully reverted;
reference the rollback() behavior and the rebindWorkspaces flow to ensure
readers understand that rollback intentionally skips deleting the clone when
workspace rebinding fails to avoid leaving workspaces referencing a non-existent
provider.
🧹 Nitpick comments (5)
cmd/provider/rename.go (2)
112-112: Nit: prefer idiomatic short variable declaration.
var _, err = ...is unusual Go. The idiomatic form is_, err :=.Suggested fix
- var _, err = rebindWorkspaces(devPodConfig, workspacesTouched, oldName) + _, err := rebindWorkspaces(devPodConfig, workspacesTouched, oldName)
67-87: Consider threading the command context throughrebindWorkspaces.
SwitchProvideris called withcontext.Background()(line 78), making the rename non-cancellable once rebinding starts. If a user sends an interrupt (e.g., Ctrl+C), the operation will continue until all workspaces are processed. Consider accepting and forwarding the parent context.Suggested change
func rebindWorkspaces( + ctx context.Context, devPodConfig *config.Config, workspacesToRebind []*provider.Workspace, newName string, ) ([]*provider.Workspace, error) { @@ - err := workspace.SwitchProvider(context.Background(), devPodConfig, ws, newName) + err := workspace.SwitchProvider(ctx, devPodConfig, ws, newName)This would require updating all callers (
cloneAndRebindProvider,rollback,Run) to pass the context through.desktop/src/views/Providers/AddProvider/ConfigureProviderOptionsForm.tsx (2)
273-290: Missing uniqueness validation for the provider name.
SetupProviderSourceFormvalidates that the name is unique against existing providers (lines 380-385 in that file), but this form does not. A user could enter an already-taken name and the error would only surface from the CLI, producing a less friendly UX.♻️ Add a uniqueness check consistent with SetupProviderSourceForm
You can access
providersfromuseProviders(already imported on line 37) and add avalidaterule:<Input {...formMethods.register(FieldName.NAME, { pattern: { value: PROVIDER_NAME_REGEX, message: "Name can only contain lowercase letters, numbers and hyphens", }, maxLength: { value: 32, message: "Name cannot be longer than 32 characters", }, + validate: { + unique: (value) => { + if (!value || value === providerID) return true + return !providers?.[value] || "Name must be unique" + }, + }, })} />Note:
providerswould need to be extracted fromuseProviders()in theConfigureOptionsFormcomponent (or passed down as a prop).
229-239: Unsafeas stringcast onname.
data.nameis typed asstring | boolean | undefined(fromTFieldValues), andnewNameon theonSavesignature acceptsstring | undefined. Theas stringcast is unnecessary and masks the type mismatch — if the field were somehow a boolean, this would silently pass it through.♻️ Safer alternative
- newName: name as string, + newName: typeof name === "string" ? name : undefined,desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
370-406: UseMAX_PROVIDER_NAME_LENGTHconstant instead of hardcoding32.The
MAX_PROVIDER_NAME_LENGTHconstant exists indesktop/src/lib/validation.ts(line 6). Line 387 currently hardcodes32instead of using this constant. SinceSetupProviderSourceFormalready imports from that module, using the constant ensures both the form validation and the constant stay synchronized if the limit is ever updated.♻️ Proposed fix
-import { PROVIDER_NAME_REGEX } from "../../../lib/validation" +import { PROVIDER_NAME_REGEX, MAX_PROVIDER_NAME_LENGTH } from "../../../lib/validation"- maxLength: { value: 32, message: "Name cannot be longer than 32 characters" }, + maxLength: { value: MAX_PROVIDER_NAME_LENGTH, message: `Name cannot be longer than ${MAX_PROVIDER_NAME_LENGTH} characters` },
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/pages/managing-providers/rename-provider.md`:
- Line 38: Fix the typo and grammar in the rollback caveat sentence: replace “If
workspace rebinding on rollback fails, cloned provider will remain, to avoid
orphanted workspaces” with a corrected version that adds the missing article and
fixes the misspelling—e.g. “If workspace rebinding on rollback fails, the cloned
provider will remain to avoid orphaned workspaces.” Ensure the updated sentence
appears in the same paragraph in rename-provider.md.
8cec446 to
cc29619
Compare
|
The failing test on windows is possibly due to an incorrect provider configuration. Windows CI machine uses podman, but provider does not set DOCKER_PATH=podman environment var or option. Struggled finding it, on rename failing tests |
fix: provider name regex fixed added more description in rename documentation
blstjohnson
force-pushed
the
feature/274-rename
branch
from
cc29619 to
b3b2aa0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
New feature to rename providers #274
new command for provider - rename accepting old name and new name, that will validate new name and change the name of the provider
new command workspace rebind - to rebind workspace to a new provider, runs inside provider rename, but can also be used to transfer workspace to a different provider
Summary by CodeRabbit
New Features
UI
API
Documentation
Tests
Chores