feat: auto-trigger debugger in CI#2158
Conversation
📝 WalkthroughWalkthroughAdds an Orphan Cleanup + Non-Interactive Debug Agent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🧹 Nitpick comments (1)
src/pkg/agent/tools/cleanup_test.go (1)
85-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case exercising
CleanupOrphanfailure.
mockCleanerProvider.cleanupErris wired into the mock (Line 31) but no table case sets it, so thefailed++branch and the"... failed: %v"/ "N cleaned, N skipped, N failed" reporting inHandleCleanupToolare never covered. A case withcleanupErrset andconfirm: "yes"would close this gap.🧪 Suggested case
+ { + name: "cleanup failure is reported", + provider: &mockCleanerProvider{orphans: twoOrphans, cleanupErr: errors.New("boom")}, + confirm: "yes", + expectContains: []string{"0 cleaned, 0 skipped, 2 failed", "failed: boom"}, + expectCleaned: nil, + },As per path instructions: "Add tests for new behavior and important failure modes: ... missing returns, ... failed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/agent/tools/cleanup_test.go` around lines 85 - 121, Add a table-driven test case in cleanup_test.go that sets mockCleanerProvider.cleanupErr and confirms with "yes" so CleanupOrphan failure is exercised. Use the existing HandleCleanupTool test table and a case alongside the current "confirm yes cleans every orphan" scenario, asserting the failed branch is reported with the "N cleaned, N skipped, N failed" summary and the "... failed: %v" message while still checking the cleaned/skipped counts.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/agent/tools/cleanup.go`:
- Around line 32-36: The cleanup flow is discarding the error from
loader.ProjectWorkingDir(ctx), which can hide real filesystem or loader failures
before stacks.NewManager is called. Update the cleanup logic to handle that
error explicitly in the same path that sets workingDir, and return a wrapped
error instead of continuing with an empty directory unless there is a clearly
documented reason it is safe to ignore. Keep the fix localized around
ProjectWorkingDir and the subsequent stacks.NewManager call so the root cause is
preserved.
In `@src/pkg/cli/client/byoc/aws/cleanup.go`:
- Around line 70-102: The ALB/RDS discovery in cleanup uses broad prefix
matching that can catch resources from the wrong project, so tighten the
matching to the full generated name shape instead of a bare prefix. Update the
lookup logic around b.resourceBaseName, FindLoadBalancersByPrefix, and
FindDBInstancesByPrefix so it validates the expected suffix/boundary for the
stack name before calling add, and only offers resources whose names exactly
match the intended defang-generated pattern.
In `@src/pkg/cli/client/byoc/aws/domain_test.go`:
- Around line 40-43: The Route53 mock mutation method is currently a silent
no-op, which can hide unexpected calls from prepareDomainDelegation. Update
r53Mock.ChangeResourceRecordSets in the domain test mock to fail fast by
panicking or otherwise triggering an explicit test failure instead of returning
nil, nil, so any unexpected record mutation is surfaced immediately during
tests.
In `@src/pkg/clouds/aws/rds_test.go`:
- Around line 26-38: Add a paginated discovery test for FindDBInstancesByPrefix
to cover the out.Marker loop in rds.go, since the current single-page mockRDS
case won’t catch broken page traversal. Extend the rds_test.go coverage with a
two-page mock similar to the ELB pagination test, using mockRDS and
FindDBInstancesByPrefix to verify results are accumulated across pages and the
marker handoff is handled correctly.
In `@src/pkg/clouds/aws/route53_cleanup_test.go`:
- Around line 33-37: The delete test currently verifies record deletion but not
the new hosted-zone ID trimming behavior. Update
fakeR53.ChangeResourceRecordSets to capture the incoming HostedZoneId from the
Route53 request, then assert in TestDeleteResourceRecordSet that the value
passed is the trimmed form "Z123" rather than the original "/hostedzone/..."
string. Use the existing fakeR53 and TestDeleteResourceRecordSet symbols to keep
the check focused on the new behavior.
In `@src/pkg/clouds/aws/route53.go`:
- Around line 191-216: Wrap the raw Route53 SDK errors in the helper functions
that list and delete records so callers get both AWS detail and operation
context. In the record-listing logic that calls ListResourceRecordSets, return a
wrapped error that includes the zone identifier and that listing failed; in
DeleteResourceRecordSet, wrap the ChangeResourceRecordSets error with the
trimmed hosted zone ID and record identity (name/type/set info from the
ResourceRecordSet). Use the existing function names ListResourceRecordSets and
DeleteResourceRecordSet to locate the changes and preserve the original error
with %w.
---
Nitpick comments:
In `@src/pkg/agent/tools/cleanup_test.go`:
- Around line 85-121: Add a table-driven test case in cleanup_test.go that sets
mockCleanerProvider.cleanupErr and confirms with "yes" so CleanupOrphan failure
is exercised. Use the existing HandleCleanupTool test table and a case alongside
the current "confirm yes cleans every orphan" scenario, asserting the failed
branch is reported with the "N cleaned, N skipped, N failed" summary and the
"... failed: %v" message while still checking the cleaned/skipped counts.
🪄 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: 3c9f2a0f-8f5a-4601-b506-aad846df4855
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
src/cmd/cli/command/compose.gosrc/cmd/cli/command/debug.gosrc/cmd/cli/command/session.gosrc/go.modsrc/pkg/agent/agent.gosrc/pkg/agent/tools/cleanup.gosrc/pkg/agent/tools/cleanup_test.gosrc/pkg/agent/tools/tools.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/cleanup.gosrc/pkg/cli/client/byoc/aws/domain_test.gosrc/pkg/cli/client/cleanup.gosrc/pkg/cli/stacks.gosrc/pkg/cli/stacks_test.gosrc/pkg/clouds/aws/ecr.gosrc/pkg/clouds/aws/ecr_test.gosrc/pkg/clouds/aws/elbv2.gosrc/pkg/clouds/aws/elbv2_test.gosrc/pkg/clouds/aws/rds.gosrc/pkg/clouds/aws/rds_test.gosrc/pkg/clouds/aws/route53.gosrc/pkg/clouds/aws/route53_cleanup_test.gosrc/pkg/debug/debug.gosrc/pkg/debug/debug_test.gosrc/pkg/elicitations/elicitations.gosrc/pkg/utils.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/pkg/agent/tools/cleanup_test.go (1)
85-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case exercising
CleanupOrphanfailure.
mockCleanerProvider.cleanupErris wired into the mock (Line 31) but no table case sets it, so thefailed++branch and the"... failed: %v"/ "N cleaned, N skipped, N failed" reporting inHandleCleanupToolare never covered. A case withcleanupErrset andconfirm: "yes"would close this gap.🧪 Suggested case
+ { + name: "cleanup failure is reported", + provider: &mockCleanerProvider{orphans: twoOrphans, cleanupErr: errors.New("boom")}, + confirm: "yes", + expectContains: []string{"0 cleaned, 0 skipped, 2 failed", "failed: boom"}, + expectCleaned: nil, + },As per path instructions: "Add tests for new behavior and important failure modes: ... missing returns, ... failed".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/agent/tools/cleanup_test.go` around lines 85 - 121, Add a table-driven test case in cleanup_test.go that sets mockCleanerProvider.cleanupErr and confirms with "yes" so CleanupOrphan failure is exercised. Use the existing HandleCleanupTool test table and a case alongside the current "confirm yes cleans every orphan" scenario, asserting the failed branch is reported with the "N cleaned, N skipped, N failed" summary and the "... failed: %v" message while still checking the cleaned/skipped counts.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pkg/agent/tools/cleanup.go`:
- Around line 32-36: The cleanup flow is discarding the error from
loader.ProjectWorkingDir(ctx), which can hide real filesystem or loader failures
before stacks.NewManager is called. Update the cleanup logic to handle that
error explicitly in the same path that sets workingDir, and return a wrapped
error instead of continuing with an empty directory unless there is a clearly
documented reason it is safe to ignore. Keep the fix localized around
ProjectWorkingDir and the subsequent stacks.NewManager call so the root cause is
preserved.
In `@src/pkg/cli/client/byoc/aws/cleanup.go`:
- Around line 70-102: The ALB/RDS discovery in cleanup uses broad prefix
matching that can catch resources from the wrong project, so tighten the
matching to the full generated name shape instead of a bare prefix. Update the
lookup logic around b.resourceBaseName, FindLoadBalancersByPrefix, and
FindDBInstancesByPrefix so it validates the expected suffix/boundary for the
stack name before calling add, and only offers resources whose names exactly
match the intended defang-generated pattern.
In `@src/pkg/cli/client/byoc/aws/domain_test.go`:
- Around line 40-43: The Route53 mock mutation method is currently a silent
no-op, which can hide unexpected calls from prepareDomainDelegation. Update
r53Mock.ChangeResourceRecordSets in the domain test mock to fail fast by
panicking or otherwise triggering an explicit test failure instead of returning
nil, nil, so any unexpected record mutation is surfaced immediately during
tests.
In `@src/pkg/clouds/aws/rds_test.go`:
- Around line 26-38: Add a paginated discovery test for FindDBInstancesByPrefix
to cover the out.Marker loop in rds.go, since the current single-page mockRDS
case won’t catch broken page traversal. Extend the rds_test.go coverage with a
two-page mock similar to the ELB pagination test, using mockRDS and
FindDBInstancesByPrefix to verify results are accumulated across pages and the
marker handoff is handled correctly.
In `@src/pkg/clouds/aws/route53_cleanup_test.go`:
- Around line 33-37: The delete test currently verifies record deletion but not
the new hosted-zone ID trimming behavior. Update
fakeR53.ChangeResourceRecordSets to capture the incoming HostedZoneId from the
Route53 request, then assert in TestDeleteResourceRecordSet that the value
passed is the trimmed form "Z123" rather than the original "/hostedzone/..."
string. Use the existing fakeR53 and TestDeleteResourceRecordSet symbols to keep
the check focused on the new behavior.
In `@src/pkg/clouds/aws/route53.go`:
- Around line 191-216: Wrap the raw Route53 SDK errors in the helper functions
that list and delete records so callers get both AWS detail and operation
context. In the record-listing logic that calls ListResourceRecordSets, return a
wrapped error that includes the zone identifier and that listing failed; in
DeleteResourceRecordSet, wrap the ChangeResourceRecordSets error with the
trimmed hosted zone ID and record identity (name/type/set info from the
ResourceRecordSet). Use the existing function names ListResourceRecordSets and
DeleteResourceRecordSet to locate the changes and preserve the original error
with %w.
---
Nitpick comments:
In `@src/pkg/agent/tools/cleanup_test.go`:
- Around line 85-121: Add a table-driven test case in cleanup_test.go that sets
mockCleanerProvider.cleanupErr and confirms with "yes" so CleanupOrphan failure
is exercised. Use the existing HandleCleanupTool test table and a case alongside
the current "confirm yes cleans every orphan" scenario, asserting the failed
branch is reported with the "N cleaned, N skipped, N failed" summary and the
"... failed: %v" message while still checking the cleaned/skipped counts.
🪄 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: 3c9f2a0f-8f5a-4601-b506-aad846df4855
⛔ Files ignored due to path filters (1)
src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
src/cmd/cli/command/compose.gosrc/cmd/cli/command/debug.gosrc/cmd/cli/command/session.gosrc/go.modsrc/pkg/agent/agent.gosrc/pkg/agent/tools/cleanup.gosrc/pkg/agent/tools/cleanup_test.gosrc/pkg/agent/tools/tools.gosrc/pkg/cli/client/byoc/aws/byoc.gosrc/pkg/cli/client/byoc/aws/cleanup.gosrc/pkg/cli/client/byoc/aws/domain_test.gosrc/pkg/cli/client/cleanup.gosrc/pkg/cli/stacks.gosrc/pkg/cli/stacks_test.gosrc/pkg/clouds/aws/ecr.gosrc/pkg/clouds/aws/ecr_test.gosrc/pkg/clouds/aws/elbv2.gosrc/pkg/clouds/aws/elbv2_test.gosrc/pkg/clouds/aws/rds.gosrc/pkg/clouds/aws/rds_test.gosrc/pkg/clouds/aws/route53.gosrc/pkg/clouds/aws/route53_cleanup_test.gosrc/pkg/debug/debug.gosrc/pkg/debug/debug_test.gosrc/pkg/elicitations/elicitations.gosrc/pkg/utils.go
🛑 Comments failed to post (6)
src/pkg/agent/tools/cleanup.go (1)
32-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Don't silently swallow the
ProjectWorkingDirerror.The error from
loader.ProjectWorkingDir(ctx)is discarded, so a real filesystem/loader failure silently yields an emptyworkingDirthat is then passed intostacks.NewManager, masking the root cause. Either propagate it or document why it's safe to ignore here.🛠️ Proposed fix
- workingDir, _ := loader.ProjectWorkingDir(ctx) + workingDir, err := loader.ProjectWorkingDir(ctx) + if err != nil { + return "", fmt.Errorf("failed to determine project working directory: %w", err) + }As per coding guidelines: "Do not silently swallow real errors; return wrapped errors for filesystem, parsing, network, and provider failures."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.workingDir, err := loader.ProjectWorkingDir(ctx) if err != nil { return "", fmt.Errorf("failed to determine project working directory: %w", err) } sm, err := stacks.NewManager(fabric, workingDir, params.ProjectName, ec) if err != nil { return "", fmt.Errorf("failed to create stack manager: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/agent/tools/cleanup.go` around lines 32 - 36, The cleanup flow is discarding the error from loader.ProjectWorkingDir(ctx), which can hide real filesystem or loader failures before stacks.NewManager is called. Update the cleanup logic to handle that error explicitly in the same path that sets workingDir, and return a wrapped error instead of continuing with an empty directory unless there is a clearly documented reason it is safe to ignore. Keep the fix localized around ProjectWorkingDir and the subsequent stacks.NewManager call so the root cause is preserved.Source: Coding guidelines
src/pkg/cli/client/byoc/aws/cleanup.go (1)
70-102: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect how CD names ALBs/RDS to confirm a separator boundary after the base. rg -nP -C3 'resourceBaseName|LoadBalancerName|DBInstanceIdentifier' src/pkg/clouds/aws src/pkg/cli/client/byoc/aws rg -nP -C3 'alb_logs|maxALBNameLen|Prefix' src/pkg/clouds/awsRepository: DefangLabs/defang
Length of output: 28162
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant files and inspect the naming/selection logic. ast-grep outline src/pkg/cli/client/byoc/aws/cleanup.go --view expanded ast-grep outline src/pkg/clouds/aws/elbv2.go --view expanded ast-grep outline src/pkg/clouds/aws/rds.go --view expanded # Read the cleanup logic and the matching helpers in context. sed -n '1,180p' src/pkg/cli/client/byoc/aws/cleanup.go printf '\n--- elbv2 ---\n' sed -n '1,120p' src/pkg/clouds/aws/elbv2.go printf '\n--- rds ---\n' sed -n '1,120p' src/pkg/clouds/aws/rds.go # Look for any tests or code that establish the expected name shape. rg -n -C3 'LoadBalancerName|DBInstanceIdentifier|resourceBaseName|Prefix' src/pkg/clouds/aws src/pkg/cli/client/byoc/awsRepository: DefangLabs/defang
Length of output: 50373
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the resource naming logic that produces ALB/RDS identifiers. rg -n -C3 'alb_logs|LoadBalancerName|DBInstanceIdentifier|resourceBaseName|trunc|31' src/pkg/clouds/aws src/pkg/cli/client/byoc/aws # Read the specific ALB naming source if present. fd -a 'alb_logs.go|*alb*logs*.go' src/pkg/clouds/aws src/pkg/cli/client/byoc/awsRepository: DefangLabs/defang
Length of output: 50374
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the small slices that define ALB/RDS naming and the cleanup matcher. sed -n '1,220p' src/pkg/cli/client/byoc/aws/alb_logs.go printf '\n--- cleanup ---\n' sed -n '1,130p' src/pkg/cli/client/byoc/aws/cleanup.go printf '\n--- cloudformation template references ---\n' rg -n -C2 'LoadBalancerName|DBInstanceIdentifier|DeletionProtection|resourceBaseName|CdTaskPrefix|Prefix' src/pkg/clouds/aws src/pkg/cli/client/byoc/aws | head -n 120Repository: DefangLabs/defang
Length of output: 20667
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the exact ALB/RDS name construction and any fixed suffixes. rg -n -C4 'ALB|load balancer|DBInstanceIdentifier|db|suffix|hash|name.*31|resourceBaseName' src/pkg/clouds/aws src/pkg/cli/client/byoc/aws # Read the remainder of the ALB log helper if naming is defined there. sed -n '220,420p' src/pkg/cli/client/byoc/aws/alb_logs.go # Read any cloudformation/template code that sets the ALB/RDS names. fd -a 'template.go|*.yaml|*.yml' src/pkg/clouds/aws src/pkg/cli/client/byoc/aws | head -n 50Repository: DefangLabs/defang
Length of output: 50374
Anchor ALB/RDS discovery to the full generated name shape.
FindLoadBalancersByPrefixuses a 31-char truncated ALB prefix, andFindDBInstancesByPrefixuses the raw base prefix, so any other resource with the same leading characters can be offered for cleanup. That can disable deletion protection on the wrong project/stack. Match the exact expected suffix/boundary instead of a bare prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/aws/cleanup.go` around lines 70 - 102, The ALB/RDS discovery in cleanup uses broad prefix matching that can catch resources from the wrong project, so tighten the matching to the full generated name shape instead of a bare prefix. Update the lookup logic around b.resourceBaseName, FindLoadBalancersByPrefix, and FindDBInstancesByPrefix so it validates the expected suffix/boundary for the stack name before calling add, and only offers resources whose names exactly match the intended defang-generated pattern.src/pkg/cli/client/byoc/aws/domain_test.go (1)
40-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the unimplemented Route53 mutation mock fail fast.
Returning
nil, nilhere silently accepts unexpected record mutations, so these tests could mask a regression ifprepareDomainDelegationever starts callingChangeResourceRecordSets. Panic or return an explicit test failure instead of succeeding. Based on learnings, "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."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/cli/client/byoc/aws/domain_test.go` around lines 40 - 43, The Route53 mock mutation method is currently a silent no-op, which can hide unexpected calls from prepareDomainDelegation. Update r53Mock.ChangeResourceRecordSets in the domain test mock to fail fast by panicking or otherwise triggering an explicit test failure instead of returning nil, nil, so any unexpected record mutation is surfaced immediately during tests.Source: Learnings
src/pkg/clouds/aws/rds_test.go (1)
26-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a paginated RDS discovery case.
FindDBInstancesByPrefixinsrc/pkg/clouds/aws/rds.goloops onout.Marker, but this test double never returns a second page, so a broken marker handoff would still pass here. Add a two-page case like the ELB test to lock down the new traversal behavior. As per coding guidelines, "Add tests for new behavior and important failure modes" and "Test behavior boundaries, not implementation mechanics."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/clouds/aws/rds_test.go` around lines 26 - 38, Add a paginated discovery test for FindDBInstancesByPrefix to cover the out.Marker loop in rds.go, since the current single-page mockRDS case won’t catch broken page traversal. Extend the rds_test.go coverage with a two-page mock similar to the ELB pagination test, using mockRDS and FindDBInstancesByPrefix to verify results are accumulated across pages and the marker handoff is handled correctly.Source: Coding guidelines
src/pkg/clouds/aws/route53_cleanup_test.go (1)
33-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the trimmed hosted-zone ID in the delete test.
The new behavior here is not just “delete this record” but “delete it with
/hostedzone/stripped first.”fakeR53.ChangeResourceRecordSetsdoes not captureHostedZoneId, soTestDeleteResourceRecordSetwould still pass if that trim regressed. Record the incoming zone ID in the fake and assert it is"Z123". As per coding guidelines, "Add tests for new behavior and important failure modes" and "Test behavior boundaries, not implementation mechanics."Also applies to: 58-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/clouds/aws/route53_cleanup_test.go` around lines 33 - 37, The delete test currently verifies record deletion but not the new hosted-zone ID trimming behavior. Update fakeR53.ChangeResourceRecordSets to capture the incoming HostedZoneId from the Route53 request, then assert in TestDeleteResourceRecordSet that the value passed is the trimmed form "Z123" rather than the original "/hostedzone/..." string. Use the existing fakeR53 and TestDeleteResourceRecordSet symbols to keep the check focused on the new behavior.Source: Coding guidelines
src/pkg/clouds/aws/route53.go (1)
191-216: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap Route53 SDK errors with operation context.
Both helpers currently return raw SDK errors. When cleanup fails, that loses which zone or record operation failed. Wrap with
%wand include the zone ID / record identity so callers keep AWS detail plus actionable context. As per coding guidelines, "Preserve cloud-provider error detail while wrapping errors with operation context in cloud SDK wrappers."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pkg/clouds/aws/route53.go` around lines 191 - 216, Wrap the raw Route53 SDK errors in the helper functions that list and delete records so callers get both AWS detail and operation context. In the record-listing logic that calls ListResourceRecordSets, return a wrapped error that includes the zone identifier and that listing failed; in DeleteResourceRecordSet, wrap the ChangeResourceRecordSets error with the trimmed hosted zone ID and record identity (name/type/set info from the ResourceRecordSet). Use the existing function names ListResourceRecordSets and DeleteResourceRecordSet to locate the changes and preserve the original error with %w.Source: Coding guidelines
lionello
changed the title
1 participant
Description
Trigger the AI debugger on failure in CI.
Linked Issues
Checklist
Summary by CodeRabbit
New Features
Bug Fixes