Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThreads a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Command
participant Compose as ComposeUp
participant Stacks as stacks.Manager
participant Fabric as Fabric gRPC
participant Storage as Backend Storage
CLI->>Compose: ComposeUp(ctx, fabric, provider, stack, params)
Compose->>Stacks: Marshal(stack)
Stacks-->>Compose: stackFile (bytes)
Compose->>Fabric: PutStack(StackFile, metadata)
Fabric->>Storage: store stack record
Storage-->>Fabric: OK
Fabric-->>Compose: PutStack OK
Compose->>Fabric: PutDeployment(deployment payload with deployedAt)
Fabric->>Storage: store deployment record
Storage-->>Fabric: OK
Fabric-->>Compose: PutDeployment OK
Compose-->>CLI: deployment result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
✏️ Tip: You can disable this entire section by setting Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" 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
🤖 Fix all issues with AI agents
In @src/pkg/stacks/manager.go:
- Around line 116-119: Replace the direct call to
stack.GetLastDeployedAt().AsTime() when building the StackListItem to avoid a
nil pointer: import the timeutils package and call
timeutils.AsTime(stack.GetLastDeployedAt()) to safely convert the timestamp;
update the StackListItem DeployedAt assignment in the stackParams append so it
uses that helper instead of .AsTime() directly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/protos/io/defang/v1/fabric.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
pkgs/npm/README.mdsrc/README.mdsrc/cmd/cli/command/commands_test.gosrc/cmd/cli/command/compose.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/agent/tools/deploy.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/cd.gosrc/pkg/cli/client/client.gosrc/pkg/cli/client/grpc.gosrc/pkg/cli/client/mock.gosrc/pkg/cli/common.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/composeUp_dockerfile_test.gosrc/pkg/cli/composeUp_test.gosrc/pkg/cli/preview.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.gosrc/pkg/stacks/stacks.gosrc/protos/io/defang/v1/fabric.proto
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-01-07T00:34:13.131Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1742
File: src/pkg/cli/composeDown.go:14-18
Timestamp: 2026-01-07T00:34:13.131Z
Learning: In Defang's Defang CLI, CdCommandDown performs refresh + destroy, while CdCommandDestroy performs destroy only (no refresh). Update ComposeDown (src/pkg/cli/composeDown.go) to call CdCommandDestroy to perform destruction without refreshing. This ensures the intended semantics are preserved when tearing down compositions; avoid using CdCommandDown in ComposeDown unless a refresh is explicitly desired. Verify that ComposeDown's destroy path does not trigger a refresh side effect from CdCommandDown and that tests cover both pathways if they exist.
Applied to files:
src/pkg/cli/client/client.gosrc/pkg/stacks/stacks.gosrc/pkg/cli/client/mock.gosrc/cmd/cli/command/commands_test.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/deploy.gosrc/pkg/cli/cd.gosrc/pkg/stacks/manager_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/cli/common.gosrc/pkg/cli/preview.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/composeUp.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/cli/composeUp_test.gosrc/pkg/stacks/manager.gosrc/pkg/cli/client/grpc.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/composeUp_dockerfile_test.go
📚 Learning: 2026-01-09T20:12:21.986Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:30-30
Timestamp: 2026-01-09T20:12:21.986Z
Learning: In Go files, recognize and accept the import path go.yaml.in/yaml/v3 as the maintained fork of the YAML library. Do not flag this import as incorrect; this fork supersedes the archived gopkg.in/yaml.v3 path. If you encounter this or similar forked import paths, treat them as valid Go imports and do not raise review flags.
Applied to files:
src/pkg/cli/client/client.gosrc/pkg/stacks/stacks.gosrc/pkg/cli/client/mock.gosrc/cmd/cli/command/commands_test.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/deploy.gosrc/pkg/cli/cd.gosrc/pkg/stacks/manager_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/cli/common.gosrc/pkg/cli/preview.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/composeUp.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/cli/composeUp_test.gosrc/pkg/stacks/manager.gosrc/pkg/cli/client/grpc.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/composeUp_dockerfile_test.go
📚 Learning: 2026-01-13T17:46:06.788Z
Learnt from: jordanstephens
Repo: DefangLabs/defang PR: 1754
File: src/pkg/agent/tools/provider.go:19-27
Timestamp: 2026-01-13T17:46:06.788Z
Learning: Go interfaces should be consumer-defined and small, defined in the package that uses them, rather than centralized, broad interfaces. Prefer interfaces that capture the specific methods required by a consumer, enabling implicit satisfaction and easier testing. Do not assume a single, global interface name across packages; it is acceptable (and sometimes intentional) for different packages to define interfaces with the same name but different method sets. Apply this guideline across all Go files in the repository.
Applied to files:
src/pkg/cli/client/client.gosrc/pkg/stacks/stacks.gosrc/pkg/cli/client/mock.gosrc/cmd/cli/command/commands_test.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/agent/tools/deploy.gosrc/pkg/cli/cd.gosrc/pkg/stacks/manager_test.gosrc/pkg/agent/tools/interfaces.gosrc/pkg/cli/common.gosrc/pkg/cli/preview.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/composeUp.gosrc/pkg/agent/tools/default_tool_cli.gosrc/pkg/cli/composeUp_test.gosrc/pkg/stacks/manager.gosrc/pkg/cli/client/grpc.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/composeUp_dockerfile_test.go
📚 Learning: 2025-12-31T13:47:12.225Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1740
File: src/pkg/cli/client/byoc/parse_test.go:18-21
Timestamp: 2025-12-31T13:47:12.225Z
Learning: In Go test files (any _test.go under the Defang codebase), it's acceptable for mocks to panic to surface issues quickly during tests. Do not add defensive error handling in mocks within tests, since panics will fail fast and highlight problems. Ensure this behavior is confined to test code and does not affect production code or non-test paths.
Applied to files:
src/cmd/cli/command/commands_test.gosrc/pkg/agent/tools/deploy_test.gosrc/pkg/stacks/manager_test.gosrc/pkg/cli/composeUp_test.gosrc/pkg/agent/tools/services_test.gosrc/pkg/cli/composeUp_dockerfile_test.go
📚 Learning: 2026-01-07T03:07:56.002Z
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/byoc.go:448-450
Timestamp: 2026-01-07T03:07:56.002Z
Learning: In src/pkg/cli/client/byoc/gcp/byoc.go, the GetDeploymentStatus method intentionally does not pre-validate b.cdExecution before calling b.driver.GetBuildStatus. If b.cdExecution is empty, it represents an error state that will be surfaced by the GCP API as an "invalid operation name" error, which is the intended behavior.
Applied to files:
src/pkg/cli/cd.go
📚 Learning: 2025-12-25T04:38:40.356Z
Learnt from: lionello
Repo: DefangLabs/defang PR: 1734
File: src/cmd/cli/command/commands.go:1220-1226
Timestamp: 2025-12-25T04:38:40.356Z
Learning: In the Defang CLI codebase (src/cmd/cli/command/commands.go), empty project names (from inputs like "/stack" or when --project-name is omitted) are acceptable and intentional behavior, as they work similarly to not providing the --project-name flag at all.
Applied to files:
src/pkg/agent/tools/interfaces.gosrc/pkg/cli/common.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/composeUp_test.gosrc/pkg/stacks/manager.go
🧬 Code graph analysis (13)
src/pkg/cli/client/client.go (1)
src/protos/io/defang/v1/fabric.pb.go (3)
PutStackRequest(759-764)PutStackRequest(777-777)PutStackRequest(792-794)
src/cmd/cli/command/commands_test.go (1)
src/protos/io/defang/v1/fabric.pb.go (9)
ListStacksRequest(899-904)ListStacksRequest(917-917)ListStacksRequest(932-934)ListStacksResponse(943-948)ListStacksResponse(961-961)ListStacksResponse(976-978)Stack(683-692)Stack(705-705)Stack(720-722)
src/pkg/agent/tools/deploy_test.go (2)
src/pkg/cli/composeUp.go (2)
ComposeUp(31-168)ComposeUpParams(24-28)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)
src/pkg/agent/tools/deploy.go (1)
src/pkg/cli/composeUp.go (2)
ComposeUp(31-168)ComposeUpParams(24-28)
src/pkg/stacks/manager_test.go (1)
src/pkg/cli/client/provider_id.go (1)
ProviderAWS(15-15)
src/pkg/agent/tools/interfaces.go (2)
src/pkg/cli/composeUp.go (2)
ComposeUp(31-168)ComposeUpParams(24-28)src/pkg/stacks/stacks.go (1)
StackParameters(18-25)
src/pkg/cli/common.go (2)
src/pkg/cli/client/provider.go (1)
Provider(63-89)src/pkg/stacks/stacks.go (2)
StackParameters(18-25)Marshal(181-186)
src/pkg/cli/preview.go (1)
src/pkg/cli/composeUp.go (1)
ComposeUp(31-168)
src/cmd/cli/command/compose.go (1)
src/pkg/cli/composeUp.go (2)
ComposeUp(31-168)ComposeUpParams(24-28)
src/pkg/cli/composeUp.go (1)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)
src/pkg/stacks/manager.go (2)
src/pkg/stacks/stacks.go (5)
StackListItem(131-134)DefaultBeta(75-75)Parse(177-179)ParamsFromMap(44-70)StackParameters(18-25)src/pkg/timeutils/timeutils.go (1)
AsTime(51-56)
src/pkg/cli/client/grpc.go (1)
src/protos/io/defang/v1/fabric.pb.go (12)
PutStackRequest(759-764)PutStackRequest(777-777)PutStackRequest(792-794)PutDeploymentRequest(3404-3409)PutDeploymentRequest(3422-3422)PutDeploymentRequest(3437-3439)ListStacksRequest(899-904)ListStacksRequest(917-917)ListStacksRequest(932-934)ListStacksResponse(943-948)ListStacksResponse(961-961)ListStacksResponse(976-978)
src/pkg/cli/composeUp_dockerfile_test.go (3)
src/pkg/stacks/stacks.go (1)
StackParameters(18-25)src/pkg/cli/client/provider_id.go (1)
ProviderDefang(14-14)src/pkg/cli/composeUp.go (2)
ComposeUp(31-168)ComposeUpParams(24-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: go-test
🔇 Additional comments (33)
src/README.md (1)
60-60: LGTM!Minor formatting change adding a trailing blank line for file consistency.
pkgs/npm/README.md (1)
60-60: LGTM!Consistent formatting change matching the other README files.
src/pkg/stacks/stacks.go (1)
181-186: LGTM!Good defensive nil-check that prevents nil pointer dereference when
params.ToMap()would be called. Returns an empty marshaled map, which is consistent with having no stack parameters. This properly supports call sites likePreviewthat passnilfor the stack.src/cmd/cli/command/commands_test.go (1)
249-254: LGTM!The mock correctly implements
ListStacksto support the new Stack RPC in tests. The implementation follows the same pattern as other mock methods in this file (e.g.,ListDeployments), returning an empty response which is appropriate for testing scenarios.src/pkg/cli/preview.go (1)
11-15: Pass nil for stack during Preview since preview operations don't persist actual stack metadata.Passing
nilfor the stack parameter is correct. Thestacks.Marshalfunction handles nil gracefully by returning an empty stack file content, which is then recorded viaPutStack. This ensures preview operations don't persist unnecessary stack metadata while still maintaining deployment history records.src/pkg/cli/client/client.go (1)
32-32: LGTM!The
PutStackmethod addition follows the existing interface patterns and is consistent with similar methods likePutDeployment. The signature appropriately returns only an error, matching the void-return style of mutation operations in this interface.src/pkg/cli/client/mock.go (1)
152-155: LGTM!The mock implementation follows the established pattern used by other methods in
MockFabricClient, such asPutDeployment. The simple nil return is appropriate for a test double.src/pkg/agent/tools/interfaces.go (1)
11-11: LGTM!The
ComposeUpsignature update correctly adds thestack *stacks.StackParametersparameter, aligning with the updated implementation insrc/pkg/cli/composeUp.go. The import and parameter placement are consistent with the broader PR changes. Based on learnings, this consumer-defined interface approach is the preferred pattern for Go.Also applies to: 18-18
src/pkg/agent/tools/deploy.go (1)
63-67: LGTM!The
ComposeUpcall correctly passessc.Stackto match the updated interface signature. This is consistent with howsc.Stackis already used forSetupProvideron line 48, maintaining coherent stack context throughout the deployment flow.src/pkg/cli/cd.go (1)
46-52: No issue:stacks.Marshalalready handlesnilstack gracefully.The
stacks.Marshalfunction (src/pkg/stacks/stacks.go:181-186) includes explicit nil-guard logic that returns an empty marshaled map when the stack parameter is nil. The code at src/pkg/cli/cd.go:46-52 safely passesnilfor CD teardown operations without risk of panics.Likely an incorrect or invalid review comment.
src/pkg/cli/composeUp.go (2)
31-31: LGTM! Stack parameter correctly added to function signature.The new
stack *stacks.StackParametersparameter is properly threaded through toputDeploymentfor recording stack metadata during deployment.
147-159: Stack parameter correctly passed to putDeployment.The error handling is appropriate - deployment proceeds even if recording fails, with a warning to the user.
src/pkg/agent/tools/services_test.go (2)
73-75: LGTM! Mock signature correctly updated.The
MockCLI.ComposeUpmethod now accepts thestack *stacks.StackParametersparameter, aligning with the updatedCLIInterface.
258-270: LGTM! Test properly instantiates and passes stack parameter.The test correctly creates a
StackParametersinstance withNameandProviderfields, then passes it through theStackConfigtoHandleServicesTool.src/pkg/cli/client/grpc.go (1)
94-106: LGTM! New gRPC client methods follow established patterns.Both
PutStackandListStacksare implemented consistently with existing methods likePutDeploymentandListDeployments, using thegetMsghelper for response handling.src/pkg/cli/composeUp_test.go (3)
104-111: LGTM! Test correctly passes stack parameter to ComposeUp.The stack is properly instantiated with
ProviderDefangfor the mock deployment test.
295-303: LGTM! Stack parameter properly added to error handling tests.The test suite for deployment failure scenarios correctly includes the new stack parameter.
332-338: LGTM! Empty stack correctly tests dry-run scenario.Using an empty
StackParameters{}is appropriate for theUploadModeIgnoredry-run test case.src/pkg/cli/composeUp_dockerfile_test.go (2)
45-57: LGTM! Stack parameter properly added to Dockerfile validation tests.The inline comment clarifies the rationale for using
ProviderDefang, and the updatedComposeUpcall correctly includes the stack parameter.
88-112: LGTM! Both test cases for skipped validation updated correctly.The stack is properly instantiated and passed to both
UploadModeIgnoreandUploadModeEstimatetest scenarios.src/pkg/agent/tools/deploy_test.go (1)
64-70: LGTM!The mock signature update correctly aligns with the new
ComposeUpfunction signature that now includes thestack *stacks.StackParametersparameter. The mock appropriately accepts but doesn't use the parameter, which is suitable for testing purposes.src/protos/io/defang/v1/fabric.proto (2)
158-164: LGTM!The
Providerenum follows protobuf best practices withPROVIDER_UNSPECIFIED = 0as the default value. The enum values align with the existingProviderIDconstants used in the codebase.
166-172: LGTM!The new
providerandlast_deployed_atfields extend theStackmessage appropriately. Usinggoogle.protobuf.Timestampfor the deployment timestamp is the correct choice for cross-language compatibility.src/pkg/cli/common.go (1)
53-76: LGTM!The implementation correctly marshals the stack parameters and invokes
PutStackbeforePutDeployment. Error handling follows the early-return pattern, and the stack metadata (name, project, provider, timestamp) is properly populated from the available context.src/cmd/cli/command/compose.go (2)
156-160: LGTM!The call site correctly passes
session.Stackto align with the updatedComposeUpsignature.
618-622: LGTM!Consistent with the other call site,
session.Stackis correctly passed toComposeUpfor the config command.src/pkg/stacks/manager_test.go (3)
19-32: LGTM!The mock correctly implements the new
StackListerinterface withListStacksreplacing the previous deployment-based approach. The return type and response structure align with the protobuf definitions.
271-289: LGTM!The test fixtures correctly use the new
Stackstructure withName,StackFile(as dotenv-formatted content), andLastDeployedAtfields.
445-490: Verify: Duplicate stack names are now returned without deduplication.The test expects 2 stacks with the same name to be returned (lines 477-489), sorted by
LastDeployedAtdescending. This differs from typical behavior where duplicate names might be deduplicated. Confirm this is the intended design for theListStacksAPI.src/pkg/agent/tools/default_tool_cli.go (1)
53-55: LGTM!The signature update correctly propagates the
stack *stacks.StackParametersparameter tocli.ComposeUp, maintaining the passthrough pattern used byDefaultToolCLI. This aligns with the PR's goal of passing stack context through the deployment flow.src/pkg/stacks/manager.go (3)
16-18: LGTM!The
StackListerinterface follows Go best practices with a consumer-defined, single-method interface. This enables easy mocking for testing. Based on learnings, this aligns with the guideline that Go interfaces should be small and defined in the consumer package.
20-41: LGTM!The
managerstruct andNewManagerconstructor are correctly updated to use the newStackListerinterface. The constructor logic remains unchanged, with only the dependency type being swapped.
91-115: LGTM on the migration logic.The migration from
ListDeploymentstoListStacksis well-implemented:
- Proper error wrapping with context
- Graceful handling of invalid stacks with warnings and skipping
- Correct usage of
DefaultBetafor empty namesParamsFromMapcorrectly parses variables from the stack fileThe overall flow is clean and maintains parity with the previous behavior while adapting to the new Stack API.
jordanstephens
force-pushed
the
jordan/list-stacks-rpc
branch
from
9ca89cc to
9b79305
Compare
Co-authored-by: Lio李歐 <lionello@users.noreply.github.com>
jordanstephens
force-pushed
the
jordan/list-stacks-rpc
branch
from
bc5f662 to
4509f22
Compare
2 participants
Description
PutStackduring deploymentListStackswhen fetching remote stacksProviderandLastDeployedAtproperties toStackprotobuf message.Linked Issues
Checklist
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.