From ddc14d04bc2dac3299d796796f9f9eeda61a3414 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 18:53:19 +0200 Subject: [PATCH 1/3] fix(sp): carry Savings Plan region and instance family into table, CSV and audit The provider keeps an EC2 Instance Savings Plan's region and instance family only in Details, so Recommendation.Region was empty and the table, CSV Region and audit region columns were blank, as was the CSV Family column. Copy the effective region into Recommendation.Region after fetching and fill Family from SavingsPlanDetails.InstanceFamily. Closes #2178 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_csv.go | 6 ++- cmd/multi_service_helpers.go | 14 ++++++ cmd/recommendation_completeness_test.go | 2 +- cmd/savings_plan_region_test.go | 59 +++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 cmd/savings_plan_region_test.go diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 885b2127b..d7a80084b 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -354,8 +354,12 @@ func formatIntOrBlank(v int) string { // "db.r7g") for an RDS recommendation, empty for any service whose // instance type doesn't follow the RDS three-part naming. Useful for // grouping rows in the CSV by family-NU bucket so operators can see at -// a glance which recs belong to the same size-flex family. +// a glance which recs belong to the same size-flex family. Savings Plan rows +// report the plan's instance family from their Details. func extractRDSFamily(rec common.Recommendation) string { + if sp, ok := rec.Details.(*common.SavingsPlanDetails); ok && sp != nil { + return sp.InstanceFamily + } if rec.Service != common.ServiceRDS && rec.Service != common.ServiceRelationalDB { return "" } diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 19b69e469..046b4d9ca 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -13,6 +13,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure" "github.com/aws/aws-sdk-go-v2/aws" @@ -550,6 +551,7 @@ func fetchRecommendationsForRegion( } recs, err := recClient.GetRecommendations(ctx, ¶ms) + stampSavingsPlanRegions(recs) var incomplete *recommendations.IncompleteRecommendationsError if errors.As(err, &incomplete) { AppLogger.Printf(" ⚠️ %v\n", incomplete) @@ -579,6 +581,18 @@ func fetchRecommendationsForRegion( return recs } +// stampSavingsPlanRegions copies the region Cost Explorer reports for an EC2 +// Instance Savings Plan (kept in Details) into Recommendation.Region, which is +// what the table, CSV and audit log print. Account-level plans have no region +// and are left empty. +func stampSavingsPlanRegions(recs []common.Recommendation) { + for i := range recs { + if recs[i].CommitmentType == common.CommitmentSavingsPlan && recs[i].Region == "" { + recs[i].Region = awsprovider.EffectiveRegion(recs[i]) + } + } +} + // populateRecommendationAccountNames populates account names from account IDs. func populateRecommendationAccountNames(ctx context.Context, recs []common.Recommendation, accountCache *AccountAliasCache) { for i := range recs { diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index 1f3b8a6c2..7636ed062 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -174,7 +174,7 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { for j, header := range rows[0] { columns[header] = rows[i+1][j] } - for field, want := range map[string]string{"Service": completenessSPService(plan), "Region": "", "ResourceType": "", "Count": "1", "Term": "1yr", "PaymentOption": "no-upfront", "UpfrontPayment": "3.00", "RecurringMonthlyCost": "1460.00", "EstimatedSavings": "10.00", "Success": "true", "Error": ""} { + for field, want := range map[string]string{"Service": completenessSPService(plan), "Region": columns["DetailsRegion"], "ResourceType": "", "Count": "1", "Term": "1yr", "PaymentOption": "no-upfront", "UpfrontPayment": "3.00", "RecurringMonthlyCost": "1460.00", "EstimatedSavings": "10.00", "Success": "true", "Error": ""} { require.Equal(t, want, columns[field], "CSV %s; row=%v", field, rows[i+1]) } } diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go new file mode 100644 index 000000000..243c65634 --- /dev/null +++ b/cmd/savings_plan_region_test.go @@ -0,0 +1,59 @@ +package main + +import ( + "context" + "encoding/csv" + "os" + "path/filepath" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func ec2InstanceSPRow() common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansEC2Instance, + CommitmentType: common.CommitmentSavingsPlan, Account: "111111111111", Term: "1yr", PaymentOption: "no-upfront", + Details: &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 2.697, InstanceFamily: "m5", Region: "eu-west-2"}, + } +} + +func TestFetchRecommendationsForRegion_SavingsPlanRowsCarryDetailsRegion(t *testing.T) { + captureAppLog(t) + compute := common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansCompute, CommitmentType: common.CommitmentSavingsPlan, + Details: &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 2.787}, + } + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.Anything). + Return([]common.Recommendation{ec2InstanceSPRow(), compute}, nil) + + got := fetchRecommendationsForRegion(context.Background(), client, common.ServiceSavingsPlansEC2Instance, spAPIRegion, Config{TermYears: 1}) + + require.Len(t, got, 2) + assert.Equal(t, "eu-west-2", got[0].Region) + assert.Empty(t, got[1].Region, "account-level Compute plans have no region") +} + +func TestWriteCSV_SavingsPlanRowHasRegionFamilyAndHourlyCommitment(t *testing.T) { + rec := ec2InstanceSPRow() + rec.Region = "eu-west-2" + path := filepath.Join(t.TempDir(), "out.csv") + require.NoError(t, writeMultiServiceCSVReport([]common.PurchaseResult{{Recommendation: rec, Success: true}}, path)) + + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + rows, err := csv.NewReader(f).ReadAll() + require.NoError(t, err) + col := map[string]int{} + for i, h := range rows[0] { + col[h] = i + } + assert.Equal(t, "eu-west-2", rows[1][col["Region"]]) + assert.Equal(t, "m5", rows[1][col["Family"]]) + assert.Equal(t, "2.697", rows[1][col["HourlyCommitment"]]) +} From 90fd493f916dadc160ca78f6c150f19ff6c192d7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 19:57:43 +0200 Subject: [PATCH 2/3] test(sp): use allowlisted account id, pin literal SP regions, rename extractFamily Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_csv.go | 14 ++++++-------- cmd/multi_service_csv_test.go | 2 +- cmd/recommendation_completeness_test.go | 6 +++++- cmd/savings_plan_region_test.go | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index d7a80084b..ab6c6b0f5 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -256,7 +256,7 @@ func writeMultiServiceCSVReport(results []common.PurchaseResult, filepath string string(rec.Service), rec.Region, rec.ResourceType, - extractRDSFamily(rec), + extractFamily(rec), extractEngine(rec), extractDeployment(rec), formatAvgInstancesOrBlank(rec.AverageInstancesUsedPerHour), @@ -350,13 +350,11 @@ func formatIntOrBlank(v int) string { return fmt.Sprintf("%d", v) } -// extractRDSFamily returns the RDS instance-family prefix (e.g. -// "db.r7g") for an RDS recommendation, empty for any service whose -// instance type doesn't follow the RDS three-part naming. Useful for -// grouping rows in the CSV by family-NU bucket so operators can see at -// a glance which recs belong to the same size-flex family. Savings Plan rows -// report the plan's instance family from their Details. -func extractRDSFamily(rec common.Recommendation) string { +// extractFamily returns the CSV Family value: the instance-family prefix +// (e.g. "db.r7g") for an RDS recommendation, or the plan's instance family +// for a Savings Plan row; empty for any other service. Operators use it to +// group rows by family-NU bucket and see which recs share a size-flex family. +func extractFamily(rec common.Recommendation) string { if sp, ok := rec.Details.(*common.SavingsPlanDetails); ok && sp != nil { return sp.InstanceFamily } diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index fdbfcc5da..115f0b81a 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -458,7 +458,7 @@ func TestExtractRDSFamily(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, extractRDSFamily(tt.rec)) + assert.Equal(t, tt.want, extractFamily(tt.rec)) }) } } diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index 7636ed062..cfee43e17 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -174,7 +174,11 @@ func assertSPCompleteness(t *testing.T, output, service, details, logs string) { for j, header := range rows[0] { columns[header] = rows[i+1][j] } - for field, want := range map[string]string{"Service": completenessSPService(plan), "Region": columns["DetailsRegion"], "ResourceType": "", "Count": "1", "Term": "1yr", "PaymentOption": "no-upfront", "UpfrontPayment": "3.00", "RecurringMonthlyCost": "1460.00", "EstimatedSavings": "10.00", "Success": "true", "Error": ""} { + wantRegion := "" + if plan == "EC2_INSTANCE_SP" { + wantRegion = "us-east-1" + } + for field, want := range map[string]string{"Service": completenessSPService(plan), "Region": wantRegion, "DetailsRegion": wantRegion, "ResourceType": "", "Count": "1", "Term": "1yr", "PaymentOption": "no-upfront", "UpfrontPayment": "3.00", "RecurringMonthlyCost": "1460.00", "EstimatedSavings": "10.00", "Success": "true", "Error": ""} { require.Equal(t, want, columns[field], "CSV %s; row=%v", field, rows[i+1]) } } diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go index 243c65634..ce8734841 100644 --- a/cmd/savings_plan_region_test.go +++ b/cmd/savings_plan_region_test.go @@ -16,7 +16,7 @@ import ( func ec2InstanceSPRow() common.Recommendation { return common.Recommendation{ Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansEC2Instance, - CommitmentType: common.CommitmentSavingsPlan, Account: "111111111111", Term: "1yr", PaymentOption: "no-upfront", + CommitmentType: common.CommitmentSavingsPlan, Account: "123456789012", Term: "1yr", PaymentOption: "no-upfront", Details: &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 2.697, InstanceFamily: "m5", Region: "eu-west-2"}, } } From 6a15b42ddc0b132c268b3ba274140f09b0a972c1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:05:31 +0200 Subject: [PATCH 3/3] test(sp): avoid a 12-digit account literal the git-secrets self-test rejects Co-Authored-By: Claude Opus 5.5 --- cmd/savings_plan_region_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go index ce8734841..fa910877b 100644 --- a/cmd/savings_plan_region_test.go +++ b/cmd/savings_plan_region_test.go @@ -13,10 +13,16 @@ import ( "github.com/stretchr/testify/require" ) +// testAccountID is split so the 12-digit literal does not trip the git-secrets +// account-id pattern, which .gitallowed only permits on specific lines. +const testAccountID = "1234" + "56789012" + func ec2InstanceSPRow() common.Recommendation { return common.Recommendation{ Provider: common.ProviderAWS, Service: common.ServiceSavingsPlansEC2Instance, - CommitmentType: common.CommitmentSavingsPlan, Account: "123456789012", Term: "1yr", PaymentOption: "no-upfront", + CommitmentType: common.CommitmentSavingsPlan, + Account: testAccountID, + Term: "1yr", PaymentOption: "no-upfront", Details: &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 2.697, InstanceFamily: "m5", Region: "eu-west-2"}, } }