feat: add deployment mode selection to parameter collection#2135
Open
lionello wants to merge 7 commits into
Open
feat: add deployment mode selection to parameter collection#2135lionello wants to merge 7 commits into
lionello wants to merge 7 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Recipe type and migrates mode handling from Mode to Recipe across stacks, compose/deploy, estimate/validation, client APIs, CLI commands, and tests; adds recipe CRUD client methods and CLI commands; includes Recipe in BYOC deploy payloads and persisted deployment records. ChangesRecipe type and workflow migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/cmd/cli/command/estimate.go (1)
88-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the duplicate Azure case.
case pkg.AzureInEnv() != ""already exists at Lines 92-93, so the second occurrence at Lines 98-99 is unreachable dead code. Either drop it or correct it if a different provider was intended.🧹 Proposed cleanup
case pkg.GcpInEnv() != "": defaultOption = client.ProviderGCP.String() - case pkg.AzureInEnv() != "": - defaultOption = client.ProviderAzure.String() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/cli/command/estimate.go` around lines 88 - 100, The switch in estimate.go contains a duplicate Azure branch, making the later case unreachable. Remove the repeated pkg.AzureInEnv() check from the default option selection, or replace it with the intended provider case if another environment was meant; verify the logic in the provider selection block around defaultOption so each provider is handled only once.src/pkg/agent/tools/create_aws_stack.go (1)
20-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
params.Modeinstead of silently accepting unknown values
src/pkg/agent/tools/create_aws_stack.gousesmodes.ParseRecipe(params.Mode)inline (no error handling). Insrc/pkg/modes/recipe.go,ParseRecipe(str string) Recipehas no error return; it uppercases the input, maps only a few legacy aliases, and otherwise falls through toreturn Recipe(upper)for unrecognized strings. This removes the prior “invalid mode” error behavior frommodes.Parse. Restore explicit validation/error handling (or ensure unknown recipes are rejected downstream) before persisting/using the stack.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/agent/tools/create_aws_stack.go` around lines 20 - 31, HandleCreateAWSStackTool currently calls modes.ParseRecipe(params.Mode) with no validation; restore explicit validation by parsing the mode then checking it against the known/allowed Recipe values (e.g., via a new or existing validator like modes.IsValid/knownRecipes or by updating modes.ParseRecipe to return an error). If the parsed Recipe is not one of the recognized recipes, return a descriptive error from HandleCreateAWSStackTool instead of persisting the stack; otherwise proceed to build newStack (referencing newStack and modes.ParseRecipe) and call createAndLoadStack as before.src/pkg/cli/common.go (1)
56-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop
putDeploymentParams.RecipeinputDeploymentAndStack(common.go)
src/pkg/cli/common.godefinesputDeploymentParams.Recipe, butputDeploymentAndStacknever reads it and never setsStack.Recipe/Deployment.Recipein thePutStackRequestandPutDeploymentRequest.src/pkg/cli/composeUp.godoes populateRecipe: deployRequest.Recipe, and the protobufs (io.defang.v1.Stack/io.defang.v1.Deployment) include arecipefield (withmodemarked deprecated), so wiringreq.Recipethrough would prevent losing recipe metadata in the stored deployment/stack.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/common.go` around lines 56 - 124, putDeploymentAndStack currently ignores putDeploymentParams.Recipe so recipe metadata is never saved; update the function to pass req.Recipe into both the Stack and Deployment in the PutStackRequest and PutDeploymentRequest (set Stack.Recipe = req.Recipe and Deployment.Recipe = req.Recipe), ensuring the same recipe populated by composeUp.go (deployRequest.Recipe) is persisted; locate these fields in putDeploymentAndStack and add the Recipe assignments to the created defangv1.Stack and defangv1.Deployment objects.
🧹 Nitpick comments (3)
src/cmd/cli/command/recipe.go (2)
76-93: 💤 Low valueRename variable for consistency.
The variable is named
recipeUnarchiveCmdbut represents the activate command. Consider renaming torecipeActivateCmdfor consistency with the function name and command semantics.♻️ Proposed change
- var recipeUnarchiveCmd = &cobra.Command{ + var recipeActivateCmd = &cobra.Command{ Use: "activate [RECIPE_NAME...]", Aliases: []string{"restore", "enable", "undelete", "unarchive"}, Annotations: authNeededAlways, Args: cobra.MinimumNArgs(1), Short: "Activates a recipe in the current workspace", RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() var errs []error for _, name := range args { errs = append(errs, cli.RecipeActivate(ctx, global.Client, name, true)) } return errors.Join(errs...) }, } - return recipeUnarchiveCmd + return recipeActivateCmd🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/cli/command/recipe.go` around lines 76 - 93, The local variable recipeUnarchiveCmd in makeRecipeActivateCmd should be renamed to recipeActivateCmd to match the function/command semantics; update its declaration and the final return statement to use recipeActivateCmd, and ensure any internal references (the cobra.Command literal and its RunE closure) remain unchanged so only the variable identifier is renamed.
58-73: 💤 Low valueRename variable for consistency.
The variable is named
recipeArchiveCmdbut represents the deactivate command. Consider renaming torecipeDeactivateCmdfor consistency with the function name and command semantics.♻️ Proposed change
- var recipeArchiveCmd = &cobra.Command{ + var recipeDeactivateCmd = &cobra.Command{ Use: "deactivate [RECIPE_NAME...]", Aliases: []string{"remove", "rm", "delete", "del", "disable", "archive"}, Annotations: authNeededAlways, Args: cobra.MinimumNArgs(1), Short: "Deactivates a recipe in the current workspace", RunE: func(cmd *cobra.Command, args []string) error { ctx := cmd.Context() var errs []error for _, name := range args { errs = append(errs, cli.RecipeActivate(ctx, global.Client, name, false)) } return errors.Join(errs...) }, } - return recipeArchiveCmd + return recipeDeactivateCmd🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cmd/cli/command/recipe.go` around lines 58 - 73, The variable recipeArchiveCmd is misnamed for the deactivate command; rename it to recipeDeactivateCmd and update its declaration, any references (including the return statement) to use recipeDeactivateCmd so the symbol matches the command semantics and the function name; ensure the variable name change is applied consistently within this function block (the cobra.Command variable and the final return).src/pkg/cli/composeUp.go (1)
138-140: 💤 Low valueClarify the recipe override and simplify mode conversion.
After the compatibility check, if
newModeis notModeUnspecified, you override the user-providedrecipewith a new one derived from the mode viamodes.FromMode(newMode.Value()). This pattern has two concerns:
Redundant conversion:
newModeis already amodes.Mode, so calling.Value()to getdefangv1.DeploymentModeand then passing it back toFromModeis a round-trip. Consider storing a direct Mode-to-Recipe mapping or checking ifmodes.FromMode(newMode)is valid (without the.Value()step).Potential information loss: If the original
recipecarries any custom configuration beyond just the mode name, this override discards it. The subsequentGetRecipecall fetches backend config by name, but clarify in a comment whether user-provided recipe details (if any exist) are intentionally replaced by the compatibility-derived mode.♻️ Optional: simplify or document the override
Document the intent:
if newMode != modes.ModeUnspecified { + // Replace recipe with mode-based one to enforce compatibility; + // backend GetRecipe will supply full config. recipe = modes.FromMode(newMode.Value()) }Or refactor if a direct conversion exists:
-recipe = modes.FromMode(newMode.Value()) +recipe = modes.RecipeFromMode(newMode) // if such a helper exists🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/composeUp.go` around lines 138 - 140, The current override replaces the entire user-provided recipe by calling recipe = modes.FromMode(newMode.Value()), which both does a redundant .Value() round-trip and discards any custom recipe fields; change this to directly convert the modes.Mode without .Value() (e.g., call modes.FromMode(newMode) or use a small Mode->Recipe mapping) and when applying the result only override the recipe's mode/name (not the whole recipe) so user-specified configuration is preserved; also add a short comment near newMode, recipe, modes.FromMode and GetRecipe clarifying that only the mode/name is being canonicalized for compatibility, not wholesale replacement of user recipe data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cmd/cli/command/recipe.go`:
- Line 11: Rename the incorrectly named variable stackCmd to recipeCmd so the
command variable reflects its purpose; find the declaration "var stackCmd =
&cobra.Command{", rename the identifier to "recipeCmd" everywhere it is
referenced (including any uses in init/register functions) and ensure any
related help/usage text remains unchanged.
In `@src/pkg/cli/composeUp.go`:
- Around line 143-155: The deploy request mixes the local recipe.Mode() and the
backend-fetched rresp.Recipe, which can produce conflicting Mode values; after
calling fabric.GetRecipe (GetRecipe) check that rresp.Recipe is non-nil and that
rresp.Recipe.Mode equals recipe.Mode().Value(), and if they differ return an
error indicating the mismatch, or else set deployRequest.DeployRequest.Mode from
the fetched rresp.Recipe.Mode so both deployRequest.Mode and Recipe
(rresp.Recipe) use the same source of truth; update the code paths that build
the deployRequest (the DeployRequest struct and any early return) accordingly.
In `@src/pkg/cli/recipeShow.go`:
- Around line 13-19: resp.Recipe may be nil after fabric.GetRecipe; before
accessing resp.Recipe.PulumiConfig in recipeShow.go, add a nil-guard: check that
resp != nil && resp.Recipe != nil and handle the nil case (return a clear error
from the surrounding function or print a “recipe not found” message) instead of
dereferencing; update the code around the GetRecipe call and the subsequent
term.Println usage to avoid a panic and return the appropriate error when Recipe
is nil.
In `@src/pkg/modes/recipe.go`:
- Around line 22-39: The Set method currently accepts any string and ParseRecipe
returns Recipe(upper) for unknown inputs, letting typos become ModeUnspecified
downstream; update ParseRecipe to validate against known constants
(RecipeAffordable, RecipeBalanced, RecipeHighAvailability, etc.) and have
Recipe.Set call ParseRecipe and return a descriptive error when the input is
unrecognized (instead of nil), or alternatively provide a separate
ParseRecipeAllowFreeForm used only by the agent; ensure Recipe.Mode() still maps
properly but that CLI pflag.Value parsing and DEFANG_MODE reading fail fast on
invalid values.
In `@src/pkg/stacks/stacks.go`:
- Around line 72-80: modes.ParseRecipe currently uppercases and maps aliases but
returns Recipe(upper) for unknown inputs, allowing invalid modes to slip
through; update ParseRecipe to validate the uppercased value against known
recipes and return modes.RecipeUnspecified for any unrecognized string (while
preserving case-insensitive matching and legacy alias mapping), add a unit test
that calls modes.ParseRecipe with an invalid value and asserts
RecipeUnspecified, and ensure callers (e.g., the Parameters construction in
stacks.go where recipe is set) continue to treat RecipeUnspecified as the
sentinel for unspecified/invalid mode.
---
Outside diff comments:
In `@src/cmd/cli/command/estimate.go`:
- Around line 88-100: The switch in estimate.go contains a duplicate Azure
branch, making the later case unreachable. Remove the repeated pkg.AzureInEnv()
check from the default option selection, or replace it with the intended
provider case if another environment was meant; verify the logic in the provider
selection block around defaultOption so each provider is handled only once.
In `@src/pkg/agent/tools/create_aws_stack.go`:
- Around line 20-31: HandleCreateAWSStackTool currently calls
modes.ParseRecipe(params.Mode) with no validation; restore explicit validation
by parsing the mode then checking it against the known/allowed Recipe values
(e.g., via a new or existing validator like modes.IsValid/knownRecipes or by
updating modes.ParseRecipe to return an error). If the parsed Recipe is not one
of the recognized recipes, return a descriptive error from
HandleCreateAWSStackTool instead of persisting the stack; otherwise proceed to
build newStack (referencing newStack and modes.ParseRecipe) and call
createAndLoadStack as before.
In `@src/pkg/cli/common.go`:
- Around line 56-124: putDeploymentAndStack currently ignores
putDeploymentParams.Recipe so recipe metadata is never saved; update the
function to pass req.Recipe into both the Stack and Deployment in the
PutStackRequest and PutDeploymentRequest (set Stack.Recipe = req.Recipe and
Deployment.Recipe = req.Recipe), ensuring the same recipe populated by
composeUp.go (deployRequest.Recipe) is persisted; locate these fields in
putDeploymentAndStack and add the Recipe assignments to the created
defangv1.Stack and defangv1.Deployment objects.
---
Nitpick comments:
In `@src/cmd/cli/command/recipe.go`:
- Around line 76-93: The local variable recipeUnarchiveCmd in
makeRecipeActivateCmd should be renamed to recipeActivateCmd to match the
function/command semantics; update its declaration and the final return
statement to use recipeActivateCmd, and ensure any internal references (the
cobra.Command literal and its RunE closure) remain unchanged so only the
variable identifier is renamed.
- Around line 58-73: The variable recipeArchiveCmd is misnamed for the
deactivate command; rename it to recipeDeactivateCmd and update its declaration,
any references (including the return statement) to use recipeDeactivateCmd so
the symbol matches the command semantics and the function name; ensure the
variable name change is applied consistently within this function block (the
cobra.Command variable and the final return).
In `@src/pkg/cli/composeUp.go`:
- Around line 138-140: The current override replaces the entire user-provided
recipe by calling recipe = modes.FromMode(newMode.Value()), which both does a
redundant .Value() round-trip and discards any custom recipe fields; change this
to directly convert the modes.Mode without .Value() (e.g., call
modes.FromMode(newMode) or use a small Mode->Recipe mapping) and when applying
the result only override the recipe's mode/name (not the whole recipe) so
user-specified configuration is preserved; also add a short comment near
newMode, recipe, modes.FromMode and GetRecipe clarifying that only the mode/name
is being canonicalized for compatibility, not wholesale replacement of user
recipe data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b363f9dc-d3d7-47e2-b66a-eeefa9d62564
📒 Files selected for processing (54)
src/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/cmd/cli/command/estimate.gosrc/cmd/cli/command/estimate_test.gosrc/cmd/cli/command/globals.gosrc/cmd/cli/command/globals_test.gosrc/cmd/cli/command/recipe.gosrc/cmd/cli/command/stack.gosrc/cmd/cli/command/stack_test.gosrc/pkg/agent/tools/create_aws_stack.gosrc/pkg/agent/tools/create_azure_stack.gosrc/pkg/agent/tools/create_gcp_stack.gosrc/pkg/agent/tools/current_stack_test.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/estimate.gosrc/pkg/agent/tools/estimate_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/agent/tools/select_stack_test.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/azure/byoc.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/client/mock.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/common.gosrc/pkg/cli/compose/validation.gosrc/pkg/cli/compose/validation_test.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/composeUp_dockerfile_test.gosrc/pkg/cli/composeUp_test.gosrc/pkg/cli/estimate.gosrc/pkg/cli/preview_test.gosrc/pkg/cli/recipeActivate.gosrc/pkg/cli/recipeList.gosrc/pkg/cli/recipeShow.gosrc/pkg/cli/stacks.gosrc/pkg/cli/stacks_test.gosrc/pkg/cli/tail_test.gosrc/pkg/modes/mode.gosrc/pkg/modes/mode_test.gosrc/pkg/modes/recipe.gosrc/pkg/session/session.gosrc/pkg/session/session_test.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector_test.gosrc/pkg/stacks/stacks.gosrc/pkg/stacks/stacks_test.gosrc/pkg/stacks/wizard.gosrc/pkg/stacks/wizard_test.go
💤 Files with no reviewable changes (1)
- src/cmd/cli/command/stack.go
jordanstephens
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
defang recipe …commands, withlist,show,activate,deactivateGetRecipegRPC and attachPulumiConfig(if any) to theProjectUpdateLinked Issues
This needs https://github.com/DefangLabs/defang-mvp/pull/3027 and DefangLabs/pulumi-defang#279
Part of https://github.com/DefangLabs/defang-global/issues/75
Checklist
Summary by CodeRabbit
New Features
Tests