From 223588ec59382ecc9b097e76dc19aa7275a20e82 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 20:55:27 +0200 Subject: [PATCH 1/2] feat(commitmentopts): wire SP keys into Validate + tests (closes #134) Fix stale comment in Validate that still listed Savings Plans as "not in the probe set" after PR #569 added SavingsPlansProber to DefaultProbers. Add four Validate tests covering the SP probe path: - SavingsPlansProbeHit: probe snapshot governs hit/miss for SP keys - SavingsPlansKeyMissingInSnapshot: absent key stays permissive - SavingsPlansErrNoDataFallback: cold-start never blocks saves - SavingsPlansProbeConsulted: end-to-end probe-first precedence Precedence: probe snapshot -> permissive fallback (ErrNoData or absent service key). The Validate logic was already correct post #569; this commit closes the test coverage gap and removes the misleading comment. --- internal/commitmentopts/service.go | 11 ++- internal/commitmentopts/service_test.go | 106 +++++++++++++++++++++++- 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/internal/commitmentopts/service.go b/internal/commitmentopts/service.go index ce3a1fda1..66fa7f845 100644 --- a/internal/commitmentopts/service.go +++ b/internal/commitmentopts/service.go @@ -166,10 +166,13 @@ func (s *Service) Validate(ctx context.Context, provider, service string, term i // We have probe data for this provider but not for this service. // This typically means the service is not commitment-capable per // the probe (e.g. Savings Plans has no per-service offering list) - // or it is a new service not yet covered by the probe set. - // Log at Warn so operators can detect misconfigured service names - // or probe gaps without blocking the plan save (05-M4). The - // frontend's hardcoded rules are the primary user-facing gate. + // or it is a new service not yet covered by the probe set. The + // probe ran but returned no combos for this service key, so the + // permissive fallback keeps parity with ErrNoData and unknown + // services are never silently blocked. Log at Warn so operators + // can detect misconfigured service names or probe gaps without + // blocking the plan save (05-M4). The frontend's hardcoded rules + // are the primary user-facing gate. logging.Warnf("commitmentopts: provider %q known in probe data but service %q absent; treating as valid (term=%d payment=%q)", provider, service, term, payment) return true, nil diff --git a/internal/commitmentopts/service_test.go b/internal/commitmentopts/service_test.go index e3d01ae47..b9d7c53d9 100644 --- a/internal/commitmentopts/service_test.go +++ b/internal/commitmentopts/service_test.go @@ -241,12 +241,116 @@ func TestService_Validate_UnknownServiceUnderKnownProviderPermissive(t *testing. store := &fakeStore{opts: cached, has: true} svc := New(store, &fakeAccounts{}, noopBuildConfig, nil) - // savingsplans has no probe — don't block. + // A service key absent from the probe snapshot must not block saves. ok, err := svc.Validate(context.Background(), "aws", "savingsplans", 1, "all-upfront") require.NoError(t, err) assert.True(t, ok) } +// TestService_Validate_SavingsPlansProbeHit verifies that Validate consults +// the probe snapshot for SP service keys and rejects combinations that the +// probe did not return. Precedence: probe data wins over permissive fallback +// when the key is present in the snapshot. +func TestService_Validate_SavingsPlansProbeHit(t *testing.T) { + // Seed a snapshot that only has 1yr/all-upfront for savings-plans-compute. + cached := Options{"aws": { + "savings-plans-compute": { + {Provider: "aws", Service: "savings-plans-compute", TermYears: 1, Payment: "all-upfront"}, + {Provider: "aws", Service: "savings-plans-compute", TermYears: 1, Payment: "no-upfront"}, + }, + }} + store := &fakeStore{opts: cached, has: true} + svc := New(store, &fakeAccounts{}, noopBuildConfig, nil) + + // Probe returned this combo — must be valid. + ok, err := svc.Validate(context.Background(), "aws", "savings-plans-compute", 1, "all-upfront") + require.NoError(t, err) + assert.True(t, ok, "combo present in probe snapshot must validate") + + // Probe did NOT return 3yr/partial-upfront — must be rejected. + ok, err = svc.Validate(context.Background(), "aws", "savings-plans-compute", 3, "partial-upfront") + require.NoError(t, err) + assert.False(t, ok, "combo absent from probe snapshot must be rejected") +} + +// TestService_Validate_SavingsPlansKeyMissingInSnapshot verifies that +// Validate falls back to permissive-true when the probe ran successfully but +// returned no combos for a specific SP service key (e.g. a plan type not +// offered in the account's region). The presence of another SP key in the +// snapshot confirms the probe did run; the missing key means "no data for +// this plan type", not "probe not run". +func TestService_Validate_SavingsPlansKeyMissingInSnapshot(t *testing.T) { + // Probe ran and stored combos for savings-plans-compute only. + cached := Options{"aws": { + "savings-plans-compute": { + {Provider: "aws", Service: "savings-plans-compute", TermYears: 1, Payment: "all-upfront"}, + }, + }} + store := &fakeStore{opts: cached, has: true} + svc := New(store, &fakeAccounts{}, noopBuildConfig, nil) + + // savings-plans-database is absent from the snapshot — permissive fallback. + ok, err := svc.Validate(context.Background(), "aws", "savings-plans-database", 1, "all-upfront") + require.NoError(t, err) + assert.True(t, ok, "absent service key must not block saves (permissive fallback)") +} + +// TestService_Validate_SavingsPlansErrNoDataFallback verifies that Validate +// returns permissive-true for SP service keys when no probe data exists at all +// (ErrNoData path). This covers the cold-start case where the probe has never +// run — saves must not be blocked. +func TestService_Validate_SavingsPlansErrNoDataFallback(t *testing.T) { + // Cold store: no probe run, no AWS account to probe. + store := &fakeStore{} + accounts := &fakeAccounts{accounts: nil} + svc := New(store, accounts, noopBuildConfig, nil) + + for _, spService := range []string{ + "savings-plans-compute", + "savings-plans-ec2instance", + "savings-plans-sagemaker", + "savings-plans-database", + } { + ok, err := svc.Validate(context.Background(), "aws", spService, 1, "all-upfront") + require.NoError(t, err, "service=%s", spService) + assert.True(t, ok, "ErrNoData must not block saves for %s", spService) + } +} + +// TestService_Validate_SavingsPlansProbeConsulted verifies the probe-first +// precedence for SP keys end-to-end: cold store + live SP prober mock +// -> probe fires -> Validate uses the probe result, not the permissive default. +// Precedence chain: probe data in snapshot -> permissive fallback (ErrNoData +// or absent key). +func TestService_Validate_SavingsPlansProbeConsulted(t *testing.T) { + store := &fakeStore{} + accounts := &fakeAccounts{accounts: []config.CloudAccount{awsAccount("123456789012")}} + + // SP prober returns only 1yr/no-upfront for savings-plans-compute. + spCombos := []Combo{ + {Provider: "aws", Service: "savings-plans-compute", TermYears: 1, Payment: "no-upfront"}, + } + probers := []Prober{ + &stubProber{name: "savings-plans", combos: spCombos}, + } + svc := New(store, accounts, noopBuildConfig, probers) + + // Trigger probe by calling Get (cold store). + opts, err := svc.Get(context.Background()) + require.NoError(t, err) + require.Contains(t, opts["aws"], "savings-plans-compute", + "probe result must land under its per-product service key") + + // The probe snapshot now governs Validate. + ok, err := svc.Validate(context.Background(), "aws", "savings-plans-compute", 1, "no-upfront") + require.NoError(t, err) + assert.True(t, ok, "1yr/no-upfront was returned by probe — must validate") + + ok, err = svc.Validate(context.Background(), "aws", "savings-plans-compute", 3, "all-upfront") + require.NoError(t, err) + assert.False(t, ok, "3yr/all-upfront was NOT returned by probe — must be rejected") +} + func TestService_Get_ConcurrentCallersProbeOnce(t *testing.T) { // Serialize probes via the mutex: N concurrent Get()s on a cold store // must result in exactly one Save. From 61e2fec5ddf21ea45a64d334e42c26826b7d5211 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 21:41:53 -0700 Subject: [PATCH 2/2] fix(commitmentopts): adopt SP-specific comment in Validate absent-service branch Keep the base operator-visible logging.Warnf (05-M4) and replace the generic probe comment with the SP-specific wording from fix/134-wave9: "a plan type not sold in the account's region". --- internal/commitmentopts/service.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/internal/commitmentopts/service.go b/internal/commitmentopts/service.go index 66fa7f845..95cdc14ea 100644 --- a/internal/commitmentopts/service.go +++ b/internal/commitmentopts/service.go @@ -163,16 +163,14 @@ func (s *Service) Validate(ctx context.Context, provider, service string, term i } combos, ok := byService[service] if !ok { - // We have probe data for this provider but not for this service. - // This typically means the service is not commitment-capable per - // the probe (e.g. Savings Plans has no per-service offering list) - // or it is a new service not yet covered by the probe set. The - // probe ran but returned no combos for this service key, so the - // permissive fallback keeps parity with ErrNoData and unknown + // We have data for this provider but not this service key. + // The probe ran but returned no combos for this service + // (e.g. a plan type not sold in the account's region). + // Permissive fallback keeps parity with ErrNoData so unknown // services are never silently blocked. Log at Warn so operators // can detect misconfigured service names or probe gaps without - // blocking the plan save (05-M4). The frontend's hardcoded rules - // are the primary user-facing gate. + // blocking the plan save (05-M4). The frontend's hardcoded + // rules are the primary user-facing gate. logging.Warnf("commitmentopts: provider %q known in probe data but service %q absent; treating as valid (term=%d payment=%q)", provider, service, term, payment) return true, nil