Stacks onboarding with "remote" stacks #1721
Conversation
WalkthroughIntroduces stack-driven provider resolution: new stacks.Manager, stack selector/wizard, and elicitations Controller integration; adds getStack/importStack and updates newProvider to accept (ctx, ec, sm); rewires CLI, agent tools, loader/provider interfaces, many tests, and stack parameter serialization/loading. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Elicit as Elicitations
participant StackMgr as StacksManager
participant Selector as StackSelector
participant ProviderFactory as ProviderFactory
User->>CLI: run command (e.g., compose up / estimate)
CLI->>StackMgr: NewManager(fabricClient, targetDir, projectName)
CLI->>Elicit: create controller
CLI->>CLI: getStack(ctx, ec, sm)
alt stack determinable (flag/env/file)
CLI-->>CLI: return StackParameters, whence
else interactive
CLI->>Selector: SelectStack(ctx)
Selector->>StackMgr: List(ctx)
Selector->>Elicit: RequestEnum(stackOptions)
User->>Elicit: choose existing or "Create new stack"
alt create new
Selector->>Elicit: collect wizard params
Selector->>StackMgr: Create(params)
Selector->>StackMgr: Load(createdName)
else choose existing
Selector->>StackMgr: Load(selectedName)
end
Selector-->>CLI: selected StackParameters
end
CLI->>ProviderFactory: newProvider(ctx, ec, sm)
ProviderFactory-->>CLI: Provider (uses provider.GetStackName())
CLI->>ProviderFactory: proceed with operation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" Comment |
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
2031da8 to
c1f97a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/agent/tools/estimate.go (1)
23-26: Duplicate error wrapping on line 24-25.The error is wrapped twice with the same message. Line 24 assigns the wrapped error to
err(which is then unused), and line 25 wraps it again.if err != nil { - err = fmt.Errorf("failed to parse compose file: %w", err) return "", fmt.Errorf("failed to parse compose file: %w", err) }
🧹 Nitpick comments (8)
src/pkg/stacks/selector.go (2)
30-32: Clarify error message terminology.The error message references "mcp client," which may not be immediately clear to all users. Consider whether this is the correct user-facing terminology or if a more general term would be appropriate.
- return nil, errors.New("your mcp client does not support elicitations, use the 'select_stack' tool to choose a stack") + return nil, errors.New("interactive prompts are not supported in this environment; use the 'select_stack' tool to choose a stack")
61-63: Consider informing users when auto-creating their first stack.When no stacks exist, the code automatically returns
CreateNewStackwithout showing any message to the user. Consider adding a brief informational message so users understand they're creating their first stack.if len(stackList) == 0 { + term.Info("No stacks found. You'll be guided through creating your first stack.\n") return CreateNewStack, nil }src/pkg/agent/tools/provider.go (1)
38-45: Variable shadowing:erris redeclared with:=instead of assigned with=.On line 40,
erris declared with:=which shadows theerrdeclared on line 37. While this works correctly since the error is checked immediately, it's inconsistent with line 47 which uses=for assignment.if stack.Name == "" { selector := stacks.NewSelector(pp.ec, pp.sm) - newStack, err := selector.SelectStack(ctx) + var newStack *stacks.StackParameters + newStack, err = selector.SelectStack(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to setup stack: %w", err) } *stack = *newStack }src/pkg/stacks/selector_test.go (2)
120-123: Consider movingWizardInterfaceto production code.Defining
WizardInterfacein the test file limits its reusability and makes it invisible to production code. If this interface is needed for dependency injection in the productionstackSelector, it should be defined inselector.goalongside the selector implementation.
142-196: Test double duplicates production logic.The
testableStackSelectorduplicates theSelectStackandelicitStackSelectionlogic from production code. If the productionstackSelectorimplementation changes, these test methods may drift out of sync, leading to passing tests but broken production code.Consider either:
- Exposing a
WithWizardoption on the production selector for testing- Using composition to inject only the wizard while reusing production logic
src/cmd/cli/command/commands_test.go (1)
250-274: Mock implementation uses panic for invalid inputs.The
mockElicitationsClient.Requestmethod panics on invalid schema properties (line 257-258) and multi-field requests (line 264-266). While this is acceptable for controlled test scenarios, it could make debugging test failures harder.Consider returning an error instead of panicking for better test diagnostics:
func (m *mockElicitationsClient) Request(ctx context.Context, req elicitations.Request) (elicitations.Response, error) { properties, ok := req.Schema["properties"].(map[string]any) if !ok || len(properties) == 0 { - panic("invalid schema properties") + return elicitations.Response{}, fmt.Errorf("invalid schema properties") } fields := make([]string, 0) for field := range properties { fields = append(fields, field) } if len(fields) > 1 { - panic("mockElicitationsClient only supports single-field requests") + return elicitations.Response{}, fmt.Errorf("mockElicitationsClient only supports single-field requests") }src/pkg/stacks/manager.go (1)
73-113: Consider handlingOutsideErrorfromListLocalgracefully inList().When
ListLocal()returns anOutsideError(because the working directory differs from the target directory), theList()method currently propagates this error. However, remote stacks should still be listable even when outside the working directory. Consider handlingOutsideErroras a non-fatal case where you proceed with only remote stacks.func (sm *manager) List(ctx context.Context) ([]StackListItem, error) { remoteStacks, err := sm.ListRemote(ctx) if err != nil { return nil, fmt.Errorf("failed to list remote stacks: %w", err) } localStacks, err := sm.ListLocal() if err != nil { - return nil, fmt.Errorf("failed to list local stacks: %w", err) + // If we're outside the working directory, just use remote stacks + var outsideErr *OutsideError + if !errors.As(err, &outsideErr) { + return nil, fmt.Errorf("failed to list local stacks: %w", err) + } + localStacks = nil // No local stacks available when outside } // Merge remote and local stacks into a single list of type StackOption,You'll also need to add
"errors"to the imports.src/cmd/cli/command/commands.go (1)
1342-1354: Consider handling the case whenSetSelectedProviderfails more gracefully.Currently, if
SetSelectedProviderfails, only a debug message is logged (line 1350), but the success message on line 1352 is still printed. This could be confusing since it prints that the provider "is now the default" even though the save failed.func saveSelectedProvider(ctx context.Context, projectName string, providerID cliClient.ProviderID) { if projectName == "" { return } if err := global.Client.SetSelectedProvider(ctx, &defangv1.SetSelectedProviderRequest{ Project: projectName, Provider: providerID.Value(), }); err != nil { term.Debugf("Unable to save selected provider to defang server: %v", err) - } else { - term.Printf("%v is now the default provider for project %v and will auto-select next time if no other provider is specified. Use --provider=auto to reselect.", global.Stack.Provider, projectName) + return } + term.Printf("%v is now the default provider for project %v and will auto-select next time if no other provider is specified. Use --provider=auto to reselect.", global.Stack.Provider, projectName) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(15 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/estimate.go(1 hunks)src/cmd/cli/command/globals.go(2 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(2 hunks)src/pkg/agent/tools/deploy_test.go(1 hunks)src/pkg/agent/tools/destroy.go(2 hunks)src/pkg/agent/tools/destroy_test.go(1 hunks)src/pkg/agent/tools/estimate.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/logs.go(3 hunks)src/pkg/agent/tools/provider.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/cli/client/byoc/baseclient.go(1 hunks)src/pkg/cli/client/caniuse.go(2 hunks)src/pkg/cli/client/client.go(0 hunks)src/pkg/cli/client/mock.go(1 hunks)src/pkg/cli/client/playground.go(1 hunks)src/pkg/cli/client/provider.go(1 hunks)src/pkg/cli/compose/loader.go(1 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/manager_test.go(12 hunks)src/pkg/stacks/selector.go(1 hunks)src/pkg/stacks/selector_test.go(1 hunks)src/pkg/stacks/stacks.go(2 hunks)src/testdata/sanity/.defang/beta(1 hunks)
💤 Files with no reviewable changes (1)
- src/pkg/cli/client/client.go
🧰 Additional context used
🧬 Code graph analysis (22)
src/pkg/cli/compose/loader.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/clouds/driver.go (1)
ProjectName(8-8)
src/pkg/cli/client/playground.go (1)
src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/provider.go (1)
src/pkg/stacks/selector.go (1)
NewSelector(22-27)
src/pkg/agent/tools/estimate.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)
src/pkg/agent/tools/interfaces.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/destroy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/setConfig.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/pkg/agent/tools/deploy_test.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-24)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/cmd/cli/command/estimate.go (1)
src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderAWS(15-15)ProviderGCP(17-17)
src/pkg/agent/tools/logs.go (5)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(19-22)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-17)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/listConfig.go (4)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/pkg/agent/tools/removeConfig.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/pkg/agent/tools/destroy.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/pkg/agent/tools/default_tool_cli.go (1)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/cmd/cli/command/commands_test.go (4)
src/pkg/elicitations/elicitations.go (3)
Request(25-28)Response(30-33)NewController(35-40)src/pkg/stacks/manager.go (1)
Manager(14-18)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderDO(16-16)ProviderGCP(17-17)src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)
src/pkg/cli/client/caniuse.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(11-38)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/deploy.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/services.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/cmd/cli/command/globals.go (3)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)
src/cmd/cli/command/commands.go (11)
src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (3)
NewController(35-40)Client(16-18)Controller(8-14)src/pkg/stacks/manager.go (2)
NewManager(32-71)Manager(14-18)src/pkg/stacks/stacks.go (2)
StackParameters(18-25)Load(227-229)src/pkg/cli/client/provider.go (2)
Provider(43-70)Loader(72-76)src/protos/io/defang/v1/fabric.pb.go (10)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)GetSelectedProviderRequest(631-636)GetSelectedProviderRequest(649-649)GetSelectedProviderRequest(664-666)SetSelectedProviderRequest(736-742)SetSelectedProviderRequest(755-755)SetSelectedProviderRequest(770-772)src/pkg/cli/client/provider_id.go (3)
ProviderAuto(13-13)ProviderDefang(14-14)ProviderID(10-10)src/pkg/stacks/selector.go (1)
NewSelector(22-27)src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/cmd/cli/command/compose.go (4)
src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/pkg/stacks/manager.go (1)
src/pkg/stacks/stacks.go (9)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)ListInDirectory(101-135)ReadInDirectory(213-225)LoadInDirectory(231-239)CreateInDirectory(40-84)
🪛 GitHub Actions: Go package
src/cmd/cli/command/commands.go
[error] 1408-1408: Panic: runtime error: invalid memory address or nil pointer dereference
🔇 Additional comments (42)
src/testdata/sanity/.defang/beta (1)
1-2: Test data file format is correct and properly structured.The key=value format matches the stack file parsing implementation. Godotenv loads environment variables from .env files, and the codebase's
Parse()function usesgodotenv.Parse()to handle the format. The test file correctly uses standard environment variable names (DEFANG_PROVIDER,AWS_REGION) that the stack loader expects, and the file location at.defang/betafollows the established naming convention. This test data properly supports stack loading scenarios.src/pkg/stacks/selector.go (1)
84-88: LGTM! Clear onboarding message for beta stack.The informational message about the implicit "beta" stack effectively guides users through the migration to explicit stacks. The conditional message when only the beta stack exists is particularly helpful.
src/pkg/cli/client/mock.go (1)
175-177: LGTM! Mock implementation is consistent.The
OutsideWorkingDirectory()method correctly returnsfalsefor the mock loader, which is appropriate for test scenarios.src/cmd/cli/command/estimate.go (1)
30-38: LGTM! Provider selection flow is consistent.The interactive provider selection correctly updates
global.Stack.Providerand aligns with the new stack-centric provider resolution flow.src/pkg/cli/client/provider.go (1)
69-69: LGTM! Interface extensions support stack-aware operations.The new
GetStackName()method onProviderandOutsideWorkingDirectory()onLoadercleanly enable stack-aware provider initialization and directory-context checks across the codebase.Also applies to: 75-75
src/pkg/cli/client/playground.go (1)
21-21: LGTM! Default stack name provides backward compatibility.The
GetStackName()method returns "beta" as the default whenPulumiStackis empty, which aligns with the PR's goal of surfacing the implicit "beta" stack for existing projects.Also applies to: 26-31
src/pkg/cli/client/byoc/baseclient.go (1)
74-76: LGTM! Stack name accessor is consistent.The
GetStackName()method provides a clean accessor for the Pulumi stack name, consistent with thePlaygroundProviderimplementation.src/pkg/stacks/stacks.go (1)
94-94: InitializeDeployedAtfield inListInDirectoryfunction.The
DeployedAtfield inStackListItemstruct is not explicitly initialized inListInDirectory(lines 126-131). While local stacks have no deployment information, the field should be explicitly set totime.Time{}for consistency with how local stacks are handled inmanager.goline 93, rather than relying on implicit zero value initialization.src/pkg/cli/client/caniuse.go (1)
10-23: LGTM!The signature change correctly centralizes stack name retrieval via
provider.GetStackName(), eliminating the need for callers to pass the stack name separately. This aligns well with the stack-centric refactor and reduces the chance of inconsistency between the provider's internal stack and the passed parameter.src/pkg/agent/tools/removeConfig.go (2)
21-21: LGTM on signature update.The change from
ProjectLoadertoLoaderaligns with the broader refactor to use the unifiedLoaderinterface.
28-31: No action needed. The code correctly handles an optionalparams.ProjectName. The stack manager can be created with an empty project name, and the actual project name is properly loaded viaLoadProjectNameWithFallbackbefore being used in operations. This pattern is consistent with other parts of the codebase (e.g., "list --remote" uses empty project names).src/pkg/agent/tools/estimate.go (1)
20-20: Signature change aligns with the Loader interface migration.The parameter type change from
cliClient.ProjectLoadertocliClient.Loaderis consistent with the broader refactoring across the codebase.src/pkg/agent/tools/destroy_test.go (1)
60-66: Mock signature correctly updated to match the interface.The
CanIUseProvidersignature change (removingstackNameparameter) aligns with the updatedCLIInterfaceininterfaces.go. The stack name is now retrieved viaprovider.GetStackName()in the actual implementation.src/pkg/agent/tools/listConfig.go (2)
20-20: Signature change consistent with the loader migration.The update from
cliClient.ProjectLoadertocliClient.Loaderaligns with the repository-wide interface consolidation.
27-31: Stack manager initialization with proper error handling.The updated
stacks.NewManagercall correctly passes the client, working directory, and project name. Error handling is appropriately propagated.src/cmd/cli/command/globals.go (2)
98-98: Explicit field initialization improves clarity.Adding the explicit
Name: ""initialization makes the default state clear and aligns with the updatedStackParametersstruct.
257-261: Useerrors.Is(err, os.ErrNotExist)instead ofos.IsNotExistto properly detect missing files.os.IsNotExist does not unwrap wrapped errors. If
godotenv.Readwraps the underlying file error, the current check will fail to detect missing files. Use errors.Is(err, os.ErrNotExist) instead, which properly unwraps and checks the error chain.// Current code (unreliable with wrapped errors) if os.IsNotExist(err) { return nil } // Recommended fix if errors.Is(err, os.ErrNotExist) { return nil }src/pkg/agent/tools/setConfig.go (2)
21-21: Signature change aligns with the Loader migration.The update from
cliClient.ProjectLoadertocliClient.Loaderis consistent with the other tool handlers in this PR.
28-31: Stack manager initialization follows established pattern.The
stacks.NewManagercall and error handling are consistent with other tool handlers likelistConfig.goanddestroy.go.src/pkg/agent/tools/destroy.go (3)
20-20: Signature change consistent with the Loader migration.The parameter type change from
cliClient.ProjectLoadertocliClient.Loaderaligns with the broader interface consolidation.
27-35: Stack manager and provider setup follow established patterns.The
stacks.NewManagerinitialization with error handling, followed byNewProviderPreparerandSetupProvider, is consistent with other tool handlers in this PR.
42-42:CanIUseProvidercall updated to match new interface.The
stackNameparameter has been removed from the call, aligning with the updatedCLIInterfacewhere the stack name is now retrieved internally viaprovider.GetStackName().src/pkg/stacks/selector_test.go (2)
50-72: Good nil-safe mock implementation.The
MockStacksManager.ListandMockStacksManager.Loadmethods correctly handle nil returns with type assertions, preventing panics when tests return nil values. This is a robust pattern for testify mocks.
79-118: Comprehensive test coverage for stack selection.The test suite thoroughly covers:
- Existing stack selection
- New stack creation via wizard
- Auto-create when no stacks exist
- Error handling for all failure points (elicitations disabled, list errors, elicitation errors, load errors, wizard errors, create errors)
The use of
AssertExpectationsandAssertNotCalledensures mock interactions are verified.src/cmd/cli/command/commands_test.go (3)
276-295: Mock implementation looks correct for test scenarios.The
mockStacksManagerimplements the requiredList,Load, andCreatemethods. The embeddedstacks.Managerinterface will cause panics if unimplemented methods are called, which is acceptable for these specific tests.
377-397: Test correctly exercises interactive stack/provider selection.The test properly sets up mocks for the elicitations client and stacks manager, then verifies that
newProvideruses them for interactive selection and saves the selected provider.
220-226: Good defensive nil check for map initialization.The nil check before map assignment prevents panics when
savedProviderhasn't been initialized, improving test reliability.src/pkg/agent/tools/services.go (2)
30-34: Stack manager creation with proper error handling.The two-step pattern (create manager, then provider preparer) with explicit error handling is clean and aligns with the updated
NewManagersignature that now returns an error.
23-23: Signature aligns with Loader interface refactor.The change from
ProjectLoadertoLoaderis consistent with the interface simplification across the codebase. TheLoaderinterface provides the necessary methods (LoadProject,LoadProjectName,OutsideWorkingDirectory).src/pkg/agent/tools/default_tool_cli.go (1)
30-32: SimplifiedCanIUseProvidersignature removes redundant parameter.The
stackNameparameter was removed becausecliClient.CanIUseProvidernow retrieves the stack name directly fromprovider.GetStackName(). This reduces parameter duplication and keeps stack information encapsulated within the provider.src/pkg/agent/tools/logs.go (3)
48-56: Consistent stack manager and provider setup pattern.The stack manager creation and provider setup follows the same pattern as other tools (services.go), maintaining consistency across the codebase.
65-68:CanIUseProvidercall uses updated interface signature.The call correctly uses the simplified signature without the
stackNameparameter. TheDefaultToolCLI.CanIUseProviderwrapper handles the parameter reordering to match the underlyingcliClient.CanIUseProviderfunction.
25-25: Signature aligns with Loader interface refactor.Consistent with the changes in
services.goand other tools.src/cmd/cli/command/compose.go (2)
223-252: LGTM - handleExistingDeployments correctly handles stack name parameter.The updated filtering logic properly compares
dep.ProviderwithaccountInfo.Provider.Value()and correctly defaults to "beta" when stackName is empty, aligning with the PR objective to surface the implicit "beta" stack for existing projects.
566-577: LGTM - Provider construction properly integrated with elicitations and stack manager.The new flow correctly wires up the elicitation client, controller, and stack manager before calling
newProvider. This aligns with the stack-centric provider resolution changes.src/pkg/agent/tools/deploy.go (1)
47-57: LGTM - Stack manager integration and error handling look correct.The stack manager is properly created with error handling, and the
CanIUseProvidercall uses the updated signature correctly.src/pkg/stacks/manager.go (1)
127-164: LGTM - ListRemote correctly handles empty stack names and duplicate deployments.The logic properly defaults empty stack names to "beta" (line 139) and correctly avoids overwriting existing entries since deployments are sorted by deployed_at desc (lines 143-144).
src/pkg/stacks/manager_test.go (3)
668-675: Test expectsList()to fail when directories differ - verify this is intended behavior.This test expects
List()to fail when the working directory differs from the target directory. However, ifList()should gracefully fall back to only remote stacks whenListLocal()fails withOutsideError, this test would need to be updated accordingly. Please verify the intended behavior.
18-31: LGTM - Mock client properly implements DeploymentLister interface.The mock correctly returns configured deployments or an error for testing purposes.
340-391: LGTM - Comprehensive tests for ListRemote functionality.Good coverage of the remote stack listing including timestamp verification.
src/cmd/cli/command/commands.go (2)
1258-1325: LGTM - Stack resolution logic with proper precedence.The
getStackfunction correctly implements the provider/stack precedence rules:
- Stack file (via
--stackflag)- Command line flag (
--provider)- Environment variable (
DEFANG_PROVIDER)- Previously used provider (stored preference)
- Interactive selection
The fallback to
cliClient.ProviderDefangin non-interactive mode is appropriate.
531-546: LGTM - whoami command properly integrates with new provider flow.The whoami command correctly creates the elicitation client, controller, and stack manager before calling
newProvider.
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
4bdccea to
d0e373f
Compare
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 (2)
src/pkg/stacks/manager_test.go (2)
253-268: Test may not verify the intended behavior.This test doesn't call
t.Chdir(tmpDir), so the manager will haveoutside = trueandLoadwill return anOutsideErrorrather than a "stack not found" error. If the intent is to test loading a nonexistent stack file, you need to change to the target directory first.func TestManager_LoadNonexistentStack(t *testing.T) { // Create a temporary directory for testing tmpDir := t.TempDir() + // Change to temp directory so working directory matches target directory + t.Chdir(tmpDir) + mockClient := &mockFabricClient{} manager, err := NewManager(mockClient, tmpDir, "test-project")
270-305: Test may not verify the intended validation behavior.Similar to
TestManager_LoadNonexistentStack, this test is missingt.Chdir(tmpDir). TheCreatemethod will returnOutsideErrorbefore reaching the stack name validation logic.func TestManager_CreateInvalidStackName(t *testing.T) { // Create a temporary directory for testing tmpDir := t.TempDir() + // Change to temp directory so working directory matches target directory + t.Chdir(tmpDir) + mockClient := &mockFabricClient{} manager, err := NewManager(mockClient, tmpDir, "test-project")
♻️ Duplicate comments (2)
src/pkg/agent/tools/interfaces.go (1)
15-15: LGTM! Parameter order now matches implementation.The updated signature resolves the inconsistency flagged in previous reviews. The interface now correctly matches the actual
CanIUseProviderimplementation insrc/pkg/cli/client/caniuse.go, withproviderbeforeprojectNameandstackNameremoved.src/pkg/agent/tools/deploy_test.go (1)
80-83: LGTM! Mock signature updated consistently.The mock now correctly implements the updated
CanIUseProviderinterface with the proper parameter order.
🧹 Nitpick comments (6)
src/pkg/stacks/selector_test.go (1)
120-196: Consider movingWizardInterfaceandtestableStackSelectorto production code or use a different testing approach.The
testableStackSelectorduplicates the entireSelectStackandelicitStackSelectionlogic from the productionstackSelector. This creates maintenance burden since any changes to the production code must be manually synchronized to the test harness.Consider one of these alternatives:
- Move
WizardInterfacetoselector.goand make wizard injection a first-class feature ofstackSelector- Use a functional option pattern to inject the wizard during construction
- Test the production
stackSelectordirectly by mocking only the external dependenciesExample refactor for option 1 in
selector.go:type WizardInterface interface { CollectParameters(ctx context.Context) (*StackParameters, error) } type stackSelector struct { ec elicitations.Controller sm Manager wizard WizardInterface } func NewSelector(ec elicitations.Controller, sm Manager, wizard WizardInterface) *stackSelector { return &stackSelector{ ec: ec, sm: sm, wizard: wizard, } }src/pkg/stacks/manager.go (1)
108-112: Stack list order is non-deterministic due to map iteration.The returned slice order depends on Go's random map iteration order. If consistent ordering is desired for UI display (e.g., alphabetical or by deployment time), consider sorting the result.
stackList := make([]StackListItem, 0, len(stackMap)) for _, stack := range stackMap { stackList = append(stackList, stack) } + // Sort by name for consistent ordering + sort.Slice(stackList, func(i, j int) bool { + return stackList[i].Name < stackList[j].Name + }) return stackList, nilsrc/pkg/agent/tools/removeConfig.go (1)
28-31: Consider extracting stack manager creation into a helper.The stack manager creation pattern
stacks.NewManager(client, params.WorkingDirectory, params.ProjectName)with error handling is repeated across multiple tool files (removeConfig.go, listConfig.go, destroy.go). A shared helper function could reduce duplication and ensure consistency.Example helper:
func createStackManager(client cliClient.FabricClient, workingDirectory, projectName string) (stacks.Manager, error) { sm, err := stacks.NewManager(client, workingDirectory, projectName) if err != nil { return nil, fmt.Errorf("failed to create stack manager: %w", err) } return sm, nil }src/cmd/cli/command/commands_test.go (1)
265-289:mockElicitationsClientimplementation has intentional single-field limitation.The panic on multi-field requests is acceptable for focused unit tests. Consider adding a brief comment explaining this is a test limitation.
type mockElicitationsClient struct { responses map[string]string } +// Request implements elicitations.Client for testing. +// Note: Only supports single-field schemas; panics otherwise. func (m *mockElicitationsClient) Request(ctx context.Context, req elicitations.Request) (elicitations.Response, error) {src/cmd/cli/command/commands.go (2)
1380-1393:getPreviouslyUsedProvidererror propagation may disrupt the flow.When
GetSelectedProviderfails (e.g., network issues), the error is returned and stops the entire provider selection. Consider whether a graceful fallback to interactive selection would be more user-friendly.func getPreviouslyUsedProvider(ctx context.Context, projectName string) (cliClient.ProviderID, error) { if projectName == "" { return cliClient.ProviderAuto, nil } resp, err := global.Client.GetSelectedProvider(ctx, &defangv1.GetSelectedProviderRequest{ Project: projectName, }) if err != nil { - return cliClient.ProviderAuto, fmt.Errorf("unable to get selected provider: %w", err) + term.Debugf("Unable to get selected provider: %v", err) + return cliClient.ProviderAuto, nil // Fall back to interactive selection } var providerID cliClient.ProviderID providerID.SetValue(resp.Provider) return providerID, nil }
1473-1488: Redundantstacks.NewManagercalls with different error handling paths.Both branches (lines 1475 and 1484) create a
NewManagerbut theerrvariable from theos.Getwd()call on line 1480 shadows the outererr, which is fine. However, the error fromNewManageron line 1484 shadowserrfrom line 1480, which is also fine but could be clearer.Consider extracting the common error-handling pattern:
elicitationsClient := elicitations.NewSurveyClient(os.Stdin, os.Stdout, os.Stderr) ec := elicitations.NewController(elicitationsClient) - var sm stacks.Manager - if outside { - sm, err = stacks.NewManager(global.Client, "", projectName) - if err != nil { - return nil, fmt.Errorf("failed to create stack manager: %w", err) - } - } else { - wd, err := os.Getwd() - if err != nil { - return nil, fmt.Errorf("failed to get working directory: %w", err) - } - sm, err = stacks.NewManager(global.Client, wd, projectName) - if err != nil { - return nil, fmt.Errorf("failed to create stack manager: %w", err) - } - } + targetDir := "" + if !outside { + targetDir, err = os.Getwd() + if err != nil { + return nil, fmt.Errorf("failed to get working directory: %w", err) + } + } + sm, err := stacks.NewManager(global.Client, targetDir, projectName) + if err != nil { + return nil, fmt.Errorf("failed to create stack manager: %w", err) + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(15 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/estimate.go(1 hunks)src/cmd/cli/command/globals.go(2 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(2 hunks)src/pkg/agent/tools/deploy_test.go(1 hunks)src/pkg/agent/tools/destroy.go(2 hunks)src/pkg/agent/tools/destroy_test.go(1 hunks)src/pkg/agent/tools/estimate.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/logs.go(3 hunks)src/pkg/agent/tools/provider.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/cli/client/byoc/baseclient.go(1 hunks)src/pkg/cli/client/caniuse.go(2 hunks)src/pkg/cli/client/client.go(0 hunks)src/pkg/cli/client/mock.go(1 hunks)src/pkg/cli/client/playground.go(1 hunks)src/pkg/cli/client/provider.go(1 hunks)src/pkg/cli/compose/loader.go(1 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/manager_test.go(12 hunks)src/pkg/stacks/selector.go(1 hunks)src/pkg/stacks/selector_test.go(1 hunks)src/pkg/stacks/stacks.go(2 hunks)src/testdata/sanity/.defang/beta(1 hunks)
💤 Files with no reviewable changes (1)
- src/pkg/cli/client/client.go
✅ Files skipped from review due to trivial changes (1)
- src/testdata/sanity/.defang/beta
🚧 Files skipped from review as they are similar to previous changes (11)
- src/pkg/cli/client/caniuse.go
- src/pkg/agent/tools/services.go
- src/pkg/agent/tools/estimate.go
- src/pkg/cli/client/mock.go
- src/cmd/cli/command/estimate.go
- src/pkg/agent/tools/setConfig.go
- src/pkg/stacks/selector.go
- src/pkg/cli/client/provider.go
- src/pkg/cli/compose/loader.go
- src/pkg/cli/client/playground.go
- src/pkg/stacks/stacks.go
🧰 Additional context used
🧬 Code graph analysis (15)
src/cmd/cli/command/globals.go (3)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)
src/pkg/agent/tools/deploy.go (6)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(19-22)src/pkg/mcp/mcp_server.go (1)
StackConfig(48-48)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/listConfig.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/pkg/agent/tools/provider.go (1)
src/pkg/stacks/selector.go (1)
NewSelector(22-27)
src/pkg/agent/tools/logs.go (2)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/cmd/cli/command/compose.go (4)
src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/stacks/manager.go (1)
NewManager(32-71)
src/pkg/stacks/manager.go (1)
src/pkg/stacks/stacks.go (9)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)ListInDirectory(101-135)ReadInDirectory(213-225)LoadInDirectory(231-239)CreateInDirectory(40-84)
src/pkg/agent/tools/deploy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/default_tool_cli.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/stacks/manager_test.go (4)
src/pkg/stacks/manager.go (1)
NewManager(32-71)src/pkg/stacks/stacks.go (5)
List(97-99)Load(227-229)StackParameters(18-25)StackListItem(87-95)Directory(29-29)src/protos/io/defang/v1/fabric.pb.go (2)
Provider_AWS(33-33)Provider_GCP(35-35)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)
src/cmd/cli/command/commands_test.go (8)
src/pkg/elicitations/elicitations.go (3)
Request(25-28)Response(30-33)NewController(35-40)src/pkg/stacks/manager.go (1)
Manager(14-18)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderDO(16-16)ProviderGCP(17-17)src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)src/pkg/clouds/aws/common.go (1)
Region(15-15)src/pkg/crun/common.go (1)
Region(12-12)src/pkg/cli/client/byoc/aws/byoc.go (1)
StsClient(57-57)src/pkg/clouds/gcp/account.go (1)
FindGoogleDefaultCredentials(16-16)
src/cmd/cli/command/commands.go (7)
src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (3)
NewController(35-40)Client(16-18)Controller(8-14)src/pkg/stacks/manager.go (2)
NewManager(32-71)Manager(14-18)src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/cli/client/provider_id.go (3)
ProviderAuto(13-13)ProviderDefang(14-14)ProviderID(10-10)src/pkg/stacks/selector.go (1)
NewSelector(22-27)src/pkg/cli/connect.go (1)
NewProvider(62-76)
src/pkg/agent/tools/interfaces.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/stacks/selector_test.go (7)
src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)src/pkg/stacks/selector.go (2)
CreateNewStack(15-15)NewSelector(22-27)src/pkg/agent/tools/provider.go (1)
CreateNewStack(13-13)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)src/pkg/modes/modes.go (3)
Mode(12-12)ModeBalanced(17-17)ModeAffordable(16-16)src/pkg/elicitations/elicitations.go (1)
Controller(8-14)src/pkg/stacks/manager.go (1)
Manager(14-18)
src/pkg/agent/tools/destroy_test.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/pkg/cli/client/provider.go (1)
Provider(43-70)
⏰ 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 (39)
src/pkg/stacks/selector_test.go (4)
79-118: LGTM!Well-structured test for the existing stack selection flow with proper mock setup and assertion of expectations.
198-293: LGTM!Good coverage of the stack creation flow, including the automatic creation path when no existing stacks are found.
295-497: LGTM!Comprehensive error path coverage including elicitations not supported, list failures, elicitation errors, load errors, wizard errors, and creation errors.
499-536: LGTM!Good use of subtests to verify both the multi-stack selection and empty stack list scenarios for
elicitStackSelection.src/pkg/stacks/manager.go (4)
115-120: LGTM!Clean implementation with proper access control via
OutsideError.
127-164: LGTM!Good handling of the implicit "beta" stack for backwards compatibility with existing deployments, and proper deduplication logic that preserves the most recent deployment's data.
166-173: LGTM!Well-designed custom error type that enables type-based error handling and provides clear context about the directory mismatch.
175-195: LGTM!Consistent access control pattern across
LoadandCreatemethods with proper delegation to directory-specific implementations.src/pkg/stacks/manager_test.go (3)
18-31: LGTM!Clean mock implementation of
DeploymentListerinterface for testing remote stack listing functionality.
340-584: LGTM!Comprehensive test coverage for remote stack listing including beta stack handling, duplicate deployment merging, and the merged local/remote list behavior.
586-721: LGTM!Thorough testing of the directory constraint behavior, verifying that local operations work when directories match, fail with
OutsideErrorwhen they differ, and that remote operations work regardless of directory state.src/pkg/cli/client/byoc/baseclient.go (2)
74-76: LGTM!Simple accessor method that exposes the
PulumiStackvalue, aligning with the broader stack-name propagation pattern introduced in this PR.
116-122: Remove this comment—the concern is based on incorrect premise.
GetProjectDomainis newly added code in the latest commit, not a modification of existing logic. The method never included stack-name prefixing; the simple domain normalization is the intended design. For delegated/custom domains, infrastructure stack names should not be embedded.Likely an incorrect or invalid review comment.
src/cmd/cli/command/globals.go (1)
98-98: LGTM!Initialization of the
Namefield inStackParametersaligns with the struct definition instacks.go.src/pkg/agent/tools/provider.go (1)
38-45: LGTM! Clean refactor to selector-based stack selection.The new flow properly delegates stack selection to the stacks.Selector when no stack name is provided, simplifying the provider preparer's responsibilities.
src/pkg/agent/tools/destroy_test.go (1)
60-66: LGTM! Mock signature aligned with interface.The mock correctly implements the updated
CanIUseProvidersignature.src/pkg/agent/tools/listConfig.go (1)
27-31: LGTM! Consistent with the stack-based initialization refactor.The updated stack manager creation aligns with the broader PR changes. Note: This pattern is duplicated in removeConfig.go and destroy.go - see comment in removeConfig.go about a potential helper function.
src/pkg/agent/tools/destroy.go (1)
42-45: LGTM! Provider validation call updated correctly.The
CanIUseProvidercall now uses the updated signature with the correct parameter order.src/cmd/cli/command/compose.go (3)
223-229: LGTM! Improved deployment location tracking.The updated signature parameterizes the stack name, and the provider consistency check at line 225-227 properly validates that deployments match both provider and location (account/region).
237-251: LGTM! Effective onboarding for implicit "beta" stack.The automatic defaulting to "beta" and stack creation aligns well with the PR objectives to surface the implicit "beta" stack for existing projects. This provides a smooth onboarding path when deploying to a new location.
566-577: LGTM! Provider construction updated to new flow.The refactored provider construction properly instantiates the elicitations controller and stack manager before calling
newProvider, consistent with the stack-centric provider resolution introduced across the PR.src/pkg/agent/tools/default_tool_cli.go (1)
30-32: Signature update aligns with the new stack-centric provider flow.The removal of
stackNameparameter is correct sinceCanIUseProvidernow retrieves the stack name viaprovider.GetStackName()internally, as shown in thecaniuse.gosnippet whereStack: provider.GetStackName()is used in the request.src/pkg/agent/tools/logs.go (3)
25-25: Interface type change tocliClient.Loaderis consistent with the refactored loader interface.The
Loaderinterface providesLoadProject,LoadProjectName, andOutsideWorkingDirectorymethods, which aligns with the new stack-aware flow.
48-51: Stack manager initialization with proper error handling.The updated
NewManagersignature correctly passesclient,params.WorkingDirectory, andparams.ProjectName. Error handling is appropriately added.
65-65: CanIUseProvider call updated to new signature.The parameter order
(ctx, client, provider, projectName, 0)correctly matches the updated interface.src/pkg/agent/tools/deploy.go (3)
25-25: Interface type change is consistent with the refactor.
47-50: Stack manager initialization matches the new pattern.
47-57: Potential inconsistency:params.ProjectNamevsproject.Name.The stack manager is created with
params.ProjectName(line 47), butCanIUseProviderusesproject.Namefrom the loaded compose file (line 57). If these differ, the stack context and the CanIUse check may reference different projects.Consider verifying that
params.ProjectNameandproject.Nameare expected to be consistent, or whether the stack manager should be created after loading the project to use the authoritative project name:+ // Note: project.Name from compose may override params.ProjectName + // Ensure consistency for stack manager and CanIUseProvidersrc/cmd/cli/command/commands_test.go (5)
236-241: Defensive initialization ofsavedProvidermap.Good addition to prevent nil map panics when
SetSelectedProvideris called before any provider is saved.
243-247: NewListDeploymentsmock supports the stack manager's remote listing functionality.
291-310:mockStacksManagerimplementation correctly satisfies thestacks.Managerinterface.The
Loadmethod returns aStackParameterswith theexpectedProvider, enabling provider-specific test scenarios.
339-339: UpdatednewProvidercall signature with nil elicitations controller and stack manager.When
ecandsmare nil, the function should fall back to non-interactive defaults (playground provider). EnsurenewProviderhandles nil gracefully.
513-514: ClearingDIGITALOCEAN_TOKENand manually settingglobal.Stack.Providerfor test isolation.These additions ensure environment variables from previous tests don't interfere. The manual
global.Stack.Providerassignment simulates environment variable reflection.Also applies to: 534-535, 555-556, 588-589
src/cmd/cli/command/commands.go (6)
558-573:whoamiCmdcorrectly initializes stack manager with working directory and project name.The flow properly handles project name loading failures with a warning and continues execution.
1311-1378:getStackimplements clear precedence for stack/provider resolution.The precedence (stack flag → provider flag → env var → stored preference → interactive) is well-documented through the
whencevariable. The flow correctly returns early when a definitive provider is determined.
1395-1407:saveSelectedProvidersilently ignores errors, which is appropriate for a best-effort save.The debug log provides visibility without disrupting the user flow.
1460-1495: Nil loader handling addresses the previous review comment.The fix correctly checks
loader != nilbefore callingloader.LoadProjectNameandloader.OutsideWorkingDirectory, preventing the nil pointer dereference that caused the pipeline failure.
1497-1499:canIUseProviderupdated to the new signature without stackName.
1501-1537:interactiveSelectProvidernow returnscliClient.ProviderIDfor type safety.The conversion from string to
ProviderIDviaSetensures validation.
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
d0e373f to
0e9dac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pkg/stacks/manager.go (1)
57-62: Shadowed error variable in symlink resolution.The short variable declaration on line 57 creates a new
errin the if-block scope. Line 58 then reuses this shadowederr, which can cause confusion. While the current code works, restructuring would improve clarity.Consider the refactor suggested in the past review:
// For target directory, only resolve symlinks if the path exists resolvedTargetDirectory := absTargetDirectory if _, err := os.Stat(absTargetDirectory); err == nil { - resolvedTargetDirectory, err = filepath.EvalSymlinks(absTargetDirectory) + resolved, evalErr := filepath.EvalSymlinks(absTargetDirectory) + if evalErr != nil { + return nil, fmt.Errorf("failed to resolve symlinks in target directory: %w", evalErr) + } + resolvedTargetDirectory = resolved - if err != nil { - return nil, fmt.Errorf("failed to resolve symlinks in target directory: %w", err) - } }
🧹 Nitpick comments (4)
src/pkg/agent/tools/deploy_test.go (2)
54-57: Consider returning a mock Provider instead of nil.Returning
nilfromNewProvidermakes the tests fragile. IfHandleDeployToolcalls any methods on the provider (e.g.,GetStackName(),AccountInfo()), the tests would panic. Consider creating a minimal mock Provider that implements theclient.Providerinterface with no-op or simple stub methods to make tests more robust and realistic.Example approach:
type MockProvider struct { client.Provider stackName string } func (p *MockProvider) GetStackName() string { return p.stackName } func (p *MockProvider) AccountInfo(ctx context.Context) (*client.AccountInfo, error) { return &client.AccountInfo{}, nil } // Add other methods as needed with no-op implementations func (m *MockDeployCLI) NewProvider(ctx context.Context, providerId client.ProviderID, client client.FabricClient, stack string) client.Provider { m.CallLog = append(m.CallLog, fmt.Sprintf("NewProvider(%s)", providerId)) return &MockProvider{stackName: stack} }
200-209: Verify call log for both successful test cases.The call log verification only runs for
"successful_deploy_defang_provider"but not for"successful_deploy_aws_provider". Consider verifying the expected call sequence for both successful cases to ensure consistent behavior across providers.Example:
// For successful cases, verify CLI methods were called in order -if tt.expectedError == "" && tt.name == "successful_deploy_defang_provider" { +if tt.expectedError == "" && (tt.name == "successful_deploy_defang_provider" || tt.name == "successful_deploy_aws_provider") { expectedCalls := []string{ "LoadProject", "Connect(test-cluster)", "NewProvider(aws)", "CanIUseProvider", "ComposeUp", } assert.Equal(t, expectedCalls, mockCLI.CallLog) }src/cmd/cli/command/commands_test.go (1)
271-295: Consider returning errors instead of panicking in test mocks.While panics work for catching test setup issues, returning errors would provide clearer test failure messages. However, this is acceptable for test code since it fails fast on misconfiguration.
func (m *mockElicitationsClient) Request(ctx context.Context, req elicitations.Request) (elicitations.Response, error) { properties, ok := req.Schema["properties"].(map[string]any) if !ok || len(properties) == 0 { - panic("invalid schema properties") + return elicitations.Response{}, errors.New("invalid schema properties in test mock") } fields := make([]string, 0) for field := range properties { fields = append(fields, field) } if len(fields) > 1 { - panic("mockElicitationsClient only supports single-field requests") + return elicitations.Response{}, errors.New("mockElicitationsClient only supports single-field requests") }src/cmd/cli/command/commands.go (1)
1398-1411: Consider graceful degradation when fetching previously used provider fails.If the RPC call to
GetSelectedProviderfails (e.g., network issues), the error propagates up and blocks provider selection. Consider logging a warning and returningProviderAutoto allow fallback to interactive selection.func getPreviouslyUsedProvider(ctx context.Context, projectName string) (cliClient.ProviderID, error) { if projectName == "" { return cliClient.ProviderAuto, nil } resp, err := global.Client.GetSelectedProvider(ctx, &defangv1.GetSelectedProviderRequest{ Project: projectName, }) if err != nil { - return cliClient.ProviderAuto, fmt.Errorf("unable to get selected provider: %w", err) + term.Debugf("Unable to get previously selected provider: %v", err) + return cliClient.ProviderAuto, nil } var providerID cliClient.ProviderID providerID.SetValue(resp.Provider) return providerID, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(16 hunks)src/cmd/cli/command/globals.go(1 hunks)src/cmd/cli/command/workspace_test.go(2 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(2 hunks)src/pkg/agent/tools/deploy_test.go(1 hunks)src/pkg/agent/tools/destroy.go(2 hunks)src/pkg/agent/tools/destroy_test.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/logs.go(3 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/manager_test.go(12 hunks)src/pkg/stacks/selector.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pkg/agent/tools/interfaces.go
- src/cmd/cli/command/globals.go
🧰 Additional context used
🧬 Code graph analysis (10)
src/pkg/agent/tools/default_tool_cli.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/destroy_test.go (3)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/stacks/manager_test.go (2)
src/pkg/stacks/manager.go (1)
NewManager(33-72)src/pkg/stacks/stacks.go (6)
List(97-99)Create(36-38)Load(227-229)StackParameters(18-25)StackListItem(87-95)Directory(29-29)
src/pkg/stacks/selector.go (4)
src/pkg/elicitations/elicitations.go (1)
Controller(8-14)src/pkg/stacks/manager.go (1)
Manager(15-19)src/pkg/stacks/stacks.go (4)
StackParameters(18-25)Create(36-38)Load(227-229)List(97-99)src/pkg/stacks/wizard.go (1)
NewWizard(20-25)
src/pkg/agent/tools/deploy.go (5)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(33-72)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/logs.go (3)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(33-72)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/cmd/cli/command/commands_test.go (5)
src/pkg/elicitations/elicitations.go (3)
Request(25-28)Response(30-33)NewController(35-40)src/protos/io/defang/v1/fabric.pb.go (13)
ListDeploymentsRequest(2992-2999)ListDeploymentsRequest(3012-3012)ListDeploymentsRequest(3027-3029)ListDeploymentsResponse(3052-3057)ListDeploymentsResponse(3070-3070)ListDeploymentsResponse(3085-3087)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)src/pkg/stacks/manager.go (1)
Manager(15-19)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderDO(16-16)ProviderGCP(17-17)src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)
src/pkg/stacks/manager.go (8)
src/pkg/stacks/stacks.go (9)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)ListInDirectory(101-135)ReadInDirectory(213-225)LoadInDirectory(231-239)CreateInDirectory(40-84)src/pkg/term/colorizer.go (2)
Errorf(322-324)Error(318-320)src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/cli/compose/loader.go (1)
Project(25-25)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/timeutils/timeutils.go (1)
AsTime(39-44)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
src/pkg/agent/tools/deploy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/destroy.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(33-72)
⏰ 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 (24)
src/pkg/agent/tools/deploy_test.go (1)
80-83: Past issue resolved: parameter order now matches implementation.The mock signature now correctly matches the real
CanIUseProviderimplementation with parameters in the order(ctx, client, provider, projectName, serviceCount). The previous parameter order mismatch has been fixed.src/cmd/cli/command/workspace_test.go (1)
55-56: LGTM: Test isolation improved.Resetting
global.Stack.Nameprevents unintended stack file loading during tests, ensuring deterministic test behavior.Also applies to: 90-91
src/pkg/agent/tools/default_tool_cli.go (1)
30-31: LGTM: Signature updated to align with interface changes.The
stackNameparameter is no longer needed since the provider now exposesGetStackName(). The updated signature matches the new interface definition.src/pkg/agent/tools/destroy_test.go (1)
60-60: LGTM: Test mock updated to match interface.The mock signature correctly reflects the updated
CanIUseProviderinterface.src/pkg/agent/tools/destroy.go (1)
20-42: LGTM: Stack manager integration and error handling.The loader type change, stack manager creation with error handling, and updated provider validation signature are all consistent with the broader refactoring. Error propagation is properly handled with contextual wrapping.
src/pkg/agent/tools/logs.go (1)
25-65: LGTM: Consistent refactoring applied.The loader type change, stack manager creation, and provider validation updates mirror the pattern in other tool handlers. Error handling is appropriate.
src/pkg/stacks/selector.go (1)
29-53: LGTM: Stack selection flow is well-structured.The logic correctly handles both creating new stacks via the wizard and loading existing stacks. Error handling is appropriate throughout the flow.
src/pkg/agent/tools/deploy.go (1)
25-57: LGTM: Consistent stack manager integration.The loader type change, stack manager creation, and provider validation updates follow the established pattern across tool handlers. Error handling and propagation are appropriate.
src/pkg/stacks/manager.go (3)
74-117: LGTM: Local and remote stack merging logic.The
Listmethod correctly merges local and remote stacks, preferring remote data (includingDeployedAttimestamps) when both exist. Error handling properly distinguishesOutsideErrorfrom other failures.
131-168: LGTM: Remote deployment listing with implicit beta stack handling.The
ListRemotemethod correctly treats deployments with empty stack names as the implicit "beta" stack, aligning with the PR's onboarding objectives. The deduplication logic (preferring the most recent deployment per stack) is sound.
170-199: LGTM: Outside directory protection.The
OutsideErrortype and its usage inLoadandCreatemethods correctly prevent cross-project stack file access, addressing the PR objective of restricting operations when the working directory differs from the target directory.src/pkg/stacks/manager_test.go (5)
18-31: LGTM! Clean mock implementation.The
mockFabricClientcorrectly implements theDeploymentListerinterface with configurable deployments and error injection for testing error paths.
46-133: LGTM! Comprehensive CRUD test coverage.The test thoroughly validates stack creation, listing, and loading operations with appropriate assertions for all stack parameters including provider, region, and mode.
411-509: LGTM! Well-structured merge behavior test.The test correctly validates that remote stacks take precedence over local stacks when names match, while both local-only and remote-only stacks are properly included in the merged result.
640-716: LGTM! Important security boundary test.This test correctly validates the directory isolation behavior that prevents cross-project stack file access, ensuring Create and Load fail with appropriate error messages while List gracefully falls back to remote-only stacks.
33-44: Go version compatibility is already satisfied in go.mod.The project specifies
go 1.24.1, which fully supports thet.Chdir()method used throughout the test file. No action required.src/cmd/cli/command/commands_test.go (4)
85-89: LGTM! Required interface method added.The
ListDeploymentsstub correctly returns an empty deployment list, enabling tests to work with the new stacks manager that depends on this method.
241-247: Good defensive fix for nil map panic.The nil check before map assignment prevents a panic when
SetSelectedProvideris called before any provider has been saved. This aligns with Go best practices for map initialization.
297-316: LGTM! Appropriate mock implementation.The
mockStacksManagercorrectly implements thestacks.Managerinterface with configurableexpectedProviderfor testing different provider scenarios.
340-355: LGTM! Non-interactive path correctly tested.Passing
nilfor the elicitations controller correctly tests the non-interactive code path where no user prompts are expected.src/cmd/cli/command/commands.go (4)
558-573: LGTM! Proper stack manager initialization in whoami command.The whoami command correctly initializes the elicitations controller and stack manager before calling
newProvider, following the new stack-based provider resolution pattern.
1478-1513: LGTM! Nil loader check properly implemented.The function now correctly handles the case when
loaderis nil by defaultingoutsidetotrueand skipping theLoadProjectNamecall, addressing the previously identified nil pointer dereference issue.
1427-1456: LGTM! Helpful user experience improvement.The warning messages help users identify common configuration mismatches between selected provider and environment variables, preventing deployment issues.
1519-1556: LGTM! Improved type safety.Changing the return type to
cliClient.ProviderIDprovides better type safety and consistency with the rest of the codebase.
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
0e9dac8 to
c9c661b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cmd/cli/command/commands_test.go (2)
334-359: MissingAWS_REGIONenvironment variable causing test failure.The pipeline failure indicates this test fails with "missing AWS region". Unlike
TestCommandGates(line 151), this test doesn't set theAWS_REGIONenvironment variable before creating an AWS provider.Apply this diff to fix the test:
t.Run("Should set cd image from canIUse response", func(t *testing.T) { t.Chdir("../../../../src/testdata/sanity") t.Setenv("DEFANG_STACK", "beta") + t.Setenv("AWS_REGION", "us-west-2") // Set up RootCmd with required flags for getStack function
378-394: MissingAWS_REGIONenvironment variable causing test failure.Same issue as the previous test—this test also fails with "missing AWS region" and needs the environment variable set.
Apply this diff:
t.Run("Can override cd image from environment variable", func(t *testing.T) { t.Chdir("../../../../src/testdata/sanity") t.Setenv("DEFANG_STACK", "beta") + t.Setenv("AWS_REGION", "us-west-2") sts := aws.StsClient
🧹 Nitpick comments (5)
src/cmd/cli/command/stack_test.go (2)
56-108: Test duplicates production command logic.The inline
stackListCmddefinition duplicates most of the logic frommakeStackListCmd()instack.go. This creates maintenance burden and risks test/production divergence.Consider refactoring to use the actual
makeStackListCmd()function with any necessary test setup, or extract the shared logic into a testable helper:- // Create stackListCmd with manual RunE to avoid configureLoader call during test - stackListCmd := &cobra.Command{ - Use: "list", - ... - } + // Use the actual command + stackListCmd := makeStackListCmd()If
configureLoaderis the issue, consider making it injectable or using a test-specific configuration.
140-142: Consider testing with non-zero DeployedAt values.The test uses zero-value
DeployedAttimestamps. Consider adding a test case with actual deployment timestamps to verify the column formatting for realistic scenarios.src/cmd/cli/command/commands.go (1)
1406-1419: Warning messages reference deprecated flags.The warnings mention
--providerandDEFANG_PROVIDER, but line 1351 indicates these are being deprecated in favor of--stack. Consider updating the warning messages for consistency.if provider == cliClient.ProviderDefang { if awsInEnv() { - term.Warn("AWS environment variables were detected; did you forget --provider=aws or DEFANG_PROVIDER=aws?") + term.Warn("AWS environment variables were detected; did you intend to use an AWS stack?") } if doInEnv() { - term.Warn("DIGITALOCEAN_TOKEN environment variable was detected; did you forget --provider=digitalocean or DEFANG_PROVIDER=digitalocean?") + term.Warn("DIGITALOCEAN_TOKEN environment variable was detected; did you intend to use a DigitalOcean stack?") } if gcpInEnv() { - term.Warn("GCP_PROJECT_ID/CLOUDSDK_CORE_PROJECT environment variable was detected; did you forget --provider=gcp or DEFANG_PROVIDER=gcp?") + term.Warn("GCP_PROJECT_ID/CLOUDSDK_CORE_PROJECT environment variable was detected; did you intend to use a GCP stack?") } }src/cmd/cli/command/commands_test.go (2)
273-292: Consider consolidating or renaming duplicate mock types.Two mock implementations of
stacks.Managerexist:mockStacksManager(line 273) andmockStackManager(line 445). The names differ by only one letter ("Stacks" vs "Stack"), which is confusing. Consider either consolidating into a single configurable mock or renaming for clarity (e.g.,simpleStackManagerMockvsconfigurableStackManagerMock).Also applies to: 445-453
735-786: Remove dead code or implement warning capture.The
outputbuffer is declared but never used to capture output. The warning check at lines 778-784 doesn't actually verify warning output—it only checks if a flag was changed, which is already ensured by the test setup. Either remove this dead code or implement proper warning capture.- // Capture output to check for warnings - var output bytes.Buffer - // Call the function under test stack, whence, err := getStack(ctx, ec, sm) ... - // Check warning expectations - if tc.expectWarning { - // Since we can't easily capture term.Warn output in tests, we just verify - // that the code path that would produce warnings was taken - if tc.providerFlag != "" && !testRootCmd.PersistentFlags().Changed("provider") { - t.Error("expected provider flag to be marked as changed for warning path") - } - } - - _ = output // Suppress unused variable warning for now })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/cmd/cli/command/commands.go(4 hunks)src/cmd/cli/command/commands_test.go(10 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/estimate.go(3 hunks)src/cmd/cli/command/stack.go(2 hunks)src/cmd/cli/command/stack_test.go(3 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/selector.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/stacks/selector.go
🧰 Additional context used
🧬 Code graph analysis (6)
src/cmd/cli/command/stack_test.go (5)
src/pkg/cli/compose/loader.go (1)
NewLoader(57-72)src/pkg/stacks/manager.go (1)
NewManager(33-59)src/pkg/stacks/stacks.go (1)
List(97-99)src/pkg/term/colorizer.go (2)
Infof(306-308)src/pkg/term/table.go (1)
Table(11-13)
src/cmd/cli/command/estimate.go (5)
src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderGCP(17-17)ProviderDO(16-16)src/pkg/track/track.go (1)
Evt(37-52)src/cmd/cli/command/commands.go (1)
P(54-54)src/pkg/cli/tail.go (1)
P(93-93)src/pkg/debug/debug.go (1)
P(21-21)
src/pkg/stacks/manager.go (4)
src/pkg/stacks/stacks.go (4)
List(97-99)StackListItem(87-95)StackParameters(18-25)ListInDirectory(101-135)src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
src/cmd/cli/command/commands_test.go (4)
src/protos/io/defang/v1/fabric.pb.go (13)
ListDeploymentsRequest(2992-2999)ListDeploymentsRequest(3012-3012)ListDeploymentsRequest(3027-3029)ListDeploymentsResponse(3052-3057)ListDeploymentsResponse(3070-3070)ListDeploymentsResponse(3085-3087)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)src/pkg/stacks/manager.go (1)
Manager(15-19)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)
src/cmd/cli/command/stack.go (3)
src/pkg/stacks/manager.go (1)
NewManager(33-59)src/pkg/stacks/stacks.go (1)
List(97-99)src/pkg/term/table.go (1)
Table(11-13)
src/cmd/cli/command/compose.go (4)
src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/stacks/manager.go (1)
NewManager(33-59)
🪛 GitHub Actions: Go package
src/cmd/cli/command/stack_test.go
[error] 1-1: TestNewProvider/Should_set_cd_image_from_canIUse_response failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
[error] 1-1: TestNewProvider/Can_override_cd_image_from_environment_variable failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
src/cmd/cli/command/estimate.go
[error] 1-1: TestNewProvider/Should_set_cd_image_from_canIUse_response failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
[error] 1-1: TestNewProvider/Can_override_cd_image_from_environment_variable failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
src/cmd/cli/command/commands_test.go
[error] 1-1: TestNewProviderShould_set_cd_image_from_canIUse_response failed: missing AWS region: set AWS_REGION or edit your AWS profile; also expected cd image tag to be site/registry/repo:tag@sha256:digest but got an empty value.
[error] 1-1: TestNewProviderCan_override_cd_image_from_environment_variable failed: expected cd image tag to be site/registry/repo:tag@sha256:digest but got an empty value.
src/cmd/cli/command/stack.go
[error] 1-1: TestNewProvider/Should_set_cd_image_from_canIUse_response failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
[error] 1-1: TestNewProvider/Can_override_cd_image_from_environment_variable failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
src/cmd/cli/command/commands.go
[error] 1-1: TestNewProvider/Should_set_cd_image_from_canIUse_response failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
[error] 1-1: TestNewProvider/Can_override_cd_image_from_environment_variable failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
src/cmd/cli/command/compose.go
[error] 1-1: TestNewProvider/Should_set_cd_image_from_canIUse_response failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
[error] 1-1: TestNewProvider/Can_override_cd_image_from_environment_variable failed: CanIUseProvider() failed: missing AWS region: set AWS_REGION or edit your AWS profile at ~/.aws/config (https://docs.defang.io/docs/providers/aws#region); Expected cd image tag to be site/registry/repo:tag@sha256:digest, got:
⏰ 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 (18)
src/cmd/cli/command/estimate.go (1)
69-106: LGTM on the new interactive provider selection helper.The function is well-structured with appropriate error handling, survey integration, and tracking. The panic for invalid input is acceptable as a programming contract violation.
src/cmd/cli/command/commands.go (3)
558-573: LGTM on whoami stack-aware provider flow.The integration of elicitations controller and stack manager follows the new stack-driven pattern consistently with other commands.
1372-1386: LGTM on single stack auto-selection logic.The auto-selection when exactly one stack matches the specified provider is a good UX improvement. The provider validation before assignment is appropriate.
1457-1492: LGTM on newProviderChecked with nil safety.The nil check for
loaderis properly implemented, addressing the previous review concern. The stack manager creation correctly handles both inside and outside working directory cases.src/cmd/cli/command/stack.go (3)
90-105: LGTM on manager-based stack listing.The refactoring to use
stacks.NewManagerwith proper context propagation and project name resolution follows the new architecture well.
97-100: Strict error handling for project name loading.This returns an error if
LoadProjectNamefails, which is stricter than silently falling back to an empty project name. This is the correct production behavior.Note: The test in
stack_test.gohandles this differently (swallows the error) - see earlier comment.
130-131: LGTM on DeployedAt column addition.The new column provides useful deployment metadata to users when listing stacks.
src/cmd/cli/command/compose.go (3)
223-230: Good addition of provider consistency check.The new check at lines 225-227 ensures deployments are from the same cloud provider before considering them as "same place". This prevents confusion when switching between providers.
237-250: LGTM on "beta" stack creation for legacy deployments.This correctly implements the onboarding path for existing projects by surfacing the implicit "beta" stack, which aligns with the PR objectives from issue #1719.
566-577: LGTM on compose config command stack integration.The elicitations and stack manager setup follows the established pattern from other commands, using
project.Namefor proper project context.src/cmd/cli/command/commands_test.go (3)
82-91: LGTM!The
ListDeploymentsmock implementations correctly return test data that supports the new stacks onboarding flow. The server mock returns a "beta" stack with AWS provider, which aligns with testing the implicit beta stack behavior described in the PR objectives.Also applies to: 251-255
414-443: LGTM!The
mockElicitationsControllerprovides good flexibility for testing interactive vs non-interactive paths with configurableisSupportedandenumChoicefields.
488-696: Comprehensive test coverage for stack selection.Good table-driven test structure covering key scenarios: valid/invalid stacks, auto provider errors, provider flags with warnings, non-interactive defaults, single-stack matching, interactive selection, and error propagation.
src/pkg/stacks/manager.go (5)
61-104: LGTM!The merge logic correctly handles the remote/local precedence—remote stacks overwrite local entries to show deployment timestamps. Silently ignoring
OutsideErrorfor local listing is appropriate since remote stacks should still be available when working outside the project directory.
127-149: API sort-order assumption should be verified.Line 134 comments that "deployments are already sorted by deployed_at desc", but this relies on an undocumented API contract. If the API behavior changes or varies by endpoint version, the first-occurrence logic would capture incorrect timestamps.
Consider either:
- Documenting this API contract in the proto/API specification
- Adding explicit sorting after retrieval to be defensive:
// Sort deployments by timestamp descending to ensure most recent is first sort.Slice(deployments, func(i, j int) bool { ti := deployments[i].GetTimestamp() tj := deployments[j].GetTimestamp() if ti == nil || tj == nil { return ti != nil } return ti.AsTime().After(tj.AsTime()) })
128-131: Good handling of implicit "beta" stack.Treating empty stack names as "beta" aligns with the PR objective to surface the implicit stack used by existing projects during onboarding.
157-164: LGTM!Clean custom error type with clear, informative message that will help users understand why an operation was blocked.
166-186: LGTM!Both
LoadandCreatecorrectly guard against cross-project file access by returningOutsideErrorwhen operating outside the working directory. This prevents accidental stack file modifications in unrelated projects.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pkg/stacks/manager.go (1)
45-52: Symlink-relative paths may cause incorrectoutsidedetection.The comparison at line 51 uses raw absolute paths without resolving symlinks. If either
workingDirectoryorabsTargetDirectorycontains symlinks, two paths pointing to the same physical directory will compare as different, incorrectly settingoutside = true.Consider resolving symlinks before comparison:
} else { // abs path for targetDirectory var err error absTargetDirectory, err = filepath.Abs(targetDirectory) if err != nil { return nil, fmt.Errorf("failed to get absolute path for target directory: %w", err) } + // Resolve symlinks for accurate comparison + if resolved, err := filepath.EvalSymlinks(workingDirectory); err == nil { + workingDirectory = resolved + } + if resolved, err := filepath.EvalSymlinks(absTargetDirectory); err == nil { + absTargetDirectory = resolved + } outside = workingDirectory != absTargetDirectory }src/cmd/cli/command/commands.go (1)
1350-1364: Warning says flag is "ignored" but value is still used.The warning states
--provider flag is being ignoredbut the code immediately parses and assigns the provider value tostack.Provider. This is confusing for users. The same applies to theDEFANG_PROVIDERenvironment variable warning.Consider either:
- Actually ignoring the value (don't set
stack.Provider), or- Updating the warning to say the flag is "deprecated" rather than "ignored":
if RootCmd.PersistentFlags().Changed("provider") { - term.Warn("Warning: --provider flag is being ignored. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks") + term.Warn("Warning: --provider flag is deprecated. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks")
🧹 Nitpick comments (2)
src/pkg/stacks/manager.go (1)
104-114: Duplicate comment and verbose sort comparator.Line 104-105 has a duplicate comment. The sort comparator can be simplified using
strings.Compare:- // sort stacks by name asc // sort stacks by name asc slices.SortFunc(stackList, func(a, b StackListItem) int { - if a.Name < b.Name { - return -1 - } - if a.Name > b.Name { - return 1 - } - return 0 + return strings.Compare(a.Name, b.Name) })src/cmd/cli/command/commands.go (1)
1476-1485: Variable shadowing witherrin else branch.The
errvariable on line 1478 shadows the outererrfrom line 1458, and then line 1481 redeclareserragain with:=. While this works correctly, it's confusing. Consider using explicit assignments:} else { - wd, err := os.Getwd() - if err != nil { - return nil, fmt.Errorf("failed to get working directory: %w", err) + wd, wdErr := os.Getwd() + if wdErr != nil { + return nil, fmt.Errorf("failed to get working directory: %w", wdErr) } - sm, err = stacks.NewManager(global.Client, wd, projectName) - if err != nil { - return nil, fmt.Errorf("failed to create stack manager: %w", err) + sm, smErr := stacks.NewManager(global.Client, wd, projectName) + if smErr != nil { + return nil, fmt.Errorf("failed to create stack manager: %w", smErr) } + sm = sm // or just use smErr check inline }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cmd/cli/command/commands.go(4 hunks)src/pkg/cli/client/byoc/gcp/byoc.go(1 hunks)src/pkg/stacks/manager.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/stacks/manager.go (1)
src/pkg/stacks/stacks.go (9)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)ListInDirectory(101-135)ReadInDirectory(213-225)LoadInDirectory(231-239)CreateInDirectory(40-84)
⏰ 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 (5)
src/pkg/stacks/manager.go (3)
62-98: LGTM on the merge logic.The merge correctly prioritizes remote stack data over local by processing local stacks first and then overwriting with remote entries. The
DeployedAt.Local()conversion is appropriate for display purposes.
131-168: LGTM on ListRemote implementation.The method correctly handles the implicit "beta" stack for deployments without an explicit stack name, and properly keeps only the most recent deployment per stack by leveraging the pre-sorted deployment list.
179-199: LGTM on Load and Create methods.Both methods correctly enforce directory boundary checks via
OutsideErrorand operate ontargetDirectoryas expected.src/cmd/cli/command/commands.go (2)
558-573: LGTM on whoamiCmd changes.The inline creation of the elicitations controller and stack manager is appropriate here since
whoamiCmdneeds graceful handling of provider resolution failures while still displaying user information.
1406-1435: LGTM on provider mismatch warnings.The warnings provide helpful guidance when cloud credentials are detected but the playground is selected, and vice versa.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/cmd/cli/command/estimate.go (1)
80-83: Default option references unavailable provider.The
doInEnv()check setsdefaultOptiontoProviderDO, but the caller on lines 32-35 only passesProviderAWSandProviderGCPto this function. If DigitalOcean environment variables are detected, the survey will have a default that doesn't exist in the options list, which may cause unexpected behavior.Apply the fix suggested in the previous review:
var defaultOption any // not string! if awsInEnv() { defaultOption = cliClient.ProviderAWS.String() - } else if doInEnv() { - defaultOption = cliClient.ProviderDO.String() } else if gcpInEnv() { defaultOption = cliClient.ProviderGCP.String() }src/cmd/cli/command/commands.go (2)
1351-1365: Deprecation warning is misleading.The code warns that
--providerflag andDEFANG_PROVIDERenvironment variable are "deprecated" but immediately uses their values to setstack.Provider. This contradicts the warning and confuses users—if they're deprecated and shouldn't be used, the code should not honor them.Consider either:
- Actually deprecating them (warn but don't use the values), or
- Changing the warning to clarify they still work but stacks are preferred
If truly deprecating:
if RootCmd.PersistentFlags().Changed("provider") { term.Warn("Warning: --provider flag is deprecated. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks") - providerIDString := RootCmd.Flags().Lookup("provider").Value.String() - err := stack.Provider.Set(providerIDString) - if err != nil { - return nil, "", fmt.Errorf("invalid provider %q: %w", providerIDString, err) - } } else if _, ok := os.LookupEnv("DEFANG_PROVIDER"); ok { term.Warn("Warning: DEFANG_PROVIDER environment variable is deprecated. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks") - providerIDString := os.Getenv("DEFANG_PROVIDER") - err := stack.Provider.Set(providerIDString) - if err != nil { - return nil, "", fmt.Errorf("invalid provider %q: %w", providerIDString, err) - } }Or if keeping them functional:
if RootCmd.PersistentFlags().Changed("provider") { - term.Warn("Warning: --provider flag is deprecated. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks") + term.Warn("Warning: --provider flag is being used but stacks are preferred. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks")
1373-1387: Auto-select logic won't trigger when provider is auto.The condition on line 1374 compares
knownStacks[0].Provider(e.g.,"aws") tostack.Provider.String()which defaults to"auto"(line 1318). This comparison will never match when the provider is auto, preventing the single-stack auto-selection from working in the most common case.Additionally, line 1383 assigns
stack.Provider(the originalProviderAutovalue) instead of the parsedproviderID, which means even if the condition matched, the stack would have the wrong provider.Apply this diff:
// if there is exactly one stack with that provider, use it - if len(knownStacks) == 1 && knownStacks[0].Provider == stack.Provider.String() { + if len(knownStacks) == 1 && (stack.Provider == cliClient.ProviderAuto || knownStacks[0].Provider == stack.Provider.String()) { knownStack := knownStacks[0] var providerID cliClient.ProviderID err := providerID.Set(knownStack.Provider) if err != nil { return nil, "", fmt.Errorf("invalid provider %q in stack %q: %w", knownStack.Provider, knownStack.Name, err) } stack = &stacks.StackParameters{ Name: knownStack.Name, - Provider: stack.Provider, + Provider: providerID, } whence = "only stack" return stack, whence, nil }
🧹 Nitpick comments (2)
src/cmd/cli/command/estimate.go (1)
70-72: Enhance panic message with context.The panic message could be more helpful by including the actual count received.
Apply this diff:
func interactiveSelectProvider(providers []cliClient.ProviderID) (cliClient.ProviderID, error) { if len(providers) < 2 { - panic("interactiveSelectProvider called with less than 2 providers") + panic(fmt.Sprintf("interactiveSelectProvider requires at least 2 providers, got %d", len(providers))) }src/cmd/cli/command/commands.go (1)
1312-1405: Complex stack selection logic - consider simplifying.The
getStackfunction implements multiple precedence rules and decision paths. While the logic appears correct, the complexity and multiple exit points make it harder to maintain and reason about.Consider refactoring to separate concerns:
- Stack file loading (lines 1325-1341)
- Known stacks enumeration (lines 1343-1350)
- Deprecated flag handling (lines 1351-1365)
- Auto-selection logic (lines 1373-1387)
- Interactive selection (lines 1389-1404)
This could be split into helper functions like
loadFromStackFile(),handleDeprecatedFlags(), andselectOrCreateStack()to improve clarity and testability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/estimate.go(3 hunks)src/cmd/cli/command/stack_test.go(4 hunks)src/pkg/cli/client/byoc/gcp/byoc.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/cli/client/byoc/gcp/byoc.go
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmd/cli/command/stack_test.go (3)
src/cmd/cli/command/commands.go (2)
RootCmd(412-517)Execute(75-160)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/cmd/cli/command/commands_test.go (1)
MockFabricControllerClient(223-227)
src/cmd/cli/command/estimate.go (3)
src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/track/track.go (1)
Evt(37-52)src/cmd/cli/command/commands.go (1)
P(54-54)
⏰ 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/stack_test.go (2)
31-58: Well-structured test isolation.The test properly saves and restores global state, sets up a mock client, and creates a complete RootCmd with required flags. This enables testing the full command execution path rather than just the RunE handler.
95-102: Good defensive practice.Creating the compose.yaml file prevents the stackListCmd from erroring due to missing input files, making the test more robust.
src/cmd/cli/command/commands.go (1)
559-574: EnsurewhoamiCmdinitialization handles outside-working-directory scenarios correctly.The
whoamiCmduses direct initialization of elicitations controller and stack manager, differing from most other commands which usenewProviderChecked. Critically,whoamiCmd:
- Always passes the working directory to
stacks.NewManager, without checkingloader.OutsideWorkingDirectory()first. ThenewProviderCheckedfunction correctly handles this by passing an empty string when outside the working directory.- Skips provider validation—
newProviderCheckedcallsprovider.AccountInfo(ctx)to validate, butwhoamiCmddoes not.In remote or outside-working-directory execution contexts, this pattern could fail. Consider either using
newProviderCheckeddirectly or adopting its "outside" check logic before initializing the stack manager.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cmd/cli/command/commands.go (1)
1373-1387: Auto-select logic may not trigger as intended when provider is unspecified.When neither
--providernorDEFANG_PROVIDERis set,stack.ProviderremainsProviderAuto. The condition on line 1374 comparesknownStacks[0].Provider(e.g.,"aws") againststack.Provider.String()(which would be"auto"), so this branch will only match if the user explicitly set--provider.Additionally, on line 1383, when auto-selecting, the code uses
Provider: stack.Providerwhich could still beProviderAutoif the user happened to set--provider=auto. It should use the concrete provider fromknownStack:if len(knownStacks) == 1 && knownStacks[0].Provider == stack.Provider.String() { knownStack := knownStacks[0] var providerID cliClient.ProviderID err := providerID.Set(knownStack.Provider) if err != nil { return nil, "", fmt.Errorf("invalid provider %q in stack %q: %w", knownStack.Provider, knownStack.Name, err) } stack = &stacks.StackParameters{ Name: knownStack.Name, - Provider: stack.Provider, + Provider: providerID, } whence = "only stack" return stack, whence, nil }If you intend for auto-selection to work when the user hasn't specified a provider, the condition should also handle the
ProviderAutocase.
🧹 Nitpick comments (2)
src/cmd/cli/command/commands.go (2)
559-574: Consider extracting common setup logic.This pattern of creating
elicitationsClient,ec, andsmis duplicated here and innewProviderChecked(lines 1469-1486). Consider extracting this into a helper function to reduce duplication.That said, the immediate implementation is correct and functional.
1407-1420: Consider updating warning messages to reference--stackinstead of deprecated--provider.The warning messages on lines 1412, 1415, and 1418 still reference
--providerandDEFANG_PROVIDERwhich are deprecated. Consider updating them to guide users toward the new--stackapproach:- term.Warn("AWS environment variables were detected; did you forget --provider=aws or DEFANG_PROVIDER=aws?") + term.Warn("AWS environment variables were detected; did you forget to use a stack with an AWS provider? See: defang stack create")This is optional since the current messages still work during the deprecation transition period.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cmd/cli/command/commands.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cmd/cli/command/commands.go (7)
src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (3)
NewController(35-40)Client(16-18)Controller(8-14)src/pkg/stacks/manager.go (2)
NewManager(34-60)Manager(16-20)src/pkg/stacks/stacks.go (3)
StackParameters(18-25)Load(227-229)List(97-99)src/pkg/cli/client/provider.go (2)
Provider(43-70)Loader(72-76)src/pkg/cli/client/provider_id.go (3)
ProviderAuto(13-13)ProviderDefang(14-14)ProviderID(10-10)src/pkg/cli/connect.go (1)
NewProvider(62-76)
⏰ 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 (5)
src/cmd/cli/command/commands.go (5)
29-29: LGTM!The
elicitationspackage import is correctly added to support the new stack selection flow.
203-203: LGTM!The
--providerflag deprecation aligns with the stack-first approach introduced in this PR.
1438-1456: LGTM with acknowledgment of global state concern.The function correctly implements the stack-driven provider resolution. The TODO comment on line 1444 appropriately flags the global state mutation that should be addressed in a future refactor.
1458-1493: LGTM!The function correctly handles the nil loader case and properly integrates with the new stack-driven flow. The logic for determining
outsideand creating the appropriate stack manager is sound.
1495-1497: LGTM!Clean delegation to
cliClient.CanIUseProvider.
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
e4c335b to
bc99562
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 (3)
src/pkg/agent/tools/estimate.go (1)
20-26: Redundant error assignment creates unused variable.Line 24 assigns to
errbut then immediately returns on line 25 with a new error that wraps the originalerr(not the reassigned one). The assignment on line 24 is dead code.Suggested fix
func HandleEstimateTool(ctx context.Context, loader cliClient.Loader, params EstimateParams, cli CLIInterface, sc StackConfig) (string, error) { term.Debug("Function invoked: loader.LoadProject") project, err := cli.LoadProject(ctx, loader) if err != nil { - err = fmt.Errorf("failed to parse compose file: %w", err) return "", fmt.Errorf("failed to parse compose file: %w", err) }src/pkg/agent/tools/provider.go (2)
13-13: Remove duplicate constant - usestacks.CreateNewStackinstead.This constant duplicates
stacks.CreateNewStack(defined at selector.go:14). Using the same constant from two places can lead to subtle bugs if they drift out of sync.🔎 Apply this diff to remove the duplicate:
-const CreateNewStack = "Create new stack"If this constant is used elsewhere in this file, replace usages with
stacks.CreateNewStack.
35-45: Variable shadowing:erris declared twice in the same scope.The
errvariable is declared with:=on line 40, but it was already declared on line 37. While this works due to block scoping, it can cause confusion. The outererr(line 37) is never used.🔎 Apply this diff to fix the shadowing:
func (pp *providerPreparer) SetupProvider(ctx context.Context, stack *stacks.StackParameters) (*cliClient.ProviderID, cliClient.Provider, error) { var providerID cliClient.ProviderID - var err error if stack.Name == "" { selector := stacks.NewSelector(pp.ec, pp.sm) newStack, err := selector.SelectStack(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to setup stack: %w", err) } *stack = *newStack } - err = providerID.Set(stack.Provider.Name()) + err := providerID.Set(stack.Provider.Name()) if err != nil { return nil, nil, fmt.Errorf("failed to set provider ID: %w", err) }
♻️ Duplicate comments (6)
src/pkg/cli/compose/loader.go (1)
94-97: Fix typo in comment: "provider" → "provided".The comment still contains the typo. It should read "if --project-name is provided" not "if --project-name is provider".
Suggested fix
func (l *Loader) OutsideWorkingDirectory() bool { - // if --project-name is provider, we assume we are outside the project's working directory + // if --project-name is provided, we assume we are outside the project's working directory return l.options.ProjectName != "" }src/pkg/stacks/manager.go (1)
44-52: Symlink-relative paths may cause incorrectoutsidedetection.The comparison at line 51 doesn't resolve symlinks. If either path contains symlinks, two paths pointing to the same physical directory may compare as different, incorrectly setting
outside = true.Suggested fix
} else { // abs path for targetDirectory - var err error - absTargetDirectory, err = filepath.Abs(targetDirectory) + absTargetDirectory, err := filepath.Abs(targetDirectory) if err != nil { return nil, fmt.Errorf("failed to get absolute path for target directory: %w", err) } + // Resolve symlinks for accurate comparison + if resolved, err := filepath.EvalSymlinks(workingDirectory); err == nil { + workingDirectory = resolved + } + if resolved, err := filepath.EvalSymlinks(absTargetDirectory); err == nil { + absTargetDirectory = resolved + } outside = workingDirectory != absTargetDirectory }src/cmd/cli/command/estimate.go (1)
80-84: Default option still references unavailable provider.As flagged in the previous review, the code sets
defaultOptiontoProviderDOwhendoInEnv()returns true (line 82), but DigitalOcean is not included in theoptionsslice passed on lines 32-35. This mismatch may cause unexpected survey behavior or selection errors.🔎 Apply the previously suggested fix
var defaultOption any // not string! if awsInEnv() { defaultOption = cliClient.ProviderAWS.String() - } else if doInEnv() { - defaultOption = cliClient.ProviderDO.String() } else if gcpInEnv() { defaultOption = cliClient.ProviderGCP.String() }src/cmd/cli/command/commands.go (3)
1347-1361: Warning message is misleading: says "deprecated" but value is still used.The warning tells users to use
--stackinstead, but the code still reads and applies the provider value tostack.Provider. This is actually reasonable behavior for backwards compatibility during deprecation. Consider clarifying the warning:- term.Warn("Warning: --provider flag is deprecated. Please use --stack instead. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks") + term.Warn("Warning: --provider flag is deprecated and will be removed in a future release. Please migrate to --stack. To learn about stacks, visit https://docs.defang.io/docs/concepts/stacks")This makes it clearer that the flag still works but will be removed later.
1369-1383: Auto-select only triggers when provider explicitly matches.The condition
knownStacks[0].Provider == stack.Provider.String()means auto-selection only occurs when:
- There's exactly one stack
- Its provider matches the explicitly set provider (or "auto" if nothing was set)
Since
stack.Providerdefaults toProviderAuto, andknownStacks[0].Providerwill be a concrete provider like "aws", this path will rarely match unless the user explicitly set--provider.If the intent is to auto-select when there's exactly one stack regardless of provider, consider:
- if len(knownStacks) == 1 && knownStacks[0].Provider == stack.Provider.String() { + if len(knownStacks) == 1 && (stack.Provider == cliClient.ProviderAuto || knownStacks[0].Provider == stack.Provider.String()) {If the current behavior is intentional, the comment should clarify this limitation.
1318-1320: Typo: "RoomCmd" should be "RootCmd".// This code unfortunately replicates the provider precedence rules in the - // RoomCmd's PersistentPreRunE func, I think we should avoid reading the + // RootCmd's PersistentPreRunE func, I think we should avoid reading the // stack file during startup, and only read it here instead.
🧹 Nitpick comments (6)
src/pkg/stacks/manager.go (1)
104-114: Duplicate comment and verbose sorting.Line 104-105 has a duplicate comment. The sort function can be simplified using
strings.Compare.Suggested fix
- // sort stacks by name asc // sort stacks by name asc slices.SortFunc(stackList, func(a, b StackListItem) int { - if a.Name < b.Name { - return -1 - } - if a.Name > b.Name { - return 1 - } - return 0 + return strings.Compare(a.Name, b.Name) })Note: Add
"strings"to the imports.src/pkg/stacks/selector_test.go (2)
120-123: MoveWizardInterfaceto production code for better dependency injection.The interface currently defined in the test file (selector_test.go) matches the contract implemented by the concrete
Wizardtype in production (wizard.go). Moving this interface to the main package and updatingselector.goto depend on it rather than the concreteWizardtype would follow the dependency inversion principle and enable cleaner testing while supporting alternative implementations.
142-196: Consider injecting wizard dependency in productionstackSelectorto avoid duplication.The
testableStackSelectorduplicatesSelectStacklogic specifically to inject aWizardInterfacefor testing. This duplication could be eliminated by refactoring the productionstackSelectorto accept wizard injection in its constructor or method, matching the pattern already used in tests.The
elicitStackSelectionimplementation difference is intentional—the test version correctly omits UI formatting logic (label formatting, deploy dates, info message) that belongs in production code.src/cmd/cli/command/estimate.go (1)
70-72: Consider returning an error instead of panic.The panic on line 71 will crash the CLI if called with fewer than 2 providers. While this appears to be an internal programming error rather than a user input issue, returning an error would be more graceful and testable.
🔎 View suggested fix
func interactiveSelectProvider(providers []cliClient.ProviderID) (cliClient.ProviderID, error) { if len(providers) < 2 { - panic("interactiveSelectProvider called with less than 2 providers") + return "", fmt.Errorf("interactiveSelectProvider requires at least 2 providers, got %d", len(providers)) }src/cmd/cli/command/stack.go (1)
90-110: Consider graceful handling when project name cannot be loaded.The current implementation returns an error if
loader.LoadProjectName(ctx)fails. Forstack list, users might want to see local stacks even without a valid compose file. Consider making project name loading optional:projectName, err := loader.LoadProjectName(ctx) if err != nil { - return err + term.Debugf("Unable to load project name: %v", err) + projectName = "" // Allow listing with empty project name }Alternatively, if the current behavior is intentional (require project context for remote deployment info), consider adding a clearer error message explaining why a compose file is needed.
src/cmd/cli/command/commands_test.go (1)
271-302: Side effect in mock: Load sets environment variables.The
mockStacksManager.Loadmethod setsAWS_REGIONandDEFANG_PROVIDERenvironment variables (lines 291-292). Whilet.Setenvis used (which restores values after the test), this couples the mock to specific test expectations and could cause confusion.Consider making the mock more focused:
func (m *mockStacksManager) Load(name string) (*stacks.StackParameters, error) { - m.t.Setenv("AWS_REGION", "us-west-2") - m.t.Setenv("DEFANG_PROVIDER", m.expectedProvider.String()) return &stacks.StackParameters{ Name: name, Provider: m.expectedProvider, Region: "us-west-2", }, nil }Let the test code set environment variables explicitly if needed, keeping the mock's responsibility limited to returning data.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(9 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/estimate.go(3 hunks)src/cmd/cli/command/globals.go(1 hunks)src/cmd/cli/command/stack.go(2 hunks)src/cmd/cli/command/stack_test.go(4 hunks)src/cmd/cli/command/workspace_test.go(2 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(2 hunks)src/pkg/agent/tools/deploy_test.go(1 hunks)src/pkg/agent/tools/destroy.go(2 hunks)src/pkg/agent/tools/destroy_test.go(1 hunks)src/pkg/agent/tools/estimate.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/logs.go(3 hunks)src/pkg/agent/tools/provider.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/cli/client/byoc/baseclient.go(1 hunks)src/pkg/cli/client/byoc/gcp/byoc.go(1 hunks)src/pkg/cli/client/caniuse.go(2 hunks)src/pkg/cli/client/client.go(0 hunks)src/pkg/cli/client/mock.go(1 hunks)src/pkg/cli/client/playground.go(1 hunks)src/pkg/cli/client/provider.go(1 hunks)src/pkg/cli/compose/loader.go(1 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/manager_test.go(12 hunks)src/pkg/stacks/selector.go(1 hunks)src/pkg/stacks/selector_test.go(1 hunks)src/pkg/stacks/stacks.go(2 hunks)src/testdata/sanity/.defang/beta(1 hunks)
💤 Files with no reviewable changes (1)
- src/pkg/cli/client/client.go
🚧 Files skipped from review as they are similar to previous changes (10)
- src/pkg/cli/client/playground.go
- src/pkg/cli/client/provider.go
- src/pkg/stacks/stacks.go
- src/pkg/agent/tools/listConfig.go
- src/pkg/cli/client/mock.go
- src/pkg/cli/client/caniuse.go
- src/pkg/agent/tools/default_tool_cli.go
- src/pkg/cli/client/byoc/gcp/byoc.go
- src/pkg/stacks/selector.go
- src/cmd/cli/command/stack_test.go
🧰 Additional context used
🧬 Code graph analysis (19)
src/pkg/stacks/selector_test.go (6)
src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)src/pkg/stacks/selector.go (2)
CreateNewStack(15-15)NewSelector(22-27)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)src/pkg/modes/modes.go (3)
Mode(12-12)ModeBalanced(17-17)ModeAffordable(16-16)src/pkg/elicitations/elicitations.go (1)
Controller(8-14)src/pkg/stacks/manager.go (1)
Manager(16-20)
src/pkg/agent/tools/destroy.go (6)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(19-22)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-17)src/pkg/cli/connect.go (1)
Connect(17-19)src/pkg/stacks/manager.go (1)
NewManager(34-60)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/removeConfig.go (5)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(19-22)src/pkg/cli/connect.go (1)
Connect(17-19)src/pkg/stacks/manager.go (1)
NewManager(34-60)
src/pkg/agent/tools/provider.go (1)
src/pkg/stacks/selector.go (1)
NewSelector(22-27)
src/cmd/cli/command/globals.go (3)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/provider_id.go (1)
ProviderAuto(13-13)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)
src/pkg/stacks/manager.go (4)
src/pkg/stacks/stacks.go (9)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)ListInDirectory(101-135)ReadInDirectory(213-225)LoadInDirectory(231-239)CreateInDirectory(40-84)src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
src/cmd/cli/command/stack.go (2)
src/pkg/stacks/manager.go (1)
NewManager(34-60)src/pkg/stacks/stacks.go (1)
List(97-99)
src/pkg/agent/tools/deploy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/cmd/cli/command/compose.go (5)
src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/stacks/manager.go (1)
NewManager(34-60)
src/pkg/agent/tools/services.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(34-60)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/cmd/cli/command/estimate.go (6)
src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/term/colorizer.go (2)
Errorf(322-324)DefaultTerm(27-27)src/pkg/track/track.go (1)
Evt(37-52)src/cmd/cli/command/commands.go (1)
P(54-54)src/pkg/cli/tail.go (1)
P(93-93)src/pkg/debug/debug.go (1)
P(21-21)
src/pkg/cli/compose/loader.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/clouds/driver.go (1)
ProjectName(8-8)
src/pkg/agent/tools/interfaces.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/estimate.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)
src/pkg/agent/tools/destroy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/agent/tools/setConfig.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(34-60)
src/pkg/agent/tools/logs.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(34-60)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/cmd/cli/command/commands_test.go (4)
src/pkg/stacks/manager.go (1)
Manager(16-20)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/stacks.go (5)
List(97-99)StackListItem(87-95)Load(227-229)StackParameters(18-25)Create(36-38)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)
src/pkg/stacks/manager_test.go (5)
src/pkg/stacks/manager.go (1)
NewManager(34-60)src/pkg/stacks/stacks.go (5)
List(97-99)Create(36-38)StackParameters(18-25)StackListItem(87-95)Directory(29-29)src/protos/io/defang/v1/fabric.pb.go (2)
Provider_AWS(33-33)Provider_GCP(35-35)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)src/pkg/modes/modes.go (2)
Mode(12-12)ModeAffordable(16-16)
🪛 GitHub Actions: Go package
src/cmd/cli/command/workspace_test.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/commands.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/globals.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/stack.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/compose.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/estimate.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
src/cmd/cli/command/commands_test.go
[error] 1-1: Tests failed in package 'github.com/DefangLabs/defang/src/cmd/cli/command'.
⏰ 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 (61)
src/testdata/sanity/.defang/beta (1)
1-2: LGTM! Appropriate test data for stack configuration.The environment variable format is standard and the values (AWS provider with us-west-2 region) are appropriate for testing the beta stack functionality introduced in this PR.
src/cmd/cli/command/globals.go (1)
98-98: LGTM!The
Namefield initialization aligns correctly with theStackParametersstruct definition. The empty string default is appropriate as the stack name will be determined later via flags or environment variables.src/pkg/stacks/manager.go (1)
140-161: Good handling of implicit "beta" stack for legacy deployments.The logic correctly treats deployments without an explicit stack name as the implicit "beta" stack (line 142-144), which aligns with the PR objective to surface the implicit beta stack for existing projects. The map-based deduplication with "first wins" semantics (line 148) correctly preserves the most recent deployment since deployments are pre-sorted by
deployed_at desc.src/pkg/agent/tools/removeConfig.go (1)
21-31: LGTM!The function correctly adapts to the new
stacks.NewManagersignature and adds appropriate error handling. The loader type change fromProjectLoadertoLoaderaligns with the interface updates across the codebase.src/pkg/stacks/selector_test.go (9)
16-43: LGTM! Well-structured mock implementations.The mock implementations correctly implement all methods of the
elicitations.Controllerinterface with proper type assertions and error handling.
45-77: LGTM! MockStacksManager handles nil returns correctly.The mock properly handles nil return values with type assertions, preventing panics when simulating error scenarios.
79-118: LGTM! Comprehensive test for existing stack selection.The test properly validates the complete flow: elicitations support check → list stacks → user selection → load stack parameters.
198-248: LGTM! Good test coverage for the "create new stack" flow.The test validates the complete wizard-driven stack creation flow including parameter collection, stack creation, and subsequent loading.
250-293: LGTM! Tests the automatic stack creation when no stacks exist.Good coverage for the edge case where
elicitStackSelectionreturnsCreateNewStackautomatically due to empty stack list.
295-314: LGTM! Proper error handling test for unsupported elicitations.The test correctly verifies that when elicitations are not supported, an appropriate error message is returned guiding users to use the
select_stacktool.
316-405: LGTM! Comprehensive error path coverage.The tests for
ListStacksError,ElicitationError, andLoadStackErrorproperly validate error propagation and message wrapping.
407-497: LGTM! Good coverage for wizard and creation error paths.Tests properly validate that errors from wizard parameter collection and stack creation are correctly propagated with appropriate context.
499-536: LGTM! Sub-tests forelicitStackSelectionbehavior.Good use of table-driven sub-tests to verify both the "multiple stacks" and "no stacks" scenarios for the elicitation helper.
src/pkg/stacks/manager_test.go (9)
18-31: LGTM! Clean mock implementation for FabricClient.The mock properly simulates both success and error scenarios for
ListDeployments.
46-66: LGTM! Test properly updated for new Manager API.The test correctly uses
t.Chdir()to set up the working directory match condition and passes context toList().
340-391: LGTM! Good test coverage for remote stack listing.The test validates that remote deployments are correctly converted to
StackListItementries with proper field mapping includingDeployedAt.
411-509: LGTM! Comprehensive test for merged local/remote stack lists.The test validates the expected merge behavior:
- Shared stacks prefer remote data (region, deployment time)
- Remote-only stacks are included
- Local-only stacks retain their properties without deployment time
511-543: LGTM! Good test for beta stack onboarding.This test validates the core onboarding feature: empty
Stackfield in deployments is correctly interpreted as the implicit "beta" stack.
545-584: LGTM! Tests duplicate deployment handling.The test confirms that when multiple deployments exist for the same stack name, only one entry is returned (using the first/most recent deployment).
586-638: LGTM! Tests local operations when directories match.Good validation that Create, List, and Load all work correctly when the working directory matches the target directory.
640-716: Good test for cross-project access prevention.This test validates the security feature that prevents local stack file access when operating outside the project directory, while still allowing remote stack listing.
718-752: LGTM! Confirms remote operations work regardless of directory.Good validation that
ListRemoteis not subject to directory restrictions.src/cmd/cli/command/workspace_test.go (2)
90-92: Same fix applied - ensure both tests pass together.Both tests apply the same reset pattern. Verify this is sufficient by running the tests locally or checking if additional state needs to be cleared.
55-57: The fix is sufficient - tests properly isolate global state.The tests save and restore the entire
globalstate via cleanup, ensuring full restoration after execution. Resettingglobal.Stack.Nameto""before the test is intentional and targeted, preventing unwanted stack file loading during the test execution. This pattern provides complete test isolation without requiring other global fields to be explicitly reset.src/pkg/cli/client/byoc/baseclient.go (2)
74-77: LGTM! Simple accessor implementing the Provider interface.The
GetStackName()method correctly returns thePulumiStackfield, enabling stack-driven flows throughout the codebase.
116-122: The current implementation is correct.GetProjectDomainreturns only the normalized zone, which is the intended behavior. Stack management is handled separately through a tagging system (zone tags likedefang:stack), not by prefixing the domain. There is no evidence of a previous implementation that included the stack name in the domain, and this change does not affect existing deployments.Likely an incorrect or invalid review comment.
src/pkg/agent/tools/provider.go (1)
39-44: LGTM! Clean integration with the new selector workflow.The delegation to
stacks.NewSelectorand the in-place update via pointer assignment correctly integrates with the new stack selection flow while maintaining the caller's state.src/cmd/cli/command/estimate.go (1)
31-40: Interactive provider selection integrated correctly.The flow correctly prompts for provider selection when auto-detection is active and updates the global stack provider with the result. Error handling and context wrapping are appropriate.
src/pkg/agent/tools/interfaces.go (1)
15-15: LGTM!The
CanIUseProvidersignature update correctly aligns with the implementation insrc/pkg/cli/client/caniuse.go. The removal ofstackNameand reordering ofproviderbeforeprojectNameis consistent with the stack-driven provider resolution flow introduced in this PR.src/pkg/agent/tools/deploy_test.go (1)
80-83: LGTM!The mock signature correctly matches the updated
CLIInterface.CanIUseProvidermethod. The parameter order change (provider before projectName) and removal of stackName align with the interface definition and implementation.src/pkg/agent/tools/destroy_test.go (1)
60-66: LGTM!The mock signature correctly reflects the updated interface with provider before projectName and stackName removed. The implementation properly logs the call and returns the configured error.
src/pkg/agent/tools/services.go (2)
23-23: LGTM!The signature update from
cliClient.ProjectLoadertocliClient.Loaderaligns with the broader refactor across tool handlers and the updated Loader interface definition.
30-34: LGTM!The stack manager creation correctly uses the new 3-parameter signature
stacks.NewManager(client, workingDirectory, projectName)with proper error handling. The error is appropriately wrapped with context and propagated.src/pkg/agent/tools/setConfig.go (3)
21-21: LGTM!The signature update to
cliClient.Loaderis consistent with the refactor across all tool handlers.
28-36: LGTM!The stack manager creation and provider setup follow the standardized pattern introduced in this PR. Error handling is appropriate and errors are properly wrapped with context.
38-45: LGTM!The project name loading flow correctly handles the case where
params.ProjectNameis empty by callingLoadProjectNameWithFallbackand assigning the result. Error handling and wrapping are appropriate.src/pkg/agent/tools/destroy.go (3)
20-20: LGTM!The signature update to
cliClient.Loaderis consistent with the broader refactor across tool handlers.
27-35: LGTM!The stack manager creation follows the new 3-parameter pattern with proper error handling, and the provider setup flow is correctly integrated. Error wrapping provides clear context.
42-45: LGTM!The
CanIUseProvidercall correctly uses the updated signature with provider before projectName and without the stackName parameter. Error handling and wrapping are appropriate.src/pkg/agent/tools/logs.go (3)
25-25: LGTM!The signature update to
cliClient.Loaderaligns with the refactor applied consistently across all tool handlers in this PR.
48-56: LGTM!The stack manager creation and provider setup follow the standardized pattern with proper error handling. The new 3-parameter
NewManagersignature is used correctly and errors are appropriately wrapped.
65-68: LGTM!The
CanIUseProvidercall correctly uses the updated signature with provider positioned before projectName. Error handling is appropriate.src/cmd/cli/command/compose.go (4)
8-8: LGTM: New imports support elicitations and stack management.The added imports for
osandelicitationsare required for the new stack-driven provider resolution flow inmakeComposeConfigCmd.Also applies to: 22-22
117-117: LGTM: Passing stack name to handleExistingDeployments.Correctly passes
provider.GetStackName()to enable proper stack file creation when deploying to a new location.
223-252: LGTM: Updated signature and provider consistency check.The function now:
- Accepts an explicit
stackNameparameter for stack awareness- Compares
dep.ProvideragainstaccountInfo.Provider.Value()for proper type matching- Defaults to
"beta"whenstackNameis empty, supporting the implicit beta stack onboardingThe logic correctly filters deployments by provider, account ID, and region.
566-577: LGTM: Elicitations and stack manager wiring in compose config.The inline construction of elicitations client, controller, and stack manager follows the same pattern used elsewhere in the PR. The error handling for working directory and stack manager creation is appropriate.
src/pkg/agent/tools/deploy.go (2)
25-25: LGTM: Updated function signature.The change from
cliClient.ProjectLoadertocliClient.Loaderaligns with the broader interface updates in this PR for stack-driven provider resolution.
47-57: LGTM: Stack manager creation and provider usage check.The stack manager is correctly instantiated with
client,params.WorkingDirectory, andparams.ProjectName. Error propagation is properly handled. TheCanIUseProvidercall matches the updated signature used elsewhere in the codebase.src/cmd/cli/command/stack.go (1)
130-131: LGTM: Extended table columns.The addition of the "DeployedAt" column provides useful information about when stacks were last deployed.
src/cmd/cli/command/commands.go (6)
199-199: LGTM: Provider flag deprecation.Marking
--provideras deprecated guides users toward the new--stackapproach.
555-570: LGTM: Whoami command updated for stack-driven flow.The whoami command now properly creates elicitations client, controller, and stack manager to integrate with the new provider resolution path.
1403-1431: LGTM: Provider mismatch warnings extracted to helper.Clean extraction of warning logic into a dedicated function improves readability and reusability.
1434-1452: LGTM: Refactored newProvider with stack-driven flow.The function now correctly:
- Uses
getStackfor stack resolution- Updates the global stack state (with TODO noting this should be removed)
- Prints informative message about stack usage
- Calls provider mismatch warnings
1454-1489: LGTM: newProviderChecked properly handles nil loader and outside working directory.The function correctly:
- Handles nil loader case (addressed from past review)
- Uses
loader.OutsideWorkingDirectory()to determine stack manager construction- Creates appropriate stack manager based on context
1491-1492: LGTM: Simplified canIUseProvider.Clean delegation to
cliClient.CanIUseProviderwith the updated signature.src/cmd/cli/command/commands_test.go (6)
16-17: LGTM: New imports for test support.The
modesandstacksimports are required for the new test mocks and assertions.
82-91: LGTM: ListDeployments mock added to fabric service.Returns a mock deployment for testing the new deployment-aware stack flow.
242-253: LGTM: MockFabricControllerClient enhanced.Good addition of nil check for
savedProvidermap andListDeploymentsimplementation.
427-500: LGTM: Well-structured mock implementations.Both
mockElicitationsControllerandmockStackManagerprovide flexible mocking capabilities:
- Support for default behaviors and specific overrides
- Clean interface implementations
- Appropriate field exposure for test configuration
501-802: Comprehensive test suite for getStack.The
TestGetStackfunction provides good coverage of:
- Stack flag with valid/invalid stacks
- Provider flag deprecation path
- Environment variable handling
- Non-interactive defaults
- Single stack auto-selection
- Interactive selection
- Error propagation
Minor observation: The test at line 750 creates an
outputbuffer that's never used for capturing warnings (as noted in the comment at line 793-798). This is acceptable since verifyingterm.Warnoutput in tests is complex.
345-388: Test setup correctly triggers production behavior.The test properly tests the DEFANG_STACK environment variable path. Since
os.Getenv("DEFANG_STACK")is set to "beta", the condition in getStack evaluates to true and the code executes as expected. The test does not test the--stackflag alternative path, but that limitation is outside the scope of this test case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cmd/cli/command/commands.go (2)
1347-1361: Clarify deprecated flag behavior to reduce user confusion.The deprecation warnings state "please use --stack instead," but the code immediately reads and uses the deprecated flag/environment variable values. While technically correct (deprecated doesn't mean removed), users might expect these to be ignored based on the warning.
Consider either:
- Making the warning more explicit: "Warning: --provider flag is deprecated but still supported. Please migrate to --stack. See https://docs.defang.io/docs/concepts/stacks"
- Adding a brief code comment explaining that deprecated inputs are honored for backward compatibility
This would help users understand that their existing workflows still work while encouraging migration.
1369-1383: Consider adding a clarifying comment about auto-select conditions.The auto-select logic at line 1370 only triggers when there's exactly one stack AND the provider matches. When
stack.ProviderisProviderAuto(the default when no provider is specified), this condition won't match even if there's only one stack available.This appears intentional based on past review discussions, but a brief comment explaining the behavior would improve maintainability:
// Auto-select only when provider was explicitly specified and matches the single known stack. // When provider is Auto (not specified), we'll prompt the user even with one stack to increase awareness. if len(knownStacks) == 1 && knownStacks[0].Provider == stack.Provider.String() {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cmd/cli/command/commands.go(5 hunks)
🔇 Additional comments (4)
src/cmd/cli/command/commands.go (4)
555-570: LGTM: whoami integration with stack-driven provider resolution.The changes correctly integrate the whoami command with the new stack-driven provider resolution flow. Setting
global.NonInteractive = trueappropriately prevents prompts, and error handling allows the command to function even when project loading fails.
1403-1432: LGTM: helpful provider mismatch warnings.This function provides valuable feedback to users when their environment variables don't match their selected provider, helping catch common configuration mistakes early.
1434-1452: LGTM: clean integration of stack-driven provider resolution.The function properly delegates to
getStack, provides clear user feedback about which stack and provider are being used, validates the configuration with mismatch warnings, and creates the provider. The TODO comment on line 1440 appropriately flags the global state mutation for future refactoring.
1454-1489: LGTM: robust nil handling and stack manager initialization.The function correctly handles the nil loader case (lines 1458-1464) and properly determines whether to create a stack manager with or without a working directory based on the loader's location. This supports the "remote stacks" feature mentioned in the PR objectives. Error handling is comprehensive throughout.
This update allows the stack manager to list both local and remote stacks, enabling deployment to previously deployed stacks without requiring local stack files. Local stacks are read from the `.defang` directory while remote stacks are fetched from deployed infrastructure. Key changes: - Enhanced Manager interface to accept context for remote API calls - Extended NewManager to accept DeploymentLister for remote stack queries - Added ListRemote method to fetch deployed stacks from infrastructure - Updated List method to merge local and remote stacks with remote precedence - Added DeployedAt timestamp to StackListItem for deployment tracking 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The NewManager function signature was changed to return error for better error handling. Updated all callers to properly handle the error return value and added appropriate error checking with descriptive error messages. Changes: - Updated all agent tools to handle NewManager error return - Fixed test cases to use proper error handling syntax - Added error checks with consistent "failed to create stack manager" messages 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The stack manager was updated to enforce working directory restrictions for local operations, but the tests were failing due to symlink path comparison issues and improper test setup. This commit addresses both the underlying symlink resolution issue in the manager and updates tests to properly handle directory constraints. Key fixes: - Add symlink resolution in NewManager to handle macOS /var -> /private/var symlinks - Update failing tests to use t.Chdir() when testing local operations - Add comprehensive test coverage for both directory scenarios: * TestManager_WorkingDirectoryMatches: validates local operations work when directories match * TestManager_WorkingDirectoryDifferent: validates local operations are properly blocked * TestManager_RemoteOperationsWorkRegardlessOfDirectory: validates remote operations always work - Add proper error message validation in directory mismatch tests All tests now pass, providing clear validation of the manager's directory-based access control while maintaining backwards compatibility for valid use cases. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
instead of loader
instead of loader
its only remaining callsite
jordanstephens
force-pushed
the
jordan/remote-stacks
branch
from
d1f8d44 to
67cb5b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
src/cmd/cli/command/commands.go (3)
1311-1313: Typo: "RoomCmd" should be "RootCmd".This was noted in a previous review and appears to still be present.
// This code unfortunately replicates the provider precedence rules in the - // RoomCmd's PersistentPreRunE func, I think we should avoid reading the + // RootCmd's PersistentPreRunE func, I think we should avoid reading the // stack file during startup, and only read it here instead.
1377-1382: Bug:stack.Providershould use the parsedproviderIDfrom the known stack.On line 1366,
knownStack.Provideris parsed intoproviderID, but on line 1379,stack.Provideruses the original value (which could beProviderAuto) instead of the parsedproviderID.🔎 Proposed fix
stack = &stacks.StackParameters{ Name: knownStack.Name, - Provider: stack.Provider, + Provider: providerID, Region: knownStack.Region, Mode: mode, }
1362-1389: Auto-select condition doesn't work when provider is unspecified.When the user doesn't specify
--provider,stack.ProviderremainsProviderAuto("auto"), but real stacks have concrete providers like "aws" or "gcp". The condition on line 1363 comparesknownStacks[0].Provider(e.g., "aws") againststack.Provider.String()("auto"), which will never match.Auto-selection only works when the user explicitly sets
--providerto match the stack's provider. For a single stack with an unspecified provider, users are prompted to select instead of being auto-selected.If the intent is to auto-select any single stack regardless of provider specification, apply the proposed fix:
// if there is exactly one stack with that provider, use it - if len(knownStacks) == 1 && knownStacks[0].Provider == stack.Provider.String() { + if len(knownStacks) == 1 && (stack.Provider == cliClient.ProviderAuto || knownStacks[0].Provider == stack.Provider.String()) {src/pkg/stacks/selector.go (1)
101-115: Past review comment is incorrect; current implementation works correctly.The past review comment claimed that the beta detection at line 103 would fail for deployed stacks because
stackNamescontains labels like"beta (deployed Jan 2)". However, examining the code at lines 66-77,stackNamesis populated withs.Name(the raw stack name), whilestackLabelsreceives the formatted version with deployment dates. ThebetaExistscheck at line 103 correctly usesstackNames, so it will detect "beta" regardless of deployment status.src/pkg/stacks/manager.go (1)
42-52: Symlink-relative paths may cause incorrectoutsidedetection.The comparison at line 52 uses raw absolute paths without resolving symlinks. If either
workingDirectoryorabsTargetDirectorycontains symlinks, two paths pointing to the same physical directory may compare as different, incorrectly settingoutside = trueand blocking legitimate local operations (Create, Load, ListLocal).🔎 Proposed fix to resolve symlinks before comparison
} else { // abs path for targetDirectory var err error absTargetDirectory, err = filepath.Abs(targetDirectory) if err != nil { return nil, fmt.Errorf("failed to get absolute path for target directory: %w", err) } + // Resolve symlinks for accurate comparison + resolvedWD, err := filepath.EvalSymlinks(workingDirectory) + if err != nil { + return nil, fmt.Errorf("failed to resolve symlinks in working directory: %w", err) + } + resolvedTarget, err := filepath.EvalSymlinks(absTargetDirectory) + if err != nil { + return nil, fmt.Errorf("failed to resolve symlinks in target directory: %w", err) + } + outside = resolvedWD != resolvedTarget - outside = workingDirectory != absTargetDirectory }Based on learnings from past review comments.
🧹 Nitpick comments (3)
src/pkg/stacks/stacks.go (1)
181-186: DeployedAt field is not populated for local stacks.When listing stacks from the
.defangdirectory,DeployedAtis initialized to its zero value. Per the AI summary, this is noted as "not yet populated in the existing code path." Consider adding a comment here to clarify thatDeployedAtis populated only for remote/deployed stacks via the manager'sListmethod.src/pkg/stacks/selector_test.go (1)
147-201: Test doubletestableStackSelectorduplicates production logic.The
testableStackSelector.SelectStackmethod duplicates the flow from the productionstackSelector. This means:
- Tests for the "Create New Stack" path (lines 203-253, 255-298, 412-451, 453-502) test the test double, not the production code
- Changes to production
stackSelector.SelectStackwon't be caught by these testsConsider either:
- Refactoring production code to accept a
WizardInterfacedependency- Or adding integration tests that exercise the actual production path
src/pkg/stacks/selector.go (1)
52-52: Wrap Load error for consistency.For consistency with error handling elsewhere in this function (lines 35, 42, 46), wrap the
Loaderror with context.🔎 Proposed fix
- return ss.sm.Load(selectedStackName) + stack, err := ss.sm.Load(selectedStackName) + if err != nil { + return nil, fmt.Errorf("failed to load stack %q: %w", selectedStackName, err) + } + return stack, nil
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(9 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/estimate.go(3 hunks)src/cmd/cli/command/globals.go(1 hunks)src/cmd/cli/command/stack.go(2 hunks)src/cmd/cli/command/stack_test.go(5 hunks)src/cmd/cli/command/workspace_test.go(2 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(2 hunks)src/pkg/agent/tools/deploy_test.go(1 hunks)src/pkg/agent/tools/destroy.go(2 hunks)src/pkg/agent/tools/destroy_test.go(1 hunks)src/pkg/agent/tools/estimate.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/logs.go(3 hunks)src/pkg/agent/tools/provider.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/cli/client/byoc/baseclient.go(1 hunks)src/pkg/cli/client/byoc/gcp/byoc.go(1 hunks)src/pkg/cli/client/caniuse.go(2 hunks)src/pkg/cli/client/client.go(0 hunks)src/pkg/cli/client/mock.go(1 hunks)src/pkg/cli/client/playground.go(1 hunks)src/pkg/cli/client/provider.go(1 hunks)src/pkg/cli/compose/loader.go(1 hunks)src/pkg/stacks/manager.go(1 hunks)src/pkg/stacks/manager_test.go(12 hunks)src/pkg/stacks/selector.go(1 hunks)src/pkg/stacks/selector_test.go(1 hunks)src/pkg/stacks/stacks.go(5 hunks)src/pkg/stacks/stacks_test.go(1 hunks)src/testdata/sanity/.defang/beta(1 hunks)
💤 Files with no reviewable changes (1)
- src/pkg/cli/client/client.go
✅ Files skipped from review due to trivial changes (1)
- src/testdata/sanity/.defang/beta
🚧 Files skipped from review as they are similar to previous changes (11)
- src/pkg/cli/client/playground.go
- src/pkg/cli/client/provider.go
- src/cmd/cli/command/workspace_test.go
- src/pkg/cli/client/byoc/baseclient.go
- src/pkg/cli/compose/loader.go
- src/pkg/cli/client/byoc/gcp/byoc.go
- src/pkg/agent/tools/default_tool_cli.go
- src/cmd/cli/command/stack.go
- src/pkg/agent/tools/deploy_test.go
- src/pkg/agent/tools/interfaces.go
- src/cmd/cli/command/globals.go
🧰 Additional context used
🧬 Code graph analysis (21)
src/pkg/agent/tools/deploy.go (5)
src/pkg/cli/compose/loader.go (2)
Loader(38-41)Services(29-29)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/estimate.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/cli/compose/loader.go (1)
Loader(38-41)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)
src/pkg/agent/tools/provider.go (1)
src/pkg/stacks/selector.go (1)
NewSelector(22-27)
src/cmd/cli/command/estimate.go (5)
src/pkg/cli/client/provider_id.go (3)
ProviderID(10-10)ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/track/track.go (1)
Evt(37-52)src/cmd/cli/command/commands.go (1)
P(54-54)src/pkg/cli/tail.go (1)
P(93-93)src/pkg/debug/debug.go (1)
P(21-21)
src/pkg/agent/tools/services.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/pkg/agent/tools/removeConfig.go (3)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/cli/connect.go (1)
Connect(17-19)src/pkg/stacks/manager.go (1)
NewManager(35-61)
src/pkg/agent/tools/setConfig.go (2)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/stacks/manager.go (1)
NewManager(35-61)
src/pkg/agent/tools/destroy_test.go (2)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/stacks/manager_test.go (5)
src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/stacks/stacks.go (6)
List(152-154)Create(91-93)Load(236-238)StackParameters(18-25)StackListItem(142-150)Directory(84-84)src/protos/io/defang/v1/fabric.pb.go (2)
Provider_AWS(33-33)Provider_GCP(35-35)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/stacks/selector_test.go (6)
src/pkg/stacks/stacks.go (6)
List(152-154)StackListItem(142-150)Load(236-238)StackParameters(18-25)LoadParameters(266-284)Create(91-93)src/pkg/stacks/selector.go (2)
CreateNewStack(15-15)NewSelector(22-27)src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)src/pkg/modes/modes.go (3)
Mode(12-12)ModeBalanced(17-17)ModeAffordable(16-16)src/pkg/elicitations/elicitations.go (1)
Controller(8-14)src/pkg/stacks/manager.go (1)
Manager(16-21)
src/pkg/cli/client/caniuse.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(11-38)src/pkg/cli/client/provider.go (1)
Provider(43-70)
src/pkg/stacks/stacks.go (2)
src/pkg/cli/client/provider_id.go (2)
ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/modes/modes.go (2)
Mode(12-12)ModeUnspecified(15-15)
src/cmd/cli/command/stack_test.go (3)
src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/cmd/cli/command/commands_test.go (1)
MockFabricControllerClient(228-232)src/pkg/stacks/stacks.go (1)
LoadParameters(266-284)
src/cmd/cli/command/compose.go (5)
src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (2)
NewController(35-40)Client(16-18)src/pkg/stacks/manager.go (1)
NewManager(35-61)
src/cmd/cli/command/commands.go (8)
src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/elicitations/elicitations.go (3)
NewController(35-40)Client(16-18)Controller(8-14)src/pkg/stacks/manager.go (2)
NewManager(35-61)Manager(16-21)src/pkg/stacks/stacks.go (3)
StackParameters(18-25)Load(236-238)List(152-154)src/pkg/stacks/selector.go (1)
NewSelector(22-27)src/pkg/cli/client/provider_id.go (3)
ProviderAuto(13-13)ProviderDefang(14-14)ProviderID(10-10)src/pkg/utils.go (1)
Getenv(48-53)src/pkg/cli/connect.go (1)
NewProvider(62-76)
src/pkg/agent/tools/destroy.go (3)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/logs.go (6)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/agent/tools/interfaces.go (1)
CLIInterface(14-32)src/pkg/agent/tools/default_tool_cli.go (1)
StackConfig(19-22)src/pkg/mcp/tools/tools.go (1)
StackConfig(14-17)src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/stacks/manager.go (4)
src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/timeutils/timeutils.go (1)
AsTime(39-44)src/pkg/cli/client/region.go (1)
GetRegion(5-16)
src/pkg/agent/tools/listConfig.go (4)
src/pkg/cli/client/provider.go (1)
Loader(72-76)src/pkg/cli/connect.go (1)
Connect(17-19)src/pkg/stacks/manager.go (1)
NewManager(35-61)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(26-33)
src/cmd/cli/command/commands_test.go (4)
src/protos/io/defang/v1/fabric.pb.go (14)
ListDeploymentsRequest(2992-2999)ListDeploymentsRequest(3012-3012)ListDeploymentsRequest(3027-3029)ListDeploymentsResponse(3052-3057)ListDeploymentsResponse(3070-3070)ListDeploymentsResponse(3085-3087)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)DeploymentMode_DEVELOPMENT(87-87)src/pkg/modes/modes.go (2)
Mode(12-12)ModeAffordable(16-16)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/stacks.go (3)
StackListItem(142-150)StackParameters(18-25)List(152-154)
src/pkg/stacks/stacks_test.go (3)
src/pkg/stacks/stacks.go (2)
StackParameters(18-25)ParamsFromMap(55-80)src/pkg/cli/client/provider_id.go (2)
ProviderAWS(15-15)ProviderGCP(17-17)src/pkg/modes/modes.go (3)
Mode(12-12)ModeAffordable(16-16)ModeBalanced(17-17)
🪛 GitHub Actions: Go package
src/cmd/cli/command/estimate.go
[error] 1-1: Go test failed for package src/cmd/cli/command (likely TestCommandGates).
src/cmd/cli/command/stack_test.go
[error] 1-1: Go test failed for package src/cmd/cli/command (likely TestCommandGates).
src/cmd/cli/command/compose.go
[error] 1-1: Go test failed for package src/cmd/cli/command (likely TestCommandGates).
src/cmd/cli/command/commands.go
[error] 1-1: Go test failed for package src/cmd/cli/command (likely TestCommandGates).
src/cmd/cli/command/commands_test.go
[error] 1-1: Go test failed for package src/cmd/cli/command (likely TestCommandGates).
⏰ 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 (36)
src/pkg/cli/client/mock.go (1)
175-177: LGTM!The mock implementation correctly implements the new
OutsideWorkingDirectory()method from theLoaderinterface. Returningfalseis appropriate for a simple mock.src/pkg/agent/tools/estimate.go (1)
20-20: LGTM!The parameter type change from
cliClient.ProjectLoadertocliClient.Loaderaligns with the interface consolidation across the PR. The function continues to work correctly with the updated type.src/pkg/stacks/stacks_test.go (2)
383-438: LGTM!The test thoroughly validates the
ToMap()conversion for both AWS and GCP stack parameters, checking all expected environment variable mappings.
440-491: LGTM!The test properly validates
ParamsFromMap()parsing for both GCP and AWS configurations, ensuring correct conversion from map toStackParameters.src/pkg/cli/client/caniuse.go (1)
10-22: LGTM!The refactor to remove the
stackparameter and useprovider.GetStackName()is clean and aligns with the stack-driven provider resolution pattern introduced in this PR. This simplifies the API by eliminating a redundant parameter.src/pkg/agent/tools/removeConfig.go (2)
21-21: LGTM!The parameter type change aligns with the broader
Loaderinterface refactoring across agent tools.
28-31: LGTM!The stack manager creation correctly uses the new three-parameter constructor and properly handles potential errors.
src/pkg/agent/tools/provider.go (1)
38-44: LGTM!The refactor to use
stacks.NewSelectorandSelectStackcleanly separates stack selection logic and aligns with the PR's goal of unified stack selection. The in-place update pattern is correct.src/pkg/agent/tools/deploy.go (3)
25-25: LGTM!The parameter type change is consistent with the
Loaderinterface consolidation across all agent tools.
47-50: LGTM!Stack manager creation follows the updated constructor pattern with proper error handling.
57-57: LGTM!The
CanIUseProvidercall correctly uses the updated signature without thestackparameter, consistent with the refactor incaniuse.go.src/cmd/cli/command/estimate.go (2)
31-40: LGTM!The interactive provider selection flow is clean and properly handles the auto-detection case with appropriate error wrapping.
69-104: LGTM!The
interactiveSelectProviderfunction is well-implemented with proper error handling, user prompt configuration, and telemetry tracking. The panic on line 71 is appropriate for an internal helper that should only be called with valid inputs.src/pkg/agent/tools/destroy_test.go (1)
60-66: LGTM!The
CanIUseProvidermock signature correctly aligns with the updated interface, placingproviderbeforeprojectNameand removing thestackNameparameter. The implementation properly logs the project name and returns the configured error.src/pkg/agent/tools/services.go (1)
23-38: LGTM!The signature update to use
cliClient.Loaderand the new three-argumentstacks.NewManagercall with proper error handling aligns with the broader refactor. The provider preparer correctly receives the stack manager.src/pkg/stacks/stacks.go (3)
27-53: LGTM!The
ToMap()method correctly serializesStackParametersto environment variable format. The conditional handling of region variable names based on provider and the optional fields (Mode, AWSProfile, GCPProjectID) is well-structured.
55-80: LGTM!The
ParamsFromMap()function correctly handles the reverse mapping. Unknown keys are silently ignored, which is appropriate for forward compatibility when new environment variables are added.
264-284: LGTM!The
LoadParametersfunction correctly reimplements the godotenv Load/Overload logic for map inputs. Theoverloadparameter properly controls whether existing environment variables should be overwritten.src/pkg/agent/tools/setConfig.go (1)
21-36: LGTM!The signature update and stack manager initialization follow the same pattern as other tool handlers, with proper error handling for the new
stacks.NewManagercall.src/pkg/agent/tools/listConfig.go (1)
20-35: LGTM!The updates to use
cliClient.Loaderand the three-argumentstacks.NewManagerwith proper error handling are consistent with the refactoring pattern across other tool handlers.src/pkg/agent/tools/logs.go (2)
25-56: LGTM!The signature update to
cliClient.Loader, the three-argumentstacks.NewManagercall with proper error handling, and the provider preparer setup are consistent with other tool handlers.
65-68: LGTM!The
CanIUseProvidercall correctly uses the updated argument order withproviderbeforeprojectName, matching the interface signature change across the codebase.src/cmd/cli/command/stack_test.go (1)
30-45: The mock setup is valid and follows Go's standard embedding pattern.
MockFabricControllerClientcorrectly embeds theFabricControllerClientinterface to satisfy the interface contract, and the test provides implementation fields (canIUseResponse,savedProvider) for test-specific behavior. ThemockClient.SetClient(mockCtrl)pattern is the correct injection approach for this codebase. If test failures occur in the pipeline, they are likely due to other factors, not the mock setup structure.Likely an incorrect or invalid review comment.
src/pkg/agent/tools/destroy.go (1)
20-48: LGTM! The destroy handler correctly integrates with the new stack-driven provider resolution.The changes properly:
- Create the stack manager with the correct parameters
- Handle the error from
NewManager- Use the updated
CanIUseProvidersignature that retrieves the stack name from the providersrc/pkg/stacks/selector_test.go (2)
84-123: Good test coverage for production stack selector.This test correctly uses
NewSelectorto test the actual production code path for selecting an existing stack. The mock setup and assertions are well-structured.
300-319: Thorough error handling test.Good coverage for the "elicitations not supported" error path, using the production
NewSelector.src/cmd/cli/command/compose.go (2)
223-252: LGTM! ThehandleExistingDeploymentsfunction properly handles the new stack-driven flow.The logic correctly:
- Filters deployments by provider first (line 225-227)
- Matches location by account ID and region (line 229)
- Creates a "beta" stack with appropriate parameters when deploying to a new location without an explicit stack
566-577: LGTM! Proper integration of elicitations and stack manager in compose config.The code correctly initializes the elicitation controller and stack manager for interactive stack selection in the
compose configcommand.src/pkg/stacks/manager_test.go (4)
18-31: Well-structured mock for the fabric client.The
mockFabricClientproperly implements theDeploymentListerinterface with configurable responses and error injection for testing various scenarios.
340-391: Good coverage for remote stack listing.The test properly verifies:
- Remote stacks are retrieved from the fabric client
- Deployment timestamps are correctly mapped to
DeployedAt- Multiple remote stacks are handled
511-543: Important test for implicit "beta" stack handling.This test verifies the core PR objective: deployments with empty stack names are treated as the implicit "beta" stack, enabling onboarding for existing projects.
640-716: Comprehensive test for cross-directory access restriction.The test properly verifies that when the working directory differs from the target directory:
CreateandLoadoperations are blocked with appropriate error messagesListstill returns remote stacks (not blocked)- Remote operations (
ListRemote) work regardless of directoryThis aligns with the PR objective to restrict cross-project stack file access.
src/cmd/cli/command/commands.go (3)
1460-1495: LGTM! ThenewProviderCheckedfunction properly handles nil loader and working directory logic.The function correctly:
- Handles nil loader case (lines 1464-1470)
- Creates stack manager with appropriate target directory based on
OutsideWorkingDirectory()- Validates the provider by calling
AccountInfo
1340-1354: Good deprecation messaging.The warning correctly indicates the flag is "deprecated" rather than "ignored", and provides guidance to use
--stackinstead. The flag value is still honored for backward compatibility.
1301-1407: Review comment appears incomplete and lacks substantiation.The original review references bugs on lines 1363 and 1379 in the
getStackfunction related to provider auto-selection, but provides no concrete explanation of what these bugs are or how they manifest. Additionally, the claimed connection to pipeline failures inTestCommandGatescannot be verified—this test name does not appear in the Defang CLI repository.To properly evaluate this review:
- Define which specific conditions are incorrect or problematic
- Include reproducible test cases showing the failure
- Verify whether
TestCommandGatesactually exists or provide the correct test name- Explain the concrete impact of each identified issue
src/cmd/cli/command/commands_test.go (1)
148-226: Unable to verify TestCommandGates pipeline failure without actual error output.The test structure appears sound:
t.Chdir()is correctly placed at the parent level before sequential subtests, avoiding parallel execution conflicts. Without the specific error message from the pipeline, I cannot confirm whether a failure exists or what the root cause is. If a failure does occur, run the test locally with verbose output to capture the actual error before proceeding with fixes.
| // If we have expected provider/region (from old NewMockStackManager usage), create default params | ||
| if m.expectedProvider != "" && m.expectedRegion != "" { | ||
| params := stacks.StackParameters{ | ||
| Name: name, | ||
| Provider: m.expectedProvider, | ||
| Region: m.expectedRegion, | ||
| Mode: modes.ModeAffordable, | ||
| } | ||
| stacks.LoadParameters(params.ToMap(), true) | ||
| return ¶ms, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for LoadParameters function definition
rg -n -B2 -A8 'func.*LoadParameters' --type=go src/
# Also search for how it's used in the test
rg -n 'LoadParameters' --type=go src/cmd/cli/command/Repository: DefangLabs/defang
Length of output: 4315
🏁 Script executed:
#!/bin/bash
# Search for context around Load() calls in commands_test.go
rg -n -B5 -A10 'mockStackManager.*Load' src/cmd/cli/command/commands_test.go | head -80
# Also check if there are any t.Setenv or t.Cleanup patterns
rg -n 't\.Setenv|t\.Cleanup|defer.*Unsetenv' --type=go src/cmd/cli/command/commands_test.goRepository: DefangLabs/defang
Length of output: 1392
Wrap LoadParameters calls with proper test isolation to prevent environment variable pollution between tests.
Line 332 calls stacks.LoadParameters(params.ToMap(), true), which sets environment variables using os.Setenv() without cleanup. These variables will persist across sequential test runs, potentially causing test interference. Use t.Setenv() and t.Cleanup() to properly isolate environment variable changes, consistent with the patterns used elsewhere in this test file.
🤖 Prompt for AI Agents
In src/cmd/cli/command/commands_test.go around lines 324 to 334, the call to
stacks.LoadParameters(params.ToMap(), true) sets process-level environment
variables via os.Setenv without cleanup; replace that with per-key t.Setenv(...)
calls (or iterate params.ToMap() and call t.Setenv(key, value) for each) so the
test runner will automatically restore environment variables, and remove the
direct LoadParameters call (or call it only with a map built from t.Getenv-safe
values) while ensuring any temporary state is reverted with t.Cleanup where
needed to avoid cross-test pollution.
| term.Debugf("AccountInfo failed: %v", err) | ||
| } else if len(resp.Deployments) > 0 { | ||
| handleExistingDeployments(resp.Deployments, accountInfo, project.Name) | ||
| handleExistingDeployments(resp.Deployments, accountInfo, project.Name, provider.GetStackName()) |
There was a problem hiding this comment.
Return value from handleExistingDeployments is ignored.
The function returns an error, but the call site doesn't check it. If confirmDeploymentToNewLocation fails or the user cancels, the deployment will proceed anyway.
🔎 Proposed fix
} else if len(resp.Deployments) > 0 {
- handleExistingDeployments(resp.Deployments, accountInfo, project.Name, provider.GetStackName())
+ if err := handleExistingDeployments(resp.Deployments, accountInfo, project.Name, provider.GetStackName()); err != nil {
+ return err
+ }
} else if global.Stack.Name == "" {📝 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.
| handleExistingDeployments(resp.Deployments, accountInfo, project.Name, provider.GetStackName()) | |
| } else if len(resp.Deployments) > 0 { | |
| if err := handleExistingDeployments(resp.Deployments, accountInfo, project.Name, provider.GetStackName()); err != nil { | |
| return err | |
| } | |
| } else if global.Stack.Name == "" { |
🤖 Prompt for AI Agents
In src/cmd/cli/command/compose.go around line 117, the call to
handleExistingDeployments(resp.Deployments, accountInfo, project.Name,
provider.GetStackName()) ignores its returned error; change the call to capture
the error, check it, and return or abort the current function on non-nil (e.g.,
if err := handleExistingDeployments(...); err != nil { return err } or otherwise
handle cancellation) so that failures from confirmDeploymentToNewLocation or
user cancellation stop the deployment from proceeding.
| os.WriteFile( | ||
| "compose.yaml", | ||
| []byte(`services: | ||
| web: | ||
| image: nginx`), | ||
| os.FileMode(0644), | ||
| ) | ||
| for _, stack := range tt.stacks { | ||
| stacks.Create(stack) | ||
| } |
There was a problem hiding this comment.
Unchecked errors in test setup may cause silent failures.
The os.WriteFile and stacks.Create calls don't check for errors. If these fail, tests will proceed with incorrect state, potentially causing confusing failures or false passes.
🔎 Proposed fix
- os.WriteFile(
+ err := os.WriteFile(
"compose.yaml",
[]byte(`services:
web:
image: nginx`),
os.FileMode(0644),
)
+ assert.NoError(t, err, "failed to create compose.yaml")
for _, stack := range tt.stacks {
- stacks.Create(stack)
+ _, err := stacks.Create(stack)
+ assert.NoError(t, err, "failed to create stack")
}🤖 Prompt for AI Agents
In src/cmd/cli/command/stack_test.go around lines 96 to 105, the test setup
calls os.WriteFile and stacks.Create without checking their return errors;
update the code to capture and handle those errors (e.g., if err :=
os.WriteFile(...); err != nil { t.Fatalf("write compose.yaml: %v", err) } and
for each stack call capture the error returned by stacks.Create and fail the
test on error (or use require.NoError/require.NoErrorf if using testify) so the
test stops immediately on setup failures.
| func TestLoadParameters(t *testing.T) { | ||
| params := map[string]string{ | ||
| "DEFANG_PROVIDER": "aws", | ||
| "AWS_REGION": "us-west-2", | ||
| "AWS_PROFILE": "default", | ||
| "DEFANG_MODE": "AFFORDABLE", | ||
| } | ||
|
|
||
| // Clear any existing env vars that might interfere with the test | ||
| os.Unsetenv("DEFANG_PROVIDER") | ||
| os.Unsetenv("AWS_REGION") | ||
| os.Unsetenv("AWS_PROFILE") | ||
| os.Unsetenv("DEFANG_MODE") | ||
|
|
||
| err := stacks.LoadParameters(params, true) | ||
| if err != nil { | ||
| t.Fatalf("LoadParameters() error = %v", err) | ||
| } | ||
|
|
||
| assert.Equal(t, "aws", os.Getenv("DEFANG_PROVIDER")) | ||
| assert.Equal(t, "us-west-2", os.Getenv("AWS_REGION")) | ||
| assert.Equal(t, "default", os.Getenv("AWS_PROFILE")) | ||
| assert.Equal(t, "AFFORDABLE", os.Getenv("DEFANG_MODE")) | ||
| } |
There was a problem hiding this comment.
Test modifies environment variables without cleanup.
TestLoadParameters sets environment variables that persist after the test completes. This can cause test pollution and flaky behavior when tests run in parallel or in a specific order. Add cleanup to restore the original state.
🔎 Proposed fix
func TestLoadParameters(t *testing.T) {
params := map[string]string{
"DEFANG_PROVIDER": "aws",
"AWS_REGION": "us-west-2",
"AWS_PROFILE": "default",
"DEFANG_MODE": "AFFORDABLE",
}
// Clear any existing env vars that might interfere with the test
os.Unsetenv("DEFANG_PROVIDER")
os.Unsetenv("AWS_REGION")
os.Unsetenv("AWS_PROFILE")
os.Unsetenv("DEFANG_MODE")
+
+ // Restore env vars after test
+ t.Cleanup(func() {
+ os.Unsetenv("DEFANG_PROVIDER")
+ os.Unsetenv("AWS_REGION")
+ os.Unsetenv("AWS_PROFILE")
+ os.Unsetenv("DEFANG_MODE")
+ })
err := stacks.LoadParameters(params, true)📝 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 TestLoadParameters(t *testing.T) { | |
| params := map[string]string{ | |
| "DEFANG_PROVIDER": "aws", | |
| "AWS_REGION": "us-west-2", | |
| "AWS_PROFILE": "default", | |
| "DEFANG_MODE": "AFFORDABLE", | |
| } | |
| // Clear any existing env vars that might interfere with the test | |
| os.Unsetenv("DEFANG_PROVIDER") | |
| os.Unsetenv("AWS_REGION") | |
| os.Unsetenv("AWS_PROFILE") | |
| os.Unsetenv("DEFANG_MODE") | |
| err := stacks.LoadParameters(params, true) | |
| if err != nil { | |
| t.Fatalf("LoadParameters() error = %v", err) | |
| } | |
| assert.Equal(t, "aws", os.Getenv("DEFANG_PROVIDER")) | |
| assert.Equal(t, "us-west-2", os.Getenv("AWS_REGION")) | |
| assert.Equal(t, "default", os.Getenv("AWS_PROFILE")) | |
| assert.Equal(t, "AFFORDABLE", os.Getenv("DEFANG_MODE")) | |
| } | |
| func TestLoadParameters(t *testing.T) { | |
| params := map[string]string{ | |
| "DEFANG_PROVIDER": "aws", | |
| "AWS_REGION": "us-west-2", | |
| "AWS_PROFILE": "default", | |
| "DEFANG_MODE": "AFFORDABLE", | |
| } | |
| // Clear any existing env vars that might interfere with the test | |
| os.Unsetenv("DEFANG_PROVIDER") | |
| os.Unsetenv("AWS_REGION") | |
| os.Unsetenv("AWS_PROFILE") | |
| os.Unsetenv("DEFANG_MODE") | |
| // Restore env vars after test | |
| t.Cleanup(func() { | |
| os.Unsetenv("DEFANG_PROVIDER") | |
| os.Unsetenv("AWS_REGION") | |
| os.Unsetenv("AWS_PROFILE") | |
| os.Unsetenv("DEFANG_MODE") | |
| }) | |
| err := stacks.LoadParameters(params, true) | |
| if err != nil { | |
| t.Fatalf("LoadParameters() error = %v", err) | |
| } | |
| assert.Equal(t, "aws", os.Getenv("DEFANG_PROVIDER")) | |
| assert.Equal(t, "us-west-2", os.Getenv("AWS_REGION")) | |
| assert.Equal(t, "default", os.Getenv("AWS_PROFILE")) | |
| assert.Equal(t, "AFFORDABLE", os.Getenv("DEFANG_MODE")) | |
| } |
🤖 Prompt for AI Agents
In src/cmd/cli/command/stack_test.go around lines 169 to 192, the test mutates
environment variables and never restores them, causing test pollution; capture
the original values of DEFANG_PROVIDER, AWS_REGION, AWS_PROFILE, and DEFANG_MODE
at the start of the test and defer restoring them (or use t.Setenv for each on
Go 1.17+ which handles cleanup) so the environment is returned to its previous
state after the test completes; ensure the defer restores even when
LoadParameters returns an error.
| // WizardInterface defines the interface for collecting stack parameters | ||
| type WizardInterface interface { | ||
| CollectParameters(ctx context.Context) (*StackParameters, error) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if WizardInterface or similar wizard abstraction exists in production code
rg -n "WizardInterface|CollectParameters" --type=go -g '!*_test.go' src/pkg/stacks/Repository: DefangLabs/defang
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Read the production selector.go to understand how it uses the wizard
echo "=== selector.go ==="
cat -n src/pkg/stacks/selector.go | head -60
echo -e "\n=== wizard.go around line 34 ==="
head -n 40 src/pkg/stacks/wizard.go | tail -n 15
echo -e "\n=== selector_test.go around WizardInterface (lines 120-140) ==="
sed -n '120,150p' src/pkg/stacks/selector_test.goRepository: DefangLabs/defang
Length of output: 3391
Move WizardInterface to production code and inject it into stackSelector.
The WizardInterface is currently defined only in the test file (lines 125-128), but production code in selector.go directly instantiates NewWizard() (line 39) and calls CollectParameters() on the concrete type. This couples production code to a concrete implementation, violating the dependency inversion principle.
Define the interface in production (stacks package) and accept it as a dependency in stackSelector (via constructor or method parameter) so that both production and tests depend on the abstraction rather than a concrete type. This eliminates the need for test workarounds like testableStackSelector and makes the code more flexible and testable.
🤖 Prompt for AI Agents
In src/pkg/stacks/selector_test.go lines 125-128, the WizardInterface is defined
only in the test, but selector.go instantiates NewWizard() directly (line ~39)
which couples production code to a concrete implementation; move the
WizardInterface definition into the production package (e.g.,
src/pkg/stacks/wizard.go or selector.go), change stackSelector to accept a
WizardInterface dependency (via its constructor or as a parameter) instead of
calling NewWizard(), update the selector constructor to take and store the
interface, update selector.go to call the injected interface's
CollectParameters, and update tests to provide a test double implementing the
interface (removing test-only workarounds like testableStackSelector).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/pkg/stacks/manager.go (1)
47-54: Symlink-relative paths may still cause incorrectoutsidedetection.The comparison
workingDirectory != absTargetDirectoryat line 53 doesn't resolve symlinks. If either path contains symlinks, two paths pointing to the same physical directory may compare as different, incorrectly settingoutside = trueand blocking local operations. This was flagged in a previous review.src/cmd/cli/command/stack_test.go (1)
96-105: Unchecked errors in test setup may cause silent failures.The
os.WriteFileandstacks.Createcalls don't check for errors. If these fail, tests will proceed with incorrect state, potentially causing confusing failures or false passes.🔎 Proposed fix
- os.WriteFile( + err := os.WriteFile( "compose.yaml", []byte(`services: web: image: nginx`), os.FileMode(0644), ) + if err != nil { + t.Fatalf("failed to create compose.yaml: %v", err) + } for _, stack := range tt.stacks { - stacks.Create(stack) + _, err := stacks.Create(stack) + if err != nil { + t.Fatalf("failed to create stack: %v", err) + } }src/cmd/cli/command/commands_test.go (1)
324-337: mockStackManager.Load sets environment variables without cleanup.Line 332 calls
stacks.LoadParameters(params.ToMap(), true), which sets environment variables usingos.Setenv()without cleanup. These variables will persist across sequential test runs, potentially causing test interference. Consider usingt.Setenv()in the test setup instead of relying onLoadParametersin the mock.src/cmd/cli/command/commands.go (2)
1311-1313: Typo in comment: "RoomCmd" should be "RootCmd".The comment references "RoomCmd" but should reference "RootCmd".
🔎 Proposed fix
// This code unfortunately replicates the provider precedence rules in the - // RoomCmd's PersistentPreRunE func, I think we should avoid reading the + // RootCmd's PersistentPreRunE func, I think we should avoid reading the // stack file during startup, and only read it here instead.
1340-1354: Deprecation warning followed by usage is appropriate for backwards compatibility.The code warns that
--providerflag is deprecated, then proceeds to use its value. While this was flagged in a previous review as "contradictory," this is actually the correct behavior for deprecation:
- Warn the user the flag is deprecated
- Still honor the flag value for backwards compatibility
The warning message now includes a link to documentation about stacks, which helps guide users to the new approach.
🧹 Nitpick comments (4)
src/pkg/stacks/manager.go (2)
106-107: Duplicate comment.There's a duplicate comment on lines 106-107.
🔎 Proposed fix
// sort stacks by name asc - // sort stacks by name asc slices.SortFunc(stackList, func(a, b StackListItem) int {
108-116: Consider usingstrings.Comparefor cleaner sort logic.The sort comparison can be simplified using the standard library's
strings.Comparefunction.🔎 Proposed fix
+ "strings"slices.SortFunc(stackList, func(a, b StackListItem) int { - if a.Name < b.Name { - return -1 - } - if a.Name > b.Name { - return 1 - } - return 0 + return strings.Compare(a.Name, b.Name) })src/cmd/cli/command/commands_test.go (2)
744-748: Environment variable cleanup may be incomplete.When
tc.envProvideris empty,os.Unsetenv("DEFANG_PROVIDER")is called, but this doesn't restore the original value if one existed before the test. Consider usingt.Setenvwhich handles this automatically.🔎 Proposed fix
// Set environment variable if provided if tc.envProvider != "" { t.Setenv("DEFANG_PROVIDER", tc.envProvider) - } else { - os.Unsetenv("DEFANG_PROVIDER") }Note:
t.Setenvautomatically restores the original value after the test, but it doesn't unset variables. If you need to ensureDEFANG_PROVIDERis unset for certain tests, consider setting up a parent test that usest.Setenv("DEFANG_PROVIDER", "")or restructuring the test to not depend on the absence of this variable.
800-808: Warning verification is incomplete.The comment acknowledges that warning output cannot be easily captured, but the verification logic on lines 803-805 checks if the provider flag was "changed" rather than actually verifying warnings were emitted. This test assertion will likely always pass since
ParseFlagswith--providerwould mark it as changed.Consider either removing this incomplete assertion or using a captured output buffer to verify warnings are actually printed.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cmd/cli/command/commands.go(5 hunks)src/cmd/cli/command/commands_test.go(9 hunks)src/cmd/cli/command/stack_test.go(5 hunks)src/pkg/stacks/manager.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmd/cli/command/stack_test.go (4)
src/cmd/cli/command/commands.go (2)
RootCmd(408-513)Execute(71-156)src/pkg/cli/client/grpc.go (1)
GrpcClient(19-26)src/cmd/cli/command/commands_test.go (1)
MockFabricControllerClient(228-232)src/pkg/stacks/stacks.go (1)
LoadParameters(266-284)
src/cmd/cli/command/commands_test.go (3)
src/pkg/modes/modes.go (3)
Mode(12-12)ModeAffordable(16-16)ModeUnspecified(15-15)src/pkg/cli/client/provider_id.go (4)
ProviderID(10-10)ProviderAWS(15-15)ProviderAuto(13-13)ProviderDefang(14-14)src/pkg/stacks/stacks.go (1)
StackParameters(18-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). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (13)
src/pkg/stacks/manager.go (3)
155-164: Mode assignment is now included.The
Modefield is now being assigned usingmodes.Mode(deployment.GetMode())at line 160, addressing the previous review comment about missing Mode assignment.
64-119: List method implementation looks correct.The
Listmethod properly merges remote and local stacks, preferring remote entries when duplicates exist to show deployment timestamps. The error handling forOutsideErrorallows graceful degradation when operating outside the working directory.
197-199: LoadParameters delegates but ignores the receiver's context.The
LoadParametersmethod on the manager simply delegates to the package-levelLoadParametersfunction without using the manager'stargetDirectoryoroutsidestate. This is consistent with the design but may be surprising if callers expect directory-scoped behavior.Consider whether this method should also check
sm.outsidelikeLoadandCreatedo, or document that it operates on environment variables globally regardless of directory context.src/cmd/cli/command/stack_test.go (2)
169-200: Test properly cleans up environment variables.The
TestLoadParameterstest now includes proper cleanup of environment variables using adeferblock. This addresses the previous review comment about test pollution.Consider using
t.Setenvinstead, which automatically handles cleanup:t.Setenv("DEFANG_PROVIDER", "aws") t.Setenv("AWS_REGION", "us-west-2") // etc.
30-58: Test setup properly saves and restores global state.The test correctly saves
RootCmdandglobal.Clientand restores them in adeferblock. The mock setup creates a minimal RootCmd with required flags for the stack list flow.src/cmd/cli/command/commands_test.go (3)
249-254: Good defensive initialization of savedProvider map.The
SetSelectedProvidermethod now properly initializes thesavedProvidermap before use, preventing nil map assignment panics.
476-505: Mock elicitations controller implementation is well-structured.The
mockElicitationsControllerprovides appropriate test doubles for the elicitations interface, with configurable behavior viaenumChoiceandisSupportedfields.
507-718: Comprehensive test coverage for getStack.
TestGetStackprovides good coverage of various scenarios including:
- Stack flag with valid/invalid stacks
- Auto provider handling
- Provider flag deprecation warnings
- Non-interactive defaults
- Single stack auto-selection
- Interactive selection
- Error propagation
src/cmd/cli/command/commands.go (5)
1362-1381: Single stack auto-selection logic is now correct.The condition on line 1363 now properly handles
ProviderAutoby checkingstack.Provider == cliClient.ProviderAuto || knownStacks[0].Provider == stack.Provider.String(). This addresses the previous review comment about auto-select not triggering when expected.The fallback to
importStackwhen the stack file cannot be loaded (lines 1366-1377) provides good resilience for stacks that exist remotely but not locally.
1401-1426: importStack properly converts StackListItem to StackParameters.The function correctly:
- Validates and converts the provider string to ProviderID
- Handles optional Mode field
- Loads parameters into the environment
- Returns the constructed StackParameters
1459-1477: newProvider now properly integrates stack selection.The refactored
newProviderfunction:
- Calls
getStackto resolve stack parameters- Updates
global.Stackwith the resolved stack- Prints informative messages about which stack is being used
- Warns about provider mismatches in environment
The informational message on line 1472 clearly indicates the stack name, provider, and source (whence), which helps with debugging and user understanding.
1479-1514: newProviderChecked properly handles nil loader.The function now correctly handles
loader == nilby defaultingoutside = trueand only calling loader methods when loader is non-nil. This addresses the previous review comment about nil pointer dereference.
557-572: whoami command creates stacks.Manager correctly.The whoami command now properly initializes the elicitations controller and stacks manager before calling
newProvider. The working directory is correctly obtained and passed toNewManager.
Description
This PR improves the onboarding experience for stacks. Special care is given to users with existing deployments to the implicit "beta" stack.
We are also care to support using stacks outside the working directory of the project with which the stack is associated.
stacks.Selectorwhich uses the elicitations controller, and the stacks manager to implement stack with survey for defang subcommand invocation. This allows us to unify the codepaths for stack selection across agent tool calls and subcommand invocationsStack, assume the stack was"beta".defangdirectory"beta", show a message introducing users to stacks and to the implicit "beta" stackLinked Issues
Checklist
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.