Repository navigation
Conversation
Term and PaymentOption cells are already checked against allow-lists (LeanerCloud#2145), but the Service cell was cast straight to a ServiceType. A --services alias such as "elasticsearch", "RDS", "rds " or the "savingsplans" umbrella loaded fine, then the purchase loop found no service client for it and skipped the row with "Service client not yet implemented", so the run quietly bought less than the file asked for. Service is now parsed with parseCSVEnum against the canonical slugs from getAllServices() (what the CSV report writes), failing the load with the row and column. Count is also capped at MaxReasonableInstances, the ceiling validateFlags already applies to --max-instances and --override-count. Closes LeanerCloud#379 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 |
# Conflicts: # CHANGELOG.md
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.
Summary
Most of this issue is already fixed on main. #2145 made
TermandPaymentOptionrequired and checks each row against the allow-listsvalidatePaymentAndTermuses. It also rejects aCountbelow 1. Two parts of the issue were still open, and this PR fixes them:parseCSVRecordpassed theServicecell straight tocommon.ServiceType. Values like the--servicesaliaselasticsearch,RDS,rds(trailing space), thesavingsplansumbrella orsavingsplans-computeall loaded without error. The purchase loop then found no client for them increateServiceClientand skipped the row withService client not yet implemented. The run bought less than the file asked for and still ended normally.Counthad no ceiling.Fix
Serviceis now parsed with the existingparseCSVEnum. The allowed values are the canonical slugs fromgetAllServices(), which are the nameswriteMultiServiceCSVwrites, so a report from one run still loads as input to the next. A blank or unknown value fails the whole load with the row and column, the same way a badTermorPaymentOptiondoes. Matching is exact, like the Term/PaymentOption checks from fix(cli): validate CSV headers, encoding and numeric bounds #2145. Flag aliases are rejected on purpose rather than normalized: the report never writes them, and rejecting them keeps this input path strict.parseCSVCountnow rejectsCount > MaxReasonableInstances(10000). This is the same ceilingvalidateFlagsalready puts on--max-instancesand--override-count.docs/cli/README.md(--input-csvrow) andCHANGELOG.mdare updated.Closes #379
Verification
All of this is unit and fixture evidence on macOS, using temporary CSV files. The end-to-end test runs
runToolFromCSVas a dry run against the repo'sisolateAWSEnvfake endpoint. No real cloud account was used.New tests in
cmd/multi_service_csv_strict_test.go:TestLoadRecommendationsFromCSV_ServiceValues: blank, whitespace,elasticsearch,rds,RDS,savingsplans,savingsplans-computeanddynamodbeach fail withline 3andcolumn 1 "Service".TestLoadRecommendationsFromCSV_ServiceAccepted: every service ingetAllServices()still loads.TestLoadRecommendationsFromCSV_CountUpperBound:Count = 10000loads and10001fails oncolumn 4 "Count".TestRunToolFromCSV_RejectsUnknownService: anelasticsearchrow stopsrunToolFromCSVbefore any AWS request (awsHits == 0) and before any report is written.Fail before, at origin/main c134fc7 with only the tests applied:
Pass after, at the PR head: all four pass.
Commands:
The tests ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause the default profile on the dev machine is invalid. The new tests don't need this (isolateAWSEnvalready sets them).Not covered: no real
--input-csvpurchase against an AWS account.Scope notes
--servicesaliases (elasticsearch,savingsplans-*,savingsplans) or a different case in theServicecolumn. Those rows were already never purchased. They were skipped mid-run, and now the load fails with an error naming the cell.parseServicesinstead of rejecting them, that is a small follow-up. Thesavingsplansumbrella has no single service, so it would still need to be rejected.Serviceare rejected rather than trimmed, to match the exact matching thatTermandPaymentOptionuse.Labels: I can't edit labels on this repo. Could a maintainer please copy the issue's triage labels to this PR:
bug,triaged,priority/p3,severity/low,urgency/eventually,impact/few,effort/s,type/bug.🤖 Generated with Claude Code