fix(cd): update deployment history for cd down/destroy commands #1742
Conversation
📝 WalkthroughWalkthroughRenames BootstrapCommand/BootstrapList to CdCommand/CdList across CLI and provider layers, removes public Destroy APIs, adds CdCommand/CdCommandAndTail/TailAndWaitForCD flows, propagates a FabricClient through CLI handlers, and updates related tests and BYOC implementations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant Provider
participant Fabric
participant TailSvc
Note over User,CLI: User runs `cli cd <command>` (e.g., destroy/down)
User->>CLI: invoke cdCommand(cmd, args, CdCommand, FabricClient)
CLI->>Provider: CdCommand(ctx, CdCommandRequest{Project, Command})
alt Provider handles command
Provider->>Fabric: start CD task / return ETag
Fabric-->>Provider: ETag
Provider-->>CLI: (ETag, nil)
else Provider returns error
Provider-->>CLI: error
end
CLI->>Fabric: putDeployment(ctx, params{Action, ETag, Project,...})
Fabric-->>CLI: ack
CLI->>TailSvc: TailAndWaitForCD(ctx, TailOptions{Since, Deployment, LogType, Verbose})
TailSvc-->>CLI: streamed logs / completion
CLI->>CLI: postDown (on destroy/down) -> Fabric.DeleteSubdomainZone (non-fatal errors logged)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cmd/cli/command/cd.gosrc/pkg/cli/bootstrap.go
🧰 Additional context used
🧠 Learnings (1)
📚 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/cli/bootstrap.go
🧬 Code graph analysis (2)
src/pkg/cli/bootstrap.go (2)
src/protos/io/defang/v1/fabric.pb.go (14)
Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)DeleteSubdomainZoneRequest(4420-4426)DeleteSubdomainZoneRequest(4439-4439)DeleteSubdomainZoneRequest(4454-4456)PutDeploymentRequest(2948-2953)PutDeploymentRequest(2966-2966)PutDeploymentRequest(2981-2983)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)DeploymentAction_DEPLOYMENT_ACTION_DOWN(428-428)src/pkg/cli/client/client.go (1)
FabricClient(17-44)
src/cmd/cli/command/cd.go (2)
src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/cli/bootstrap.go (1)
BootstrapCommand(18-73)
⏰ 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 (4)
src/cmd/cli/command/cd.go (2)
36-61: LGTM: Parameter threading is correct.The addition of the
fabricparameter and forwarding it tocli.BootstrapCommandis straightforward and correct.
68-68: LGTM: All command handlers updated consistently.All
RunEhandlers correctly passglobal.Clientas the fabric argument tobootstrapCommand.Also applies to: 77-77, 86-86, 95-95, 141-141
src/pkg/cli/bootstrap.go (2)
14-15: LGTM: Necessary imports added.The new imports support the DeleteSubdomainZone and PutDeployment functionality.
18-18: LGTM: Function signature updated correctly.The addition of the
fabricparameter enables the new deployment history tracking functionality.
jordanstephens
left a comment
jordanstephens
left a comment
There was a problem hiding this comment.
Personally, I'm not convinced we should continue to maintain these cd commands. They are obscure and little used and will always lag behind the premier "compose" commands. If it's very important that we maintain them, let's discuss if it is possible to define the compose commands in terms of the cd commands—thereby easing the maintenance burden.
What's important is that I'm able to down without having access to the source files for the project.
This has been supported by defang [compose] down --project-name and will be also be supported by defang [compose] down --stack after #1721 is merged.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pkg/cli/composeDown_test.go (1)
75-83: Fix variable naming in type assertion.The type assertion on line 76 assigns the boolean result to a variable named
err, which is confusing since it's not an error but a success indicator. The variable should be namedokto follow Go conventions.🔎 Proposed fix
- req, err := req.(*client.CdCommandRequest) - if !err { + req, ok := req.(*client.CdCommandRequest) + if !ok { t.Errorf("ComposeDown() failed: expected CdCommandRequest, got %v", req) }Based on learnings: In Go test files under the Defang codebase, test code quality still matters for maintainability even though panics are acceptable in mocks.
🤖 Fix all issues with AI Agents
In @src/pkg/cli/client/playground.go:
- Around line 87-92: CdCommand currently only treats req.Command == "down" and
returns an error for "destroy"; update the command check in
PlaygroundProvider.CdCommand to also accept "destroy" so both commands call
g.destroy(ctx, &defangv1.DestroyRequest{Project: req.Project}). Locate the
CdCommand method and adjust the conditional (or switch) to include "destroy"
alongside "down" before returning the invalid-command error.
In @src/pkg/cli/composeDown_test.go:
- Around line 101-103: In composeDown_test.go change the type-assertion boolean
variable from err to ok in the ComposeDown() test: replace the line doing "req,
err := req.(*defangv1.DeleteRequest)" and the subsequent check "if !err { ... }"
with "req, ok := req.(*defangv1.DeleteRequest)" and "if !ok { ... }" so the
boolean result of the type assertion uses the conventional name `ok` and the
error variable name remains available for real errors.
In @src/pkg/cli/destroy_test.go:
- Line 60: The test uses fabric.CdCommand with Command: "down" but the test is
named TestDestroy; change the request to use Command: "destroy" for semantic
consistency—update the call to fabric.CdCommand(ctx,
client.CdCommandRequest{Command: "destroy", Project: "test-project"}) so the
test matches the intended command name while keeping the same behavior (both map
to postDown()).
🧹 Nitpick comments (1)
src/pkg/cli/composeDown.go (1)
29-32: Error message loses context from the original error.The error wrapping discards the original
errfromGetDelegateSubdomainZone. Consider preserving it for debugging:🔎 Suggested fix
if err != nil { term.Debug("GetDelegateSubdomainZone failed:", err) - return "", errors.New("failed to get delegate domain") + return "", fmt.Errorf("failed to get delegate domain: %w", err) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/cmd/cli/command/cd.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/activeDeployments_test.gosrc/pkg/cli/cd.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/common.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/composeDown_test.gosrc/pkg/cli/composeUp.gosrc/pkg/cli/deploymentsList_test.gosrc/pkg/cli/destroy_test.gosrc/pkg/cli/teardown_cd.go
💤 Files with no reviewable changes (1)
- src/pkg/cli/activeDeployments_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cmd/cli/command/cd.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/cli/common.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/client/byoc/baseclient.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/pkg/cli/deploymentsList_test.gosrc/pkg/cli/client/byoc/gcp/byoc_test.gosrc/pkg/cli/composeDown_test.gosrc/pkg/cli/destroy_test.go
🧬 Code graph analysis (13)
src/pkg/cli/client/provider.go (2)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/types/etag.go (1)
ETag(9-9)
src/pkg/cli/client/byoc/gcp/byoc.go (3)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/pkg/types/etag.go (1)
ETag(9-9)
src/pkg/cli/common.go (7)
src/protos/io/defang/v1/fabric.pb.go (18)
DeploymentAction(423-423)DeploymentAction(458-460)DeploymentAction(462-464)DeploymentAction(471-473)DeploymentMode(83-83)DeploymentMode(118-120)DeploymentMode(122-124)DeploymentMode(131-133)Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)PutDeploymentRequest(2948-2953)PutDeploymentRequest(2966-2966)PutDeploymentRequest(2981-2983)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/clouds/driver.go (1)
ProjectName(8-8)src/pkg/cli/client/provider.go (1)
Provider(43-70)src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)src/pkg/cli/compose/loader.go (1)
Project(25-25)
src/cmd/cli/command/compose.go (2)
src/pkg/cli/composeDown.go (1)
InteractiveComposeDown(58-73)src/pkg/elicitations/elicitations.go (1)
Client(16-18)
src/pkg/cli/composeUp.go (5)
src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/modes/modes.go (1)
Mode(12-12)src/pkg/clouds/driver.go (1)
ProjectName(8-8)src/pkg/cli/compose/loader.go (1)
Services(29-29)src/pkg/term/colorizer.go (2)
Debug(294-296)Warn(310-312)
src/pkg/cli/client/byoc/aws/byoc.go (2)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)
src/pkg/cli/client/playground.go (3)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/protos/io/defang/v1/fabric.pb.go (3)
DestroyRequest(840-845)DestroyRequest(858-858)DestroyRequest(873-875)
src/pkg/cli/client/byoc/baseclient.go (4)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/pkg/types/etag.go (1)
ETag(9-9)src/protos/io/defang/v1/fabric.pb.go (3)
ProjectUpdate(3672-3686)ProjectUpdate(3699-3699)ProjectUpdate(3714-3716)
src/pkg/cli/composeDown_test.go (3)
src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/cd.go (1)
CdCommand(18-41)
src/pkg/cli/destroy_test.go (3)
src/pkg/cli/client/playground.go (3)
PlaygroundProvider(17-21)PlaygroundProvider(162-164)PlaygroundProvider(166-168)src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)
src/pkg/cli/composeDown.go (1)
src/protos/io/defang/v1/fabric.pb.go (7)
GetDelegateSubdomainZoneRequest(4472-4478)GetDelegateSubdomainZoneRequest(4491-4491)GetDelegateSubdomainZoneRequest(4506-4508)DeleteRequest(1608-1615)DeleteRequest(1628-1628)DeleteRequest(1643-1645)DeploymentAction_DEPLOYMENT_ACTION_UP(427-427)
src/pkg/cli/client/byoc/do/byoc.go (2)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)
src/pkg/cli/cd.go (6)
src/pkg/cli/client/provider.go (2)
Provider(43-70)CdCommandRequest(20-23)src/pkg/cli/client/client.go (1)
FabricClient(17-44)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/dryrun/dryrun.go (2)
DoDryRun(6-6)ErrDryRun(8-8)src/protos/io/defang/v1/fabric.pb.go (1)
DeploymentAction_DEPLOYMENT_ACTION_DOWN(428-428)src/pkg/cli/tail.go (1)
TailOptions(45-58)
⏰ 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 (32)
src/pkg/cli/deploymentsList_test.go (3)
122-152: LGTM!The test setup and first subtest follow good testing practices. The structure mirrors
TestDeploymentsListconsistently, and the validation appropriately checks for the "No deployments found" message when there are no active deployments.
154-199: Well-structured test with comprehensive provider coverage.The test data covers multiple providers (AWS, DigitalOcean, GCP, Defang) with varied regions, and the validation logic properly verifies each deployment appears in the output.
Note: Line 181 uses a hardcoded offset to skip the header. This is a common CLI testing pattern, but be aware it may require updates if the output format changes.
111-120: No changes needed – WhoAmI is not called in the tested code path.The
mockActiveDeploymentsHandlerdoes not need to implementWhoAmIbecause the test callsConnect(), notConnectWithTenant(), and neitherConnect()norDeploymentsList()invokesWhoAmI. ThemockListDeploymentsHandlerimplementingWhoAmIin the existing test is similarly unnecessary. Both mocks are appropriate for their respective test cases.Likely an incorrect or invalid review comment.
src/pkg/cli/client/byoc/gcp/byoc_test.go (1)
43-46: LGTM! Test strings updated to reflect API rename.The test assertions and log messages correctly reference the renamed
CdCommand()method instead ofBootstrapCommand(), consistent with the broader refactoring.src/pkg/cli/teardown_cd.go (1)
18-18: LGTM! Updated to use renamed CdList method.The call correctly uses
CdListinstead of the oldBootstrapListmethod, maintaining the same functionality.src/pkg/cli/common.go (1)
42-71: LGTM! Well-structured helper for recording deployment history.The new
putDeploymenthelper effectively centralizes deployment history recording. The function properly retrieves account information, constructs the deployment message with all required fields, and handles errors appropriately.The comment on line 53 about AccountInfo "should not happen because we checked earlier" is accurate - callers like ComposeUp invoke Deploy/Preview operations that require valid account info, so this error path is defensive.
src/pkg/cli/composeUp.go (1)
143-153: LGTM! Good refactoring to use the centralized putDeployment helper.The change from inline deployment history recording to the new
putDeploymenthelper improves consistency and maintainability. The error handling appropriately treats deployment history as nice-to-have - logging a warning but allowing the deployment to proceed.src/cmd/cli/command/compose.go (1)
313-313: LGTM! Updated call matches the new InteractiveComposeDown signature.The call correctly provides arguments in the updated order:
(ctx, projectName, global.Client, provider), matching the expanded API that now includes the fabric client parameter for CD operations.src/pkg/cli/destroy_test.go (1)
58-58: LGTM: Variable rename improves clarity.The rename from
clienttofabricbetter reflects the nature of the PlaygroundProvider wrapping a FabricClient.src/pkg/cli/client/provider.go (2)
20-23: LGTM: Clean API rename.The rename from
BootstrapCommandRequesttoCdCommandRequestbetter reflects the CD-focused workflow. The struct fields remain unchanged, maintaining backward compatibility at the data level.
46-47: LGTM: Systematic API consolidation.The renaming of
BootstrapCommand→CdCommandandBootstrapList→CdListaligns with the CD-focused workflow. The removal of theDestroymethod from the interface indicates consolidation into theCdCommandpath with command strings like "down" or "destroy".src/pkg/cli/client/byoc/baseclient.go (2)
30-33: LGTM: Interface updates align with Provider changes.The
ProjectBackendinterface correctly addsCdCommandand renamesBootstrapListtoCdList, maintaining consistency with the broader API refactoring in the Provider interface.
95-95: LGTM: Call site correctly updated.The method call is properly updated from
BootstrapListtoCdList, preserving the existing logic.src/pkg/cli/client/byoc/aws/byoc.go (2)
926-943: LGTM: CdCommand implementation is correct.The method correctly:
- Sets up the CD infrastructure
- Constructs and runs the CD command from the request
- Tracks deployment state (etag, start time, task ARN)
- Returns the etag for tracking purposes
957-974: LGTM: CdList implementation preserves original logic.The method correctly handles both single-region and multi-region Pulumi stack listing. The rename is the only change, with logic intact.
src/pkg/cli/composeDown_test.go (3)
16-16: LGTM: Mock field correctly updated.The
MockCdCommandfield properly replacesMockDestroywith the new signature matching the Provider interface.
27-32: LGTM: Method implementation correct.The
CdCommandmethod properly implements the Provider interface and delegates to the mock function.
55-58: LGTM: Mock initialization updated correctly.The mock properly stores the
CdCommandRequestfor later assertions and returns the expected etag.src/pkg/cli/client/byoc/gcp/byoc.go (3)
263-301: LGTM!The
CdListmethod (renamed fromBootstrapList) correctly implements the provider interface for listing CD stacks. The implementation properly handles bucket access, object iteration, and Pulumi stack parsing with appropriate error logging.
329-342: LGTM!The
CdCommandmethod (renamed fromBootstrapCommand) correctly implements the updated interface signature, acceptingclient.CdCommandRequestand returning(types.ETag, error). The delegation toSetUpCDandrunCdCommandis appropriate.
563-565: LGTM!The comment correctly documents that
CdCommandreturns the job execution ID as the eTag.src/pkg/cli/composeDown.go (3)
19-22: LGTM!Clean delegation to
CdCommandfor full project destruction when no service names are specified. The flow correctly routes full teardown through the CD path which handles subdomain deletion and deployment history updates viapostDown.
43-51: Verify: UsingDEPLOYMENT_ACTION_UPfor a delete operation seems counterintuitive.When deleting specific services (partial update), the code records this as
DEPLOYMENT_ACTION_UP. While semantically this could be considered an "update" (the project continues with remaining services), it may be confusing from a deployment history perspective. Consider whether a separate action likeDEPLOYMENT_ACTION_UPDATEor documenting this choice would improve clarity.
58-72: LGTM!The
InteractiveComposeDownsignature update correctly propagates thefabricclient, and the delegation toComposeDownuses the updated parameter order.src/pkg/cli/client/playground.go (2)
94-100: LGTM!The internal
destroymethod is correctly extracted and properly calls the FabricClient's Destroy endpoint, returning the ETag from the response.
110-112: LGTM!The
CdListmethod (renamed fromBootstrapList) correctly returns an appropriate error indicating this command is not valid for the Playground provider.src/pkg/cli/client/byoc/do/byoc.go (2)
224-242: LGTM with note: ETag generation timing differs from other providers.The implementation correctly renames the method and delegates to
runCdCommand. Note that unlike GCP (which returns the execution ID as ETag), this implementation generates a new ETag after the command runs. This is acceptable as long as the ETag is used consistently for subsequent operations like log tailing.
244-256: LGTM!The
CdListmethod correctly delegates toawsbyoc.ListPulumiStacksfor listing stacks. The error handling appropriately returns early when the bucket doesn't exist or on errors.src/pkg/cli/cd.go (4)
18-41: LGTM!The
CdCommandfunction correctly:
- Logs the command execution with project context
- Handles dry-run mode
- Invokes the provider's
CdCommand- Calls
postDownfor "down" and "destroy" commands as non-fatal post-processing- Returns the ETag for subsequent operations like log tailing
The error handling for
postDownfailure is appropriate—logging a warning and proceeding ensures the CD operation isn't blocked by bookkeeping failures.
43-67: LGTM!The
postDownhelper cleanly separates concerns:
- Deletes the subdomain zone (with helpful workspace validation message on failure)
- Records the deployment as DOWN only if zone deletion succeeds
This ensures deployment history is only updated when the caller has proper workspace access, preventing inconsistent state.
69-83: LGTM!Capturing
sincebefore command execution ensures log tailing includes all logs generated during the CD operation. The flow correctly propagates the ETag and verbose flag to the tail options.
113-142: LGTM!The
CdListLocalfunction (renamed fromBootstrapLocalList) correctly uses the renamedprovider.CdListmethod and provides helpful output when no projects are found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pkg/cli/composeDown_test.go (1)
66-66: Update test name to reflect the new method.The test description still references
Provider.Destroy, but the code now callsCdCommand. Consider updating for clarity.🔎 Suggested fix
- t.Run("Expect `Provider.Destroy` to be called when no specific services are specified", + t.Run("Expect `Provider.CdCommand` to be called when no specific services are specified",src/pkg/cli/composeDown.go (1)
29-32: Consider wrapping the original error for better debugging.The original error is logged but the returned error loses context. Wrapping the error would help troubleshooting without exposing internal details.
🔎 Suggested fix
if err != nil { term.Debug("GetDelegateSubdomainZone failed:", err) - return "", errors.New("failed to get delegate domain") + return "", fmt.Errorf("failed to get delegate domain: %w", err) }This requires adding
"fmt"to imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/pkg/cli/client/playground.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/composeDown_test.gosrc/pkg/cli/destroy_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/cli/destroy_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/pkg/cli/composeDown_test.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/cli/client/playground.go
🧬 Code graph analysis (2)
src/pkg/cli/composeDown_test.go (3)
src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/cd.go (1)
CdCommand(18-41)
src/pkg/cli/client/playground.go (4)
src/pkg/cli/cd.go (1)
CdCommand(18-41)src/pkg/cli/client/provider.go (1)
CdCommandRequest(20-23)src/pkg/types/etag.go (1)
ETag(9-9)src/protos/io/defang/v1/fabric.pb.go (3)
DestroyRequest(840-845)DestroyRequest(858-858)DestroyRequest(873-875)
⏰ 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 (7)
src/pkg/cli/composeDown_test.go (2)
75-83: LGTM on type assertion fix.The type assertion now correctly uses
okas the boolean result variable, following Go conventions.
100-108: Type assertion fix correctly applied.The type assertion for
DeleteRequestnow uses the conventionalokvariable name, addressing the previous review feedback.src/pkg/cli/client/playground.go (2)
94-100: LGTM on destroy method refactor.Making
destroyprivate and invoking it fromCdCommandis a clean encapsulation pattern that aligns with the broader refactoring goals.
110-112: LGTM on method rename.The
CdListrename is consistent with the broaderBootstrap*→Cd*refactoring across the codebase.src/pkg/cli/composeDown.go (3)
19-22: LGTM on early exit for full project destruction.Delegating to
CdCommandwith "destroy" for full project teardown centralizes the destruction logic and ensures deployment history is updated viapostDown.
43-51: LGTM on non-blocking deployment history update.Logging a warning but continuing when
putDeploymentfails is appropriate—this ensures the actual delete operation isn't blocked by history-recording issues.
58-58: All callers have been updated correctly.The single call site at
src/cmd/cli/command/compose.go:313correctly passes arguments in the new order:ctx, projectName, global.Client, provider. The function signature change has been properly propagated.
* removed Delete from providers (down with services) * better warning message when DeleteSubdomainZone fails * strongly-typed CD commands
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/pkg/cli/cd.go:
- Line 20: The comment on the projectName check in CdCommand is misleading (it
references "list --remote"); update or remove it so it accurately reflects
CdCommand behavior: either delete the outdated comment or replace it with a
short clear note about why projectName can be empty in CdCommand (e.g., when no
argument supplied or when switching to workspace root), referencing the
projectName variable and the CdCommand function so reviewers can locate and
change the comment accordingly.
- Around line 111-114: SplitProjectStack currently assumes the input contains a
"/", which can panic; update the SplitProjectStack function to check the result
of strings.SplitN(name, "/", 2) and if len(parts) < 2 return parts[0] and an
empty stack string (or alternatively return two empty strings or an error per
project conventions), ensuring no index access of parts[1]; include a brief
comment documenting the chosen behavior.
- Around line 59-64: The comment above the putDeployment call is inaccurate
about timing; update it to state that the deployment record is marked as
destroyed only when subdomain deletion either succeeds or is skipped (i.e.,
putDeployment is only reached if DeleteSubdomainZone did not return an error).
Edit the comment near the DeleteSubdomainZone and putDeployment calls to
explicitly mention that putDeployment runs only after successful subdomain
deletion or when deletion is intentionally skipped.
In @src/pkg/cli/client/playground.go:
- Around line 87-92: PlaygroundProvider.CdCommand currently only handles
CdCommandDestroy and returns an error for other CD verbs; update CdCommand to
also handle CdCommandDown by treating it the same as destroy (call
g.destroy(ctx, &defangv1.DestroyRequest{Project: req.Project})), so both
CdCommandDown and CdCommandDestroy go through the destroy flow and return the
same types. Ensure the condition in CdCommand checks for both CdCommandDestroy
and CdCommandDown and returns the result of g.destroy for either case.
In @src/pkg/cli/composeDown.go:
- Around line 14-18: ComposeDown currently calls client.CdCommandDestroy but
should call client.CdCommandDown to implement the "compose down" flow; update
the return call in ComposeDown to pass client.CdCommandDown instead of
client.CdCommandDestroy (leave other args and the CdCommand invocation intact so
ComposeDown continues to call CdCommand with the correct command enum).
🧹 Nitpick comments (2)
src/pkg/cli/composeDown_test.go (1)
13-19: Consider removing unusedMockDeletefield.The
MockDeletefield and its initialization (lines 52-55) appear unused since the test now only exercises theCdCommandpath. If per-service deletion is no longer tested here, consider removing the dead code.🔎 Suggested cleanup
type mockComposeDown struct { client.MockProvider MockAccountInfo func(ctx context.Context) (*client.AccountInfo, error) MockCdCommand func(ctx context.Context, req client.CdCommandRequest) (types.ETag, error) - MockDelete func(ctx context.Context, req *defangv1.DeleteRequest) (*defangv1.DeleteResponse, error) request map[string]any }And remove the corresponding initialization in the test setup.
src/pkg/cli/cd.go (1)
96-96: Consider extracting magic number to a named constant.The 2-second delay before canceling the tail is hardcoded. For better maintainability and clarity, consider extracting it to a named constant like
tailCancelDelayorlogFlushDelaywith a descriptive comment explaining why this delay is necessary.🔎 Suggested refactor
At the package level:
+const ( + // tailCancelDelay ensures we capture final logs before canceling the tail + tailCancelDelay = 2 * time.Second +)Then in the function:
- pkg.SleepWithContext(ctx, 2*time.Second) // a delay before cancelling tail to make sure we got the last logs + pkg.SleepWithContext(ctx, tailCancelDelay)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/cmd/cli/command/cd.gosrc/cmd/cli/command/commands.gosrc/cmd/cli/command/compose.gosrc/pkg/cli/cd.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/client/byoc/do/byoc.gosrc/pkg/cli/client/byoc/gcp/byoc.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/provider.gosrc/pkg/cli/common.gosrc/pkg/cli/composeDown.gosrc/pkg/cli/composeDown_test.gosrc/pkg/stacks/manager.gosrc/pkg/stacks/manager_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 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/pkg/stacks/manager_test.gosrc/pkg/cli/composeDown_test.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/cli/common.gosrc/pkg/cli/client/playground.gosrc/pkg/cli/client/byoc/baseclient.gosrc/pkg/cli/cd.gosrc/cmd/cli/command/compose.go
🧬 Code graph analysis (14)
src/pkg/stacks/manager_test.go (1)
src/pkg/term/colorizer.go (1)
Error(318-320)
src/pkg/cli/client/provider.go (2)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/types/etag.go (1)
ETag(9-9)
src/pkg/cli/client/byoc/aws/byoc.go (2)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/cli/client/provider.go (2)
CdCommand(20-20)CdCommandRequest(31-34)
src/pkg/cli/common.go (4)
src/protos/io/defang/v1/fabric.pb.go (18)
DeploymentAction(423-423)DeploymentAction(458-460)DeploymentAction(462-464)DeploymentAction(471-473)DeploymentMode(83-83)DeploymentMode(118-120)DeploymentMode(122-124)DeploymentMode(131-133)Provider(28-28)Provider(66-68)Provider(70-72)Provider(79-81)PutDeploymentRequest(2948-2953)PutDeploymentRequest(2966-2966)PutDeploymentRequest(2981-2983)Deployment(2822-2838)Deployment(2851-2851)Deployment(2866-2868)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/client/provider.go (1)
Provider(54-80)src/pkg/cli/client/account_info.go (1)
AccountInfo(5-10)
src/pkg/cli/client/byoc/do/byoc.go (2)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/cli/client/provider.go (2)
CdCommand(20-20)CdCommandRequest(31-34)
src/pkg/cli/client/playground.go (3)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/cli/client/provider.go (3)
CdCommand(20-20)CdCommandRequest(31-34)CdCommandDestroy(24-24)src/protos/io/defang/v1/fabric.pb.go (3)
DestroyRequest(840-845)DestroyRequest(858-858)DestroyRequest(873-875)
src/cmd/cli/command/cd.go (3)
src/pkg/cli/cd.go (3)
CdCommand(19-42)CdCommandAndTail(72-86)CdListLocal(116-146)src/pkg/cli/client/provider.go (6)
CdCommand(20-20)CdCommandDestroy(24-24)CdCommandDown(25-25)CdCommandRefresh(27-27)CdCommandCancel(23-23)CdCommandList(26-26)src/pkg/cli/client/client.go (1)
FabricClient(11-38)
src/pkg/cli/composeDown_test.go (2)
src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/cd.go (1)
CdCommand(19-42)
src/cmd/cli/command/commands.go (1)
src/pkg/stacks/manager.go (1)
NewManager(36-56)
src/pkg/stacks/manager.go (1)
src/pkg/stacks/stacks.go (2)
Load(239-241)StackParameters(18-25)
src/pkg/cli/composeDown.go (3)
src/pkg/cli/client/provider.go (3)
Provider(54-80)CdCommand(20-20)CdCommandDestroy(24-24)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/cd.go (1)
CdCommand(19-42)
src/pkg/cli/client/byoc/baseclient.go (3)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/cli/client/provider.go (2)
CdCommand(20-20)CdCommandRequest(31-34)src/pkg/types/etag.go (1)
ETag(9-9)
src/cmd/cli/command/compose.go (2)
src/pkg/cli/composeDown.go (2)
InteractiveComposeDown(23-38)ComposeDown(14-19)src/pkg/elicitations/elicitations.go (1)
Client(16-18)
src/pkg/cli/client/byoc/gcp/byoc.go (4)
src/pkg/cli/cd.go (1)
CdCommand(19-42)src/pkg/cli/client/provider.go (2)
CdCommand(20-20)CdCommandRequest(31-34)src/pkg/types/etag.go (1)
ETag(9-9)src/pkg/cli/compose/loader.go (1)
Project(25-25)
⏰ 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/pkg/stacks/manager.go (3)
160-168: LGTM! Improved error context with operation-specific messaging.The addition of the
Operationfield toErrOutsideprovides clearer error messages that indicate exactly which operation failed when working outside the target directory.
109-114: Consistent operation context across allErrOutsideusages.The
ListLocal,Load, andCreatemethods now consistently populate theOperationfield, ensuring users get actionable error messages.Also applies to: 170-173, 189-193
129-129: Minor simplification looks good.Directly iterating over
resp.GetDeployments()instead of using an intermediate variable is cleaner.src/pkg/stacks/manager_test.go (1)
677-678: Test expectations correctly updated to match new error format.The updated error message substrings align with the
Operationfield additions inErrOutside.Also applies to: 710-711
src/cmd/cli/command/commands.go (1)
1433-1436: LGTM! Clean variable declaration.Consolidating the stack manager creation into a single short declaration is idiomatic Go.
src/pkg/cli/common.go (2)
42-71: Well-structured deployment recording helper.The
putDeploymentParamsstruct andputDeploymentfunction cleanly encapsulate the deployment history update logic. The#nosec G115annotation appropriately documents the int32 conversion, which is safe given practical service count limits.
8-8: Thetermpackage is used in the file. Line 38 callsterm.Println(string(bytes)), so the import is necessary and should not be removed.Likely an incorrect or invalid review comment.
src/pkg/cli/client/provider.go (2)
20-34: Good API design with strongly-typed CD commands.The
CdCommandtype with defined constants provides type safety and clear documentation of valid commands. TheCdCommandRequeststruct is well-designed.
54-80: Clean interface refactoring.Renaming
BootstrapCommand/BootstrapListtoCdCommand/CdListand removing the standaloneDestroymethod aligns with the PR's goal of consolidating CD operations. The unified command pattern throughCdCommandis a cleaner design.src/cmd/cli/command/cd.go (3)
36-61: Well-refactored CD command handler.The
cdCommandfunction now accepts the fabric client parameter needed for deployment history updates. The use of typedclient.CdCommandvalues instead of strings improves type safety.
63-97: Consistent command handler updates.All CD command handlers (
destroy,down,refresh,cancel) correctly pass the typed command constant andglobal.Clientto enable deployment history tracking.
130-143: List command properly updated.The remote list path now uses
CdCommandAndTailwithclient.CdCommandList, and the local path usesCdListLocal. This aligns with the new CD command pattern.src/pkg/cli/client/byoc/do/byoc.go (3)
224-242: API rename correctly implemented.The
CdCommandmethod correctly converts the typed command to a string for the underlying command execution. The method follows the same pattern as before with the updated types.
244-256:CdListimplementation looks correct.The method properly delegates to
awsbyoc.ListPulumiStacksfor listing Pulumi stacks from S3.
231-231: The hardcoded "dummy.domain" is intentional for non-deploy CD commands.The dummy domain is deliberately used only in the
CdCommand()function for operational commands (destroy, down, etc.), while deployment commands indeploy()use the actualreq.DelegateDomain. This is sound design since non-deploy operations don't require domain delegation—they only execute infrastructure teardown where domain delegation is irrelevant.src/pkg/cli/client/byoc/aws/byoc.go (2)
902-919: LGTM - CdCommand implementation is correct.The rename from
BootstrapCommandtoCdCommandis consistent with the interface changes. The use ofstring(req.Command)correctly converts theCdCommandtype to a string for the internal command structure. The etag/start/taskArn tracking is preserved for log correlation.
933-950: LGTM - CdList implementation unchanged aside from rename.The logic for listing Pulumi stacks remains intact. The rename from
BootstrapListtoCdListaligns with the broader API refactor.src/pkg/cli/client/byoc/baseclient.go (2)
29-34: LGTM - Interface updated consistently.The
ProjectBackendinterface correctly reflects the renamed methods. The return typetypes.ETagforCdCommandis appropriate.
97-118: LGTM - Call site updated correctly.The
RemoteProjectNamemethod now correctly callsCdListinstead of the oldBootstrapList.src/cmd/cli/command/compose.go (2)
328-336: LGTM - Updated call signature for InteractiveComposeDown.The call now correctly passes
global.Client(fabric client) andproviderto enable deployment history updates in the centralized CD flow.
439-471: Verify intentional removal of per-service down functionality.The command changed from accepting
SERVICE...arguments tocobra.NoArgs. This removes the ability to selectively bring down individual services. The comment on line 444 indicates this may be restored later (// TODO: optional list of service names to remove select services).Please confirm this behavioral change is intentional for this PR, as users who previously used
defang compose down service1 service2will now receive an error.src/pkg/cli/client/playground.go (2)
94-100: LGTM - Internal destroy method extraction.The
destroymethod is now private and correctly delegates to the fabric client'sDestroyendpoint.
110-112: LGTM - CdList renamed consistently.The rename from
BootstrapListtoCdListaligns with the interface changes.src/pkg/cli/composeDown.go (1)
23-37: LGTM - InteractiveComposeDown properly delegates.The updated signature correctly passes the fabric client and provider through to
ComposeDown, enabling the deployment history updates in the centralized CD flow.src/pkg/cli/client/byoc/gcp/byoc.go (3)
263-301: LGTM - CdList implementation unchanged aside from rename.The GCP provider's
CdListcorrectly mirrors the AWS implementation pattern with appropriate GCP-specific bucket handling.
329-342: LGTM - CdCommand implementation consistent with AWS.The GCP provider's
CdCommandfollows the same pattern as AWS: converts the command type to string, runs the CD command, and returns the execution ID as the etag.
562-573: LGTM - Comment updated for clarity.The comment correctly explains that
CdCommandreturns the job execution ID as the etag.src/pkg/cli/composeDown_test.go (3)
27-32: LGTM - Mock method correctly implements new interface.The
CdCommandmock method properly delegates toMockCdCommandwith the correct signature.
48-51: LGTM - Test setup uses correct types.The mock setup correctly uses
CdCommandRequestand stores it in the request map for later assertions.
68-76: LGTM - Type assertions use correct naming.The type assertions correctly use
okfor the boolean result (addressing the past review comment abouterrvsoknaming), and properly cast to*client.CdCommandRequest.src/pkg/cli/cd.go (3)
34-40: Verify the error handling behavior for postDown failures.When
postDownfails, the error is logged and a warning is printed, but the function still returns the originaletagas if the operation succeeded. This appears intentional based on the warning message "deployment will proceed anyway", but this mixed success state could be confusing for callers who might expect either full success or full failure.Consider whether callers should be informed that postDown failed through a wrapped error or a different return pattern.
72-86: LGTM!The function correctly orchestrates the CD command with tailing by capturing the baseline time, executing the command, and setting up appropriate tail options.
131-131: No action needed. The stack names returned byprovider.CdList()are always formatted as"project/name"via thePulumiState.String()method, which guarantees the "/" separator is present.SplitProjectStackwill not panic on these inputs.Likely an incorrect or invalid review comment.
Description
defang cd downanddefang cd destroywould not update the deployment history. We can consider deprecating them in favor ofdefang down. What's important is that I'm able to down without having access to the source files for the project.Linked Issues
Additionally, down/destroy commands would also not delete the delegate defang.app domain for the project, something that was added in #1738
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.