improve cd ls output and cd teardown error handling#1914
improve cd ls output and cd teardown error handling#1914jordanstephens merged 11 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughCD listing and teardown now propagate a structured Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cd teardown)
participant Provider as Provider (CdList / TearDownCD)
participant Storage as Remote storage (S3 / buckets)
participant Printer as Terminal / Table renderer
CLI->>Provider: CdList(ctx, allRegions=false)
Provider->>Storage: list Pulumi state files (multi-region)
Storage-->>Provider: stream state.PulumiState / state.Info
Provider-->>CLI: iter.Seq[state.Info]
CLI->>Printer: collect, sort, render table of stacks
alt stacks exist && not --force
CLI->>Printer: print per-stack commands + header
CLI->>CLI: return ErrExistingStacks
CLI->>Printer: print defang hint
else
CLI->>Provider: TearDownCD(ctx)
Provider->>Storage: perform teardown operations
Provider-->>CLI: teardown result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
* only print suggestion to use `--force` if there are existing projects * separate hint from error * print the names of the existing projects and stacks
jordanstephens
force-pushed
the
jordan/confusing-cd-teardown-error
branch
from
ffd5fc1 to
5331e5d
Compare
![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/cmd/cli/command/cd.go`:
- Around line 111-115: The hint text used when TearDownCD returns
cli.ErrExistingProjects is incorrect—update the message passed to
printDefangHint in the cdTearDownCmd teardown flow so it references the teardown
command and flag correctly ("defang cd teardown --force") instead of "defang cd
destroy --force"; locate the error-handling block where cli.TearDownCD(...) is
called and replace the hint string passed to printDefangHint accordingly.
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/pkg/cli/client/byoc/baseclient.go (1)
98-115:⚠️ Potential issue | 🟠 MajorDeduplicate project names before deciding “multiple projects.”
CdList can yield multiple stacks for the same project (different stacks/regions). The current logic counts duplicates and can incorrectly raise ErrMultipleProjects even when only one unique project exists.
🔧 Proposed fix
- var projectNames []string - for stack := range stacks { - projectNames = append(projectNames, stack.Project) - } + projectNameSet := make(map[string]struct{}) + var projectNames []string + for stack := range stacks { + if _, seen := projectNameSet[stack.Project]; seen { + continue + } + projectNameSet[stack.Project] = struct{}{} + projectNames = append(projectNames, stack.Project) + }
🤖 Fix all issues with AI agents
In `@src/pkg/cli/teardown_cd.go`:
- Around line 22-30: TearDownCD currently aborts on any provider.CdList error;
update the logic so "CD already deleted" / "no bucket found" cases are treated
as non-fatal by either normalizing provider.CdList to return an empty channel
and nil error for not-found, or by detecting provider-specific not-found errors
in TearDownCD and treating them as a no-op (i.e., proceed with an empty list).
Ensure the iteration in TearDownCD that does "for stackInfo := range list" is
safe by guaranteeing list is non-nil (create an empty channel or slice) or by
guarding the loop when list is nil; update provider implementations (CdList) or
add an error normalization helper used by TearDownCD to identify
annotated/not-found errors (e.g., AWS AnnotateAwsError variants, GCP "no defang
cd bucket found") and convert them into an empty result rather than returning an
error.
🧹 Nitpick comments (3)
src/pkg/cli/cd.go (1)
147-163: Consider skipping the table when no stacks exist.When
len(stacks) == 0, the code prints "No projects found in ..." and then still renders an empty table. This might produce confusing output with table headers but no data rows.♻️ Optional: Skip table when empty
if len(stacks) == 0 { accountInfo, err := provider.AccountInfo(ctx) if err != nil { return err } if allRegions { accountInfo.Region = "" } term.Printf("No projects found in %v\n", accountInfo) + return nil } return term.Table(stacks, "Project", "Stack", "Workspace", "Region")src/pkg/cli/client/byoc/aws/list_test.go (1)
70-72: Error message format inconsistency.Line 70 uses
stack.String()for comparison, but line 71 printsstackdirectly in the error message. For consistency, consider usingstack.String()in both places.Note: This also depends on
StackInfohaving aString()method, which appears to be missing (see review comment onstate.go).♻️ Proposed fix for consistency
for stack := range stacks { if stack.String() != expectedStacks[count] { - t.Errorf("expected stack %q, got %q", expectedStacks[count], stack) + t.Errorf("expected stack %q, got %q", expectedStacks[count], stack.String()) } count++ }src/pkg/cli/client/byoc/gcp/byoc.go (1)
797-799: Consider removing the commented-out warn line.Now that teardown messaging is handled at the CLI layer, keeping commented code here adds noise; either delete it or re-enable with a clear rationale when TearDownCD is implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/cmd/cli/command/cd.go`:
- Around line 111-115: The code checks for a non-existent error constant
cli.ErrExistingProjects causing build failure; update the error comparison in
the cd teardown path to use the correct constant cli.ErrExistingStacks (used by
TearDownCD in teardown_cd.go) so the condition compiles and the defang hint
still triggers for existing stacks; locate the check in the cd command (the
TearDownCD error handling block) and replace cli.ErrExistingProjects with
cli.ErrExistingStacks.
de526cd to
bbc4569
Compare
lionello
left a comment
lionello
left a comment
There was a problem hiding this comment.
We can clean up some superfluous types.
|
|
||
| func (b *ByocGcp) TearDownCD(ctx context.Context) error { | ||
| // term.Warn("Deleting Defang CD; currently existing stacks or configs will not be deleted, but they will be orphaned and they will need to be cleaned up manually") | ||
| // FIXME: implement |
There was a problem hiding this comment.
It's possible there's really nothing to do here. @edwardrf thoughts?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/pkg/cli/client/byoc/baseclient.go`:
- Around line 105-107: The loop building projectNames from stacks currently
appends duplicates (for stack.Project) which can cause a false
ErrMultipleProjects; replace the naive append with logic that deduplicates
(e.g., use a map[string]struct{} to collect unique stack.Project values), then
convert the keys to a slice, sort it, and run the existing length checks that
raise ErrMultipleProjects/ErrNoProject against this deduped, sorted slice (refer
to the stacks range loop and projectNames variable and the ErrMultipleProjects
checks to locate where to change).
🧹 Nitpick comments (1)
src/pkg/cli/client/byoc/aws/list.go (1)
86-86: Variable shadows imported package name.The variable
stateshadows the importedstatepackage. While this works due to Go's evaluation order (RHS before LHS declaration), it's confusing and error-prone for future maintenance.♻️ Suggested refactor to avoid shadowing
- state, err := state.ParsePulumiStateFile(ctx, s3Obj{obj}, bucketName, func(ctx context.Context, bucket, path string) ([]byte, error) { + st, err := state.ParsePulumiStateFile(ctx, s3Obj{obj}, bucketName, func(ctx context.Context, bucket, path string) ([]byte, error) { getObjectOutput, err := s3client.GetObject(ctx, &s3.GetObjectInput{ Bucket: &bucket, Key: &path, }) if err != nil { return nil, err } return io.ReadAll(getObjectOutput.Body) }) if err != nil { term.Debugf("Skipping %q in bucket %s: %v", *obj.Key, bucketName, AnnotateAwsError(err)) continue } - if state != nil { - if !yield(*state) { + if st != nil { + if !yield(*st) { break } }
| for stack := range stacks { | ||
| projectNames = append(projectNames, stack.Project) | ||
| } |
There was a problem hiding this comment.
Deduplicate project names to avoid false “multiple projects” errors.
If multiple stacks exist for the same project, the current logic will append duplicates and trigger ErrMultipleProjects incorrectly. Consider de-duping (and sorting) before the length checks.
🔧 Suggested fix
import (
"context"
"errors"
"fmt"
"iter"
+ "sort"
"strings"
@@
var projectNames []string
- for stack := range stacks {
- projectNames = append(projectNames, stack.Project)
- }
+ projectSet := map[string]struct{}{}
+ for stack := range stacks {
+ projectSet[stack.Project] = struct{}{}
+ }
+ projectNames = make([]string, 0, len(projectSet))
+ for name := range projectSet {
+ projectNames = append(projectNames, name)
+ }
+ sort.Strings(projectNames)🤖 Prompt for AI Agents
In `@src/pkg/cli/client/byoc/baseclient.go` around lines 105 - 107, The loop
building projectNames from stacks currently appends duplicates (for
stack.Project) which can cause a false ErrMultipleProjects; replace the naive
append with logic that deduplicates (e.g., use a map[string]struct{} to collect
unique stack.Project values), then convert the keys to a slice, sort it, and run
the existing length checks that raise ErrMultipleProjects/ErrNoProject against
this deduped, sorted slice (refer to the stacks range loop and projectNames
variable and the ErrMultipleProjects checks to locate where to change).
There was a problem hiding this comment.
This is true, but unrelated to our changes here. I'll handle separately.
There was a problem hiding this comment.
@jordanstephens Understood! Since you'll be handling this separately, would you like me to create a GitHub issue to track the deduplication fix?
🧠 Learnings used
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.
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/stream.go:497-512
Timestamp: 2026-01-09T20:31:23.614Z
Learning: In src/pkg/cli/client/byoc/gcp/stream.go, the getReadyServicesCompletedResps helper function intentionally uses variable shadowing. The loop variable `status` from `readyServices` map represents individual service statuses, while the function parameter (to be renamed `cdStatus`) represents only the CD service (defangCD) completion status. Each ready service should retain its original status from the map.
There was a problem hiding this comment.
@jordanstephens Thank you for creating issue #1918! I appreciate you tracking this separately.
🧠 Learnings used
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.
Learnt from: edwardrf
Repo: DefangLabs/defang PR: 1747
File: src/pkg/cli/client/byoc/gcp/stream.go:497-512
Timestamp: 2026-01-09T20:31:23.614Z
Learning: In src/pkg/cli/client/byoc/gcp/stream.go, the getReadyServicesCompletedResps helper function intentionally uses variable shadowing. The loop variable `status` from `readyServices` map represents individual service statuses, while the function parameter (to be renamed `cdStatus`) represents only the CD service (defangCD) completion status. Each ready service should retain its original status from the map.
2 participants
Description
--forceif there are existing projectsPrevious
cd lsoutput:New
cd lsoutput:(
--jsonmode is now supported as well):Previous
cd teardownerror output:New
cd teardownerror output:Linked Issues
Checklist
Summary by CodeRabbit
Bug Fixes
Improvements