diff --git a/cmd/helpers_test.go b/cmd/helpers_test.go index afbc0084d..6546035ae 100644 --- a/cmd/helpers_test.go +++ b/cmd/helpers_test.go @@ -308,7 +308,7 @@ func TestApplyCoverage(t *testing.T) { name: "Savings Plans - reduces hourly commitment", recs: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, Count: 1, EstimatedSavings: 100, Details: &common.SavingsPlanDetails{ diff --git a/cmd/main_test.go b/cmd/main_test.go index 79fdbd7b5..cf8ba4b3d 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -241,12 +241,12 @@ func TestCreateServiceClient(t *testing.T) { expectNil: false, }, { - // Legacy umbrella slug is no longer dispatched to a client — the + // Umbrella sentinel is no longer dispatched to a client — the // per-plan-type slugs above carry the actual work. Keep the case // to lock the contract: createServiceClient returns nil for the // umbrella so the caller knows to fan out. - name: "Savings Plans umbrella service (legacy, returns nil)", - service: common.ServiceSavingsPlans, + name: "Savings Plans umbrella sentinel (returns nil)", + service: common.ServiceSavingsPlansAll, expectNil: true, }, { diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 317fff5d6..3d9b56dbc 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -58,7 +58,7 @@ func getServiceDisplayName(service common.ServiceType) string { // one-line change. func savingsPlanDisplayName(service common.ServiceType) (string, bool) { labels := map[common.ServiceType]string{ - common.ServiceSavingsPlans: "Savings Plans", + common.ServiceSavingsPlansAll: "Savings Plans", common.ServiceSavingsPlansCompute: "Compute Savings Plans", common.ServiceSavingsPlansEC2Instance: "EC2 Instance Savings Plans", common.ServiceSavingsPlansSageMaker: "SageMaker Savings Plans", diff --git a/cmd/multi_service_stats.go b/cmd/multi_service_stats.go index 4ef161865..4c0f79314 100644 --- a/cmd/multi_service_stats.go +++ b/cmd/multi_service_stats.go @@ -112,7 +112,7 @@ func separateAndAggregateStats(serviceStats map[common.ServiceType]ServiceProces for service, stats := range serviceStats { if common.IsSavingsPlan(service) { if spStats.Service == "" { - spStats.Service = common.ServiceSavingsPlans + spStats.Service = common.ServiceSavingsPlansAll } if stats.RegionsProcessed > spStats.RegionsProcessed { spStats.RegionsProcessed = stats.RegionsProcessed diff --git a/cmd/multi_service_stats_test.go b/cmd/multi_service_stats_test.go index 2af805ff5..d571ddd59 100644 --- a/cmd/multi_service_stats_test.go +++ b/cmd/multi_service_stats_test.go @@ -273,7 +273,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { name: "Prints Compute Savings Plans", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 500.0, Details: &common.SavingsPlanDetails{ PlanType: "Compute", @@ -282,7 +282,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, stats: ServiceProcessingStats{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, RecommendationsSelected: 1, TotalEstimatedSavings: 500.0, }, @@ -296,7 +296,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { name: "Prints EC2 Instance Savings Plans", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 300.0, Details: &common.SavingsPlanDetails{ PlanType: "EC2Instance", @@ -305,7 +305,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, stats: ServiceProcessingStats{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, RecommendationsSelected: 1, TotalEstimatedSavings: 300.0, }, @@ -318,7 +318,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { name: "Prints Database Savings Plans", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 400.0, Details: &common.SavingsPlanDetails{ PlanType: "Database", @@ -327,7 +327,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, stats: ServiceProcessingStats{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, RecommendationsSelected: 1, TotalEstimatedSavings: 400.0, }, @@ -340,7 +340,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { name: "Prints SageMaker Savings Plans", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 250.0, Details: &common.SavingsPlanDetails{ PlanType: "SageMaker", @@ -349,7 +349,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, stats: ServiceProcessingStats{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, RecommendationsSelected: 1, TotalEstimatedSavings: 250.0, }, @@ -362,7 +362,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { name: "Prints multiple SP types with recommendations", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 500.0, Details: &common.SavingsPlanDetails{ PlanType: "Compute", @@ -370,7 +370,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 600.0, Details: &common.SavingsPlanDetails{ PlanType: "EC2Instance", @@ -379,7 +379,7 @@ func TestPrintSavingsPlansSection(t *testing.T) { }, }, stats: ServiceProcessingStats{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, RecommendationsSelected: 2, TotalEstimatedSavings: 1100.0, }, @@ -416,7 +416,7 @@ func TestPrintComparisonSection(t *testing.T) { name: "Comparison with EC2 RIs and EC2 Instance SP", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 600.0, Details: &common.SavingsPlanDetails{ PlanType: "EC2Instance", @@ -440,7 +440,7 @@ func TestPrintComparisonSection(t *testing.T) { name: "Comparison with Database RIs and Database SP", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 800.0, Details: &common.SavingsPlanDetails{ PlanType: "Database", @@ -466,14 +466,14 @@ func TestPrintComparisonSection(t *testing.T) { name: "Compute SP better than EC2 Instance SP", recommendations: []common.Recommendation{ { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 500.0, Details: &common.SavingsPlanDetails{ PlanType: "EC2Instance", }, }, { - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, EstimatedSavings: 700.0, Details: &common.SavingsPlanDetails{ PlanType: "Compute", diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 7187e6232..d12c6f318 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -820,8 +820,8 @@ func TestApplyFilters_RegionFiltering(t *testing.T) { { name: "Savings Plans bypass region filter", recs: []common.Recommendation{ - {Region: "us-east-1", ResourceType: "ComputeSP", Count: 1, Service: common.ServiceSavingsPlans}, - {Region: "us-west-2", ResourceType: "ComputeSP", Count: 2, Service: common.ServiceSavingsPlans}, + {Region: "us-east-1", ResourceType: "ComputeSP", Count: 1, Service: common.ServiceSavingsPlansAll}, + {Region: "us-west-2", ResourceType: "ComputeSP", Count: 2, Service: common.ServiceSavingsPlansAll}, }, includeRegions: []string{}, excludeRegions: []string{}, diff --git a/internal/purchase/coverage_extra_test.go b/internal/purchase/coverage_extra_test.go index 078145fc4..cf4768f35 100644 --- a/internal/purchase/coverage_extra_test.go +++ b/internal/purchase/coverage_extra_test.go @@ -50,14 +50,14 @@ func TestMapServiceType_AllBranches(t *testing.T) { {"opensearch", common.ServiceOpenSearch}, {"redshift", common.ServiceRedshift}, {"memorydb", common.ServiceMemoryDB}, - {"savingsplans", common.ServiceSavingsPlans}, + {"savingsplans", common.ServiceSavingsPlansAll}, // Issue #85: the legacy hyphenated form is still accepted as a // backwards-compat alias so Lambda-scheduled purchase executions // persisted before the normalisation (rec.Service == "savings-plans" // in purchase_executions.recommendations JSONB) still map correctly // when re-executed. Once historical rows have aged out, the alias // arm can be dropped and this case should flip to ServiceType("savings-plans"). - {"savings-plans", common.ServiceSavingsPlans}, + {"savings-plans", common.ServiceSavingsPlansAll}, {"unknown-service", common.ServiceType("unknown-service")}, {"", common.ServiceType("")}, } @@ -1100,7 +1100,7 @@ func TestManager_ExecuteSinglePurchase_DetailsByService(t *testing.T) { // details" on legacy rows, re-introducing #453 for SP umbrella. name: "sp_legacy_umbrella", service: "savings-plans", - serviceType: common.ServiceSavingsPlans, + serviceType: common.ServiceSavingsPlansAll, region: "us-east-1", resource: "LegacySP", details: &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 0.75}, diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index a5a43b890..030019901 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1016,8 +1016,8 @@ func mapServiceSlug(service string) (common.ServiceType, bool) { // frontend-canonical normalisation. func mapSavingsPlansSlug(service string) (common.ServiceType, bool) { slugs := map[string]common.ServiceType{ - "savings-plans": common.ServiceSavingsPlans, - "savingsplans": common.ServiceSavingsPlans, + "savings-plans": common.ServiceSavingsPlansAll, + "savingsplans": common.ServiceSavingsPlansAll, "savings-plans-compute": common.ServiceSavingsPlansCompute, "savingsplans-compute": common.ServiceSavingsPlansCompute, "savings-plans-ec2instance": common.ServiceSavingsPlansEC2Instance, diff --git a/pkg/common/service_details_codec.go b/pkg/common/service_details_codec.go index 75a43f75e..482ce630f 100644 --- a/pkg/common/service_details_codec.go +++ b/pkg/common/service_details_codec.go @@ -121,14 +121,14 @@ func newDetailsForService(service string) (ServiceDetails, bool) { // AWS Savings Plans — all umbrella + per-plan-type slugs use // SavingsPlanDetails. The dash-free spellings are the canonical - // values of ServiceSavingsPlans / ServiceSavingsPlansCompute etc.; + // values of ServiceSavingsPlansAll / ServiceSavingsPlansCompute etc.; // "savings-plans" (with a dash) is the legacy umbrella alias that // internal/purchase/execution.go: mapSavingsPlansSlug still // recognises so purchase_executions JSONB rows persisted before // the rename in PR #94 still resolve. Recognising it here means a // legacy direct-execute approval still decodes against the right // type. - case string(ServiceSavingsPlans), + case string(ServiceSavingsPlansAll), string(ServiceSavingsPlansCompute), string(ServiceSavingsPlansEC2Instance), string(ServiceSavingsPlansSageMaker), diff --git a/pkg/common/service_details_codec_test.go b/pkg/common/service_details_codec_test.go index 37243fe51..5953786b7 100644 --- a/pkg/common/service_details_codec_test.go +++ b/pkg/common/service_details_codec_test.go @@ -143,7 +143,7 @@ func TestDecodeServiceDetailsFor_LegacyEmpty(t *testing.T) { string(ServiceEC2), string(ServiceRDS), string(ServiceElastiCache), - string(ServiceSavingsPlans), + string(ServiceSavingsPlansAll), string(ServiceSavingsPlansCompute), string(ServiceOpenSearch), string(ServiceRedshift), diff --git a/pkg/common/types.go b/pkg/common/types.go index f6df840c3..ae623049c 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -56,16 +56,21 @@ const ( // Savings/Commitments // - // ServiceSavingsPlans is the canonical umbrella identifier for AWS Savings - // Plans. The string value "savingsplans" (no hyphen) matches the frontend's - // identifier and the value persisted in service_configs.service / - // purchase_history.service so that direct comparisons - // (rec.Service == ServiceSavingsPlans) work without a normaliser. See - // issue #85 for the rationale (frontend chosen as canonical to avoid a - // SQL data migration). Code that needs to recognise pre-#85 persisted - // "savings-plans" rows (e.g. purchase_executions JSONB blobs) goes - // through the mapper in internal/purchase/execution.go. - ServiceSavingsPlans ServiceType = "savingsplans" // AWS Savings Plans (umbrella) + // ServiceSavingsPlansAll is the umbrella sentinel for all AWS Savings Plans. + // Its string value "savingsplans" (no hyphen) matches the frontend identifier + // and the value persisted in service_configs.service / purchase_history.service + // so that direct comparisons work without a normaliser. See issue #85 for the + // rationale (frontend chosen as canonical to avoid a SQL data migration). + // + // This is NOT a per-plan-type slug. The four granular forms are: + // ServiceSavingsPlansCompute, ServiceSavingsPlansEC2Instance, + // ServiceSavingsPlansSageMaker, ServiceSavingsPlansDatabase. + // Use SavingsPlansPlanTypes() to iterate over them in canonical order. + // + // Code that needs to recognise pre-#85 persisted "savings-plans" rows + // (e.g. purchase_executions JSONB blobs) goes through the mapper in + // internal/purchase/execution.go. + ServiceSavingsPlansAll ServiceType = "savingsplans" // umbrella sentinel for "any SP" // Per-plan-type Savings Plans slugs. Each maps 1:1 to an AWS // types.SupportedSavingsPlansType so users can configure term/payment @@ -100,15 +105,15 @@ func (s ServiceType) String() string { return string(s) } -// IsSavingsPlan reports whether s is any Savings Plans service slug — -// the legacy umbrella (ServiceSavingsPlans), any of the four per-plan-type +// IsSavingsPlan reports whether s is any Savings Plans service slug -- +// the umbrella sentinel (ServiceSavingsPlansAll), any of the four per-plan-type // constants, or the dash-free frontend spelling "savingsplans" that the API // handler stores verbatim without normalisation. Use it when code needs to // recognise the Savings Plans family irrespective of plan type (e.g., stats // aggregation, region-ignoring filters, display-name branching). func IsSavingsPlan(s ServiceType) bool { switch s { - case ServiceSavingsPlans, + case ServiceSavingsPlansAll, ServiceSavingsPlansCompute, ServiceSavingsPlansEC2Instance, ServiceSavingsPlansSageMaker, @@ -118,6 +123,20 @@ func IsSavingsPlan(s ServiceType) bool { return string(s) == "savingsplans" } +// SavingsPlansPlanTypes returns the four per-plan-type SP slugs in canonical +// iteration order (Compute -> EC2Instance -> SageMaker -> Database). Use this +// for any fan-out over "all SP plan types"; it makes the 4-way expansion +// visible at the call site instead of relying on the umbrella sentinel +// (ServiceSavingsPlansAll) to expand implicitly inside the rec collector. +func SavingsPlansPlanTypes() []ServiceType { + return []ServiceType{ + ServiceSavingsPlansCompute, + ServiceSavingsPlansEC2Instance, + ServiceSavingsPlansSageMaker, + ServiceSavingsPlansDatabase, + } +} + // CommitmentType represents different commitment types across clouds type CommitmentType string @@ -521,7 +540,7 @@ type SavingsPlanDetails struct { } func (d SavingsPlanDetails) GetServiceType() ServiceType { - return ServiceSavingsPlans + return ServiceSavingsPlansAll } func (d SavingsPlanDetails) GetDetailDescription() string { diff --git a/pkg/common/types_test.go b/pkg/common/types_test.go index 70109d35f..1949a8b61 100644 --- a/pkg/common/types_test.go +++ b/pkg/common/types_test.go @@ -41,9 +41,9 @@ func TestServiceType_String(t *testing.T) { {ServiceStorage, "storage"}, // Regression guard for issue #85: the frontend persists "savingsplans" // (no hyphen) and the Go constant must match so direct comparisons of - // rec.Service == ServiceSavingsPlans don't silently miss rows. If you + // rec.Service == ServiceSavingsPlansAll don't silently miss rows. If you // flip this back to "savings-plans" you also need a SQL migration. - {ServiceSavingsPlans, "savingsplans"}, + {ServiceSavingsPlansAll, "savingsplans"}, {ServiceSavingsPlansCompute, "savings-plans-compute"}, {ServiceSavingsPlansEC2Instance, "savings-plans-ec2instance"}, {ServiceSavingsPlansSageMaker, "savings-plans-sagemaker"}, @@ -73,7 +73,7 @@ func TestIsSavingsPlan(t *testing.T) { service ServiceType want bool }{ - {"legacy umbrella", ServiceSavingsPlans, true}, + {"umbrella sentinel", ServiceSavingsPlansAll, true}, {"compute", ServiceSavingsPlansCompute, true}, {"ec2 instance", ServiceSavingsPlansEC2Instance, true}, {"sagemaker", ServiceSavingsPlansSageMaker, true}, @@ -90,6 +90,27 @@ func TestIsSavingsPlan(t *testing.T) { } } +func TestSavingsPlansPlanTypes(t *testing.T) { + t.Parallel() + got := SavingsPlansPlanTypes() + + // Must return exactly 4 plan types. + assert.Len(t, got, 4) + + // Canonical order: Compute, EC2Instance, SageMaker, Database. + assert.Equal(t, []ServiceType{ + ServiceSavingsPlansCompute, + ServiceSavingsPlansEC2Instance, + ServiceSavingsPlansSageMaker, + ServiceSavingsPlansDatabase, + }, got) + + // Every returned slug must be recognised as a Savings Plan. + for _, st := range got { + assert.Truef(t, IsSavingsPlan(st), "IsSavingsPlan(%q) should be true", st) + } +} + func TestCommitmentType_String(t *testing.T) { t.Parallel() tests := []struct { @@ -311,7 +332,7 @@ func TestSavingsPlanDetails_GetServiceType(t *testing.T) { HourlyCommitment: 10.50, } - assert.Equal(t, ServiceSavingsPlans, details.GetServiceType()) + assert.Equal(t, ServiceSavingsPlansAll, details.GetServiceType()) } func TestSavingsPlanDetails_GetDetailDescription(t *testing.T) { diff --git a/providers/aws/provider.go b/providers/aws/provider.go index c2b2d5a13..e9bbec0a8 100644 --- a/providers/aws/provider.go +++ b/providers/aws/provider.go @@ -433,9 +433,9 @@ func (p *AWSProvider) GetSupportedServices() []common.ServiceType { common.ServiceSavingsPlansEC2Instance, common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, - // Legacy umbrella for backward compat with persisted records; + // Umbrella sentinel for backward compat with persisted records; // see the GetServiceClient case below for rationale. - common.ServiceSavingsPlans, + common.ServiceSavingsPlansAll, // Legacy service types for backward compatibility common.ServiceEC2, common.ServiceRDS, @@ -475,8 +475,8 @@ func (p *AWSProvider) GetServiceClient(ctx context.Context, service common.Servi common.ServiceSavingsPlansDatabase: pt, _ := savingsplans.PlanTypeForServiceType(service) return NewSavingsPlansClient(regionalCfg, pt), nil - case common.ServiceSavingsPlans: - // Legacy umbrella path for any persisted RecommendationRecord + case common.ServiceSavingsPlansAll: + // Umbrella sentinel path for any persisted RecommendationRecord // (or scheduled purchase) still tagged with the pre-split slug. // The constructor's empty plan type signals "umbrella mode" to // the SP client: GetExistingCommitments returns every plan type diff --git a/providers/aws/provider_test.go b/providers/aws/provider_test.go index 650caabcd..8753b1506 100644 --- a/providers/aws/provider_test.go +++ b/providers/aws/provider_test.go @@ -251,8 +251,8 @@ func TestAWSProvider_GetSupportedServices(t *testing.T) { assert.Contains(t, services, common.ServiceSavingsPlansEC2Instance) assert.Contains(t, services, common.ServiceSavingsPlansSageMaker) assert.Contains(t, services, common.ServiceSavingsPlansDatabase) - // Legacy umbrella SP slug for backward compat with persisted records. - assert.Contains(t, services, common.ServiceSavingsPlans) + // Umbrella SP sentinel for backward compat with persisted records. + assert.Contains(t, services, common.ServiceSavingsPlansAll) // Legacy types assert.Contains(t, services, common.ServiceEC2) assert.Contains(t, services, common.ServiceRDS) @@ -323,10 +323,10 @@ func TestAWSProvider_GetServiceClient_AllServiceTypes(t *testing.T) { {common.ServiceSavingsPlansEC2Instance, common.ServiceSavingsPlansEC2Instance}, {common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansSageMaker}, {common.ServiceSavingsPlansDatabase, common.ServiceSavingsPlansDatabase}, - // Legacy umbrella: GetServiceClient returns an SP client with + // Umbrella sentinel: GetServiceClient returns an SP client with // an empty plan-type filter (umbrella mode); GetServiceType // reports the umbrella slug, matching pre-split behaviour. - {common.ServiceSavingsPlans, common.ServiceSavingsPlans}, + {common.ServiceSavingsPlansAll, common.ServiceSavingsPlansAll}, } for _, tc := range testCases { diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 01257deb5..8ebefa07a 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -344,7 +344,7 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda return nil }) g.Go(func() error { - spRecs, spErr = c.GetRecommendationsForService(gctx, common.ServiceSavingsPlans) + spRecs, spErr = c.GetRecommendationsForService(gctx, common.ServiceSavingsPlansAll) return nil }) diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 63c45779c..a86557c1c 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -234,7 +234,7 @@ func TestGetRecommendations_SavingsPlans_Success(t *testing.T) { client := NewClientWithAPI(mockAPI, "us-east-1") params := common.RecommendationParams{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "partial-upfront", Term: "1yr", LookbackPeriod: "7d", diff --git a/providers/aws/recommendations/family_nu_test.go b/providers/aws/recommendations/family_nu_test.go index e24740698..ce4000db9 100644 --- a/providers/aws/recommendations/family_nu_test.go +++ b/providers/aws/recommendations/family_nu_test.go @@ -261,7 +261,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { // in the nonRDS slice for per-pool sizing. recs := []common.Recommendation{ {Service: common.ServiceEC2, CommitmentType: common.CommitmentReservedInstance, Count: 3}, - {Service: common.ServiceSavingsPlans, CommitmentType: common.CommitmentSavingsPlan, Count: 1}, + {Service: common.ServiceSavingsPlansAll, CommitmentType: common.CommitmentSavingsPlan, Count: 1}, { Service: common.ServiceRDS, CommitmentType: common.CommitmentReservedInstance, @@ -280,7 +280,7 @@ func TestApplyFamilyNUSizingRDS(t *testing.T) { require.Len(t, sized, 1, "only the RDS rec went through family-NU") require.Len(t, nonRDS, 2, "EC2 + SP recs left for per-pool sizing") assert.Equal(t, common.ServiceEC2, nonRDS[0].Service) - assert.Equal(t, common.ServiceSavingsPlans, nonRDS[1].Service) + assert.Equal(t, common.ServiceSavingsPlansAll, nonRDS[1].Service) }) t.Run("ProjectedCoverage is cumulative across recs in a family", func(t *testing.T) { @@ -379,7 +379,7 @@ func TestIsRDSRIRec(t *testing.T) { {"RelationalDB alias", common.Recommendation{Service: common.ServiceRelationalDB, CommitmentType: common.CommitmentReservedInstance}, true}, {"EC2 RI", common.Recommendation{Service: common.ServiceEC2, CommitmentType: common.CommitmentReservedInstance}, false}, {"RDS SP (impossible but defensive)", common.Recommendation{Service: common.ServiceRDS, CommitmentType: common.CommitmentSavingsPlan}, false}, - {"SP", common.Recommendation{Service: common.ServiceSavingsPlans, CommitmentType: common.CommitmentSavingsPlan}, false}, + {"SP", common.Recommendation{Service: common.ServiceSavingsPlansAll, CommitmentType: common.CommitmentSavingsPlan}, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 41b865eaf..9667fbadf 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -25,7 +25,7 @@ import ( // the per-plan-type split: each registered SP service makes its own // Cost Explorer call with its own term/payment defaults. // 2. Otherwise, fall back to the legacy IncludeSPTypes/ExcludeSPTypes -// filter mechanism (for callers passing the umbrella ServiceSavingsPlans +// filter mechanism (for callers passing the umbrella ServiceSavingsPlansAll // slug or for direct CLI invocations that haven't been migrated yet). func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { planTypes := planTypesForParams(params) @@ -305,7 +305,7 @@ func planTypesForParams(params common.RecommendationParams) []types.SupportedSav // planTypeForServiceSlug maps a per-plan-type SP service slug to its // Cost Explorer plan-type enum. Returns false for non-SP slugs and for the -// legacy umbrella ServiceSavingsPlans (which still triggers the iterate-all +// umbrella sentinel ServiceSavingsPlansAll (which still triggers the iterate-all // fallback inside planTypesForParams). func planTypeForServiceSlug(s common.ServiceType) (types.SupportedSavingsPlansType, bool) { switch s { @@ -323,7 +323,7 @@ func planTypeForServiceSlug(s common.ServiceType) (types.SupportedSavingsPlansTy // serviceSlugForPlanType is the inverse of planTypeForServiceSlug. Used by // parseSavingsPlanDetail to tag each Recommendation with the per-plan-type -// slug rather than the legacy umbrella, so downstream stats/filters can +// slug rather than the umbrella sentinel, so downstream stats/filters can // distinguish Compute SP from SageMaker SP recommendations. func serviceSlugForPlanType(pt types.SupportedSavingsPlansType) common.ServiceType { switch pt { @@ -336,7 +336,7 @@ func serviceSlugForPlanType(pt types.SupportedSavingsPlansType) common.ServiceTy case types.SupportedSavingsPlansTypeDatabaseSp: return common.ServiceSavingsPlansDatabase } - return common.ServiceSavingsPlans + return common.ServiceSavingsPlansAll } // getFilteredPlanTypes returns the list of Savings Plan types to query based diff --git a/providers/aws/recommendations/parser_sp_additional_test.go b/providers/aws/recommendations/parser_sp_additional_test.go index cbe28a128..bd7637c43 100644 --- a/providers/aws/recommendations/parser_sp_additional_test.go +++ b/providers/aws/recommendations/parser_sp_additional_test.go @@ -277,7 +277,7 @@ func TestGetSavingsPlansRecommendations_WithFilters(t *testing.T) { client := NewClientWithAPI(mockAPI, "us-east-1") params := common.RecommendationParams{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "partial-upfront", Term: "1yr", LookbackPeriod: "7d", @@ -294,7 +294,7 @@ func TestGetSavingsPlansRecommendations_EmptyFilters(t *testing.T) { client := &Client{} params := common.RecommendationParams{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "partial-upfront", Term: "1yr", LookbackPeriod: "7d", diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 4cea975ee..6e6a0096e 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -75,7 +75,7 @@ func ServiceTypeForPlanType(pt types.SavingsPlanType) common.ServiceType { case types.SavingsPlanTypeDatabase: return common.ServiceSavingsPlansDatabase } - return common.ServiceSavingsPlans + return common.ServiceSavingsPlansAll } // PlanTypeForServiceType is the inverse mapping: a common.ServiceType slug to @@ -117,8 +117,8 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // The provider registers four SP services and calls GetExistingCommitments // on each; without filtering, every SP commitment would surface four times. // - // An empty planType signals legacy umbrella mode (the - // `case common.ServiceSavingsPlans` branch in provider.go's + // An empty planType signals umbrella sentinel mode (the + // `case common.ServiceSavingsPlansAll` branch in provider.go's // GetServiceClient): in that mode, return every commitment unfiltered // to match pre-split behaviour. Per-plan-type clients still partition. commitments := make([]common.Commitment, 0) diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index e52191a1e..de311a9f8 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -220,7 +220,7 @@ func TestClient_GetExistingCommitments(t *testing.T) { // TestClient_GetExistingCommitments_UmbrellaMode verifies that an SP client // constructed with an empty plan type (legacy umbrella mode, used by the -// AWS provider's `case common.ServiceSavingsPlans` branch in GetServiceClient) +// AWS provider's `case common.ServiceSavingsPlansAll` branch in GetServiceClient) // returns every commitment unfiltered — matching pre-issue-#22-split // behaviour for any persisted RecommendationRecord still tagged with the // umbrella slug. @@ -411,7 +411,7 @@ func TestClient_ValidateOffering(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", PaymentOption: "all-upfront", Term: "1yr", @@ -440,7 +440,7 @@ func TestClient_ValidateOffering_InvalidDetails(t *testing.T) { // Use ComputeDetails instead of SavingsPlanDetails to test type assertion failure rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, Details: common.ComputeDetails{ InstanceType: "t3.micro", }, @@ -459,7 +459,7 @@ func TestClient_PurchaseCommitment(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1, PaymentOption: "all-upfront", @@ -503,7 +503,7 @@ func TestClient_PurchaseCommitment_SetsClientTokenForIdempotency(t *testing.T) { client := &Client{client: mockSP, region: "us-east-1"} rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1, PaymentOption: "all-upfront", @@ -545,7 +545,7 @@ func TestClient_PurchaseCommitment_NoClientTokenWhenUnset(t *testing.T) { client := &Client{client: mockSP, region: "us-east-1"} rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1, PaymentOption: "all-upfront", @@ -577,7 +577,7 @@ func TestClient_PurchaseCommitment_InvalidDetails(t *testing.T) { // Use ComputeDetails instead of SavingsPlanDetails to test type assertion failure rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, Details: common.ComputeDetails{ InstanceType: "t3.micro", }, @@ -598,7 +598,7 @@ func TestClient_PurchaseCommitment_OfferingNotFound(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -628,7 +628,7 @@ func TestClient_PurchaseCommitment_CreateFails(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -663,7 +663,7 @@ func TestClient_PurchaseCommitment_EmptyResponse(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -700,7 +700,7 @@ func TestClient_GetOfferingDetails(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", PaymentOption: "all-upfront", Term: "1yr", @@ -741,7 +741,7 @@ func TestClient_GetOfferingDetails_3YearTerm(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "EC2Instance", PaymentOption: "partial-upfront", Term: "3yr", @@ -781,7 +781,7 @@ func TestClient_GetOfferingDetails_NoUpfront(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", PaymentOption: "no-upfront", Term: "1yr", @@ -819,7 +819,7 @@ func TestClient_GetOfferingDetails_InvalidDetails(t *testing.T) { // Use ComputeDetails instead of SavingsPlanDetails to test type assertion failure rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, Details: common.ComputeDetails{ InstanceType: "t3.micro", }, @@ -840,7 +840,7 @@ func TestClient_GetOfferingDetails_RatesError(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -890,7 +890,7 @@ func TestClient_FindOfferingID_AllPlanTypes(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -951,7 +951,7 @@ func TestClient_FindOfferingID_AllPaymentOptions(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: tt.paymentOption, Term: "1yr", Details: &common.SavingsPlanDetails{ @@ -1007,7 +1007,7 @@ func TestClient_FindOfferingID_TermVariations(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: tt.term, Details: &common.SavingsPlanDetails{ @@ -1045,7 +1045,7 @@ func TestClient_FindOfferingID_APIError(t *testing.T) { } rec := common.Recommendation{ - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, PaymentOption: "all-upfront", Term: "1yr", Details: &common.SavingsPlanDetails{ diff --git a/providers/azure/provider.go b/providers/azure/provider.go index 2817e7120..555c78145 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -432,7 +432,7 @@ func (p *AzureProvider) GetSupportedServices() []common.ServiceType { common.ServiceNoSQL, common.ServiceCache, common.ServiceMemoryDB, - common.ServiceSavingsPlans, + common.ServiceSavingsPlansAll, common.ServiceSearch, common.ServiceDataWarehouse, } @@ -490,7 +490,7 @@ func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceTy return NewCosmosDBClient(p.cred, subscriptionID, region), nil case common.ServiceMemoryDB: return NewManagedRedisClient(p.cred, subscriptionID, region), nil - case common.ServiceSavingsPlans: + case common.ServiceSavingsPlansAll: return NewSavingsPlansClient(p.cred, subscriptionID, region), nil case common.ServiceSearch: return NewSearchClient(p.cred, subscriptionID, region), nil diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index 432c9d86c..0296264ef 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -265,7 +265,7 @@ func TestAzureProvider_GetSupportedServices(t *testing.T) { assert.Contains(t, services, common.ServiceNoSQL) assert.Contains(t, services, common.ServiceCache) assert.Contains(t, services, common.ServiceMemoryDB) - assert.Contains(t, services, common.ServiceSavingsPlans) + assert.Contains(t, services, common.ServiceSavingsPlansAll) assert.Contains(t, services, common.ServiceSearch) assert.Contains(t, services, common.ServiceDataWarehouse) } @@ -414,7 +414,7 @@ func TestAzureProvider_GetServiceClient_AllServiceTypes(t *testing.T) { {common.ServiceNoSQL}, {common.ServiceCache}, {common.ServiceMemoryDB}, - {common.ServiceSavingsPlans}, + {common.ServiceSavingsPlansAll}, {common.ServiceSearch}, {common.ServiceDataWarehouse}, } @@ -1162,7 +1162,7 @@ func TestAzureProvider_GetServiceClientForAccount(t *testing.T) { common.ServiceNoSQL, common.ServiceCache, common.ServiceMemoryDB, - common.ServiceSavingsPlans, + common.ServiceSavingsPlansAll, common.ServiceSearch, common.ServiceDataWarehouse, } diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 8fe3719cd..560e524e6 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -134,7 +134,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // The call returns an empty slice so the service appears in the fan-out // and will start returning data once the API stabilises without requiring // a scheduler change. - if shouldIncludeService(params, common.ServiceSavingsPlans) { + if shouldIncludeService(params, common.ServiceSavingsPlansAll) { goService(&spErr, func() { spClient := savingsplans.NewClient(r.cred, r.subscriptionID, "") spRecs, spErr = spClient.GetRecommendations(gctx, params) diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index efdca18dc..833d3e2c4 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -218,8 +218,8 @@ func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { } // TestGetRecommendations_SavingsPlansServiceIncluded pins that shouldIncludeService -// allows ServiceSavingsPlans through both when params.Service is empty (all-services -// sweep) and when explicitly set to ServiceSavingsPlans, and does not include it +// allows ServiceSavingsPlansAll through both when params.Service is empty (all-services +// sweep) and when explicitly set to ServiceSavingsPlansAll, and does not include it // when a different service is requested. This ensures the SP goroutine added to the // fan-out in GetRecommendations is exercised on every scheduler collection run. func TestGetRecommendations_SavingsPlansServiceIncluded(t *testing.T) { @@ -235,7 +235,7 @@ func TestGetRecommendations_SavingsPlansServiceIncluded(t *testing.T) { }, { name: "explicit savingsplans service is included", - params: common.RecommendationParams{Service: common.ServiceSavingsPlans}, + params: common.RecommendationParams{Service: common.ServiceSavingsPlansAll}, expected: true, }, { @@ -246,7 +246,7 @@ func TestGetRecommendations_SavingsPlansServiceIncluded(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := shouldIncludeService(tt.params, common.ServiceSavingsPlans) + got := shouldIncludeService(tt.params, common.ServiceSavingsPlansAll) assert.Equal(t, tt.expected, got) }) } @@ -431,7 +431,7 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { dbRec := mkRec(common.ServiceRelationalDB, "database") cacheRec := mkRec(common.ServiceCache, "cache") cosmosRec := mkRec(common.ServiceNoSQL, "cosmosdb") - spRec := mkRec(common.ServiceSavingsPlans, "savingsplans") + spRec := mkRec(common.ServiceSavingsPlansAll, "savingsplans") advisorRec := mkRec(common.ServiceCompute, "advisor") // Advisor produces Compute recs // Replicate the exact call order from GetRecommendations. diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 8d98cf6b3..15db11bc5 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -112,7 +112,7 @@ func (c *Client) SetRPValidateClient(api RPValidateAPI) { // GetServiceType returns the service type for this client. func (c *Client) GetServiceType() common.ServiceType { - return common.ServiceSavingsPlans + return common.ServiceSavingsPlansAll } // GetRegion returns the region for this client. @@ -174,7 +174,7 @@ func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID Account: subscriptionID, CommitmentID: *sp.ID, CommitmentType: common.CommitmentSavingsPlan, - Service: common.ServiceSavingsPlans, + Service: common.ServiceSavingsPlansAll, Count: 1, State: "active", } diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index aa0b624e0..1ce975968 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -99,7 +99,7 @@ func TestNewClient(t *testing.T) { func TestGetServiceType(t *testing.T) { c := NewClient(nil, "sub", "eastus") - assert.Equal(t, common.ServiceSavingsPlans, c.GetServiceType()) + assert.Equal(t, common.ServiceSavingsPlansAll, c.GetServiceType()) } func TestGetRegion(t *testing.T) { @@ -174,7 +174,7 @@ func TestGetExistingCommitments_Happy(t *testing.T) { got := commitments[0] assert.Equal(t, common.ProviderAzure, got.Provider) assert.Equal(t, common.CommitmentSavingsPlan, got.CommitmentType) - assert.Equal(t, common.ServiceSavingsPlans, got.Service) + assert.Equal(t, common.ServiceSavingsPlansAll, got.Service) assert.Equal(t, spID, got.CommitmentID) assert.Equal(t, spName, got.ResourceType) assert.Equal(t, "Succeeded", got.State)