feat(settings): per-plan-type Savings Plans configuration (closes #22)#123
Merged
cristim merged 7 commits intofeat/multicloud-web-frontendfrom Apr 27, 2026
Merged
Conversation
AWS Cost Explorer returns Savings Plans recommendations across four
distinct SavingsPlansType values (ComputeSp, Ec2InstanceSp, SagemakerSp,
DatabaseSp) but the codebase collapses all four into a single
`ServiceSavingsPlans` slug, which prevents per-plan-type term/payment
defaults and forces user-visible lumping in Settings → Purchasing.
This is the first commit in a chain that splits the slug end to end.
It is intentionally additive:
- Four new `ServiceSavingsPlans{Compute,EC2Instance,SageMaker,Database}`
constants, hyphenated to match the existing `savings-plans` style.
- `IsSavingsPlan` helper so call sites that need "any SP slug" semantics
(stats aggregation, region-ignoring filters, display-name branches)
can keep working as each branch migrates to the new constants.
- `ServiceSavingsPlans` remains defined as the legacy umbrella so the
rest of the tree keeps compiling. It will be removed once every caller
either dispatches to one of the four new constants or switches to
`IsSavingsPlan`.
The helper also tolerates the dash-free `"savingsplans"` spelling the
frontend sends and the API handler persists verbatim without
normalisation — see `internal/api/handler_config.go` and the
dual-spelling case in `internal/purchase/execution.go`. The migration
in a later commit canonicalises this, but until then the helper keeps
reality consistent.
No behaviour change yet: callers still use the umbrella.
Treat each AWS Savings Plans plan type as a distinct service end to end: provider registration, client construction, Cost Explorer dispatch, recommendation tagging, CLI flag parsing, stats aggregation, and purchase normalisation. Per-plan-type ServiceConfig rows can now drive divergent term/payment defaults — Compute SP can default to 3yr all-upfront while SageMaker SP runs 1yr no-upfront, etc. The savingsplans client is constructed with an AWS plan type and stores it; GetServiceType returns the matching common.ServiceType slug, and GetExistingCommitments filters DescribeSavingsPlans output to the client's plan type so a four-service-per-account collection cycle returns each commitment exactly once. The recommendations client routes any Savings Plans slug — legacy umbrella plus the four new ones — into getSavingsPlansRecommendations via common.IsSavingsPlan. The function reads the per-plan-type slug from params.Service when present and queries Cost Explorer for that single plan type; it falls back to the legacy IncludeSPTypes filter when the umbrella ServiceSavingsPlans slug is passed, so existing callers keep working until they migrate. parseSavingsPlanDetail now tags each Recommendation with the per-plan-type slug instead of the umbrella, which is what unlocks distinguishing Compute and SageMaker recommendations downstream. cmd/main.go gains four `savingsplans-<type>` (and dash-free) entries in the service map and the dispatch switch. The legacy `savingsplans`, `sp`, and the dash-only `savings-plans` aliases fan out to all four new slugs so existing CLI scripts and the smoke-test fixture keep covering every plan type without source changes. cmd/multi_service_* helpers swap `== common.ServiceSavingsPlans` for the IsSavingsPlan family predicate so stats aggregation, the region-ignoring filter, and the account-level fetch hint all keep working with both the legacy umbrella and the per-plan-type slugs flowing past them. internal/purchase/execution.go's mapServiceType normaliser learns the four new canonical (and dash-free) forms; the legacy entries stay so purchase records persisted before the migration still resolve. Tests updated: provider_test.go expects the four new SP services in GetSupportedServices and dispatches each to its own GetServiceClient case; service_client_test.go and savingsplans/client_test.go drive the constructor with each plan type and assert the matching slug; recommendations/client_test.go locks the per-plan-type tagging on recommendations seeded via IncludeSPTypes; cmd/main_test.go covers all four per-plan-type createServiceClient cases plus the umbrella's nil return; cmd/multi_service_test.go updates the SP-account-level path to the Compute slug. Pre-existing TestAWSProvider_GetDefaultRegion fails in environments where AWS_DEFAULT_REGION is set to anything other than us-east-1; the failure is unrelated to this change.
…r-plan-type rows Migration 000040_split_savingsplans completes the per-plan-type Savings Plans split started in 24e9b3a + da64908. The backend has accepted per-plan-type service slugs since C2; this migration converts existing data to the new schema atomically: - INSERT four `(aws, savings-plans-<type>)` rows per existing umbrella `(aws, savings-plans)` row, with full column copy (enabled/coverage/ramp_schedule/include_*/exclude_* preserved verbatim). Term/payment for the SageMaker slot prefer the value from PR #71's `(aws, sagemaker)` row when present, falling back to the umbrella's value otherwise; the other three slots always inherit from the umbrella. - DELETE the umbrella row in the same transaction. The `(aws, sagemaker)` row from PR #71 is intentionally KEPT for one release behind a SQL deprecation comment so a user mid-rollout doesn't lose their save (a follow-up migration removes it). - Rewrite `purchase_plans.services` JSONB keys: any existing `aws:savings-plans` (or dash-free `aws:savingsplans`) entry fans out into four `aws:savings-plans-<type>` entries carrying the same value object, and the source key is removed. Implementation uses `jsonb_object_agg` over `jsonb_each` because `jsonb_set` can't atomically delete-and-insert multiple keys in one pass. The pre-flight DO block guards three states — empty (no SP rows), already-split (re-run safe), and inconsistent (umbrella + split rows coexist; RAISE EXCEPTION rather than double-write). The down migration is lossy by design (the four per-plan-type rows may carry divergent term/payment that collapse back into one row); it sources the restored umbrella from `savings-plans-compute` per the deterministic fallback rule, no-ops when no source row exists, and leaves the PR #71 sagemaker row untouched. Test coverage in `migrations/split_savingsplans_test.go` (integration build tag) drives the three plan §7 scenarios: umbrella-only, umbrella + PR #71 sagemaker, and fresh install — plus the `purchase_plans.services` JSONB key rewrite. The integration tests require Docker for testcontainers-go; they run in CI but are skipped locally when Docker is unavailable (the migration itself was hand-verified against the reference pattern in 000032_recommendations_add_term_payment_to_key.up.sql).
The frontend half of the per-plan-type SP split. Replaces the umbrella "Savings Plans" card and PR #71's "SageMaker Savings Plans" card with four per-plan-type cards: Compute / EC2 Instance / SageMaker / Database. Each card writes to its own `(aws, savings-plans-<type>)` ServiceConfig row via the existing PUT /api/config/service path, matching the backend slugs introduced in 24e9b3a + da64908 and the DB schema produced by migration 000040. - `frontend/src/index.html`: replace 2 SP cards with 4. Each carries product-specific help text under the title (Compute SP: "EC2, Fargate, Lambda — most flexible"; EC2 Instance SP: "EC2 only, region-locked — deepest discount"; SageMaker SP: "SageMaker training/inference"; Database SP: "Reserved for future AWS Database Savings Plans (currently not GA; defaults stored for forward-compatibility)"). Service-filter dropdowns on Recommendations and Plan-creation tabs gain a "Savings Plans" `<optgroup>` with the four per-plan-type values; the legacy `<option value="savingsplans">` is removed from those filters. - `frontend/src/settings.ts`: `SERVICE_FIELDS[]` swaps the legacy `savingsplans` and PR #71 `sagemaker` entries for the four `savings-plans-<type>` entries. The save / load / dirty-tracking / cascade machinery is data-driven, so no other code changes were needed. - `frontend/src/commitmentOptions.ts`: NO new per-key entries — the existing `_default` AWS arm already accepts all 6 (term × payment) combos for SP, which is what we want for all four per-plan-type keys. A new `it.each` test in commitmentOptions.test.ts asserts 24 cases (4 keys × 6 combos) all return true so a future change to the fallback can't silently restrict SP saves. Tests: - `settings.test.ts`: HTML id arrays updated; the SageMaker save round-trip now writes via `savings-plans-sagemaker`; the `'calls updateServiceConfig once per service field'` regression count goes from 16 → 18 (5 AWS RIs + 4 AWS SP + 5 Azure + 4 GCP); the Lambda-card guard test stays. New empty-state regression test asserts the four cards remain interactable when `services: []` is loaded (no crash on missing service entries; selects keep the HTML defaults — wider "cascade globalCfg defaults to empty selects" behaviour is OUT OF SCOPE per plan §6.5). - `commitmentOptions.test.ts`: 24-case `_default`-fallback lock-in for the four new keys. - `internal/api/validation_test.go`: each of the four canonical SP slugs is added to the `TestValidateServiceName` table so a future `serviceNameRegex` tightening can't silently break SP saves at the API layer. Frontend test count: 1274 (pre-existing + 24 commitmentOptions cases + 1 empty-state + 1 four-cards-present, with the SageMaker round-trip test refit to the new slug). `npm run build` clean; backend `internal/api` tests green.
Light docs touch-up alongside the per-plan-type Savings Plans split landed in 24e9b3a + da64908 + the C5/C6 commits. - docs/smoke-test-multi-account.sh: the service-override step now PUTs to aws/savings-plans-compute (the most common plan type) and the example purchase plan keys its services JSONB on aws:savings-plans-compute. A comment notes how to swap to other plan types. The legacy aws:savingsplans key would still be accepted by the migration's idempotent JSONB rewrite, but the smoke-test fixture should demonstrate the canonical post-split shape. - known-issues.md: append the two pre-existing UX limitations the split exposes - multi-SP plan-summary rendering shows only the first plan type, and bulk-buy-from-Recommendations no longer groups SP recommendations across plan types. Both are tracked as follow-up UI-only fixes that don't block the migration. The entry links them to plans.ts:231 and the bulk-buy modal in recommendations.ts so a future maintainer has the call sites. README updates documenting the four new --services flag values are deferred to a follow-up PR — pre-existing markdownlint table-style errors on README block the hook for now and fixing them is broader cleanup than this PR's scope.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. 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)
📝 WalkthroughWalkthroughThe PR splits the single unified Savings Plans service type into four distinct plan-type-specific service types (Compute, EC2Instance, SageMaker, Database), introducing a centralized Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Parser
participant Backend as Backend Services
participant AWS as AWS APIs
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over User,DB: Legacy Flow (Single Savings Plans Service)
User->>CLI: Specify "savingsplans" or "savings-plans"
CLI->>Backend: Route to ServiceSavingsPlans
Backend->>AWS: Query all SP plan types
AWS-->>Backend: Mixed recommendations (Compute, EC2, SageMaker, DB)
Backend->>DB: Store with service="savings-plans"
DB-->>Backend: Confirm storage
Backend-->>CLI: Return all SP types grouped
end
rect rgba(150, 200, 100, 0.5)
Note over User,DB: New Flow (Per-Plan-Type Services)
User->>CLI: Specify "savings-plans-compute" or legacy "savingsplans"
CLI->>Backend: Route to ServiceSavingsPlansCompute
Backend->>AWS: Query only Compute plan type
AWS-->>Backend: Compute recommendations only
Backend->>DB: Store with service="savings-plans-compute"
DB-->>Backend: Confirm storage
Backend-->>CLI: Return Compute SP recommendations
end
sequenceDiagram
participant Frontend as Frontend UI
participant Backend as API/Backend
participant Provider as AWS Provider
participant SPClient as SP Client
participant AWS as AWS APIs
rect rgba(100, 150, 200, 0.5)
Note over Frontend,AWS: Recommendations Processing per Plan Type
Frontend->>Backend: Request recommendations for "savings-plans-sagemaker"
Backend->>Provider: GetRecommendations(service="savings-plans-sagemaker")
Provider->>SPClient: Instantiate with SavingsPlanTypeSageMaker
SPClient->>AWS: DescribeSavingsPlans + GetServiceCostExplorer
AWS-->>SPClient: SageMaker plan recommendations only
SPClient->>SPClient: Tag each with service="savings-plans-sagemaker"
SPClient-->>Provider: Return plan-type-specific recommendations
Provider-->>Backend: Return tagged recommendations
Backend-->>Frontend: Display SageMaker-scoped results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related issues
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 unit tests (beta)
Comment |
Member
Author
|
@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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/helpers.go (1)
130-139:⚠️ Potential issue | 🟠 MajorDon't silently discard Savings Plans recommendations when details are absent.
If the type assertion at Line 131 fails, the unconditional
continueat Line 139 skips appending anything, so partial coverage can erase a valid Savings Plans recommendation entirely. Please preserve the record or fail explicitly instead of dropping it silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/helpers.go` around lines 130 - 139, The savings-plan branch silently drops records when the type assertion on rec.Details to *common.SavingsPlanDetails fails; update the block in the common.IsSavingsPlan branch (the code that inspects rec.Details and builds adjusted, sets adjusted.Details and adjusted.EstimatedSavings) to handle the failed assertion explicitly: if the assertion succeeds, apply the HourlyCommitment and EstimatedSavings scaling as before; if it fails, preserve the original rec (or at minimum set adjusted = rec) and still append it to result, or log/return an explicit error instead of taking the unconditional continue that currently discards the record. Ensure you reference and update rec.Details, SavingsPlanDetails, adjusted, and adjusted.EstimatedSavings handling so no Savings Plan recommendation is silently removed.providers/aws/recommendations/parser_sp.go (2)
206-245:⚠️ Potential issue | 🟡 MinorMake Savings Plans query order deterministic.
getFilteredPlanTypesiterates a map, so the returned order can vary between runs. That makes downstream “first Savings Plans type wins” behavior unstable and hard to test.Proposed fix
func getFilteredPlanTypes(includeSPTypes, excludeSPTypes []string) []types.SupportedSavingsPlansType { - // All available plan types - allPlanTypes := map[string]types.SupportedSavingsPlansType{ - "compute": types.SupportedSavingsPlansTypeComputeSp, - "ec2instance": types.SupportedSavingsPlansTypeEc2InstanceSp, - "sagemaker": types.SupportedSavingsPlansTypeSagemakerSp, - "database": types.SupportedSavingsPlansTypeDatabaseSp, - } + allPlanTypes := []struct { + name string + typ types.SupportedSavingsPlansType + }{ + {"compute", types.SupportedSavingsPlansTypeComputeSp}, + {"ec2instance", types.SupportedSavingsPlansTypeEc2InstanceSp}, + {"sagemaker", types.SupportedSavingsPlansTypeSagemakerSp}, + {"database", types.SupportedSavingsPlansTypeDatabaseSp}, + } @@ if len(includeMap) > 0 { - for name, planType := range allPlanTypes { - if includeMap[name] && !excludeMap[name] { - result = append(result, planType) + for _, item := range allPlanTypes { + if includeMap[item.name] && !excludeMap[item.name] { + result = append(result, item.typ) } } } else { - for name, planType := range allPlanTypes { - if !excludeMap[name] { - result = append(result, planType) + for _, item := range allPlanTypes { + if !excludeMap[item.name] { + result = append(result, item.typ) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/aws/recommendations/parser_sp.go` around lines 206 - 245, The function getFilteredPlanTypes relies on iterating the allPlanTypes map which makes output order nondeterministic; make the order deterministic by iterating a fixed ordered list of plan names instead of the map keys. Replace the map iteration in getFilteredPlanTypes with a predefined slice like orderedNames := []string{"compute","ec2instance","sagemaker","database"} and loop over orderedNames to look up planType := allPlanTypes[name], applying the existing includeMap/excludeMap logic (both when includeMap is non-empty and when it is empty) so result is appended in the fixed order every run.
62-65:⚠️ Potential issue | 🟠 MajorPropagate single-plan-type Cost Explorer failures.
After the split, each Savings Plans service can hit this path with exactly one
planType. In that case thiscontinueturns a real AWS/API failure into “0 recommendations”, silently dropping an entire plan type from the run.Proposed fix
if err != nil { + if len(planTypes) == 1 { + return nil, fmt.Errorf("failed to get %s recommendations: %w", planType, err) + } fmt.Printf("Warning: Failed to get %s recommendations: %v\n", planType, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/aws/recommendations/parser_sp.go` around lines 62 - 65, The loop in providers/aws/recommendations/parser_sp.go that handles each planType currently logs the error and does "continue", which hides real API failures for single-plan-type runs; change the error handling where planType and err are checked so that instead of continuing you return or propagate a wrapped error (e.g., return fmt.Errorf("failed to get %s recommendations: %w", planType, err)) from the enclosing function (the Parse/parseSavingsPlans/ParseRecommendations function containing that loop), ensuring the caller sees the failure and you include planType and the original err in the message for context.providers/aws/services/savingsplans/client.go (1)
216-228:⚠️ Potential issue | 🔴 CriticalUse the client scope as the source of truth for plan type.
findOfferingIDtrustsrec.Details.PlanType, so asavings-plans-computeclient can still validate or purchase anEC2Instance/SageMakeroffering if upstream passes mismatched details. This breaks the per-plan-type isolation the split is trying to enforce.Proposed fix
planType, err := convertPlanType(spDetails.PlanType) - if err != nil { - return "", err + if c.planType != "" { + if err != nil { + return "", err + } + if planType != c.planType { + return "", fmt.Errorf( + "recommendation plan type %q does not match client scope %q", + spDetails.PlanType, + c.planType, + ) + } + } else if err != nil { + return "", err } termSeconds := convertTermToSeconds(rec.Term) paymentOption := convertPaymentOption(rec.PaymentOption) input := &savingsplans.DescribeSavingsPlansOfferingsInput{ - PlanTypes: []types.SavingsPlanType{planType}, + PlanTypes: []types.SavingsPlanType{c.planType}, Durations: []int64{termSeconds}, PaymentOptions: []types.SavingsPlanPaymentOption{paymentOption}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@providers/aws/services/savingsplans/client.go` around lines 216 - 228, The code currently derives planType from spDetails.PlanType which lets upstream data override the client's intended scope; change the source of truth to the client's configured plan type when determining the offering: in the findOfferingID flow use the client's internal plan-type field (e.g., c.planType / c.scope) as the input to convertPlanType instead of spDetails.PlanType (and if rec.Details.PlanType exists, validate it against the client's plan and return an error on mismatch) so convertPlanType and the DescribeSavingsPlansOfferingsInput are always driven by the client scope.
🧹 Nitpick comments (1)
cmd/main_test.go (1)
224-251: Please addparseServicescoverage for the new Savings Plans spellings too.These cases verify dispatch, but not the CLI input contract. A regression where
parseServicesstops acceptingsavings-plans-*,savingsplans-*, or the legacy fan-out aliases would still pass this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/main_test.go` around lines 224 - 251, Add unit test cases exercising parseServices to cover the new Savings Plans input spellings and the legacy umbrella aliases: include inputs like "savings-plans-compute", "savingsplans-compute" and similarly for the other plan types (EC2Instance, SageMaker, Database), plus the legacy umbrella tokens that should map to a nil/expand behavior; update the existing test function that asserts service dispatch (referencing parseServices and the ServiceSavingsPlans* constants such as common.ServiceSavingsPlansCompute and common.ServiceSavingsPlans) so it verifies parseServices accepts both hyphenated and non-hyphenated forms and that the umbrella slug produces the expected fan-out / nil result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Around line 166-173: The loop that expands serviceNames into result may append
duplicates (e.g., allSPSlugs from savingsplans plus an explicit
savingsplans-compute) so modify the expansion logic in the function using
serviceNames, serviceMap, result and allSPSlugs to prevent duplicates: either
maintain a local seen map[string]struct{} and check before appending each
service slug, or build result then filter it into a deduplicated slice using a
seen set; ensure checks cover both the savingsplans fan-out branch (allSPSlugs)
and the serviceMap append path so each slug is included only once.
In `@cmd/multi_service_stats.go`:
- Around line 105-107: The code currently assigns spStats = stats when
common.IsSavingsPlan(service) is true, which overwrites any previously seen
Savings Plan entry (last-entry-wins) and causes non-deterministic summaries;
instead, detect if spStats already exists and merge the incoming stats into it
by summing numeric fields (cost, usage, counts) and merging/adding any nested
maps or per-dimension summaries, or if spStats is nil, initialize it as a copy
of stats; update the block that references IsSavingsPlan, spStats, stats, and
service to perform an additive/merge operation (deep-merge numeric fields and
combine maps) rather than simple assignment so all five SP service types
contribute to the aggregated SP summary.
In `@frontend/src/settings.ts`:
- Around line 34-45: The AWS_OVERRIDE_SERVICES array needs to be updated to
reflect the new Savings Plans slugs so the account-override modal can create
per-account overrides for each new SP type: replace the legacy 'savingsplans'
and update 'sagemaker' to 'savings-plans-sagemaker', and add
'savings-plans-compute', 'savings-plans-ec2instance', and
'savings-plans-database' entries; locate and edit the AWS_OVERRIDE_SERVICES
constant in frontend/src/settings.ts (and any code that posts the selected slug
from the override modal) so the modal builds and posts the new slugs instead of
the old ones.
In `@internal/database/postgres/migrations/000040_split_savingsplans.down.sql`:
- Around line 61-66: The current rollback SELECT builds the umbrella
'aws:savings-plans' only from the 'aws:savings-plans-compute' key, which can
drop data when only other split keys exist; change the query that reads from
jsonb_each(services) (the SELECT 'aws:savings-plans' AS new_key, v AS new_val
FROM jsonb_each(services) AS e(k, v) WHERE k = 'aws:savings-plans-compute' LIMIT
1) to prefer 'aws:savings-plans-compute' but fall back to the first existing
savings-plan key (any k LIKE 'aws:savings-plans-%') — e.g. by selecting from
jsonb_each and ordering by (k = 'aws:savings-plans-compute') DESC, k ASC LIMIT
1, so the UPDATE will use compute if present otherwise the first available split
key.
In `@internal/database/postgres/migrations/000040_split_savingsplans.up.sql`:
- Around line 124-129: The current SELECT uses jsonb_each + LIMIT 1 which can
arbitrarily pick between 'aws:savings-plans' and 'aws:savingsplans'; update the
query to choose deterministically (e.g., prefer 'aws:savings-plans' if present,
otherwise fall back to 'aws:savingsplans') instead of relying on
jsonb_each/LIMIT 1 — replace the subquery that produces sp_val with a
deterministic expression using COALESCE or a CASE that reads
services->'aws:savings-plans' first then services->'aws:savingsplans' so the
new_key/new_val selection is stable.
- Around line 61-69: The split_count check currently only counts rows for
service = 'savings-plans-compute', which misses other per-plan-type rows; update
the SELECT INTO split_count from service_configs (the query that sets
split_count) to detect any per-plan-type savings plans (e.g. use WHERE provider
= 'aws' AND (service LIKE 'savings-plans-%' AND service <> 'savings-plans') or
explicitly OR together
'savings-plans-compute','savings-plans-sagemaker','savings-plans-database','savings-plans-ec2instance')
so split_count reflects any existing split rows before the IF that compares
umbrella_count and split_count and triggers RAISE NOTICE/EXCEPTION. Ensure you
update only the SELECT that populates split_count and leave the subsequent IF
logic (umbrella_count, RAISE NOTICE/EXCEPTION) unchanged.
In `@internal/purchase/execution.go`:
- Around line 399-400: The mapping in execution.go that aliases "savings-plans"
and "savingsplans" to common.ServiceSavingsPlans causes runtime failures because
GetServiceClient expects one of the four concrete Savings Plans types that
providers/aws/provider.go constructs; update execution.go to stop mapping legacy
umbrella slugs to common.ServiceSavingsPlans and instead normalize those aliases
to the concrete plan constants (the four split ServiceSavingsPlans types) before
reaching GetServiceClient, or alternatively restore an umbrella client path by
adding handling in GetServiceClient/providers/aws client factory to accept
common.ServiceSavingsPlans; reference the keys "savings-plans"/"savingsplans",
the constant common.ServiceSavingsPlans, and the call site GetServiceClient when
making the change.
---
Outside diff comments:
In `@cmd/helpers.go`:
- Around line 130-139: The savings-plan branch silently drops records when the
type assertion on rec.Details to *common.SavingsPlanDetails fails; update the
block in the common.IsSavingsPlan branch (the code that inspects rec.Details and
builds adjusted, sets adjusted.Details and adjusted.EstimatedSavings) to handle
the failed assertion explicitly: if the assertion succeeds, apply the
HourlyCommitment and EstimatedSavings scaling as before; if it fails, preserve
the original rec (or at minimum set adjusted = rec) and still append it to
result, or log/return an explicit error instead of taking the unconditional
continue that currently discards the record. Ensure you reference and update
rec.Details, SavingsPlanDetails, adjusted, and adjusted.EstimatedSavings
handling so no Savings Plan recommendation is silently removed.
In `@providers/aws/recommendations/parser_sp.go`:
- Around line 206-245: The function getFilteredPlanTypes relies on iterating the
allPlanTypes map which makes output order nondeterministic; make the order
deterministic by iterating a fixed ordered list of plan names instead of the map
keys. Replace the map iteration in getFilteredPlanTypes with a predefined slice
like orderedNames := []string{"compute","ec2instance","sagemaker","database"}
and loop over orderedNames to look up planType := allPlanTypes[name], applying
the existing includeMap/excludeMap logic (both when includeMap is non-empty and
when it is empty) so result is appended in the fixed order every run.
- Around line 62-65: The loop in providers/aws/recommendations/parser_sp.go that
handles each planType currently logs the error and does "continue", which hides
real API failures for single-plan-type runs; change the error handling where
planType and err are checked so that instead of continuing you return or
propagate a wrapped error (e.g., return fmt.Errorf("failed to get %s
recommendations: %w", planType, err)) from the enclosing function (the
Parse/parseSavingsPlans/ParseRecommendations function containing that loop),
ensuring the caller sees the failure and you include planType and the original
err in the message for context.
In `@providers/aws/services/savingsplans/client.go`:
- Around line 216-228: The code currently derives planType from
spDetails.PlanType which lets upstream data override the client's intended
scope; change the source of truth to the client's configured plan type when
determining the offering: in the findOfferingID flow use the client's internal
plan-type field (e.g., c.planType / c.scope) as the input to convertPlanType
instead of spDetails.PlanType (and if rec.Details.PlanType exists, validate it
against the client's plan and return an error on mismatch) so convertPlanType
and the DescribeSavingsPlansOfferingsInput are always driven by the client
scope.
---
Nitpick comments:
In `@cmd/main_test.go`:
- Around line 224-251: Add unit test cases exercising parseServices to cover the
new Savings Plans input spellings and the legacy umbrella aliases: include
inputs like "savings-plans-compute", "savingsplans-compute" and similarly for
the other plan types (EC2Instance, SageMaker, Database), plus the legacy
umbrella tokens that should map to a nil/expand behavior; update the existing
test function that asserts service dispatch (referencing parseServices and the
ServiceSavingsPlans* constants such as common.ServiceSavingsPlansCompute and
common.ServiceSavingsPlans) so it verifies parseServices accepts both hyphenated
and non-hyphenated forms and that the umbrella slug produces the expected
fan-out / nil result.
🪄 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: a7317b02-2454-4c4d-9a73-d93203a5387e
📒 Files selected for processing (32)
cmd/helpers.gocmd/main.gocmd/main_test.gocmd/multi_service_filters.gocmd/multi_service_helpers.gocmd/multi_service_stats.gocmd/multi_service_stats_helpers.gocmd/multi_service_test.godocs/smoke-test-multi-account.shfrontend/src/__tests__/commitmentOptions.test.tsfrontend/src/__tests__/settings.test.tsfrontend/src/index.htmlfrontend/src/settings.tsinternal/api/validation_test.gointernal/database/postgres/migrations/000040_split_savingsplans.down.sqlinternal/database/postgres/migrations/000040_split_savingsplans.up.sqlinternal/database/postgres/migrations/split_savingsplans_test.gointernal/purchase/execution.goknown-issues.mdpkg/common/types.gopkg/common/types_test.goproviders/aws/provider.goproviders/aws/provider_test.goproviders/aws/recommendations/client.goproviders/aws/recommendations/client_test.goproviders/aws/recommendations/parser_sp.goproviders/aws/recommendations/parser_sp_additional_test.goproviders/aws/recommendations/parser_sp_test.goproviders/aws/service_client.goproviders/aws/service_client_test.goproviders/aws/services/savingsplans/client.goproviders/aws/services/savingsplans/client_test.go
Eleven findings (1 critical, 9 major, 1 minor) addressed in one follow-up commit on the per-plan-type Savings Plans split. Critical (1) - providers/aws/services/savingsplans/client.go findOfferingID: enforce client scope as the source of truth for plan type. Per-plan-type clients now reject mismatched recommendations client-side before any AWS call, so a Compute-scoped client can't validate or buy a SageMaker offering even if upstream stamps the wrong rec.Details. Umbrella clients (c.planType == "") fall back to rec.Details.PlanType to preserve pre-split behaviour. Major (9) - cmd/main.go parseServices: dedupe via a `seen` set so `--services savingsplans,savingsplans-compute` doesn't double-process Compute SP through both the fan-out and the explicit-slug paths. - cmd/multi_service_stats.go separateAndAggregateStats: aggregate Savings Plans counters across all matching slugs instead of last-write-wins assignment. Pre-split there was one SP slug; post-split there are five (umbrella + four per-plan-type) and Go map iteration is non-deterministic, so the previous code lost data unpredictably. - cmd/helpers.go ApplyCoverage: don't silently drop SP recs when the Details type assertion fails. Append the original recommendation unscaled and emit a WARNING log instead. - providers/aws/recommendations/parser_sp.go: propagate single- plan-type Cost Explorer failures. After the split, each SP service hits this loop with one plan type; the previous warn-and-continue path turned a real outage into a silent "0 recommendations" result. - providers/aws/recommendations/parser_sp.go getFilteredPlanTypes: iterate a fixed-order slice rather than the map literal. Map range in Go is non-deterministic, so downstream "first plan type wins" expectations and tests now have stable order. - frontend/src/settings.ts AWS_OVERRIDE_SERVICES: replace the legacy `savingsplans` and PR #71 `sagemaker` entries with the four per-plan-type slugs so the per-account override modal can target the same ServiceConfig rows the global Settings cards write to. Settings-accounts test fixtures updated to match the new 9-element list. - internal/database/postgres/migrations/000040 up.sql split_count guard: count any per-plan-type row, not just `savings-plans-compute`. Otherwise the migration would treat a partially-split DB (e.g. only sagemaker rows present) as a clean state, mix old split rows with newly inserted ones, then delete the umbrella. - internal/database/postgres/migrations/000040 up.sql JSONB rewrite: replace `jsonb_each ... LIMIT 1` with COALESCE of the canonical hyphenated key first, dash-free spelling second. Eliminates the non-deterministic key-pick when both spellings coexist. - internal/database/postgres/migrations/000040 down.sql JSONB collapse: order by preference (compute → ec2instance → sagemaker → database) instead of restricting to compute-only. Plans created post-split that contain only sagemaker / database / ec2instance keys now survive rollback instead of losing the umbrella-key UPDATE. - providers/aws/provider.go: restore `common.ServiceSavingsPlans` to GetSupportedServices and add the matching GetServiceClient case, constructing the SP client with an empty plan type (umbrella mode). Any persisted RecommendationRecord still tagged with the legacy slug — and any scheduled purchase derived from it — keeps a working executable client through the purchase pipeline. The internal/purchase/execution.go alias mapping was untouched, but now resolves to a registered service again. New tests pin the umbrella-mode unfiltered behaviour and the per-plan-type strict-match rejection. Minor (1) - providers/aws/recommendations/parser_sp.go getFilteredPlanTypes determinism — same fix as the major item above. Tests: backend `go build ./...` clean, savingsplans tests green including two new ones (umbrella unfiltered + mismatch rejected), provider_test.go updated to expect the umbrella back in the supported list, frontend `npm test` 1274/1274.
This was referenced Apr 27, 2026
Member
Author
CodeRabbit review addressed (commit
|
| # | Title | Source |
|---|---|---|
| #131 | fix(plans): multi-SP purchase-plan summary shows only one plan type | Plan §5a / known-issues.md |
| #132 | feat(recommendations): re-enable bulk-buy across SP plan types | Plan §5a / known-issues.md |
| #133 | chore(db): drop deprecated (aws, sagemaker) ServiceConfig row after one stable release |
Plan §6.5 — cited in 000040.up.sql now |
| #134 | feat(commitmentopts): probe AWS SP offerings for live term/payment validation | Plan §5a |
| #135 | feat(recommendations): group per-plan-type SP rows in Recommendations table | Plan §6.5 |
| #136 | feat(settings): per-product Coverage / Include-Exclude / Enabled for SP cards | Plan §6.5 |
| #137 | feat(filter): "All Savings Plans" aggregate option in Recommendations service-filter | Plan §5 #5 |
| #138 | docs(README): document four per-plan-type --services slugs and update tables | Reverted README patch from this PR |
Tests: backend go build ./... clean, savingsplans tests green (incl. 2 new), provider/AccountTypes tests updated, frontend npm test 1274/1274.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
… first (#156) Closes #131. PR #123 (per-plan-type Savings Plans split) gave a single plan up to four `aws:savings-plans-<type>` entries, but `extractPlanInfo` in `frontend/src/plans.ts` rendered only `Object.values(services)[0]` — hiding all but one. The pre-existing "Multiple" placeholder for >1 service was an unhelpful stub: an operator looking at the card had no way to know which SP plan-types the plan actually covered. Fix: replace the "Multiple" branch with a comma-joined list of every service's label. SP slugs get an abbreviated label ("Compute SP", "SageMaker SP", "EC2 Instance SP", "Database SP") so a plan with all four still fits in the summary line. Non-SP slugs pass through unchanged so single-service plans render exactly as before (regression-safe). Tests: - `multi-SP plan summary lists every plan-type covered (issue #131)` pins the new behaviour: a plan with Compute SP + SageMaker SP shows both labels and the old "Multiple" placeholder is gone. - `single-service plan still renders one label (no regression)` pins the unchanged single-service path. Frontend-only change. Backend unchanged.
This was referenced Apr 27, 2026
cristim
added
priority/p1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #22.
Splits AWS Savings Plans into four per-plan-type services end to end so users can pin term/payment defaults independently per SP plan type (Compute, EC2 Instance, SageMaker, Database). Builds on the merged label clarification in PR #53 and the SageMaker card from PR #71 — those landed the user-visible scope of #22, but #22 stayed open because the underlying limitation (one shared term/payment for all four SP plan types) remained.
This PR completes the split:
common.ServiceTypeconstants (savings-plans-{compute,ec2instance,sagemaker,database}), four-way provider registration, per-plan-typeSavingsPlansClientthat filtersDescribeSavingsPlansresults so each service invocation returns only its plan type's commitments,parser_sp.gothat resolves the AWS plan type fromparams.Service, andcmd/main.goaccepting the four canonical and dash-free spellings while keeping the legacysavingsplans/sp/savings-plansaliases as fan-out shorthands so existing scripts and the smoke-test fixture keep working unchanged.(aws, savings-plans)ServiceConfig row into four per-plan-type rows with verbatim copy ofenabled/coverage/ramp_schedule/include_*/exclude_*columns; the SageMaker slot inherits term/payment from PR ux(settings): add SageMaker and Lambda purchasing-defaults cards #71's(aws, sagemaker)row when present, falling back to the umbrella otherwise. The umbrella is deleted in the same transaction; PR ux(settings): add SageMaker and Lambda purchasing-defaults cards #71's(aws, sagemaker)row is intentionally KEPT for one release behind a SQL deprecation comment so a user mid-rollout doesn't lose their save (a follow-up migration removes it).purchase_plans.servicesJSONB keys are rewritten in the same transaction: anyaws:savings-plans(or dash-freeaws:savingsplans) entry fans out into fouraws:savings-plans-<type>entries carrying the same value object, with the source key removed.sagemakercards with four per-plan-type cards in Settings → Purchasing. Each card has product-specific help text. Service-filter dropdowns on the Recommendations and Plan-creation tabs gain a "Savings Plans"<optgroup>with four per-plan-type values; the legacy<option value="savingsplans">is removed from those filters.commitmentOptions.tsdoesn't add per-key entries — the existing_defaultAWS arm already accepts all 6 (term × payment) combos, locked in by 24 new test cases.Commits
24e9b3a5a refactor(common): add per-plan-type Savings Plans service constantsda649085d refactor(aws): split Savings Plans into four per-plan-type services1bb2fe6ce feat(db): split umbrella savings-plans ServiceConfig row into four per-plan-type rows7b024ab56 feat(settings): four per-plan-type Savings Plans cards in Purchasing UIdee724305 docs: update smoke-test and known-issues for per-plan-type SP splitKnown caveats (exposed, not caused, by the split)
These are pre-existing UX limitations that became visible because the per-plan-type split surfaces multiple SP plan types where there used to be one umbrella. Both are tracked in
known-issues.mdas follow-ups; neither blocks this PR.frontend/src/plans.ts:231reads the FIRST entry fromplan.servicesfor the summary card. A purchase plan that targets multiple SP plan types now lists only one — whichever sorts first. Fix is plans.ts-only (render comma-separated or count badge) and is OUT OF SCOPE here.IsSavingsPlan(rec.service)for the bulk-buy view only. OUT OF SCOPE.Database SP isn't generally available — AWS Database SP exists in the Cost Explorer enum but isn't widely deployed for purchase. The card is exposed for forward-compat; help text on the card surfaces the caveat to end users.
README CLI-flag-table updates documenting the four new
--servicesslugs are deferred to a follow-up — pre-existing markdownlint table-style errors block the hook on this PR.Deploy-order safety
internal/server/app.goruns migrations on Lambda startup BEFOREreinitializeAfterConnect, so the migrated DB is in place before the new backend binary serves any request. CloudFront's frontend flip happens last. There's no window where new frontend talks to a pre-migration DB or where old backend reads the new schema.Existing scheduled purchases keep working unchanged because the legacy umbrella slug (
"savings-plans"and the dash-free"savingsplans") stays recognised byIsSavingsPlanandmapServiceTypefor backward compat with persistedRecommendationRecord.Servicevalues.Test plan
go build ./...clean (root +pkg/+providers/aws/)go test ./...green at root +pkg/(full suite)go test ./internal/api/...covers the four new SP slugs againstserviceNameRegexproviders/aws/...green except pre-existing env-dependentTestAWSProvider_GetDefaultRegionflake (caused by localAWS_DEFAULT_REGION=ap-south-1; unrelated to this PR)npm run buildcleannpm test— 1274/1274 pass across 35 suites (24 newcommitmentOptions._defaultcases + 4 four-cards-present + 1 empty-state + Lambda-card guard kept)migrations/split_savingsplans_test.go(build tagintegration) drive three scenarios (umbrella-only, umbrella + PR ux(settings): add SageMaker and Lambda purchasing-defaults cards #71 sagemaker, fresh install) plus the JSONB key rewrite. They require Docker for testcontainers-go; runs in CI but skipped locally where Docker is unavailable.dist/index.htmlconfirms structure pre-deploy).Summary by CodeRabbit
New Features
Database Migrations