chore(common): normalize Savings Plans identifier to "savingsplans" (frontend canonical)#94
Conversation
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR renames the canonical Go ServiceSavingsPlans identifier from "savings-plans" to "savingsplans" and updates tests and documentation. It preserves backward compatibility by mapping the legacy hyphenated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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)
Review rate limit: 3/5 reviews remaining, refill in 14 minutes and 11 seconds. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cristim
added
triaged
|
Merge conflict detected ( |
|
Heads-up: this PR may need a substantive rework or closure. Attempted a rebase onto current Root cause: base branch shipped the per-plan-type Savings Plans split (migration This PR's intent — flip the umbrella slug from
A correct rebase would require a product call: rename all five SP-related ServiceType values to a dash-free convention (and the migration that consumes them, plus the frontend) — much bigger than the original chore. Suggest either closing this PR (the SP split made the umbrella moot), or reworking it to flip ALL five SP-related ServiceType values + frontend + migration in lockstep. |
…frontend canonical)
Aligns common.ServiceSavingsPlans with the value the frontend already
persists ("savingsplans") and removes the foot-gun where any new code
comparing rec.Service == common.ServiceSavingsPlans would silently miss
Savings Plans rows. Picks the frontend canonical (issue #85 Option B) to
avoid a SQL data migration on service_configs.service / purchase_history.
Pre-flight findings (grep over the whole tree):
- frontend already uses "savingsplans" everywhere — no frontend changes.
- the only Go string literals using the hyphenated form were the
constant declaration, its constant-value test, the dual-case branch in
the purchase-execution mapper, and the matching test case for that
mapper.
- service_configs.service is VARCHAR(64) with no CHECK constraint and no
seed data; the frontend has always written "savingsplans".
- purchase_executions.recommendations is JSONB and rec.Service is
serialised as `string(rec.Service)` (scheduler.go, audit.go), so any
Lambda-scheduled SP execution persisted before this change carries
rec.Service == "savings-plans" in the JSONB blob and is re-fed through
mapServiceType on retry / approval. The dual-case in execution.go is
therefore retained as a backwards-compat alias (with a TODO and a
follow-up issue) instead of being removed; the persistent rows are the
reason the issue's hard "remove the dual-case branch" couldn't be
satisfied in a single PR.
Changes:
- pkg/common/types.go: ServiceSavingsPlans value flipped from
"savings-plans" to "savingsplans"; expanded godoc explains the
rationale and points back at the issue.
- pkg/common/types_test.go: regression guard now asserts the constant
value is "savingsplans" so any future flip back is a deliberate change
that requires a SQL migration.
- internal/purchase/execution.go: mapServiceType keeps both
"savingsplans" and "savings-plans" as legacy alias, with a comment
block + TODO(#85) explaining when it can be removed.
- internal/purchase/coverage_extra_test.go: legacy "savings-plans" input
case retained and re-documented.
Refs #85
cristim
force-pushed
the
chore/normalize-savings-plans-identifier
branch
from
0b321f3 to
ab8290e
Compare
|
Rebased onto current Conflicts resolved (3 files):
@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.
🧹 Nitpick comments (1)
internal/purchase/execution.go (1)
387-397: ⚡ Quick winConsolidate Savings Plans alias mapping to one path.
Line 371 already maps both values through
mapSavingsPlansSlug, so Lines 396-397 are dead code. Keeping duplicate alias logic makes TODO(#85) retirement easy to do incorrectly.Suggested diff
- // Backwards-compat: "savings-plans" was the value of - // common.ServiceSavingsPlans before issue `#85` normalised the constant to - // match the frontend ("savingsplans"). Lambda-scheduled purchase - // executions persisted before that change carry rec.Service == - // "savings-plans" in the purchase_executions.recommendations JSONB blob - // and are re-fed through this mapper on retry / approval. Keep the - // legacy alias accepted here so historical rows still execute correctly. - // TODO(`#85`): drop the "savings-plans" alias once no purchase_executions - // rows older than the `#85` cutoff remain (~6 months retention). - case "savingsplans", "savings-plans": - return common.ServiceSavingsPlans🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/purchase/execution.go` around lines 387 - 397, Remove the duplicate "savings-plans"/"savingsplans" case branch in the switch inside the purchase execution mapper and rely solely on mapSavingsPlansSlug to normalize the slug; ensure mapSavingsPlansSlug explicitly accepts both "savings-plans" and "savingsplans" and returns common.ServiceSavingsPlans, then delete the redundant case returning common.ServiceSavingsPlans so the legacy alias logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/purchase/execution.go`:
- Around line 387-397: Remove the duplicate "savings-plans"/"savingsplans" case
branch in the switch inside the purchase execution mapper and rely solely on
mapSavingsPlansSlug to normalize the slug; ensure mapSavingsPlansSlug explicitly
accepts both "savings-plans" and "savingsplans" and returns
common.ServiceSavingsPlans, then delete the redundant case returning
common.ServiceSavingsPlans so the legacy alias logic is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c63f78c9-6ad0-4478-9d48-10a050255354
📒 Files selected for processing (4)
internal/purchase/coverage_extra_test.gointernal/purchase/execution.gopkg/common/types.gopkg/common/types_test.go
…rviceType (CR pass 1) CR pass 1 on PR #94 flagged that the case I added for the legacy alias is unreachable. Verified: * mapServiceType opens with `if svc, ok := mapSavingsPlansSlug(service); ok { return svc }`. * mapSavingsPlansSlug explicitly maps both "savings-plans" and "savingsplans" to common.ServiceSavingsPlans (and the per-plan-type spellings to their typed constants). So by the time control reaches the switch, those values can never appear — my added case was dead code, and keeping it made the future TODO(#85) retirement easy to do incorrectly (delete the dead case and miss the real mapping site). Removed the case + its TODO comment block. The legacy alias behaviour is unchanged: pre-#85 purchase_executions JSONB rows whose rec.Service == "savings-plans" still resolve via mapSavingsPlansSlug. Verified: go test ./internal/purchase/... passes; full repo go test ./... shows no regressions.
|
CR pass 1 — pushed Addressed (1 nitpick):
if svc, ok := mapSavingsPlansSlug(service); ok { return svc }and Removed the case + its TODO comment block. Behaviour unchanged: pre-#85 The TODO content is now arguably better placed inside
@coderabbitai review |
|
// TODO(`#85`): once purchase_executions JSONB rows persisted before the "savingsplans"
// rename (~6-month retention window) have aged out, the "savings-plans" alias below
// can be removed and only "savingsplans" need be matched here.That way a future maintainer deleting the alias has the full rationale inline at the actual code path, rather than having to grep for it. Happy to trigger the review now — you can decide whether to add the comment in this pass or a follow-up. ✅ Actions performedReview triggered.
|
…Slug (CR pass 2) Per CR pass 2 follow-up: the previous commit removed a dead case that carried the TODO(#85) context. mapSavingsPlansSlug is now the sole canonical alias site for "savings-plans" → "savingsplans" mapping, so the TODO belongs there — a future maintainer deleting the alias has the full rationale (6-month retention, umbrella came from #94, per-plan-type slugs are forward-compat) inline at the actual code path, rather than having to grep for it. Comment-only change. No behaviour change. No new tests needed.
|
Going with the inline-comment approach over an issue. Pushed Reasoning: a 4-line block-comment in Comment captures three pieces of context at the point of action:
That last bit is the one that would have been hardest to recover from git archaeology alone, so worth committing now.
@coderabbitai review |
|
Triggering the incremental review now. ✅ Actions performedReview triggered.
|
1 participant
Summary
Normalises
common.ServiceSavingsPlansfrom the hyphenated"savings-plans"to the frontend canonical"savingsplans"so that direct comparisons (rec.Service == common.ServiceSavingsPlans) work without the historical normaliser shim. Picks issue #85 Option B (frontend wins) to avoid a SQL data migration onservice_configs.service/purchase_history.service.Refs #85(the issue's hard removal of the dual-case branch is intentionally deferred — see "WhyRefsnotCloses" below).Pre-flight findings
Tree-wide grep for
savings-plans/savingsplans/ServiceSavingsPlans:frontend/src/**)"savingsplans"everywhere"savingsplans"pkg/common/types.go:49"savings-plans""savingsplans"pkg/common/types_test.go:42"savings-plans"internal/purchase/execution.go:384dual-case"savings-plans", "savingsplans"internal/purchase/coverage_extra_test.gocommon.ServiceSavingsPlanssymbolicallycmd/main.go::parseServicesmap"savingsplans"service VARCHAR(64), no CHECK, no seed data with either formpkg/common/audit.go)Why the dual-case in
execution.gois retained (deferred from #85's acceptance)scheduler.go:834andaudit.go:50serialiserec.Serviceasstring(rec.Service)into thepurchase_executions.recommendationsJSONB column. Any Lambda-scheduled Savings Plans execution persisted before this PR carries"service": "savings-plans"in that JSONB blob. On retry / approval (internal/purchase/execution.go:231iteratesexec.Recommendations), each rec is re-fed throughmapServiceType(). Removing the legacy alias arm now would silently break those rows by mapping them toServiceType("savings-plans")instead ofServiceSavingsPlans, after whichcloudProvider.GetServiceClient(ctx, serviceType, …)would fail.Trade-off:
TODO(#85)marker, and a follow-up issue tracking eventual removal once historical rows have aged out (~6 months retention). The constant is canonical; the alias is purely a read-side shim at the API boundary.purchase_executions.recommendationsJSONB to flip"savings-plans"→"savingsplans". Out of scope for this PR (matches issue Normalize 'savings-plans' vs 'savingsplans' identifier across frontend + backend #85's explicit "Option B" preference of avoiding migrations).The regression test in
pkg/common/types_test.gowill fail if anyone flips the constant back, so the canonical never silently drifts again.Changes
pkg/common/types.go—ServiceSavingsPlans = "savingsplans"; expanded godoc explains rationale + points back at Normalize 'savings-plans' vs 'savingsplans' identifier across frontend + backend #85.pkg/common/types_test.go— regression guard asserts"savingsplans".internal/purchase/execution.go—mapServiceTypekeeps both"savingsplans"and"savings-plans"with a documented backwards-compat block +TODO(#85)for removal.internal/purchase/coverage_extra_test.go— legacy"savings-plans"input case retained and re-documented.Net diff: 30 insertions, 5 deletions across 4 files.
Verification
go build ./...clean from the root and from each submodule (pkg/,providers/aws,providers/azure,providers/gcp).go test ./...from root +pkg/submodule: all tests pass that were passing before.go test ./providers/aws/...: the only AWS-side failures areTestAWSProvider_GetDefaultRegion/*, which are pre-existing environment-leak bugs unrelated to this PR (the test mutatesaws.Config.RegionbutIsConfigured()clobbers it from the SDK default-credentials chain reading~/.aws/config).TestSavingsPlans*andTestAWSProvider_GetSupportedServices(which touchesServiceSavingsPlansdirectly) all pass.go test ./providers/gcp/...: pre-existing failure inTestMemorystoreClient_GetExistingCommitments_WithMockServiceis unrelated to Savings Plans.go vet ./...clean.Why
Refs #85notCloses #85Issue #85's acceptance includes "The dual-case branch in
execution.go:384removed." This PR retains the dual-case as a backwards-compat alias for persisted Lambda-scheduled rows (rationale above). The follow-up issue tracking the eventual removal will be filed once this lands and the post-merge verification completes.Test plan
service_configs(it should — frontend was already writing"savingsplans"); the round-trip read should now compare equal tocommon.ServiceSavingsPlanswithout the normaliserSummary by CodeRabbit