Conversation
📝 WalkthroughWalkthroughList() merge behavior changed to a remote-wins strategy: local stacks are inserted first, then remote stacks overwrite entries with matching names. Previous post-merge alignment of fields like DeployedAt and Default was removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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" Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/pkg/stacks/manager_test.go`:
- Line 419: The assertion message for assert.Equal(t, "us-east-1",
sharedStack.Region, ...) is misleading — it mentions “local region” while the
test expects the remote region to win; update the message to reflect remote-wins
behavior (e.g., "Expected shared stack to use remote region us-east-1" or
similar) so it matches the expected value and clarifies the test intent; locate
the assertion referencing sharedStack.Region in manager_test.go and replace the
message string accordingly.
In `@src/pkg/stacks/manager.go`:
- Around line 65-73: The merge currently overwrites stackMap entries with later
remoteStacks entries causing older duplicates to replace newer ones; update the
remote merge so the most recent remote wins by either iterating remoteStacks in
reverse or comparing timestamps: in the loop that processes remoteStacks (which
populates stackMap from remote into ListItem), check existing :=
stackMap[remote.Name]; if existing exists and existing.DeployedAt is after
remote.DeployedAt, skip assignment; otherwise set stackMap[remote.Name] = remote
so the entry with the latest DeployedAt (newest) is kept for List().
lionello
left a comment
There was a problem hiding this comment.
I see how this fixes the issue, but can't say for sure this is the best solution. What to do if local stack (used when I do up) differs from remote?
Perhaps we should add a column to show whether the info was local or remote.
I agree, but I think this PR is a good step forward for the moment. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Description
Prefer remote to local stacks when listing
Linked Issues
Fixes https://github.com/DefangLabs/defang-mvp/issues/2655
Checklist
Summary by CodeRabbit
Bug Fixes
Tests