From 7579572b5b2bd61317e02bee3ff7e116283ff3e5 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 21:44:12 +0100 Subject: [PATCH] fix(csv): honor an explicit --coverage 80 on --input-csv runs determineCSVCoverage compared cfg.Coverage against the flag's default (80) to decide whether the user had asked for a coverage, so an explicit --coverage 80 was indistinguishable from no flag and the CSV run bought 100% of every row's Count. runTool now records cobra's Changed("coverage") in a new Config.CoverageFromDefault field, and determineCSVCoverage returns 100 only when that is set. With no flag a CSV run still buys each row's full Count; any explicit value, including 80, is applied. A Config built in code leaves the field false, so its Coverage is honored as given. The non-CSV path never reads the field. Closes #2236 Co-Authored-By: Claude Opus 5.5 --- cmd/csv_coverage_flag_test.go | 89 +++++++++++++++++++++++++++++++++++ cmd/main.go | 11 ++++- cmd/multi_service_csv.go | 9 ++-- cmd/multi_service_csv_test.go | 13 ++++- docs/cli/README.md | 2 +- 5 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 cmd/csv_coverage_flag_test.go diff --git a/cmd/csv_coverage_flag_test.go b/cmd/csv_coverage_flag_test.go new file mode 100644 index 000000000..1f84bae39 --- /dev/null +++ b/cmd/csv_coverage_flag_test.go @@ -0,0 +1,89 @@ +package main + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + csvCoverageArgsEnv = "CUDLY_CSV_COVERAGE_TEST_ARGS" + csvCoverageWantEnv = "CUDLY_CSV_COVERAGE_TEST_WANT" +) + +// TestCSVCoverageFlagThroughRootCommand reproduces #2236 through the real +// command: cobra flag parsing, validateFlags, runTool and the --input-csv +// dry run. Before the fix an explicit --coverage 80 equaled the flag's default, +// so determineCSVCoverage treated it as unset and every row was bought at its +// full Count. Each case runs in a child process so the flag state cobra keeps +// on rootCmd (values and Changed bits) never leaks between cases or tests. +func TestCSVCoverageFlagThroughRootCommand(t *testing.T) { + if args, ok := os.LookupEnv(csvCoverageArgsEnv); ok { + runCSVCoverageChild(t, args, os.Getenv(csvCoverageWantEnv)) + return + } + tests := []struct { + name string + args []string + want string + }{ + {"no flag buys each row's full count", nil, "10,20"}, + {"explicit 80 is honored", []string{"--coverage", "80"}, "8,16"}, + {"explicit 80 via shorthand is honored", []string{"-c", "80"}, "8,16"}, + {"explicit 80 with equals is honored", []string{"--coverage=80"}, "8,16"}, + {"explicit 100 buys each row's full count", []string{"--coverage", "100"}, "10,20"}, + {"explicit 50 halves each row", []string{"--coverage", "50"}, "5,10"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + child := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestCSVCoverageFlagThroughRootCommand$", "-test.v") + child.Env = []string{ + csvCoverageArgsEnv + "=" + strings.Join(tt.args, "\x1f"), + csvCoverageWantEnv + "=" + tt.want, + } + output, err := child.CombinedOutput() + require.NoError(t, err, "%s", output) + assert.Contains(t, string(output), "--- PASS: TestCSVCoverageFlagThroughRootCommand") + }) + } +} + +// runCSVCoverageChild executes rootCmd with the case's flags on a dry-run +// --input-csv invocation and checks the per-row purchase counts in the report. +func runCSVCoverageChild(t *testing.T, joinedArgs, want string) { + awsHits := isolateAWSEnv(t) + dir := t.TempDir() + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account,EstimatedSavings +rds,us-east-1,db.t3.small,postgres,10,1yr,all-upfront,111111111111,100 +rds,us-east-1,db.t3.medium,postgres,20,1yr,all-upfront,111111111111,200 +`) + reportPath := filepath.Join(dir, "report.csv") + args := []string{"--input-csv", csvPath, "--output", reportPath, "--audit-log", filepath.Join(dir, "audit.jsonl")} + if joinedArgs != "" { + args = append(args, strings.Split(joinedArgs, "\x1f")...) + } + rootCmd.SetArgs(args) + require.NoError(t, rootCmd.Execute()) + + header, rows := readPurchaseReport(t, reportPath) + byType := map[string]string{} + types := reportColumn(t, header, rows, "ResourceType") + for i, count := range reportColumn(t, header, rows, "Count") { + byType[types[i]] = count + } + wantCounts := strings.Split(want, ",") + assert.Equal(t, map[string]string{"db.t3.small": wantCounts[0], "db.t3.medium": wantCounts[1]}, byType) + for _, dryRun := range reportColumn(t, header, rows, "Success") { + assert.Equal(t, "true", dryRun) + } + assert.Positive(t, awsHits.Load(), "the local AWS endpoint must have served the duplicate checks") +} diff --git a/cmd/main.go b/cmd/main.go index eb77a29be..f2c0baf3a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -77,6 +77,12 @@ type Config struct { IncludeExtendedSupport bool AllServices bool ActualPurchase bool + // CoverageFromDefault is true when --coverage was not passed on the + // command line, so Coverage holds the flag's default. runTool sets it + // from cobra's Changed(); --input-csv runs use it to buy each row's full + // Count when no coverage was asked for (determineCSVCoverage). A Config + // built in code leaves it false, so its Coverage is always honored. + CoverageFromDefault bool // RecLookbackPeriod controls the LookbackPeriodInDays passed to // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d" // (recommendations.DefaultRecLookbackPeriod is the shared default). @@ -391,8 +397,11 @@ func runTool(cmd *cobra.Command, args []string) { ctx, stop := invocationContext() defer stop() + cfg := toolCfg + cfg.CoverageFromDefault = cmd == nil || !cmd.Flags().Changed("coverage") + // Always use the multi-service implementation - runToolMultiService(ctx, toolCfg) + runToolMultiService(ctx, cfg) } // invocationContext returns the context for one CLI invocation, canceled by diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 08621807a..58dd7593d 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -20,11 +20,12 @@ import ( ) // determineCSVCoverage determines the coverage percentage to use for CSV mode. +// Without --coverage on the command line a CSV run buys each row's full Count +// (100%). An explicit --coverage is always honored, including a value equal to +// the flag's default (80): the check uses CoverageFromDefault, which runTool +// sets from cobra's Changed(), never a comparison against the default value. func determineCSVCoverage(cfg Config) float64 { - // When using CSV input, default to 100% coverage (use exact numbers from CSV) - // unless user explicitly provided a different coverage value - if cfg.Coverage == 80.0 { - // User didn't override the default, so use 100% for CSV mode + if cfg.CoverageFromDefault { return 100.0 } return cfg.Coverage diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index 4ec85e604..a1ea58c4e 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -21,12 +21,21 @@ func TestDetermineCSVCoverage(t *testing.T) { expected float64 }{ { - name: "Default coverage (80) changed to 100 for CSV", + name: "No --coverage flag buys 100% for CSV", cfg: Config{ - Coverage: 80.0, + Coverage: 80.0, + CoverageFromDefault: true, }, expected: 100.0, }, + { + // #2236: an explicit value equal to the flag's default is honored. + name: "Explicit 80% coverage preserved", + cfg: Config{ + Coverage: 80.0, + }, + expected: 80.0, + }, { name: "User-specified coverage preserved", cfg: Config{ diff --git a/docs/cli/README.md b/docs/cli/README.md index 06b04ff66..532c1922f 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -30,7 +30,7 @@ All flags belong to the root command unless noted otherwise. | Flag | Short | Default | Description | |------|-------|---------|-------------| -| `--coverage` | `-c` | `80` | Percentage (0-100) of each recommendation's instance count to purchase (`rec.Count * coverage/100`). Ignored when `--target-coverage` is also set. | +| `--coverage` | `-c` | `80` | Percentage (0-100) of each recommendation's instance count to purchase (`rec.Count * coverage/100`). Ignored when `--target-coverage` is also set. With `--input-csv`, omitting the flag buys each row's full `Count` (100%); any explicitly passed value, including `80`, is applied. | | `--target-coverage` | `-u` | `0` (disabled) | Target percentage (0-100) of historical average hourly usage to cover with commitments. Sizes each recommendation to `floor(avg * target/100)`, leaving the remainder on-demand. Overrides `--coverage` when non-zero. Pairs with `--rebuy-window-days` and `--min-pool-size` (see [filtering.md](filtering.md)). | | `--coverage-lookback-days` | | `30` | Calendar days of historical demand fed to `GetReservationCoverage` when computing the existing-RI coverage map for `--target-coverage` sizing. Match this to your AWS console coverage report window to reconcile cudly's `ExistingCoverage` column against the console export. Only affects `--target-coverage`. | | `--override-count` | | `0` (disabled) | Replace every recommendation's count with this fixed number, rescaling that row's costs and savings by the same ratio so they describe the quantity that will actually be bought. Savings Plans are exempt and pass through unchanged: an SP commitment is priced in dollars per hour, not in instances. Rows with a non-positive count are exempt too, having no quantity to rescale from. Overriding above the quantity the provider's figures cover keeps the costs accurate but extrapolates the savings past the observed demand; all three cases are named on stdout. Applied before `--max-instances`. Useful when testing a specific purchase size. |