Support non-interactive deployment to new stack #1754
Conversation
📝 WalkthroughWalkthroughIntroduce a session-driven stack and provider loading flow, convert StackParameters to a Variables-backed struct with getters/setters, make stacks.Manager.Load context-aware with local-first then remote fallback, and update CLI commands/tests to use session.Loader and session.Provider throughout. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI
participant SessionLoader as SessionLoader
participant StacksMgr as stacks.Manager
participant LocalFS as Local FS
participant RemoteAPI as Remote API
participant Provider as Provider
participant Loader as compose.Loader
CLI->>SessionLoader: LoadSession(ctx, options)
SessionLoader->>StacksMgr: Load(ctx, stackName)
StacksMgr->>LocalFS: read local stack
alt local found
LocalFS-->>StacksMgr: StackParameters
StacksMgr-->>SessionLoader: StackParameters
else not found
LocalFS-->>StacksMgr: error
StacksMgr->>RemoteAPI: LoadRemote(ctx, stackName)
alt remote found
RemoteAPI-->>StacksMgr: StackParameters (imported)
StacksMgr-->>SessionLoader: StackParameters
else remote not found
RemoteAPI-->>StacksMgr: error
StacksMgr-->>SessionLoader: error
end
end
SessionLoader->>Loader: init loader(project, compose paths)
SessionLoader->>Provider: NewProvider(ctx, providerID, stackName)
Provider-->>SessionLoader: Provider instance
SessionLoader-->>CLI: Session{Stack, Loader, Provider} (or error)
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" 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/cmd/cli/command/globals_test.go (1)
149-181: Test case expects removed functionality and is currently failing.The test case "env_file_used_when_no_env_vars_or_flags_set" expects values to be loaded from
.defangfiles (lines 153-164), but theloadStackFilefunctionality was removed in line 281 and fromglobals.go. The test creates.defangfiles at lines 246-266, but these files are never read anymore.Pipeline failures confirm all 8 assertions in this test case are failing because the expected values from the
.defangfiles are not being loaded.Either update the test expectations to match the new behavior (defaultConfig) or remove this test case entirely, as
.defangfile loading is no longer supported.🔧 Proposed fix: Update test expectations or remove the test case
Option 1: Update the test case to expect default values since
.defangfiles are no longer loaded:{ - name: "env file used when no env vars or flags set", + name: "defaults used when no env vars or flags set", createRCFile: true, rcStack: stack{ stackname: "test", - entries: map[string]string{ - "DEFANG_MODE": "AFFORDABLE", - "DEFANG_VERBOSE": "true", - "DEFANG_DEBUG": "false", - "DEFANG_STACK": "from-env", - "DEFANG_FABRIC": "from-env-cluster", - "DEFANG_PROVIDER": "defang", - "DEFANG_COLOR": "always", - "DEFANG_TTY": "false", - "DEFANG_NON_INTERACTIVE": "true", - "DEFANG_HIDE_UPDATE": "true", - }, }, - expected: GlobalConfig{ - Verbose: true, - Debug: false, - Stack: stacks.StackParameters{ - Name: "from-env", - Provider: client.ProviderDefang, - Mode: modes.ModeAffordable, - }, - Cluster: "from-env-cluster", - Tenant: "", - ColorMode: ColorAlways, - HasTty: false, // from env - NonInteractive: true, // from env - HideUpdate: true, // from env - }, + expected: defaultConfig, },Option 2: Remove the test case entirely since it validates removed functionality.
src/pkg/stacks/selector_test.go (2)
62-72: Mock implementation has incorrect argument passing.The
Loadmethod signature accepts bothctxandnameparameters (line 62), but only passesnametom.Called()on line 63. This should bem.Called(ctx, name)to properly record all arguments for mock expectations and assertions.🐛 Proposed fix
-func (m *MockStacksManager) Load(ctx context.Context, name string) (*StackParameters, error) { - args := m.Called(name) +func (m *MockStacksManager) Load(ctx context.Context, name string) (*StackParameters, error) { + args := m.Called(ctx, name) if args.Get(0) == nil { return nil, args.Error(1) }
276-276: Mock expectations need to include context parameter.After fixing the mock implementation to pass both
ctxandnametom.Called(), these mock expectations need to be updated to match. They should usemock.Anythingfor the context parameter.🔧 Proposed fix
// Mock loading the created stack - mockSM.On("Load", "staging").Return(newStackParams, nil) + mockSM.On("Load", mock.Anything, "staging").Return(newStackParams, nil)And at line 321:
// Mock loading the created stack - mockSM.On("Load", "firststack").Return(newStackParams, nil) + mockSM.On("Load", mock.Anything, "firststack").Return(newStackParams, nil)Also applies to: 321-321
src/cmd/cli/command/commands_test.go (1)
310-340: Inconsistency between test and production code regarding theoverloadparameter inLoadParameterscalls.The mock's
Load()method (lines 322, 336) callsstacks.LoadParameters(params.ToMap(), true)withoverload=true, but the production implementation inmanager.go:180callsLoadParameters(..., false). This behavioral difference means:
- Production code respects existing environment variables (doesn't overwrite them)
- Mock code overwrites environment variables
Tests could pass even when production would fail due to pre-existing environment variables conflicting with the stack parameters being loaded.
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/commands.go:
- Around line 1279-1309: The PR claims "--provider" is no longer deprecated but
the code still emits deprecation warnings via term.Warn when
RootCmd.PersistentFlags().Changed("provider") and when DEFANG_PROVIDER is set;
remove or modify those term.Warn calls to align with the PR objective: either
delete the two term.Warn(...) lines (the ones triggered for the "provider" flag
and DEFANG_PROVIDER env) so the flag/env are no longer flagged as deprecated, or
replace them with informational messages if you want to encourage using --stack
while not calling it deprecated; adjust only the warnings related to
RootCmd.PersistentFlags().Changed("provider") and the DEFANG_PROVIDER branch
while leaving the provider parsing logic (stack.Provider.Set(...)) intact.
- Around line 1319-1333: The code treats a successful fallback LoadParameters as
a failure because after sm.LoadParameters(...) succeeds the function still
returns the original sm.Load error; to fix, when LoadParameters succeeds you
must retry loading the stack (e.g. call stackParams, err = sm.Load(ctx,
stackName) or otherwise populate the stack object) and only return an error if
that retry fails, ensuring the code proceeds normally when LoadParameters
succeeds; update the logic around sm.Load, sm.LoadParameters and the subsequent
return so a successful fallback does not immediately return the original error.
🧹 Nitpick comments (3)
src/cmd/cli/command/globals_test.go (2)
183-189: Test case may be redundant after removing .defang file support.The test case "env file with no values used when no env vars or flags set" creates an empty
.defangfile but expectsdefaultConfigvalues. Since.defangfiles are no longer loaded, this test is functionally identical to the test at lines 199-202 ("no env file, no env vars and no flags").Consider removing this test case as redundant, or update its name and description to clarify it's testing default behavior.
191-197: Test case name is misleading after removing .defang file support.The test name "default .defang name and no values, when no env vars or flags" references
.defangfiles, but since.defangfile loading was removed, this test now only validates default configuration behavior. The test is functionally identical to the test at lines 199-202.Consider updating the test name to reflect that it's testing default behavior, or remove it as redundant.
src/pkg/stacks/manager.go (1)
171-185: Consider the appropriateness of the informational message.The informational message at line 177 states "unable to load stack from file, attempting to import from previous deployments" and logs the error. This message will appear in normal operation when:
- Working outside the target directory (sm.outside is true)
- A stack exists remotely but not locally
This could be confusing or noisy for users who are intentionally working with remote-only stacks. Consider either:
- Changing it to a debug-level message instead of info
- Making it more specific about what's happening (e.g., "Stack not found locally, loading from remote deployments")
- Only logging in non-outside cases where a local file was expected but failed to read
💡 Potential refinement
Option 1: Use debug level for less noise:
- term.Infof("unable to load stack from file, attempting to import from previous deployments: %v", err) + term.Debugf("Stack not found locally (%v), loading from remote deployments", err)Option 2: Make the message clearer:
- term.Infof("unable to load stack from file, attempting to import from previous deployments: %v", err) + term.Infof("Stack %q not found locally, loading from remote deployments", name)Option 3: Only log when not outside (when a local file was expected):
params, err := ReadInDirectory(sm.targetDirectory, name) if err != nil { - term.Infof("unable to load stack from file, attempting to import from previous deployments: %v", err) + // Fallback to remote loading return sm.LoadRemote(ctx, name) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.gosrc/cmd/cli/command/commands.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/commands_test.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/selector_test.gosrc/cmd/cli/command/commands.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/globals_test.gosrc/cmd/cli/command/commands_test.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.go
🧬 Code graph analysis (4)
src/pkg/stacks/manager_test.go (1)
src/pkg/stacks/stacks.go (1)
Load(256-258)
src/pkg/stacks/manager.go (2)
src/pkg/stacks/stacks.go (6)
List(172-174)StackListItem(145-153)Load(256-258)StackParameters(18-25)ReadInDirectory(242-254)LoadParameters(286-304)src/pkg/term/colorizer.go (2)
Infof(306-308)Errorf(322-324)
src/pkg/stacks/selector_test.go (1)
src/pkg/stacks/stacks.go (2)
Load(256-258)StackParameters(18-25)
src/cmd/cli/command/commands.go (7)
src/pkg/term/colorizer.go (2)
Warn(310-312)Errorf(322-324)src/protos/io/defang/v1/fabric.pb.go (4)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)src/pkg/utils.go (1)
Getenv(48-53)src/pkg/stacks/stacks.go (3)
Parse(212-219)Load(256-258)LoadParameters(286-304)src/pkg/modes/modes.go (2)
Parse(47-63)Mode(12-12)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
🪛 GitHub Actions: Go package
src/cmd/cli/command/globals_test.go
[error] 291-291: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected Verbose to be true, got false
[error] 297-297: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected Stack.Name to be 'from-env', got ''
[error] 300-300: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected Stack.Provider to be 'defang', got 'auto'
[error] 306-306: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected Cluster to be 'from-env-cluster', got ''
[error] 312-312: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected ColorMode to be 'always', got 'auto'
[error] 315-315: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected HasTty to be false, got true
[error] 318-318: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected NonInteractive to be true, got false
[error] 321-321: Test_configurationPrecedence/env_file_used_when_no_env_vars_or_flags_set failed: expected HideUpdate to be true, got false
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
src/pkg/stacks/manager_test.go (2)
114-114: LGTM! Context parameter correctly added to all Load calls.All test invocations of
manager.Load()have been properly updated to passt.Context()as the first parameter, aligning with the new context-aware signature introduced insrc/pkg/stacks/manager.go.Also applies to: 168-168, 240-240, 264-264, 630-630, 706-706
710-712: Error message expectation correctly updated.The expected error message has been updated to match the new error format returned by
LoadRemotewhen a stack is not found, which usesfmt.Errorf("unable to find stack %q", name)insrc/pkg/stacks/manager.goline 200.src/cmd/cli/command/globals.go (1)
15-223: LGTM! Stack-file loading correctly removed in favor of explicit configuration.The removal of
loadStackFileandcheckEnvConflictsaligns with the PR's goal of supporting non-interactive deployments by relying solely on explicit flags and environment variables. The configuration precedence is now simplified to: command-line flags > environment variables > defaults.The updated documentation clearly describes the new configuration flow and how to add new options.
src/pkg/stacks/manager.go (3)
15-15: New term import added for informational logging.The term package import is used for logging informational messages when falling back to remote stack loading (line 177).
21-21: Load signature updated to accept context parameter.The interface signature correctly updated to
Load(ctx context.Context, name string), enabling context propagation for remote API calls. All implementations and call sites across the codebase have been updated accordingly.
187-208: LGTM! LoadRemote implementation is correct.The new
LoadRemotemethod properly:
- Lists remote stacks via the fabric client
- Searches for the specified stack by name
- Returns a clear error if the stack is not found remotely
- Loads the stack parameters into the environment
- Returns the stack parameters on success
The implementation correctly handles the remote fallback scenario for the new non-interactive deployment use case.
src/cmd/cli/command/commands_test.go (2)
7-7: LGTM!The
fmtimport is appropriately added to support the new error formatting in the mock's Load method.
293-293: LGTM!The signature update to accept context aligns with the broader refactoring to make stack loading context-aware.
src/cmd/cli/command/commands.go (1)
1362-1364: LGTM!The updated
Loadcall with context and simplified error handling is appropriate for the "only stack" case. Removing the import fallback logic makes the code clearer.
jordanstephens
force-pushed
the
jordan/allow-creating-new-stack-during-non-interactive-deployment
branch
from
70996b3 to
ba5a206
Compare
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/commands.go (1)
1317-1341: Critical: Fallback logic doesn't populate the stack after LoadParameters succeeds.This is the bug causing the pipeline failure. When
sm.Load(ctx, stackName)fails and the non-interactive fallback path succeeds viaLoadParameters, the code still falls through to line 1335 which assigns the originalnilstackParamstostack.The fallback at lines 1325-1330 loads parameters into environment variables but never constructs or returns a valid
StackParametersobject. AfterLoadParameterssucceeds, you need to either:
- Build and return a
StackParameterswith the values used, or- Retry
sm.Load(ctx, stackName)after parameters are loaded🐛 Proposed fix
stackParams, err := sm.Load(ctx, stackName) if err != nil { if !global.NonInteractive || stack.Provider == client.ProviderAuto { return nil, "", fmt.Errorf("unable to load stack %q: %w", stackName, err) } // if we are in non-interactive mode, try to create a new stack with the given name, provider, region, and mode region := client.GetRegion(stack.Provider) - err = sm.LoadParameters(map[string]string{ + loadErr := sm.LoadParameters(map[string]string{ "name": stackName, "provider": stack.Provider.String(), "region": region, "mode": stack.Mode.String(), }, false) - if err != nil { - return nil, "", fmt.Errorf("unable to load stack %q and unable to create it: %w", stackName, err) + if loadErr != nil { + return nil, "", fmt.Errorf("unable to load stack %q and unable to create it: %w", stackName, loadErr) } + // LoadParameters succeeded; return stack with the parameters + stack.Name = stackName + stack.Region = region + // stack.Provider and stack.Mode already set earlier + return stack, whence, nil } - stack = stackParams + stack = stackParams
🧹 Nitpick comments (1)
src/cmd/cli/command/globals.go (1)
147-147: Documentation mentions "stack file values" but stack-file loading was removed.Line 147 states priority as "command-line flags > environment variables > stack file values > defaults", but stack-file loading at startup was removed per the PR objectives. Consider updating this comment to reflect the current behavior.
📝 Proposed documentation fix
-This ensures the priority order: command-line flags > environment variables > stack file values > defaults +This ensures the priority order: command-line flags > environment variables > defaults
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cmd/cli/command/globals_test.go
- src/pkg/stacks/manager_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/pkg/stacks/selector_test.gosrc/cmd/cli/command/commands_test.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/stacks/selector_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/pkg/stacks/manager.go
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.gosrc/cmd/cli/command/commands.go
🧬 Code graph analysis (4)
src/pkg/stacks/selector_test.go (1)
src/pkg/stacks/stacks.go (2)
Load(256-258)StackParameters(18-25)
src/cmd/cli/command/commands.go (5)
src/pkg/cli/client/provider.go (1)
Provider(54-80)src/pkg/stacks/stacks.go (3)
Parse(212-219)Load(256-258)LoadParameters(286-304)src/pkg/modes/modes.go (2)
Parse(47-63)Mode(12-12)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
src/cmd/cli/command/commands_test.go (3)
src/pkg/stacks/stacks.go (3)
Load(256-258)StackParameters(18-25)LoadParameters(286-304)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/modes/modes.go (1)
Mode(12-12)
src/pkg/stacks/manager.go (2)
src/pkg/stacks/stacks.go (6)
List(172-174)StackListItem(145-153)Load(256-258)StackParameters(18-25)ReadInDirectory(242-254)LoadParameters(286-304)src/pkg/term/colorizer.go (2)
Infof(306-308)Errorf(322-324)
🪛 GitHub Actions: Go package
src/cmd/cli/command/globals.go
[error] 1-1: Go test failed for package 'src/cmd/cli/command'. Exit code 1 reported in CI: 'FAIL github.com/DefangLabs/defang/src/cmd/cli/command 0.104s'.
src/cmd/cli/command/commands.go
[error] 1-1: Go test failed for package 'src/cmd/cli/command'. Exit code 1 reported in CI: 'FAIL github.com/DefangLabs/defang/src/cmd/cli/command 0.104s'.
src/cmd/cli/command/commands_test.go
[error] 1-1: Go test failed for package 'src/cmd/cli/command'. Exit code 1 reported in CI: 'FAIL github.com/DefangLabs/defang/src/cmd/cli/command 0.104s'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
src/pkg/stacks/manager.go (2)
171-185: Context-aware Load with remote fallback looks good.The method correctly passes context and falls back to remote loading when local file read fails. The info message helps users understand what's happening during fallback.
187-208: The code is correct. The project requires Go 1.24.1 (per go.mod), which is well above Go 1.22 where the loop variable capture behavior was fixed. In Go 1.22+, each loop iteration gets its own copy of loop variables, soremoteStack = &remoteis safe regardless of thebreakstatement.Likely an incorrect or invalid review comment.
src/cmd/cli/command/commands_test.go (1)
293-341: Mock Load implementation correctly updated for context-aware API.The multi-path resolution logic (loadResults → loadResult → listResult → expectedProvider/Region) provides flexible test configuration. The context parameter is accepted but intentionally unused in the mock, which is appropriate for test code.
One consideration:
stacks.LoadParameters(params.ToMap(), true)on lines 322 and 336 usesoverload=true, meaning it will overwrite existing environment variables. This may be intentional for test isolation but differs from production code which usesoverload=false. Ensure this test behavior matches expectations.src/cmd/cli/command/commands.go (2)
1279-1307: Provider/mode flag and environment variable handling looks correct.The precedence logic properly:
- Checks if flag was explicitly changed via
RootCmd.PersistentFlags().Changed()- Falls back to environment variable lookup
- Parses and validates the values with appropriate error handling
This aligns with the PR objective of supporting non-interactive deployment with flags/env vars.
1362-1365: Verify: Single-stack load failure now returns immediately without fallback.When there's exactly one known stack but loading fails, the code now returns an error immediately. This is a behavior change from potentially attempting import logic. Confirm this is intentional.
src/cmd/cli/command/globals.go (1)
149-222: syncFlagsWithEnv implementation is correct.The method properly synchronizes environment variables with flags, respecting the precedence where explicitly set flags take priority. The handling of various DEFANG_* environment variables is consistent and includes appropriate error handling.
src/pkg/stacks/selector_test.go (2)
62-72: Mock Load signature updated correctly, but context is intentionally not matched.The mock signature now accepts
context.Contextbutm.Called(name)only passesnameto the mock framework. This is an intentional design choice - the mock doesn't validate the context argument. This is consistent with how the mock expectations are set up (e.g.,mockSM.On("Load", "staging")).Based on learnings, this panic-on-unexpected-call behavior is acceptable in test code for fail-fast debugging.
215-215: testableStackSelector.SelectStack correctly passes context to Load.The call
tss.sm.Load(ctx, selectedStackName)properly propagates the context to the manager, aligning with the updated interface signature.
jordanstephens
force-pushed
the
jordan/allow-creating-new-stack-during-non-interactive-deployment
branch
from
ba5a206 to
90eb984
Compare
src/cmd/cli/command/commands.go
Outdated
| } | ||
| return loaded, nil | ||
| } | ||
| if global.HasTty { |
There was a problem hiding this comment.
I forgot the history here, but we almost never check HasTty and almost always use NonInteractive.
There was a problem hiding this comment.
HasTty = !NonInteractive , so I thought I would avoid the double negative here.
defang/src/cmd/cli/command/globals.go
Lines 133 to 135 in 90eb984
There was a problem hiding this comment.
ah I think this must have broken somewhere, they used to be different. hasTTY is a "fact", it doesn't change with env or flags. NonInteractive you can change with flag --non-interactive or env CI=1
There was a problem hiding this comment.
Regardless, we should make them the same. Happy to switch NonInteractive to Interactive
There was a problem hiding this comment.
Sorry, I'm confused about what you want changed. At first, you suggested that hasTTY in this PR incorrect, then you said "Regardless, we should make them the same. Happy to switch NonInteractive to Interactive". What should we do about hasTTY?
src/cmd/cli/command/commands_test.go
Outdated
| } | ||
|
|
||
| if m.listResult != nil { | ||
| for _, item := range m.listResult { |
There was a problem hiding this comment.
This is not about safety, it's about the behaviour of the mock
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/pkg/stacks/selector_test.go (1)
62-72: Mock Load method doesn't pass context to m.Called.The
Loadmethod signature was updated to acceptcontext.Context, but the implementation still callsm.Called(name)instead ofm.Called(ctx, name). This will cause mock expectation mismatches with the test setups at lines 276 and 321 that usemockSM.On("Load", "staging").🔧 Proposed fix
func (m *MockStacksManager) Load(ctx context.Context, name string) (*StackParameters, error) { - args := m.Called(name) + args := m.Called(ctx, name) if args.Get(0) == nil { return nil, args.Error(1) }Then update the test expectations:
- mockSM.On("Load", "staging").Return(newStackParams, nil) + mockSM.On("Load", ctx, "staging").Return(newStackParams, nil)
🧹 Nitpick comments (3)
src/cmd/cli/command/globals_test.go (1)
13-135: Test coverage significantly reduced; precedence testing requires completion.The test has been simplified to cover only basic defaults and invalid environment variable handling. The PR description acknowledges: "existing logic that prevents overwriting flags or environment variables with stackfile variables requires migration; a related test currently fails and needs further attention."
Consider tracking the completion of comprehensive precedence testing (flags > env vars > stack files) to ensure configuration handling remains correct across all scenarios.
src/cmd/cli/command/commands.go (1)
1264-1294: Function naming could be clearer.
loadOrCreateStackdoesn't actually persist a new stack file—it only loads parameters into environment variables viaLoadParameters. This is appropriate for CI environments where stack files may not be writable, but the name could be misleading.Consider renaming to
loadOrInitializeStackor adding a comment clarifying the behavior:// loadOrCreateStack attempts to load an existing stack by name. // If the stack doesn't exist and we're in non-interactive mode with a valid provider, // it initializes stack parameters without persisting a stack file.src/pkg/stacks/manager.go (1)
177-178: Consider using Debug level for internal fallback logging.The
term.Infofmessage about falling back to remote import is helpful for troubleshooting, but it may confuse users who expect local stack files to exist. Consider usingterm.Debugfinstead, or making this conditional on verbose mode.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/workspace_test.gosrc/pkg/cli/client/region.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.gosrc/pkg/stacks/stacks.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/stacks/manager.gosrc/cmd/cli/command/globals_test.gosrc/pkg/stacks/stacks.gosrc/cmd/cli/command/workspace_test.gosrc/pkg/cli/client/region.gosrc/cmd/cli/command/globals.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/globals_test.gosrc/cmd/cli/command/workspace_test.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/commands_test.go
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.go
🧬 Code graph analysis (10)
src/pkg/stacks/manager.go (2)
src/pkg/stacks/stacks.go (6)
List(164-166)StackListItem(137-145)Load(248-250)StackParameters(18-25)LoadParameters(278-297)ReadInDirectory(234-246)src/pkg/term/colorizer.go (2)
Infof(306-308)Errorf(322-324)
src/cmd/cli/command/globals_test.go (2)
src/cmd/cli/command/globals.go (1)
GlobalConfig(64-76)src/pkg/term/colorizer.go (1)
Fatalf(332-334)
src/pkg/stacks/stacks.go (1)
src/pkg/cli/client/region.go (1)
GetRegionVarName(26-41)
src/pkg/cli/client/region.go (2)
src/pkg/cli/client/provider_id.go (6)
ProviderAWS(15-15)ProviderGCP(17-17)ProviderDO(16-16)ProviderDefang(14-14)ProviderAuto(13-13)ProviderID(10-10)src/pkg/utils.go (1)
Getenv(48-53)
src/cmd/cli/command/globals.go (8)
src/cmd/cli/command/color.go (2)
ColorAuto(11-11)ColorMode(5-5)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
ModeUnspecified(15-15)Mode(12-12)src/pkg/utils.go (2)
GetenvBool(56-59)Getenv(48-53)src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)src/pkg/cli/client/cluster.go (1)
DefangFabric(16-16)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/region.go (1)
GetRegion(5-24)
src/pkg/stacks/manager_test.go (1)
src/pkg/stacks/stacks.go (1)
Load(248-250)
src/pkg/stacks/selector_test.go (1)
src/pkg/stacks/stacks.go (3)
Load(248-250)StackParameters(18-25)LoadParameters(278-297)
src/cmd/cli/command/stack_test.go (4)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)src/pkg/modes/modes.go (2)
Mode(12-12)ModeAffordable(16-16)src/pkg/utils.go (1)
Getenv(48-53)
src/cmd/cli/command/commands.go (4)
src/pkg/stacks/manager.go (1)
Manager(19-24)src/pkg/stacks/stacks.go (4)
StackParameters(18-25)Load(248-250)LoadParameters(278-297)DefaultBeta(77-77)src/pkg/cli/client/provider_id.go (2)
ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/cli/client/region.go (1)
GetRegion(5-24)
src/cmd/cli/command/commands_test.go (2)
src/pkg/stacks/stacks.go (3)
Load(248-250)StackParameters(18-25)LoadParameters(278-297)src/pkg/cli/client/provider.go (1)
Provider(54-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (18)
src/cmd/cli/command/workspace_test.go (1)
61-61: LGTM! In-memory workspace assignment aligns with new global config pattern.The direct assignment to
global.Tenantcorrectly replaces environment-based initialization, consistent with the PR's refactoring toNewGlobalConfig()and lazy stack loading.Also applies to: 94-94
src/pkg/stacks/manager_test.go (2)
114-114: LGTM! Context parameter correctly added to all Load calls.All invocations of
manager.Loadhave been properly updated to passt.Context(), aligning with the new signatureLoad(ctx context.Context, name string).Also applies to: 168-168, 240-240, 264-264, 630-630, 706-706
710-711: Error message change reflects remote stack fallback behavior.The error expectation changed from
"Load not allowed: target directory"to"unable to find stack \"teststack\"", indicatingLoadnow attempts remote lookup when local stacks are unavailable. This aligns with the PR's goal of supporting remote-only stack references in non-interactive environments.src/pkg/stacks/stacks.go (2)
31-32: LGTM! Region variable name delegation improves maintainability.Using
client.GetRegionVarName(params.Provider)centralizes provider-specific region environment variable mapping, reducing duplication and ensuring consistency with region.go.
278-287: LGTM! Signature change to strongly-typed StackParameters.The function now accepts
StackParametersinstead ofmap[string]string, improving type safety. The implementation correctly converts parameters viaparams.ToMap()before loading into the environment.src/pkg/cli/client/region.go (2)
6-24: LGTM! Clean refactoring with centralized region variable mapping.Extracting
GetRegionVarNamereduces duplication and centralizes provider-to-environment-variable mapping. The singlepkg.Getenv(varName, defaultRegion)call is cleaner than the previous per-provider lookups. Default regions (AWS: us-west-2, GCP: us-central1, DO: nyc3) are sensible.
26-41: New helper function correctly maps providers to region environment variables.
GetRegionVarNameprovides a reusable mapping (AWS→AWS_REGION, GCP→GCP_LOCATION, DO→REGION) that's consumed bystacks.ToMap()for consistent region handling across the codebase.Note: Both this function and
GetRegionpanic on unsupported providers. While aggressive, this is acceptable for internal helpers where all provider values are controlled by the codebase's exhaustive ProviderID enumeration.src/cmd/cli/command/stack_test.go (2)
170-175: LGTM! Migration to strongly-typed StackParameters.The test correctly migrates from the previous map-based approach to the new
StackParametersstruct, aligning with the API changes across the codebase.
199-199: Verify the case change for DEFANG_MODE is intentional.The expected value changed from "AFFORDABLE" to "affordable". This aligns with how
Mode.String()outputs values, but ensure downstream consumers (scripts, CI configs) expect lowercase mode values.src/cmd/cli/command/globals.go (2)
111-119: Verify TTY flag precedence is intentional.The current logic allows
DEFANG_TTY=trueto overrideDEFANG_NON_INTERACTIVE=true, resulting inNonInteractive=false. If both are set, the later check wins. Consider whether this is the intended behavior or ifDEFANG_NON_INTERACTIVEshould take precedence.// Current behavior: // DEFANG_NON_INTERACTIVE=true → hastty=false // DEFANG_TTY=true → hastty=true (overrides above) // NonInteractive = !hastty → false
86-144: LGTM! Clean initialization pattern.The
NewGlobalConfig()constructor consolidates environment variable parsing into a single location, improving maintainability. The structured approach with validation and debug logging for invalid values is a good pattern.src/pkg/stacks/selector_test.go (1)
74-77: LGTM! LoadParameters signature updated correctly.The mock correctly accepts
StackParametersstruct instead ofmap[string]string, aligning with the new API.src/cmd/cli/command/commands.go (1)
1296-1367: LGTM! Comprehensive stack selection logic.The updated
getStackfunction properly handles:
- Explicit stack names with load-or-create fallback
- Non-interactive defaults to beta/defang provider
- Single stack auto-selection
- Interactive stack selection/creation flow
The error messages are clear and actionable.
src/pkg/stacks/manager.go (2)
171-185: LGTM! Graceful fallback from local to remote loading.The Load method correctly attempts local file loading first, then falls back to remote discovery. The deferred error wrapping provides clear context.
187-208: LGTM! Clean remote stack loading implementation.The
LoadRemotemethod properly:
- Lists remote stacks
- Finds the matching stack by name
- Loads parameters into environment
- Returns appropriate errors when stack not found
src/cmd/cli/command/commands_test.go (3)
293-341: LGTM! Mock implementation updated correctly.The mock properly:
- Accepts context parameter (signature alignment)
- Looks up stacks in
listResultby name- Calls
stacks.LoadParametersto simulate real behavior- Returns appropriate error with new format
The side effect of calling
LoadParametersis appropriate since the real implementation does the same.
353-355: LGTM! LoadParameters signature updated.The mock correctly delegates to the real
stacks.LoadParametersfunction with the newStackParametersstruct parameter.
559-590: Good test coverage for new stack creation in non-interactive mode.The test case "stack flag provided with unknown stack but valid provider" validates the new CI deployment scenario where a stack doesn't exist locally but can be created with explicit provider/region parameters. The expected region "us-central1" correctly matches the GCP default.
| }) | ||
| } | ||
|
|
||
| func Test_configurationPrecedence(t *testing.T) { |
There was a problem hiding this comment.
My comment, which doesn’t need to be addressed here, is that flags still take precedence over environment variables. Since we are no longer testing this here. It would be good to have a test to ensure this behavior remains enforced. I understand that the test may no longer live in this file, as long as it’s covered somewhere.
There was a problem hiding this comment.
Flags are no longer the responsibility of this Globals struct. If we want to test that we have configured flags with the default value equal to the global value, we will need to add a new test for each command where it is relevant.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cmd/cli/command/commands_test.go (1)
310-324: Ignored error fromstacks.LoadParameters.The return value of
stacks.LoadParameters(params, true)is discarded on lines 321 and 334. WhileLoadParameterscan return an error ifos.Setenvfails, ignoring it in tests may mask unexpected failures.Based on learnings, in Go test mocks it's acceptable to panic to surface issues quickly. Consider either propagating the error or panicking on failure to catch issues during test runs.
♻️ Suggested fix
- stacks.LoadParameters(params, true) + if err := stacks.LoadParameters(params, true); err != nil { + panic(fmt.Sprintf("LoadParameters failed: %v", err)) + }Apply similar change at line 334.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cmd/cli/command/commands_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/commands_test.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/commands_test.go
🧬 Code graph analysis (1)
src/cmd/cli/command/commands_test.go (2)
src/pkg/stacks/stacks.go (3)
Load(248-250)StackParameters(18-25)LoadParameters(278-297)src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderGCP(17-17)ProviderAuto(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (7)
src/cmd/cli/command/commands_test.go (7)
7-7: LGTM!The
fmtimport is correctly added and used for error formatting in the mock.
264-275: LGTM!The mock struct fields are well-organized to support flexible test scenarios with per-name results via
loadResultsmap and default fallback vialoadResult.
351-353: LGTM!Clean delegation to
stacks.LoadParameterswith proper signature alignment.
362-366: LGTM!Proper initialization and cleanup of
global.Stackensures test isolation.
518-524: LGTM!Clean test setup with proper state restoration in cleanup.
750-755: LGTM!Clean test case initialization pattern. The
provider.Setcall gracefully handles empty strings by keeping the defaultProviderAutovalue.
571-588: This test is correct and requires no changes.The test expects
Region: "us-central1"for the newly created GCP stack, and this is correctly populated byclient.GetRegion(params.Provider)in theloadOrCreateStack()function (line 1282 in commands.go). Whensm.Load()fails, the code callsGetRegion(ProviderGCP), which returns the hardcoded default "us-central1", before callingsm.LoadParameters()to persist the configuration. The mock'sCreate()method is never invoked in this test flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/globals.go:
- Around line 111-114: The build fails because defangtty is undefined; update
the logic around hastty so it no longer references the removed symbol: remove
the conditional that checks defangtty and simply set hastty :=
term.IsTerminal(), or if you intended an environment-override, replace defangtty
with the proper env lookup (e.g., os.Getenv or a config flag) and use that value
in the conditional; locate the hastty assignment near term.IsTerminal() in
globals.go to apply the change.
🧹 Nitpick comments (1)
src/cmd/cli/command/globals.go (1)
25-59: Stale documentation references removed functions.The documentation references
syncFlagsWithEnv()(lines 25, 42-57) andsyncNonFlagEnvVars()(line 59), but according to the PR changes, these functions have been removed. The documentation should be updated to reflect the new environment-driven initialization pattern inNewGlobalConfig().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cmd/cli/command/globals.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/globals.go
🧬 Code graph analysis (1)
src/cmd/cli/command/globals.go (7)
src/cmd/cli/command/color.go (2)
ColorAuto(11-11)ColorMode(5-5)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
ModeUnspecified(15-15)Mode(12-12)src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)src/pkg/cli/client/cluster.go (1)
DefangFabric(16-16)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/region.go (1)
GetRegion(5-24)
🪛 GitHub Actions: Go package
src/cmd/cli/command/globals.go
[error] 112-112: go test failed: undefined: defangtty (cmd/cli/command/globals.go:112) during 'go test -test.short -v ./...'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/cmd/cli/command/globals.go (2)
84-84: LGTM!Using a constructor function for global initialization consolidates the environment variable reading logic cleanly.
124-139: LGTM!The consolidated initialization logic properly handles environment variables with appropriate fallbacks. The region initialization correctly defers to
GetRegion(provider)which returns an empty string forProviderAuto, allowing downstream resolution.
This reverts commit 836a4d48e0c71fc9c2786a691e388cf5a01c9606.
stacks are now loaded lazily by each command's handler func where we can safely consider the loader target directory
in non-interactive environments. This is to support deploying to a new stack for the first time in CI
Co-authored-by: Lio李歐 <lionello@users.noreply.github.com>
jordanstephens
force-pushed
the
jordan/allow-creating-new-stack-during-non-interactive-deployment
branch
from
12bf82d to
92ccc19
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)
src/pkg/stacks/selector_test.go (1)
62-72: MockLoadmethod ignoresctxparameter inCalled().The signature declares
ctx context.Contextbutm.Called(name)at line 63 doesn't passctx. This creates a mismatch with the interface and test expectations. Either includectxinCalled()and update allOn("Load", ...)expectations to includemock.Anythingfor context, or document that context is intentionally ignored in mocks.🔧 Suggested fix
func (m *MockStacksManager) Load(ctx context.Context, name string) (*StackParameters, error) { - args := m.Called(name) + args := m.Called(ctx, name) if args.Get(0) == nil { return nil, args.Error(1) }Then update test expectations:
- mockSM.On("Load", "staging").Return(newStackParams, nil) + mockSM.On("Load", mock.Anything, "staging").Return(newStackParams, nil)
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/globals.go:
- Around line 111-114: The code references an undefined boolean defangtty when
setting hastty (hastty := term.IsTerminal()), which breaks the build; either
remove the entire defangtty conditional block or introduce a defined boolean
(e.g., a function parameter, package-level var or a flag/env-driven variable)
named defangtty and initialize it before use so the conditional compiles; update
the logic around term.IsTerminal(), hastty and any callers to ensure the new
defangtty source (flag/env/param) is correctly wired and documented.
🧹 Nitpick comments (4)
src/cmd/cli/command/globals_test.go (1)
78-92: Dead code:rcEnvsis declared but never populated.The
rcEnvsslice and its cleanup loop at lines 84-92 do nothing since the slice is never populated (line 82 just has a comment). Either remove this scaffolding or implement stack file creation logic.Based on PR description noting "a related test currently fails and requires further attention," this may be intentional WIP.
🔧 Suggested cleanup if not WIP
- var rcEnvs []string - // Create stack files in the temporary directory - - t.Cleanup(func() { - // Unseting env vars set for this test is handled by t.Setenv automatically - // t.tempDir() will clean up created files - - // Unset all env vars created by loadDotDefang since it uses os.Setenv - for _, rcEnv := range rcEnvs { - os.Unsetenv(rcEnv) - } - })src/pkg/stacks/manager.go (2)
158-169: Consider whether logging an info message on local load failure is appropriate.When a user explicitly provides a
--stackflag referencing a remote-only stack, theterm.Infofmessage on line 161 might appear as noise. This is informational logging for what could be an expected/normal code path (remote stacks without local files).If the intent is to support remote stacks as a first-class use case, consider lowering this to
term.Debugor conditionally logging only when a local file was expected.
198-203: Edge case: ErrOutside message may be confusing when targetDirectory is empty.When
sm.targetDirectory == "", the error message inErrOutside.Error()will show an empty string for the target directory, which may be confusing to users. Consider providing a more descriptive message like "no target directory specified" in this case.🔧 Suggested improvement
func (sm *manager) Create(params StackParameters) (string, error) { if sm.targetDirectory == "" { - return "", &ErrOutside{Operation: "Create", TargetDirectory: sm.targetDirectory} + return "", &ErrOutside{Operation: "Create", TargetDirectory: "(none)"} } return CreateInDirectory(sm.targetDirectory, params) }src/cmd/cli/command/commands_test.go (1)
293-339: Return value fromstacks.LoadParametersis ignored on lines 321 and 334.The
stacks.LoadParametersfunction returns an error, but it's being ignored in the mock'sLoadmethod. While this is test code and panicking on errors is acceptable per learnings, silently ignoring errors could mask test setup issues.🔧 Suggested fix
params := stacks.StackParameters{ Name: name, Provider: provider, Mode: mode, } - stacks.LoadParameters(params, true) + if err := stacks.LoadParameters(params, true); err != nil { + panic(fmt.Sprintf("mock LoadParameters failed: %v", err)) + } return ¶ms, nilApply similar change at line 334.
Based on learnings, mocks can panic to surface issues quickly during tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/workspace_test.gosrc/pkg/cli/client/region.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.gosrc/pkg/stacks/stacks.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cmd/cli/command/stack_test.go
- src/pkg/stacks/manager_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/workspace_test.gosrc/pkg/stacks/selector_test.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/commands_test.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/workspace_test.gosrc/cmd/cli/command/globals.gosrc/pkg/stacks/stacks.gosrc/pkg/cli/client/region.gosrc/pkg/stacks/selector_test.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/commands_test.gosrc/pkg/stacks/manager.go
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.go
🧬 Code graph analysis (7)
src/pkg/stacks/stacks.go (1)
src/pkg/cli/client/region.go (1)
GetRegionVarName(26-41)
src/pkg/cli/client/region.go (2)
src/pkg/cli/client/provider_id.go (6)
ProviderAWS(15-15)ProviderGCP(17-17)ProviderDO(16-16)ProviderDefang(14-14)ProviderAuto(13-13)ProviderID(10-10)src/pkg/utils.go (1)
Getenv(48-53)
src/pkg/stacks/selector_test.go (1)
src/pkg/stacks/stacks.go (3)
Load(260-262)StackParameters(18-27)LoadParameters(290-309)
src/cmd/cli/command/commands.go (7)
src/pkg/stacks/manager.go (1)
Manager(18-23)src/pkg/stacks/stacks.go (4)
StackParameters(18-27)Load(260-262)LoadParameters(290-309)DefaultBeta(89-89)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)src/protos/io/defang/v1/fabric.pb.go (4)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)src/pkg/cli/client/provider_id.go (2)
ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/cli/client/region.go (1)
GetRegion(5-24)src/pkg/stacks/selector.go (1)
NewSelector(20-25)
src/cmd/cli/command/globals_test.go (2)
src/cmd/cli/command/globals.go (1)
GlobalConfig(64-76)src/pkg/term/colorizer.go (1)
Fatalf(332-334)
src/cmd/cli/command/commands_test.go (2)
src/pkg/stacks/stacks.go (3)
Load(260-262)StackParameters(18-27)LoadParameters(290-309)src/pkg/cli/client/provider.go (1)
Provider(54-80)
src/pkg/stacks/manager.go (1)
src/pkg/stacks/stacks.go (7)
List(176-178)StackListItem(149-157)Load(260-262)StackParameters(18-27)LoadParameters(290-309)ReadInDirectory(246-258)Create(98-100)
🪛 GitHub Actions: Go package
src/cmd/cli/command/globals.go
[error] 112-112: undefined: defangtty (during 'go test -test.short -v ./...')
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
src/cmd/cli/command/globals.go (1)
86-139: LGTM on the overall structure ofNewGlobalConfig.The centralized initialization approach is clean. Environment variable parsing with graceful fallback to defaults and debug logging for invalid values is appropriate.
src/cmd/cli/command/workspace_test.go (1)
56-61: LGTM on test setup changes.Direct assignment to
global.Tenantis appropriate here since the test already saves and restores the global state. This aligns with the refactored configuration approach.src/pkg/stacks/stacks.go (3)
25-27: LGTM on new Prefix/Suffix fields.The new fields are properly integrated into
ToMap()andParamsFromMap()for serialization/deserialization.
29-53: LGTM onToMap()refactor.Using
client.GetRegionVarName(params.Provider)centralizes the provider-to-region-var-name mapping, reducing duplication withregion.go.
290-309: LGTM onLoadParameterssignature change.The typed
StackParametersparameter improves API clarity. The internal conversion to map maintains backward-compatible behavior.src/pkg/cli/client/region.go (2)
5-24: LGTM onGetRegionrefactor.Clean separation of the region variable name lookup into
GetRegionVarName. Early returns forProviderDefang/ProviderAutocorrectly skip environment lookup when no region applies.
26-41: LGTM onGetRegionVarNamehelper.This new helper is correctly consumed by
stacks.ToMap()to determine the appropriate region environment variable name per provider.src/pkg/stacks/selector_test.go (1)
74-77: LGTM onLoadParametersmock signature update.The mock correctly uses the new
StackParameterstyped parameter.src/cmd/cli/command/globals_test.go (1)
31-48: Test coverage significantly reduced.The test matrix now only contains 2 cases (baseline and empty debug bool). The PR description acknowledges this: "logic that prevented overwriting flags/env vars with stackfile variables needs migration; a related test currently fails."
Consider adding a TODO comment or tracking issue to restore comprehensive precedence testing.
src/cmd/cli/command/commands.go (3)
1254-1284: NewloadOrCreateStackfunction enables non-interactive stack creation.The logic correctly:
- Returns loaded stack (with mode override) on success
- Errors in interactive mode to defer to user selection
- Requires explicit provider in non-interactive mode
- Creates ephemeral stack params and loads into env when non-interactive with explicit provider
One observation: the stack is not persisted to disk (no
sm.Createcall), only loaded into environment. This aligns with the PR goal of supporting CI deployments without requiring local stackfiles.
1286-1357: LGTM ongetStackrefactor.The function correctly orchestrates:
- Named stack loading via
loadOrCreateStack- Non-interactive default to Defang playground
- Single-stack auto-selection
- Interactive selection via
stackSelectorThe error messages are informative for each failure case.
172-179: LGTM:--providerflag deprecation removed.Per PR objectives, the
--providerflag is no longer marked deprecated.src/pkg/stacks/manager.go (4)
35-50: LGTM on NewManager initialization logic.The simplified check
targetDirectory != ""to determine if we're operating in-directory is cleaner than a separateoutsideflag.
52-98: LGTM on List method.The merge logic correctly prefers remote stacks (with deployment timestamps) over local stacks, and the sorting by name is appropriate.
194-196: LGTM on LoadParameters wrapper.Clean delegation to the package-level
LoadParametersfunction.
171-192: This concern does not apply to your codebase. Go 1.24.1 (your minimum version per go.mod) is well after Go 1.22, which fixed loop variable scoping. Each for-range iteration now has its own scope for loop variables, makingremoteStack = &remotesafe and correct. No changes needed.Likely an incorrect or invalid review comment.
src/cmd/cli/command/commands_test.go (6)
264-275: LGTM on mockStackManager struct definition.The struct now supports multiple configuration options (
loadResultsmap,loadResult,loadError,createResult,createError) which provides good flexibility for different test scenarios.
351-353: LGTM on LoadParameters mock.Clean delegation to the package-level function.
398-422: LGTM on test setup with global.Stack initialization.The cleanup correctly resets
global.Stack = stacks.StackParameters{}to avoid test pollution.
515-524: Good test hygiene with cleanup of global state.The deferred cleanup properly resets both
global.NonInteractiveandglobal.Stack.
745-803: LGTM on test execution logic.The test cases properly verify stack name, provider, region, and whence values. The error checking with
strings.Containsprovides flexibility for error message changes.
559-588: Test case correctly exercises the intended code path for new stack creation.The region "us-central1" is properly set in the
loadOrCreateStackfunction viaclient.GetRegion(params.Provider), which returns "us-central1" as the default region for GCP (seesrc/pkg/cli/client/region.go:12). The mockStackManager doesn't need to populate the region in the Create path because the actual implementation sets it before callingLoadParameters. The test correctly verifies this behavior.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/commands.go:
- Around line 1336-1342: When running in non-interactive mode
(global.NonInteractive) and no stacks exist, detect if stackParams.Provider ==
ProviderAuto and return a different error message: instead of suggesting
"--provider=auto", instruct the user to specify a concrete provider (e.g.,
"aws", "gcp", "azure" or list available providers) or provide guidance to run an
interactive stack creation; update the error branch that currently references
stackParams.Provider to check for ProviderAuto and craft an actionable message
accordingly while preserving the existing message for non-auto providers.
- Around line 1254-1284: In loadOrCreateStack, make the error message
capitalization consistent by changing the fmt.Errorf call that currently returns
fmt.Errorf("Unable to load stack %q and unable to create it with provider %q in
non-interactive mode", ...) to use lowercase "unable" so it matches the other
messages (i.e., "unable to load stack %q and unable to create it with provider
%q in non-interactive mode").
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/globals.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/globals.gosrc/cmd/cli/command/commands.go
🧬 Code graph analysis (1)
src/cmd/cli/command/commands.go (6)
src/pkg/stacks/manager.go (1)
Manager(18-23)src/pkg/stacks/stacks.go (3)
StackParameters(18-27)Load(260-262)LoadParameters(290-309)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/cli/client/provider_id.go (2)
ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/selector.go (1)
NewSelector(20-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
src/cmd/cli/command/globals.go (2)
78-80: LGTM!Clean helper method that improves readability when checking interactive mode throughout the codebase.
90-140: Good consolidation of initialization logic.The constructor pattern centralizes environment variable parsing and GlobalConfig construction, which is cleaner than the previous scattered initialization. The debug logging for invalid env values (lines 95, 103, 111) occurs during package initialization before
term.SetDebugis called, but since these are just diagnostics for malformed env vars, the impact is acceptable.src/cmd/cli/command/commands.go (1)
1303-1308: Well-designed non-interactive default behavior.This implements the core PR feature: when running in non-interactive mode (e.g., CI) without an explicit stack or provider, the code defaults to the beta stack on the Defang playground. This enables CI deployments without requiring manual stack configuration, while still allowing explicit provider/stack specification when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cmd/cli/command/stack_test.go (1)
226-261: Consider testing theoverload=falsescenario and adding negative assertions.The test currently only verifies expected env vars are set with
overload=true. Consider:
- Adding a test case for
overload=falseto ensure existing env vars are not overwritten.- Adding negative assertions to verify provider-specific env vars from other providers aren't incorrectly set (e.g., in the AWS test case, assert
GCP_LOCATIONandGCP_PROJECT_IDremain unset).Example: Test overload=false behavior
{ name: "AWS parameters with overload=false respects existing env", parameters: stacks.StackParameters{ Provider: client.ProviderAWS, Region: "us-west-2", AWSProfile: "default", Mode: modes.ModeAffordable, }, overload: false, presetEnv: map[string]string{ "AWS_REGION": "eu-west-1", // Should not be overwritten }, expectedEnv: map[string]string{ "DEFANG_PROVIDER": "aws", "AWS_REGION": "eu-west-1", // Preserved "AWS_PROFILE": "default", "DEFANG_MODE": "affordable", }, },src/cmd/cli/command/commands_test.go (1)
318-318: Consider handling the error fromLoadParameters.The error returned by
stacks.LoadParametersis being ignored. While this is test code where panics are acceptable, silently ignoring this error could mask issues during test execution.♻️ Suggested fix
- stacks.LoadParameters(params, true) + if err := stacks.LoadParameters(params, true); err != nil { + panic(fmt.Sprintf("LoadParameters failed in mock: %v", err)) + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/stack_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/commands.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/commands.gosrc/cmd/cli/command/stack_test.gosrc/cmd/cli/command/commands_test.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/stack_test.gosrc/cmd/cli/command/commands_test.go
🧬 Code graph analysis (3)
src/cmd/cli/command/commands.go (4)
src/pkg/stacks/stacks.go (3)
Load(260-262)DefaultBeta(89-89)StackParameters(18-27)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)src/pkg/cli/client/provider_id.go (2)
ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/selector.go (1)
NewSelector(20-25)
src/cmd/cli/command/stack_test.go (5)
src/pkg/stacks/stacks.go (2)
StackParameters(18-27)LoadParameters(290-309)src/pkg/cli/client/provider_id.go (2)
ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/clouds/aws/common.go (1)
Region(15-15)src/pkg/modes/modes.go (3)
Mode(12-12)ModeAffordable(16-16)ModeBalanced(17-17)src/pkg/utils.go (1)
Getenv(48-53)
src/cmd/cli/command/commands_test.go (2)
src/pkg/stacks/stacks.go (3)
Load(260-262)StackParameters(18-27)LoadParameters(290-309)src/pkg/cli/client/provider.go (1)
Provider(54-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (6)
src/cmd/cli/command/stack_test.go (1)
169-224: LGTM! Well-structured table-driven tests covering key scenarios.The refactoring to table-driven tests with
stacks.StackParametersaligns well with the updatedLoadParameterssignature. The test cases appropriately cover AWS, GCP, and prefix/suffix scenarios.src/cmd/cli/command/commands.go (2)
181-181: LGTM!The updated deprecation message aligns with the PR objective of promoting
--stackover--providerfor stack selection.
1255-1331: LGTM!The refactored
getStackfunction properly handles the different resolution paths:
- Named stack loading with mode override capability
- Non-interactive default to beta/Defang for auto provider
- Single stack auto-selection
- Interactive selection fallback
The provider validation at lines 1271-1273 correctly rejects stacks with
ProviderAuto, which would be an invalid configuration.src/cmd/cli/command/commands_test.go (3)
543-567: Good coverage for unknown stack scenarios.These test cases properly validate that:
- An unknown stack without a provider flag returns the expected error
- An unknown stack with a valid provider flag still returns the not-found error (confirming the stack must exist before loading)
This aligns with the PR objective that a stack must be created before it can be referenced.
382-427: LGTM!The test setup correctly initializes
global.Stackwith the stack name before invokingnewProvider, which aligns with the updatedgetStacklogic that requires a named stack to be loaded viasm.Load.
729-787: LGTM!The test loop properly sets up
global.Stackwith both the stack name and provider before callinggetStack. The use ofprovider.Set(tc.providerFlag)correctly handles the provider flag parsing, and the test cases comprehensively cover the various resolution paths.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/cmd/cli/command/globals.go (1)
15-64: Update outdated documentation.The comment block describes
syncFlagsWithEnv()andsyncNonFlagEnvVars()methods that no longer exist after the refactoring. The configuration flow has changed to environment-driven initialization viaNewGlobalConfig(), making these instructions obsolete.📝 Suggested documentation update
Update the comment block to reflect the new initialization flow:
- Describe how
NewGlobalConfig()reads environment variables directly- Remove references to
syncFlagsWithEnv()andsyncNonFlagEnvVars()- Update the "Configuration Flow" section to describe the constructor-based approach
- Simplify the "Adding New Configuration Options" section since synchronization logic is removed
src/pkg/stacks/wizard.go (1)
38-123: Potential panic: writing toparams.Variableswhen it’s nil.
CollectParameters()passes&StackParameters{}; unlessSetProvider/SetRegioninitializeVariables, assignments likeparams.Variables["AWS_PROFILE"] = .../["GCP_PROJECT_ID"] = ...can panic.Proposed fix
func (w *Wizard) CollectRemainingParameters(ctx context.Context, params *StackParameters) (*StackParameters, error) { + if params.Variables == nil { + params.Variables = map[string]string{} + } + if params.GetProvider() == client.ProviderAuto { var providerNames []string for _, p := range client.AllProviders() { providerNames = append(providerNames, p.Name()) } @@ switch params.GetProvider() { case client.ProviderAWS: if params.Variables["AWS_PROFILE"] == "" { @@ case client.ProviderGCP: if params.Variables["GCP_PROJECT_ID"] == "" {src/pkg/stacks/manager.go (1)
97-131: Potential panic / bad key: unguardedGetRegionVarName()+variables[""] = ....
client.GetRegionVarName(providerID)can return""(or panic on unsupported provider). Thenvariables[regionVarName] = deployment.GetRegion()will write to the empty key.Proposed fix
variables := map[string]string{ "DEFANG_PROVIDER": providerID.String(), "DEFANG_MODE": deployment.GetMode().String(), } regionVarName := client.GetRegionVarName(providerID) - variables[regionVarName] = deployment.GetRegion() + if regionVarName != "" && deployment.GetRegion() != "" { + variables[regionVarName] = deployment.GetRegion() + }src/cmd/cli/command/commands.go (2)
520-578: Critical:whoamican panic by usingsession.Providerwhen session failed to load.If
LoadSession()returns(nil, err), the code warns but then dereferencessession.Provider.Proposed fix
session, err := sessionLoader.LoadSession(ctx) if err != nil { - term.Warn("unable to load session:", err) + term.Warn("unable to load session:", err) + return err } @@ - data, err := cli.Whoami(cmd.Context(), global.Client, session.Provider, userInfo, global.Tenant) + data, err := cli.Whoami(cmd.Context(), global.Client, session.Provider, userInfo, global.Tenant)
1168-1214: Set ProviderID from the persistent--providerflag instead of leaving it empty.Currently,
ProviderIDis only populated if the provider name is detected in theprojectNamestring. In normal cases, it remains empty, breaking fallback stack loading and provider initialization. Useglobal.Stack.GetProvider()to retrieve the actual provider value from the--providerpersistent flag.Proposed fix
func NewSessionLoaderOptionsForCommand(cmd *cobra.Command) session.SessionLoaderOptions { stack, err := cmd.Flags().GetString("stack") if err != nil { panic(err) } + + // provider is a persistent flag wired into global.Stack.Provider + prov := global.Stack.GetProvider() configPaths, err := cmd.Flags().GetStringArray("file") if err != nil { panic(err) } @@ - // Avoid common mistakes - var prov client.ProviderID - if prov.Set(projectName) == nil && !cmd.Flag("provider").Changed { + // Avoid common mistakes (use a temp var so we don't clobber the real provider) + var maybeProvider client.ProviderID + if maybeProvider.Set(projectName) == nil && !cmd.Flag("provider").Changed { // using -p with a provider name instead of -P term.Warnf("Project name %q looks like a provider name; did you mean to use -P=%s instead of -p?", projectName, projectName) doubleCheckProjectName(projectName) } else if strings.HasPrefix(projectName, "roject-name") { @@ return session.SessionLoaderOptions{ Stack: stack, ProviderID: prov, ComposeFilePaths: configPaths, ProjectName: projectName, Interactive: !global.NonInteractive, } }Also applies to: 1216-1219
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/commands.go:
- Around line 184-186: The code still calls
RootCmd.Flags().MarkDeprecated("provider", ...) which both contradicts the PR
objective to stop marking --provider deprecated and may target the wrong flagset
since provider is a persistent flag; remove the MarkDeprecated call entirely and
if you intended to modify a different flag, ensure you call MarkDeprecated on
the correct flagset (use RootCmd.PersistentFlags() for persistent flags) and
reference the flag name "provider" (the CLI flag --provider) accordingly so the
deprecation behavior is fully removed or correctly applied.
In @src/pkg/session/session.go:
- Around line 162-173: loadFallbackStack currently forces provider from
sl.opts.ProviderID and newProvider also always reads opts.ProviderID, which can
produce the wrong provider when a loaded stack specifies a different one; change
the flow to bind the provider to the stack and make provider resolution use the
stack's provider when present. Update loadFallbackStack (and any other code that
constructs the fallback stack around lines 175-177) to set the provider
identifier into the returned stacks.StackParameters (e.g., via the
"DEFANG_PROVIDER" variable or an explicit field) and refactor newProvider to
accept the stack (or an explicit providerID argument) and prefer the provider
declared on the stack over sl.opts.ProviderID; ensure callers pass the loaded
stack into newProvider so provider selection comes from the stack first, falling
back to opts only if the stack has none.
- Around line 47-94: LoadSession currently returns early when
findTargetDirectory fails and sl.opts.ProjectName == "", causing callers to get
a nil session; change the error handling so a missing project dir is non-fatal
when no project name is supplied: if findTargetDirectory returns an error and
sl.opts.ProjectName == "" treat that as a non-fatal condition (set
targetDirectory to "" or a zero value and continue) instead of returning the
error, but preserve returning fatal errors for other error cases; keep creating
the loader/provider and ensure a non-nil Session is returned (the function
LoadSession, the call to sl.findTargetDirectory, and the provider.AccountInfo
handling are the relevant symbols to modify).
- Around line 138-159: In loadSpecifiedStack, the code incorrectly uses
errors.Is(err, &stacks.ErrOutside{}) which will never match; change the
persistence error check to detect stacks.ErrOutside via errors.As (or by
comparing its type) after sm.Create(*stack) so that an ErrOutside is treated as
non-fatal while other errors return a wrapped error; update the condition that
currently reads errors.Is(err, &stacks.ErrOutside{}) to use errors.As to extract
a *stacks.ErrOutside and handle it accordingly, then keep the existing logging
of stackFilename and return the stack.
In @src/pkg/stacks/manager.go:
- Around line 93-95: ListLocal currently calls ListInDirectory unconditionally
causing List to hard-fail when sm.targetDirectory == ""; change ListLocal
(method on manager) to check sm.targetDirectory and immediately return an empty
[]StackListItem, nil when it's empty, and otherwise call
ListInDirectory(sm.targetDirectory); additionally, make List ignore/localize
errors from ListLocal when the targetDirectory is empty or when the error
corresponds to “.defang not found” (treat those cases as empty results) so
callers of List don’t fail for the supported remote-only scenario.
- Around line 150-157: The Load method currently falls back to sm.LoadRemote on
any LoadLocal error; change it so Load only calls sm.LoadRemote when the local
error indicates "not found" (use errors.Is(err, fs.ErrNotExist) or
os.IsNotExist(err) or compare to your package's ErrNotFound if defined). For any
other LoadLocal error (parse/permission/etc.) return that error immediately
(preserve the original error in the returned value or wrap it with context)
instead of silently attempting remote import; keep symbols: manager.Load,
sm.LoadLocal, sm.LoadRemote, and StackParameters to locate the change.
- Around line 171-192: In LoadRemote, assigning remoteStack = &remote captures
the loop variable address (the same address each iteration) which yields the
wrong StackListItem; instead locate the slice element's address by using the
slice index (e.g., take &remoteStacks[i]) or otherwise copy the value into a new
variable and take its address; then pass remoteStack.StackParameters to
sm.LoadParameters and return &remoteStack.StackParameters as before to ensure
the pointer refers to the correct element.
In @src/pkg/stacks/stacks.go:
- Around line 56-65: GetMode currently swallows parse errors and silently
returns ModeUnspecified; update StackParameters.GetMode to detect when
modes.Parse(sp.Variables["DEFANG_MODE"]) returns an error and emit a warning log
containing the invalid value and the parse error (use the existing logger on
StackParameters if present, e.g. sp.Logger, or the package logger) before
falling back to modes.ModeUnspecified so misconfigurations are visible.
- Around line 31-37: GetProvider currently ignores the error from
providerID.Set(val) and returns a zero value on invalid data; change GetProvider
to capture the error returned by providerID.Set(val) and emit a warning/debug
log when Set fails (do not swallow the error silently). Update the
StackParameters.GetProvider implementation to check the error from
providerID.Set and call your project's logger (e.g., log.Printf or the existing
logger used elsewhere) with a clear message including the invalid value and
context (similar validation performed in ParamsFromMap), then return the
providerID as before.
- Around line 67-74: The ToMap method on StackParameters currently mutates the
original sp.Variables by assigning vars := sp.Variables and then writing new
keys; make ToMap read-only by creating a new map, copying all key/value pairs
from sp.Variables into it, then add the extra keys ("DEFANG_PROVIDER",
regionVarName, "DEFANG_MODE") and return the new map; update the function that
uses sp.Variables to reference the copied map and leave sp.Variables unmodified
(referencing StackParameters.ToMap, sp.Variables, GetProvider, GetRegion, and
GetMode to locate the code).
- Around line 18-25: StackParameters currently duplicates Provider/Mode/Region
as struct fields while getters/setters use Variables causing stale state; remove
the redundant Provider, Mode, and Region fields from the StackParameters struct
and update ParamsFromMap to stop assigning those removed fields (rely on
Variables entries instead), and ensure GetProvider/GetMode/GetRegion and
SetProvider/SetMode/SetRegion continue to read/write the Variables map so all
code uses the Variables canonical store (also update any external code that
referenced the removed struct fields to call the getters).
🧹 Nitpick comments (5)
src/cmd/cli/command/globals.go (1)
143-161: Consider including stack name and tenant in ToMap() output.The method exports configuration as environment variables but omits
DEFANG_STACKandDEFANG_WORKSPACE/DEFANG_ORG, which are part of the GlobalConfig. Including these would provide a complete environment representation.♻️ Proposed enhancement
func (global *GlobalConfig) ToMap() map[string]string { m := make(map[string]string) m["DEFANG_CLUSTER"] = global.Cluster m["DEFANG_COLOR"] = global.ColorMode.String() m["DEFANG_DEBUG"] = strconv.FormatBool(global.Debug) m["DEFANG_NON_INTERACTIVE"] = strconv.FormatBool(global.NonInteractive) + if global.Stack.Name != "" { + m["DEFANG_STACK"] = global.Stack.Name + } + if global.Tenant != "" { + m["DEFANG_WORKSPACE"] = string(global.Tenant) + } if global.Stack.Provider != client.ProviderAuto { m["DEFANG_PROVIDER"] = global.Stack.Provider.String() } if global.Stack.Region != "" { regionVarName := client.GetRegionVarName(global.Stack.Provider) m[regionVarName] = global.Stack.Region } if global.Stack.Mode != modes.ModeUnspecified { m["DEFANG_MODE"] = global.Stack.Mode.String() } m["DEFANG_VERBOSE"] = strconv.FormatBool(global.Verbose) return m }src/pkg/agent/tools/provider.go (1)
46-47: Consider caching GetProvider() result.
GetProvider()is called twice on consecutive lines. While this is unlikely to be a performance issue, caching the result would be slightly more efficient.♻️ Proposed optimization
term.Debug("Function invoked: cli.NewProvider") - provider := pp.pc.NewProvider(ctx, stack.GetProvider(), pp.fc, stack.Name) - providerID := stack.GetProvider() + providerID := stack.GetProvider() + provider := pp.pc.NewProvider(ctx, providerID, pp.fc, stack.Name) return &providerID, provider, nilsrc/pkg/stacks/stacks_test.go (1)
302-327: Tests may be asserting the wrong shape (direct fields vs Variables/getters), and “.” hides workingDirectory bugs.
- Since stacks are now directory-scoped, using
"."aftert.Chdir(t.TempDir())works but doesn’t exercise non-dotworkingDirectoryinputs (a common failure mode when callers pass an explicit dir).- If
StackParametersis now Variables-backed (with getters), comparingparams.Provider/Region/Modedirectly may be brittle or incorrect; preferparams.GetProvider() / GetRegion(...) / GetMode()or comparingparams.ToMap()/params.Variables.Also applies to: 343-380
src/pkg/agent/tools/create_aws_stack.go (1)
33-41: Guard required inputs + avoid stringly-typed env keys.Consider validating
params.Name,params.Region, andparams.AWS_Profilebefore creating the stackfile, and centralizing the variable keys (constants or helper methods) to avoid subtle mismatches (e.g.,AWS_REGIONvs other region var names).src/pkg/stacks/stacks.go (1)
39-50: Consider simplifying GetRegion/SetRegion by removing the provider parameter.Both methods accept a
providerparameter, yetToMap(lines 70–71) demonstrates the typical pattern: retrieving the provider viasp.GetProvider()and passing it toGetRegion. Allowing callers to specify an arbitrary provider permits setting/getting a region variable that doesn't correspond tosp's stored provider, which can lead to subtle configuration mismatches.If there's no use case for querying or setting regions for providers other than the one already stored in the StackParameters, removing the parameter and using
sp.GetProvider()internally would reduce the API surface and prevent accidental misuse.♻️ Proposed refactor to remove provider parameter
-func (sp *StackParameters) SetRegion(provider client.ProviderID, region string) { - regionVarName := client.GetRegionVarName(provider) +func (sp *StackParameters) SetRegion(region string) { + regionVarName := client.GetRegionVarName(sp.GetProvider()) sp.Variables[regionVarName] = region } -func (sp *StackParameters) GetRegion(provider client.ProviderID) string { - regionVarName := client.GetRegionVarName(provider) +func (sp *StackParameters) GetRegion() string { + regionVarName := client.GetRegionVarName(sp.GetProvider()) if val, ok := sp.Variables[regionVarName]; ok { return val } return "" }Then update
ToMap:func (sp StackParameters) ToMap() map[string]string { // ... (after fixing the map copy issue) vars["DEFANG_PROVIDER"] = sp.GetProvider().String() - regionVarName := client.GetRegionVarName(sp.GetProvider()) - vars[regionVarName] = sp.GetRegion(sp.GetProvider()) + vars[client.GetRegionVarName(sp.GetProvider())] = sp.GetRegion() vars["DEFANG_MODE"] = sp.GetMode().String() return vars }Note: This change would require updating all call sites in other files (e.g.,
wizard.go,create_aws_stack.go,create_gcp_stack.go).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/cmd/cli/command/cd.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/globals.gosrc/pkg/agent/tools/create_aws_stack.gosrc/pkg/agent/tools/create_gcp_stack.gosrc/pkg/agent/tools/provider.gosrc/pkg/mcp/mcp_server.gosrc/pkg/session/session.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/selector.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/wizard.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/globals.gosrc/pkg/agent/tools/create_gcp_stack.gosrc/pkg/mcp/mcp_server.gosrc/cmd/cli/command/cd.gosrc/pkg/stacks/manager.gosrc/pkg/agent/tools/create_aws_stack.gosrc/pkg/stacks/stacks_test.gosrc/pkg/agent/tools/provider.gosrc/pkg/stacks/wizard.gosrc/cmd/cli/command/commands.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/selector.gosrc/pkg/session/session.go
📚 Learning: 2026-01-09T20:12:21.986Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:30-30
Timestamp: 2026-01-09T20:12:21.986Z
Learning: In Go files, recognize and accept the import path go.yaml.in/yaml/v3 as the maintained fork of the YAML library. Do not flag this import as incorrect; this fork supersedes the archived gopkg.in/yaml.v3 path. If you encounter this or similar forked import paths, treat them as valid Go imports and do not raise review flags.
Applied to files:
src/cmd/cli/command/globals.gosrc/pkg/agent/tools/create_gcp_stack.gosrc/pkg/mcp/mcp_server.gosrc/cmd/cli/command/cd.gosrc/pkg/stacks/manager.gosrc/pkg/agent/tools/create_aws_stack.gosrc/pkg/stacks/stacks_test.gosrc/pkg/agent/tools/provider.gosrc/pkg/stacks/wizard.gosrc/cmd/cli/command/commands.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/selector.gosrc/pkg/session/session.go
📚 Learning: 2026-01-06T20:32:17.173Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/clouds/gcp/cloudbuild.go:120-123
Timestamp: 2026-01-06T20:32:17.173Z
Learning: In src/pkg/clouds/gcp/cloudbuild.go, the YAML unmarshaling into cloudbuildpb.BuildStep works because CloudBuildStep (from byoc.go) marshals to YAML with lowercase keys (name, args, env, entrypoint) that match the protobuf BuildStep exported field names via case-insensitive matching in yaml.Unmarshal. This is an intentional intermediate-struct pattern.
Applied to files:
src/pkg/agent/tools/create_gcp_stack.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/pkg/stacks/stacks_test.go
🧬 Code graph analysis (11)
src/cmd/cli/command/globals.go (8)
src/cmd/cli/command/color.go (2)
ColorAuto(11-11)ColorMode(5-5)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
ModeUnspecified(15-15)Mode(12-12)src/pkg/utils.go (2)
GetenvBool(56-59)Getenv(48-53)src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)src/pkg/cli/client/cluster.go (1)
DefangFabric(16-16)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/region.go (2)
GetRegion(5-24)GetRegionVarName(26-41)
src/cmd/cli/command/cd.go (1)
src/pkg/modes/modes.go (1)
Mode(12-12)
src/pkg/stacks/manager.go (2)
src/pkg/stacks/stacks.go (6)
List(166-168)StackListItem(161-164)StackParameters(18-25)LoadParameters(268-287)Create(110-112)ReadInDirectory(230-244)src/pkg/cli/client/region.go (2)
GetRegionVarName(26-41)GetRegion(5-24)
src/pkg/agent/tools/create_aws_stack.go (1)
src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)
src/pkg/stacks/stacks_test.go (1)
src/pkg/stacks/stacks.go (3)
CreateInDirectory(114-158)ReadInDirectory(230-244)LoadInDirectory(246-254)
src/pkg/agent/tools/provider.go (1)
src/pkg/cli/connect.go (1)
NewProvider(36-50)
src/pkg/stacks/wizard.go (4)
src/pkg/cli/client/provider_id.go (3)
ProviderAuto(13-13)ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/cli/client/region.go (1)
GetRegion(5-24)src/pkg/stacks/stacks.go (1)
MakeDefaultName(105-108)src/pkg/utils.go (1)
Getenv(48-53)
src/cmd/cli/command/commands.go (12)
src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/track/track.go (2)
Cmd(64-88)P(19-21)src/pkg/session/session.go (2)
NewSessionLoader(39-45)SessionLoaderOptions(24-31)src/pkg/cli/client/provider.go (2)
Provider(54-80)Loader(82-86)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/cli/compose/loader.go (2)
Loader(38-41)Project(25-25)src/pkg/cli/configList.go (1)
ConfigList(15-35)src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/modes/modes.go (1)
ModeUnspecified(15-15)src/pkg/stacks/stacks.go (1)
DefaultBeta(101-101)src/pkg/stacks/selector.go (1)
NewSelector(20-25)
src/pkg/stacks/stacks.go (3)
src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)src/pkg/cli/client/region.go (2)
GetRegionVarName(26-41)GetRegion(5-24)
src/pkg/stacks/selector.go (1)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)
src/pkg/session/session.go (5)
src/pkg/stacks/stacks.go (4)
StackParameters(18-25)Directory(102-102)Create(110-112)DefaultBeta(101-101)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)src/pkg/stacks/selector.go (2)
NewSelector(20-25)SelectStackOptions(34-37)src/pkg/cli/connect.go (1)
NewProvider(36-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/cmd/cli/command/globals.go (2)
79-81: LGTM!Clean helper method for checking interactive mode.
91-141: Environment variable handling for all providers is correctly implemented.The code properly handles all 5 provider types (AWS, GCP, DigitalOcean, Defang, and Auto) with provider-specific environment variables and defaults:
- AWS uses
AWS_REGION(default:us-west-2)- GCP uses
GCP_LOCATION(default:us-central1)- DigitalOcean uses
REGION(default:nyc3)- Defang and Auto return empty string with no env var
The
GetRegion()andGetRegionVarName()implementations are consistent with the codebase and properly enforce defaults.src/cmd/cli/command/cd.go (1)
170-170: LGTM!The change to use
GetMode()accessor aligns with the new StackParameters API pattern.src/pkg/mcp/mcp_server.go (1)
84-89: LGTM!The change to retrieve provider ID via
config.Stack.GetProvider()and store it as a reference for the ToolTracker is clean and aligns with the accessor pattern introduced across the codebase.src/pkg/agent/tools/create_gcp_stack.go (1)
33-41: LGTM!The migration to a Variables-based approach for StackParameters is clean and includes all necessary configuration (provider, mode, region, project ID). This aligns with the broader refactoring to consolidate stack parameters into a key-value structure.
src/cmd/cli/command/commands.go (1)
54-56: Centralizing elicitations controller is fine.This removes repeated construction and should make interactive flows more consistent.
| // RootCmd.Flag("provider").NoOptDefVal = "auto" NO this will break the "--provider aws" | ||
| RootCmd.Flags().MarkDeprecated("provider", "please use --stack instead") | ||
| RootCmd.Flags().MarkDeprecated("provider", "use '--stack' to select a stack instead") | ||
| RootCmd.PersistentFlags().BoolVarP(&global.Verbose, "verbose", "v", global.Verbose, "verbose logging") // backwards compat: only used by tail |
There was a problem hiding this comment.
--provider is still marked deprecated (and possibly on the wrong flagset), which conflicts with the PR objectives.
PR objectives say to remove the deprecated mark from --provider, but this still calls MarkDeprecated. Also, provider is a persistent flag, so marking it via RootCmd.Flags() may not actually apply.
🤖 Prompt for AI Agents
In @src/cmd/cli/command/commands.go around lines 184 - 186, The code still calls
RootCmd.Flags().MarkDeprecated("provider", ...) which both contradicts the PR
objective to stop marking --provider deprecated and may target the wrong flagset
since provider is a persistent flag; remove the MarkDeprecated call entirely and
if you intended to modify a different flag, ensure you call MarkDeprecated on
the correct flagset (use RootCmd.PersistentFlags() for persistent flags) and
reference the flag name "provider" (the CLI flag --provider) accordingly so the
deprecation behavior is fully removed or correctly applied.
| func (sl *SessionLoader) LoadSession(ctx context.Context) (*Session, error) { | ||
| // cd into working dir with .defang directory, assume a compose file is also there | ||
| targetDirectory, err := sl.findTargetDirectory() | ||
| if err != nil { | ||
| if sl.opts.ProjectName == "" { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| return nil, errors.New("project name is required when outside of a project directory") | ||
| } | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| // load stack | ||
| stack, err := sl.loadStack(ctx, targetDirectory) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // TODO: update the environment and globals with the values from any corresponding stack parameters unless overwritten by flags | ||
| // iterate over the stack variables | ||
| // for each, if the corresponding global.ToMap() is not the empty value, bail | ||
| // if any global config properties | ||
| // TODO: the stack may change the project name and compose file paths | ||
| // if stack.ProjectName != "" { | ||
| // sl.opts.ProjectName = stack.ProjectName | ||
| // } | ||
| // if len(stack.ComposeFilePaths) > 0 { | ||
| // sl.opts.ComposeFilePaths = stack.ComposeFilePaths | ||
| // } | ||
| // initialize loader | ||
| loader := compose.NewLoader( | ||
| compose.WithProjectName(sl.opts.ProjectName), | ||
| compose.WithPath(sl.opts.ComposeFilePaths...), | ||
| ) | ||
| // load provider with selected stack | ||
| provider := sl.newProvider(ctx, stack.Name) | ||
| session := &Session{ | ||
| Stack: stack, | ||
| Loader: loader, | ||
| Provider: provider, | ||
| } | ||
| _, err = provider.AccountInfo(ctx) | ||
| if err != nil { | ||
| // HACK: return the session even on error to allow `whoami` and `compose config` to return a result even on provider error | ||
| return session, fmt.Errorf("failed to get account info from provider %q: %w", stack.GetProvider(), err) | ||
| } | ||
| // also call accountInfo and update the region | ||
| return session, nil | ||
| } |
There was a problem hiding this comment.
Session loading is too strict outside project dirs; callers can end up with nil session.
If findTargetDirectory() fails and opts.ProjectName == "", LoadSession() returns an error. That likely breaks commands that should work anywhere (notably whoami), and any caller that logs+continues risks dereferencing session when it’s nil.
🤖 Prompt for AI Agents
In @src/pkg/session/session.go around lines 47 - 94, LoadSession currently
returns early when findTargetDirectory fails and sl.opts.ProjectName == "",
causing callers to get a nil session; change the error handling so a missing
project dir is non-fatal when no project name is supplied: if
findTargetDirectory returns an error and sl.opts.ProjectName == "" treat that as
a non-fatal condition (set targetDirectory to "" or a zero value and continue)
instead of returning the error, but preserve returning fatal errors for other
error cases; keep creating the loader/provider and ensure a non-nil Session is
returned (the function LoadSession, the call to sl.findTargetDirectory, and the
provider.AccountInfo handling are the relevant symbols to modify).
| func (sl *SessionLoader) loadSpecifiedStack(ctx context.Context, sm stacks.Manager, name string) (*stacks.StackParameters, error) { | ||
| stack, err := sm.LoadLocal(name) | ||
| if err == nil { | ||
| return stack, nil | ||
| } | ||
| // the stack file does not exist locally | ||
| if !os.IsNotExist(err) { | ||
| return nil, err | ||
| } | ||
| stack, err = sm.LoadRemote(ctx, name) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load stack %q remotely: %w", name, err) | ||
| } | ||
| // persist the remote stack file to the local target directory | ||
| stackFilename, err := sm.Create(*stack) | ||
| if err != nil && !errors.Is(err, &stacks.ErrOutside{}) { | ||
| return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err) | ||
| } | ||
| if stackFilename != "" { | ||
| term.Infof("Stack %q loaded and saved to %q. Add this file to source control", name, stackFilename) | ||
| } | ||
| return stack, nil |
There was a problem hiding this comment.
Bug: errors.Is(err, &stacks.ErrOutside{}) won’t match; use errors.As.
This currently treats “outside project” as a hard error when persisting an imported stack.
Proposed fix
stackFilename, err := sm.Create(*stack)
- if err != nil && !errors.Is(err, &stacks.ErrOutside{}) {
+ var outside *stacks.ErrOutside
+ if err != nil && !errors.As(err, &outside) {
return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sl *SessionLoader) loadSpecifiedStack(ctx context.Context, sm stacks.Manager, name string) (*stacks.StackParameters, error) { | |
| stack, err := sm.LoadLocal(name) | |
| if err == nil { | |
| return stack, nil | |
| } | |
| // the stack file does not exist locally | |
| if !os.IsNotExist(err) { | |
| return nil, err | |
| } | |
| stack, err = sm.LoadRemote(ctx, name) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load stack %q remotely: %w", name, err) | |
| } | |
| // persist the remote stack file to the local target directory | |
| stackFilename, err := sm.Create(*stack) | |
| if err != nil && !errors.Is(err, &stacks.ErrOutside{}) { | |
| return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err) | |
| } | |
| if stackFilename != "" { | |
| term.Infof("Stack %q loaded and saved to %q. Add this file to source control", name, stackFilename) | |
| } | |
| return stack, nil | |
| func (sl *SessionLoader) loadSpecifiedStack(ctx context.Context, sm stacks.Manager, name string) (*stacks.StackParameters, error) { | |
| stack, err := sm.LoadLocal(name) | |
| if err == nil { | |
| return stack, nil | |
| } | |
| // the stack file does not exist locally | |
| if !os.IsNotExist(err) { | |
| return nil, err | |
| } | |
| stack, err = sm.LoadRemote(ctx, name) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to load stack %q remotely: %w", name, err) | |
| } | |
| // persist the remote stack file to the local target directory | |
| stackFilename, err := sm.Create(*stack) | |
| var outside *stacks.ErrOutside | |
| if err != nil && !errors.As(err, &outside) { | |
| return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err) | |
| } | |
| if stackFilename != "" { | |
| term.Infof("Stack %q loaded and saved to %q. Add this file to source control", name, stackFilename) | |
| } | |
| return stack, nil | |
| } |
🤖 Prompt for AI Agents
In @src/pkg/session/session.go around lines 138 - 159, In loadSpecifiedStack,
the code incorrectly uses errors.Is(err, &stacks.ErrOutside{}) which will never
match; change the persistence error check to detect stacks.ErrOutside via
errors.As (or by comparing its type) after sm.Create(*stack) so that an
ErrOutside is treated as non-fatal while other errors return a wrapped error;
update the condition that currently reads errors.Is(err, &stacks.ErrOutside{})
to use errors.As to extract a *stacks.ErrOutside and handle it accordingly, then
keep the existing logging of stackFilename and return the stack.
| func (sl *SessionLoader) loadFallbackStack() (*stacks.StackParameters, error) { | ||
| if sl.opts.ProviderID == "" { | ||
| return nil, errors.New("--provider is required if --stack is not specified") | ||
| } | ||
| // TODO: list remote stacks, and if there is exactly one with the matched provider, load it | ||
| return &stacks.StackParameters{ | ||
| Name: stacks.DefaultBeta, | ||
| Variables: map[string]string{ | ||
| "DEFANG_PROVIDER": sl.opts.ProviderID.String(), | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Provider selection is likely wrong: provider should come from the stack (not only from opts).
loadFallbackStack() requires opts.ProviderID, but newProvider() always uses opts.ProviderID even when a loaded stack specifies a different provider. This can construct the wrong provider (or default) and then cascade into misleading failures.
Proposed fix (bind provider to stack)
-func (sl *SessionLoader) newProvider(ctx context.Context, stackName string) client.Provider {
- return cli.NewProvider(ctx, sl.opts.ProviderID, sl.client, stackName)
+func (sl *SessionLoader) newProvider(ctx context.Context, stack *stacks.StackParameters) client.Provider {
+ providerID := stack.GetProvider()
+ if providerID == "" || providerID == client.ProviderAuto {
+ providerID = sl.opts.ProviderID
+ }
+ return cli.NewProvider(ctx, providerID, sl.client, stack.Name)
}- provider := sl.newProvider(ctx, stack.Name)
+ provider := sl.newProvider(ctx, stack)Also applies to: 175-177
🤖 Prompt for AI Agents
In @src/pkg/session/session.go around lines 162 - 173, loadFallbackStack
currently forces provider from sl.opts.ProviderID and newProvider also always
reads opts.ProviderID, which can produce the wrong provider when a loaded stack
specifies a different one; change the flow to bind the provider to the stack and
make provider resolution use the stack's provider when present. Update
loadFallbackStack (and any other code that constructs the fallback stack around
lines 175-177) to set the provider identifier into the returned
stacks.StackParameters (e.g., via the "DEFANG_PROVIDER" variable or an explicit
field) and refactor newProvider to accept the stack (or an explicit providerID
argument) and prefer the provider declared on the stack over sl.opts.ProviderID;
ensure callers pass the loaded stack into newProvider so provider selection
comes from the stack first, falling back to opts only if the stack has none.
| func (sm *manager) ListLocal() ([]StackListItem, error) { | ||
| if sm.outside { | ||
| return nil, &ErrOutside{Operation: "ListLocal", TargetDirectory: sm.targetDirectory} | ||
| } | ||
| return ListInDirectory(sm.targetDirectory) | ||
| } |
There was a problem hiding this comment.
Listing local stacks should not fail when targetDirectory == "" (outside project).
Right now List() hard-fails if ListLocal() errors. If targetDirectory is empty (a supported case in this PR for remote stacks), local listing should probably be an empty list (or “.defang not found” treated as empty), not an error.
Also applies to: 58-66
🤖 Prompt for AI Agents
In @src/pkg/stacks/manager.go around lines 93 - 95, ListLocal currently calls
ListInDirectory unconditionally causing List to hard-fail when
sm.targetDirectory == ""; change ListLocal (method on manager) to check
sm.targetDirectory and immediately return an empty []StackListItem, nil when
it's empty, and otherwise call ListInDirectory(sm.targetDirectory);
additionally, make List ignore/localize errors from ListLocal when the
targetDirectory is empty or when the error corresponds to “.defang not found”
(treat those cases as empty results) so callers of List don’t fail for the
supported remote-only scenario.
| func (sm *manager) LoadRemote(ctx context.Context, name string) (*StackParameters, error) { | ||
| remoteStacks, err := sm.ListRemote(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list remote stacks: %w", err) | ||
| } | ||
| var remoteStack *StackListItem | ||
| for _, remote := range remoteStacks { | ||
| if remote.Name == name { | ||
| remoteStack = &remote | ||
| break | ||
| } | ||
| } | ||
| if remoteStack == nil { | ||
| return nil, fmt.Errorf("unable to find stack %q", name) | ||
| } | ||
| err = sm.LoadParameters(remoteStack.StackParameters, false) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to import stack %q: %w", name, err) | ||
| } | ||
|
|
||
| return &remoteStack.StackParameters, nil | ||
| } |
There was a problem hiding this comment.
Critical Go bug: taking &remote in a range loop returns the wrong stack.
remoteStack = &remote points at the loop variable, not the slice element.
Proposed fix
var remoteStack *StackListItem
- for _, remote := range remoteStacks {
- if remote.Name == name {
- remoteStack = &remote
+ for i := range remoteStacks {
+ if remoteStacks[i].Name == name {
+ remoteStack = &remoteStacks[i]
break
}
}
@@
- return &remoteStack.StackParameters, nil
+ return &remoteStack.StackParameters, nil🤖 Prompt for AI Agents
In @src/pkg/stacks/manager.go around lines 171 - 192, In LoadRemote, assigning
remoteStack = &remote captures the loop variable address (the same address each
iteration) which yields the wrong StackListItem; instead locate the slice
element's address by using the slice index (e.g., take &remoteStacks[i]) or
otherwise copy the value into a new variable and take its address; then pass
remoteStack.StackParameters to sm.LoadParameters and return
&remoteStack.StackParameters as before to ensure the pointer refers to the
correct element.
| func (ss *stackSelector) SelectOrCreateStack(ctx context.Context) (*StackParameters, error) { | ||
| return ss.selectStack(ctx, true) | ||
| return ss.SelectStackWithOptions(ctx, SelectStackOptions{ | ||
| AllowCreate: true, | ||
| AllowImport: false, | ||
| }) | ||
| } | ||
|
|
||
| type SelectStackOptions struct { | ||
| AllowCreate bool | ||
| AllowImport bool | ||
| } | ||
|
|
||
| func (ss *stackSelector) SelectStack(ctx context.Context) (*StackParameters, error) { | ||
| return ss.selectStack(ctx, false) | ||
| return ss.SelectStackWithOptions(ctx, SelectStackOptions{ | ||
| AllowCreate: false, | ||
| AllowImport: false, | ||
| }) | ||
| } |
There was a problem hiding this comment.
AllowImport is currently a no-op (likely missing behavior).
SelectStackOptions.AllowImport is exposed and propagated (e.g., from session loading) but never used here, so “import” capability can’t actually be enabled/disabled by callers yet.
Also, if the selected name isn’t found in stackList (unexpected but possible with label collisions/changes), the code returns a zero-value StackParameters without error—consider failing fast.
Also applies to: 46-109
| type StackParameters struct { | ||
| Name string | ||
| Provider client.ProviderID | ||
| Region string | ||
| AWSProfile string | ||
| GCPProjectID string | ||
| Mode modes.Mode | ||
| Name string | ||
| Provider client.ProviderID | ||
| Mode modes.Mode | ||
| Region string | ||
| // replace properties with variable map, but keep getters and setters for common ones | ||
| Variables map[string]string | ||
| } |
There was a problem hiding this comment.
Resolve the dual storage of Provider, Mode, and Region.
The struct contains both direct fields (Provider, Mode, Region) and a Variables map, but the getters exclusively read from Variables while the setters only update Variables. This creates a source-of-truth ambiguity: ParamsFromMap populates Provider and Mode fields directly (lines 93-94), but subsequent calls to SetProvider or SetMode only modify Variables, leaving the direct fields stale and potentially misleading future readers.
Either remove the direct Provider/Mode/Region fields entirely (since Variables is the canonical store) or ensure all accessors and mutators keep both representations synchronized.
♻️ Suggested fix: remove redundant direct fields
type StackParameters struct {
Name string
- Provider client.ProviderID
- Mode modes.Mode
- Region string
// replace properties with variable map, but keep getters and setters for common ones
Variables map[string]string
}Then update ParamsFromMap to remove the now-invalid field assignments:
func ParamsFromMap(properties map[string]string) (StackParameters, error) {
var provider client.ProviderID
if val, ok := properties["DEFANG_PROVIDER"]; ok {
err := provider.Set(val)
if err != nil {
return StackParameters{}, fmt.Errorf("invalid DEFANG_PROVIDER value %q: %w", val, err)
}
}
var mode modes.Mode
if val, ok := properties["DEFANG_MODE"]; ok {
err := mode.Set(val)
if err != nil {
return StackParameters{}, fmt.Errorf("invalid DEFANG_MODE value %q: %w", val, err)
}
}
return StackParameters{
Variables: properties,
- Provider: provider,
- Mode: mode,
}, nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 18 - 25, StackParameters currently
duplicates Provider/Mode/Region as struct fields while getters/setters use
Variables causing stale state; remove the redundant Provider, Mode, and Region
fields from the StackParameters struct and update ParamsFromMap to stop
assigning those removed fields (rely on Variables entries instead), and ensure
GetProvider/GetMode/GetRegion and SetProvider/SetMode/SetRegion continue to
read/write the Variables map so all code uses the Variables canonical store
(also update any external code that referenced the removed struct fields to call
the getters).
| func (sp *StackParameters) GetProvider() client.ProviderID { | ||
| var providerID client.ProviderID | ||
| if val, ok := sp.Variables["DEFANG_PROVIDER"]; ok { | ||
| providerID.Set(val) | ||
| } | ||
| return providerID | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: GetProvider silently ignores invalid values.
Line 34 ignores the error returned by providerID.Set(val), returning the zero value if the stored provider string is invalid. This contrasts with ParamsFromMap (line 79), which returns an error for the same condition. While invalid values should be caught at creation time, silently ignoring errors at read time can mask configuration corruption or partial updates.
Consider logging a warning or debug message when an invalid provider is encountered in Variables.
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 31 - 37, GetProvider currently ignores
the error from providerID.Set(val) and returns a zero value on invalid data;
change GetProvider to capture the error returned by providerID.Set(val) and emit
a warning/debug log when Set fails (do not swallow the error silently). Update
the StackParameters.GetProvider implementation to check the error from
providerID.Set and call your project's logger (e.g., log.Printf or the existing
logger used elsewhere) with a clear message including the invalid value and
context (similar validation performed in ParamsFromMap), then return the
providerID as before.
| func (sp *StackParameters) GetMode() modes.Mode { | ||
| mode := modes.ModeUnspecified | ||
| if val, ok := sp.Variables["DEFANG_MODE"]; ok { | ||
| parsedMode, err := modes.Parse(val) | ||
| if err == nil { | ||
| mode = parsedMode | ||
| } | ||
| } | ||
| return properties | ||
| return mode | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: GetMode silently ignores invalid values.
Lines 59–62 suppress parsing errors from modes.Parse, defaulting to ModeUnspecified when the stored mode string is invalid. This mirrors the issue in GetProvider and contrasts with ParamsFromMap (line 86), which surfaces the error. Silent fallback can obscure configuration issues that arose after initial validation.
Consider logging a warning when an invalid mode is encountered to aid debugging.
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 56 - 65, GetMode currently swallows
parse errors and silently returns ModeUnspecified; update
StackParameters.GetMode to detect when modes.Parse(sp.Variables["DEFANG_MODE"])
returns an error and emit a warning log containing the invalid value and the
parse error (use the existing logger on StackParameters if present, e.g.
sp.Logger, or the package logger) before falling back to modes.ModeUnspecified
so misconfigurations are visible.
09744cf to
7ad1290
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @src/cmd/cli/command/compose.go:
- Line 92: The code calls CreateProjectForDebug on session.Loader, but Loader
doesn't define that method; update the two call sites (the Compose command where
CreateProjectForDebug is invoked) to use the existing Loader API or the correct
object: either add a CreateProjectForDebug(context.Context)
(*composeTypes.Project, error) method to the Loader interface, or replace the
calls with session.Loader.LoadProject(ctx) (and use
LoadProjectName/TargetDirectory as needed) or call the appropriate
compose.Loader object that actually implements CreateProjectForDebug; pick one
option and make the corresponding change consistently for both occurrences so
the code compiles.
In @src/cmd/cli/command/session.go:
- Around line 44-65: The ProviderID in the returned SessionLoaderOptions is
never populated from the --provider flag; prov is only set when projectName
accidentally parses as a provider. Read the provider flag (e.g. via
cmd.Flags().GetString("provider")) and if the flag was provided
(cmd.Flag("provider").Changed or non-empty string), call prov.Set(providerStr)
and handle any error, then assign that prov to SessionLoaderOptions.ProviderID
so the explicit --provider value is used instead of relying on projectName
heuristics.
In @src/pkg/session/session.go:
- Around line 151-159: The error check after calling sm.Create in session.go
incorrectly uses errors.Is with a newly allocated &stacks.ErrOutside{} which
will never match an ErrOutside value with fields; change this to use errors.As
to detect the error type: declare a variable like var errOutside
*stacks.ErrOutside and replace the condition errors.Is(err,
&stacks.ErrOutside{}) with !errors.As(err, &errOutside) so the code correctly
skips the specialized ErrOutside case when returned from sm.Create and still
wraps other errors.
In @src/pkg/stacks/manager.go:
- Around line 122-123: The code unconditionally sets variables[regionVarName]
using client.GetRegionVarName(providerID) which can return an empty string for
ProviderDefang/ProviderAuto; change the logic in the block around regionVarName
to skip adding to the variables map when regionVarName == "" or when
deployment.GetRegion() is empty: call client.GetRegionVarName(providerID), if it
returns a non-empty string then set variables[regionVarName] =
deployment.GetRegion() (optionally only when deployment.GetRegion() is
non-empty) so LoadParameters never receives an empty-string key.
In @src/pkg/stacks/stacks.go:
- Around line 27-29: SetProvider (and similarly SetRegion and SetMode) can panic
when sp.Variables is nil; update each setter (SetProvider, SetRegion, SetMode on
StackParameters) to guard and initialize the map before assignment by checking
if sp.Variables == nil and setting sp.Variables = make(map[string]string) when
needed, then assign the key (e.g., "DEFANG_PROVIDER") the value; ensure you use
the same nil-check/initialization pattern in all three setters.
- Around line 67-74: ToMap uses a value receiver but assigns sp.Variables to
vars and mutates it, which unintentionally modifies the original
StackParameters.Variables (and will panic if nil); update ToMap to avoid
mutating the original by creating a new map and copying entries from
sp.Variables (or initializing an empty map if nil) and then add/override keys
"DEFANG_PROVIDER", the region key from
client.GetRegionVarName(sp.GetProvider()), and "DEFANG_MODE" before returning
the new map; locate the ToMap method on StackParameters and adjust it to
copy/initialize the map rather than modifying sp.Variables in place.
- Around line 230-243: ReadInDirectory currently returns a StackParameters with
only Name and Variables set, leaving Provider/Mode/Region zeroed; update
ReadInDirectory to build the returned StackParameters by calling
ParamsFromMap(variables) (or the equivalent constructor) and then set Name
before returning so Provider, Mode, Region and other derived fields are
populated consistently; locate the ReadInDirectory function and replace the
direct literal return with a call to ParamsFromMap using the parsed variables,
attach Name = name, and return that populated struct.
- Around line 76-96: ParamsFromMap currently parses DEFANG_PROVIDER and
DEFANG_MODE but never sets the Region on the returned StackParameters; update
ParamsFromMap to read the region from the properties map (e.g. check
properties["DEFANG_REGION"]), assign it to a local region variable (or validate
if required) and include that value in the returned StackParameters.Region so
callers receive a populated Region field alongside Provider and Mode.
🧹 Nitpick comments (6)
src/pkg/agent/tools/create_aws_stack.go (1)
33-41: Consider filtering empty values from the Variables map.If
params.AWS_Profileis empty, an empty string will be stored in theVariablesmap forAWS_PROFILE. This could lead to unexpected behavior when the stack is marshaled or when environment variables are set from these parameters, potentially overwriting a validAWS_PROFILEenvironment variable with an empty value.♻️ Suggested improvement
newStack := stacks.StackParameters{ Name: params.Name, - Variables: map[string]string{ - "DEFANG_PROVIDER": client.ProviderAWS.String(), - "DEFANG_MODE": mode.String(), - "AWS_REGION": params.Region, - "AWS_PROFILE": params.AWS_Profile, - }, } +newStack.Variables = map[string]string{ + "DEFANG_PROVIDER": client.ProviderAWS.String(), + "DEFANG_MODE": mode.String(), + "AWS_REGION": params.Region, +} +if params.AWS_Profile != "" { + newStack.Variables["AWS_PROFILE"] = params.AWS_Profile +}src/pkg/session/session.go (2)
47-57: Error handling whentargetDirectorylookup fails butProjectNameis provided.When
findTargetDirectoryreturns an error butProjectNameis non-empty, the code proceeds with an emptytargetDirectory. This is likely intentional to allow operations outside project directories, but thetargetDirectoryvariable will be the empty string""rather than an explicit value. Consider documenting this behavior or using a sentinel value.
143-146: Useerrors.Isfor consistency with the rest of the file.For consistency with line 52 and 104 which use
errors.Is(err, os.ErrNotExist), consider using the same pattern here instead ofos.IsNotExist(err).♻️ Suggested fix
- if !os.IsNotExist(err) { + if !errors.Is(err, os.ErrNotExist) { return nil, err }src/pkg/agent/tools/provider.go (1)
45-48: Consider cachingGetProvider()result to avoid duplicate call.
stack.GetProvider()is called twice in succession. While functionally correct, caching the result in a local variable would be slightly cleaner.♻️ Optional improvement
term.Debug("Function invoked: cli.NewProvider") - provider := pp.pc.NewProvider(ctx, stack.GetProvider(), pp.fc, stack.Name) - providerID := stack.GetProvider() + providerID := stack.GetProvider() + provider := pp.pc.NewProvider(ctx, providerID, pp.fc, stack.Name) return &providerID, provider, nilsrc/cmd/cli/command/session.go (1)
36-42: Setting environment variable as a side effect may cause issues.
os.Setenv("DEFANG_SUFFIX", stackName)is a global side effect that persists beyond this function call. This could cause unexpected behavior if the function is called multiple times or in tests. Consider passing this value through the options struct instead.src/pkg/stacks/selector.go (1)
34-37: Unused struct field:AllowImport.The
AllowImportfield inSelectStackOptionsis defined but never used in theSelectStackWithOptionsimplementation. Consider removing it if not needed, or document that it's reserved for future use.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/cmd/cli/command/cd.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/session.gosrc/pkg/agent/tools/create_aws_stack.gosrc/pkg/agent/tools/create_gcp_stack.gosrc/pkg/agent/tools/provider.gosrc/pkg/mcp/mcp_server.gosrc/pkg/session/session.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/selector.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/wizard.go
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pkg/agent/tools/create_gcp_stack.go
- src/pkg/stacks/wizard.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/cmd/cli/command/globals.gosrc/cmd/cli/command/session.gosrc/cmd/cli/command/commands.gosrc/pkg/stacks/manager.gosrc/cmd/cli/command/compose.go
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/cmd/cli/command/globals.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/selector.gosrc/cmd/cli/command/session.gosrc/pkg/mcp/mcp_server.gosrc/pkg/session/session.gosrc/cmd/cli/command/commands.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/stacks.gosrc/cmd/cli/command/compose.gosrc/pkg/agent/tools/provider.gosrc/cmd/cli/command/cd.gosrc/pkg/agent/tools/create_aws_stack.go
📚 Learning: 2026-01-09T20:12:21.986Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:30-30
Timestamp: 2026-01-09T20:12:21.986Z
Learning: In Go files, recognize and accept the import path go.yaml.in/yaml/v3 as the maintained fork of the YAML library. Do not flag this import as incorrect; this fork supersedes the archived gopkg.in/yaml.v3 path. If you encounter this or similar forked import paths, treat them as valid Go imports and do not raise review flags.
Applied to files:
src/cmd/cli/command/globals.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/selector.gosrc/cmd/cli/command/session.gosrc/pkg/mcp/mcp_server.gosrc/pkg/session/session.gosrc/cmd/cli/command/commands.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/stacks.gosrc/cmd/cli/command/compose.gosrc/pkg/agent/tools/provider.gosrc/cmd/cli/command/cd.gosrc/pkg/agent/tools/create_aws_stack.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/pkg/stacks/stacks_test.go
📚 Learning: 2026-01-07T03:07:48.228Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:48.228Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/cmd/cli/command/compose.go
🧬 Code graph analysis (9)
src/cmd/cli/command/globals.go (7)
src/cmd/cli/command/color.go (2)
ColorAuto(11-11)ColorMode(5-5)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
ModeUnspecified(15-15)Mode(12-12)src/pkg/utils.go (2)
GetenvBool(56-59)Getenv(48-53)src/pkg/types/tenant.go (2)
TenantNameOrID(8-8)TenantNameOrID(31-33)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/region.go (2)
GetRegion(5-24)GetRegionVarName(26-41)
src/pkg/stacks/stacks_test.go (1)
src/pkg/stacks/stacks.go (3)
CreateInDirectory(114-158)ReadInDirectory(230-244)LoadInDirectory(246-254)
src/pkg/stacks/selector.go (1)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)
src/cmd/cli/command/session.go (3)
src/pkg/session/session.go (3)
Session(18-22)NewSessionLoader(39-45)SessionLoaderOptions(24-31)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/clouds/driver.go (1)
ProjectName(8-8)
src/pkg/stacks/manager.go (3)
src/pkg/stacks/stacks.go (6)
List(166-168)StackListItem(161-164)StackParameters(18-25)LoadParameters(268-287)Create(110-112)ReadInDirectory(230-244)src/pkg/term/colorizer.go (2)
Errorf(322-324)Infof(306-308)src/pkg/cli/client/region.go (2)
GetRegionVarName(26-41)GetRegion(5-24)
src/pkg/stacks/stacks.go (4)
src/pkg/cli/client/provider.go (1)
Provider(54-80)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)src/pkg/cli/client/region.go (2)
GetRegionVarName(26-41)GetRegion(5-24)
src/cmd/cli/command/compose.go (4)
src/cmd/cli/command/session.go (2)
NewSessionLoaderOptionsForCommand(20-66)NewCommandSession(13-18)src/pkg/session/session.go (1)
NewSessionLoader(39-45)src/pkg/cli/client/provider.go (2)
Loader(82-86)Provider(54-80)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)
src/pkg/agent/tools/provider.go (1)
src/pkg/cli/connect.go (1)
NewProvider(36-50)
src/cmd/cli/command/cd.go (4)
src/cmd/cli/command/session.go (1)
NewCommandSession(13-18)src/pkg/cli/client/projectName.go (1)
LoadProjectNameWithFallback(10-26)src/pkg/cli/client/provider.go (2)
Loader(82-86)Provider(54-80)src/pkg/cli/composeUp.go (1)
ComposeUpParams(23-27)
🪛 GitHub Actions: Go package
src/cmd/cli/command/compose.go
[error] 92-92: session.Loader.CreateProjectForDebug undefined (type "github.com/DefangLabs/defang/src/pkg/cli/client".Loader has no field or method CreateProjectForDebug)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
src/pkg/stacks/stacks_test.go (2)
302-327: LGTM!Test correctly updated to use directory-scoped
CreateInDirectoryandReadInDirectoryfunctions with"."as the working directory. The test name accurately reflects the function being tested.
330-380: LGTM!Tests for
LoadInDirectorycorrectly use the directory-scoped API and verify both the case where env vars are set and the case where existing env vars should not be overwritten.src/pkg/session/session.go (1)
162-173: LGTM!The fallback logic correctly requires
--providerwhen--stackis not specified in non-interactive mode, aligning with the PR objective to support non-interactive deployment with required inputs via flags.src/pkg/stacks/stacks.go (1)
160-164:StackListItemembedding pattern looks good.The change to embed
StackParameterswithinStackListItemis a clean refactor that maintains the same information while simplifying the struct.src/pkg/mcp/mcp_server.go (1)
84-89: LGTM!The change correctly uses the new
GetProvider()accessor pattern. Taking the address ofproviderIDis safe here since theToolTrackeris used within the server's lifetime managed by this function.src/cmd/cli/command/commands.go (1)
185-185: Verify deprecation message clarity.The deprecation message says "use '--stack' to select a stack instead", but this might be confusing since users may not understand the relationship between
--providerand--stack. Consider clarifying that stack selection implicitly includes provider information, or that users should create a stack with the desired provider and then use--stackto select it.Example improved message:
"deprecated; create a stack with your desired provider using 'defang stack create', then use '--stack' to select it"src/pkg/stacks/manager.go (10)
13-13: LGTM: Import addition supports fallback logging.The
termpackage is correctly imported to support the informational logging at line 153 when falling back from local to remote stack loading.
17-25: LGTM: Interface refactor supports context-aware and remote stack loading.The updated Manager interface correctly introduces context-aware loading, explicit local/remote methods, and StackParameters-based parameter handling, aligning with the PR's goal to support non-interactive deployment to remote stacks.
37-52: LGTM: Conditional path computation enables remote-only workflows.The refactored
NewManagercorrectly computes the absolute path only whentargetDirectoryis non-empty, allowing the manager to support remote-only stack operations without requiring a local directory.
54-56: LGTM: Straightforward accessor.The
TargetDirectorygetter is correctly implemented.
58-91: LGTM: Merge and sort logic is sound.The
Listmethod correctly merges local and remote stacks, preferring remote entries to show deployment times, and sorts the result by stack name for consistent output.
159-169: LGTM: LoadLocal correctly reads and applies local stack parameters.The implementation correctly reads the stack from the target directory and applies its parameters without overloading existing environment variables.
171-192: LGTM: LoadRemote correctly imports stack from remote deployments.The method lists remote stacks, finds the matching one by name, applies its parameters, and provides clear error messages. The linear search is acceptable for typical use cases.
194-196: LGTM: Delegation is correct.The method correctly delegates to the package-level
LoadParametersfunction.
198-203: LGTM: Create guard prevents operation without a target directory.The check correctly prevents stack creation when no local target directory is available, which aligns with the requirement that stack files must be written to a specific location.
150-157: The fallback behavior is correct and intentional. TheLoadmethod falls back toLoadRemoteon anyLoadLocalerror to support the PR objective of enabling remote-only stacks without requiring a local stackfile. This behavior is confirmed by tests (TestManager_LoadNonexistentStack,TestManager_TargetDirectoryEmpty) which explicitly verify that stacks can be loaded from remote deployments when local files are unavailable. Theterm.Infofmessage keeps users informed of the fallback. While this approach may mask local file corruption, this is an accepted design trade-off to enable the remote-first functionality.
|
|
||
| term.Error("Cannot load project:", loadErr) | ||
| project, err := loader.CreateProjectForDebug() | ||
| project, err := session.Loader.CreateProjectForDebug() |
There was a problem hiding this comment.
Critical compilation error: method does not exist.
The pipeline failure confirms that CreateProjectForDebug is not a method on the Loader interface. According to the interface definition in the relevant code snippets, Loader only provides:
LoadProject(context.Context) (*composeTypes.Project, error)LoadProjectName(context.Context) (string, error)TargetDirectory() string
This same issue occurs again at line 566.
🔍 Verification needed
Check if CreateProjectForDebug should be:
- Added to the
Loaderinterface - Called on a different object (e.g.,
compose.Loaderinstead ofclient.Loader) - Replaced with an alternative approach
#!/bin/bash
# Search for CreateProjectForDebug definition and usage
ast-grep --pattern 'func $_($$) CreateProjectForDebug($$$) $$$'
rg -n "CreateProjectForDebug" --type=go🧰 Tools
🪛 GitHub Actions: Go package
[error] 92-92: session.Loader.CreateProjectForDebug undefined (type "github.com/DefangLabs/defang/src/pkg/cli/client".Loader has no field or method CreateProjectForDebug)
🤖 Prompt for AI Agents
In @src/cmd/cli/command/compose.go at line 92, The code calls
CreateProjectForDebug on session.Loader, but Loader doesn't define that method;
update the two call sites (the Compose command where CreateProjectForDebug is
invoked) to use the existing Loader API or the correct object: either add a
CreateProjectForDebug(context.Context) (*composeTypes.Project, error) method to
the Loader interface, or replace the calls with session.Loader.LoadProject(ctx)
(and use LoadProjectName/TargetDirectory as needed) or call the appropriate
compose.Loader object that actually implements CreateProjectForDebug; pick one
option and make the corresponding change consistently for both occurrences so
the code compiles.
| // Avoid common mistakes | ||
| var prov client.ProviderID | ||
| if prov.Set(projectName) == nil && !cmd.Flag("provider").Changed { | ||
| // using -p with a provider name instead of -P | ||
| term.Warnf("Project name %q looks like a provider name; did you mean to use -P=%s instead of -p?", projectName, projectName) | ||
| doubleCheckProjectName(projectName) | ||
| } else if strings.HasPrefix(projectName, "roject-name") { | ||
| // -project-name= instead of --project-name | ||
| term.Warn("Did you mean to use --project-name instead of -project-name?") | ||
| doubleCheckProjectName(projectName) | ||
| } else if strings.HasPrefix(projectName, "rovider") { | ||
| // -provider= instead of --provider | ||
| term.Warn("Did you mean to use --provider instead of -provider?") | ||
| doubleCheckProjectName(projectName) | ||
| } | ||
| return session.SessionLoaderOptions{ | ||
| Stack: stack, | ||
| ProviderID: prov, | ||
| ComposeFilePaths: configPaths, | ||
| ProjectName: projectName, | ||
| Interactive: !global.NonInteractive, | ||
| } |
There was a problem hiding this comment.
ProviderID in options is not read from --provider flag.
The prov variable is only set when projectName accidentally looks like a provider name (line 46). The actual --provider flag value is never read, so SessionLoaderOptions.ProviderID will always be empty unless the user mistakenly uses -p=aws (project name flag) instead of -P=aws (provider flag).
This appears to be missing logic to read the provider from cmd.Flags().GetString("provider") or similar.
🐛 Suggested fix
func NewSessionLoaderOptionsForCommand(cmd *cobra.Command) session.SessionLoaderOptions {
+ providerStr, err := cmd.Flags().GetString("provider")
+ if err != nil {
+ panic(err)
+ }
+ var providerID client.ProviderID
+ if providerStr != "" {
+ if err := providerID.Set(providerStr); err != nil {
+ // Handle invalid provider - could warn or let session loader handle it
+ }
+ }
+
stack, err := cmd.Flags().GetString("stack")
// ... rest of function ...
- var prov client.ProviderID
- if prov.Set(projectName) == nil && !cmd.Flag("provider").Changed {
+ if providerID == "" && client.ProviderID("").Set(projectName) == nil && !cmd.Flag("provider").Changed {
// using -p with a provider name instead of -P
term.Warnf("Project name %q looks like a provider name; did you mean to use -P=%s instead of -p?", projectName, projectName)
doubleCheckProjectName(projectName)
}
// ...
return session.SessionLoaderOptions{
Stack: stack,
- ProviderID: prov,
+ ProviderID: providerID,
// ...
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/cmd/cli/command/session.go around lines 44 - 65, The ProviderID in the
returned SessionLoaderOptions is never populated from the --provider flag; prov
is only set when projectName accidentally parses as a provider. Read the
provider flag (e.g. via cmd.Flags().GetString("provider")) and if the flag was
provided (cmd.Flag("provider").Changed or non-empty string), call
prov.Set(providerStr) and handle any error, then assign that prov to
SessionLoaderOptions.ProviderID so the explicit --provider value is used instead
of relying on projectName heuristics.
| // persist the remote stack file to the local target directory | ||
| stackFilename, err := sm.Create(*stack) | ||
| if err != nil && !errors.Is(err, &stacks.ErrOutside{}) { | ||
| return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err) | ||
| } | ||
| if stackFilename != "" { | ||
| term.Infof("Stack %q loaded and saved to %q. Add this file to source control", name, stackFilename) | ||
| } | ||
| return stack, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the stacks package location
fd -t f 'stacks' -e go | head -20Repository: DefangLabs/defang
Length of output: 150
🏁 Script executed:
# Search for ErrOutside type definition
rg -n 'type ErrOutside' -A 10Repository: DefangLabs/defang
Length of output: 754
🏁 Script executed:
# Check if ErrOutside has an Is method
rg -n 'func.*ErrOutside.*Is\('Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
# Search for other uses of ErrOutside to see how it's typically checked
rg -n 'ErrOutside' -B 2 -A 2Repository: DefangLabs/defang
Length of output: 1449
🏁 Script executed:
# Check the specific file where the error is returned
cat -n src/pkg/stacks/manager.go | sed -n '135,155p'Repository: DefangLabs/defang
Length of output: 839
Use errors.As() instead of errors.Is() for type checking.
The current pattern errors.Is(err, &stacks.ErrOutside{}) attempts value equality, but ErrOutside has populated fields when returned (line 200 in manager.go). An empty struct instance will never equal the actual error, causing the check to fail silently. Use errors.As() instead to check the error type:
var errOutside *stacks.ErrOutside
if err != nil && !errors.As(err, &errOutside) {
return nil, fmt.Errorf("failed to save imported stack %q to local directory: %w", name, err)
}Alternatively, implement an Is() method on ErrOutside to define custom matching behavior.
🤖 Prompt for AI Agents
In @src/pkg/session/session.go around lines 151 - 159, The error check after
calling sm.Create in session.go incorrectly uses errors.Is with a newly
allocated &stacks.ErrOutside{} which will never match an ErrOutside value with
fields; change this to use errors.As to detect the error type: declare a
variable like var errOutside *stacks.ErrOutside and replace the condition
errors.Is(err, &stacks.ErrOutside{}) with !errors.As(err, &errOutside) so the
code correctly skips the specialized ErrOutside case when returned from
sm.Create and still wraps other errors.
| regionVarName := client.GetRegionVarName(providerID) | ||
| variables[regionVarName] = deployment.GetRegion() |
There was a problem hiding this comment.
Prevent setting an empty-string key in the variables map.
For ProviderDefang and ProviderAuto, client.GetRegionVarName returns an empty string. Setting variables[""] creates an invalid empty-string key in the map, which could cause issues during environment variable conversion in LoadParameters.
Based on the relevant code snippet from src/pkg/cli/client/region.go, GetRegionVarName returns "" for ProviderDefang and ProviderAuto.
🐛 Proposed fix to skip region setting for providers without a region variable
regionVarName := client.GetRegionVarName(providerID)
-variables[regionVarName] = deployment.GetRegion()
+if regionVarName != "" {
+ variables[regionVarName] = deployment.GetRegion()
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| regionVarName := client.GetRegionVarName(providerID) | |
| variables[regionVarName] = deployment.GetRegion() | |
| regionVarName := client.GetRegionVarName(providerID) | |
| if regionVarName != "" { | |
| variables[regionVarName] = deployment.GetRegion() | |
| } |
🤖 Prompt for AI Agents
In @src/pkg/stacks/manager.go around lines 122 - 123, The code unconditionally
sets variables[regionVarName] using client.GetRegionVarName(providerID) which
can return an empty string for ProviderDefang/ProviderAuto; change the logic in
the block around regionVarName to skip adding to the variables map when
regionVarName == "" or when deployment.GetRegion() is empty: call
client.GetRegionVarName(providerID), if it returns a non-empty string then set
variables[regionVarName] = deployment.GetRegion() (optionally only when
deployment.GetRegion() is non-empty) so LoadParameters never receives an
empty-string key.
| func (sp *StackParameters) SetProvider(provider client.ProviderID) { | ||
| sp.Variables["DEFANG_PROVIDER"] = provider.String() | ||
| } |
There was a problem hiding this comment.
Nil map panic risk in SetProvider.
If sp.Variables is nil, calling SetProvider will panic. The same applies to SetRegion and SetMode. Consider initializing the map if nil.
🐛 Proposed fix for all setters
func (sp *StackParameters) SetProvider(provider client.ProviderID) {
+ if sp.Variables == nil {
+ sp.Variables = make(map[string]string)
+ }
sp.Variables["DEFANG_PROVIDER"] = provider.String()
}Apply the same pattern to SetRegion and SetMode.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sp *StackParameters) SetProvider(provider client.ProviderID) { | |
| sp.Variables["DEFANG_PROVIDER"] = provider.String() | |
| } | |
| func (sp *StackParameters) SetProvider(provider client.ProviderID) { | |
| if sp.Variables == nil { | |
| sp.Variables = make(map[string]string) | |
| } | |
| sp.Variables["DEFANG_PROVIDER"] = provider.String() | |
| } |
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 27 - 29, SetProvider (and similarly
SetRegion and SetMode) can panic when sp.Variables is nil; update each setter
(SetProvider, SetRegion, SetMode on StackParameters) to guard and initialize the
map before assignment by checking if sp.Variables == nil and setting
sp.Variables = make(map[string]string) when needed, then assign the key (e.g.,
"DEFANG_PROVIDER") the value; ensure you use the same nil-check/initialization
pattern in all three setters.
| func (sp StackParameters) ToMap() map[string]string { | ||
| vars := sp.Variables | ||
| vars["DEFANG_PROVIDER"] = sp.GetProvider().String() | ||
| regionVarName := client.GetRegionVarName(sp.GetProvider()) | ||
| vars[regionVarName] = sp.GetRegion(sp.GetProvider()) | ||
| vars["DEFANG_MODE"] = sp.GetMode().String() | ||
| return vars | ||
| } |
There was a problem hiding this comment.
Value receiver mutates the original struct's Variables map.
ToMap() uses a value receiver but assigns sp.Variables to vars and then modifies it. Since maps are reference types in Go, this mutates the original StackParameters.Variables map. Additionally, if sp.Variables is nil, this will panic.
🐛 Proposed fix
func (sp StackParameters) ToMap() map[string]string {
- vars := sp.Variables
+ vars := make(map[string]string)
+ for k, v := range sp.Variables {
+ vars[k] = v
+ }
vars["DEFANG_PROVIDER"] = sp.GetProvider().String()
regionVarName := client.GetRegionVarName(sp.GetProvider())
vars[regionVarName] = sp.GetRegion(sp.GetProvider())
vars["DEFANG_MODE"] = sp.GetMode().String()
return vars
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sp StackParameters) ToMap() map[string]string { | |
| vars := sp.Variables | |
| vars["DEFANG_PROVIDER"] = sp.GetProvider().String() | |
| regionVarName := client.GetRegionVarName(sp.GetProvider()) | |
| vars[regionVarName] = sp.GetRegion(sp.GetProvider()) | |
| vars["DEFANG_MODE"] = sp.GetMode().String() | |
| return vars | |
| } | |
| func (sp StackParameters) ToMap() map[string]string { | |
| vars := make(map[string]string) | |
| for k, v := range sp.Variables { | |
| vars[k] = v | |
| } | |
| vars["DEFANG_PROVIDER"] = sp.GetProvider().String() | |
| regionVarName := client.GetRegionVarName(sp.GetProvider()) | |
| vars[regionVarName] = sp.GetRegion(sp.GetProvider()) | |
| vars["DEFANG_MODE"] = sp.GetMode().String() | |
| return vars | |
| } |
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 67 - 74, ToMap uses a value receiver
but assigns sp.Variables to vars and mutates it, which unintentionally modifies
the original StackParameters.Variables (and will panic if nil); update ToMap to
avoid mutating the original by creating a new map and copying entries from
sp.Variables (or initializing an empty map if nil) and then add/override keys
"DEFANG_PROVIDER", the region key from
client.GetRegionVarName(sp.GetProvider()), and "DEFANG_MODE" before returning
the new map; locate the ToMap method on StackParameters and adjust it to
copy/initialize the map rather than modifying sp.Variables in place.
| func ParamsFromMap(properties map[string]string) (StackParameters, error) { | ||
| var params StackParameters | ||
| for key, value := range properties { | ||
| switch key { | ||
| case "DEFANG_PROVIDER": | ||
| if err := params.Provider.Set(value); err != nil { | ||
| return params, err | ||
| } | ||
| case "AWS_REGION": | ||
| params.Region = value | ||
| case "GCP_LOCATION": | ||
| params.Region = value | ||
| case "AWS_PROFILE": | ||
| params.AWSProfile = value | ||
| case "GCP_PROJECT_ID": | ||
| params.GCPProjectID = value | ||
| case "DEFANG_MODE": | ||
| mode, err := modes.Parse(value) | ||
| if err != nil { | ||
| return params, err | ||
| } | ||
| params.Mode = mode | ||
| var provider client.ProviderID | ||
| if val, ok := properties["DEFANG_PROVIDER"]; ok { | ||
| err := provider.Set(val) | ||
| if err != nil { | ||
| return StackParameters{}, fmt.Errorf("invalid DEFANG_PROVIDER value %q: %w", val, err) | ||
| } | ||
| } | ||
| return params, nil | ||
| var mode modes.Mode | ||
| if val, ok := properties["DEFANG_MODE"]; ok { | ||
| err := mode.Set(val) | ||
| if err != nil { | ||
| return StackParameters{}, fmt.Errorf("invalid DEFANG_MODE value %q: %w", val, err) | ||
| } | ||
| } | ||
| return StackParameters{ | ||
| Variables: properties, | ||
| Provider: provider, | ||
| Mode: mode, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
ParamsFromMap does not populate the Region field.
ParamsFromMap sets Provider and Mode fields on the returned StackParameters, but does not set the Region field. This is inconsistent with the struct definition which includes a Region field. Callers may expect all typed fields to be populated when converting from a map.
💡 Suggested fix
func ParamsFromMap(properties map[string]string) (StackParameters, error) {
var provider client.ProviderID
if val, ok := properties["DEFANG_PROVIDER"]; ok {
err := provider.Set(val)
if err != nil {
return StackParameters{}, fmt.Errorf("invalid DEFANG_PROVIDER value %q: %w", val, err)
}
}
var mode modes.Mode
if val, ok := properties["DEFANG_MODE"]; ok {
err := mode.Set(val)
if err != nil {
return StackParameters{}, fmt.Errorf("invalid DEFANG_MODE value %q: %w", val, err)
}
}
+ regionVarName := client.GetRegionVarName(provider)
+ region := properties[regionVarName]
return StackParameters{
Variables: properties,
Provider: provider,
Mode: mode,
+ Region: region,
}, nil
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 76 - 96, ParamsFromMap currently
parses DEFANG_PROVIDER and DEFANG_MODE but never sets the Region on the returned
StackParameters; update ParamsFromMap to read the region from the properties map
(e.g. check properties["DEFANG_REGION"]), assign it to a local region variable
(or validate if required) and include that value in the returned
StackParameters.Region so callers receive a populated Region field alongside
Provider and Mode.
| func ReadInDirectory(workingDirectory, name string) (*StackParameters, error) { | ||
| path := filename(workingDirectory, name) | ||
| content, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not read stack %q from %q: %w", name, path, err) | ||
| } | ||
| parsed, err := Parse(string(content)) | ||
| variables, err := Parse(string(content)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| parsed.Name = name | ||
| return &parsed, nil | ||
| } | ||
|
|
||
| func Load(name string) error { | ||
| return LoadInDirectory(".", name) | ||
| return &StackParameters{ | ||
| Name: name, | ||
| Variables: variables, | ||
| }, nil |
There was a problem hiding this comment.
ReadInDirectory returns StackParameters with only Name and Variables populated.
The returned StackParameters has Provider, Mode, and Region fields at their zero values. Callers that access these fields directly (e.g., params.Provider) instead of using getters (e.g., params.GetProvider()) will get unexpected results. Consider using ParamsFromMap to ensure all fields are consistently populated.
💡 Suggested fix
func ReadInDirectory(workingDirectory, name string) (*StackParameters, error) {
path := filename(workingDirectory, name)
content, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("could not read stack %q from %q: %w", name, path, err)
}
variables, err := Parse(string(content))
if err != nil {
return nil, err
}
- return &StackParameters{
- Name: name,
- Variables: variables,
- }, nil
+ params, err := ParamsFromMap(variables)
+ if err != nil {
+ return nil, err
+ }
+ params.Name = name
+ return ¶ms, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func ReadInDirectory(workingDirectory, name string) (*StackParameters, error) { | |
| path := filename(workingDirectory, name) | |
| content, err := os.ReadFile(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not read stack %q from %q: %w", name, path, err) | |
| } | |
| parsed, err := Parse(string(content)) | |
| variables, err := Parse(string(content)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| parsed.Name = name | |
| return &parsed, nil | |
| } | |
| func Load(name string) error { | |
| return LoadInDirectory(".", name) | |
| return &StackParameters{ | |
| Name: name, | |
| Variables: variables, | |
| }, nil | |
| func ReadInDirectory(workingDirectory, name string) (*StackParameters, error) { | |
| path := filename(workingDirectory, name) | |
| content, err := os.ReadFile(path) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not read stack %q from %q: %w", name, path, err) | |
| } | |
| variables, err := Parse(string(content)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| params, err := ParamsFromMap(variables) | |
| if err != nil { | |
| return nil, err | |
| } | |
| params.Name = name | |
| return ¶ms, nil | |
| } |
🤖 Prompt for AI Agents
In @src/pkg/stacks/stacks.go around lines 230 - 243, ReadInDirectory currently
returns a StackParameters with only Name and Variables set, leaving
Provider/Mode/Region zeroed; update ReadInDirectory to build the returned
StackParameters by calling ParamsFromMap(variables) (or the equivalent
constructor) and then set Name before returning so Provider, Mode, Region and
other derived fields are populated consistently; locate the ReadInDirectory
function and replace the direct literal return with a call to ParamsFromMap
using the parsed variables, attach Name = name, and return that populated
struct.
Description
Users should be able to deploy to new stacks in CI if all of the necessary information is provider through flags or env vars. For example, using the following command:
This PR makes the following changes to support this:
--providerflag--stackfailed to find a stack file—we will no longer require a stackfile, we can reference a remote stack by name.Linked Issues
Checklist
Summary by CodeRabbit
Chores
New Features
Bug Fixes / Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.