fix(release): resolve authoritative product type via DescribeEntity#1
fix(release): resolve authoritative product type via DescribeEntity#1cristim wants to merge 2 commits into
Conversation
findProduct returned the first product type whose ListEntities filter matched by name. A single entity can surface under more than one type filter (a ContainerProduct also appears under the ServerProduct filter), so EBS Optimizer was labelled ServerProduct and StartChangeSet was called with ServerProduct@1.0 + CreateVersion, which AWS rejects with a ValidationException. Resolve the real type from DescribeEntity's EntityType so the correct entity type and change type (ContainerProduct@1.0 + AddDeliveryOptions) are used. Mock made nil-safe and a regression test added for the dual-match case.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughfindProduct now queries DescribeEntity to determine an authoritative EntityType (stripping any ChangesProduct Type Resolution with DescribeEntity
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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.
🧹 Nitpick comments (1)
product.go (1)
293-296: ⚡ Quick winDescribeEntity errors are silently swallowed.
When
describeEntityTypereturns a non-nil error (e.g. throttling or a transient API failure), thede == nilguard fails andfindProductfalls back to the non-authoritative list-filter typept. This is the very condition the PR fixes, so a transient error can quietly re-surface the wrong-typeValidationExceptiondownstream inStartChangeSet. Consider at least logging the error so the fallback is observable.This is no worse than the prior behavior, but logging makes the degraded path diagnosable.
♻️ Surface the fallback
- if actual, de := describeEntityType(svc, *eid); de == nil && actual != "" { - return *eid, actual, nil - } + actual, de := describeEntityType(svc, *eid) + if de == nil && actual != "" { + return *eid, actual, nil + } + if de != nil { + fmt.Printf("warning: DescribeEntity failed for %s, falling back to list-filter type %s: %v\n", *eid, pt, de) + } return *eid, pt, nil🤖 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 `@product.go` around lines 293 - 296, When describeEntityType(svc, *eid) returns a non-nil error inside findProduct, the error is currently ignored and the code falls back to the non-authoritative pt; modify findProduct to log the returned error before falling back so the degraded path is observable. Specifically, in the block that calls describeEntityType (the lines using variables eid, actual, de), add a processLogger (or the package's logger) call that records the error and relevant context (eid and pt) when de != nil, then continue to return *eid, pt, nil as before; do not change the existing return behavior used by StartChangeSet. Ensure the logger message clearly states that describeEntityType failed and the fallback to pt is being used.
🤖 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.
Nitpick comments:
In `@product.go`:
- Around line 293-296: When describeEntityType(svc, *eid) returns a non-nil
error inside findProduct, the error is currently ignored and the code falls back
to the non-authoritative pt; modify findProduct to log the returned error before
falling back so the degraded path is observable. Specifically, in the block that
calls describeEntityType (the lines using variables eid, actual, de), add a
processLogger (or the package's logger) call that records the error and relevant
context (eid and pt) when de != nil, then continue to return *eid, pt, nil as
before; do not change the existing return behavior used by StartChangeSet.
Ensure the logger message clearly states that describeEntityType failed and the
fallback to pt is being used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d4fef2-ea6c-4a9e-a10b-a51310dacc69
📒 Files selected for processing (3)
mock_test.goproduct.goproduct_test.go
Surface DescribeEntity errors instead of silently falling back to the non-authoritative list-filter type, so the degraded path is diagnosable. Addresses CodeRabbit review on PR #1.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Problem
release-ing a new version of a container product fails with:Root cause
findProductreturns the first product type whoseListEntitiesfilter matches theproduct name. A single entity can surface under more than one type filter: a
ContainerProductalso appears under theServerProductfilter. BecauseServerProductis checked first inallProductTypes, such a product is mislabeledServerProduct, andStartChangeSetis then called withServerProduct@1.0+CreateVersion, which AWS rejects.The
ListEntitiesfilter type is not authoritative;DescribeEntityis.Fix
Resolve the real product type from
DescribeEntity'sEntityType(e.g.
ContainerProduct@1.0, with the version suffix stripped) once an entity isfound, so the correct entity type and change type are used
(
ContainerProduct@1.0+AddDeliveryOptionsfor container products).findProductnow consultsDescribeEntityfor the authoritative type, fallingback to the matched list-filter type if the lookup yields nothing.
describeEntityTypehelper.DescribeEntitymade nil-safe so existing tests that do not exercise itkeep working.
ServerProductfilterfirst, but
DescribeEntityreportsContainerProduct@1.0-> resolves toContainerProduct).Testing
go vet ./...,go build,go test ./...all pass.is now created as
ContainerProductand accepted by AWS (previously rejected).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests