Hide Down stacks in interactive selector and in stacks ls#2046
Hide Down stacks in interactive selector and in stacks ls#2046jordanstephens wants to merge 7 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a StackStatus enum and Stack.status field in the protobuf; surfaced status into ListItem, populated it from remote stacks, filtered out DOWN stacks in the selector and CLI, and updated tests and mocks to cover status behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Selector as "Stack Selector"
participant Manager as "StacksManager"
participant Remote as "Defang API"
participant UI as "ElicitationsController"
User->>Selector: SelectStack()
Selector->>Manager: List()
Manager->>Remote: ListStacks()
Remote-->>Manager: stacks (include Status)
Manager-->>Selector: ListItem[] (Status populated)
Selector->>Selector: filter out Status == STACK_STATUS_DOWN
Selector->>UI: RequestEnum(options = filtered labels)
UI-->>Selector: user selection
Selector->>User: return selected stack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (1)
src/protos/io/defang/v1/fabric.proto (1)
166-170: Consider prefixingStackStatusenum values for consistency with protocol buffers naming conventions.At lines 168-169,
UPandDOWNare generic names. Protocol buffers convention recommends prefixing enum values with their enum type name (e.g.,STACK_STATUS_UP,STACK_STATUS_DOWN) for improved clarity across language bindings and consistency with patterns elsewhere in proto schemas.Suggested refactoring
enum StackStatus { STACK_STATUS_UNSPECIFIED = 0; - UP = 1; - DOWN = 2; + STACK_STATUS_UP = 1; + STACK_STATUS_DOWN = 2; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/protos/io/defang/v1/fabric.proto` around lines 166 - 170, Rename the enum values in StackStatus so they are prefixed with the enum name: change UP to STACK_STATUS_UP and DOWN to STACK_STATUS_DOWN (leave STACK_STATUS_UNSPECIFIED as-is); update all usages/references of StackStatus.UP and StackStatus.DOWN throughout the codebase (and regenerate any generated protobuf artifacts) to use StackStatus.STACK_STATUS_UP and StackStatus.STACK_STATUS_DOWN to maintain consistency with protobuf naming conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/protos/io/defang/v1/fabric.proto`:
- Line 182: Add the google.api.field_behavior import and mark the Stack.status
field as OUTPUT_ONLY so clients cannot set it; update the proto by importing
"google/api/field_behavior.proto" and annotate the Stack.message's status field
(Stack.status) with the google.api.field_behavior = OUTPUT_ONLY option to
enforce server-managed semantics (also verify usages like PutStackRequest that
embed Stack reflect the intent).
---
Nitpick comments:
In `@src/protos/io/defang/v1/fabric.proto`:
- Around line 166-170: Rename the enum values in StackStatus so they are
prefixed with the enum name: change UP to STACK_STATUS_UP and DOWN to
STACK_STATUS_DOWN (leave STACK_STATUS_UNSPECIFIED as-is); update all
usages/references of StackStatus.UP and StackStatus.DOWN throughout the codebase
(and regenerate any generated protobuf artifacts) to use
StackStatus.STACK_STATUS_UP and StackStatus.STACK_STATUS_DOWN to maintain
consistency with protobuf naming conventions.
🪄 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: d9ba03d7-78fa-44d6-87af-cedcce66d5e6
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
src/protos/io/defang/v1/fabric.proto
| string region = 7; | ||
| DeploymentMode mode = 8; | ||
| bool is_default = 9; | ||
| StackStatus status = 10; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PutStack handler definitions =="
rg -nP --type=go -C4 '\bPutStack\s*\([^)]*\*defangv1\.PutStackRequest'
echo
echo "== Reads of client-provided stack status in PutStack flow =="
rg -nP --type=go -C4 'GetStack\(\)\.GetStatus\(\)|\.Stack\.Status|GetStatus\(\)'
echo
echo "== Writes of stack status in server responses/storage path =="
rg -nP --type=go -C4 'StackStatus|STATUS_UP|STATUS_DOWN|status\s*:'Repository: DefangLabs/defang
Length of output: 28491
🏁 Script executed:
cat -n src/protos/io/defang/v1/fabric.proto | sed -n '170,200p'Repository: DefangLabs/defang
Length of output: 817
🏁 Script executed:
rg -n "message Stack|message PutStackRequest" -A 15 src/protos/io/defang/v1/fabric.protoRepository: DefangLabs/defang
Length of output: 713
🏁 Script executed:
# Look for where Stack objects are created/modified in request path
rg -n "stack.*=.*&defangv1\.Stack|\.Status\s*=" --type=go | grep -i "putstack\|request" | head -20Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
# Find server-side implementations of PutStack (usually in gRPC service handlers)
rg -n "func.*PutStack" --type=go -B 2 -A 8 | grep -v test | head -40Repository: DefangLabs/defang
Length of output: 3736
🏁 Script executed:
# Search for fabric service implementations
fd -name "*.go" -path "*/service*" -o -path "*/server*" | xargs rg -l "PutStack" 2>/dev/null | head -5Repository: DefangLabs/defang
Length of output: 435
🏁 Script executed:
# More specific: look for proto service implementation handling
rg -n "PutStack\(" --type=go -B 2 -A 10 | grep -E "(func|Stack|status)" | head -30Repository: DefangLabs/defang
Length of output: 3395
🏁 Script executed:
# Find where PutStack is actually implemented (server handler)
find src/pkg -name "*.go" -type f | xargs rg "func.*PutStack.*ctx" 2>/dev/null | grep -v test | grep -v "interface"Repository: DefangLabs/defang
Length of output: 298
🏁 Script executed:
# Look for fabric service implementation
fd "fabric" --type f --path "*/service*" src/pkgRepository: DefangLabs/defang
Length of output: 294
🏁 Script executed:
# Search more broadly for server implementation
rg "PutStack.*context.Context.*\*defangv1.PutStackRequest" --type=go -A 15 | head -60Repository: DefangLabs/defang
Length of output: 3538
🏁 Script executed:
# Search for FabricController implementation across entire src/pkg
rg "type.*FabricController" --type=go -A 2Repository: DefangLabs/defang
Length of output: 1502
🏁 Script executed:
# Look for any PutStack implementation that's not a client wrapper
rg "func.*FabricController.*PutStack|func.*Server.*PutStack" --type=go -A 12Repository: DefangLabs/defang
Length of output: 1847
🏁 Script executed:
# Broader search for where Stack.Status might be set or used in responses
rg "\.Status\s*=" --type=go | grep -i stack | grep -v test | head -20Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
# Look for GetStack or ListStacks implementations that might populate status
rg "GetStack|ListStack" --type=go -A 10 | grep -E "(Status|status)" -B 2 -A 2 | head -40Repository: DefangLabs/defang
Length of output: 2705
🏁 Script executed:
# Check if there's a separate service directory or different code structure
find src -type d -name "*service*" -o -name "*server*" -o -name "*handler*"Repository: DefangLabs/defang
Length of output: 216
🏁 Script executed:
# Look for any database/storage interactions with Stack
rg "db.*Stack|storage.*Stack|sql.*Stack" --type=go -l | head -10Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
# Final verification: check if there's any mention of output_only or field_behavior in existing protos
rg "OUTPUT_ONLY|field_behavior" src/protos/Repository: DefangLabs/defang
Length of output: 43
🏁 Script executed:
# Check what google.api imports currently exist
rg "import.*google.api" src/protos/io/defang/v1/fabric.protoRepository: DefangLabs/defang
Length of output: 43
Add OUTPUT_ONLY annotation to Stack.status to enforce server-managed semantics.
Line 182 adds status to the Stack message, which is embedded in PutStackRequest (line 186). This allows clients to set a field that should be server-determined. Add the google.api.field_behavior import and mark status as output-only to prevent accidental misuse and clarify the contract.
Suggested proto contract hardening
import "google/protobuf/empty.proto";
import "google/protobuf/timestamp.proto";
import "google/type/money.proto";
+import "google/api/field_behavior.proto";
@@
- StackStatus status = 10;
+ StackStatus status = 10 [(google.api.field_behavior) = OUTPUT_ONLY];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/protos/io/defang/v1/fabric.proto` at line 182, Add the
google.api.field_behavior import and mark the Stack.status field as OUTPUT_ONLY
so clients cannot set it; update the proto by importing
"google/api/field_behavior.proto" and annotate the Stack.message's status field
(Stack.status) with the google.api.field_behavior = OUTPUT_ONLY option to
enforce server-managed semantics (also verify usages like PutStackRequest that
embed Stack reflect the intent).
There was a problem hiding this comment.
Alternatively, we can add a new rpc GetStackStatus instead.
There was a problem hiding this comment.
@lionello, that's a valid alternative worth weighing. Here's a quick comparison of both approaches:
Stack.status (OUTPUT_ONLY) |
New GetStackStatus RPC |
|
|---|---|---|
| Client ergonomics | Status returned inline with GetStack/ListStacks — no extra roundtrip |
Separate call required to check status |
| API clarity | Requires OUTPUT_ONLY annotation to signal "server-managed, ignored on write" |
Unambiguous — no field in PutStackRequest to misinterpret |
| Proto contract | Needs google/api/field_behavior.proto import + annotation |
Slightly more surface area (new request/response message + RPC) |
| Future extensibility | Status detail (e.g. reason, timestamp) would still require a dedicated response type | Easier to add rich status detail in a purpose-built response message |
If the status is expected to be polled independently (e.g. to track provisioning progress), a dedicated GetStackStatus RPC makes the intent very clear and avoids any ambiguity about the PutStackRequest payload. If status is primarily an informational field always read alongside the stack config (like is_default or last_deployed_at), keeping it in Stack with an OUTPUT_ONLY annotation is the lighter, more idiomatic choice.
Either way, the concern about PutStack accepting an ignored status field can be addressed — either by the new RPC design or the annotation. Which pattern better fits how callers will use this in practice?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pkg/stacks/selector.go (1)
58-77: Consider handling the case when all stacks are filtered out.The empty-list check at lines 51-57 runs before filtering, so if stacks exist but all have
STACK_STATUS_DOWN:
- With
AllowStackCreation=false:stackLabelsis empty →RequestEnumfails with a generic elicitation error instead of a clear "no active stacks available" message.- With
AllowStackCreation=true: Works, but the user only sees "Create new stack" with no indication their stacks are DOWN.💡 Suggested improvement
filteredStackList := make([]ListItem, 0, len(stackList)) for _, stack := range stackList { if stack.Status == defangv1.StackStatus_STACK_STATUS_DOWN { continue } filteredStackList = append(filteredStackList, stack) } + +if len(filteredStackList) == 0 { + if opts.AllowStackCreation { + return ss.createStack(ctx) + } + return nil, errors.New("no active stacks available to select (all stacks are down)") +} + labelMap := MakeStackSelectorLabels(filteredStackList)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/stacks/selector.go` around lines 58 - 77, After filtering out DOWN stacks (filteredStackList via MakeStackSelectorLabels), add a post-filter empty check: if filteredStackList is empty and AllowStackCreation is false then return/raise a clear elicitation error like "no active stacks available" before building stackLabels/RequestEnum; if filteredStackList is empty and AllowStackCreation is true then still build stackLabels to include only the "Create new stack" option but also add an informational label or placeholder entry (e.g., "All existing stacks are DOWN") so the user sees why no active stacks are listed; update the logic around stackLabels/stackNames and RequestEnum to handle these two branches accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pkg/stacks/selector.go`:
- Around line 58-77: After filtering out DOWN stacks (filteredStackList via
MakeStackSelectorLabels), add a post-filter empty check: if filteredStackList is
empty and AllowStackCreation is false then return/raise a clear elicitation
error like "no active stacks available" before building stackLabels/RequestEnum;
if filteredStackList is empty and AllowStackCreation is true then still build
stackLabels to include only the "Create new stack" option but also add an
informational label or placeholder entry (e.g., "All existing stacks are DOWN")
so the user sees why no active stacks are listed; update the logic around
stackLabels/stackNames and RequestEnum to handle these two branches accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b6f1e12-9702-4078-8ccc-49d226a3cd12
📒 Files selected for processing (5)
src/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/selector.gosrc/pkg/stacks/selector_test.gosrc/pkg/stacks/stacks.go
jordanstephens
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/stack.go`:
- Around line 143-145: The message shown when len(filteredStacks) == 0 is
misleading if stackList is non-empty; update the logic in the function that
handles stack listing (referencing filteredStacks and stackList in stack.go) to
distinguish the two cases: if stackList is empty, keep the existing "No Defang
stacks found" message, but if stackList is non-empty and filteredStacks is
empty, change the output to something like "No running Defang stacks found; all
stacks are down" (or similar) before returning the error from term.Infof; ensure
you check both len(filteredStacks) and len(stackList) and call term.Infof with
the appropriate message.
🪄 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: ba3d0cef-1f19-4b67-b974-d3f691838131
📒 Files selected for processing (1)
src/cmd/cli/command/stack.go
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/cmd/cli/command/stack_test.go (1)
163-172:⚠️ Potential issue | 🟡 MinorFail fast on fixture-setup errors in subtests.
Ignoring setup errors here can mask broken fixtures and produce hard-to-diagnose failures later in assertions.
💡 Proposed fix
- os.WriteFile( + err := os.WriteFile( "compose.yaml", []byte(`services: web: image: nginx`), os.FileMode(0644), ) + assert.NoError(t, err) for _, stack := range tt.localStacks { - stacks.CreateInDirectory(".", stack) + _, err = stacks.CreateInDirectory(".", stack) + assert.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/stack_test.go` around lines 163 - 172, The test fixture setup currently ignores errors from os.WriteFile and stacks.CreateInDirectory in the subtest setup loop; modify the code to capture and check returned errors and fail the subtest immediately (use t.Fatalf or t.Errorf+return) when any setup call fails. Specifically, check the error returned by os.WriteFile and call t.Fatalf("failed to write compose.yaml: %v", err) on non-nil, and when calling stacks.CreateInDirectory (in the loop for _, stack := range tt.localStacks) ensure it returns an error and fail the test immediately if non-nil (or update CreateInDirectory to return an error and propagate/fail). This ensures the subtest stops on fixture-setup failures rather than continuing with broken fixtures.
🧹 Nitpick comments (1)
src/cmd/cli/command/stack.go (1)
142-148: Consider centralizing DOWN-stack filtering logic.This filtering is now duplicated with
src/pkg/stacks/selector.go(status-based exclusion). A shared helper insrc/pkg/stackswould reduce drift risk and keep behavior consistent across list/selector paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cmd/cli/command/stack.go` around lines 142 - 148, The inline DOWN-stack filtering in the loop (building filteredStacks from stackList and checking StackStatus_STACK_STATUS_DOWN) is duplicated with selector logic; create a shared helper in package stacks (e.g., func ExcludeDownStacks(items []stacks.ListItem) []stacks.ListItem or FilterOutDown(items []stacks.ListItem) []stacks.ListItem) that encapsulates the status check, replace the loop in stack.go to call that helper, and update src/pkg/stacks/selector.go to use the same helper so status-based exclusion is centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/cli/command/stack.go`:
- Around line 158-163: The CLI currently always returns term.Table(stackList,
columns...) so the --json flag is ignored; inside the stackListCmd RunE closure,
read the "json" flag (e.g. cmd.Flags().GetBool("json") or
stackListCmd.Flags().GetBool("json")) and branch: if true, marshal stackList to
JSON and write it to stdout (or use an existing JSON helper) and return,
otherwise call term.Table as before; modify the code around stackList, columns,
and the term.Table call so the JSON path is taken when "json" is true.
---
Outside diff comments:
In `@src/cmd/cli/command/stack_test.go`:
- Around line 163-172: The test fixture setup currently ignores errors from
os.WriteFile and stacks.CreateInDirectory in the subtest setup loop; modify the
code to capture and check returned errors and fail the subtest immediately (use
t.Fatalf or t.Errorf+return) when any setup call fails. Specifically, check the
error returned by os.WriteFile and call t.Fatalf("failed to write compose.yaml:
%v", err) on non-nil, and when calling stacks.CreateInDirectory (in the loop for
_, stack := range tt.localStacks) ensure it returns an error and fail the test
immediately if non-nil (or update CreateInDirectory to return an error and
propagate/fail). This ensures the subtest stops on fixture-setup failures rather
than continuing with broken fixtures.
---
Nitpick comments:
In `@src/cmd/cli/command/stack.go`:
- Around line 142-148: The inline DOWN-stack filtering in the loop (building
filteredStacks from stackList and checking StackStatus_STACK_STATUS_DOWN) is
duplicated with selector logic; create a shared helper in package stacks (e.g.,
func ExcludeDownStacks(items []stacks.ListItem) []stacks.ListItem or
FilterOutDown(items []stacks.ListItem) []stacks.ListItem) that encapsulates the
status check, replace the loop in stack.go to call that helper, and update
src/pkg/stacks/selector.go to use the same helper so status-based exclusion is
centralized and consistent.
🪄 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: 501132bd-6c68-40d7-ba77-a09466a6c59f
📒 Files selected for processing (3)
src/cmd/cli/command/commands_test.gosrc/cmd/cli/command/stack.gosrc/cmd/cli/command/stack_test.go
| columns := []string{"Name", "Default", "Provider", "Region", "Account", "Mode", "DeployedAt"} | ||
| return term.Table(stacks, columns...) | ||
| return term.Table(stackList, columns...) | ||
| }, | ||
| } | ||
| stackListCmd.Flags().Bool("json", false, "Output in JSON format") | ||
| stackListCmd.Flags().BoolP("all", "a", false, "Include stacks that are down") |
There was a problem hiding this comment.
--json output path is currently bypassed.
The command always returns term.Table(...), so --json is effectively ignored despite being defined. This is a user-facing regression for scripted usage.
💡 Proposed fix
import (
"context"
+ json_package "encoding/json"
"fmt"
+ "os"
@@
- columns := []string{"Name", "Default", "Provider", "Region", "Account", "Mode", "DeployedAt"}
- return term.Table(stackList, columns...)
+ if jsonOut, _ := cmd.Flags().GetBool("json"); jsonOut {
+ encoder := json_package.NewEncoder(os.Stdout)
+ return encoder.Encode(stackList)
+ }
+
+ columns := []string{"Name", "Default", "Provider", "Region", "Account", "Mode", "DeployedAt"}
+ return term.Table(stackList, columns...)
},
}📝 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.
| columns := []string{"Name", "Default", "Provider", "Region", "Account", "Mode", "DeployedAt"} | |
| return term.Table(stacks, columns...) | |
| return term.Table(stackList, columns...) | |
| }, | |
| } | |
| stackListCmd.Flags().Bool("json", false, "Output in JSON format") | |
| stackListCmd.Flags().BoolP("all", "a", false, "Include stacks that are down") | |
| if jsonOut, _ := cmd.Flags().GetBool("json"); jsonOut { | |
| encoder := json_package.NewEncoder(os.Stdout) | |
| return encoder.Encode(stackList) | |
| } | |
| columns := []string{"Name", "Default", "Provider", "Region", "Account", "Mode", "DeployedAt"} | |
| return term.Table(stackList, columns...) | |
| }, | |
| } | |
| stackListCmd.Flags().Bool("json", false, "Output in JSON format") | |
| stackListCmd.Flags().BoolP("all", "a", false, "Include stacks that are down") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cmd/cli/command/stack.go` around lines 158 - 163, The CLI currently
always returns term.Table(stackList, columns...) so the --json flag is ignored;
inside the stackListCmd RunE closure, read the "json" flag (e.g.
cmd.Flags().GetBool("json") or stackListCmd.Flags().GetBool("json")) and branch:
if true, marshal stackList to JSON and write it to stdout (or use an existing
JSON helper) and return, otherwise call term.Table as before; modify the code
around stackList, columns, and the term.Table call so the JSON path is taken
when "json" is true.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jordanstephens
force-pushed
the
jordan/stack-status
branch
from
df64bc7 to
6451812
Compare
2 participants
Description
We are going to have fabric determine stack status server side and include the result in this response
Linked Issues
Checklist
Summary by CodeRabbit
New Features
Behavior Changes
Tests