feat: add deployment mode selection to parameter collection#2135
Open
lionello wants to merge 6 commits into
Open
feat: add deployment mode selection to parameter collection#2135lionello wants to merge 6 commits into
lionello wants to merge 6 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
| ) | ||
|
|
||
| func makeRecipeCmd() *cobra.Command { | ||
| var stackCmd = &cobra.Command{ |
Contributor
There was a problem hiding this comment.
Fix the variable name.
The variable is named stackCmd but should be recipeCmd to match its actual purpose.
📝 Proposed fix
- var stackCmd = &cobra.Command{
+ var recipeCmd = &cobra.Command{
Use: "recipe",
Aliases: []string{"recipes", "modes", "mode"},
Short: "Manage workspace recipes (deployment modes)",
}
recipeListCmd := makeRecipeListCmd()
- stackCmd.AddCommand(recipeListCmd)
+ recipeCmd.AddCommand(recipeListCmd)
recipeShowCmd := makeRecipeShowCmd()
- stackCmd.AddCommand(recipeShowCmd)
+ recipeCmd.AddCommand(recipeShowCmd)
recipeDeactivateCmd := makeRecipeDeactivateCmd()
- stackCmd.AddCommand(recipeDeactivateCmd)
+ recipeCmd.AddCommand(recipeDeactivateCmd)
recipeActivateCmd := makeRecipeActivateCmd()
- stackCmd.AddCommand(recipeActivateCmd)
- return stackCmd
+ recipeCmd.AddCommand(recipeActivateCmd)
+ return recipeCmd📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var stackCmd = &cobra.Command{ | |
| var recipeCmd = &cobra.Command{ | |
| Use: "recipe", | |
| Aliases: []string{"recipes", "modes", "mode"}, | |
| Short: "Manage workspace recipes (deployment modes)", | |
| } | |
| recipeListCmd := makeRecipeListCmd() | |
| recipeCmd.AddCommand(recipeListCmd) | |
| recipeShowCmd := makeRecipeShowCmd() | |
| recipeCmd.AddCommand(recipeShowCmd) | |
| recipeDeactivateCmd := makeRecipeDeactivateCmd() | |
| recipeCmd.AddCommand(recipeDeactivateCmd) | |
| recipeActivateCmd := makeRecipeActivateCmd() | |
| recipeCmd.AddCommand(recipeActivateCmd) | |
| return recipeCmd |
🤖 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` at 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.
Comment on lines
+143
to
+155
| rresp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipe.String()}) | ||
| if err != nil { | ||
| return nil, project, fmt.Errorf("failed to get recipe for deployment mode %q: %w", recipe, err) | ||
| } | ||
|
|
||
| deployRequest := &client.DeployRequest{ | ||
| DeployRequest: defangv1.DeployRequest{ | ||
| Mode: mode.Value(), | ||
| Mode: recipe.Mode().Value(), | ||
| Project: project.Name, | ||
| Compose: composeYaml, | ||
| DelegateDomain: delegateDomain.Zone, | ||
| }, | ||
| Recipe: rresp.Recipe, |
Contributor
There was a problem hiding this comment.
Verify Mode and Recipe consistency after fetching from backend.
Lines 150 and 155 populate Mode from the local recipe.Mode().Value() and Recipe from the backend rresp.Recipe. If the backend recipe's mode differs from the local recipe's mode (due to misconfiguration, drift, or backend logic), the deploy request will carry inconsistent mode signals to the CD task.
Example scenario:
// Local recipe after compatibility check
recipe.Mode() == modes.ModeBalanced
// Backend returns a different mode
rresp.Recipe.Mode == defangv1.DeploymentMode_DEPLOYMENT_MODE_HIGH_AVAILABILITY
// Deploy request has conflicting fields:
deployRequest.Mode = BALANCED // from local
deployRequest.Recipe.Mode = HIGH_AVAILABILITY // from backendRecommendation: After fetching the recipe, validate consistency or use the fetched recipe as the source of truth for both fields:
rresp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipe.String()})
if err != nil {
return nil, project, fmt.Errorf("failed to get recipe for deployment mode %q: %w", recipe, err)
}
// Validate mode consistency
if rresp.Recipe != nil && rresp.Recipe.Mode != recipe.Mode().Value() {
return nil, project, fmt.Errorf("backend recipe mode %s does not match expected mode %s",
modes.Mode(rresp.Recipe.Mode), recipe.Mode())
}
deployRequest := &client.DeployRequest{
DeployRequest: defangv1.DeployRequest{
Mode: rresp.Recipe.Mode, // use fetched recipe as source of truth
...
},
Recipe: rresp.Recipe,
}Or validate and continue using local recipe, but confirm they match.
🤖 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 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.
Comment on lines
+13
to
+19
| resp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipeName}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get recipe: %w", err) | ||
| } | ||
|
|
||
| _, err = term.Println(resp.Recipe.PulumiConfig) | ||
| return err |
Contributor
There was a problem hiding this comment.
Possible nil-pointer dereference on resp.Recipe.
GetRecipe may return a successful response with a nil Recipe (e.g. an empty/not-found result). resp.Recipe.PulumiConfig would then panic. Guard against a nil Recipe before dereferencing.
🛡️ Proposed nil guard
resp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipeName})
if err != nil {
return fmt.Errorf("failed to get recipe: %w", err)
}
+ if resp.Recipe == nil {
+ return fmt.Errorf("recipe %q not found", recipeName)
+ }
_, err = term.Println(resp.Recipe.PulumiConfig)
return err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipeName}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get recipe: %w", err) | |
| } | |
| _, err = term.Println(resp.Recipe.PulumiConfig) | |
| return err | |
| resp, err := fabric.GetRecipe(ctx, &defangv1.GetRecipeRequest{Name: recipeName}) | |
| if err != nil { | |
| return fmt.Errorf("failed to get recipe: %w", err) | |
| } | |
| if resp.Recipe == nil { | |
| return fmt.Errorf("recipe %q not found", recipeName) | |
| } | |
| _, err = term.Println(resp.Recipe.PulumiConfig) | |
| return err |
🤖 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/recipeShow.go` around lines 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.
Comment on lines
+22
to
+39
| func (r *Recipe) Set(s string) error { | ||
| *r = ParseRecipe(s) | ||
| return nil | ||
| } | ||
|
|
||
| func ParseRecipe(str string) Recipe { | ||
| upper := strings.ToUpper(str) | ||
| // Handle legacy aliases | ||
| switch upper { | ||
| case "CHEAP", "DEVELOPMENT": | ||
| return RecipeAffordable | ||
| case "STAGING": | ||
| return RecipeBalanced | ||
| case "HA", "HIGH-AVAILABILITY", "PRODUCTION": | ||
| return RecipeHighAvailability | ||
| } | ||
| return Recipe(upper) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find flag bindings using Recipe as a pflag.Value and any callers of ParseRecipe/Set
rg -nP --type=go -C3 '\bVar\s*\(\s*&[A-Za-z0-9_.]*[Rr]ecipe\b'
rg -nP --type=go -C2 '\bParseRecipe\s*\('
ast-grep --pattern 'func ($R *Recipe) Set($_ string) error { $$$ }'Repository: DefangLabs/defang
Length of output: 2688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect recipe.go around Set/ParseRecipe/Type/Mode
sed -n '1,120p' src/pkg/modes/recipe.go
echo "----"
# Find any pflag/cobra usage that mentions "recipe" type or calls Set/ParseRecipe for CLI flags
rg -n --type=go --glob '!**/*_test.go' '\b("recipe"|modes\.Recipe|ParseRecipe|DeploymentMode|DEFANG_MODE)\b' src cmd || true
# Find flag definitions that set "mode" / deployment mode
rg -n --type=go --glob '!**/*_test.go' '(pflag\.|cobra\.|Flags\(\)|PersistentFlags\(\)|VarP?\(|StringVarP?\(|BoolVarP?\()' src cmd || true
# Specifically look for Var(&..., ...) patterns that pass a Recipe or call Recipe.Set
rg -n --type=go --glob '!**/*_test.go' 'VarP?\s*\(\s*&.*[Rr]ecipe|Var\s*\(\s*&.*[Rr]ecipe|\.Set\(\s*[^,)]+' src cmd || trueRepository: DefangLabs/defang
Length of output: 47302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/pkg/cli/compose/validation.go
echo "----"
sed -n '1,120p' src/pkg/modes/mode.go
echo "----"
sed -n '180,260p' src/cmd/cli/command/compose.go
echo "----"
sed -n '1,140p' src/cmd/cli/command/estimate.go
echo "----"
sed -n '1,120p' src/cmd/cli/command/stack.goRepository: DefangLabs/defang
Length of output: 17474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Global.Stack.Mode type and env parsing
sed -n '1,240p' src/cmd/cli/command/globals.go
echo "----"
rg -n --type=go 'type .*Stack|Stack struct|var global' src/cmd/cli/command || true
echo "----"
# How flags are wired into global.Stack.Mode (mode flags)
rg -n --type=go 'Flags\(\)\.VarP\(&global\.Stack\.Mode,\s*\"mode\"' -S src/cmd/cli/command || true
echo "----"
# Where Recipe is converted to protobuf / used in requests
sed -n '1,220p' src/pkg/agent/tools/default_tool_cli.go
echo "----"
sed -n '1,120p' src/pkg/agent/tools/estimate.go
echo "----"
sed -n '1,120p' src/pkg/cli/estimate.go
echo "----"
sed -n '1,120p' src/pkg/agent/tools/create_aws_stack.go
echo "----"
sed -n '1,120p' src/pkg/agent/tools/create_gcp_stack.go
echo "----"
sed -n '1,120p' src/pkg/agent/tools/create_azure_stack.go
echo "----"
# Any explicit use of Recipe.Mode() (mapping to ModeUnspecified)
rg -n --type=go '\.Mode\(\)' src/pkg src/cmd | head -n 200
echo "----"
# Any handling of RecipeUnspecified after parsing
rg -n --type=go 'RecipeUnspecified' src | head -n 200Repository: DefangLabs/defang
Length of output: 23674
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/pkg/stacks/manager.go
echo "----"
sed -n '1,240p' src/pkg/stacks/wizard.go
echo "----"
sed -n '1,120p' src/pkg/stacks/stacks.go
echo "----"
sed -n '1,120p' src/pkg/cli/stacks.go
echo "----"
sed -n '500,620p' src/cmd/cli/command/compose.go
echo "----"
sed -n '80,210p' src/cmd/cli/command/globals.goRepository: DefangLabs/defang
Length of output: 25940
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where deployment mode is validated/checked in compose up flow
rg -n --type=go 'checkDeploymentMode|ModeUnspecified|RecipeUnspecified|DeploymentMode' src/pkg/cli/composeUp.go src/pkg/cli -S
# Show the relevant sections
sed -n '1,260p' src/pkg/cli/composeUp.go
echo "----"
sed -n '260,420p' src/pkg/cli/composeUp.go
echo "----"
# How stacks parameters are validated/used when uploading/creating
sed -n '1,140p' src/pkg/stacks/manager.go
echo "----"
sed -n '140,260p' src/pkg/stacks/manager.go
echo "----"
# How recipe/mode is used when composing stacks deployment requests
sed -n '1,120p' src/pkg/cli/stacks.go
echo "----"
rg -n --type=go 'RecipeUnspecified|ModeUnspecified|recipe\.Mode\(\)' src/pkg/cli/stacks.go src/pkg/cli/composeUp.go src/pkg/cli/preview.go -S || trueRepository: DefangLabs/defang
Length of output: 23423
Fix validation for unknown Recipe values in Recipe.Set/ParseRecipe.
Recipe.Set always returns nil, and ParseRecipe returns Recipe(upper) for any unrecognized string; when Recipe is used as a pflag.Value for the CLI --mode flags (e.g., compose/estimate/stack commands) and for DEFANG_MODE env parsing, typos won’t fail fast. Unknown values map to ModeUnspecified via Recipe.Mode(), which can silently alter behavior downstream (e.g., compose can fall back to the previous deployment mode; estimate/preview sends MODE_UNSPECIFIED). Add validation in Set (or keep a dedicated “allow free-form” parse path for agent use if that’s truly intended).
🤖 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/modes/recipe.go` around lines 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.
Comment on lines
+72
to
+80
| var recipe modes.Recipe | ||
| if val, ok := variables["DEFANG_MODE"]; ok { | ||
| err := mode.Set(val) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid DEFANG_MODE value: %w", err) | ||
| } | ||
| recipe = modes.ParseRecipe(val) | ||
| } | ||
| return &Parameters{ | ||
| Variables: variables, | ||
| Provider: provider, | ||
| Region: region, | ||
| Mode: mode, | ||
| Mode: recipe, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify ParseRecipe handles invalid/case-mismatched input gracefully
# Check ParseRecipe implementation for case-insensitivity and invalid input handling
ast-grep --pattern $'func ParseRecipe($$$) $_ {
$$$
}'
# Check for any error handling or validation in ParseRecipe
rg -nP -A10 'func ParseRecipe' --type=goRepository: DefangLabs/defang
Length of output: 1313
Adjust/cover invalid DEFANG_MODE parsing behavior in modes.ParseRecipe
modes.ParseRecipe uppercases the input, maps legacy aliases, and otherwise returns Recipe(upper) with no validation/error path (so an unknown DEFANG_MODE value will propagate as a non-RecipeUnspecified mode rather than being coerced to the sentinel). Add an explicit policy (e.g., map unknowns to RecipeUnspecified) and a test for an invalid value, while keeping the case-insensitive behavior.
[src/pkg/modes/recipe.go:27-38], [src/pkg/stacks/stacks.go:72-80]
🤖 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/stacks/stacks.go` around lines 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.
jordanstephens
approved these changes
Jun 3, 2026
Comment on lines
-21
to
+25
| mode, err := modes.Parse(params.Mode) | ||
| if err != nil { | ||
| return "Invalid mode provided", err | ||
| } | ||
|
|
||
| newStack := stacks.Parameters{ | ||
| Name: params.Name, | ||
| Region: params.Region, | ||
| Provider: client.ProviderAWS, | ||
| Mode: mode, | ||
| Mode: modes.ParseRecipe(params.Mode), |
Member
There was a problem hiding this comment.
If a recipe doesnt exist server side, how does this fail now?
|
|
||
| term.Debugf("Fixedup project: %s", string(composeData)) | ||
|
|
||
| // TODO: this will need to read the recipe from Fabric first |
Comment on lines
+18
to
+32
| err = fabric.PutRecipe(ctx, &defangv1.PutRecipeRequest{ | ||
| Recipe: &defangv1.Recipe{ | ||
| Name: resp.Recipe.Name, | ||
| PulumiConfig: resp.Recipe.PulumiConfig, | ||
| Active: active, | ||
| }, | ||
| }) | ||
| if err == nil { | ||
| state := "active" | ||
| if !active { | ||
| state = "inactive" | ||
| } | ||
| term.Info(fmt.Sprintf("Recipe %q is now %s.", name, state)) | ||
| } | ||
| return err |
Member
There was a problem hiding this comment.
This raised my eyebrows at first glance. A few strange things about this code:
- the error handling is inverted from the idiomatic pattern
- state is inferred from the err, rather than from the record we update.
Can we avoid introducing a new state variable by instead modifying the recipe record in memory before sending it back and printing the state directly from the recipe?
Suggested change
| err = fabric.PutRecipe(ctx, &defangv1.PutRecipeRequest{ | |
| Recipe: &defangv1.Recipe{ | |
| Name: resp.Recipe.Name, | |
| PulumiConfig: resp.Recipe.PulumiConfig, | |
| Active: active, | |
| }, | |
| }) | |
| if err == nil { | |
| state := "active" | |
| if !active { | |
| state = "inactive" | |
| } | |
| term.Info(fmt.Sprintf("Recipe %q is now %s.", name, state)) | |
| } | |
| return err | |
| recipe := resp.Recipe | |
| recipe.Active = active | |
| err = fabric.PutRecipe(ctx, &defangv1.PutRecipeRequest{ | |
| Recipe: recipe, | |
| }) | |
| if err != nil { | |
| return err | |
| } | |
| term.Info(fmt.Sprintf("Recipe %q is now %s.", name, recipe.Active)) | |
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
Part of https://github.com/DefangLabs/defang-global/issues/75
Checklist
Summary by CodeRabbit
New Features
Tests