Fix canary target group settings copy#194
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Makefile VERSION variable was downgraded from 3.0.0 to 2.2.1. ELBV2 target group creation was reworked to copy configuration and attributes from a source target group via new helper functions and client methods, with an accompanying unit test. Runner.Deploy now conditionally skips summary printing when CompleteCanary is set. ChangesTarget Group Attribute Copy
Canary Summary Print Gating
Makefile Version Bump
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ELBV2Client
participant AWSELBV2API
Caller->>ELBV2Client: CreateTargetGroup(tg, tgName)
ELBV2Client->>ELBV2Client: BuildCreateTargetGroupInput(tg, tgName)
ELBV2Client->>AWSELBV2API: CreateTargetGroup(input)
AWSELBV2API-->>ELBV2Client: new TargetGroupArn
alt source and new ARN present
ELBV2Client->>AWSELBV2API: DescribeTargetGroupAttributes(sourceArn)
AWSELBV2API-->>ELBV2Client: attributes
alt attributes not empty
ELBV2Client->>AWSELBV2API: ModifyTargetGroupAttributes(newArn, attributes)
end
end
ELBV2Client-->>Caller: created target group
Related issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, aws, tests Suggested reviewers: None found in the provided context. 🐰 hop, hop, through the ELBV2 code, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
The changes properly implement comprehensive target group settings replication for canary deployments. The refactored CreateTargetGroup function now copies all health check settings, target type, protocol version, IP address type, matcher, and custom attributes from the original target group to the canary target group, addressing the root cause where these settings previously fell back to AWS defaults. The implementation includes appropriate error handling and comprehensive test coverage.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/aws/elbv2_test.go (1)
50-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the attribute-copy flow.
This test covers the input builder, but the PR’s main regression also depends on
CreateTargetGroupcalling describe/modify attributes after creation. Add a test that verifies source attributes are applied to the new target group ARN.🤖 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 `@pkg/aws/elbv2_test.go` around lines 50 - 125, Add a test that covers the attribute-copy flow in CreateTargetGroup, not just BuildCreateTargetGroupInput. Verify that after the target group is created, CreateTargetGroup uses the returned target group ARN to call the describe/modify attributes path and applies the source attributes to the new group. Use the existing CreateTargetGroup flow and its attribute-handling helpers to locate the code, and assert the new ARN receives the copied attributes.
🤖 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 `@pkg/aws/elbv2.go`:
- Line 141: The create-target-group response is indexed directly in the ELBv2
flow, which can panic if no target groups are returned. Update the target group
creation logic around the create response in elbv2.go to check that
result.TargetGroups has at least one entry before taking the first element, and
return a typed error from the same path instead of assuming success. Keep the
fix localized to the code that assigns newTargetGroup so the canary flow fails
safely when the SDK response is empty.
- Around line 146-156: `CreateTargetGroup` currently returns immediately if
`ModifyTargetGroupAttributes` fails, which can leave the newly created target
group orphaned. Update the error path in `CreateTargetGroup` in
`pkg/aws/elbv2.go` to call `DeleteTargetGroup` on
`newTargetGroup.TargetGroupArn` before returning the error, and make sure any
rollback failure is handled/logged without hiding the original attribute-copy
error. Keep the fix localized around the `DescribeTargetGroupAttributes` and
`ModifyTargetGroupAttributes` flow.
---
Nitpick comments:
In `@pkg/aws/elbv2_test.go`:
- Around line 50-125: Add a test that covers the attribute-copy flow in
CreateTargetGroup, not just BuildCreateTargetGroupInput. Verify that after the
target group is created, CreateTargetGroup uses the returned target group ARN to
call the describe/modify attributes path and applies the source attributes to
the new group. Use the existing CreateTargetGroup flow and its
attribute-handling helpers to locate the code, and assert the new ARN receives
the copied attributes.
🪄 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 Plus
Run ID: 120f1c73-9dd8-4606-a124-05ce385aa746
📒 Files selected for processing (3)
Makefilepkg/aws/elbv2.gopkg/aws/elbv2_test.go
| attributes, err := e.DescribeTargetGroupAttributes(tg.TargetGroupArn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &result.TargetGroups[0], nil | ||
| if len(attributes) == 0 { | ||
| return newTargetGroup, nil | ||
| } | ||
|
|
||
| if err := e.ModifyTargetGroupAttributes(newTargetGroup.TargetGroupArn, attributes); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether a target-group delete helper already exists for rollback.
rg -nP 'DeleteTargetGroup\s*\(|func \(e ELBV2Client\) DeleteTargetGroup' pkg/aws pkg/deployerRepository: DevopsArtFactory/goployer
Length of output: 576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target-group creation flow around the reported lines.
sed -n '110,190p' pkg/aws/elbv2.go
# Inspect the delete helper implementation.
sed -n '210,240p' pkg/aws/elbv2.go
# Inspect the caller to see whether it already cleans up on create/copy failures.
sed -n '780,930p' pkg/deployer/canary.goRepository: DevopsArtFactory/goployer
Length of output: 8481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target-group copy path for cleanup on failures.
rg -n "func \(c \*Canary\) CopyTargetGroups|CopyTargetGroups\(" pkg/deployer/canary.go
sed -n '640,760p' pkg/deployer/canary.go
# Check whether canary target-group names are reused across retries.
rg -n "GenerateCanaryTargetGroupName|canaryVersion" pkg/deployer/canary.goRepository: DevopsArtFactory/goployer
Length of output: 5410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full target-group copy implementation and nearby helpers.
sed -n '428,520p' pkg/deployer/canary.go
# Inspect the call site around the copy/deploy flow for retry/error handling.
sed -n '100,150p' pkg/deployer/canary.goRepository: DevopsArtFactory/goployer
Length of output: 4550
Rollback the new target group if attribute copy fails.
CreateTargetGroup can succeed before the attribute copy step, so returning the error here leaves an orphaned AWS target group behind and makes a retry hit a name conflict. Call DeleteTargetGroup on the newly created ARN before returning.
🤖 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 `@pkg/aws/elbv2.go` around lines 146 - 156, `CreateTargetGroup` currently
returns immediately if `ModifyTargetGroupAttributes` fails, which can leave the
newly created target group orphaned. Update the error path in
`CreateTargetGroup` in `pkg/aws/elbv2.go` to call `DeleteTargetGroup` on
`newTargetGroup.TargetGroupArn` before returning the error, and make sure any
rollback failure is handled/logged without hiding the original attribute-copy
error. Keep the fix localized around the `DescribeTargetGroupAttributes` and
`ModifyTargetGroupAttributes` flow.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/coderabbit review |
Summary
2.2.1Root cause
The canary target group was created with only name, port, protocol, and VPC ID. Health check settings, target type, protocol version, IP address type, matcher, and target group attributes could fall back to AWS defaults instead of matching the original target group.
Validation
/opt/homebrew/bin/go test ./...Summary by CodeRabbit
Bug Fixes
Chores