ux(settings): add SageMaker and Lambda purchasing-defaults cards#71
Merged
cristim merged 2 commits intoApr 27, 2026
Merged
Conversation
Settings → Purchasing previously offered per-service term/payment cards for EC2, RDS, ElastiCache, OpenSearch, Redshift, and the umbrella Savings Plans, but had no cards for SageMaker or Lambda. Both services are significant cost-optimisation targets covered by Savings Plans (SageMaker SP and Compute SP respectively), so the omission forced users to fall back to the global default term/payment for any SageMaker/Lambda recommendation instead of pinning a per-workload preference. Add `aws/sagemaker` and `aws/lambda` cards mirroring the existing AWS card shape (1y/3y term, NoUpfront / Partial / AllUpfront payment). Both services use AWS_PAYMENTS via commitmentOptions.ts's `_default` fallback — no per-service constraints. The new entries plug into the existing SERVICE_FIELDS array so save / load / dirty-tracking / sticky save bar wiring picks them up automatically. The umbrella Savings Plans hint copy is tweaked to clarify the new per-service cards override it. Tests cover render presence (grep index.html for heading + select IDs) and save round-trip (per-service config endpoint receives the edited term and payment) for each service, plus constraint-suite regression tests confirming the 3yr / no-upfront combination stays selectable (SageMaker / Lambda have no service-level restriction). Closes #22
Member
Author
|
@coderabbitai review |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 6 seconds. ⌛ 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 (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Lambda has no dedicated Savings Plan product — Lambda usage is covered exclusively by Compute Savings Plans (alongside EC2 + Fargate), already represented by the umbrella "Savings Plans" card. A separate "Lambda Savings Plans" card invents a non-existent SKU: picking term/payment on it has no purchasable instrument behind it. SageMaker's card stays — SageMaker Savings Plans is a real, distinct SP type (separate product since 2020). - frontend/src/index.html: drop the Lambda service-default-card; rephrase the umbrella SP hint copy so SageMaker is the only call-out (Lambda is implicitly part of "Compute (EC2, Fargate, Lambda)"). - frontend/src/settings.ts: remove the aws/lambda entry from SERVICE_FIELDS, with a comment explaining the omission so a future copy-paste doesn't re-add it. - frontend/src/__tests__/settings.test.ts: drop the three Lambda-specific tests (presence, save round-trip, no-upfront visibility), drop Lambda IDs from the test fixture, and add a negative-guard test asserting no Lambda card or selects ever land in index.html. updateServiceConfig call-count expectation: 17 → 16 (7 AWS + 5 Azure + 4 GCP).
Member
Author
|
Follow-up commit Why: Lambda has no standalone Savings Plan product — Lambda invocations are covered exclusively by Compute Savings Plans, alongside EC2 + Fargate. The umbrella Savings Plans card in this PR already controls Compute SP defaults, which is Lambda's only commitment path. A separate "Lambda Savings Plans" card invents a non-existent SKU: picking term/payment on it has no purchasable instrument behind it. SageMaker stays — SageMaker Savings Plans is a real, distinct SP product (since 2020). Diff scope of the follow-up commit:
Issue #22 has been retitled and rescoped to SageMaker only, with a comment explaining the rationale. Tests: 45/45 settings tests pass; typecheck clean; pre-commit |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
5 tasks
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
…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).
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
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.
8 tasks
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
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.
cristim
added a commit
that referenced
this pull request
Apr 27, 2026
#123) * refactor(common): add per-plan-type Savings Plans service constants 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. * refactor(aws): split Savings Plans into four per-plan-type services 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. * feat(db): split umbrella savings-plans ServiceConfig row into four per-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). * feat(settings): four per-plan-type Savings Plans cards in Purchasing UI 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. * docs: update smoke-test and known-issues for per-plan-type SP split 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. * fix(sp-split): address CodeRabbit review feedback on PR #123 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. * chore(migrations): cite issue #133 in 000040 deprecation comment
cristim
added
triaged
cristim
added a commit
that referenced
this pull request
Apr 30, 2026
The AWS recommendations parser emits `sagemaker` as a distinct service (see providers/aws/recommendations/parser_sp.go:153, mapping to SagemakerSp), but the Recommendations page Service filter dropdown omitted it — users could see SageMaker recommendations in the unfiltered list but could not narrow to them. Adds `<option value="sagemaker">SageMaker</option>` under the AWS optgroup in the static `<select id="service-filter">`, alphabetically positioned between `redshift` and `savingsplans`. The option participates automatically in provider-based show/hide via the existing `updateServiceFilterVisibility` (it iterates optgroups, no per-option wiring needed). Lambda is intentionally NOT added in this PR: - The AWS recommendations parser does not emit `service=lambda`. Lambda commitments are surfaced under the existing `savingsplans` (Compute Savings Plans) umbrella, which the dropdown already exposes. - SageMaker Savings Plans is a distinct AWS product from Compute Savings Plans (different commitment scope + pricing), which is why SageMaker warrants its own filter value while Lambda does not. - This decision is correct under both possible resolutions of the still-open PR #71 (separate purchasing-defaults cards) and the already-merged PR #53 (Savings Plans is the umbrella for SM/Lambda). Adds a regression test in `frontend/src/__tests__/html.test.ts` that parses the real index.html and asserts SageMaker is present under the AWS optgroup, alongside the existing optgroup-presence test. Verification: - `npx jest` — 1252/1252 pass across 35 suites - `npx tsc --noEmit` — clean - `npm run build` — webpack production build succeeds Closes #77
cristim
added a commit
that referenced
this pull request
Apr 30, 2026
The AWS recommendations parser emits `sagemaker` as a distinct service (see providers/aws/recommendations/parser_sp.go:153, mapping to SagemakerSp), but the Recommendations page Service filter dropdown omitted it — users could see SageMaker recommendations in the unfiltered list but could not narrow to them. Adds `<option value="sagemaker">SageMaker</option>` under the AWS optgroup in the static `<select id="service-filter">`, alphabetically positioned between `redshift` and `savingsplans`. The option participates automatically in provider-based show/hide via the existing `updateServiceFilterVisibility` (it iterates optgroups, no per-option wiring needed). Lambda is intentionally NOT added in this PR: - The AWS recommendations parser does not emit `service=lambda`. Lambda commitments are surfaced under the existing `savingsplans` (Compute Savings Plans) umbrella, which the dropdown already exposes. - SageMaker Savings Plans is a distinct AWS product from Compute Savings Plans (different commitment scope + pricing), which is why SageMaker warrants its own filter value while Lambda does not. - This decision is correct under both possible resolutions of the still-open PR #71 (separate purchasing-defaults cards) and the already-merged PR #53 (Savings Plans is the umbrella for SM/Lambda). Adds a regression test in `frontend/src/__tests__/html.test.ts` that parses the real index.html and asserts SageMaker is present under the AWS optgroup, alongside the existing optgroup-presence test. Verification: - `npx jest` — 1252/1252 pass across 35 suites - `npx tsc --noEmit` — clean - `npm run build` — webpack production build succeeds Closes #77
5 tasks
cristim
added a commit
that referenced
this pull request
May 3, 2026
#133) (#227) Migration 000040 (PR #123) split the legacy `(aws, savings-plans)` umbrella into four per-plan-type rows and copied PR #71's `(aws, sagemaker)` term/payment forward into the new `(aws, savings-plans-sagemaker)` row, but intentionally KEPT the deprecated `(aws, sagemaker)` row for one stable release behind a SQL deprecation comment so users mid-rollout would not lose their save. Now that the per-plan-type cards are canonical, this follow-up migration removes the deprecated row. Up: idempotent `DELETE FROM service_configs WHERE provider = 'aws' AND service = 'sagemaker'` — safe no-op on installations that never had PR #71's row or that already ran this migration. Down: emergency rollback recreates the row by INSERT...SELECT from `(aws, savings-plans-sagemaker)`, lossy by design (only the post-040 term/payment survive). `ON CONFLICT (provider, service) DO NOTHING` keeps an existing row untouched; an empty SELECT (no `savings-plans-sagemaker` row to source from) yields a no-op INSERT. Production-code grep confirms no callers reference the bare `'sagemaker'` service slug — all references are to the `savings-plans-sagemaker` / `savingsplans-sagemaker` slugs introduced by 000040 and consumed by `cmd/main.go`, `internal/purchase/execution.go`, and `pkg/common/types.go`. Updates `split_savingsplans_test.go` scenario 2 to reflect the new post-migration state: the `(aws, sagemaker)` row is now absent after running all migrations. The "sagemaker slot inherits from PR #71's row" assertion still holds because 000040 copies the values forward before 000045 deletes the source row.
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.
Settings → Purchasing previously rendered per-service term/payment cards for EC2, RDS, ElastiCache, OpenSearch, Redshift, and the umbrella Savings Plans, but had no cards for SageMaker or Lambda. Both services are significant cost-optimisation targets covered by Savings Plans (SageMaker SP and Compute SP respectively), so users could not pin a per-workload term/payment preference and SageMaker/Lambda recommendations fell back to global defaults only.
Changes
frontend/src/index.html: Two newservice-default-cards under the AWS fieldset —SageMaker Savings PlansandLambda Savings Plans. Each follows the existing EC2/RDS shape (Term: 1y/3y, Payment: NoUpfront/Partial/AllUpfront). Hint copy on the umbrella Savings Plans card now mentions the new per-service overrides.frontend/src/settings.ts: Two new entries inSERVICE_FIELDS(aws/sagemaker,aws/lambda). The save / load / dirty-tracking / sticky-save-bar wiring is already SERVICE_FIELDS-driven, so no other code changes were needed. Both services use AWS_PAYMENTS viacommitmentOptions.ts's_defaultAWS fallback (no service-level restriction — SageMaker and Lambda both accept all 3 payment options at both terms).frontend/src/__tests__/settings.test.ts: 6 new tests:index.htmlwith their term and payment selects.updateServiceConfig.no-upfrontselectable (regression-proof against a future over-cautious copy-paste of the RDS rule).Test plan
npx jest src/__tests__/settings.test.ts— 47/47 pass (including 6 new).npx jest(full frontend suite) — 1257/1257 pass across 35 suites.npx tsc --noEmit— clean.npm run build— webpack production build succeeds.merge-watchafter CI deploy.)Out of scope
/api/config/service/{provider}/{service}endpoint already accepts arbitrary alphanumeric service strings (internal/api/validation.go:230), so persistence works without backend changes.