Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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?
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