From 16c15936c0f12c6727ae2829409b393e966b1b5f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:29:51 +0200 Subject: [PATCH] chore(sp): tidy cli#2184 follow-ups Drop the split account-id literal and unused Account field from the SP region test, rename TestExtractRDSFamily to TestExtractFamily with SP cases, reword the extractFamily comment, and drop the Region guard in stampSavingsPlanRegions (EffectiveRegion already returns a non-empty Region unchanged; deletion probe: full suite passes without it). Closes #2187 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_csv.go | 2 +- cmd/multi_service_csv_test.go | 9 ++++++--- cmd/multi_service_helpers.go | 2 +- cmd/savings_plan_region_test.go | 5 ----- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index ab6c6b0f5..ff4fb0175 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -353,7 +353,7 @@ func formatIntOrBlank(v int) 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. +// group rows by family (for RDS, the family-NU bucket). 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 115f0b81a..8209914c2 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -439,9 +439,9 @@ func TestFormatRecurringMonthlyOrBlank(t *testing.T) { } } -// TestExtractRDSFamily covers the family-prefix extraction used by the -// CSV writer to group rows by size-flex family. -func TestExtractRDSFamily(t *testing.T) { +// TestExtractFamily covers the CSV Family column: the RDS family prefix and +// the Savings Plan instance family. +func TestExtractFamily(t *testing.T) { tests := []struct { name string rec common.Recommendation @@ -455,6 +455,9 @@ func TestExtractRDSFamily(t *testing.T) { {"ElastiCache ignored", common.Recommendation{Service: common.ServiceElastiCache, ResourceType: "cache.t3.micro"}, ""}, // Malformed RDS type. {"RDS bare type", common.Recommendation{Service: common.ServiceRDS, ResourceType: "db.r7g"}, ""}, + {"EC2 Instance SP", common.Recommendation{Service: common.ServiceSavingsPlansEC2Instance, Details: &common.SavingsPlanDetails{InstanceFamily: "m5"}}, "m5"}, + {"Compute SP has no family", common.Recommendation{Service: common.ServiceSavingsPlansCompute, Details: &common.SavingsPlanDetails{}}, ""}, + {"nil SP details", common.Recommendation{Service: common.ServiceSavingsPlansCompute, Details: (*common.SavingsPlanDetails)(nil)}, ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b76e4fcd4..a4b6d0d86 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -653,7 +653,7 @@ func fetchRecommendationsForRegion( // and are left empty. func stampSavingsPlanRegions(recs []common.Recommendation) { for i := range recs { - if recs[i].CommitmentType == common.CommitmentSavingsPlan && recs[i].Region == "" { + if recs[i].CommitmentType == common.CommitmentSavingsPlan { recs[i].Region = awsprovider.EffectiveRegion(recs[i]) } } diff --git a/cmd/savings_plan_region_test.go b/cmd/savings_plan_region_test.go index fa910877b..d13725aff 100644 --- a/cmd/savings_plan_region_test.go +++ b/cmd/savings_plan_region_test.go @@ -13,15 +13,10 @@ 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: testAccountID, Term: "1yr", PaymentOption: "no-upfront", Details: &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 2.697, InstanceFamily: "m5", Region: "eu-west-2"}, }