refactor(cli)!: harmonize provider verbs and add rm/mv aliases - #469
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ 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:
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 |
014dfd8 to
1572eef
Compare
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)
docs/pages/managing-providers/add-provider.mdx (1)
112-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate descriptive text to match the new command name.
The text mentions "during the
configureoraddphase" but the command has been renamed toinit. Update the wording for consistency.📝 Proposed fix
-Options for providers can be set during the `configure` or `add` phase: +Options for providers can be set during the `init` or `add` phase:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/pages/managing-providers/add-provider.mdx` at line 112, Update the descriptive sentence that currently reads "Options for providers can be set during the `configure` or `add` phase" to reflect the new command name by replacing `add` with `init` (so it reads "Options for providers can be set during the `configure` or `init` phase"); ensure the inline code backticks around `configure` and `init` are preserved and update any nearby mentions in the same paragraph if present to keep wording consistent with the renamed command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/provider/set_source.go`:
- Around line 77-87: The call to ConfigureProvider in set-source.go is missing
Reconfigure: true so ConfigureProvider will merge prior user options (via
mergeExistingOptions) into the new source/schema; update the
ProviderOptionsConfig passed to ConfigureProvider to include Reconfigure: true
(and explicitly set other default fields as needed) so the provider option
resolution behaves like provider add/update-provider and does not carry stale
values; locate the ConfigureProvider invocation in set-source.go and modify the
ProviderOptionsConfig struct to include Reconfigure: true.
---
Outside diff comments:
In `@docs/pages/managing-providers/add-provider.mdx`:
- Line 112: Update the descriptive sentence that currently reads "Options for
providers can be set during the `configure` or `add` phase" to reflect the new
command name by replacing `add` with `init` (so it reads "Options for providers
can be set during the `configure` or `init` phase"); ensure the inline code
backticks around `configure` and `init` are preserved and update any nearby
mentions in the same paragraph if present to keep wording consistent with the
renamed command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 474450c1-c273-4d8b-bca7-fa1e9cf33b28
📒 Files selected for processing (26)
cmd/agent/workspace/delete.gocmd/context/delete.gocmd/machine/delete.gocmd/pro/provider/delete.gocmd/provider/add.gocmd/provider/delete.gocmd/provider/delete_test.gocmd/provider/init.gocmd/provider/init_test.gocmd/provider/provider.gocmd/provider/rename.gocmd/provider/set_source.gocmd/provider/set_source_test.gocmd/provider/update.gocmd/workspace/delete.gocmd/workspace/rename.godesktop/src/main/ipc.tsdocs/pages/managing-providers/add-provider.mdxdocs/pages/managing-providers/set-source.mdxdocs/pages/managing-providers/what-are-providers.mdxdocs/sidebars.jse2e/framework/command.goe2e/tests/provider/provider.goexamples/ztunnel/README.mdpkg/provider/provider.gopkg/workspace/workspace.go
💤 Files with no reviewable changes (1)
- cmd/provider/update.go
| if err := ConfigureProvider(ctx, ProviderOptionsConfig{ | ||
| Provider: providerConfig, | ||
| Context: devsyConfig.DefaultContext, | ||
| UserOptions: cmd.Options, | ||
| }); err != nil { | ||
| log.Errorf( | ||
| "Error initializing provider, retry with 'devsy provider init %s --reconfigure'", | ||
| providerConfig.Name, | ||
| ) | ||
| return fmt.Errorf("configure provider: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Recover the removed update.go from git history and inspect its ConfigureProvider call.
del=$(git rev-list -n1 HEAD -- cmd/provider/update.go)
if [ -n "$del" ]; then
echo "Last commit touching update.go: $del"
echo "===== content prior to deletion ====="
git show "${del}^:cmd/provider/update.go" 2>/dev/null | sed -n '1,200p'
else
echo "No git history found for cmd/provider/update.go"
fiRepository: devsy-org/devsy
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check whether cmd/provider/update.go exists =="
ls -la cmd/provider | sed -n '1,200p' || true
test -f cmd/provider/update.go && echo "FOUND: cmd/provider/update.go" || echo "MISSING: cmd/provider/update.go"
echo
echo "== Find all ConfigureProvider calls in cmd/provider =="
rg -n "ConfigureProvider\\(" cmd/provider -S || true
echo
echo "== Search for Reconfigure: true / SkipRequired / SkipInit / SkipSubOptions / SingleMachine usage =="
rg -n "Reconfigure\\s*:\\s*true|SkipRequired|SkipInit|SkipSubOptions|SingleMachine" cmd/provider -S || true
echo
echo "== Search for 'provider update' command wiring =="
rg -n "provider\\s+update|update\\b" cmd/provider -S || true
echo
echo "== Inspect cmd/provider/set_source.go around ConfigureProvider call =="
sed -n '1,140p' cmd/provider/set_source.go || trueRepository: devsy-org/devsy
Length of output: 7111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect cmd/provider/configure_shared.go =="
sed -n '1,220p' cmd/provider/configure_shared.go
echo
echo "== Inspect cmd/provider/add.go around ConfigureProvider call =="
sed -n '90,170p' cmd/provider/add.go
echo
echo "== Inspect cmd/provider/init.go around ConfigureProvider call =="
sed -n '1,130p' cmd/provider/init.go
echo
echo "== Inspect cmd/provider/set_source_test.go to see expected behavior =="
sed -n '1,220p' cmd/provider/set_source_test.goRepository: devsy-org/devsy
Length of output: 10191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for workspace.UpdateProvider usage =="
rg -n "workspace\.UpdateProvider" -S . || true
echo
echo "== Search for ConfigureProvider calls with Reconfigure flags =="
rg -n "ConfigureProvider\\(ctx, ProviderOptionsConfig\\{" cmd/provider -S || true
rg -n "Reconfigure\\s*:\\s*(true|false)" cmd/provider -S || true
echo
echo "== Search for 'update' command wiring in cmd/provider =="
rg -n "\\bprovider\\s+update\\b|\\bUpdateCmd\\b|Use:\\s*\"update\"" cmd/provider -S || true
echo
echo "== Inspect cmd/provider/set.go for related reconfigure logic =="
sed -n '1,220p' cmd/provider/set.go
echo
echo "== Inspect cmd/provider/provider.go for command registration =="
sed -n '1,240p' cmd/provider/provider.goRepository: devsy-org/devsy
Length of output: 5179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect cmd/pro/update_provider.go =="
sed -n '1,220p' cmd/pro/update_provider.go
echo
echo "== Inspect workspace.UpdateProvider implementation =="
rg -n "func UpdateProvider\\(" -S ./pkg ./cmd || true
fd -e go UpdateProvider -a ./pkg ./cmd | head -n 20 || true
# Print the exact file that contains the function, if found
f=$(rg -l "func UpdateProvider\\(" -S ./pkg ./cmd | head -n 1 || true)
if [ -n "${f}" ]; then
echo
echo "== File: $f =="
sed -n '1,260p' "$f"
fiRepository: devsy-org/devsy
Length of output: 9187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for any provider command named update =="
rg -n 'Use:\s*"update' cmd/provider -S || true
echo
echo "== Search for ConfigureProvider calls in cmd/provider excluding set_source.go/add.go =="
rg -n "ConfigureProvider\\(ctx, ProviderOptionsConfig\\{" cmd/provider -S || true
echo
echo "== Search for 'Update' in cmd/provider file names =="
ls -ლა cmd/provider | sed -n '1,200p'
echo
echo "== Search for 'reconfigure' flag wiring in cmd/provider =="
rg -n 'reconfigure' cmd/provider -S || trueRepository: devsy-org/devsy
Length of output: 510
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate ResolveOptions implementation =="
rg -n "func ResolveOptions\\(" -S pkg | head -n 20
f=$(rg -l "func ResolveOptions\\(" pkg | head -n 1 || true)
if [ -n "$f" ]; then
echo
echo "== File: $f =="
sed -n '1,260p' "$f"
fi
echo
echo "== Search how resolve options handles unknown keys / required options =="
rg -n "unknown|not provided|required|SkipRequired|SkipSubOptions" -S "$f" || trueRepository: devsy-org/devsy
Length of output: 7492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect cmd/provider/set_source_test.go =="
sed -n '1,260p' cmd/provider/set_source_test.go
echo
echo "== Search for set-source usage in tests =="
rg -n "set-source" -S . || true
echo
echo "== Search for provider init --reconfigure references =="
rg -n "init .*--reconfigure|--reconfigure.*provider init|provider init.*--reconfigure" -S . || trueRepository: devsy-org/devsy
Length of output: 2110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect docs for provider set-source =="
sed -n '1,120p' docs/pages/managing-providers/set-source.mdx
echo
echo "== Inspect resolver behavior for unknown/extra options =="
rg -n "unknown option|not provided|not defined|extra option|unrecognized|skip.*unknown" pkg/options -S || true
echo
echo "== Locate resolver.New and its handling of input option keys =="
rg -n "func New\\(" pkg/options/resolver -S || true
f=$(rg -l "package resolver" pkg/options/resolver | head -n 1 || true)
echo "resolver dir sample file: $f"
# Print key resolver files (small-ish)
ls pkg/options/resolver 2>/dev/null | head -n 50 || true
for f in $(ls pkg/options/resolver 2>/dev/null | head -n 20); do
echo "---- pkg/options/resolver/$f ----"
sed -n '1,220p' "pkg/options/resolver/$f" | head -n 220
doneRepository: devsy-org/devsy
Length of output: 28043
set-source should likely pass Reconfigure: true after swapping provider source
cmd/provider/set_source.go calls ConfigureProvider without Reconfigure, so configure_shared.go will merge existing user-provided option values into the new option resolution (mergeExistingOptions when !cfg.Reconfigure). This differs from provider add (sets Reconfigure: true) and pro update-provider (also sets Reconfigure: true). Consider setting Reconfigure: true (and explicitly setting other defaults for clarity) to avoid carrying stale user-provided option values across source/schema changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/provider/set_source.go` around lines 77 - 87, The call to
ConfigureProvider in set-source.go is missing Reconfigure: true so
ConfigureProvider will merge prior user options (via mergeExistingOptions) into
the new source/schema; update the ProviderOptionsConfig passed to
ConfigureProvider to include Reconfigure: true (and explicitly set other default
fields as needed) so the provider option resolution behaves like provider
add/update-provider and does not carry stale values; locate the
ConfigureProvider invocation in set-source.go and modify the
ProviderOptionsConfig struct to include Reconfigure: true.
Match the behavior of 'provider add' and 'pro update-provider', which both pass Reconfigure: true when the provider source changes. Without this, mergeExistingOptions carries forward the user's prior option values into the new source's option schema — which may have different keys or types — silently leaking stale state. Surfaced by CodeRabbit on #469.
Two changes:
1. Remove the `log.Errorf("Error initializing provider, retry with
'devsy provider init %s --reconfigure'")` line. The wrapped error
from `fmt.Errorf("configure provider: %w", err)` already conveys the
failure to cobra's error output; the additional log line was
duplicate noise.
2. Document why `Reconfigure: false` (zero value) is safe on
set-source despite CodeRabbit's concern on #469. The downstream
resolver (pkg/options/resolver) already prunes keys absent from the
new schema (resolver.go:153-157) and re-resolves values that fail
type validation against the new schema (resolve.go:207-215), so
stale data cannot leak through this path. The 'Reconfigure' flag
only controls whether previously user-provided values are
carried forward for prompts that still apply -- not whether stale
data is pruned.
Drops the original PR's Reconfigure: true flip after investigation
showed the resolver already handles the case it was meant to address;
forcing reconfigure would just re-prompt users for options whose
values are still valid for the new source.
…orValues The 'Reconfigure' flag on ProviderOptionsConfig was opaque: it sounded like "should we run init again?" (which is what SkipInit controls) but actually toggled whether mergeExistingOptions seeds the new prompts with the user's previous answers. CodeRabbit reading PR #469 reached for 'Reconfigure: true' on 'provider set-source' defensively, assuming it prevented stale data leaking through the merge — but the downstream resolver already prunes stale keys and re-resolves invalid values regardless of this flag. Renaming the internal field to DiscardPriorValues makes the mechanism self-documenting at every call site: - cmd/provider/add.go: DiscardPriorValues=true (fresh install; no prior values to merge anyway, kept explicit for clarity) - cmd/provider/set_source.go: omitted (zero value); preserves prior values, resolver handles stale-key pruning - cmd/provider/init.go: bound to user-facing --reconfigure flag - cmd/provider/set.go: same - cmd/pro/update_provider.go: DiscardPriorValues=true (automated version bump, re-resolves from new schema defaults) - cmd/pro/login.go: omitted (zero value) User-facing --reconfigure flags on 'provider init' and 'provider set' keep their names; the rename is internal only. Also drops the redundant log.Errorf on set-source failure (the wrapped error already conveys the failure). Originally proposed adding Reconfigure: true to set-source per CodeRabbit feedback on #469; investigation showed that the resolver (pkg/options/resolver/resolver.go:153-157 and resolve.go:207-215) already prunes keys absent from the new schema and re-resolves values that fail validation, so the original Reconfigure: false on 'provider update' was correct and the rename preserves that behavior with clearer code.
…orValues The 'Reconfigure' flag on ProviderOptionsConfig was opaque: it sounded like "should we run init again?" (which is what SkipInit controls) but actually toggled whether mergeExistingOptions seeds the new prompts with the user's previous answers. CodeRabbit reading PR #469 reached for 'Reconfigure: true' on 'provider set-source' defensively, assuming it prevented stale data leaking through the merge — but the downstream resolver already prunes stale keys and re-resolves invalid values regardless of this flag. Renaming the internal field to DiscardPriorValues makes the mechanism self-documenting at every call site: - cmd/provider/add.go: DiscardPriorValues=true (fresh install; no prior values to merge anyway, kept explicit for clarity) - cmd/provider/set_source.go: omitted (zero value); preserves prior values, resolver handles stale-key pruning - cmd/provider/init.go: bound to user-facing --reconfigure flag - cmd/provider/set.go: same - cmd/pro/update_provider.go: DiscardPriorValues=true (automated version bump, re-resolves from new schema defaults) - cmd/pro/login.go: omitted (zero value) User-facing --reconfigure flags on 'provider init' and 'provider set' keep their names; the rename is internal only. Also drops the redundant log.Errorf on set-source failure (the wrapped error already conveys the failure). Originally proposed adding Reconfigure: true to set-source per CodeRabbit feedback on #469; investigation showed that the resolver (pkg/options/resolver/resolver.go:153-157 and resolve.go:207-215) already prunes keys absent from the new schema and re-resolves values that fail validation, so the original Reconfigure: false on 'provider update' was correct and the rename preserves that behavior with clearer code.
…orValues The 'Reconfigure' flag on ProviderOptionsConfig was opaque: it sounded like "should we run init again?" (which is what SkipInit controls) but actually toggled whether mergeExistingOptions seeds the new prompts with the user's previous answers. CodeRabbit reading PR #469 reached for 'Reconfigure: true' on 'provider set-source' defensively, assuming it prevented stale data leaking through the merge — but the downstream resolver already prunes stale keys and re-resolves invalid values regardless of this flag. Renaming the internal field to DiscardPriorValues makes the mechanism self-documenting at every call site: - cmd/provider/add.go: DiscardPriorValues=true (fresh install; no prior values to merge anyway, kept explicit for clarity) - cmd/provider/set_source.go: omitted (zero value); preserves prior values, resolver handles stale-key pruning - cmd/provider/init.go: bound to user-facing --reconfigure flag - cmd/provider/set.go: same - cmd/pro/update_provider.go: DiscardPriorValues=true (automated version bump, re-resolves from new schema defaults) - cmd/pro/login.go: omitted (zero value) User-facing --reconfigure flags on 'provider init' and 'provider set' keep their names; the rename is internal only. Also drops the redundant log.Errorf on set-source failure (the wrapped error already conveys the failure). Originally proposed adding Reconfigure: true to set-source per CodeRabbit feedback on #469; investigation showed that the resolver (pkg/options/resolver/resolver.go:153-157 and resolve.go:207-215) already prunes keys absent from the new schema and re-resolves values that fail validation, so the original Reconfigure: false on 'provider update' was correct and the rename preserves that behavior with clearer code.
…orValues The 'Reconfigure' flag on ProviderOptionsConfig was opaque: it sounded like "should we run init again?" (which is what SkipInit controls) but actually toggled whether mergeExistingOptions seeds the new prompts with the user's previous answers. CodeRabbit reading PR #469 reached for 'Reconfigure: true' on 'provider set-source' defensively, assuming it prevented stale data leaking through the merge — but the downstream resolver already prunes stale keys and re-resolves invalid values regardless of this flag. Renaming the internal field to DiscardPriorValues makes the mechanism self-documenting at every call site: - cmd/provider/add.go: DiscardPriorValues=true (fresh install; no prior values to merge anyway, kept explicit for clarity) - cmd/provider/set_source.go: omitted (zero value); preserves prior values, resolver handles stale-key pruning - cmd/provider/init.go: bound to user-facing --reconfigure flag - cmd/provider/set.go: same - cmd/pro/update_provider.go: DiscardPriorValues=true (automated version bump, re-resolves from new schema defaults) - cmd/pro/login.go: omitted (zero value) User-facing --reconfigure flags on 'provider init' and 'provider set' keep their names; the rename is internal only. Also drops the redundant log.Errorf on set-source failure (the wrapped error already conveys the failure). Originally proposed adding Reconfigure: true to set-source per CodeRabbit feedback on #469; investigation showed that the resolver (pkg/options/resolver/resolver.go:153-157 and resolve.go:207-215) already prunes keys absent from the new schema and re-resolves values that fail validation, so the original Reconfigure: false on 'provider update' was correct and the rename preserves that behavior with clearer code.
Summary
Hard-cut breaking change to the devsy CLI verb surface:
devsy provider configure→devsy provider initdevsy provider update→devsy provider set-sourcermas an alias fordeleteonworkspace,context,machine,provider,pro provider, andagent workspacemvas an alias forrenameonworkspaceandproviderNo deprecation shims — old verbs are removed entirely. Desktop IPC, E2E framework, in-tree Go callers, and markdown docs all updated to the new verbs.
Why
configurewas a false friend — most CLIs use it for "set my preferences," but here it meant "re-init."updatewas misleading — sounded like "fetch newer version" but actually swapped the source URL. The other provider verbs (get,set,list,use) already matchedcontext/ideexactly; this brings the remaining two outliers in line.lswas already aliased on everylistcommand —rmandmvclose the analogous gaps.Summary by CodeRabbit
New Features
rmalias for workspace, context, machine, and provider delete commands.mvalias for workspace and provider rename commands.provider set-sourcecommand for updating provider sources and pinning to specific versions.Documentation