feat(aws): rich self-describing reservation names matching Azure #686 (closes #687)#689
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 2 minutes and 48 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a shared BuildReservationName utility that composes self-describing AWS reservation identifiers within a 60-character cap, with normalization, timestamp and random suffix, optional-tail dropping and SKU truncation. Rolls the utility out to ElastiCache, MemoryDB, OpenSearch, RDS, and Redshift (plus tests and expanded Redshift tags). ChangesReservation Name Builder and Service Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
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)
Comment |
cristim
added
triaged
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/common/reservation_name_test.go`:
- Around line 100-120: The test
TestBuildReservationName_LengthFit_DropsPaymentLast needs an assertion that the
payment segment was actually dropped; update the test that calls
BuildReservationName(ReservationNameFields{... Payment: "all-upfront",
...}.WithRandSource(...), ...) to include assert.NotContains(t, got,
"all-upfront") (and optionally assert.NotContains(t, got, "allup") if shortened
forms are produced) so the test verifies payment is removed in this
length-regime alongside the existing timestamp and rand checks; keep the other
assertions (length, service/region, count, term) unchanged.
In `@pkg/common/reservation_name.go`:
- Line 98: The timestamp formatting currently uses f.Now directly which can be
the zero time and produce "00010101T000000"; update the code that builds the
timestamp (the line using f.Now.UTC().Format in the
ReservationNameFields/ReservationName helper) to check for a zero value and fall
back to time.Now() when f.Now.IsZero(), then use that non-zero time converted to
UTC and formatted; reference the ReservationNameFields.Now field and the
timestamp formatting expression to locate where to add the IsZero() check and
fallback.
🪄 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: e20c29a5-da08-4ecf-bed3-36151bac9cf6
📒 Files selected for processing (12)
pkg/common/reservation_name.gopkg/common/reservation_name_test.goproviders/aws/services/elasticache/client.goproviders/aws/services/elasticache/client_test.goproviders/aws/services/memorydb/client.goproviders/aws/services/memorydb/client_test.goproviders/aws/services/opensearch/client.goproviders/aws/services/opensearch/client_test.goproviders/aws/services/rds/client.goproviders/aws/services/rds/client_test.goproviders/aws/services/redshift/client.goproviders/aws/services/redshift/client_test.go
…op in length test Default f.Now to time.Now() when IsZero() so a missed caller field produces a current timestamp rather than a year-0001 artifact. Deterministic tests are unaffected because they set Now: testFixedNow explicitly. Add assert.NotContains(t, got, "allup", ...) to TestBuildReservationName_LengthFit_DropsPaymentLast so the test actually verifies the payment segment is dropped in the tight-length regime, not just that timestamp and random were removed. Addresses CodeRabbit findings on PR #689.
|
Both CodeRabbit findings addressed in 55b571e: Finding 1 (test, Finding 2 ( All 33 tests in @coderabbitai review |
|
Triggering a fresh review of commit (´• ω •`) 🐇 ✅ Actions performedReview triggered.
|
Add a shared builder that composes self-describing identifiers for AWS reservation purchases. The format mirrors the Azure DisplayName format introduced in #686 (cross-cloud symmetry for ops dashboards): {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} Output is always sanitized via SanitizeReservationID for the AWS reservation-name allowlist ([a-zA-Z0-9-], dots in SKUs become hyphens) and capped at 60 characters — the tightest length cap across the five AWS services that accept a customer-supplied reservation ID/name (RDS ReservedDBInstanceId, ElastiCache ReservedCacheNodeId, MemoryDB ReservationId, OpenSearch ReservationName). When composed length exceeds the cap, optional tail segments drop in priority order: random suffix first, then timestamp, then payment option. The service code, region, SKU, count, and term are never dropped. In the pathological worst case the builder truncates the SKU itself so count and term still survive the final assembly. Time and random sources are injectable for deterministic tests. Refs #687 pkg/common/reservation_name.go | +201 pkg/common/reservation_name_test.go | +281
#687) Wire common.BuildReservationName into the five AWS reservation purchase paths. On the no-token CLI fallback path (i.e. when opts.IdempotencyToken is empty), each service now composes a self-describing identifier of shape: {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} For example, an OpenSearch RI purchase that previously yielded the opaque `opensearch-Standard_X-1779468234` now becomes `opensearch-us-east-1-r6g-large-search-3x-1yr-allup-20260521T002019-a1b2c3d4` (truncated to 60 chars per service allowlist; see #687). Per service: * rds: deriveReservationID's fmt.Sprintf fallback -> BuildReservationName ("rds-" prefix). opts.ReservationID still wins when set; idempotency token still wins above both. * elasticache: ReservedCacheNodeId fallback -> BuildReservationName ("cache-" prefix). * memorydb: ReservationId fallback -> BuildReservationName ("memdb-"). * opensearch: ReservationName fallback -> BuildReservationName ("opensearch-"). * redshift: PurchaseReservedNodeOffering doesn't accept a customer- supplied node ID, so the rich descriptors travel as tags instead. tagReservedNode now writes Count / Term / PaymentOption alongside the pre-existing NodeType / Region / PurchaseDate / Tool tags so the reserved node is identifiable from the AWS console alone. The token-based path (issue #641) is unchanged: IdempotentReservationID still derives the deterministic ID from the token so a re-drive sends the identical name and AWS rejects the duplicate server-side. The rich builder applies ONLY to the no-token fallback. Tests: one new per-service capture test asserts ^{svc-code}- prefix, region presence, SKU (dots->hyphens) presence, count+term embedded, and the 60-char cap. Redshift's test asserts the new tag keys (Count/Term/PaymentOption) appear in the CreateTags input. Closes #687
…op in length test Default f.Now to time.Now() when IsZero() so a missed caller field produces a current timestamp rather than a year-0001 artifact. Deterministic tests are unaffected because they set Now: testFixedNow explicitly. Add assert.NotContains(t, got, "allup", ...) to TestBuildReservationName_LengthFit_DropsPaymentLast so the test actually verifies the payment segment is dropped in the tight-length regime, not just that timestamp and random were removed. Addresses CodeRabbit findings on PR #689.
cristim
force-pushed
the
feat/issue-687-aws-rich-names
branch
from
55b571e to
0ddd4e5
Compare
|
Rebased onto feat/multicloud-web-frontend@b581fe777 to resolve conflicts in providers/aws/services/rds/client_test.go and providers/aws/services/opensearch/client_test.go. Both conflicts were additive: this PR's The two CR-fix commits from the prior head (55b571e) survived the rebase: the zero- Tests re-run green: New HEAD: 0ddd4e5 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
DisplayNameformat from fix(azure): sanitize Azure DisplayName to [A-Za-z0-9_-] for calculatePrice (closes #685) #686 — operators can identify a reservation from the AWS console alone, without cross-referencing CUDly's purchase audit log.common.BuildReservationNamecomposes{svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand}, sanitized to the AWS reservation-name allowlist and capped at 60 chars (the tightest length cap across the 5 services).Per-service examples (no-token CLI path)
Token-based path is unchanged (deterministic IDs via
IdempotentReservationID).Length-fit algorithm
When composed length exceeds 60 chars, optional tail segments drop in priority order:
YYYYMMDDThhmmss)allup/noup/partup)Service code / region / SKU / count / term never drop. In the pathological worst case (an impossibly long SKU), the SKU itself is truncated rather than the joined string, so count and term still survive the final assembly.
Test plan
BuildReservationNameunit tests inpkg/common/reservation_name_test.go— happy path, length-fit at each drop priority (rand → ts → payment), worst-case SKU truncation, payment normalization (3 canonical + unknown), term normalization, dot→hyphen SKU normalization, deterministic output with fixed sources, always-sanitized output, UTC normalization, per-service prefix coverage^{svc-code}-prefix, region presence, SKU presence (dots→hyphens),{count}x-{term}embedded, and 60-char capgo test ./...clean (4 744 tests across root + AWS submodule)gofmt,go vet,go build ./...cleanAfter merge + deploy, AWS console will show rich self-describing IDs for new reservation purchases via the no-token CLI path. Existing reservations are untouched. The token-based path (web UI) keeps its deterministic IDs from #641.
Related
DisplayName(the model)Summary by CodeRabbit
New Features
Tests