allow --provider to override default stack#1862
Conversation
📝 WalkthroughWalkthroughRemoved the Changes
Sequence DiagramsequenceDiagram
participant Cmd as CLI Command
participant Loader as Session Loader
participant SM as Stack Manager
participant Provider as Provider
Cmd->>Loader: Build SessionLoaderOptions (GetStackOpts.ProviderID)
Loader->>SM: Request GetStack(ctx, GetStackOpts)
alt ProviderID present (non-auto)
SM->>Provider: Resolve provider-specific stack
Provider-->>SM: Return stack (whence: provider)
else Interactive selected
SM->>Cmd: Prompt for stack selection
Cmd-->>SM: User selection
else Default / fallback
SM-->>SM: Retrieve default stack or fallback local stack
end
SM-->>Loader: Return resolved stack
Loader-->>Cmd: Return session with resolved stack (or error propagated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.vscode/launch.json:
- Around line 12-19: The "args" array in the launch.json configuration contains
a trailing comma after the commented lines which makes the JSON invalid; open
the "args" array for the launch configuration, remove the trailing comma after
the last active entry ("whoami") or uncomment/add at least one real argument so
the array ends without a dangling comma, then save to ensure the JSONC/JSON
parser no longer errors on the "args" array.
In `@src/cmd/cli/command/config.go`:
- Around line 83-85: Remove the redundant/unreachable error check for the
variable err in the function where the earlier if err != nil { return err }
already returns; specifically delete the second occurrence of "if err != nil {
return err }" (the dead-code check referencing err) so only the first error
handling remains and no duplicate return check is left in the config command
implementation (look for the err variable and the duplicate if err != nil {
return err } inside the function handling config in config.go).
ae711a9 to
39a6c0b
Compare
39a6c0b to
e09be6c
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 (2)
src/cmd/cli/command/session.go (1)
34-38: Verify deploy commands don't silently proceed without a stack manager.Errors from
newStackManagerForLoaderare logged and ignored (lines 36–38). When the stack manager fails,LoadSessionfalls back to using the default "beta" stack instead of failing. This allows deploy commands likecompose upto proceed even when.defangis missing or corrupt, silently using a fallback stack. If stack-based configuration is required for correct deployment behavior, this silent fallback undermines that requirement and should either fail or be made explicit to the user.src/pkg/session/session.go (1)
70-76: Normalize ProviderID to ProviderDefang when no stack manager is available.When
sl.smis nil,loadStackreturnssl.opts.ProviderIDdirectly. If unset (empty string) or "auto", this represents an unresolved provider value. The codebase elsewhere treats both empty string andProviderAutoas requiring resolution (see wizard.go:45, manager.go:216). Whilecli.NewProvidercorrectly defaults toPlaygroundProviderfor these cases, it's clearer to explicitly normalize toProviderDefangin this code path to match the intended default behavior and improve code clarity.🛠️ Suggested fix
if sl.sm == nil { + provider := sl.opts.ProviderID + if provider == "" || provider == client.ProviderAuto { + provider = client.ProviderDefang + } return &stacks.Parameters{ Name: stacks.DefaultBeta, - Provider: sl.opts.ProviderID, + Provider: provider, }, "no stack manager available", nil }
🤖 Fix all issues with AI agents
In `@src/pkg/stacks/manager.go`:
- Around line 203-248: GetStack currently silently returns a hardcoded fallback
stack when no stack/provider/default exists; add an explicit opt-out in
GetStackOpts (e.g., RequireStack or DisallowFallback bool) and change GetStack
to honor it: when the opt is set and no explicit Stack/Provider and
sl.getDefaultStack returns ErrDefaultStackNotSet, return that error instead of
creating the DefaultBeta fallback; update callers that need strict behavior
(e.g., compose up) to pass the new flag so they can fail fast, and leave
interactive branches (getStackInteractively) unchanged.
♻️ Duplicate comments (1)
src/cmd/cli/command/config.go (1)
78-84: Remove unreachable duplicate error check.
erris already handled afterAccountInfo; the second check is unreachable.🔧 Cleanup
_, err = session.Provider.AccountInfo(ctx) if err != nil { return fmt.Errorf("failed to get account info from provider %q: %w", session.Stack.Provider, err) } - if err != nil { - return err - }
🧹 Nitpick comments (1)
src/pkg/stacks/manager_test.go (1)
650-825: Add coverage for provider/default/fallback stack resolution.With
RequireStackremoved and provider-first/fallback logic moved intoGetStack,TestGetStackno longer exercises:
- ProviderID-based selection (flag vs env),
- Default stack resolution, and
- Fallback stack behavior when no default is set.
Consider adding table cases to lock in these new paths and prevent regressions around the “fallback allowed only for non-deploy” objective.
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 (1)
src/cmd/cli/command/config.go (1)
34-81: Remove early return on stack manager failure to allow provider-based fallback.At line 69-71, returning early on
newStackManagerForLoadererror blocks the provider-based fallback. The session loader'sloadStackmethod explicitly handles nil stack managers (returning a default stack via the provider), but your code prevents reaching that fallback by erroring out. Follow the pattern used insession.goline 35-37: log the error but continue with the nil stack manager.
* Retry setting the policies on service account up to 3 times (#1851) * Retry setting the policies on service account up to 3 times * Address code rabbit comment * continue to the correct outter check policy loop --------- Co-authored-by: Edward J <edw@defang.io> * fix(upgrade): avoid running slow "brew config" unless necessary (#1859) * fix: use shortened links for docs (#1858) * Stacks Cleanup (#1855) * avoid reading from global stacks when session stacks is available * require stack during deployment in interactive mode * Lio/stacks (#1860) * avoid reading from global stacks when session stacks is available * require stack during deployment in interactive mode * fix: panic when DEFANG_PROVIDER is not set would cause GetRegionVarName("") * fix: handle empty stack file * fix: abort stack loading on ctrl-c --------- Co-authored-by: Jordan Stephens <jordan@stephens.io> * chore: fix estimate unit test * Standardize the file and dir mode for context test (#1852) * Standardize the file and dir mode for context test * Update src/pkg/cli/compose/context_test.go * Remove unused import term --------- Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Lio李歐 <lionello@users.noreply.github.com> * allow --provider to override default stack (#1862) * allow fallback stack when !RequireStack * s/RequireStack/DisallowFallbackStack/ * only log fallback when actually using fallback * s/RequireStack/DisallowFallbackStack/ * prefer --provider if available * coderabbbit feedback * refactor: improve error handling and warnings in whoami command --------- Co-authored-by: Lionello Lunesu <lio+git@lunesu.com> Co-authored-by: Hao Jiang <edwardrf@gmail.com> * fix: don't overwrite known state with NOT_SPECIFIED * fix(aws): add CloudFormation metadata --------- Co-authored-by: Hao Jiang <edwardrf@gmail.com> Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Jordan Stephens <jordan@stephens.io>
* fix(aws): make CIRoleArn output ARN * feat(aws): CloudFormation metadata (#1863) * Retry setting the policies on service account up to 3 times (#1851) * Retry setting the policies on service account up to 3 times * Address code rabbit comment * continue to the correct outter check policy loop --------- Co-authored-by: Edward J <edw@defang.io> * fix(upgrade): avoid running slow "brew config" unless necessary (#1859) * fix: use shortened links for docs (#1858) * Stacks Cleanup (#1855) * avoid reading from global stacks when session stacks is available * require stack during deployment in interactive mode * Lio/stacks (#1860) * avoid reading from global stacks when session stacks is available * require stack during deployment in interactive mode * fix: panic when DEFANG_PROVIDER is not set would cause GetRegionVarName("") * fix: handle empty stack file * fix: abort stack loading on ctrl-c --------- Co-authored-by: Jordan Stephens <jordan@stephens.io> * chore: fix estimate unit test * Standardize the file and dir mode for context test (#1852) * Standardize the file and dir mode for context test * Update src/pkg/cli/compose/context_test.go * Remove unused import term --------- Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Lio李歐 <lionello@users.noreply.github.com> * allow --provider to override default stack (#1862) * allow fallback stack when !RequireStack * s/RequireStack/DisallowFallbackStack/ * only log fallback when actually using fallback * s/RequireStack/DisallowFallbackStack/ * prefer --provider if available * coderabbbit feedback * refactor: improve error handling and warnings in whoami command --------- Co-authored-by: Lionello Lunesu <lio+git@lunesu.com> Co-authored-by: Hao Jiang <edwardrf@gmail.com> * fix: don't overwrite known state with NOT_SPECIFIED * fix(aws): add CloudFormation metadata --------- Co-authored-by: Hao Jiang <edwardrf@gmail.com> Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Jordan Stephens <jordan@stephens.io> --------- Co-authored-by: Hao Jiang <edwardrf@gmail.com> Co-authored-by: Edward J <edw@defang.io> Co-authored-by: Jordan Stephens <jordan@stephens.io>
Description
provider selection has been simplified to the following:
--stackis specified, try to load that stack1a. from the local filesystem
1b. from fabric's record of previous deployments
--provideris specified, load the "beta" stack for that providerLinked Issues
Checklist
Summary by CodeRabbit
Refactor
Behavior Changes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.