diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d1baaf5c..856759c8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- BREAKING: `--input-csv` now requires `Term` and `PaymentOption` columns in + addition to `Service`, `Region`, `ResourceType` and `Count`. `Term` must be + `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` + or `no-upfront` on every row. CSVs with only the older four columns, or with + a blank or unknown value, now fail to load with an error naming the missing + column or the row. - Split the CLI out of the CUDly monorepo into its own module, `github.com/LeanerCloud/cloud-commitments-cli`. The shared client and provider code it depends on now lives in diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index 9e5c8b7f1..80694e42c 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -51,7 +51,7 @@ func TestFilterAndAdjustRecommendations_CancellationAndDeadlineAreTerminal(t *te func TestPrepareCSVPurchaseRun_CanceledContextStopsBeforeConfirmationAndPurchase(t *testing.T) { dir := t.TempDir() csvPath := filepath.Join(dir, "in.csv") - require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count,Engine\nrds,us-east-1,db.t3.small,2,mysql\n"), 0o600)) + require.NoError(t, os.WriteFile(csvPath, []byte("Service,Region,ResourceType,Count,Engine,Term,PaymentOption\nrds,us-east-1,db.t3.small,2,mysql,1yr,no-upfront\n"), 0o600)) cfg := Config{CSVInput: csvPath, AuditLog: filepath.Join(dir, "audit.jsonl")} for _, dry := range []bool{true, false} { diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index ff4fb0175..ce2025d8a 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -13,6 +13,7 @@ import ( "strconv" "strings" "time" + "unicode/utf8" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" @@ -48,9 +49,14 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) if err != nil { return nil, fmt.Errorf("failed to read CSV header: %w", err) } + if err = validateCSVUTF8(header); err != nil { + return nil, fmt.Errorf("CSV header: %w", err) + } - // Build column index map - colIdx := buildColumnIndexMap(header) + colIdx, err := buildColumnIndexMap(header) + if err != nil { + return nil, err + } // Parse all records parsed, err := parseCSVRecords(reader, colIdx) @@ -61,13 +67,60 @@ func loadRecommendationsFromCSV(csvPath string) ([]common.Recommendation, error) return parsed, nil } -// buildColumnIndexMap creates a map from column names to indices. -func buildColumnIndexMap(header []string) map[string]int { +// Minimal CSVs require these columns; service-specific columns stay optional. +// Term and PaymentOption are required because the purchase path reads them per +// row and must never guess a commitment's term or payment option. +var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count", "Term", "PaymentOption"} + +// Accepted Term values match fetchRecommendationsForRegion; accepted +// PaymentOption values match validatePaymentAndTerm. +var ( + validCSVTerms = []string{"1yr", "3yr"} + validCSVPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"} +) + +// parseCSVEnum requires the cell to be exactly one of allowed. +func parseCSVEnum(record []string, colIdx map[string]int, fieldName string, allowed []string) (string, error) { + col := colIdx[fieldName] + 1 + value := getCSVField(record, colIdx, fieldName) + if strings.TrimSpace(value) == "" { + return "", fmt.Errorf("column %d %q: value is required (one of: %s)", col, fieldName, strings.Join(allowed, ", ")) + } + for _, a := range allowed { + if value == a { + return value, nil + } + } + return "", fmt.Errorf("column %d %q: invalid value %q (must be one of: %s)", col, fieldName, value, strings.Join(allowed, ", ")) +} + +func validateCSVUTF8(fields []string) error { + for i, field := range fields { + if !utf8.ValidString(field) { + return fmt.Errorf("column %d: invalid UTF-8 encoding", i+1) + } + } + return nil +} + +func buildColumnIndexMap(header []string) (map[string]int, error) { + if len(header) > 0 { + header[0] = strings.TrimPrefix(header[0], "\ufeff") + } colIdx := make(map[string]int) for i, col := range header { colIdx[col] = i } - return colIdx + var missing []string + for _, req := range requiredCSVColumns { + if _, ok := colIdx[req]; !ok { + missing = append(missing, req) + } + } + if len(missing) > 0 { + return nil, fmt.Errorf("CSV header missing required columns: %s", strings.Join(missing, ", ")) + } + return colIdx, nil } // parseCSVRecords reads and parses all CSV records. @@ -82,6 +135,10 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm if err != nil { return nil, fmt.Errorf("failed to read CSV record: %w", err) } + if err = validateCSVUTF8(record); err != nil { + line, _ := reader.FieldPos(0) + return nil, fmt.Errorf("CSV line %d: %w", line, err) + } // Skip the trailing TOTAL summary row that writeMultiServiceCSVReport // emits (label in the Service column). Without this, feeding the tool @@ -113,14 +170,25 @@ func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendati rec.ResourceType = getCSVField(record, colIdx, "ResourceType") rec.Account = getCSVField(record, colIdx, "Account") rec.AccountName = getCSVField(record, colIdx, "AccountName") - rec.Term = getCSVField(record, colIdx, "Term") - rec.PaymentOption = getCSVField(record, colIdx, "PaymentOption") - if err := parseCSVCount(record, colIdx, &rec.Count); err != nil { + term, err := parseCSVEnum(record, colIdx, "Term", validCSVTerms) + if err != nil { + return rec, err + } + rec.Term = term + payment, err := parseCSVEnum(record, colIdx, "PaymentOption", validCSVPaymentOptions) + if err != nil { + return rec, err + } + rec.PaymentOption = payment + + err = parseCSVCount(record, colIdx, &rec.Count) + if err != nil { return rec, err } - if err := parseCSVFloat(record, colIdx, "EstimatedSavings", &rec.EstimatedSavings); err != nil { + err = parseCSVFloat(record, colIdx, "EstimatedSavings", &rec.EstimatedSavings) + if err != nil { return rec, err } @@ -143,9 +211,7 @@ func getCSVField(record []string, colIdx map[string]int, fieldName string) strin return "" } -// parseCSVCount parses the required Count field as a whole non-negative -// integer. It drives purchase quantities, so a blank, missing, fractional or -// otherwise malformed cell is an error rather than a truncated or zero value. +// Savings Plans exports use Count=1, preserving positive-count round trips. func parseCSVCount(record []string, colIdx map[string]int, target *int) error { const fieldName = "Count" idx, ok := colIdx[fieldName] @@ -158,17 +224,16 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error { } n, err := strconv.Atoi(value) if err != nil { - return fmt.Errorf("column %d %q: invalid integer %q: %w", idx+1, fieldName, value, err) + return fmt.Errorf("column %d %q: invalid integer: %w", idx+1, fieldName, err) } - if n < 0 { - return fmt.Errorf("column %d %q: must not be negative, got %d", idx+1, fieldName, n) + if n < 1 { + return fmt.Errorf("column %d %q: must be at least 1, got %d", idx+1, fieldName, n) } *target = n return nil } -// parseCSVFloat parses a finite float field from a CSV record. A blank or -// absent cell leaves target untouched (see requireRankingSignal). +// Blank savings stay absent-as-zero for requireRankingSignal. func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, target *float64) error { value := strings.TrimSpace(getCSVField(record, colIdx, fieldName)) if value == "" { @@ -177,11 +242,14 @@ func parseCSVFloat(record []string, colIdx map[string]int, fieldName string, tar col := colIdx[fieldName] + 1 f, err := strconv.ParseFloat(value, 64) if err != nil { - return fmt.Errorf("column %d %q: invalid number %q: %w", col, fieldName, value, err) + return fmt.Errorf("column %d %q: invalid number: %w", col, fieldName, err) } if math.IsNaN(f) || math.IsInf(f, 0) { return fmt.Errorf("column %d %q: invalid number %q: must be finite", col, fieldName, value) } + if f < 0 { + return fmt.Errorf("column %d %q: must not be negative, got %g", col, fieldName, f) + } *target = f return nil } diff --git a/cmd/multi_service_csv_header_test.go b/cmd/multi_service_csv_header_test.go new file mode 100644 index 000000000..8adfd7fd9 --- /dev/null +++ b/cmd/multi_service_csv_header_test.go @@ -0,0 +1,151 @@ +package main + +import ( + "encoding/binary" + "testing" + "unicode/utf16" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadRecommendationsFromCSV_HeaderValidation_1327(t *testing.T) { + tests := []struct { + name string + header string + row string + errContains string + }{ + {"missing Service", "Region,ResourceType,Count,Term,PaymentOption\n", "us-east-1,db.t3.micro,2,3yr,partial-upfront\n", "Service"}, + {"missing Region", "Service,ResourceType,Count,Term,PaymentOption\n", "rds,db.t3.micro,2,3yr,partial-upfront\n", "Region"}, + {"missing ResourceType", "Service,Region,Count,Term,PaymentOption\n", "rds,us-east-1,2,3yr,partial-upfront\n", "ResourceType"}, + {"missing Count", "Service,Region,ResourceType,Term,PaymentOption\n", "rds,us-east-1,db.t3.micro,3yr,partial-upfront\n", "Count"}, + {"missing Term", "Service,Region,ResourceType,Count,PaymentOption\n", "rds,us-east-1,db.t3.micro,2,partial-upfront\n", "Term"}, + {"missing PaymentOption", "Service,Region,ResourceType,Count,Term\n", "rds,us-east-1,db.t3.micro,2,3yr\n", "PaymentOption"}, + {"legacy 4-column header rejected", "Service,Region,ResourceType,Count\n", "rds,us-east-1,db.t3.micro,2\n", "Term, PaymentOption"}, + {"multiple missing", "Service,Count\n", "rds,2\n", "Region, ResourceType, Term, PaymentOption"}, + { + "legacy TEST-02 headers rejected", + "Service,Region,Instance Type,Instance Count\n", + "rds,us-east-1,db.t3.micro,2\n", + "ResourceType, Count, Term, PaymentOption", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := loadCSVContent(t, tt.header+tt.row) + require.Error(t, err) + assert.Contains(t, err.Error(), "CSV header missing required columns") + assert.Contains(t, err.Error(), tt.errContains) + }) + } +} + +func TestLoadRecommendationsFromCSV_BOMPrefixed_1327(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "\ufeffService,Region,ResourceType,Count,Term,PaymentOption\nrds,us-east-1,db.t3.micro,2,3yr,partial-upfront\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, common.ServiceRDS, recs[0].Service) + assert.Equal(t, "us-east-1", recs[0].Region) + assert.Equal(t, 2, recs[0].Count) +} + +func TestLoadRecommendationsFromCSV_Encoding_1327(t *testing.T) { + t.Run("UTF-16", func(t *testing.T) { + units := utf16.Encode([]rune("Service,Region,ResourceType,Count,Term,PaymentOption\nrds,us-east-1,db.t3.micro,2,3yr,partial-upfront\n")) + for _, tt := range []struct { + name string + bom []byte + order binary.ByteOrder + }{ + {"little endian", []byte{0xff, 0xfe}, binary.LittleEndian}, + {"big endian", []byte{0xfe, 0xff}, binary.BigEndian}, + } { + t.Run(tt.name, func(t *testing.T) { + encoded := make([]byte, 2+2*len(units)) + copy(encoded, tt.bom) + for i, unit := range units { + tt.order.PutUint16(encoded[2+2*i:], unit) + } + err := loadCSVContent(t, string(encoded)) + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 1: invalid UTF-8 encoding") + }) + } + }) + + t.Run("Latin-1 optional header", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Caf\xe9,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,2,prod,3yr,partial-upfront\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV header: column 5: invalid UTF-8 encoding") + }) + + t.Run("Latin-1 AccountName", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,AccountName,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,2,Caf\xe9,3yr,partial-upfront\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 2: column 5: invalid UTF-8 encoding") + }) + + t.Run("invalid ignored field in TOTAL after multiline record", func(t *testing.T) { + err := loadCSVContent(t, "Service,Region,ResourceType,Count,Ignored,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,2,\"prod\naccount\",3yr,partial-upfront\n"+ + "TOTAL,,,2,\xff,,\n") + require.Error(t, err) + assert.EqualError(t, err, "CSV line 4: column 5: invalid UTF-8 encoding") + }) + + t.Run("valid non-ASCII UTF-8", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,2,Café 東京,3yr,partial-upfront\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "Café 東京", recs[0].AccountName) + }) +} + +// Lock in encoding/csv behaviors the purchase path relies on. +func TestLoadRecommendationsFromCSV_EdgeCases_1327(t *testing.T) { + t.Run("CRLF line endings", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\r\nrds,us-east-1,db.t3.micro,2,3yr,partial-upfront\r\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, 2, recs[0].Count) + }) + + t.Run("quoted commas and newlines inside fields", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,AccountName,Term,PaymentOption\n"+ + "rds,us-east-1,\"db.t3.micro, burstable\",2,\"prod\naccount\",3yr,partial-upfront\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "db.t3.micro, burstable", recs[0].ResourceType) + assert.Equal(t, "prod\naccount", recs[0].AccountName) + }) + + t.Run("headers only loads zero recs", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\n")) + require.NoError(t, err) + assert.Empty(t, recs) + }) + + t.Run("wrong field count on a data row", func(t *testing.T) { + err := loadCSVContent(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\nrds,us-east-1,db.t3.micro\n") + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to read CSV record") + }) + + t.Run("TOTAL row still skipped with full header", func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\nrds,us-east-1,db.t3.micro,2,3yr,partial-upfront\nTOTAL,,,2,,\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + }) +} diff --git a/cmd/multi_service_csv_strict_test.go b/cmd/multi_service_csv_strict_test.go index ac0ed3fba..62ced1c8c 100644 --- a/cmd/multi_service_csv_strict_test.go +++ b/cmd/multi_service_csv_strict_test.go @@ -26,15 +26,16 @@ func TestLoadRecommendationsFromCSV_StrictCount(t *testing.T) { {"trailing garbage", "3abc"}, {"trailing unit", "12 units"}, {"negative", "-1"}, + {"zero", "0"}, {"blank", ""}, {"whitespace only", " "}, {"overflows int64", "99999999999999999999"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - content := "Service,Region,ResourceType,Count\n" + - "rds,us-east-1,db.t3.micro,1\n" + - "rds,us-east-1,db.t3.small,\"" + tt.cell + "\"\n" + content := "Service,Region,ResourceType,Count,Term,PaymentOption\n" + + "rds,us-east-1,db.t3.micro,1,3yr,partial-upfront\n" + + "rds,us-east-1,db.t3.small,\"" + tt.cell + "\",3yr,partial-upfront\n" err := loadCSVContent(t, content) require.Error(t, err) assert.Contains(t, err.Error(), "line 3") @@ -44,7 +45,7 @@ func TestLoadRecommendationsFromCSV_StrictCount(t *testing.T) { } func TestLoadRecommendationsFromCSV_CountTrimmed(t *testing.T) { - csvPath := writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count\nrds,us-east-1,db.t3.micro,\" 3 \"\n") + csvPath := writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,Term,PaymentOption\nrds,us-east-1,db.t3.micro,\" 3 \",3yr,partial-upfront\n") recs, err := loadRecommendationsFromCSV(csvPath) require.NoError(t, err) require.Len(t, recs, 1) @@ -52,7 +53,7 @@ func TestLoadRecommendationsFromCSV_CountTrimmed(t *testing.T) { } func TestLoadRecommendationsFromCSV_CountColumnRequired(t *testing.T) { - err := loadCSVContent(t, "Service,Region,ResourceType\nrds,us-east-1,db.t3.micro\n") + err := loadCSVContent(t, "Service,Region,ResourceType,Term,PaymentOption\nrds,us-east-1,db.t3.micro,3yr,partial-upfront\n") require.Error(t, err) assert.Contains(t, err.Error(), "Count") } @@ -62,6 +63,7 @@ func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { name string cell string }{ + {"negative", "-100"}, {"trailing currency", "1000 USD"}, {"trailing garbage", "12.5abc"}, {"NaN", "NaN"}, @@ -70,8 +72,8 @@ func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - content := "Service,Region,ResourceType,Count,EstimatedSavings\n" + - "rds,us-east-1,db.t3.micro,2,\"" + tt.cell + "\"\n" + content := "Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption\n" + + "rds,us-east-1,db.t3.micro,2,\"" + tt.cell + "\",3yr,partial-upfront\n" err := loadCSVContent(t, content) require.Error(t, err) assert.Contains(t, err.Error(), "line 2") @@ -83,7 +85,7 @@ func TestLoadRecommendationsFromCSV_StrictEstimatedSavings(t *testing.T) { // A blank EstimatedSavings cell stays absent-as-zero: requireRankingSignal // relies on it to refuse a binding --max-instances cap. func TestLoadRecommendationsFromCSV_BlankSavingsStillZero(t *testing.T) { - csvPath := writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings\nrds,us-east-1,db.t3.micro,2, \n") + csvPath := writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption\nrds,us-east-1,db.t3.micro,2, ,3yr,partial-upfront\n") recs, err := loadRecommendationsFromCSV(csvPath) require.NoError(t, err) require.Len(t, recs, 1) @@ -107,8 +109,8 @@ func TestRunToolFromCSV_RejectsMalformedNumbers(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { cfg := Config{ - CSVInput: writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings\n"+ - "rds,us-east-1,db.t3.micro,"+tt.count+","+tt.savings+"\n"), + CSVInput: writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,"+tt.count+","+tt.savings+",3yr,partial-upfront\n"), CSVOutput: filepath.Join(t.TempDir(), "report.csv"), AuditLog: filepath.Join(t.TempDir(), "audit.jsonl"), ActualPurchase: false, @@ -122,3 +124,51 @@ func TestRunToolFromCSV_RejectsMalformedNumbers(t *testing.T) { }) } } + +// Term and PaymentOption cells must be exactly what the purchase path accepts +// (1yr/3yr, all-upfront/partial-upfront/no-upfront); a blank or unknown value +// fails the load with the row number instead of reaching the purchase loop. +func TestLoadRecommendationsFromCSV_TermAndPaymentValues(t *testing.T) { + tests := []struct { + name string + term string + payment string + column string + }{ + {"empty Term", "", "no-upfront", `column 5 "Term"`}, + {"whitespace Term", " ", "no-upfront", `column 5 "Term"`}, + {"unknown Term", "5yr", "no-upfront", `column 5 "Term"`}, + {"numeric Term", "3", "no-upfront", `column 5 "Term"`}, + {"uppercase Term", "3YR", "no-upfront", `column 5 "Term"`}, + {"empty PaymentOption", "1yr", "", `column 6 "PaymentOption"`}, + {"unknown PaymentOption", "1yr", "monthly", `column 6 "PaymentOption"`}, + {"display-form PaymentOption", "1yr", "All Upfront", `column 6 "PaymentOption"`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := "Service,Region,ResourceType,Count,Term,PaymentOption\n" + + "rds,us-east-1,db.t3.micro,1,1yr,all-upfront\n" + + "rds,us-east-1,db.t3.small,2," + tt.term + "," + tt.payment + "\n" + err := loadCSVContent(t, content) + require.Error(t, err) + assert.Contains(t, err.Error(), "line 3") + assert.Contains(t, err.Error(), tt.column) + }) + } +} + +func TestLoadRecommendationsFromCSV_TermAndPaymentAccepted(t *testing.T) { + for _, term := range []string{"1yr", "3yr"} { + for _, payment := range []string{"all-upfront", "partial-upfront", "no-upfront"} { + t.Run(term+"/"+payment, func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,2,"+term+","+payment+"\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, term, recs[0].Term) + assert.Equal(t, payment, recs[0].PaymentOption) + }) + } + } +} diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index 8209914c2..4ec85e604 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -614,8 +614,8 @@ ec2,us-west-2,t3.medium,10,123456789012,Development,1yr,all-upfront,2000.75`, }, { name: "Valid CSV with minimal fields", - csvContent: `Service,Region,ResourceType,Count -elasticache,eu-west-1,cache.t3.micro,3`, + csvContent: `Service,Region,ResourceType,Count,Term,PaymentOption +elasticache,eu-west-1,cache.t3.micro,3,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) @@ -627,8 +627,8 @@ elasticache,eu-west-1,cache.t3.micro,3`, }, { name: "Valid CSV with empty optional Account field", - csvContent: `Service,Region,ResourceType,Count,Account -rds,us-east-1,db.t3.micro,2,`, + csvContent: `Service,Region,ResourceType,Count,Account,Term,PaymentOption +rds,us-east-1,db.t3.micro,2,,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) @@ -699,8 +699,8 @@ TOTAL,,,,,3,,`, }, { name: "CSV with different column order", - csvContent: `Count,Service,Region,ResourceType -7,rds,ap-south-1,db.r5.large`, + csvContent: `Count,Service,Region,ResourceType,Term,PaymentOption +7,rds,ap-south-1,db.r5.large,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) @@ -721,25 +721,25 @@ TOTAL,,,,,3,,`, }, { name: "Invalid Count value - non-numeric", - csvContent: `Service,Region,ResourceType,Count -rds,us-east-1,db.t3.micro,abc`, + csvContent: `Service,Region,ResourceType,Count,Term,PaymentOption +rds,us-east-1,db.t3.micro,abc,3yr,partial-upfront`, wantErr: true, errContains: `column 4 "Count": invalid integer`, }, { name: "Invalid EstimatedSavings value - non-numeric", - csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,5,invalid`, + csvContent: `Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption +rds,us-east-1,db.t3.micro,5,invalid,3yr,partial-upfront`, wantErr: true, errContains: `column 5 "EstimatedSavings": invalid number`, }, { name: "Multiple rows with various services", - csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,5,100.00 -ec2,us-west-2,t3.medium,10,200.50 -elasticache,eu-west-1,cache.t3.micro,3,50.25 -opensearch,ap-southeast-1,t3.small.search,2,75.00`, + csvContent: `Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption +rds,us-east-1,db.t3.micro,5,100.00,3yr,partial-upfront +ec2,us-west-2,t3.medium,10,200.50,3yr,partial-upfront +elasticache,eu-west-1,cache.t3.micro,3,50.25,3yr,partial-upfront +opensearch,ap-southeast-1,t3.small.search,2,75.00,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 4) @@ -751,8 +751,8 @@ opensearch,ap-southeast-1,t3.small.search,2,75.00`, }, { name: "CSV with large Count values", - csvContent: `Service,Region,ResourceType,Count -ec2,us-east-1,t3.large,1000`, + csvContent: `Service,Region,ResourceType,Count,Term,PaymentOption +ec2,us-east-1,t3.large,1000,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) @@ -761,8 +761,8 @@ ec2,us-east-1,t3.large,1000`, }, { name: "CSV with decimal savings", - csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,5,1234.5678`, + csvContent: `Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption +rds,us-east-1,db.t3.micro,5,1234.5678,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) @@ -770,13 +770,13 @@ rds,us-east-1,db.t3.micro,5,1234.5678`, }, }, { - name: "CSV with zero values", - csvContent: `Service,Region,ResourceType,Count,EstimatedSavings -rds,us-east-1,db.t3.micro,0,0`, + name: "CSV with zero EstimatedSavings stays valid", + csvContent: `Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption +rds,us-east-1,db.t3.micro,5,0,3yr,partial-upfront`, wantErr: false, validate: func(t *testing.T, recs []common.Recommendation) { require.Len(t, recs, 1) - assert.Equal(t, 0, recs[0].Count) + assert.Equal(t, 5, recs[0].Count) assert.Equal(t, float64(0), recs[0].EstimatedSavings) }, }, diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 4ec6144b3..8e52d4bd3 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -484,9 +484,9 @@ func TestCSVCapKeepsHighestSavingsNotFileOrder(t *testing.T) { countPerRow = 6 ) - recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,all-upfront,123456789012 `) var ( @@ -537,8 +537,8 @@ func TestCSVCapDropsTruncationBelowMinCount(t *testing.T) { countPerRow = 6 ) - recs := csvRecsFrom(t, `rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,all-upfront,123456789012 `) var ( @@ -571,8 +571,8 @@ func TestCSVMinCountDropsRowsUnderTheFloor(t *testing.T) { isolateAWSEnv(t) const minCount = 5 - recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,2,500.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,postgres,6,100.00,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,2,500.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,100.00,1yr,all-upfront,123456789012 `) var ( @@ -602,9 +602,9 @@ func TestRunToolFromCSVEnforcesMinCountAndCap(t *testing.T) { // Worst-value row first, so file order and savings order disagree. csvPath := writeTestRecommendationsCSV(t, csvFixtureHeader+ - `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,All Upfront,123456789012 + `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,6,100.00,1yr,all-upfront,123456789012 `) tests := []struct { @@ -688,8 +688,8 @@ func TestCSVCapRanksBySavingsPerInstance(t *testing.T) { isolateAWSEnv(t) const maxInstances = 10 - recs := csvRecsFrom(t, `rds,us-east-1,db.bulk.large,postgres,100,600.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.rich.large,postgres,6,500.00,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.bulk.large,postgres,100,600.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.rich.large,postgres,6,500.00,1yr,all-upfront,123456789012 `) var ( @@ -737,8 +737,8 @@ func TestCSVCapRefusesRowsWithoutARankingSignal(t *testing.T) { // Worst alphabetically first, so a name-ordered cap is visible: it // keeps db.aaa.large and drops db.zzz.large regardless of file order. recs := csvRecsFromHeader(t, csvNoSavingsHeader, - `rds,us-east-1,db.zzz.large,postgres,6,1yr,All Upfront,123456789012 -rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 + `rds,us-east-1,db.zzz.large,postgres,6,1yr,all-upfront,123456789012 +rds,us-east-1,db.aaa.large,postgres,6,1yr,all-upfront,123456789012 `) var err error @@ -756,8 +756,8 @@ rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 // The partial case is the worse one: the blank row loads as $0, ranks // last, and is dropped first, with nothing saying the value was absent // rather than genuinely zero. - recs := csvRecsFrom(t, `rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.small,postgres,6,,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.small,postgres,6,,1yr,all-upfront,123456789012 `) var err error @@ -774,8 +774,8 @@ rds,us-east-1,db.t3.small,postgres,6,,1yr,All Upfront,123456789012 // Nothing has to be chosen between, so nothing has to be ranked. A // guard that over-blocks is as wrong as one that never fires. recs := csvRecsFromHeader(t, csvNoSavingsHeader, - `rds,us-east-1,db.zzz.large,postgres,6,1yr,All Upfront,123456789012 -rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 + `rds,us-east-1,db.zzz.large,postgres,6,1yr,all-upfront,123456789012 +rds,us-east-1,db.aaa.large,postgres,6,1yr,all-upfront,123456789012 `) var ( @@ -792,8 +792,8 @@ rds,us-east-1,db.aaa.large,postgres,6,1yr,All Upfront,123456789012 }) t.Run("populated savings column still ranks and caps", func(t *testing.T) { - recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,All Upfront,123456789012 + recs := csvRecsFrom(t, `rds,us-east-1,db.t3.small,postgres,6,10.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,postgres,6,500.00,1yr,all-upfront,123456789012 `) var ( @@ -873,8 +873,8 @@ func TestCSVCapOrderIsIndependentOfFileOrder(t *testing.T) { // Two rows at the identical rate ($100 for 6 instances), listed // worst-alphabetically-first. rows := []string{ - "rds,us-east-1,db.t3.zulu,postgres,6,100.00,1yr,All Upfront,123456789012\n", - "rds,us-east-1,db.t3.alpha,postgres,6,100.00,1yr,All Upfront,123456789012\n", + "rds,us-east-1,db.t3.zulu,postgres,6,100.00,1yr,all-upfront,123456789012\n", + "rds,us-east-1,db.t3.alpha,postgres,6,100.00,1yr,all-upfront,123456789012\n", } selection := func(t *testing.T, rows []string) []string { diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index fadc4c585..99644bf9b 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1700,8 +1700,8 @@ func TestRunToolFromCSV(t *testing.T) { // silently ignored, so every row loaded with Count=0 and the whole run // processed nothing while the NotPanics assertion stayed green. csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,111111111111 -elasticache,us-west-2,cache.t3.micro,redis,1,1yr,All Upfront,111111111111 +rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111 +elasticache,us-west-2,cache.t3.micro,redis,1,1yr,all-upfront,111111111111 `) tests := []struct { @@ -1768,7 +1768,7 @@ func TestRunToolFromCSV_DuplicateCheckFailureRefusesToPurchase(t *testing.T) { isolateAWSEnv(t) csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,111111111111 +rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111 `) reportPath := filepath.Join(t.TempDir(), "report.csv") auditPath := filepath.Join(t.TempDir(), "audit.jsonl") @@ -1836,9 +1836,9 @@ func TestRunToolFromCSV_WithMaxInstances(t *testing.T) { // rather than capped by name; that case is covered by // TestCSVCapRefusesRowsWithoutARankingSignal. csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,EstimatedSavings,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,10,100.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,mysql,10,200.00,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.large,postgres,10,300.00,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.small,postgres,10,100.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,mysql,10,200.00,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.large,postgres,10,300.00,1yr,all-upfront,123456789012 `) reportPath := filepath.Join(t.TempDir(), "report.csv") @@ -1871,8 +1871,8 @@ func TestRunToolFromCSV_WithOverrideCount(t *testing.T) { isolateAWSEnv(t) csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,10,1yr,All Upfront,123456789012 -rds,us-east-1,db.t3.medium,mysql,5,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.small,postgres,10,1yr,all-upfront,123456789012 +rds,us-east-1,db.t3.medium,mysql,5,1yr,all-upfront,123456789012 `) reportPath := filepath.Join(t.TempDir(), "report.csv") @@ -1917,7 +1917,7 @@ func TestRunToolFromCSV_WritesAuditRecordsOnDryRun(t *testing.T) { isolateAWSEnv(t) csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,111111111111 +rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111 `) auditPath := filepath.Join(t.TempDir(), "audit.jsonl") @@ -2025,7 +2025,7 @@ func TestRunToolFromCSV_ChecksAuditLogWritability(t *testing.T) { defer func() { toolCfg = origCfg }() csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,111111111111 +rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111 `) toolCfg.CSVInput = csvPath @@ -2060,8 +2060,8 @@ func TestRunToolFromCSV_ConfirmsOnceBeforeAnyRegion(t *testing.T) { t.Cleanup(func() { log.SetOutput(oldWriter) }) csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account -rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,111111111111 -rds,us-west-2,db.t3.medium,postgres,3,1yr,All Upfront,123456789012 +rds,us-east-1,db.t3.small,postgres,2,1yr,all-upfront,111111111111 +rds,us-west-2,db.t3.medium,postgres,3,1yr,all-upfront,123456789012 `) reportPath := filepath.Join(t.TempDir(), "report.csv") auditPath := filepath.Join(t.TempDir(), "audit.jsonl") diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 5a4c5820d..dbf095228 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -331,6 +331,10 @@ 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} + for i := range in { + // The CSV loader requires Term and PaymentOption on every row. + in[i].Term, in[i].PaymentOption = "1yr", "no-upfront" + } results := make([]common.PurchaseResult, 0, len(in)) for _, r := range in { results = append(results, common.PurchaseResult{Recommendation: r, Success: true, Timestamp: time.Now()}) @@ -375,7 +379,7 @@ func TestCSVWriter_AppendsDetailColumnsWithoutReorderingExistingOnes(t *testing. 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" + content := "Service,Region,ResourceType,Count,PlanType,HourlyCommitment,Term,PaymentOption\nsavings-plans-compute,,Compute,1,Compute,abc,1yr,no-upfront\n" require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) _, err := loadRecommendationsFromCSV(path) require.Error(t, err) diff --git a/docs/cli/README.md b/docs/cli/README.md index 17f69e539..b853b757f 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -88,7 +88,7 @@ All flags belong to the root command unless noted otherwise. | Flag | Short | Default | Description | |------|-------|---------|-------------| | `--output` | `-o` | auto-generated | Output CSV file path. Auto-name format: `ri-helper-dryrun-YYYYMMDD-HHMMSS.csv` or `ri-helper-purchase-YYYYMMDD-HHMMSS.csv`. | -| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. | +| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. The header must contain `Service`, `Region`, `ResourceType`, `Count`, `Term` and `PaymentOption`; a missing column is a load error that names it. `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or unknown value fails the load with the row number. CSVs with only the older four columns are rejected. | ### Authentication