Repository navigation
Conversation
…ithout one in the dry run The CSV report carried EC2 Tenancy and Scope but not AvailabilityZone, so a zonal EC2 row exported and replayed with --input-csv lost its AZ. Since the go pin bump (go#305) the library buys a zonal RI only in the rec's AZ and refuses a zonal rec without one, so the replayed row was refused at purchase time while validatePurchasePreconditions, which does not look at the AZ, let the dry run report success. Append an AvailabilityZone detail column after Scope (blank for regional rows and other services; columns are looked up by name, so older CSVs still load) and read it back into ComputeDetails. Mirror the library's scope/AZ check in the EC2 preconditions so the dry run refuses a zonal row with no AZ, or a regional row that names one, exactly as the real run does. Closes LeanerCloud#2174 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
csvDetailColumnscarried EC2TenancyandScopebut notAvailabilityZone, so a zonal EC2 row exported by cudly and replayed with--input-csvcame back withComputeDetails.AvailabilityZoneempty. Since the go pin bump in #2173 (go#305 / fb39c9d) the library buys a zonal RI only in the rec's AZ and refuses a zonal rec without one (buildEC2OfferingQuery), so the real run refused the row.validatePurchasePreconditionsnever looked at the AZ, so the dry run reported success for the same row.Fix
cmd/multi_service_csv_details.go): newAvailabilityZonedetail column, appended last afterScope(the "append, never reorder" rule from fix(cli): validate CSV headers, encoding and numeric bounds #2145 / owner item 42; columns are looked up by name). The writer fills it fromComputeDetails.AvailabilityZone(pointer and value shapes); the reader puts it back. It is blank for regional EC2 rows and every other service. It is not a required column, so the required set from fix(cli): validate CSV headers, encoding and numeric bounds #2145 (Service, Region, ResourceType, Count, Term, PaymentOption) does not change and older CSVs still load.cmd/savings_plan_purchase.go): the EC2 branch moves intovalidateEC2Preconditions(keeps gocyclo under 10) and addsvalidateEC2AvailabilityZone, which mirrors the library: zonal scope (Availability Zone, plus the legacyavailability-zone/availability zonespellingsparseEC2Scopeaccepts) with a blank or whitespace AZ is refused, andRegionscope with a non-blank AZ is refused. Both the main path (purchaseSingleRec) and the CSV path (runOnePurchase) already call this before the dry-run/real split, so dry run and real run now refuse the same rows before any AWS call.--input-csvrow indocs/cli/README.mddocuments the column and the refusal. There is also a CHANGELOG entry.User-visible change: every CSV report gets one more trailing column. A zonal EC2 row from a CSV written before this change (no
AvailabilityZonecolumn) is now refused in the dry run too, with a message saying to re-export or add the AZ. Since #2173 the real run already refused it.Closes #2174
Verification
All of this is unit, fixture and mock-transport evidence (
awsFixtureStub, anonymous credentials,isolateAWSEnvdeny endpoint). No real AWS call, no--purchase/--yes, and no real-account coverage: there is no live check of a zonal offering lookup.New tests in
cmd/csv_zonal_az_test.go:TestCSVRoundTrip_ZonalEC2KeepsAvailabilityZone: a zonal rec (us-east-1b) and a regional rec go through the realwriteMultiServiceCSVReportand back throughloadRecommendationsFromCSV. The zonal row keepsus-east-1band the regional row stays blank.TestCSVRealPurchase_ZonalEC2BuysInItsAZ(write -> read -> purchase): the exported CSV is loaded and bought withrunCSVPurchases(isDryRun=false)against the stub. EveryDescribeReservedInstancesOfferingsrequest carriesAvailabilityZone=us-east-1b. The purchase uses theus-east-1boffering, not theus-east-1aone listed first in the fixture. The purchase report keeps the AZ, and nothing reaches the deny endpoint.TestCSVDryRun_RefusesZonalEC2RowsTheRealRunRefuses:runCSVPurchases(isDryRun=true)on five CSVs. A zonal row from a CSV without the column, a zonal row with a blank AZ, and a regional row with an AZ each getSuccess=falseand the refusal inError. A zonal row with its AZ and a regional row with no AZ both succeed. No offering or purchase request is made.TestValidatePurchasePreconditions_EC2AvailabilityZone: covers the scope spellings, the blank and whitespace AZ cases, and both directions of the check.TestDetailCells_AvailabilityZone: covers the pointer and valueComputeDetailsshapes in the writer.Fail before, pass after. I restored the parent's
cmd/multi_service_csv_details.goandcmd/savings_plan_purchase.go(origin/main 83a7193) and kept the new tests:expected "us-east-1b", actual "")errPurchasesFailed)Successtrue,Errorempty)An error is expected but got nil)Commands, run on macOS with go1.26 and
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause this machine's default AWS profile is invalid:go test ./cmd -count=1: ok (the full package;./cmdis the only package in the module)go test -race -short -count=1 ./cmd -run 'Zonal|AvailabilityZone|ValidatePurchasePreconditions|CSV|DetailCells': okmake build,go vet ./...,go mod tidy -diff,make tidy-check: cleangolangci-lint run ./...(v2.10.1): 0 issuesgocyclo -over 10 -ignore "_test\.go" cmd/: silentbash scripts/gosec-hook.sh <changed files>: 0 issuesmarkdownlint-cli@0.47.0 CHANGELOG.md docs/cli/README.md: cleangit secrets --scanin a freshgit initcopy with an isolated HOME: clean.scripts/test-git-secrets-allowlist.sh: 44 PASS, 0 FAILOut of scope (noted, not changed)
ScopeorTenancyvalue (for exampleregional, which the library'sparseEC2Scoperefuses). The new AZ check only acts on recognized scopes, and the library still refuses the rest at purchase. Closing that gap would mean changing the existingvalidEC2Recfixture (Scope: "regional") that many tests share, so I left it for a separate issue.Maintainer: please mirror the issue's triage labels on this PR:
triaged,priority/p2,severity/medium,urgency/this-quarter,impact/few,effort/m,type/bug(I have no label permission on this repo).🤖 Generated with Claude Code