From df5f97b07d2ae999c32a74cc4a6fc1126af6ab11 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 18:46:22 +0200 Subject: [PATCH] fix(sp): fetch only the Savings Plan types requested by --include/--exclude-sp-types Each per-plan-type service queries exactly one plan type and the provider ignores IncludeSPTypes/ExcludeSPTypes for it, so the flags never narrowed the fetch. Filter the SP service list in determineServicesToProcess and reject unknown type names. Closes #2177 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_helpers.go | 72 ++++++++++++++++++++++++++++--- cmd/multi_service_helpers_test.go | 37 ++++++++++++++++ 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 19b69e469..a82e6a5d1 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -148,14 +148,74 @@ func applyCommonCoverage(recs []common.Recommendation, coverage float64) []commo // determineServicesToProcess returns the list of services to process based on flags. func determineServicesToProcess(cfg Config) ([]common.ServiceType, error) { - if cfg.AllServices { - return getAllServices(), nil + var services []common.ServiceType + switch { + case cfg.AllServices: + services = getAllServices() + case len(cfg.Services) > 0: + parsed, err := parseServices(cfg.Services) + if err != nil { + return nil, err + } + services = parsed + default: + // Default to RDS only for backward compatibility + return []common.ServiceType{common.ServiceRDS}, nil + } + return filterSavingsPlanTypes(services, cfg.IncludeSPTypes, cfg.ExcludeSPTypes) +} + +// spTypeServices maps the --include-sp-types / --exclude-sp-types names +// (matched case-insensitively) to the per-plan-type service slugs. +var spTypeServices = map[string]common.ServiceType{ + "compute": common.ServiceSavingsPlansCompute, + "ec2instance": common.ServiceSavingsPlansEC2Instance, + "sagemaker": common.ServiceSavingsPlansSageMaker, + "database": common.ServiceSavingsPlansDatabase, +} + +// spTypeSet resolves --include-sp-types / --exclude-sp-types names to services. +func spTypeSet(flag string, names []string) (map[common.ServiceType]bool, error) { + set := make(map[common.ServiceType]bool, len(names)) + for _, name := range names { + service, ok := spTypeServices[strings.ToLower(strings.TrimSpace(name))] + if !ok { + return nil, fmt.Errorf("unknown Savings Plan type %q in %s; valid types: Compute, EC2Instance, SageMaker, Database", name, flag) + } + set[service] = true } - if len(cfg.Services) > 0 { - return parseServices(cfg.Services) + return set, nil +} + +// filterSavingsPlanTypes drops the Savings Plan services that +// --include-sp-types / --exclude-sp-types rule out, so only the requested +// plan types are fetched. Each per-plan-type service queries exactly one plan +// type and the provider ignores the include/exclude lists for it, so the +// narrowing has to happen on the service list. Non-SP services pass through. +func filterSavingsPlanTypes(services []common.ServiceType, include, exclude []string) ([]common.ServiceType, error) { + includeSet, err := spTypeSet("--include-sp-types", include) + if err != nil { + return nil, err + } + excludeSet, err := spTypeSet("--exclude-sp-types", exclude) + if err != nil { + return nil, err + } + kept := make([]common.ServiceType, 0, len(services)) + for _, service := range services { + if common.IsSavingsPlan(service) && !spTypeWanted(service, includeSet, excludeSet) { + continue + } + kept = append(kept, service) + } + return kept, nil +} + +func spTypeWanted(service common.ServiceType, includeSet, excludeSet map[common.ServiceType]bool) bool { + if len(includeSet) > 0 && !includeSet[service] { + return false } - // Default to RDS only for backward compatibility - return []common.ServiceType{common.ServiceRDS}, nil + return !excludeSet[service] } // printRunMode prints the current run mode (dry run or purchase). diff --git a/cmd/multi_service_helpers_test.go b/cmd/multi_service_helpers_test.go index e5088a8c4..a67a19798 100644 --- a/cmd/multi_service_helpers_test.go +++ b/cmd/multi_service_helpers_test.go @@ -15,6 +15,7 @@ import ( orgtypes "github.com/aws/aws-sdk-go-v2/service/organizations/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestGetAllAWSRegions(t *testing.T) { @@ -857,3 +858,39 @@ func TestDetermineServicesToProcess_DefaultAndInvalid(t *testing.T) { assert.ErrorContains(t, err, "elasticahe") assert.Nil(t, result) } + +func TestDetermineServicesToProcess_SavingsPlanTypeFilters(t *testing.T) { + allSP := []common.ServiceType{ + common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance, + common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, + } + tests := []struct { + name string + cfg Config + want []common.ServiceType + wantErr string + }{ + {"no filter keeps all four", Config{Services: []string{"savingsplans"}}, allSP, ""}, + {"include narrows, case-insensitive", Config{Services: []string{"savingsplans"}, IncludeSPTypes: []string{"EC2Instance"}}, + []common.ServiceType{common.ServiceSavingsPlansEC2Instance}, ""}, + {"exclude drops", Config{Services: []string{"savingsplans"}, ExcludeSPTypes: []string{"sagemaker", "Database"}}, + []common.ServiceType{common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance}, ""}, + {"include then exclude", Config{Services: []string{"savingsplans"}, IncludeSPTypes: []string{"compute", "ec2instance"}, ExcludeSPTypes: []string{"compute"}}, + []common.ServiceType{common.ServiceSavingsPlansEC2Instance}, ""}, + {"non-SP services untouched", Config{Services: []string{"rds", "savingsplans"}, IncludeSPTypes: []string{"compute"}}, + []common.ServiceType{common.ServiceRDS, common.ServiceSavingsPlansCompute}, ""}, + {"unknown include fails loud", Config{Services: []string{"savingsplans"}, IncludeSPTypes: []string{"ec2"}}, nil, `unknown Savings Plan type "ec2"`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := determineServicesToProcess(tc.cfg) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +}