From 12801fe05301d394080b96d8893dfb8f9c0e1542 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 16:18:39 +0200 Subject: [PATCH 1/5] fix(cmd): collect account-level Savings Plans once regardless of --regions determineRegionsForService returned the configured --regions list before checking for Savings Plans, so each account-level plan was collected once per listed region. Once Savings Plan purchases work that is an N-fold real purchase. Check IsSavingsPlan first and add a pre-flight error for --input-csv files that already repeat a Savings Plan row, run before the confirmation and on dry runs too. Refs #2155 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service.go | 4 ++ cmd/multi_service_helpers.go | 18 ++++-- cmd/savings_plan_purchase.go | 47 ++++++++++++++++ cmd/savings_plan_purchase_test.go | 92 +++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 cmd/savings_plan_purchase.go create mode 100644 cmd/savings_plan_purchase_test.go diff --git a/cmd/multi_service.go b/cmd/multi_service.go index b00c0f328..6550e4cab 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -566,6 +566,10 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa } AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs)) + if err = rejectDuplicateSavingsPlanRows(recs); err != nil { + return nil, aws.Config{}, "", err + } + recs, err = filterAndAdjustRecommendations(recs, csvModeCoverage, cfg) if err != nil { return nil, aws.Config{}, "", err diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index bd8d8c32b..d490afc2b 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -254,17 +254,23 @@ func executePurchase(ctx context.Context, rec common.Recommendation, region stri // determineRegionsForService determines which regions to process for a given service. func determineRegionsForService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, service common.ServiceType, configuredRegions []string) ([]string, error) { + // Savings Plans are account-level, not regional - only query once, even + // when --regions lists several. The recommendation call returns the same + // account-wide set for every region, so honoring --regions here repeats + // each Savings Plan once per listed region and, now that Savings Plan + // purchases work, would buy it that many times. EC2Instance plans are + // still narrowed to the requested regions by the region filters on + // Details.Region. + if common.IsSavingsPlan(service) { + AppLogger.Printf("🌍 Fetching account-level Savings Plans recommendations...\n") + return []string{spAPIRegion}, nil // Single query for account-level data + } + // If regions are explicitly configured, use those if len(configuredRegions) > 0 { return configuredRegions, nil } - // Savings Plans are account-level, not regional - only query once - if common.IsSavingsPlan(service) { - AppLogger.Printf("🌍 Fetching account-level Savings Plans recommendations...\n") - return []string{"us-east-1"}, nil // Single query for account-level data - } - // Default to all AWS regions for other services AppLogger.Printf("🌍 Processing all AWS regions for %s...\n", getServiceDisplayName(service)) allRegions, err := getAllAWSRegions(ctx, awsCfg) diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go new file mode 100644 index 000000000..d5fb0b783 --- /dev/null +++ b/cmd/savings_plan_purchase.go @@ -0,0 +1,47 @@ +package main + +import ( + "fmt" + "strconv" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// spAPIRegion is the region the account-level Savings Plans API is called in. +// Savings Plans has a single endpoint, so every Savings Plan call (collection +// and purchase) uses this region regardless of the recommendation's own +// region. It is the commercial-partition value; GovCloud and China use other +// regions and are not supported for Savings Plan purchases. +const spAPIRegion = "us-east-1" + +// savingsPlanRowKey identifies one Savings Plan purchase. Two rows with the +// same key buy the same plan twice. +func savingsPlanRowKey(rec common.Recommendation) string { + key := fmt.Sprintf("%s|%s|%s|%s", rec.Service, rec.Account, rec.Term, rec.PaymentOption) + if d, ok := rec.Details.(*common.SavingsPlanDetails); ok && d != nil { + key += fmt.Sprintf("|%s|%s|%s|%s|%s", d.PlanType, d.InstanceFamily, d.Region, + strconv.FormatFloat(d.HourlyCommitment, 'f', -1, 64), d.OfferingID) + } + return key +} + +// rejectDuplicateSavingsPlanRows fails the run when --input-csv lists the same +// Savings Plan more than once, for example a CSV written by an older +// multi-region run that repeated the account-level plans per region. It runs +// before any confirmation or purchase, dry run included, so a duplicated file +// can never buy a plan twice. +func rejectDuplicateSavingsPlanRows(recs []common.Recommendation) error { + seen := make(map[string]int) + for i := range recs { + if !common.IsSavingsPlan(recs[i].Service) { + continue + } + key := savingsPlanRowKey(recs[i]) + if first, dup := seen[key]; dup { + return fmt.Errorf("duplicate Savings Plan rows %d and %d in CSV (service %s, account %q, term %q, payment %q): remove the repeat, it would be purchased twice", + first+1, i+1, recs[i].Service, recs[i].Account, recs[i].Term, recs[i].PaymentOption) + } + seen[key] = i + } + return nil +} diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go new file mode 100644 index 000000000..67bca9099 --- /dev/null +++ b/cmd/savings_plan_purchase_test.go @@ -0,0 +1,92 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDetermineRegionsForService_SavingsPlanCollectedOnceWithConfiguredRegions(t *testing.T) { + for _, svc := range []common.ServiceType{ + common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance, + common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, + } { + regions, err := determineRegionsForService(context.Background(), aws.Config{}, nil, svc, []string{"eu-west-1", "us-west-2", "ap-south-1"}) + require.NoError(t, err) + assert.Equal(t, []string{spAPIRegion}, regions, svc) + } +} + +func TestDetermineRegionsForService_ConfiguredRegionsKeptForRegionalServices(t *testing.T) { + regions, err := determineRegionsForService(context.Background(), aws.Config{}, nil, common.ServiceRDS, []string{"eu-west-1", "us-west-2"}) + require.NoError(t, err) + assert.Equal(t, []string{"eu-west-1", "us-west-2"}, regions) +} + +func spRow(commit float64) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, Account: "111111111111", Term: "1yr", PaymentOption: "no-upfront", + Details: &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: commit}, + } +} + +func TestRejectDuplicateSavingsPlanRows(t *testing.T) { + t.Run("identical rows rejected", func(t *testing.T) { + err := rejectDuplicateSavingsPlanRows([]common.Recommendation{spRow(1.5), spRow(1.5)}) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate Savings Plan rows 1 and 2") + }) + t.Run("differing commitment accepted", func(t *testing.T) { + assert.NoError(t, rejectDuplicateSavingsPlanRows([]common.Recommendation{spRow(1.5), spRow(1.6)})) + }) + t.Run("each key field distinguishes rows", func(t *testing.T) { + base := spRow(1.5) + mutate := map[string]func(*common.Recommendation){ + "account": func(r *common.Recommendation) { r.Account = "222222222222" }, + "term": func(r *common.Recommendation) { r.Term = "3yr" }, + "payment": func(r *common.Recommendation) { r.PaymentOption = "all-upfront" }, + "service": func(r *common.Recommendation) { r.Service = common.ServiceSavingsPlansDatabase }, + "plan": func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1.5} + }, + "family": func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1.5, InstanceFamily: "m5"} + }, + "region": func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1.5, Region: "eu-west-1"} + }, + "offer": func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1.5, OfferingID: "o-1"} + }, + } + for name, m := range mutate { + other := base + m(&other) + assert.NoError(t, rejectDuplicateSavingsPlanRows([]common.Recommendation{base, other}), name) + } + }) + t.Run("non Savings Plan repeats ignored", func(t *testing.T) { + r := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", Account: "1"} + assert.NoError(t, rejectDuplicateSavingsPlanRows([]common.Recommendation{r, r})) + }) +} + +func TestPrepareCSVPurchaseRun_DuplicateSavingsPlanRowsRejectedBeforeConfirmation(t *testing.T) { + csvPath := filepath.Join(t.TempDir(), "in.csv") + content := "Service,Region,ResourceType,Count,Account,Term,PaymentOption\n" + + "savings-plans-compute,,,1,111111111111,1yr,no-upfront\n" + + "savings-plans-compute,,,1,111111111111,1yr,no-upfront\n" + require.NoError(t, os.WriteFile(csvPath, []byte(content), 0o600)) + + for _, dry := range []bool{true, false} { + _, _, _, err := prepareCSVPurchaseRun(context.Background(), Config{CSVInput: csvPath, AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")}, 100, dry) + require.Error(t, err, "dryRun=%v", dry) + assert.Contains(t, err.Error(), "duplicate Savings Plan rows") + } +} From 885732870677f3bf34325470aac7e4927a3c7232 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 18:33:49 +0200 Subject: [PATCH 2/5] fix(cmd): build Savings Plan purchase clients in the Savings Plans API region purchaseSingleRec built the client with the recommendation's Region, which is empty for Savings Plans from the parser, and the --input-csv path built it from the "" group key, so every Savings Plan call failed with "Missing Region". The four Savings Plan services now always use the single Savings Plans API region and ignore the recommendation region; every other service still needs its own region and an empty one is an error. Purchase IDs and audit records label Savings Plans with Details.Region or "global". The region is the commercial-partition value; GovCloud and China are not supported for Savings Plan purchases. Refs #2155 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service.go | 26 ++++-- cmd/savings_plan_purchase.go | 39 ++++++++ cmd/savings_plan_purchase_test.go | 142 ++++++++++++++++++++++++++++++ 3 files changed, 200 insertions(+), 7 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 6550e4cab..8404ab972 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -481,22 +481,28 @@ func purchaseAuditStatus(result common.PurchaseResult) string { // purchaseSingleRec executes or dry-runs a single purchase and returns the result + audit status. func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recommendation, index int, isDryRun bool, cfg Config) (purchaseResult common.PurchaseResult, auditStatus string) { - AppLogger.Printf(" [%d] %s %s %s (count=%d)\n", index, rec.Service, rec.Region, rec.ResourceType, rec.Count) + label := purchaseRegionLabel(rec, rec.Region) + AppLogger.Printf(" [%d] %s %s %s (count=%d)\n", index, rec.Service, label, rec.ResourceType, rec.Count) if isDryRun { - result := createDryRunResult(rec, rec.Region, index, cfg) + result := createDryRunResult(rec, label, index, cfg) AppLogger.Printf(" [dry-run] %s\n", result.CommitmentID) return result, "skipped" } + clientRegion, err := clientRegionFor(rec.Service, rec.Region) + if err != nil { + AppLogger.Printf(" ❌ %v\n", err) + return common.PurchaseResult{Recommendation: rec, Success: false, Error: err, Timestamp: time.Now()}, "error" + } regionalCfg := awsCfg.Copy() - regionalCfg.Region = rec.Region + regionalCfg.Region = clientRegion serviceClient := createServiceClient(rec.Service, regionalCfg) if serviceClient == nil { AppLogger.Printf(" ⚠️ No service client for %s\n", rec.Service) return common.PurchaseResult{Success: false}, "error" } - result := executePurchase(ctx, rec, rec.Region, index, serviceClient, cfg) + result := executePurchase(ctx, rec, label, index, serviceClient, cfg) status := purchaseAuditStatus(result) if result.Success { AppLogger.Printf(" ✅ %s\n", result.CommitmentID) @@ -698,8 +704,13 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { // Extracted out of runToolFromCSV to keep it under the project's gocyclo // budget. func processCSVRegionPurchases(ctx context.Context, awsCfg aws.Config, service common.ServiceType, region string, recs []common.Recommendation, isDryRun bool, cfg Config, runID string) (processedRecs []common.Recommendation, results []common.PurchaseResult, ok bool) { + clientRegion, err := clientRegionFor(service, region) + if err != nil { + AppLogger.Printf(" ❌ Cannot purchase %d %s recommendation(s) from the CSV: %v\n", len(recs), getServiceDisplayName(service), err) + return nil, nil, false + } regionalCfg := awsCfg.Copy() - regionalCfg.Region = region + regionalCfg.Region = clientRegion serviceClient := createServiceClient(service, regionalCfg) if serviceClient == nil { @@ -852,17 +863,18 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi } rec := recs[j] + label := purchaseRegionLabel(rec, region) AppLogger.Printf(" [%d/%d] Processing: %s %s\n", j+1, len(recs), rec.Service, rec.ResourceType) AppLogger.Printf(" 💳 Purchasing %d instances\n", rec.Count) var result common.PurchaseResult var status string if isDryRun { - result = createDryRunResult(rec, region, j+1, cfg) + result = createDryRunResult(rec, label, j+1, cfg) status = "skipped" } else { // Execute actual purchase - result = executePurchase(ctx, rec, region, j+1, serviceClient, cfg) + result = executePurchase(ctx, rec, label, j+1, serviceClient, cfg) status = purchaseAuditStatus(result) // Add delay between purchases to avoid rate limiting diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index d5fb0b783..a19a93362 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -5,6 +5,7 @@ import ( "strconv" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/savingsplans" ) // spAPIRegion is the region the account-level Savings Plans API is called in. @@ -45,3 +46,41 @@ func rejectDuplicateSavingsPlanRows(recs []common.Recommendation) error { } return nil } + +// spGlobalLabel names the region in purchase IDs and audit records for an +// account-level Savings Plan that carries no region of its own. +const spGlobalLabel = "global" + +// clientRegionFor returns the region the AWS client for service must be built +// with. The four Savings Plan services always use spAPIRegion and ignore the +// recommendation's region: the Savings Plans API has one endpoint, and the +// region a plan is scoped to is passed separately as an offering filter from +// Details.Region. Every other service needs the recommendation's own region, +// and an empty one is an error rather than a client that fails later with +// "Missing Region". The umbrella ServiceSavingsPlansAll has no plan type and +// no client, so it gets an error instead of a silently chosen region. +func clientRegionFor(service common.ServiceType, recRegion string) (string, error) { + if common.IsSavingsPlan(service) { + if _, ok := savingsplans.PlanTypeForServiceType(service); !ok { + return "", fmt.Errorf("service %s is not a purchasable Savings Plan type", service) + } + return spAPIRegion, nil + } + if recRegion == "" { + return "", fmt.Errorf("recommendation for %s has no region", service) + } + return recRegion, nil +} + +// purchaseRegionLabel is the region shown in purchase IDs and audit records. +// For Savings Plans it is Details.Region (EC2Instance plans) or "global"; +// for everything else it is the region the recommendation was grouped under. +func purchaseRegionLabel(rec common.Recommendation, groupRegion string) string { + if !common.IsSavingsPlan(rec.Service) { + return groupRegion + } + if d, ok := rec.Details.(*common.SavingsPlanDetails); ok && d != nil && d.Region != "" { + return d.Region + } + return spGlobalLabel +} diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 67bca9099..d1c9191b7 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -2,8 +2,12 @@ package main import ( "context" + "io" + "net/http" "os" "path/filepath" + "strings" + "sync" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -90,3 +94,141 @@ func TestPrepareCSVPurchaseRun_DuplicateSavingsPlanRowsRejectedBeforeConfirmatio assert.Contains(t, err.Error(), "duplicate Savings Plan rows") } } + +// spStubTransport is an offline AWS RoundTripper (the library clones an +// *http.Client and keeps its Transport, so it must be wrapped in one): it records every request and +// answers DescribeSavingsPlans with an empty list and everything else with a +// non-retryable 400, so no call ever leaves the machine. +type spStubTransport struct { + mu sync.Mutex + requests []*http.Request +} + +func (s *spStubTransport) RoundTrip(r *http.Request) (*http.Response, error) { + s.mu.Lock() + s.requests = append(s.requests, r) + s.mu.Unlock() + status, body := http.StatusBadRequest, `{"__type":"ValidationException","message":"stubbed"}` + if strings.HasSuffix(r.URL.Path, "/DescribeSavingsPlans") { + status, body = http.StatusOK, `{"savingsPlans":[]}` + } + return &http.Response{ + StatusCode: status, + Header: http.Header{"Content-Type": []string{"application/x-amz-json-1.0"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: r, + }, nil +} + +func (s *spStubTransport) paths() []string { + s.mu.Lock() + defer s.mu.Unlock() + out := make([]string, 0, len(s.requests)) + for _, r := range s.requests { + out = append(out, r.URL.Path) + } + return out +} + +func (s *spStubTransport) signedInRegion(region string) bool { + s.mu.Lock() + defer s.mu.Unlock() + for _, r := range s.requests { + if !strings.Contains(r.Header.Get("Authorization"), "/"+region+"/") { + return false + } + } + return len(s.requests) > 0 +} + +func stubAWSConfig(t *testing.T) (aws.Config, *spStubTransport) { + t.Helper() + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + stub := &spStubTransport{} + return aws.Config{ + HTTPClient: &http.Client{Transport: stub}, + Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "AKIDEXAMPLE", SecretAccessKey: "secret"}, nil + }), + Retryer: func() aws.Retryer { return aws.NopRetryer{} }, + }, stub +} + +// parserShapedSP is a Savings Plan recommendation as the library parser builds +// it: provider set, top-level Region empty. +func parserShapedSP() common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansCompute, CommitmentType: common.CommitmentSavingsPlan, + Count: 1, Account: "111111111111", Term: "1yr", PaymentOption: "no-upfront", + Details: &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1.5}, + } +} + +func TestPurchaseSingleRec_SavingsPlanFromParserReachesAPIInSPRegion(t *testing.T) { + awsCfg, stub := stubAWSConfig(t) + cfg := Config{AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")} + + result, status := purchaseSingleRec(context.Background(), awsCfg, parserShapedSP(), 1, false, cfg) + + require.Error(t, result.Error) + assert.NotContains(t, result.Error.Error(), "Region", "client must have a region") + assert.Equal(t, "error", status) + assert.NotEmpty(t, stub.paths(), "the purchase must reach the API") + assert.True(t, stub.signedInRegion(spAPIRegion), "requests must be signed for %s", spAPIRegion) + assert.Contains(t, result.CommitmentID, "global") +} + +func TestProcessCSVRegionPurchases_SavingsPlanRowWithEmptyRegionAndProvider(t *testing.T) { + awsCfg, stub := stubAWSConfig(t) + cfg := Config{AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")} + // The real loader shape: no Provider, no Region (group key ""). + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansCompute, Count: 1, Account: "111111111111", Term: "1yr", PaymentOption: "no-upfront", + Details: &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1.5}, + } + + _, results, ok := processCSVRegionPurchases(context.Background(), awsCfg, rec.Service, "", []common.Recommendation{rec}, false, cfg, "run-1") + + require.True(t, ok) + require.Len(t, results, 1) + require.Error(t, results[0].Error) + assert.NotContains(t, results[0].Error.Error(), "Region") + assert.Contains(t, stub.paths(), "/DescribeSavingsPlansOfferings") + assert.True(t, stub.signedInRegion(spAPIRegion)) + assert.Contains(t, results[0].CommitmentID, "global") +} + +func TestClientRegionFor(t *testing.T) { + for _, svc := range []common.ServiceType{ + common.ServiceSavingsPlansCompute, common.ServiceSavingsPlansEC2Instance, + common.ServiceSavingsPlansSageMaker, common.ServiceSavingsPlansDatabase, + } { + for _, recRegion := range []string{"", "eu-west-1"} { + got, err := clientRegionFor(svc, recRegion) + require.NoError(t, err) + assert.Equal(t, spAPIRegion, got, "%s rec region %q", svc, recRegion) + } + } + got, err := clientRegionFor(common.ServiceRDS, "eu-west-1") + require.NoError(t, err) + assert.Equal(t, "eu-west-1", got) + + _, err = clientRegionFor(common.ServiceRDS, "") + assert.Error(t, err, "non Savings Plan service with no region fails loud") +} + +func TestClientRegionFor_SavingsPlansAllNeverGetsARegion(t *testing.T) { + for _, recRegion := range []string{"", "us-east-1"} { + _, err := clientRegionFor(common.ServiceSavingsPlansAll, recRegion) + assert.Error(t, err, "rec region %q", recRegion) + } + assert.Nil(t, createServiceClient(common.ServiceSavingsPlansAll, aws.Config{Region: spAPIRegion})) +} + +func TestPurchaseRegionLabel(t *testing.T) { + ec2sp := parserShapedSP() + ec2sp.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", Region: "eu-west-1"} + assert.Equal(t, "eu-west-1", purchaseRegionLabel(ec2sp, "")) + assert.Equal(t, "global", purchaseRegionLabel(parserShapedSP(), "")) + assert.Equal(t, "us-west-2", purchaseRegionLabel(common.Recommendation{Service: common.ServiceRDS}, "us-west-2")) +} From 54326a311c0e2a6d316ba456863ee581f736ffc0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 18:55:59 +0200 Subject: [PATCH 3/5] fix(cmd): make dry run check the same purchase preconditions as a real run The dry run marked every row as a success, including Savings Plan and EC2 rows that a real run rejects for missing Details. Both purchase entry points (purchaseSingleRec and the --input-csv processPurchaseLoop) now validate the preconditions first and record a failed result with audit status "error" instead of "skipped": a region for the client, Savings Plan details with a plan type and a positive hourly commitment (plus a region for EC2 Instance plans), and EC2 platform, tenancy and scope. Refs #2155 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service.go | 9 +++- cmd/multi_service_test.go | 4 +- cmd/savings_plan_purchase.go | 49 ++++++++++++++++++ cmd/savings_plan_purchase_test.go | 84 +++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 3 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 8404ab972..041197e82 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -483,6 +483,10 @@ func purchaseAuditStatus(result common.PurchaseResult) string { func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recommendation, index int, isDryRun bool, cfg Config) (purchaseResult common.PurchaseResult, auditStatus string) { label := purchaseRegionLabel(rec, rec.Region) AppLogger.Printf(" [%d] %s %s %s (count=%d)\n", index, rec.Service, label, rec.ResourceType, rec.Count) + if err := validatePurchasePreconditions(rec, rec.Region); err != nil { + AppLogger.Printf(" ❌ cannot purchase: %v\n", err) + return preconditionFailure(rec, err, isDryRun), "error" + } if isDryRun { result := createDryRunResult(rec, label, index, cfg) AppLogger.Printf(" [dry-run] %s\n", result.CommitmentID) @@ -869,7 +873,10 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi var result common.PurchaseResult var status string - if isDryRun { + if err := validatePurchasePreconditions(rec, region); err != nil { + result = preconditionFailure(rec, err, isDryRun) + status = "error" + } else if isDryRun { result = createDryRunResult(rec, label, j+1, cfg) status = "skipped" } else { diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 0c165221e..ce53e8e34 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1459,8 +1459,8 @@ func TestProcessPurchaseLoopActualPurchase(t *testing.T) { toolCfg.Coverage = 80.0 recs := []common.Recommendation{ - {Service: common.ServiceEC2, ResourceType: "t3.small", Count: 1, SourceRecommendation: "EC2 Test 1", EstimatedSavings: 100}, - {Service: common.ServiceEC2, ResourceType: "t3.medium", Count: 2, SourceRecommendation: "EC2 Test 2", EstimatedSavings: 200}, + {Service: common.ServiceEC2, ResourceType: "t3.small", Count: 1, SourceRecommendation: "EC2 Test 1", EstimatedSavings: 100, Details: &common.ComputeDetails{InstanceType: "t3.small", Platform: "Linux/UNIX", Tenancy: "default", Scope: "regional"}}, + {Service: common.ServiceEC2, ResourceType: "t3.medium", Count: 2, SourceRecommendation: "EC2 Test 2", EstimatedSavings: 200, Details: &common.ComputeDetails{InstanceType: "t3.medium", Platform: "Linux/UNIX", Tenancy: "default", Scope: "regional"}}, } mockClient := &MockServiceClient{} diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index a19a93362..80eab2452 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -3,6 +3,7 @@ package main import ( "fmt" "strconv" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/savingsplans" @@ -84,3 +85,51 @@ func purchaseRegionLabel(rec common.Recommendation, groupRegion string) string { } return spGlobalLabel } + +// validatePurchasePreconditions reports why rec cannot be purchased, using the +// same requirements the purchase path enforces, so a dry run predicts what a +// real run will do instead of reporting every row as a success. It checks only +// what the library would reject for a missing or malformed field; it does not +// call any API. region is the one the client will be built from: the +// recommendation's own on the main path, the CSV group's on the CSV path. +func validatePurchasePreconditions(rec common.Recommendation, region string) error { + if _, err := clientRegionFor(rec.Service, region); err != nil { + return err + } + switch { + case common.IsSavingsPlan(rec.Service): + return validateSavingsPlanPreconditions(rec) + case rec.Service == common.ServiceEC2: + d, ok := rec.Details.(*common.ComputeDetails) + if !ok || d == nil { + return fmt.Errorf("EC2 recommendation for %s has no compute details (platform, tenancy, scope)", rec.ResourceType) + } + if d.Platform == "" || d.Tenancy == "" || d.Scope == "" { + return fmt.Errorf("EC2 recommendation for %s is missing platform, tenancy or scope (got %q, %q, %q)", rec.ResourceType, d.Platform, d.Tenancy, d.Scope) + } + } + return nil +} + +func validateSavingsPlanPreconditions(rec common.Recommendation) error { + d, ok := rec.Details.(*common.SavingsPlanDetails) + if !ok || d == nil { + return fmt.Errorf("%s recommendation has no Savings Plan details (plan type, hourly commitment)", rec.Service) + } + if d.PlanType == "" { + return fmt.Errorf("%s recommendation is missing the plan type", rec.Service) + } + if d.HourlyCommitment <= 0 { + return fmt.Errorf("%s recommendation has no positive hourly commitment (got %v)", rec.Service, d.HourlyCommitment) + } + if rec.Service == common.ServiceSavingsPlansEC2Instance && d.Region == "" { + return fmt.Errorf("EC2 Instance Savings Plan needs a region to pick the offering") + } + return nil +} + +// preconditionFailure builds the failed result recorded for a row that cannot +// be purchased. +func preconditionFailure(rec common.Recommendation, err error, isDryRun bool) common.PurchaseResult { + return common.PurchaseResult{Recommendation: rec, Success: false, Error: err, DryRun: isDryRun, Timestamp: time.Now()} +} diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index d1c9191b7..7ce0c4e2c 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -232,3 +232,87 @@ func TestPurchaseRegionLabel(t *testing.T) { assert.Equal(t, "global", purchaseRegionLabel(parserShapedSP(), "")) assert.Equal(t, "us-west-2", purchaseRegionLabel(common.Recommendation{Service: common.ServiceRDS}, "us-west-2")) } + +func validEC2Rec() common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceEC2, Region: "us-east-1", ResourceType: "m5.large", Count: 1, + Details: &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "default", Scope: "regional"}, + } +} + +func validEC2InstanceSP() common.Recommendation { + rec := parserShapedSP() + rec.Service = common.ServiceSavingsPlansEC2Instance + rec.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1.5, InstanceFamily: "m5", Region: "eu-west-1"} + return rec +} + +func TestValidatePurchasePreconditions(t *testing.T) { + mut := func(base common.Recommendation, f func(*common.Recommendation)) common.Recommendation { + f(&base) + return base + } + ok := map[string]common.Recommendation{ + "compute sp": parserShapedSP(), + "ec2 instance sp": validEC2InstanceSP(), + "ec2": validEC2Rec(), + "rds": {Service: common.ServiceRDS, Region: "us-east-1"}, + } + for name, rec := range ok { + assert.NoError(t, validatePurchasePreconditions(rec, rec.Region), name) + } + bad := map[string]common.Recommendation{ + "sp nil details": mut(parserShapedSP(), func(r *common.Recommendation) { r.Details = nil }), + "sp wrong details": mut(parserShapedSP(), func(r *common.Recommendation) { r.Details = &common.ComputeDetails{} }), + "sp no plan type": mut(parserShapedSP(), func(r *common.Recommendation) { r.Details = &common.SavingsPlanDetails{HourlyCommitment: 1} }), + "sp zero commit": mut(parserShapedSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "Compute", OfferingID: "o-1"} + }), + "ec2 instance sp no region": mut(validEC2InstanceSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, InstanceFamily: "m5"} + }), + "sp all": mut(parserShapedSP(), func(r *common.Recommendation) { r.Service = common.ServiceSavingsPlansAll }), + "ec2 nil": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details = nil }), + "ec2 tenancy": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Tenancy = "" }), + "ec2 scope": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Scope = "" }), + "ec2 platform": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Platform = "" }), + "rds no region": {Service: common.ServiceRDS}, + } + for name, rec := range bad { + assert.Error(t, validatePurchasePreconditions(rec, rec.Region), name) + } +} + +func TestPurchaseSingleRec_DryRunReportsPreconditionFailure(t *testing.T) { + cfg := Config{AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")} + broken := parserShapedSP() + broken.Details = nil + + result, status := purchaseSingleRec(context.Background(), aws.Config{}, broken, 1, true, cfg) + assert.False(t, result.Success) + require.Error(t, result.Error) + assert.Equal(t, "error", status) + + result, status = purchaseSingleRec(context.Background(), aws.Config{}, parserShapedSP(), 1, true, cfg) + assert.True(t, result.Success) + assert.Equal(t, "skipped", status) + assert.Contains(t, result.CommitmentID, "global") +} + +func TestProcessPurchaseLoop_DryRunReportsPreconditionFailureOnCSVShapedRows(t *testing.T) { + cfg := Config{AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")} + noDetailsSP := common.Recommendation{Service: common.ServiceSavingsPlansCompute, Count: 1} + csvEC2 := validEC2Rec() + csvEC2.Details = &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX"} // loader leaves Tenancy/Scope blank + okSP := parserShapedSP() + + results := processPurchaseLoop(context.Background(), []common.Recommendation{noDetailsSP, okSP}, "", true, nil, cfg, "run-1") + require.Len(t, results, 2) + assert.False(t, results[0].Success) + assert.True(t, results[1].Success) + assert.Contains(t, results[1].CommitmentID, "global") + + results = processPurchaseLoop(context.Background(), []common.Recommendation{csvEC2}, "us-east-1", true, nil, cfg, "run-2") + require.Len(t, results, 1) + assert.False(t, results[0].Success) +} From ba889664f1d06b822f3ad8199432b1c1153c7b86 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 19:16:40 +0200 Subject: [PATCH 4/5] fix(cmd): round-trip Savings Plan and EC2 details through the CSV report The CSV writer dropped the fields a purchase needs and the reader rebuilt Details only for RDS, ElastiCache and EC2 rows with an Engine, so a Savings Plan row reached the purchase with nil Details and an EC2 row with no tenancy or scope. Append PlanType, HourlyCommitment, InstanceFamily, OfferingID, DetailsRegion, Tenancy and Scope columns (never reordering the existing ones) and rebuild the Details from them. HourlyCommitment is written at full precision. A malformed HourlyCommitment is a load error; a row without the columns keeps nil Details and is reported by the purchase preconditions. Refs #2155 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service_csv.go | 41 ++++---------- cmd/multi_service_csv_details.go | 91 +++++++++++++++++++++++++++++++ cmd/savings_plan_purchase_test.go | 85 +++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 30 deletions(-) create mode 100644 cmd/multi_service_csv_details.go diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 553747d23..672b436b4 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -123,36 +123,13 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati return rec, err } - // Reconstruct the service Details from the Engine/Deployment columns. The - // purchase path needs them: RDS findOfferingID rejects a rec with nil - // Details ("invalid service details for RDS"), and RI offerings are keyed - // by engine and Multi-AZ. This mirrors the writer side (extractEngine / - // extractDeployment emit DatabaseDetails / CacheDetails / ComputeDetails), - // so a CSV the tool wrote round-trips losslessly. Engine is stored in Cost - // Explorer format ("Aurora MySQL"); findOfferingID normalizes it. Guarded - // on a non-empty Engine so minimal CSVs and Savings Plans rows (no Engine - // column) keep their previous nil-Details behavior. - if engine := getCSVField(record, colIdx, "Engine"); engine != "" { - deployment := getCSVField(record, colIdx, "Deployment") - switch rec.Service { - case common.ServiceRDS, common.ServiceRelationalDB: - rec.Details = &common.DatabaseDetails{ - Engine: engine, - AZConfig: deployment, - InstanceClass: rec.ResourceType, - } - case common.ServiceElastiCache, common.ServiceCache: - rec.Details = &common.CacheDetails{ - Engine: engine, - NodeType: rec.ResourceType, - } - case common.ServiceEC2, common.ServiceCompute: - rec.Details = &common.ComputeDetails{ - InstanceType: rec.ResourceType, - Platform: engine, - } - } + // Details come from the Engine/Deployment columns and the appended detail + // columns (see csvDetails). + details, err := csvDetails(rec, record, colIdx) + if err != nil { + return rec, err } + rec.Details = details return rec, nil } @@ -249,6 +226,8 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string "CommitmentID", "Success", "Error", "Timestamp", "ExistingCoverage", "ProjectedCoverage", } + // Appended, never reordered: existing readers look columns up by name. + header = append(header, csvDetailColumns...) if err := writer.Write(header); err != nil { return fmt.Errorf("failed to write CSV header: %w", err) } @@ -298,6 +277,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string formatExistingCoverage(rec), formatPercentOrBlank(rec.ProjectedCoverage), } + row = append(row, detailCells(rec)...) if err := writer.Write(row); err != nil { return fmt.Errorf("failed to write CSV row: %w", err) } @@ -346,7 +326,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { if totalNU > 0 { nuCell = fmt.Sprintf("%g", totalNU) } - return []string{ + row := []string{ "TOTAL", "", "", "", "", "", // Service through Deployment "", "", // Instances, CoveredInstances fmt.Sprintf("%d", totalCount), nuCell, "", // Count, NormalizedUnits, RecommendedCount @@ -355,6 +335,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { "", "", "", "", // CommitmentID, Success, Error, Timestamp "", "", // ExistingCoverage, ProjectedCoverage } + return append(row, make([]string, len(csvDetailColumns))...) // detail columns do not aggregate } // formatIntOrBlank renders an int as its decimal string when non-zero, "" diff --git a/cmd/multi_service_csv_details.go b/cmd/multi_service_csv_details.go new file mode 100644 index 000000000..7c44e20ee --- /dev/null +++ b/cmd/multi_service_csv_details.go @@ -0,0 +1,91 @@ +package main + +import ( + "strconv" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/savingsplans" +) + +// csvDetailColumns are the report columns that carry the fields of a +// recommendation's polymorphic Details that Engine/Deployment cannot: Savings +// Plan identity and EC2 tenancy and scope. They are appended after the +// original columns so existing readers, which look columns up by name, keep +// working. The writer and csvDetails below must stay in step. +var csvDetailColumns = []string{"PlanType", "HourlyCommitment", "InstanceFamily", "OfferingID", "DetailsRegion", "Tenancy", "Scope"} + +// detailCells renders rec.Details for csvDetailColumns, in that order. +func detailCells(rec common.Recommendation) []string { + cells := make([]string, len(csvDetailColumns)) + switch d := rec.Details.(type) { + case *common.SavingsPlanDetails: + if d != nil { + cells[0] = d.PlanType + cells[1] = strconv.FormatFloat(d.HourlyCommitment, 'f', -1, 64) + cells[2] = d.InstanceFamily + cells[3] = d.OfferingID + cells[4] = d.Region + } + case *common.ComputeDetails: + if d != nil { + cells[5], cells[6] = d.Tenancy, d.Scope + } + case common.ComputeDetails: + cells[5], cells[6] = d.Tenancy, d.Scope + } + return cells +} + +// csvDetails reconstructs the service Details of a CSV row. The purchase path +// needs them: RDS findOfferingID rejects nil Details, Savings Plan purchases +// need the plan type and hourly commitment, and EC2 offerings are keyed by +// platform, tenancy and scope. This mirrors the writer (extractEngine, +// extractDeployment, detailCells), so a CSV the tool wrote round-trips. +// +// A row without the identifying columns (a minimal or older CSV) keeps nil +// Details; validatePurchasePreconditions then reports it rather than the +// loader inventing values. +func csvDetails(rec common.Recommendation, record []string, colIdx map[string]int) (common.ServiceDetails, error) { + if _, isSP := savingsplans.PlanTypeForServiceType(rec.Service); isSP { + return csvSavingsPlanDetails(record, colIdx) + } + engine := getCSVField(record, colIdx, "Engine") + if engine == "" { + return nil, nil + } + switch rec.Service { + case common.ServiceRDS, common.ServiceRelationalDB: + return &common.DatabaseDetails{ + Engine: engine, + AZConfig: getCSVField(record, colIdx, "Deployment"), + InstanceClass: rec.ResourceType, + }, nil + case common.ServiceElastiCache, common.ServiceCache: + return &common.CacheDetails{Engine: engine, NodeType: rec.ResourceType}, nil + case common.ServiceEC2, common.ServiceCompute: + return &common.ComputeDetails{ + InstanceType: rec.ResourceType, + Platform: engine, + Tenancy: getCSVField(record, colIdx, "Tenancy"), + Scope: getCSVField(record, colIdx, "Scope"), + }, nil + } + return nil, nil +} + +func csvSavingsPlanDetails(record []string, colIdx map[string]int) (common.ServiceDetails, error) { + planType := getCSVField(record, colIdx, "PlanType") + if planType == "" && getCSVField(record, colIdx, "HourlyCommitment") == "" { + return nil, nil + } + d := &common.SavingsPlanDetails{ + PlanType: planType, + InstanceFamily: getCSVField(record, colIdx, "InstanceFamily"), + OfferingID: getCSVField(record, colIdx, "OfferingID"), + Region: getCSVField(record, colIdx, "DetailsRegion"), + } + if err := parseCSVFloat(record, colIdx, "HourlyCommitment", &d.HourlyCommitment); err != nil { + return nil, err + } + return d, nil +} diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 7ce0c4e2c..17fcd09d6 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "encoding/csv" "io" "net/http" "os" @@ -9,6 +10,7 @@ import ( "strings" "sync" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/aws/aws-sdk-go-v2/aws" @@ -316,3 +318,86 @@ func TestProcessPurchaseLoop_DryRunReportsPreconditionFailureOnCSVShapedRows(t * require.Len(t, results, 1) assert.False(t, results[0].Success) } + +func TestCSVRoundTrip_SavingsPlanAndEC2DetailsSurviveWriteThenRead(t *testing.T) { + compute := parserShapedSP() + compute.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 0.123456} + ec2sp := validEC2InstanceSP() + ec2sp.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 12, InstanceFamily: "m5", Region: "eu-west-1", OfferingID: "off-1"} + ec2 := validEC2Rec() + ec2.Details = &common.ComputeDetails{InstanceType: "m5.large", Platform: "Linux/UNIX", Tenancy: "dedicated", Scope: "zonal"} + rds := common.Recommendation{Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.r6g.large", Count: 2, + Details: &common.DatabaseDetails{Engine: "Aurora MySQL", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}} + + in := []common.Recommendation{compute, ec2sp, ec2, rds} + var results []common.PurchaseResult + for _, r := range in { + results = append(results, common.PurchaseResult{Recommendation: r, Success: true, Timestamp: time.Now()}) + } + path := filepath.Join(t.TempDir(), "report.csv") + require.NoError(t, writeMultiServiceCSVReport(results, path)) + + out, err := loadRecommendationsFromCSV(path) + require.NoError(t, err) + require.Len(t, out, len(in)) + + byService := map[common.ServiceType]common.Recommendation{} + for _, r := range out { + byService[r.Service] = r + } + assert.Equal(t, compute.Details, byService[compute.Service].Details) + assert.Equal(t, ec2sp.Details, byService[ec2sp.Service].Details) + assert.Equal(t, ec2.Details, byService[ec2.Service].Details) + assert.Equal(t, rds.Details, byService[rds.Service].Details) + for _, r := range out { + assert.NoError(t, validatePurchasePreconditions(r, r.Region), "%s", r.Service) + } +} + +func TestCSVWriter_AppendsDetailColumnsWithoutReorderingExistingOnes(t *testing.T) { + path := filepath.Join(t.TempDir(), "report.csv") + require.NoError(t, writeMultiServiceCSVReport([]common.PurchaseResult{{Recommendation: parserShapedSP(), Timestamp: time.Now()}}, path)) + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + rows, err := csv.NewReader(f).ReadAll() + require.NoError(t, err) + header := rows[0] + n := len(header) - len(csvDetailColumns) + assert.Equal(t, csvDetailColumns, header[n:]) + assert.Equal(t, "Service", header[0]) + assert.Equal(t, "ProjectedCoverage", header[n-1]) + for _, row := range rows { + assert.Len(t, row, len(header)) + } +} + +func TestCSVReader_MalformedHourlyCommitmentRejected(t *testing.T) { + path := filepath.Join(t.TempDir(), "in.csv") + content := "Service,Region,Count,PlanType,HourlyCommitment\nsavings-plans-compute,,1,Compute,abc\n" + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + _, err := loadRecommendationsFromCSV(path) + require.Error(t, err) + assert.Contains(t, err.Error(), "HourlyCommitment") +} + +func TestProcessCSVRegionPurchases_SavingsPlanRowFromWrittenCSV(t *testing.T) { + awsCfg, stub := stubAWSConfig(t) + cfg := Config{AuditLog: filepath.Join(t.TempDir(), "audit.jsonl")} + path := filepath.Join(t.TempDir(), "report.csv") + sp := parserShapedSP() + require.NoError(t, writeMultiServiceCSVReport([]common.PurchaseResult{{Recommendation: sp, Timestamp: time.Now()}}, path)) + recs, err := loadRecommendationsFromCSV(path) + require.NoError(t, err) + require.Len(t, recs, 1) + require.Empty(t, recs[0].Region) + require.Empty(t, recs[0].Provider) + + _, results, ok := processCSVRegionPurchases(context.Background(), awsCfg, recs[0].Service, recs[0].Region, recs, false, cfg, "run-1") + + require.True(t, ok) + require.Len(t, results, 1) + require.Error(t, results[0].Error) + assert.NotContains(t, results[0].Error.Error(), "Details", "details must reach the client") + assert.Contains(t, stub.paths(), "/DescribeSavingsPlansOfferings") +} From cf35a09559099ffaf1740f693d486c6965d8ca29 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 20:07:16 +0200 Subject: [PATCH 5/5] fix(cmd): satisfy lint in the Savings Plan changes Use slices.Concat for the CSV header and rows, preallocate the test result slice, and avoid the if-init err reassignment. Refs #2155 Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service.go | 3 ++- cmd/multi_service_csv.go | 13 +++++++------ cmd/savings_plan_purchase_test.go | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 041197e82..8a8091308 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -576,7 +576,8 @@ func prepareCSVPurchaseRun(ctx context.Context, cfg Config, csvModeCoverage floa } AppLogger.Printf("✅ Loaded %d recommendations from CSV\n", len(recs)) - if err = rejectDuplicateSavingsPlanRows(recs); err != nil { + err = rejectDuplicateSavingsPlanRows(recs) + if err != nil { return nil, aws.Config{}, "", err } diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 672b436b4..885b2127b 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -8,6 +8,7 @@ import ( "log" "math" "os" + "slices" "sort" "strconv" "strings" @@ -217,7 +218,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string // every row, which adds noise without information; the underlying fields // stay on the Recommendation struct for internal use (SP no-signal // guard, etc.). - header := []string{ + baseHeader := []string{ "Service", "Region", "ResourceType", "Family", "Engine", "Deployment", "Instances", "CoveredInstances", "Count", "NormalizedUnits", "RecommendedCount", @@ -227,7 +228,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string "ExistingCoverage", "ProjectedCoverage", } // Appended, never reordered: existing readers look columns up by name. - header = append(header, csvDetailColumns...) + header := slices.Concat(baseHeader, csvDetailColumns) if err := writer.Write(header); err != nil { return fmt.Errorf("failed to write CSV header: %w", err) } @@ -251,7 +252,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string errStr = r.Error.Error() } - row := []string{ + baseRow := []string{ string(rec.Service), rec.Region, rec.ResourceType, @@ -277,7 +278,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string formatExistingCoverage(rec), formatPercentOrBlank(rec.ProjectedCoverage), } - row = append(row, detailCells(rec)...) + row := slices.Concat(baseRow, detailCells(rec)) if err := writer.Write(row); err != nil { return fmt.Errorf("failed to write CSV row: %w", err) } @@ -326,7 +327,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { if totalNU > 0 { nuCell = fmt.Sprintf("%g", totalNU) } - row := []string{ + totals := []string{ "TOTAL", "", "", "", "", "", // Service through Deployment "", "", // Instances, CoveredInstances fmt.Sprintf("%d", totalCount), nuCell, "", // Count, NormalizedUnits, RecommendedCount @@ -335,7 +336,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { "", "", "", "", // CommitmentID, Success, Error, Timestamp "", "", // ExistingCoverage, ProjectedCoverage } - return append(row, make([]string, len(csvDetailColumns))...) // detail columns do not aggregate + return slices.Concat(totals, make([]string, len(csvDetailColumns))) // detail columns do not aggregate } // formatIntOrBlank renders an int as its decimal string when non-zero, "" diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 17fcd09d6..1f98c3515 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -330,7 +330,7 @@ func TestCSVRoundTrip_SavingsPlanAndEC2DetailsSurviveWriteThenRead(t *testing.T) Details: &common.DatabaseDetails{Engine: "Aurora MySQL", AZConfig: "multi-az", InstanceClass: "db.r6g.large"}} in := []common.Recommendation{compute, ec2sp, ec2, rds} - var results []common.PurchaseResult + results := make([]common.PurchaseResult, 0, len(in)) for _, r := range in { results = append(results, common.PurchaseResult{Recommendation: r, Success: true, Timestamp: time.Now()}) }