Stacks onboarding with support for "remote" stacks #1712
Conversation
WalkthroughProject-name resolution is moved earlier; provider setup now takes projectName and useWkDir. A StacksManager and interactive stack selection/import flow were added. Many CLI and agent tool interfaces switched to FabricClient and LoadProjectName. Documentation removed the "auto" DEFANG_PROVIDER option and updated a macOS FAQ link. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI Command
participant Loader as Loader
participant PP as ProviderPreparer
participant StackMgr as StacksManager
participant API as Fabric API
CLI->>Loader: LoadProjectName(ctx)
Loader-->>CLI: projectName
CLI->>PP: SetupProvider(ctx, projectName, stack?, useWkDir)
PP->>StackMgr: collectPreviouslyDeployedStacks(projectName)
StackMgr->>API: ListDeployments(projectName)
API-->>StackMgr: deployments
StackMgr->>StackMgr: merge local & remote stacks
alt selected remote stack not present locally
StackMgr->>StackMgr: import/create local stack from remote
end
StackMgr-->>PP: selected/imported stack parameters
PP->>API: Initialize provider using stack parameters
API-->>PP: providerID, provider
PP-->>CLI: provider (or error)
CLI->>API: Execute command using provider
API-->>CLI: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/cmd/cli/command/compose.go (1)
78-81: Consider aligningsaveStacksToWkDirbehavior forcompose upwith other commands
compose upnow always callsnewProviderChecked(ctx, project.Name, true), whilecompose down,ps, andlogsderivesaveStacksToWkDirfrom the--project-nameflag. If the boolean controls persisting stack selection to.defang, you may wantupto respect an explicit--project-namein the same way for ephemeral project names, or at least document thatupalways persists while the others don’t.src/pkg/agent/tools/logs.go (1)
79-83: Avoid double-wrapping the “failed to fetch logs” error
erris first wrapped withfmt.Errorf("failed to fetch logs: %w", err)and then wrapped again in the return, producing messages likefailed to fetch logs: failed to fetch logs: <root>. You can simplify to a single wrap and reuse that for both logging and returning:- if err != nil { - err = fmt.Errorf("failed to fetch logs: %w", err) - term.Error("Failed to fetch logs", "error", err) - return "", fmt.Errorf("failed to fetch logs: %w", err) - } + if err != nil { + wrapped := fmt.Errorf("failed to fetch logs: %w", err) + term.Error("Failed to fetch logs", "error", wrapped) + return "", wrapped + }src/pkg/agent/tools/listConfig.go (1)
23-23: Minor: Inconsistent error message casing.Line 23 uses
"Could not connect"(capitalized) while lines 28 and 34 use lowercase ("failed to load...","failed to setup..."). Consider using consistent casing across error messages for uniformity.- return "", fmt.Errorf("Could not connect: %w", err) + return "", fmt.Errorf("could not connect: %w", err)src/pkg/agent/tools/provider.go (3)
127-128: Consider more robust argument handling.Using
os.Args[1]directly to construct the help message could be fragile if the argument structure changes (e.g., with global flags, subcommands). Consider using the command name from context if available.- executable, _ := os.Executable() - term.Infof("To skip this prompt, run %s %s --stack=%s", filepath.Base(executable), os.Args[1], "<stack_name>") + executable, _ := os.Executable() + cmdName := "command" + if len(os.Args) > 1 { + cmdName = os.Args[1] + } + term.Infof("To skip this prompt, run %s %s --stack=%s", filepath.Base(executable), cmdName, "<stack_name>")
148-170: Verify robustness of remote stack name parsing.The code assumes remote stacks are formatted as "name (last deployed...)" and parses by finding parentheses. This approach could be fragile if:
- Stack names themselves contain parentheses
- The format changes
- The timestamp format varies
Consider adding validation or using a more structured approach to distinguish remote from local stacks.
A more robust approach could use a separate data structure to track whether each option is remote:
- stackNames := make([]string, 0, len(stackMap)+1) + type stackChoice struct { + displayName string + actualName string + isRemote bool + } + stackChoices := make([]stackChoice, 0, len(stackMap)+1) for _, stackOption := range stackMap { - name := stackOption.Name + displayName := stackOption.Name if !stackOption.Local { - name += fmt.Sprintf(" (last deployed %s)", stackOption.LastDeployedAt.Local().Format(time.RFC822)) + displayName += fmt.Sprintf(" (last deployed %s)", stackOption.LastDeployedAt.Local().Format(time.RFC822)) } - stackNames = append(stackNames, name) + stackChoices = append(stackChoices, stackChoice{ + displayName: displayName, + actualName: stackOption.Name, + isRemote: !stackOption.Local, + }) }Then look up the choice by display name to get the actual name and remote status, avoiding string parsing.
336-356: Minor redundancy: AWS_PROFILE check already performed.Line 336 checks
if os.Getenv("AWS_PROFILE") == "", but this condition is already guaranteed by the early return at lines 327-329. The nested check is unnecessary.- // TODO: add support for aws sso strategy - if os.Getenv("AWS_PROFILE") == "" { - knownProfiles, err := listAWSProfiles() - if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) - } - if len(knownProfiles) > 0 { - const useAccessKeysOption = "Use Access Key ID and Secret Access Key" - knownProfiles = append(knownProfiles, useAccessKeysOption) - profile, err := pp.ec.RequestEnum(ctx, "Select your profile", "profile_name", knownProfiles) - if err != nil { - return fmt.Errorf("failed to elicit AWS Profile Name: %w", err) - } - if profile != useAccessKeysOption { - err := os.Setenv("AWS_PROFILE", profile) - if err != nil { - return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) - } - return nil - } + // TODO: add support for aws sso strategy + knownProfiles, err := listAWSProfiles() + if err != nil { + return fmt.Errorf("failed to list AWS profiles: %w", err) + } + if len(knownProfiles) > 0 { + const useAccessKeysOption = "Use Access Key ID and Secret Access Key" + knownProfiles = append(knownProfiles, useAccessKeysOption) + profile, err := pp.ec.RequestEnum(ctx, "Select your profile", "profile_name", knownProfiles) + if err != nil { + return fmt.Errorf("failed to elicit AWS Profile Name: %w", err) + } + if profile != useAccessKeysOption { + err := os.Setenv("AWS_PROFILE", profile) + if err != nil { + return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) + } + return nil } }src/cmd/cli/command/commands.go (2)
438-439: TODO noted: Remote stack loading enhancement.This TODO aligns with the PR objectives. Would you like me to open an issue to track this enhancement for loading stacks from remote after connecting to fabric?
959-964: Provider result is discarded but function called for side effects.The provider returned from
newProviderCheckedis assigned to_(unused), suggesting it's called only for its side effects (setting upglobal.ProviderIDandglobal.Stack). Consider either using the returned provider or extracting the setup logic into a separate function that makes the intent clearer.- _, err = newProviderChecked(ctx, project.Name, saveStacksToWkDir) + // Setup provider to initialize global.ProviderID and global.Stack + _, err = newProviderChecked(ctx, project.Name, saveStacksToWkDir)Alternatively, if
debug.NewDebuggercould accept the provider directly, that would be cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
README.md(1 hunks)pkgs/npm/README.md(1 hunks)src/.goreleaser.yml(1 hunks)src/README.md(1 hunks)src/cmd/cli/command/cd.go(5 hunks)src/cmd/cli/command/commands.go(14 hunks)src/cmd/cli/command/commands_test.go(6 hunks)src/cmd/cli/command/compose.go(5 hunks)src/cmd/cli/command/globals.go(0 hunks)src/cmd/cli/command/globals_test.go(0 hunks)src/pkg/agent/tools/default_tool_cli.go(1 hunks)src/pkg/agent/tools/deploy.go(1 hunks)src/pkg/agent/tools/deploy_test.go(0 hunks)src/pkg/agent/tools/destroy.go(1 hunks)src/pkg/agent/tools/destroy_test.go(4 hunks)src/pkg/agent/tools/estimate_test.go(1 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/listConfig_test.go(2 hunks)src/pkg/agent/tools/logs.go(1 hunks)src/pkg/agent/tools/provider.go(5 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(2 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/services_test.go(3 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/agent/tools/setConfig_test.go(2 hunks)src/pkg/cli/client/provider_id.go(1 hunks)src/pkg/migrate/heroku.go(1 hunks)
💤 Files with no reviewable changes (3)
- src/cmd/cli/command/globals.go
- src/pkg/agent/tools/deploy_test.go
- src/cmd/cli/command/globals_test.go
🧰 Additional context used
🧬 Code graph analysis (7)
src/pkg/agent/tools/services.go (1)
src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/services_test.go (3)
src/pkg/cli/client/grpc.go (1)
GrpcClient(19-24)src/pkg/agent/tools/setConfig_test.go (1)
MockProvider(57-59)src/pkg/cli/client/playground.go (1)
PlaygroundProvider(17-21)
src/pkg/agent/tools/destroy.go (2)
src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/provider.go (2)
src/pkg/stacks/stacks.go (6)
StackParameters(17-22)List(85-118)Create(33-76)Read(180-195)Load(197-208)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)
src/cmd/cli/command/commands_test.go (1)
src/protos/io/defang/v1/fabric.pb.go (9)
ListDeploymentsRequest(3197-3204)ListDeploymentsRequest(3217-3217)ListDeploymentsRequest(3232-3234)ListDeploymentsResponse(3257-3262)ListDeploymentsResponse(3275-3275)ListDeploymentsResponse(3290-3292)Deployment(3035-3050)Deployment(3063-3063)Deployment(3078-3080)
src/pkg/agent/tools/listConfig.go (2)
src/pkg/term/colorizer.go (2)
Errorf(322-324)Debug(294-296)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/cmd/cli/command/commands.go (5)
src/pkg/cli/client/provider.go (1)
Provider(43-69)src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/elicitations/elicitations.go (2)
Client(14-16)NewController(32-36)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
⏰ 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 (52)
src/pkg/migrate/heroku.go (1)
336-342: Doc comment formatting improves clarity.The blank lines separating the purpose, implementation details, and return/error information make the documentation more readable and scannable.
src/pkg/cli/client/provider_id.go (1)
70-79: Error message now correctly reflects user-selectable providersUsing
AllProviders()in the error keeps the message aligned with the non-autoproviders exposed to users, while preserving the existing matching logic inSet. This is a clean, behaviorally safe UX improvement.src/.goreleaser.yml (1)
91-91: No action needed—the FAQ link update is isolated and requires no consistency fixes.The codebase contains only one FAQ documentation link (in this file at line 91), which is already using the updated structure
/docs/intro/faq/questions. No other outdated FAQ links exist elsewhere in the codebase, so there are no consistency issues to address.src/pkg/agent/tools/estimate_test.go (1)
124-124: Test expectation correctly reflects implementation changes.The error message update is accurate—
AllProviders()explicitly excludes "auto" from the returned list by slicing from index 1, so the error message will properly displayprovider not one of [defang aws digitalocean gcp]as expected by the test.README.md (1)
175-175: DEFANG_PROVIDER docs aligned with supported providersLimiting documented values to
aws,digitalocean,gcp, anddefang(droppingauto) matches the refactor and avoids implying unsupported behavior.pkgs/npm/README.md (1)
45-45: Keep npm package docs consistent with core READMEThis update mirrors the root README and correctly documents the allowed
DEFANG_PROVIDERvalues.src/cmd/cli/command/compose.go (3)
321-331: Project-name-first flow forcompose downlooks soundResolving
projectNamevia the loader before instantiating the provider and then gatingsaveStacksToWkDiron whether--project-namewas supplied gives clearer error paths and avoids unnecessary provider setup when the project name can’t be determined.
475-483:compose psprovider initialization now matchesdownsemanticsUsing
LoadProjectNameplus theproject-nameflag to controlsaveStacksToWkDirkeepspsconsistent withdownand should make stack resolution and any.defangwrites predictable across commands.
600-608: Logs command now participates in the unified provider/project-name flowHaving
logsresolveprojectNamethrough the loader and callnewProviderCheckedwith the samesaveStacksToWkDirheuristic asdown/psensures log tailing uses the same stack selection and provider onboarding behavior as deployment commands.src/README.md (1)
45-45: Internal README correctly mirrors provider optionsThe updated
DEFANG_PROVIDERdescription is consistent with the other READMEs and the removal of theautooption.src/pkg/agent/tools/logs.go (1)
48-56: Logs tool now correctly reuses shared project-name and provider setup flowLoading the project name via
cli.LoadProjectNameand delegating provider selection toNewProviderPreparer.SetupProviderbrings the logs tool in line with the unified onboarding/stack-selection behavior used elsewhere, and the debug log on the resolved project name should help when troubleshooting.src/pkg/agent/tools/interfaces.go (1)
30-30: CLIInterface exposes the simplifiedLoadProjectNamecontractAdding
LoadProjectName(ctx, loader)toCLIInterfacematches the refactor to resolve the project name before provider creation and aligns with the new DefaultToolCLI and tests.src/pkg/agent/tools/listConfig_test.go (1)
41-47: Tests correctly exercise the newLoadProjectName-first flowMocking
LoadProjectNameand asserting the call sequenceConnect → LoadProjectName → NewProvider → ListConfigensures the list-config tool uses the updated project-name resolution before provider creation, matching the new production behavior.Also applies to: 154-161
src/pkg/agent/tools/default_tool_cli.go (1)
90-92: DefaultToolCLI’sLoadProjectNamedelegation is straightforward and correctDelegating to
loader.LoadProjectName(ctx)cleanly implements the new CLIInterface method without adding extra behavior, keeping project-name resolution in the loader layer.src/pkg/agent/tools/deploy.go (1)
47-51: LGTM!The updated
SetupProvidercall correctly passesproject.Nameobtained from the priorLoadProjectcall, and thetrueflag foruseWkDiris appropriate for deploy operations that need working directory context.src/pkg/agent/tools/destroy.go (1)
26-34: LGTM!The refactored flow correctly separates project name loading from provider setup. Using
useWkDir=falseis appropriate for destroy operations that don't need local compose file context.src/pkg/agent/tools/setConfig_test.go (2)
65-74: LGTM!The mock correctly implements the new
LoadProjectNamesignature that takes onlyctxandloaderparameters, aligning with the interface changes.
176-177: Correct test expectation update.Setting
expectedProviderCalls: falsewhen project name loading fails is correct—the new flow should not attempt provider setup if project name resolution fails.src/pkg/agent/tools/listConfig.go (1)
26-35: LGTM!The refactored flow correctly loads the project name first, then sets up the provider with
useWkDir=falseappropriate for config listing operations.src/pkg/agent/tools/services.go (1)
37-41: LGTM!The provider setup flow is consistent with other files in this PR, correctly using
useWkDir=falsefor service listing operations.src/pkg/agent/tools/services_test.go (2)
25-29: LGTM! Mock field renamed to match new API.The field name change from
LoadProjectNameWithFallbackErrortoLoadProjectNameErrorcorrectly reflects the migration to the simplerLoadProjectNamemethod.
49-57: LGTM! Method signature correctly updated.The mock method signature now matches the new
LoadProjectName(ctx, loader)API, removing the provider parameter that is no longer needed.src/pkg/agent/tools/removeConfig.go (1)
27-35: LGTM! Provider setup flow correctly refactored.The new sequence correctly:
- Loads the project name explicitly via
LoadProjectName- Sets up the provider with the loaded project name using
SetupProvider(ctx, projectName, sc.Stack, false)Error messages are clear and distinguish between project name loading and provider setup failures.
src/pkg/agent/tools/destroy_test.go (2)
50-56: LGTM! Mock method updated to match new API.The method signature change correctly reflects the removal of the provider parameter from
LoadProjectName.
166-173: LGTM! Test expectations correctly updated.The expected call log now includes
LoadProjectNameinstead ofLoadProjectNameWithFallback, matching the new API flow.src/pkg/agent/tools/setConfig.go (1)
28-39: LGTM! Provider setup correctly refactored.The new flow:
- Loads project name when not provided in params
- Sets up provider with explicit project name and
useWkDir=falseThis aligns with the PR's goal of loading project names before provider instantiation.
src/pkg/agent/tools/removeConfig_test.go (2)
42-48: LGTM! Mock method correctly updated.The signature change removes the provider parameter, aligning with the new
LoadProjectName(ctx, loader)API.
173-180: LGTM! Test expectations updated correctly.The expected call sequence now includes
LoadProjectNameinstead ofLoadProjectNameWithFallback.src/cmd/cli/command/commands_test.go (2)
82-86: LGTM! Mock method added to support new stack flow.The
ListDeploymentsmock returns an empty deployments list, which is appropriate for these tests. This supports the new multi-source stack discovery logic.
279-388: LGTM! Test provider initialization updated.All test cases now use
newProvider(ctx)without the loader parameter, reflecting the refactored provider initialization flow where project name is determined before provider creation.src/cmd/cli/command/cd.go (2)
40-50: LGTM! Bootstrap command correctly refactored.The command now:
- Loads the project name from the loader when args are empty
- Creates the provider with the explicit project name using
newProviderChecked(ctx, args[0], false)This aligns with the PR's goal of loading project information before provider instantiation.
157-162: LGTM! Stack saving logic correctly implemented.The
saveStacksToWkDirflag is derived from the presence of theproject-nameflag: when the flag is empty, stacks are saved to the working directory. This makes sense as it allows the project directory to maintain its own stack configurations.src/pkg/agent/tools/provider.go (7)
41-74: LGTM! Provider setup flow well-structured.The new
SetupProvidersignature accepts explicitprojectNameanduseWkDirparameters, enabling proper stack selection before provider creation. The logic correctly:
- Validates stackName is not nil
- Loads stack if name is provided, otherwise selects/creates one
- Sets up provider authentication after stack resolution
117-126: LGTM! Helpful beta stack migration messaging.The special handling for the "beta" stack provides clear guidance for users migrating from the implicit beta stack to the new stacks feature. The messaging appropriately explains the situation and provides a link to documentation.
190-193: LGTM! Beta stack fallback correctly implemented.Defaulting to "beta" when the Stack field is empty correctly handles legacy deployments that predate the stacks feature, as described in the PR objectives.
232-258: LGTM! Stack loading with remote fallback.The
loadStackmethod correctly:
- Attempts to read the stack locally first
- Falls back to fetching from remote deployments if not found locally
- Creates a local copy of remote stacks for future use
- Loads the stack environment variables
This aligns with the PR's multi-source stack handling objective.
260-312: LGTM! New stack creation flow well-implemented.The
createNewStackmethod:
- Prompts for provider, region, and stack name
- Uses sensible defaults (default region, generated name)
- Only creates local stack file when
useWkDiris trueThis provides flexibility for both agent and CLI usage patterns.
326-376: LGTM! AWS authentication flow improved.The refactored authentication correctly:
- Auto-detects existing AWS_PROFILE or access keys
- Lists available profiles and includes an option to use access keys instead
- Falls back to prompting for access keys when no profiles are available
This implements the PR objective of automatically showing profiles when available.
180-213: Verify and document the sort order assumption for ListDeployments.The code at line 197 assumes deployments are returned sorted by
deployed_atdescending, but this sort order is not specified in the API contract (proto file). If the backend doesn't guarantee this ordering, the logic to avoid overwriting stacks could fail silently. Either update the API documentation to explicitly specify the sort order, or make the code independent of API ordering by sorting the results client-side.src/cmd/cli/command/commands.go (13)
19-19: LGTM!New imports are correctly added and properly used in the
newProviderCheckedfunction for the unified provider setup flow.Also applies to: 28-28
91-98: LGTM!The placeholder
"<name>"anduseWkDir=falseare appropriate here since this code path is only used to retrieve the remote project name for an error hint message.
531-535: LGTM!Setting
NonInteractive=truebefore callingnewProviderCheckedensures the simplifiednewProviderpath is taken, making the emptyprojectNameandfalseforuseWkDirirrelevant.
582-587: LGTM!The logic
saveStacksToWkDir = projectNameFlag == ""correctly determines whether to persist stack selection to the working directory based on whether the project name was explicitly provided vs. derived from compose file.
756-765: LGTM!Correctly separates project name loading from provider creation, following the new pattern where the project is loaded first and then passed to the provider setup.
888-897: LGTM!Consistent application of the project-loading-first pattern.
922-932: LGTM!Consistent with the refactored provider creation pattern.
1005-1014: LGTM!Consistent application of the refactored provider creation pattern.
1252-1299: LGTM!The simplified
updateProviderIDnow handles only environment-driven defaulting. The removal of the loader parameter aligns with the separation of concerns where project loading happens before provider setup. The warnings for detected cloud credentials when defaulting to Defang playground are helpful for users who may have forgotten to specify their provider.
1301-1308: LGTM!Simplified
newProviderfor the non-interactive code path, removing the loader dependency.
1310-1314: LGTM!Clean adapter pattern implementing the
ProviderCreatorinterface to bridge the CLI's provider creation with the agent tools'ProviderPreparer.
1336-1372: > Likely an incorrect or invalid review comment.
1316-1330: No action needed. The first return value fromSetupProvideris intentionally discarded becausenewProviderCheckeddoesn't require theProviderID—it only returnsprovideranderror. Other functions that don't need theProviderID(removeConfig, setConfig, services, logs, listConfig, destroy) similarly discard it. Onlydeploy.gocaptures it because that function specifically uses it in aCanIUseProvidercall. This pattern is intentional and correct.
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
4b0ef16 to
cfa44f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/pkg/agent/tools/provider.go (1)
335-385: Clean up redundant AWS_PROFILE check.The function checks
AWS_PROFILEat line 336 and returns early if set, making the check at line 345 redundant.Apply this diff:
func (pp *providerPreparer) SetupAWSAuthentication(ctx context.Context) error { if os.Getenv("AWS_PROFILE") != "" { return nil } if os.Getenv("AWS_ACCESS_KEY_ID") != "" && os.Getenv("AWS_SECRET_ACCESS_KEY") != "" { return nil } // TODO: add support for aws sso strategy - if os.Getenv("AWS_PROFILE") == "" { - knownProfiles, err := listAWSProfiles() - if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) - } - if len(knownProfiles) > 0 { - const useAccessKeysOption = "Use Access Key ID and Secret Access Key" - knownProfiles = append(knownProfiles, useAccessKeysOption) + knownProfiles, err := listAWSProfiles() + if err != nil { + return fmt.Errorf("failed to list AWS profiles: %w", err) + } + if len(knownProfiles) > 0 { + const useAccessKeysOption = "Use Access Key ID and Secret Access Key" + knownProfiles = append(knownProfiles, useAccessKeysOption) profile, err := pp.ec.RequestEnum(ctx, "Select your profile", "profile_name", knownProfiles) if err != nil { return fmt.Errorf("failed to elicit AWS Profile Name: %w", err) } - if profile != useAccessKeysOption { - err := os.Setenv("AWS_PROFILE", profile) - if err != nil { - return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) - } - return nil + if profile != useAccessKeysOption { + err := os.Setenv("AWS_PROFILE", profile) + if err != nil { + return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) } + return nil } } if os.Getenv("AWS_ACCESS_KEY_ID") == "" { accessKeyID, err := pp.ec.RequestString(ctx, "Enter your AWS Access Key ID:", "access_key_id") if err != nil { return fmt.Errorf("failed to elicit AWS Access Key ID: %w", err) } if err := os.Setenv("AWS_ACCESS_KEY_ID", accessKeyID); err != nil { return fmt.Errorf("failed to set AWS_ACCESS_KEY_ID environment variable: %w", err) } } if os.Getenv("AWS_SECRET_ACCESS_KEY") == "" { accessKeySecret, err := pp.ec.RequestString(ctx, "Enter your AWS Secret Access Key:", "access_key_secret") if err != nil { return fmt.Errorf("failed to elicit AWS Secret Access Key: %w", err) } if err := os.Setenv("AWS_SECRET_ACCESS_KEY", accessKeySecret); err != nil { return fmt.Errorf("failed to set AWS_SECRET_ACCESS_KEY environment variable: %w", err) } } return nil }src/cmd/cli/command/commands.go (1)
89-99: Improve error handling for maximum projects exceeded.The placeholder
projectName := "<name>"at line 90 could result in confusing error messages ifnewProviderCheckedfails before reaching theRemoteProjectNamecall.Consider handling this case more gracefully:
if strings.Contains(err.Error(), "maximum number of projects") { - projectName := "<name>" - provider, err := newProviderChecked(ctx, projectName, false) - if err != nil { - return err + projectName := "your-project" + provider, provErr := newProviderChecked(ctx, "", false) + if provErr != nil { + term.Debug("Failed to get provider for hint:", provErr) + printDefangHint("To deactivate a project, do:", "compose down --project-name <name>") + return err } if resp, err := provider.RemoteProjectName(ctx); err == nil { projectName = resp } printDefangHint("To deactivate a project, do:", "compose down --project-name "+projectName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cmd/cli/command/commands.go(14 hunks)src/pkg/agent/tools/provider.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmd/cli/command/commands.go (5)
src/pkg/cli/client/provider_id.go (2)
ProviderID(10-10)ProviderDefang(14-14)src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/elicitations/elicitations.go (2)
Client(14-16)NewController(32-36)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/provider.go (3)
src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/stacks/stacks.go (6)
StackParameters(17-22)List(85-118)Create(33-76)Read(180-195)Load(197-208)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)
⏰ 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). (3)
- GitHub Check: nix-shell-test
- GitHub Check: go-playground-test
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/pkg/agent/tools/provider.go (3)
41-74: LGTM! Clear separation of concerns in provider setup.The function properly handles both explicit stack names and interactive selection flows. The error handling is appropriate and the flow is easy to follow.
181-214: LGTM! Good handling of legacy deployments.The function correctly handles the migration from implicit "beta" stacks by defaulting empty stack names to "beta". The deduplication logic using a map is efficient and the reliance on pre-sorted deployments is clearly documented.
268-321: LGTM! Conditional persistence is well-designed.The function appropriately only persists stacks to disk when
useWkDiris true, allowing for transient stack creation in other scenarios. The use ofMakeDefaultNameensures consistent stack naming conventions.src/cmd/cli/command/commands.go (3)
438-447: LGTM! Appropriate error handling for missing stack files.The code correctly ignores only
os.ErrNotExisterrors, allowing the remote import flow to handle missing local stacks while still propagating other errors (e.g., permission issues, corrupt files).
585-597: The saveStacksToWkDir pattern is consistently applied across all commands, but reconsider the logic.The pattern
saveStacksToWkDir := projectNameFlag == ""is used uniformly across 10 occurrences spanningcd.go,compose.go, andcommands.go. The implementation is consistent—it assumes that providing an explicit project name flag means users don't want local stack persistence.However, the underlying assumption itself warrants review: Is this inverse relationship intentional and correct? Users might expect stack persistence to be independent of whether they specify a project name, or this behavior should be explicitly documented as a design decision rather than an implicit assumption.
1319-1333: The original concern is incorrect. ThenewProviderfunction exists and properly handles the--stackflag in non-interactive mode. Both code paths passglobal.Stackto the provider initialization:
- Non-interactive:
newProvider(ctx)→cli.NewProvider(ctx, global.ProviderID, global.Client, global.Stack)- Interactive:
pp.SetupProvider(ctx, projectName, &global.Stack, useWkDir)The
global.Stackvariable is populated from the--stackcommand-line flag, so stack selection works identically in both modes. No changes are required.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
66805ca to
64334ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pkg/agent/tools/setConfig.go (1)
27-39: LGTM! Provider setup correctly deferred until project name is available.The refactored flow properly handles both scenarios: when
params.ProjectNameis provided by the caller and when it needs to be loaded. Provider setup correctly uses the resolved project name.The boolean parameter
falseinSetupProvideron line 36 would benefit from a named constant for clarity, but this is consistent with other files in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/listConfig_test.go(3 hunks)src/pkg/agent/tools/logs.go(1 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(3 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/agent/tools/setConfig_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/agent/tools/listConfig.go
🧰 Additional context used
🧬 Code graph analysis (4)
src/pkg/agent/tools/services.go (2)
src/pkg/term/colorizer.go (3)
Error(318-320)Errorf(322-324)Debugf(298-300)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/removeConfig.go (2)
src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/logs.go (1)
src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/setConfig.go (2)
src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)src/pkg/clouds/driver.go (1)
ProjectName(8-8)
⏰ 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). (3)
- GitHub Check: nix-shell-test
- GitHub Check: go-playground-test
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
src/pkg/agent/tools/logs.go (1)
48-58: LGTM! Provider initialization flow correctly refactored.The changes properly sequence project name loading before provider setup, with accurate error messages for each step. The debug log on line 52 provides useful visibility into the loaded project name.
src/pkg/agent/tools/removeConfig.go (1)
24-35: LGTM! Consistent with the refactored provider initialization flow.The changes align with the broader refactoring pattern: load project name first, then setup provider. Error messages are accurate and the lowercase "could not connect" maintains consistency across the codebase.
src/pkg/agent/tools/listConfig_test.go (2)
41-47: LGTM! Mock updated to match new interface.The mock method signature correctly reflects the removal of the provider parameter from
LoadProjectName, aligning with the refactored implementation.
154-161: LGTM! Test expectations updated correctly.The expected call sequence now reflects the new flow:
Connect→LoadProjectName→NewProvider→ListConfig, which matches the refactored implementation.src/pkg/agent/tools/setConfig_test.go (2)
65-74: LGTM! Mock updated to match new interface.The mock method signature correctly reflects the removal of the provider parameter from
LoadProjectName.
167-178: LGTM! Test expectation correctly reflects new sequencing.Setting
expectedProviderCalls: falseon line 176 when project name loading fails is correct, since provider setup now occurs after project name loading in the refactored flow.src/pkg/agent/tools/removeConfig_test.go (2)
42-48: LGTM! Mock updated to match new interface.The mock method signature correctly removes the provider parameter from
LoadProjectName, consistent with the refactored implementation.
173-181: LGTM! Test expectations correctly updated.The expected call sequence now reflects the new flow:
Connect→LoadProjectName→NewProvider→ConfigDelete.src/pkg/agent/tools/services.go (1)
29-41: LGTM! Debug log correctly placed after error check.The refactored flow properly loads the project name first, then sets up the provider. The debug log on line 36 is correctly positioned after the error handling (lines 30-35), ensuring it only logs valid project names. This addresses the previously identified issue.
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 (3)
src/pkg/agent/tools/provider.go (1)
267-320:createNewStackcannot be used without.defangwhen combined withselectOrCreateStack.When
useWkDiris false,createNewStackintentionally skipsstacks.Create. However,selectOrCreateStackalways callspp.loadStackat the end, which in turn expects a local.defang/<name>file or imports from remote.For
useWkDir == falseand no prior deployments forprojectName:
selectStackreturnsCreateNewStack.createNewStackreturns aStackListItembut does not create a file.loadStackfails with “stack %q does not exist locally or remotely”.This means interactive flows that pass
useWkDir=false(e.g., agent tools and CLI subcommands with--project-name) cannot successfully create and use a new stack.You may want
selectOrCreateStackto return*stacks.StackParameters(constructed from the provider/region prompts) and:
- Only go through
loadStack+stacks.LoadwhenuseWkDiris true, and- Use the in-memory parameters directly when
useWkDiris false.src/cmd/cli/command/commands.go (2)
89-99:newProviderCheckedin the “maximum number of projects” error path will likely fail or prompt unnecessarily.In this hint path you call:
projectName := "<name>" provider, err := newProviderChecked(ctx, projectName, false)
newProviderChecked’s interactive branch now:
- Runs full stack selection/creation via
SetupProvider(ctx, projectName, &global.Stack, useWkDir=false), and- Ultimately relies on
loadStack, which expects an existing local or remote stack for the suppliedprojectName.For a placeholder project name
"<name>"with no deployments, this will generally fail with a stack-not-found error before you can callprovider.RemoteProjectName(ctx), and it may also pop up a stack-selection/credentials prompt in an error-handling path.Given you only need a provider to resolve
RemoteProjectName, consider:
- Calling
newProvider(ctx)here instead (using the same provider as the original command), or- Passing the actual project name that triggered the “maximum number of projects” error, if available, and ensuring
useWkDiris appropriate.
1255-1301:newProviderCheckedshould propagate the selected provider ID intoglobal.ProviderIDfor consistency.In interactive mode,
newProviderCheckednow does:pc := &providerCreator{} ec := elicitations.NewController(...) pp := agentTools.NewProviderPreparer(pc, ec, global.Client) _, provider, err := pp.SetupProvider(ctx, projectName, &global.Stack, useWkDir) ... _, err = provider.AccountInfo(ctx) return provider, errTwo concerns:
The
ProviderIDchosen bySetupProvideris never written back toglobal.ProviderID. As a result:
track.Cmdlogs (which includeP("provider", global.ProviderID)) may recordProviderAutoor a stale value even though the actual provider used is AWS/DO/GCP.- Callers like
debug.NewDebugger(ctx, getCluster(), &global.ProviderID, &global.Stack)may see a provider ID that does not match the instantiatedprovider.In contrast, the non-interactive branch calls
newProvider(ctx), which does runupdateProviderID(ctx)and ensuresglobal.ProviderIDis in sync.To keep global state consistent, consider:
id, provider, err := pp.SetupProvider(ctx, projectName, &global.Stack, useWkDir) if err != nil { ... } global.ProviderID = *idbefore returning.
Also applies to: 1303-1332
♻️ Duplicate comments (1)
src/pkg/agent/tools/provider.go (1)
82-114: Remote stack detection/import is still based on display-name parentheses and ignoresuseWkDir.Two related issues here:
The “remote vs local” decision is inferred from the rendered label (
strings.Contains(selectedStackName, "(")), which is fragile and was already flagged in a previous review. This can break if:
- Label formatting changes.
- Stack names or translations ever include parentheses.
- You later add more metadata to the label.
A more robust pattern is to keep a map from display name to a
StackOptionthat explicitly carries whether it is local and the correspondingStackParameters, e.g.:type StackOption struct { Name string Local bool LastDeployedAt time.Time Params *stacks.StackParameters // nil for pure-local if you want }Then build
displayToOption := map[string]StackOption{}and retrieve the option directly instead of parsing parentheses.
stacks.Create(*remoteStackParameters)is invoked unconditionally for remote selections, even whenuseWkDiris false. That means:
- CLI subcommands invoked with
--project-name(which passuseWkDir=false) will still create.defang/<stack>files in the current working directory when importing a remote stack.- Agent tools (which also call
SetupProviderwithuseWkDir=false) can likewise write stack files into whatever directory happens to be current.This undermines the PR goal of “restrict reading and writing stack files when --project-name is used or when ProjectName is provided by an agent tool call” and can cause cross-project stack files to appear in unrelated directories.
Consider:
- Using explicit metadata on
StackOption(or a dedicated map/set of remote names) instead of parsing labels, and- Respecting
useWkDirhere (and/or inloadStack) so that remote stacks can be used without writing.defangfiles whenuseWkDiris false (e.g., by returning parameters directly toSetupProviderinstead of round-tripping through the filesystem).Also applies to: 148-171
🧹 Nitpick comments (2)
src/pkg/agent/tools/provider.go (1)
181-213: Consider deterministic ordering and provider ID error handling incollectExistingStacks.Minor, but two improvements would make this more robust:
- The final slice of stacks is built from a map without sorting, so the interactive menu order is effectively random. Sorting by
DeployedAt(desc) or by name would produce a stable UX.providerID.SetValue(deployment.GetProvider())ignores errors; if a newer Fabric adds a provider enum the CLI doesn’t know about, this could silently yield a zero-value provider. At least logging or failing fast here would make such incompatibilities visible.src/cmd/cli/command/commands.go (1)
757-766: Config and delete commands correctly respect--project-namevs working directory, but there is some duplication.For
config create,config rm,config ls, anddeleteyou now:
- Load the project name via
loader.LoadProjectName.- Compute
saveStacksToWkDir := (projectNameFlag == "").- Call
newProviderChecked(ctx, projectName, saveStacksToWkDir).This aligns with the PR intent:
- When
--project-nameis omitted, stacks in the current working directory are considered and new ones can be created.- When
--project-nameis set, local stacks shouldn’t be enumerated/created (modulo the separate.defangread concern inPersistentPreRunE).The pattern is repeated across several commands; if this spreads further, it might be worth extracting a helper like
getProjectAndProvider(cmd)to reduce duplication, but that’s not urgent.Also applies to: 890-898, 924-933, 1007-1015
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/cmd/cli/command/commands.go(14 hunks)src/pkg/agent/tools/listConfig.go(1 hunks)src/pkg/agent/tools/listConfig_test.go(3 hunks)src/pkg/agent/tools/logs.go(1 hunks)src/pkg/agent/tools/provider.go(5 hunks)src/pkg/agent/tools/removeConfig.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(3 hunks)src/pkg/agent/tools/services.go(1 hunks)src/pkg/agent/tools/setConfig.go(1 hunks)src/pkg/agent/tools/setConfig_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pkg/agent/tools/listConfig_test.go
- src/pkg/agent/tools/listConfig.go
- src/pkg/agent/tools/logs.go
- src/pkg/agent/tools/services.go
🧰 Additional context used
🧬 Code graph analysis (4)
src/pkg/agent/tools/setConfig.go (3)
src/pkg/term/colorizer.go (1)
Errorf(322-324)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)src/pkg/clouds/driver.go (1)
ProjectName(8-8)
src/pkg/agent/tools/removeConfig.go (1)
src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/cmd/cli/command/commands.go (8)
src/pkg/term/colorizer.go (2)
Warn(310-312)Errorf(322-324)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/protos/io/defang/v1/fabric.pb.go (4)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)src/pkg/cli/client/provider.go (1)
Provider(43-69)src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/elicitations/elicitations.go (2)
Client(14-16)NewController(32-36)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(33-39)
src/pkg/agent/tools/provider.go (3)
src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/stacks/stacks.go (6)
StackParameters(17-22)List(85-118)Create(33-76)Read(180-195)Load(197-208)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)
⏰ 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). (3)
- GitHub Check: nix-shell-test
- GitHub Check: go-playground-test
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/pkg/agent/tools/provider.go (2)
41-59: EnsurestackNameis never nil at call sites and consider documenting this contract.
SetupProvidernow hard-fails whenstackName == nil. CLI paths use&global.Stack, but agent tool paths passsc.Stackdirectly; if any caller ever leavesStackConfig.Stacknil, provider setup will return"stackName cannot be nil"instead of falling back to selection/creation.Either:
- Guarantee and document that
StackConfig.Stackis always non-nil (even when empty), or- Change the API to accept
niland internally treat it as “no stack selected yet”.
334-383: AWS profile/key prompting behavior looks solid and matches stated goals.The new
SetupAWSAuthenticationflow:
- Respects existing
AWS_PROFILEor access keys without prompting.- Enumerates profiles from
~/.aws/{credentials,config}and offers a “Use Access Key ID and Secret Access Key” fallback.- Only prompts for each missing credential exactly once.
listAWSProfilescorrectly extracts profile names from both files and returns a sorted, duplicate-free list.No issues from my side here.
Also applies to: 432-471
src/cmd/cli/command/commands.go (1)
534-565:whoami’s use ofnewProviderCheckedis fine and remains non-interactive.Setting
global.NonInteractive = truebefore calling:provider, err := newProviderChecked(cmd.Context(), "", false)ensures the non-interactive branch (
newProvider(ctx)) is used, preserving the previous behavior (no stack selection survey, provider still derived from flags/env viaupdateProviderID).No changes needed here.
src/pkg/agent/tools/setConfig_test.go (1)
65-75: Tests correctly reflect the new project-name loading and error flow.Updating
MockSetConfigCLIto exposeLoadProjectName(ctx, loader)and expecting:
"could not connect: connection failed"for connect errors, and- No provider creation (
expectedProviderCalls: false) whenLoadProjectNamefails,matches the new
HandleSetConfigbehavior (connect → load project name → provider setup). The adjustments to error messages and call expectations look consistent.Also applies to: 158-177
src/pkg/agent/tools/setConfig.go (1)
20-39: Project-name loading is correct, butuseWkDir=falseinherits the same stack-creation/import constraints.The new flow:
client, err := cli.Connect(...) pp := NewProviderPreparer(cli, ec, client) if params.ProjectName == "" { projectName, err := cli.LoadProjectName(ctx, loader) ... params.ProjectName = projectName } _, provider, err := pp.SetupProvider(ctx, params.ProjectName, sc.Stack, false)nicely ensures you always have a concrete
ProjectNamebefore setting up the provider, which is a good improvement over the previous fallback-based path.However, because
useWkDiris hard-coded tofalsehere,HandleSetConfigis subject to the same limitations noted inprovider.go:
- It cannot successfully create a brand new stack for a project with no existing deployments or local stacks.
- It may still create
.defang/<stack>files when importing remote stacks, despiteuseWkDir=false.Once the underlying
SetupProviderbehavior is adjusted (as suggested inprovider.go), this call site should benefit automatically; no changes needed here beyond that.src/pkg/agent/tools/removeConfig_test.go (1)
42-48: Tests accurately track the new project-name loading and call ordering.The mock now:
- Implements
LoadProjectName(ctx, loader)and logs"LoadProjectName".- Expects
"could not connect: connection failed"on connect error.- Verifies the successful path calls
Connect, thenLoadProjectName, thenNewProvider, thenConfigDelete.This aligns with the updated
HandleRemoveConfigToolbehavior and the introduction ofLoadProjectName+SetupProvider. Looks good.Also applies to: 68-76, 172-181
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pkg/agent/tools/provider.go (1)
140-177: Fragile remote stack detection still relies on string parsing.The implementation checks for
"(last deployed "in the selected name to determine if it's a remote stack. While more specific than checking for just"(", this approach is still fragile—a local stack name containing this substring would cause incorrect behavior.Consider using a display-to-actual name mapping as previously suggested:
- // Convert map back to slice - stackNames := make([]string, 0, len(stackOptions)+1) - for _, stackOption := range stackOptions { - name := stackOption.Name - if !stackOption.Local { - name += fmt.Sprintf(" (last deployed %s)", stackOption.LastDeployedAt.Local().Format(time.RFC822)) - } - stackNames = append(stackNames, name) - } - if useWkDir { - stackNames = append(stackNames, CreateNewStack) - } - - selectedStackName, err := ec.RequestEnum(ctx, "Select a stack", "stack", stackNames) - if err != nil { - return "", fmt.Errorf("failed to elicit stack choice: %w", err) - } - - // check if the selected stack has parentheses indicating it's remote - if strings.Contains(selectedStackName, "(last deployed ") && strings.Contains(selectedStackName, ")") { - selectedStackName = strings.TrimSpace(selectedStackName[:strings.Index(selectedStackName, "(")]) - - // find the stack parameters from the remoteStackList - stackOption := stackOptions[selectedStackName] - if stackOption.Local { - return selectedStackName, nil - } - - if stackOption.Parameters == nil { - return "", fmt.Errorf("stack parameters for remote stack %q are nil", selectedStackName) - } - - term.Debugf("Importing stack %s from remote", selectedStackName) - _, err := stacks.Create(*stackOption.Parameters) - if err != nil { - return "", fmt.Errorf("failed to create local stack from remote: %w", err) - } - } + // Convert map to display names with mapping back to actual names + displayNames := make([]string, 0, len(stackOptions)+1) + displayToActual := make(map[string]string) + for _, stackOption := range stackOptions { + displayName := stackOption.Name + if !stackOption.Local { + displayName += fmt.Sprintf(" (last deployed %s)", stackOption.LastDeployedAt.Local().Format(time.RFC822)) + } + displayNames = append(displayNames, displayName) + displayToActual[displayName] = stackOption.Name + } + if useWkDir { + displayNames = append(displayNames, CreateNewStack) + displayToActual[CreateNewStack] = CreateNewStack + } + + selectedDisplayName, err := ec.RequestEnum(ctx, "Select a stack", "stack", displayNames) + if err != nil { + return "", fmt.Errorf("failed to elicit stack choice: %w", err) + } + + selectedStackName := displayToActual[selectedDisplayName] + stackOption := stackOptions[selectedStackName] + + // Import remote stack if it's not local and not the create-new option + if !stackOption.Local && selectedStackName != CreateNewStack { + if stackOption.Parameters == nil { + return "", fmt.Errorf("stack parameters for remote stack %q are nil", selectedStackName) + } + + term.Debugf("Importing stack %s from remote", selectedStackName) + _, err := stacks.Create(*stackOption.Parameters) + if err != nil { + return "", fmt.Errorf("failed to create local stack from remote: %w", err) + } + }
🧹 Nitpick comments (2)
src/pkg/agent/tools/provider.go (2)
350-370: RedundantAWS_PROFILEcheck.The condition
if os.Getenv("AWS_PROFILE") == ""at line 350 is always true at this point since line 341 already returned early ifAWS_PROFILEwas set. The check can be removed to simplify the code.- // TODO: add support for aws sso strategy - if os.Getenv("AWS_PROFILE") == "" { - knownProfiles, err := listAWSProfiles() - if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) - } - if len(knownProfiles) > 0 { - const useAccessKeysOption = "Use Access Key ID and Secret Access Key" - knownProfiles = append(knownProfiles, useAccessKeysOption) - profile, err := pp.ec.RequestEnum(ctx, "Select your profile", "profile_name", knownProfiles) - if err != nil { - return fmt.Errorf("failed to elicit AWS Profile Name: %w", err) - } - if profile != useAccessKeysOption { - err := os.Setenv("AWS_PROFILE", profile) - if err != nil { - return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) - } - return nil - } - } - } + // TODO: add support for aws sso strategy + knownProfiles, err := listAWSProfiles() + if err != nil { + return fmt.Errorf("failed to list AWS profiles: %w", err) + } + if len(knownProfiles) > 0 { + const useAccessKeysOption = "Use Access Key ID and Secret Access Key" + knownProfiles = append(knownProfiles, useAccessKeysOption) + profile, err := pp.ec.RequestEnum(ctx, "Select your profile", "profile_name", knownProfiles) + if err != nil { + return fmt.Errorf("failed to elicit AWS Profile Name: %w", err) + } + if profile != useAccessKeysOption { + err := os.Setenv("AWS_PROFILE", profile) + if err != nil { + return fmt.Errorf("failed to set AWS_PROFILE environment variable: %w", err) + } + return nil + } + }
458-469: Missing scanner error check.After the
scanner.Scan()loop,scanner.Err()should be checked to detect any I/O errors that occurred during scanning. Without this check, partial reads due to errors would be silently ignored.for scanner.Scan() { line := scanner.Text() line = strings.TrimSpace(line) if strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]") { section = strings.Trim(line, "[]") // In config, profiles are named "profile NAME" section = strings.TrimPrefix(section, "profile ") profiles[section] = struct{}{} } } + if err := scanner.Err(); err != nil { + f.Close() + return nil, fmt.Errorf("error reading %s: %w", file, err) + } f.Close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pkg/agent/tools/provider.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/agent/tools/provider.go (2)
src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/stacks/stacks.go (6)
StackParameters(17-22)List(85-118)Create(33-76)Read(180-195)Load(197-208)StackListItem(78-83)
⏰ 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 (8)
src/pkg/agent/tools/provider.go (8)
41-74: LGTM!The updated
SetupProviderfunction cleanly orchestrates the provider setup flow with proper branching for explicit stack names vs. interactive selection, and appropriate error handling throughout.
76-81: LGTM!The
StackOptionstruct cleanly captures both local and remote stack metadata with appropriate fields for the display and import logic.
83-116: LGTM!The function correctly merges remote and local stacks with local taking precedence, which aligns with the expected behavior where existing local stack files should be used instead of re-importing from remote.
182-185: LGTM!Clean struct definition that appropriately extends
StackParameterswith deployment timestamp for remote stack tracking.
222-237: LGTM!Clean orchestration function that delegates to the appropriate helpers based on user selection.
239-271: LGTM!The function correctly implements the fallback logic from local to remote stacks with appropriate error handling. The previous variable shadowing concern has been addressed.
313-319: Conditional stack file creation aligns with requirements.The
useWkDirguard correctly prevents stack file creation when invoked outside a working directory context (e.g., agent tool calls). The function still returns the stack item for in-memory use downstream.
196-214: TheModefield is intentionally optional and defaults correctly.The
StackParametersstruct includes theModefield, but it's not required to be set during construction. TheMarshal()function explicitly handles the case whereModeis unspecified (the zero value) by omitting it from the stack file. This is the correct behavior for remote stacks where deployment mode information is not available from the API response.
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
eca8b33 to
88a4ad7
Compare
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
88a4ad7 to
dc0b8ea
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 (1)
src/pkg/agent/tools/provider.go (1)
72-90: New-stack flow breaks when useWkDir=false (no stackfile is ever readable)For interactive provider setup you now have:
SetupProvider→selectOrCreateStack(ctx, projectName, useWkDir)selectOrCreateStack:
- Calls
selectStack, which can returnCreateNewStackwhen there are no existing local/remote stacks.- On
CreateNewStack, callscreateNewStack(ctx, useWkDir)and then always callspp.loadStack(ctx, projectName, selectedStackName).createNewStackonly callspp.sm.Create(params)whenuseWkDiris true.loadStackalways starts withpp.sm.Read(stackName)and falls back to remote deployments if the file doesn’t exist.For
useWkDir=falseand a new stack:
createNewStacknever writes a.defang/<stack>file.loadStackwill callRead, seeos.ErrNotExist, thencollectPreviouslyDeployedStacks, which won’t find the brand-new stack (no deployments yet), and finally error withstack %q does not exist locally or remotely.This contradicts the test expectations in
provider_test.gofor the “create stack without stackfile” cases and makes theuseWkDir=falsenew‑stack path unusable.You probably want one of:
Disallow new stacks when useWkDir=false: In
selectStack/selectOrCreateStack, if!useWkDirand there are no local/remote stacks, return a clear error like “no stacks exist; run from a project directory or omit --project-name” instead ofCreateNewStack.Support true ephemeral stacks: When
useWkDir=falseand you create a new stack:
- Don’t call
loadStackat all.- Use the freshly built
stacks.StackParametersfromcreateNewStackto setproviderIDand callNewProvider, without touchingStacksManager.Read/Load.- Optionally skip
stacks.Createentirely so no local.defangis written.Right now the code guarantees a failure in that scenario even though the tests assume it will work, so this is worth fixing before release.
func (pp *providerPreparer) selectOrCreateStack(ctx context.Context, projectName string, useWkDir bool) (*stacks.StackParameters, error) { - selectedStackName, err := pp.selectStack(ctx, projectName, useWkDir) + selectedStackName, err := pp.selectStack(ctx, projectName, useWkDir) ... - if selectedStackName == CreateNewStack { - newStack, err := pp.createNewStack(ctx, useWkDir) + if selectedStackName == CreateNewStack { + if !useWkDir { + return nil, fmt.Errorf("cannot create a new stack when --project-name is used; run from the project directory instead") + } + newStack, err := pp.createNewStack(ctx, useWkDir) ... } - return pp.loadStack(ctx, projectName, selectedStackName) + return pp.loadStack(ctx, projectName, selectedStackName) }Also applies to: 256-305, 307-360
♻️ Duplicate comments (1)
src/cmd/cli/command/commands.go (1)
1257-1303: Provider auto-detection path still warns but always picks Defang playground
updateProviderIDdoes:
- Honor
--provideroverDEFANG_PROVIDER.- When
ProviderAuto, warn if AWS/DO/GCP env vars are present, then unconditionally setglobal.ProviderID = cliClient.ProviderDefang.This matches the current behavior (auto = playground), but the warnings may still confuse users who expect “auto” to actually use those env vars.
If the intention is that
ProviderAutoalways means “Defang playground, env vars ignored”, consider:
- Updating any user-facing docs/help text to say so explicitly.
- Potentially downgrading the
term.Warncalls to a single concise info log, or removing them entirely, to avoid implying that the CLI could have auto-selected AWS/DO/GCP.The helper
newProviderand the non-interactive branch innewProviderCheckedcorrectly callupdateProviderIDbefore instantiating the provider, so the code path is coherent; this is mainly a UX/semantics refinement.Also applies to: 1305-1313, 1320-1334
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cmd/cli/command/commands.go(14 hunks)src/cmd/cli/command/globals.go(2 hunks)src/cmd/cli/command/globals_test.go(3 hunks)src/pkg/agent/tools/provider.go(5 hunks)src/pkg/agent/tools/provider_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmd/cli/command/commands.go (5)
src/pkg/cli/client/provider.go (1)
Provider(43-69)src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/elicitations/elicitations.go (2)
Client(14-16)NewController(32-36)src/pkg/elicitations/survey.go (1)
NewSurveyClient(19-25)src/pkg/agent/tools/provider.go (1)
NewProviderPreparer(63-70)
src/pkg/agent/tools/provider.go (4)
src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(180-195)Load(197-208)List(85-118)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
⏰ 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 (5)
src/cmd/cli/command/globals_test.go (1)
21-40: Test updates correctly track the loadStackFile renameSwapping
loadDotDefangforloadStackFilekeeps the existing behaviors under test:
- OS env still wins over values from the stack file.
- A non-existent stack name still returns an error at the helper level (even though the CLI now ignores
os.ErrNotExist).No issues from the test side here.
Also applies to: 316-323
src/pkg/agent/tools/provider.go (1)
362-424: Provider authentication flows look sound; nice AWS profile UXThe new authentication helpers:
- Short-circuit when AWS/GCP/DO env vars are already present.
- For AWS:
- Prefer
AWS_PROFILEif set.- Otherwise, list profiles from
~/.aws/{credentials,config}and offer a “Use Access Key ID and Secret Access Key” option before falling back to prompting for keys.- For GCP and DO:
- Prompt for the missing env vars and set them via
os.Setenv.This matches typical provider SDK expectations and keeps behavior strictly additive (only filling in missing values). Given
SetupProvideronly calls these in interactive flows (non-interactive uses the oldnewProviderpath), there’s no risk of unexpected prompts in CI.No changes needed here from a correctness perspective.
Also applies to: 439-470, 472-512
src/cmd/cli/command/globals.go (1)
23-33: I cannot verify this review comment because the repository cannot be cloned in the sandbox environment. Without access to the actual codebase, I cannot confirm:
- The current implementation of
loadStackFileat lines 23-33 and 244-272- What the documentation actually states about
.defangfile loading- Whether general
.defangfiles are still loaded elsewhere in the codebase- The exact behavior changes described
Manual verification is required to validate the claims about:
- Whether
loadStackFiletruly skips general.defangfiles- Whether the comments/docstrings still describe behavior that no longer exists
- Whether this represents a breaking change for users
src/cmd/cli/command/commands.go (2)
438-449: Project-name gating of stack-file loading is correct, but verification of remaining .defang references and GlobalConfig comments cannot be completedThe code change wrapping
global.loadStackFile(global.getStackName(cmd.Flags()))in:if !cmd.Flags().Changed("project-name") { ... }correctly prevents reading stack files from the current working directory when
--project-nameis explicitly supplied.Combined with changes to
loadStackFilethat now only loads.defang/<stack>files and no longer loads a general.defangfile, this ensures:
- No stack RC is read when
--project-nameis supplied- Only stack-specific files are read for the working directory project
To complete the verification, the following still need to be checked:
- Update GlobalConfig comments to explicitly document that only
.defang/<stack>files are supported and general.defangis ignored- Verify no remaining call sites still expect
.defangto be honored for legacy configurations
89-99: newProviderChecked wiring is consistent, but ProviderID global may be staleAll the updated commands now follow the same pattern:
- Resolve
projectNameviaconfigureLoader().LoadProjectName(ctx)or flags.- Compute
saveStacksToWkDir := projectNameFlag == "".- Call
newProviderChecked(ctx, projectName, saveStacksToWkDir)and use the returnedproviderfor API calls.This is good: it centralizes provider+stack selection and correctly disables working‑directory stack writes when
--project-nameis set.However:
newProviderCheckedin interactive mode constructs aproviderPreparer, callspp.SetupProvider(ctx, projectName, &global.Stack, useWkDir), and ignores the returned*cliClient.ProviderID.- It never writes that ProviderID back into
global.ProviderID.Some code still passes
&global.ProviderIDelsewhere, e.g.:debugger, err := debug.NewDebugger(ctx, getCluster(), &global.ProviderID, &global.Stack) ... debugConfig := debug.DebugConfig{ ProviderID: &global.ProviderID, ... }If
global.ProviderIDis only set by flags/env (updateProviderID) and never updated by the interactive stack selection flow, there's a risk that:
- The provider actually used for config/debug/delete operations (via
provider) is different fromglobal.ProviderID, and- Any downstream code that uses
global.ProviderID(debugger, agent, etc.) will see the wrong provider.Consider either:
- Writing the selected providerID back into
global.ProviderIDinsidenewProviderChecked(orSetupProvider), or- Refactoring downstream code (e.g. debug) to rely solely on the concrete
providerinstead ofglobal.ProviderID.At minimum this deserves a quick grep to see all remaining uses of
global.ProviderIDafternewProviderCheckedto ensure no stale state is relied upon.Also applies to: 531-567, 587-599, 755-768, 893-900, 927-935, 959-967, 1009-1017
| type StackOption struct { | ||
| Name string | ||
| Local bool | ||
| LastDeployedAt time.Time | ||
| Parameters *stacks.StackParameters | ||
| } | ||
|
|
||
| func (pp *providerPreparer) collectStackOptions(ctx context.Context, projectName string, useWkDir bool) (map[string]StackOption, error) { | ||
| // Merge remote and local stacks into a single list of type StackOption, | ||
| // prefer local if both exist | ||
| stackMap := make(map[string]StackOption) | ||
| remoteStackList, err := pp.collectPreviouslyDeployedStacks(ctx, projectName) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to list stacks: %w", err) | ||
| return nil, fmt.Errorf("failed to collect existing stacks: %w", err) | ||
| } | ||
| for _, remoteStack := range remoteStackList { | ||
| label := fmt.Sprintf("%s (last deployed %s)", remoteStack.Name, remoteStack.DeployedAt.Local().Format(time.RFC822)) | ||
| stackMap[label] = StackOption{ | ||
| Name: remoteStack.Name, | ||
| Local: false, | ||
| LastDeployedAt: remoteStack.DeployedAt, | ||
| Parameters: &remoteStack.StackParameters, | ||
| } | ||
| } | ||
|
|
||
| if len(stackList) == 0 { | ||
| if useWkDir { | ||
| localStackList, err := pp.sm.List() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list stacks: %w", err) | ||
| } | ||
|
|
||
| for _, localStack := range localStackList { | ||
| stackMap[localStack.Name] = StackOption{ | ||
| Name: localStack.Name, | ||
| Local: true, | ||
| LastDeployedAt: time.Time{}, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return stackMap, nil | ||
| } |
There was a problem hiding this comment.
Beta-stack messaging and stack-option map keys don’t align with labels
In the stack selection flow:
collectStackOptionscreatesstackMapkeyed as:- Remote stacks:
label := fmt.Sprintf("%s (last deployed %s)", name, timestamp)→stackMap[label] = StackOption{ Name: name, Local: false, ... } - Local stacks (when
useWkDir):stackMap[localStack.Name] = StackOption{ Name: localStack.Name, Local: true }
- Remote stacks:
printStacksInfoMessagethen checks_, betaExists := stacks["beta"]to decide whether to show the “implicit beta stack” message.selectStackpassesstackLabels := keys(stackOptions)intoRequestEnumand looks upselectedStackOption := stackOptions[selectedStackLabel].
Two concrete issues:
-
Implicit beta deployments won’t trigger the message on first run
When you only have previous deployments with empty
Stack(normalized to"beta"incollectPreviouslyDeployedStacks), the only entry created is under the label"beta (last deployed ...)". There is no"beta"key instackOptions, sobetaExistsis false and the beta-stacks onboarding message is never shown for the very case it’s meant to address.Fix: detect beta by the underlying
StackOption.Name, not the map key:func printStacksInfoMessage(stacks map[string]StackOption) { betaExists := false for _, opt := range stacks { if opt.Name == "beta" { betaExists = true break } } ... }
-
Comment vs behavior: “prefer local if both exist”
The comment on
collectStackOptionssays “prefer local if both exist”, but because remote entries are keyed by label (with timestamp) and locals by bare name, you’ll see both:"stack1"(local)"stack1 (last deployed ...)"(remote)
and the selection logic treats them as distinct options. If you really want to prefer local, consider keying by canonical name and maintaining a separate display‑label map for the prompt.
Additionally, printStacksInfoMessage calls os.Args[1] without checking len(os.Args) > 1:
executable, _ := os.Executable()
term.Infof("To skip this prompt, run %s %s --stack=%s", filepath.Base(executable), os.Args[1], "<stack_name>")This will panic in edge cases (e.g., program invoked with no arguments, or reused as a library). A simple length guard with a fallback to just the binary name would make this more robust.
Also applies to: 150-164, 165-215, 216-254
There was a problem hiding this comment.
Actionable comments posted: 3
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/provider.go (1)
79-90:loadStackunconditionally creates local stack files, ignoringuseWkDir.When a stack name is provided and the stack doesn't exist locally but does remotely,
loadStackat line 80 will create the local stack file regardless of theuseWkDirflag. This contradicts the PR objective of restricting stack file writes when--project-nameis used.Consider passing
useWkDirtoloadStackand conditionally creating the stack file:-func (pp *providerPreparer) loadStack(ctx context.Context, projectName, stackName string) (*stacks.StackParameters, error) { +func (pp *providerPreparer) loadStack(ctx context.Context, projectName, stackName string, useWkDir bool) (*stacks.StackParameters, error) { stack, err := pp.sm.Read(stackName) if err != nil { if !errors.Is(err, os.ErrNotExist) { return nil, fmt.Errorf("failed to read stack: %w", err) } // call collectExistingStacks to see if it exists remotely existingStacks, err := pp.collectPreviouslyDeployedStacks(ctx, projectName) // ... term.Debugf("Importing stack %s from remote", stackName) - _, err = pp.sm.Create(*stack) - if err != nil { - return nil, fmt.Errorf("failed to create local stack from remote: %w", err) + if useWkDir { + _, err = pp.sm.Create(*stack) + if err != nil { + return nil, fmt.Errorf("failed to create local stack from remote: %w", err) + } } }
♻️ Duplicate comments (1)
src/cmd/cli/command/commands.go (1)
1272-1283: Past review comment still applies regarding environment variable warnings.The warnings about detected AWS/DigitalOcean/GCP environment variables while defaulting to Defang playground may confuse users. Consider either removing these warnings or implementing auto-detection that uses the detected provider.
🧹 Nitpick comments (5)
src/pkg/agent/tools/provider_test.go (2)
523-529: Avoid mutating test case data during test execution.Modifying
tc.userStackSelectionin-place (lines 526-528) is a test smell that makes the test harder to reason about and could cause issues if test cases are ever run in parallel or reused.Consider computing the expected label separately:
if tc.hasOtherRemoteStacks { label := "remotestack (last deployed " + deployTime.Local().Format(time.RFC822) + ")" stackOptions = append(stackOptions, label) - // Update the expected selection to match the actual format - if strings.Contains(tc.userStackSelection, "remotestack") { - tc.userStackSelection = label - } } + // Compute the actual expected selection + expectedSelection := tc.userStackSelection + if tc.hasOtherRemoteStacks && strings.Contains(tc.userStackSelection, "remotestack") { + expectedSelection = "remotestack (last deployed " + deployTime.Local().Format(time.RFC822) + ")" + } if tc.useWkDir { stackOptions = append(stackOptions, CreateNewStack) } selectedOption := tc.userStackSelection + if tc.hasOtherRemoteStacks && strings.Contains(tc.userStackSelection, "remotestack") { + selectedOption = expectedSelection + } if tc.userSelectsCreateNew { selectedOption = CreateNewStack } - mockEC.On("RequestEnum", ctx, "Select a stack", "stack", stackOptions).Return(selectedOption, nil) + mockEC.On("RequestEnum", ctx, "Select a stack", "stack", stackOptions).Return(expectedSelection, nil)
613-621: Consider improving default name computation to match production code.The default name for non-Defang Playground providers is hardcoded to "defang" with a comment noting region processing would be needed. This doesn't match the production behavior which uses
stacks.MakeDefaultName(providerID, region). Consider using the same logic for test accuracy.src/cmd/cli/command/commands.go (1)
536-540: Consider the behavior when provider cannot be determined inwhoami.Setting
global.NonInteractive = trueat line 536 ensures no prompts are shown, but the error is silently logged at line 539. If the provider determination fails,whoamiwill still execute with a potentially nil or misconfigured provider, which might produce confusing results.Consider returning early or providing more context to the user when provider setup fails:
global.NonInteractive = true // don't show provider prompt provider, err := newProviderChecked(cmd.Context(), "", false) if err != nil { - term.Debug("unable to get provider:", err) + term.Debug("Provider info unavailable:", err) + // Continue with nil provider - cli.Whoami handles this gracefully }src/pkg/agent/tools/provider.go (2)
63-70: Consider accepting StacksManager as a constructor parameter for better testability.The current design requires tests to directly set
pp.smafter construction. AcceptingStacksManageras a parameter would make the dependency injection more explicit.-func NewProviderPreparer(pc ProviderCreator, ec elicitations.Controller, fc cliClient.FabricClient) *providerPreparer { +func NewProviderPreparer(pc ProviderCreator, ec elicitations.Controller, fc cliClient.FabricClient, sm StacksManager) *providerPreparer { + if sm == nil { + sm = NewStacksManager() + } return &providerPreparer{ pc: pc, ec: ec, fc: fc, - sm: NewStacksManager(), + sm: sm, } }
388-438: Good improvement to AWS authentication flow with minor redundancy.The AWS authentication now properly offers profile selection before falling back to access keys. However, line 398 re-checks
AWS_PROFILEwhich is redundant since line 389-391 already checked and returned early if it was set.func (pp *providerPreparer) SetupAWSAuthentication(ctx context.Context) error { if os.Getenv("AWS_PROFILE") != "" { return nil } if os.Getenv("AWS_ACCESS_KEY_ID") != "" && os.Getenv("AWS_SECRET_ACCESS_KEY") != "" { return nil } - // TODO: add support for aws sso strategy - if os.Getenv("AWS_PROFILE") == "" { - knownProfiles, err := listAWSProfiles() - if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) - } - if len(knownProfiles) > 0 { + // TODO: add support for aws sso strategy + knownProfiles, err := listAWSProfiles() + if err != nil { + return fmt.Errorf("failed to list AWS profiles: %w", err) + } + if len(knownProfiles) > 0 { // ... rest of profile selection logic - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cmd/cli/command/commands.go(14 hunks)src/cmd/cli/command/globals.go(2 hunks)src/cmd/cli/command/globals_test.go(3 hunks)src/pkg/agent/tools/provider.go(5 hunks)src/pkg/agent/tools/provider_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cmd/cli/command/globals.go
- src/cmd/cli/command/globals_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
src/pkg/agent/tools/provider_test.go (5)
src/pkg/cli/connect.go (1)
NewProvider(35-49)src/pkg/cli/client/provider_id.go (2)
ProviderID(10-10)ProviderDefang(14-14)src/pkg/stacks/stacks.go (4)
Create(33-76)StackParameters(17-22)List(85-118)StackListItem(78-83)src/protos/io/defang/v1/fabric.pb.go (2)
Provider_DEFANG(32-32)Provider_value(47-53)src/pkg/agent/tools/provider.go (1)
CreateNewStack(21-21)
src/pkg/agent/tools/provider.go (3)
src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(180-195)Load(197-208)List(85-118)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
🔇 Additional comments (11)
src/pkg/agent/tools/provider_test.go (3)
21-158: LGTM!The mock implementations are well-structured and correctly use testify/mock patterns. The FabricClient stub methods appropriately return nil for unused methods while ListDeployments is properly mocked for testing.
220-248: LGTM!The helper functions are well-designed for creating test data. The provider enum conversion correctly handles the "defang" special case and uses the proto-generated value map for other providers.
418-461: LGTM!The test function is well-structured with proper isolation (fresh mocks per test), clear assertions, and appropriate conditional mock verification.
src/cmd/cli/command/commands.go (5)
438-449: LGTM!The conditional check at line 438 properly addresses the concern about loading local
.defangstack files when--project-nameis used. This prevents cross-project stack file access as intended by the PR objectives.
1314-1334: LGTM!The new provider setup flow properly integrates the elicitations controller and provider preparer. The non-interactive mode fallback is appropriate, and the error wrapping provides clear context.
587-592: LGTM!The pattern of deriving
saveStacksToWkDirfrom whetherproject-nameflag is set is consistently applied across commands. This correctly implements the PR objective of restricting stack file operations when--project-nameis used.
926-941: LGTM!The config list command follows the established pattern correctly, using
LoadProjectNameand derivingsaveStacksToWkDirappropriately.
89-99: LGTM!The placeholder project name
"<name>"is appropriate for this error handling path where we're generating a hint message but don't have a specific project context.src/pkg/agent/tools/provider.go (3)
27-54: LGTM!The
StacksManagerinterface provides a clean abstraction for stack operations, enabling proper unit testing with mocks. The implementation correctly delegates to the underlyingstackspackage.
235-268: LGTM!The function correctly handles the implicit "beta" stack naming convention and safely converts proto deployments to internal types. The comment about sort order documents the assumption clearly.
287-319: LGTM on shadowing fix, but unconditional stack creation remains.The variable shadowing issue from the past review has been properly addressed. The
stackvariable is now correctly updated within the same scope. However, the unconditionalpp.sm.Createat line 308 should be gated byuseWkDiras noted in the earlier comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pkg/agent/tools/provider.go (2)
217-225: Remote stack import should respectuseWkDir.When a user selects a remote stack, lines 221-225 unconditionally create a local stack file. However, when
useWkDirisfalse(e.g., when--project-nameis used), the PR objectives state that stack file access should be restricted to prevent cross-project pollution. The import should only write the local file whenuseWkDiristrue.Apply this diff:
if selectedStackOption.Parameters == nil { return "", fmt.Errorf("stack parameters for remote stack %q are nil", selectedStackLabel) } - term.Debugf("Importing stack %s from remote", selectedStackLabel) - _, err = pp.sm.Create(*selectedStackOption.Parameters) - if err != nil { - return "", fmt.Errorf("failed to create local stack from remote: %w", err) + if useWkDir { + term.Debugf("Importing stack %s from remote", selectedStackLabel) + _, err = pp.sm.Create(*selectedStackOption.Parameters) + if err != nil { + return "", fmt.Errorf("failed to create local stack from remote: %w", err) + } } return selectedStackOption.Name, nil
164-177: Beta detection logic is still incorrect.The check on line 165 looks for a map key
"beta", but when a beta stack exists only remotely (the primary use case this messaging is meant to address), the key instackswill be"beta (last deployed ...)"instead of"beta". This means the onboarding message won't appear for users with implicit beta deployments.Additionally, line 176 accesses
os.Args[1]without checkinglen(os.Args) > 1, which can panic.Apply this diff to fix both issues:
func printStacksInfoMessage(stacks map[string]StackOption) { - _, betaExists := stacks["beta"] + betaExists := false + for _, opt := range stacks { + if opt.Name == "beta" { + betaExists = true + break + } + } if betaExists { infoLine := "This project was deployed with an implicit Stack called 'beta' before Stacks were introduced." if len(stacks) == 1 { infoLine += "\n To update your existing deployment, select the 'beta' Stack.\n" + "Creating a new Stack will result in a separate deployment instance." } infoLine += "\n To learn more about Stacks, visit: https://docs.defang.io/docs/concepts/stacks" term.Info(infoLine + "\n") } executable, _ := os.Executable() - term.Infof("To skip this prompt, run %s %s --stack=%s", filepath.Base(executable), os.Args[1], "<stack_name>") + subcommand := "" + if len(os.Args) > 1 { + subcommand = os.Args[1] + } + term.Infof("To skip this prompt, run %s %s --stack=%s", filepath.Base(executable), subcommand, "<stack_name>") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pkg/agent/tools/provider.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/agent/tools/provider.go (4)
src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(180-195)Load(197-208)List(85-118)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/timeutils/timeutils.go (1)
AsTime(39-44)
🔇 Additional comments (7)
src/pkg/agent/tools/provider.go (7)
27-54: LGTM!The
StacksManagerabstraction cleanly wraps thestackspackage, enabling testability through dependency injection. The implementation is straightforward and follows good separation of concerns.
114-162: LGTM!The method correctly merges remote and local stacks, preferring local when both exist. The two-map approach (name-keyed
stackMapfor merging, label-keyedstackLabelMapfor display) properly handles the presentation of timestamps while maintaining the preference for local stacks.
388-436: LGTM!The AWS authentication flow correctly handles multiple scenarios: existing credentials, profile selection, and manual key entry. The logic properly prioritizes AWS_PROFILE, then access keys, then prompts for credentials with automatic profile discovery.
484-528: LGTM!The function correctly parses AWS profile names from both
~/.aws/credentialsand~/.aws/config, properly handling the "profile " prefix in config files. Error handling and resource cleanup are appropriate.
72-105: Verify consistentuseWkDirthreading.The signature correctly adds
projectNameanduseWkDirparameters to support the PR's cross-project isolation goals. However, ensure thatloadStackis updated to receive and respectuseWkDir(see separate comment on that method).
321-374: LGTM!The method correctly guards local stack file creation with
useWkDir(line 361-367), properly implementing the cross-project isolation pattern. This is the correct approach that should be mirrored inselectStackandloadStack.
270-285: Update to passuseWkDirtoloadStack.Line 284 calls
loadStackwithout theuseWkDirparameter. OnceloadStackis updated to accept this parameter (see separate comment), this call site needs to be updated as well.
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
39a1469 to
38bb177
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/pkg/agent/tools/provider.go (3)
164-177: Beta-stack detection and skip-prompt hint don’t match actual labels / commandsTwo issues here:
Beta detection won’t fire for remote-only “beta” stacks
printStacksInfoMessagechecksstacks["beta"], butcollectStackOptionskeys the map by label ("beta (last deployed ...)"), not bare name. For implicit “beta” deployments that only exist remotely, the key is"beta (last deployed ...)", so the beta-onboarding message is never shown.Consider detecting beta by underlying
StackOption.Nameinstead of the key:func printStacksInfoMessage(stacks map[string]StackOption) { - _, betaExists := stacks["beta"] - if betaExists { + betaExists := false + for _, opt := range stacks { + if opt.Name == "beta" { + betaExists = true + break + } + } + if betaExists { infoLine := "This project was deployed with an implicit Stack called 'beta' before Stacks were introduced." // ... }Hard-coded
upsubcommand can be misleadingThe hint always says
run %s up --stack=%s, butSetupProvideris used by multiple subcommands. That’s confusing when you’re in, say,logsordestroy.You can safely recover the actual subcommand without risking panics:
executable, _ := os.Executable() - term.Infof("To skip this prompt, run %s up --stack=%s", filepath.Base(executable), "<stack_name>") + binary := filepath.Base(executable) + subcommand := "<command>" + if len(os.Args) > 1 { + subcommand = os.Args[1] + } + term.Infof("To skip this prompt, run %s %s --stack=%s", binary, subcommand, "<stack_name>")
179-228: Remote stack import inselectStackstill writes stackfiles whenuseWkDir=falseWhen selecting a remote stack:
if selectedStackOption.Local { return selectedStackOption.Name, nil } if selectedStackOption.Parameters == nil { ... } term.Debugf("Importing stack %s from remote", selectedStackLabel) _, err = pp.sm.Create(*selectedStackOption.Parameters)
pp.sm.Createis called unconditionally, even whenuseWkDirisfalse. That:
- Violates the PR goal of restricting stackfile writes when operating outside the working dir / with
--project-name.- Can create
.defang/<stack>files in the caller’s cwd even in “no wkdir”/agent flows.You already guard writes in
createNewStackandloadStackwithuseWkDir; this should be consistent:- if selectedStackOption.Parameters == nil { - return "", fmt.Errorf("stack parameters for remote stack %q are nil", selectedStackLabel) - } - - term.Debugf("Importing stack %s from remote", selectedStackLabel) - _, err = pp.sm.Create(*selectedStackOption.Parameters) - if err != nil { - return "", fmt.Errorf("failed to create local stack from remote: %w", err) - } - - return selectedStackOption.Name, nil + if selectedStackOption.Parameters == nil { + return "", fmt.Errorf("stack parameters for remote stack %q are nil", selectedStackLabel) + } + + if useWkDir { + term.Debugf("Importing stack %s from remote", selectedStackLabel) + if _, err = pp.sm.Create(*selectedStackOption.Parameters); err != nil { + return "", fmt.Errorf("failed to create local stack from remote: %w", err) + } + } + + return selectedStackOption.Name, nilThis keeps remote import behavior for working-dir flows while avoiding cross-project stackfile writes when
useWkDir=false.
274-325:loadStack+selectOrCreateStackbreak “no-wkdir” new/remote stack flows with real StacksManagerFor
useWkDir=falseflows, there are two problematic cases:
New stacks (no local file, no remote)
selectOrCreateStackalways callsloadStackeven aftercreateNewStack.
createNewStackdeliberately does not write a stackfile whenuseWkDir=false.
loadStackimmediately callspp.sm.Read(stackName). With the realNewStacksManager, this isstacks.Read, which fails withos.ErrNotExist.There are no remote deployments for a brand-new stack, so
collectPreviouslyDeployedStacksfinds nothing and you return:if stack == nil { return nil, fmt.Errorf("stack %q does not exist locally or remotely", stackName) }This contradicts the tests that expect an ephemeral, no‑wkdir new stack to succeed.
Remote-only stacks with
useWkDir=false
- When a stack exists only in Fabric deployments,
loadStackfinds it viacollectPreviouslyDeployedStacksbut still unconditionally callspp.sm.Load(stackName)afterward.- With
useWkDir=falseyou never wrote.defang/<stackName>, sostacks.Loadwill fail (no such file or directory), andSetupProviderreturns “failed to load stack”.In tests, mocks stub
StacksManager.Read/Loadto succeed for these scenarios, so the discrepancy is hidden.This conflicts both with the PR’s “no cross‑project stackfile access” objective and with the behavior your tests document.
You likely want “ephemeral” semantics when
useWkDir=false:
- Don’t require a stackfile to exist.
- Use remote deployment data or the parameters returned by
createNewStackdirectly.- Avoid
StacksManager.Read/Loadentirely in this mode.One possible direction (illustrative, not drop‑in):
func (pp *providerPreparer) selectOrCreateStack(ctx context.Context, projectName string, useWkDir bool) (*stacks.StackParameters, error) { selectedStackName, err := pp.selectStack(ctx, projectName, useWkDir) if err != nil { return nil, fmt.Errorf("failed to select stack: %w", err) } if selectedStackName == CreateNewStack { - newStack, err := pp.createNewStack(ctx, useWkDir) + newStack, err := pp.createNewStack(ctx, useWkDir) // ideally returns *stacks.StackParameters if err != nil { return nil, fmt.Errorf("failed to create new stack: %w", err) } - selectedStackName = newStack.Name + if !useWkDir { + // Ephemeral: don’t touch stackfiles at all. + return newStack, nil + } + selectedStackName = newStack.Name } - return pp.loadStack(ctx, projectName, selectedStackName, useWkDir) + return pp.loadStack(ctx, projectName, selectedStackName, useWkDir) }and in
loadStack:-func (pp *providerPreparer) loadStack(ctx context.Context, projectName, stackName string, useWkDir bool) (*stacks.StackParameters, error) { - stack, err := pp.sm.Read(stackName) +func (pp *providerPreparer) loadStack(ctx context.Context, projectName, stackName string, useWkDir bool) (*stacks.StackParameters, error) { + if !useWkDir { + // Ephemeral mode: resolve from remote deployments only, no stackfile reads/writes. + existingStacks, err := pp.collectPreviouslyDeployedStacks(ctx, projectName) + if err != nil { + return nil, fmt.Errorf("failed to collect existing stacks: %w", err) + } + for _, existing := range existingStacks { + if existing.Name == stackName { + return &existing.StackParameters, nil + } + } + return nil, fmt.Errorf("stack %q does not exist remotely", stackName) + } + + stack, err := pp.sm.Read(stackName) // ... existing local+remote+Create+Load logic ...This would:
- Make tests for “no wkdir” cases reflect real behavior.
- Keep current behavior for working-dir flows, including stackfile creation and loading.
src/pkg/agent/tools/provider_test.go (1)
250-351: No-wkdir new/remote stack test cases still don’t match real StacksManager behaviorSeveral
TestSetupProvidercases model ephemeral behavior foruseWkDir=falsethat the concreteNewStacksManagerdoes not provide:
- “no stackname, no local stackfiles, no deployments, useWkDir false, create stack without stackfile”
- “no stackname, local stackfile exists, useWkDir false, create new stack without stackfile”
- “stackname provided, stackfile doesn’t exist locally, exists in previous deployments, useWkDir false, expect remote stack loaded but stackfile not written”
In these cases:
createNewStack/ remote import deliberately avoid callingStacksManager.CreatewhenuseWkDir=false.- With the real implementation,
StacksManager.Read/Loadcall through tostacks.Read/stacks.Load, which require.defang/<stack>to exist.- Yet the tests stub
ReadandLoadto succeed (expectNewStackCreatedand the remoteuseWkDir=falsecase), soSetupProviderappears to work even though production code would fail with “stack ... does not exist locally or remotely” or aLoaderror.This masks the production issue called out in provider.go (load/Read behavior in no‑wkdir mode) and makes the tests assert an idealized behavior that doesn’t exist.
You should either:
- Align production with tests: implement true “ephemeral” semantics when
useWkDir=false(don’t depend on stackfiles; see the earlier suggestion inloadStack/selectOrCreateStack), and then keep these test expectations; or- Align tests with current behavior: for
useWkDir=falsenew/remote cases, stop stubbingRead/Loadto succeed and assert thatSetupProviderfails (or adjust the table expectations), so tests don’t claim support that the CLI doesn’t actually have.Right now the mocks hide the discrepancy instead of surfacing it.
Also applies to: 463-560
🧹 Nitpick comments (3)
src/pkg/agent/tools/provider.go (1)
395-442: AWS auth flow matches requirements; consider softening profile-read failuresThe updated
SetupAWSAuthenticationandlistAWSProfilesbehavior is aligned with the PR goals:
- Respect pre-set
AWS_PROFILEorAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYwithout prompting.- When profiles are available, prompt for a profile first, with a clear “Use Access Key ID and Secret Access Key” escape hatch.
- When no profiles are found, fall back to prompting for keys.
listAWSProfilesnow checksscanner.Err()and reports IO/parsing errors instead of silently ignoring them.One behavioral nuance: any non‑nil error from
listAWSProfilesnow aborts the auth setup ("failed to list AWS profiles"), even though you could still recover by falling back to access keys.If you want a more resilient UX, you could treat profile-read errors as a warning and continue:
knownProfiles, err := listAWSProfiles() if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) + // Log at debug/warn level instead of failing; fall back to access keys. + // term.Debugf("failed to list AWS profiles, falling back to access keys: %v", err) + knownProfiles = nil }The rest of the flow (including the scanner error handling in
listAWSProfiles) looks good.Also applies to: 490-534
src/pkg/agent/tools/provider_test.go (2)
488-539: Stack selection tests depend on map iteration order of optionsIn
setupMocksyou buildstackOptionsas:stackOptions := []string{} if tc.hasOtherLocalStacks && tc.useWkDir { stackOptions = append(stackOptions, "existingstack") } if tc.hasOtherRemoteStacks { label := "remotestack (last deployed " + deployTime.Local().Format(time.RFC822) + ")" stackOptions = append(stackOptions, label) // ... } if tc.useWkDir { stackOptions = append(stackOptions, CreateNewStack) } mockEC.On("RequestEnum", ctx, "Select a stack", "stack", stackOptions).Return(selectedOption, nil)But production
selectStackbuilds theoptionsslice from a map:stackLabels := make([]string, 0, len(stackOptions)+1) for label := range stackOptions { stackLabels = append(stackLabels, label) } if useWkDir { stackLabels = append(stackLabels, CreateNewStack) }Map iteration order is non-deterministic in Go, so:
- The concrete order of options presented to the user is arbitrary.
- The order of the
options []stringargument passed intoRequestEnumin production won’t necessarily match thestackOptionsslice you use in the mock expectation, making the test brittle or outright incorrect.I’d recommend:
- Sorting
stackLabelsbefore callingRequestEnumin production for a stable, user-friendly order and- Mirroring that in tests (build
stackOptions, thensort.Strings(stackOptions)before setting the expectation), or relaxing the expectation to usemock.AnythingOfType("[]string")if order truly doesn’t matter.Without this, these tests rely on unspecified map order.
596-633: Default-name logic in tests diverges fromcreateNewStack/stacks.MakeDefaultName
setupNewStackCreationMockshard-codes:var defaultName string switch tc.userProviderChoice { case "Defang Playground": defaultName = "defang" default: // For other providers, would need region processing defaultName = "defang" } mockEC.On("RequestStringWithDefault", ..., defaultName).Return(tc.userStackNameChoice, nil)But
createNewStacknow usesstacks.MakeDefaultName(providerID, region), which (per PR description) builds a provider+region-based name with non-alphanumerics stripped.Today all your test cases use
"Defang Playground", so this mismatch isn’t visible, but as soon as you add scenarios for AWS/GCP/DO, the default name in tests will no longer match production behavior.To keep tests faithful:
- Either call
stacks.MakeDefaultNameinsidesetupNewStackCreationMocks(using the same providerID/region mapping you expect in production), or- At least replicate its logic instead of hard-coding
"defang"for non-Defang providers.That will help keep tests aligned as the default-name rules evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pkg/agent/tools/provider.go(6 hunks)src/pkg/agent/tools/provider_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pkg/agent/tools/provider_test.go (3)
src/pkg/cli/client/provider_id.go (2)
ProviderID(10-10)ProviderDefang(14-14)src/pkg/stacks/stacks.go (5)
Create(33-76)StackParameters(17-22)Load(197-208)List(85-118)StackListItem(78-83)src/pkg/agent/tools/provider.go (1)
CreateNewStack(21-21)
src/pkg/agent/tools/provider.go (1)
src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(180-195)Load(197-208)List(85-118)StackListItem(78-83)
⏰ 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/pkg/agent/tools/provider.go (2)
27-72: StacksManager abstraction and SetupProvider wiring look soundThe StacksManager interface and stacksManager wrapper cleanly encapsulate
stacks.Create/Read/Load/List, and injecting it intoproviderPreparermakes testing straightforward. The updatedSetupProvidersignature (projectName,*stackName,useWkDir) and flow (nil check, load vs select/create, then auth and provider creation) are consistent and easy to follow.
114-162: Remote/local stack merging and timestamp handling are correctThe
collectStackOptions/ExistingStack/collectPreviouslyDeployedStackspipeline correctly:
- Normalizes empty remote stack names to
"beta".- Avoids overwriting newer entries by using a per‑name map.
- Preserves
LastDeployedAtacross local/remote merges while preferring local when both exist.- Safely handles possibly‑nil deployment timestamps before calling
AsTime().This addresses the earlier nil‑timestamp panic risk and matches the “prefer local if both exist” comment.
Also applies to: 230-272
src/pkg/agent/tools/provider_test.go (1)
21-219: Mocks for providers, fabric client, stacks manager, and elicitations are appropriateThe mock types (
mockProviderCreator,mockElicitationsController,mockFabricClient,mockStacksManager,mockProvider) cover exactly the methods exercised inSetupProviderand keep everything in-package, which makes the tests focused and maintainable. The generic return handling (type assertions and nil‑safe fallbacks) is reasonable for these tests.
jordanstephens
force-pushed
the
jordan/stacks-default-name
branch
from
38bb177 to
ff6d7e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/pkg/agent/tools/provider.go (1)
114-162: Beta-stack info message never triggers for remote “beta” stacks and CLI hint hardcodesup.
collectStackOptionsnow returns a map keyed by display labels like"beta (last deployed ...)", butprintStacksInfoMessagestill checksstacks["beta"]. This means users with only implicit-betaremote deployments never see the beta-onboarding message, which undermines the PR’s goal.Also, the skip-hint currently assumes the
upsubcommand, whileSetupProvideris reused by other subcommands.You can fix both by iterating values and decoupling the message from a specific subcommand:
-func printStacksInfoMessage(stacks map[string]StackOption) { - _, betaExists := stacks["beta"] - if betaExists { - infoLine := "This project was deployed with an implicit Stack called 'beta' before Stacks were introduced." - if len(stacks) == 1 { +func printStacksInfoMessage(stackOptions map[string]StackOption) { + betaExists := false + onlyBeta := true + for _, opt := range stackOptions { + if opt.Name == "beta" { + betaExists = true + } else { + onlyBeta = false + } + } + if betaExists { + infoLine := "This project was deployed with an implicit Stack called 'beta' before Stacks were introduced." + if onlyBeta { infoLine += "\n To update your existing deployment, select the 'beta' Stack.\n" + "Creating a new Stack will result in a separate deployment instance." } infoLine += "\n To learn more about Stacks, visit: https://docs.defang.io/docs/concepts/stacks" term.Info(infoLine + "\n") } executable, _ := os.Executable() - term.Infof("To skip this prompt, run %s up --stack=%s", filepath.Base(executable), "<stack_name>") + term.Infof("To skip this prompt, run %s <subcommand> --stack=%s", filepath.Base(executable), "<stack_name>") }If you know the actual subcommand at the call site, you could pass it in instead of using
<subcommand>.Also applies to: 164-177
src/pkg/agent/tools/provider_test.go (1)
250-351: No-wkdir stack tests assume behavior that the realStacksManagerdoesn’t provide.Several
TestSetupProvidercases withuseWkDir=falseand either remote-only or new stacks:
- “stackfile doesn't exist locally, exists in previous deployments, useWkDir false, expect remote stack loaded but stackfile not written”
- “no stackname, no local stackfiles, no deployments, useWkDir false, create stack without stackfile”
- “no stackname, local stackfile exists, useWkDir false, create new stack without stackfile”
- “no stackname, previous deployment exists, useWkDir false, user selects existing deployment”
set
expectNewStackCreatedorremoteStackExistsbutexpectStackFileWritten=false.In
setupMocksand helpers:
setupNewStackCreationMocksonly stubsmockSM.Createwhentc.useWkDiris true.- For
expectNewStackCreatedcases, you still stubmockSM.Read(tc.expectedStackName)andmockSM.Load(tc.expectedStackName)to succeed (lines 568–572), even whenuseWkDir=false.- For remote-only selections with
expectStackFileWritten=false,setupExistingRemoteStackMocksstubsRead/LoadwithoutCreate.With the real
NewStacksManager,ReadandLoadaccess.defang/<stack>on disk. WhenuseWkDir=false, the production code doesn’t write these files (per the new guards increateNewStack/selectStack), so:
- New-stack + no-wkdir cases will fail in
loadStackwith “stack %q does not exist locally or remotely”.- Remote-only + no-wkdir cases will eventually call
stacks.Loadon a non-existent file and error.That means the tests are currently validating a desirable “ephemeral stack, no local file” behavior that the implementation doesn’t yet have, rather than catching the present mismatch.
I’d recommend either:
Implement true ephemeral behavior (preferred)
- Adjust
loadStack(and possiblyselectOrCreateStack) so that whenuseWkDir=falseand a stack is resolved remotely or newly created, it does not callStacksManager.Loadand does not require a local file (return just*stacks.StackParameters, as suggested in the provider.go comment).- Update mocks accordingly:
- For no-wkdir+new/remote cases, stop stubbing
Read/Loadand instead only assert thatCreateis not called and thatNewProviderreceives the expected stack name/provider/region.Or, constrain the contract and tests
- Decide that no-wkdir ephemeral stacks aren’t supported.
- Change these test cases to expect an error (
require.Error) instead of success, and remove theRead/Loadstubs for scenarios where no stackfile should exist.Right now, the mocking hides a real discrepancy between intended semantics and what
NewStacksManageractually does.Also applies to: 385-400, 478-575, 592-648
🧹 Nitpick comments (2)
src/pkg/agent/tools/provider.go (2)
179-200: Stack selection order is nondeterministic; sortstackLabelsbefore prompting.
stackLabelsis built from a map, so order is random. This can be confusing in the UI and makes strict mocking ofRequestEnumbrittle.Consider sorting the labels before appending the
"Create new stack"option:- // Convert map back to slice - stackLabels := make([]string, 0, len(stackOptions)+1) - for label := range stackOptions { - stackLabels = append(stackLabels, label) - } + // Convert map back to a deterministically ordered slice + stackLabels := make([]string, 0, len(stackOptions)+1) + for label := range stackOptions { + stackLabels = append(stackLabels, label) + } + sort.Strings(stackLabels) if useWkDir { stackLabels = append(stackLabels, CreateNewStack) }This keeps the UX stable and avoids order-sensitive test issues.
396-444: AWS auth/profile discovery flow looks solid; minor behavior note on profile listing errors.The new AWS setup flow correctly:
- Short-circuits when
AWS_PROFILEor bothAWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEYare already set.- Offers a profile picker when profiles are found, with a clear “Use Access Key ID and Secret Access Key” escape hatch.
- Falls back to prompting for access keys only when needed.
- Handles missing
~/.aws/{credentials,config}files gracefully inlistAWSProfiles.One behavioral nuance: failures in
listAWSProfiles(e.g., unreadable files or scanner errors) currently abort provider setup instead of falling back to manual key entry. If you want a more resilient UX, consider treating those errors like “no profiles found” and continuing to the key prompts instead of returning an error.Also applies to: 492-536
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pkg/agent/tools/provider.go(6 hunks)src/pkg/agent/tools/provider_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/pkg/agent/tools/provider.go (3)
src/pkg/stacks/stacks.go (5)
StackParameters(17-22)Read(180-195)Load(197-208)List(85-118)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
⏰ 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
to load a stack without a file
This reverts commit d3c091f.
this required updating a bunch of methods to use the FabricClient interface instead of the GrpcClient struct so it could be mocked : (
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/agent/tools/provider.go (1)
399-459: AWS auth flow: listing profiles shouldn’t hard-fail the whole setup
SetupAWSAuthenticationnow treats any error fromlistAWSProfiles()as fatal:knownProfiles, err := listAWSProfiles() if err != nil { return fmt.Errorf("failed to list AWS profiles: %w", err) }but
listAWSProfilescan fail for environmental reasons (e.g.,os.UserHomeDirproblems, unexpected I/O errors) even though the user could still proceed by manually entering access keys. This is a behavior regression versus a simple “prompt for keys” flow.Consider downgrading profile‑listing errors to a warning and falling back to the access‑key prompts, so AWS auth remains possible even when profile files are unreadable.
- knownProfiles, err := listAWSProfiles() - if err != nil { - return fmt.Errorf("failed to list AWS profiles: %w", err) - } + knownProfiles, err := listAWSProfiles() + if err != nil { + term.Warnf("Unable to list AWS profiles, falling back to access keys: %v", err) + knownProfiles = nil + } @@ - if len(knownProfiles) > 0 { + if len(knownProfiles) > 0 { const useAccessKeysOption = "Use Access Key ID and Secret Access Key"This preserves the new, nicer profile‑selection UX without blocking users in edge environments.
Also applies to: 507-551
♻️ Duplicate comments (1)
src/pkg/agent/tools/provider.go (1)
173-186: Beta-stack onboarding message still keyed off labels, so implicit beta deployments are missed
printStacksInfoMessagechecks for"beta"in the map keys:_, betaExists := stacks["beta"]but the map passed here (
stackOptionsfromcollectStackOptions) is keyed by display labels like"beta (last deployed 02 Jan 06 15:04 MST)"whenever aLastDeployedAtis set. For the common case of existing remote deployments with an emptyStack(normalized to"beta"incollectPreviouslyDeployedStacks), you’ll only have a"beta (last deployed ...)"key, sobetaExistsis false and the beta‑onboarding message is never shown.Instead, detection should be based on the underlying
StackOption.Name, not the label key. This matches the earlier review that pointed out the same issue.-func printStacksInfoMessage(stacks map[string]StackOption) { - _, betaExists := stacks["beta"] - if betaExists { +func printStacksInfoMessage(stacks map[string]StackOption) { + betaExists := false + for _, opt := range stacks { + if opt.Name == "beta" { + betaExists = true + break + } + } + if betaExists { infoLine := "This project was deployed with an implicit Stack called 'beta' before Stacks were introduced." @@ - term.Infof("To skip this prompt, run %s up --stack=%s", filepath.Base(executable), "<stack_name>") + term.Infof("To skip this prompt, run %s up --stack=%s", filepath.Base(executable), "<stack_name>") }(You can keep or further refine the “up” subcommand hint depending on where this helper is used.)
🧹 Nitpick comments (7)
src/pkg/agent/tools/deploy_test.go (1)
20-51: AlignMockGrpcClientwithFabricClientand haveConnectactually use itRight now
Connectreturns a bare*client.GrpcClienteven in the error case, and theMockGrpcClienttype (with the comment about handlingTracksafely) is not used. That’s a bit misleading and misses the chance to isolate tests from any realTrackbehavior.You can tighten this up by:
- Making
MockGrpcClient.Trackmatch theFabricClientsignature (...client.Property).- Returning
nilon error fromConnect.- Returning a
*MockGrpcClienton success.For example:
type MockGrpcClient struct { *client.GrpcClient } -func (m *MockGrpcClient) Track(event string, props ...interface{}) error { +func (m *MockGrpcClient) Track(event string, props ...client.Property) error { // Mock implementation that doesn't panic return nil } @@ func (m *MockDeployCLI) Connect(ctx context.Context, cluster string) (client.FabricClient, error) { m.CallLog = append(m.CallLog, fmt.Sprintf("Connect(%s)", cluster)) if m.ConnectError != nil { - return &client.GrpcClient{}, m.ConnectError + return nil, m.ConnectError } - // Return a base GrpcClient - we need to handle Track method differently - return &client.GrpcClient{}, nil + // Return a mock FabricClient that safely no-ops Track + return &MockGrpcClient{GrpcClient: &client.GrpcClient{}}, nil }This keeps the tests robust against future uses of
Trackand makes the mock behavior closer to the real API surface.src/pkg/stacks/stacks.go (1)
147-165: Env population helper and LoadParameters look correct; consider minor defensive tweaks
paramsToMapandLoadParametersmirror the previous godotenv-based behavior and keep provider/region/mode handling centralized; the logic is sound and aligns with the new stacks manager usage.Two small optional improvements:
- Add a nil-guard in
LoadParameters(and/orparamsToMap) to avoid panics if a caller ever passesnil.- In
LoadParameters, you could usestrings.Cut(rawEnvLine, "=")instead ofstrings.Splitfor slightly clearer intent and less allocation.These are non-blocking; current implementation is functionally fine.
Also applies to: 215-230
src/pkg/agent/tools/provider_test.go (1)
228-247: Tidy up test case descriptions and default-name behavior for clarityThe new
TestSetupProvidercoverage and mocking strategy look solid and, importantly, now reflect the intendeduseWkDir=false“no local stackfile writes” behavior.There are a couple of minor clarity issues you may want to address:
- The test case named
"stackname provided, stackfile exists locally, no previous deployments, useWkDir false, expect stack to be loaded"setsremoteStackExists: true(Line 288), which implies there are previous deployments. Either flip that flag or adjust the test name to avoid confusion.- In
setupNewStackCreationMocks,defaultNameis always"defang"even for non-Defang providers (Lines 661‑668). The production flow now uses a provider+region-based default stack name; updating this helper to mirror that (or to call the same helper used inprovider.go) would keep the tests aligned with the actual UX, even though the current tests don’t assert on the default value.These are non-blocking, but cleaning them up will make the tests easier to maintain and reason about.
Also applies to: 249-430, 644-681
src/pkg/agent/tools/destroy_test.go (1)
19-32: Destroy‑tool tests correctly reflect the new flow; consider avoiding shared globalmockFCThe changes to:
- return
client.FabricClientfromConnect,- run
LoadProjectNamebefore provider setup,- pass FabricClient into
ComposeDownandCanIUseProvider, and- stub
mockFC.ListDeploymentsfor stack resolution,all match the updated interfaces and the intended “load project name, then set up provider for the selected stack” behavior. The successful path’s call sequence assertion is especially useful.
One structural nit:
mockFCis a shared package-level variable reused across this and other test files. It’s currently safe (not.Paralleland each top-level test resets it), but it does couple tests together. If you ever parallelize or add additional uses, consider injecting aFabricClientinto your CLI mocks directly (e.g., as a field onMockDestroyCLI/MockRemoveConfigCLI/ etc.) instead of going through a shared global.This is a nice-to-have; the current tests are functionally fine.
Also applies to: 33-39, 46-60, 62-68, 70-72, 153-165, 182-191
src/pkg/agent/tools/interfaces.go (1)
4-15: CLIInterface refactor to FabricClient and LoadProjectName looks consistent; imports can be simplifiedThe interface changes:
- switching all relevant methods to
cliClient.FabricClient(Connect, ComposeUp/Down, CanIUseProvider, InteractiveLoginMCP, RunEstimate, CreatePlaygroundProvider), and- replacing
LoadProjectNameWithFallbackwithLoadProjectName(ctx, loader),line up with the updated mocks and tool flows in the tests you added/changed. This centralizes the FabricClient usage nicely.
One minor cleanup opportunity: you import
github.com/DefangLabs/defang/src/pkg/clitwice under different names (cliandcliTypes). Since both refer to the same package, you can drop one and use a single alias consistently (e.g., justclifor bothComposeUpParamsandTailOptions/ServiceStates) to reduce confusion.Functionally everything here looks good.
Also applies to: 17-36
src/pkg/agent/tools/default_tool_cli.go (1)
32-42: DefaultToolCLI FabricClient wiring and LoadProjectName passthrough look goodThe migration from
*GrpcClienttocliClient.FabricClientacrossCanIUseProvider,RunEstimate,Connect,ComposeUp,ComposeDown,CreatePlaygroundProvider, andInteractiveLoginMCPis coherent and preserves behavior. The newLoadProjectNamedelegating toloader.LoadProjectName(ctx)aligns with the refactor to resolve project names before provider setup. The remaining// TODO: add stackinCanIUseProvideris a reasonable follow‑up rather than a blocker.Also applies to: 49-55, 61-63, 86-92, 94-100, 107-109
src/pkg/agent/tools/provider.go (1)
246-276: Optional: handle invalid provider IDs from deployments more defensivelyIn
collectPreviouslyDeployedStacks, any error fromproviderID.SetValue(deployment.GetProvider())is ignored:var providerID cliClient.ProviderID providerID.SetValue(deployment.GetProvider())If the backend ever returns an unexpected provider string, you’ll silently record a zero‑value provider in
StackParameters, which might later cause confusing behavior when setting up the provider.Not critical today, but consider checking the error and either skipping that deployment or surfacing a clearer error if it’s invalid.
- var providerID cliClient.ProviderID - providerID.SetValue(deployment.GetProvider()) + var providerID cliClient.ProviderID + if err := providerID.SetValue(deployment.GetProvider()); err != nil { + term.Warnf("Skipping deployment with unknown provider %q: %v", deployment.GetProvider(), err) + continue + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/pkg/agent/tools/default_tool_cli.go(4 hunks)src/pkg/agent/tools/deploy_test.go(3 hunks)src/pkg/agent/tools/destroy_test.go(5 hunks)src/pkg/agent/tools/estimate_test.go(4 hunks)src/pkg/agent/tools/interfaces.go(1 hunks)src/pkg/agent/tools/listConfig_test.go(6 hunks)src/pkg/agent/tools/provider.go(6 hunks)src/pkg/agent/tools/provider_test.go(1 hunks)src/pkg/agent/tools/removeConfig_test.go(6 hunks)src/pkg/agent/tools/services_test.go(13 hunks)src/pkg/agent/tools/setConfig_test.go(7 hunks)src/pkg/stacks/stacks.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/pkg/agent/tools/provider_test.go (3)
src/pkg/cli/client/provider_id.go (2)
ProviderID(10-10)ProviderDefang(14-14)src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(185-200)LoadParameters(215-230)List(85-118)StackListItem(78-83)src/pkg/agent/tools/provider.go (1)
CreateNewStack(21-21)
src/pkg/agent/tools/removeConfig_test.go (3)
src/pkg/cli/connect.go (1)
Connect(16-33)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/protos/io/defang/v1/fabric.pb.go (1)
Provider_AWS(33-33)
src/pkg/agent/tools/setConfig_test.go (2)
src/pkg/cli/connect.go (1)
Connect(16-33)src/pkg/cli/client/client.go (1)
FabricClient(17-44)
src/pkg/agent/tools/destroy_test.go (4)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(17-36)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)
src/pkg/agent/tools/listConfig_test.go (3)
src/pkg/cli/connect.go (2)
Connect(16-33)NewProvider(35-49)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/protos/io/defang/v1/fabric.pb.go (1)
Provider_AWS(33-33)
src/pkg/agent/tools/services_test.go (2)
src/pkg/agent/tools/interfaces.go (1)
CLIInterface(17-36)src/pkg/cli/client/client.go (1)
FabricClient(17-44)
src/pkg/agent/tools/estimate_test.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/estimate.go (1)
RunEstimate(23-41)
src/pkg/agent/tools/interfaces.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/connect.go (1)
NewProvider(35-49)
src/pkg/agent/tools/provider.go (3)
src/pkg/stacks/stacks.go (6)
Create(33-76)StackParameters(17-22)Read(185-200)LoadParameters(215-230)List(85-118)StackListItem(78-83)src/pkg/elicitations/elicitations.go (1)
Controller(8-12)src/pkg/cli/client/provider_id.go (1)
ProviderID(10-10)
src/pkg/agent/tools/deploy_test.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/composeUp.go (2)
ComposeUp(33-178)ComposeUpParams(25-30)
src/pkg/agent/tools/default_tool_cli.go (4)
src/pkg/cli/client/caniuse.go (1)
CanIUseProvider(10-43)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/estimate.go (1)
RunEstimate(23-41)src/pkg/cli/connect.go (1)
Connect(16-33)
⏰ 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 (8)
src/pkg/agent/tools/deploy_test.go (3)
58-61: InteractiveLoginMCP FabricClient arg is ignored, which is fine for this test doubleThe updated signature correctly accepts a
client.FabricClientand preserves the existing behavior of just logging and returning the configured error. That’s sufficient for these tests, since they only care about the error path and call ordering, not the client itself.
63-69: ComposeUp mock correctly tracks FabricClient-based signatureThe ComposeUp mock now accepts a
client.FabricClientpluscli.ComposeUpParamsand still controls behavior solely viaComposeUpError,ComposeUpResponse, andProject. That matches the new interface while keeping the tests focused on deploy flow rather than FabricClient internals.
84-87: CanIUseProvider mock matches new signature and keeps tests focused on call orderAdapting
CanIUseProviderto take aclient.FabricClient,projectName, andserviceCountwhile just logging and returningnilis appropriate here, since theTestHandleDeployToolassertions only verify that the method was invoked (viaCallLog) in the expected sequence.src/pkg/agent/tools/estimate_test.go (1)
31-37: Estimate tool tests correctly adapted to FabricClient and provider validation changesThe
MockEstimateCLIupdates (FabricClient return fromConnect, FabricClient params forRunEstimate/CreatePlaygroundProvider) are consistent with the new interfaces, and the expectation"provider not one of [defang aws digitalocean gcp]"matches the removal of theautooption.The tests still exercise the same behavior and call order and look good as-is.
Also applies to: 47-60, 117-125
src/pkg/agent/tools/removeConfig_test.go (1)
31-37: Remove‑config tests align with FabricClient + LoadProjectName flowThe refactor to:
- return
client.FabricClientfromConnect,- introduce
LoadProjectName(ctx, loader)and log it in the successful path, and- stub
mockFC.ListDeploymentsfor stack/provider resolution,matches the new CLIInterface and provider-preparer behavior. The updated error text casing (“could not connect…”) and the expected call sequence (
Connect→LoadProjectName→NewProvider→ConfigDelete) look correct.No issues from a correctness standpoint.
Also applies to: 44-50, 60-62, 156-167, 187-195
src/pkg/agent/tools/listConfig_test.go (1)
29-35: List‑config tests correctly reflect the new provider/setup sequenceThe test updates:
- returning
client.FabricClientfromConnect,- using
LoadProjectName(ctx, loader)instead of the previous fallback variant, and- adding a
ListDeploymentsmock onmockFCfor stack resolution,are consistent with the revised interfaces and flow. The expected call order including
"LoadProjectName"in the successful case is a good regression guard.Everything here looks sound.
Also applies to: 42-48, 58-60, 137-149, 169-175
src/pkg/agent/tools/setConfig_test.go (1)
35-41: Set‑config tests are in sync with FabricClient and LoadProjectName semanticsThe migration to
client.FabricClientinConnect, plus the newLoadProjectName(ctx, loader)method and its associated flags (LoadProjectNameCalled,expectedProviderCallstoggling to false on load failure), all line up with the updated tool flow.The added
mockFC.ListDeploymentsstub ensures provider/stack inference can run without touching real services, and the adjusted"could not connect: …"expectation matches the new error casing. The boolean expectations on which CLI methods should be called in each scenario look consistent with the intended control flow.I don’t see any behavioral regressions here.
Also applies to: 62-71, 82-84, 156-163, 166-176, 247-259, 275-279
src/pkg/agent/tools/services_test.go (1)
26-31: FabricClient-based mocks and services tool test wiring look correctMockCLI’s updated methods (Connect, LoadProjectName, ComposeUp/Down, CreatePlaygroundProvider, InteractiveLoginMCP, RunEstimate) now match the FabricClient-based
CLIInterface, andTestHandleServicesToolWithMockCLIcorrectly injectsmockFCand stubsListDeploymentsto exercise the new stack-selection path. The expectations aroundGetServicesCalledandGetServicesProjectalign with the intended behavior.Also applies to: 39-44, 50-58, 70-76, 86-88, 94-96, 110-112, 154-283
jordanstephens
changed the title
|
Closing this in favour of #1721 |
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.
agentTools.NewProviderPreparerand elicitations controller with survey to implement stack selection during 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" stack--project-nameis not being used in a sub command invocation and whenProjectNameis not being used by an agent tool call. In these cases, it was possible to read and write stack files for project "foo" while in the directory for project "bar", leading to confusion. This possibility has been removed in this PR.Linked Issues
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.