fix(cli): purchase-output corrections (savings label, no-upfront warning, descriptive RI IDs)#618
Conversation
The EstimatedSavings column returned by the Cost Explorer coverage API contains a monthly value, and the "Estimated monthly savings" summary line already says "monthly". The ConfirmPurchase prompt was the only place that said "annual", which was misleading when the number shown matches the monthly column. Update the doc comment and the printed line to say "monthly" so all three surfaces are consistent. Closes #615
When running with --input-csv, the payment option comes from each CSV row rather than from the --payment flag (which keeps its default of no-upfront). The warnRDS3YearNoUpfront validator was firing a false alarm against the flag default even when every CSV row used partial-upfront or all-upfront. Return early when CSVInput is set so the warning only fires in interactive / flag-driven mode. Closes #616
Thread a descriptive commitment ID through PurchaseOptions.ReservationID so it reaches the provider and names the reserved instance in the AWS console and billing exports, rather than using a generic timestamp-based fallback. - pkg/common/types.go: add ReservationID field to PurchaseOptions - cmd/multi_service_helpers.go: compute generatePurchaseID up front in executePurchase and pass it as opts.ReservationID; the result.CommitmentID fallback reuses the same ID instead of recomputing - providers/aws/services/rds/client.go: prefer opts.ReservationID when set, sanitize it, fall back to the generic rds-<type>-<unix> form when empty - cmd/main.go: generatePurchaseID now handles pointer Details (*DatabaseDetails, *CacheDetails, *ComputeDetails, nil-guarded) so the engine/platform segment appears in real purchases where the parser stores pointer types; normalization (lowercasing, hyphenation) is consolidated after the switch rather than duplicated per case - tests: update mock expectations from exact PurchaseOptions struct to mock.MatchedBy matching on Source, so they tolerate the non-deterministic ReservationID; TestExecutePurchase additionally asserts ReservationID is non-empty and contains the instance type Closes #617
|
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 (8)
📝 WalkthroughWalkthroughThis PR fixes three purchase-related issues: descriptive RDS reservation ID generation, incorrect savings label in the confirmation prompt (monthly mislabeled as annual), and a spurious RDS warning in CSV input mode that checks the flag default instead of per-row payment options. ChangesDescriptive Reservation IDs and Purchase Messaging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 review |
✅ Actions performedReview triggered.
|
Summary
Three correctness fixes for the purchase flow output:
fix(cli): purchase confirmation prompt labels monthly savings as 'annual' #615 - Monthly savings label: The
ConfirmPurchaseprompt said "estimated annual savings" butEstimatedSavingsis a monthly value (from Cost Explorer's coverage API). Updated the doc comment and the printed line to say "monthly", consistent with the summary line and CSV column.fix(cli): spurious 3yr no-upfront warning in --input-csv mode (uses flag default, not CSV per-row payment) #616 - Skip 3yr no-upfront warning in CSV mode:
warnRDS3YearNoUpfrontwas firing a false alarm when--input-csvwas used, because the--paymentflag keeps itsno-upfrontdefault even though each CSV row carries its own payment option. Added an early return whenCSVInput != ""so the warning only fires in interactive/flag-driven mode.fix(cli): purchased RDS RIs get generic reservation IDs instead of descriptive names #617 - Descriptive RDS reservation IDs: Reserved DB instances were being named with a generic
rds-<type>-<unix-timestamp>ID. NowexecutePurchasecomputes a descriptive ID (account/engine/region/size/coverage-aware) up front and threads it throughPurchaseOptions.ReservationIDintoPurchaseCommitment, so the reservation is identifiable in the AWS console and billing exports.generatePurchaseIDwas also fixed to handle pointerDetailstypes (*DatabaseDetails,*CacheDetails,*ComputeDetails) that the live parser produces, preventing the engine segment from silently dropping out of real purchases.Closes #615, Closes #616, Closes #617
Out of scope: multi-account execution is tracked separately and intentionally not addressed here.
Verification
go build ./...- cleango vet github.com/LeanerCloud/CUDly/cmd github.com/LeanerCloud/CUDly/providers/aws/services/rds- no issuesgo test github.com/LeanerCloud/CUDly/cmd github.com/LeanerCloud/CUDly/providers/aws/services/rds github.com/LeanerCloud/CUDly/pkg/common- 889 passedSummary by CodeRabbit
Bug Fixes
Improvements