From 441761f3fb8a3dbe2f3b26e856f258dd4350cef6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 16:07:03 +0200 Subject: [PATCH] fix(cli): use purchaseSucceeded in the interrupt summary and run stats A result with Success=true and a non-nil Error is a failure. Route the Ctrl-C summary, the audit-stop message, the per-service counts and the non-zero exit check through purchaseSucceeded so they agree with the audit status. The CSV Success column stays the raw provider flag and is documented as such; the Error column carries the error. Closes #2205 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 + cmd/interrupt.go | 4 +- cmd/multi_service_stats.go | 4 +- cmd/purchase_succeeded_summaries_2205_test.go | 76 +++++++++++++++++++ docs/cli/README.md | 2 +- 5 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 cmd/purchase_succeeded_summaries_2205_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index cdcd70868..388f6888c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- A purchase result with `Success=true` and a non-nil `Error` is now treated as + a failure in the Ctrl-C summary, the audit-stop message, the per-service + success/failed counts and the non-zero exit. The CSV `Success` column keeps the + raw provider flag; read the `Error` column alongside it (#2205). - `configure gcp`: pressing Enter at a `[r]un, [S]kip (default)?` step now skips instead of creating the service account, granting roles, minting a key or running `gcloud auth login`; type `r` to run a step (#1613). diff --git a/cmd/interrupt.go b/cmd/interrupt.go index 7787b3348..eaa570233 100644 --- a/cmd/interrupt.go +++ b/cmd/interrupt.go @@ -40,7 +40,7 @@ func printInterruptedSummary(attempted, total int, results []common.PurchaseResu } for i := range results { r := results[i] - if r.Success { + if purchaseSucceeded(r) { continue } AppLogger.Printf(" %s\n", unverifiedPurchaseLine(r)) @@ -62,7 +62,7 @@ func auditStopMessage(attempted, total int, results []common.PurchaseResult, cau if n := len(results); n > 0 { r := results[n-1] last = fmt.Sprintf("last attempted: %s %s", r.Recommendation.Service, r.Recommendation.ResourceType) - if r.Success { + if purchaseSucceeded(r) { last += " (commitment " + r.CommitmentID + ")" } else { last += " (errored: " + fmt.Sprint(r.Error) + ")" diff --git a/cmd/multi_service_stats.go b/cmd/multi_service_stats.go index c53d8ced4..90dd798dc 100644 --- a/cmd/multi_service_stats.go +++ b/cmd/multi_service_stats.go @@ -20,7 +20,7 @@ func purchaseFailureError(results []common.PurchaseResult, isDryRun bool) error } failed := 0 for i := range results { - if !results[i].Success { + if !purchaseSucceeded(results[i]) { failed++ } } @@ -61,7 +61,7 @@ func calculateServiceStats(service common.ServiceType, recs []common.Recommendat for _rvc := range results { result := results[_rvc] - if result.Success { + if purchaseSucceeded(result) { stats.SuccessfulPurchases++ } else { stats.FailedPurchases++ diff --git a/cmd/purchase_succeeded_summaries_2205_test.go b/cmd/purchase_succeeded_summaries_2205_test.go new file mode 100644 index 000000000..3b09e9650 --- /dev/null +++ b/cmd/purchase_succeeded_summaries_2205_test.go @@ -0,0 +1,76 @@ +package main + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func succeededWithError() common.PurchaseResult { + return common.PurchaseResult{ + Success: true, + Error: errors.New("x"), + CommitmentID: "id-1", + Recommendation: common.Recommendation{ + Service: common.ServiceEC2, + ResourceType: "m5.large", + }, + } +} + +func TestPrintInterruptedSummary_SuccessWithErrorIsListed(t *testing.T) { + ok := common.PurchaseResult{Success: true} + bad := common.PurchaseResult{Success: false, Error: errors.New("boom")} + tests := []struct { + name string + results []common.PurchaseResult + want bool + }{ + {"success true with error", []common.PurchaseResult{succeededWithError()}, true}, + {"ordinary success", []common.PurchaseResult{ok}, false}, + {"success false with error", []common.PurchaseResult{bad}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := captureAppOutput(t, func() { printInterruptedSummary(1, 2, tt.results, false) }) + assert.Equal(t, tt.want, containsVerify(out), out) + }) + } +} + +func containsVerify(s string) bool { + return strings.Contains(s, "verify in AWS before re-running") +} + +func TestAuditStopMessage_SuccessWithErrorReportsError(t *testing.T) { + msg := auditStopMessage(1, 2, []common.PurchaseResult{succeededWithError()}, errors.New("cause")) + assert.Contains(t, msg, "(errored: x)") + assert.NotContains(t, msg, "(commitment") + + ok := common.PurchaseResult{Success: true, CommitmentID: "id-2"} + assert.Contains(t, auditStopMessage(1, 2, []common.PurchaseResult{ok}, errors.New("c")), "(commitment id-2)") + bad := common.PurchaseResult{Success: false, Error: errors.New("y")} + assert.Contains(t, auditStopMessage(1, 2, []common.PurchaseResult{bad}, errors.New("c")), "(errored: y)") +} + +func TestCalculateServiceStats_SuccessWithErrorCountsFailed(t *testing.T) { + results := []common.PurchaseResult{ + {Success: true}, + succeededWithError(), + {Success: false}, + {Success: false, Error: errors.New("z")}, + } + stats := calculateServiceStats(common.ServiceEC2, nil, results) + assert.Equal(t, 1, stats.SuccessfulPurchases) + assert.Equal(t, 3, stats.FailedPurchases) +} + +func TestPurchaseFailureError_SuccessWithErrorIsFailure(t *testing.T) { + err := purchaseFailureError([]common.PurchaseResult{{Success: true}, succeededWithError()}, false) + assert.ErrorIs(t, err, errPurchasesFailed) + assert.Contains(t, err.Error(), "1 of 2") +} diff --git a/docs/cli/README.md b/docs/cli/README.md index b37dca8d1..f607094f7 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -87,7 +87,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`. An explicit path that already exists (file, directory or symlink) is refused instead of overwritten; choose a new path or remove the old file. | +| `--output` | `-o` | auto-generated | Output CSV file path. Auto-name format: `ri-helper-dryrun-YYYYMMDD-HHMMSS.csv` or `ri-helper-purchase-YYYYMMDD-HHMMSS.csv`. An explicit path that already exists (file, directory or symlink) is refused instead of overwritten; choose a new path or remove the old file. The `Success` column is the raw flag the provider returned; a provider can report `true` together with an error, so read the `Error` column too (a row with an error counts as failed in the run summary). | | `--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