Skip to content

fix(sp): fetch only the Savings Plan types requested by --include/--exclude-sp-types - #2183

Merged
cristim merged 1 commit into
mainfrom
fix/2177-sp-types-narrow
Oct 13, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/2177-sp-types-narrow

Conversation

@cristim

@cristim cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member

Closes #2177

Root cause

parseServices expands savingsplans into four per-type slugs. Each slug sets RecommendationParams.Service to one plan type, and the library's planTypesForParams returns that single type as soon as the service is a per-type slug, so IncludeSPTypes/ExcludeSPTypes (cmd/multi_service_helpers.go fetchRecommendationsForRegion) are never consulted. The fix narrows the SP service list in determineServicesToProcess (new filterSavingsPlanTypes), so only requested types are fetched. Names match case-insensitively; an unknown name is an error rather than silently ignored. Non-SP services are untouched. No library change.

Evidence

Regression test TestDetermineServicesToProcess_SavingsPlanTypeFilters.

  • Before (source change stashed): FAIL on include/exclude cases (all four types returned) and on the unknown-name case (no error).
  • After: PASS. Full go test ./... passes (1358); go vet clean. make lint cannot run locally (golangci-lint built with older Go than go.mod); CI is authoritative.

Real data, dry run only, --services savingsplans --include-sp-types EC2Instance ...: main prints Processing services: Compute, EC2 Instance, SageMaker, Database Savings Plans; this branch prints Processing services: EC2 Instance Savings Plans and queries only that type. (Recommendations themselves need #2176, which is a separate PR.)

🤖 Generated with Claude Code

…xclude-sp-types

Each per-plan-type service queries exactly one plan type and the provider
ignores IncludeSPTypes/ExcludeSPTypes for it, so the flags never narrowed
the fetch. Filter the SP service list in determineServicesToProcess and
reject unknown type names.

Closes #2177

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/few Limited audience effort/s Hours type/bug Defect labels Oct 13, 2026
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 41 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 9a79fdfb-5230-4192-a60a-55539d4bd454
📥 Commits

Reviewing files that changed from the base of the PR and between 00d82c3 and df5f97b.

📒 Files selected for processing (2)
  • cmd/multi_service_helpers.go
  • cmd/multi_service_helpers_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Independent review (claude-opus-5-5) of df5f97b: APPROVE

Root cause (confirmed against the pinned library)

providers/aws@v0.0.0-20261012195142-218912bb8cb9, recommendations/parser_sp.go:268-272: planTypesForParams returns a single plan type for any of the four per-type slugs and only falls back to getFilteredPlanTypes(IncludeSPTypes, ExcludeSPTypes) for other slugs. cmd/main.go parseServices expands savingsplans into the four per-type slugs, so the include/exclude lists were never read. Filtering the service list in determineServicesToProcess is the right layer.

Findings (none blocking)

  • Empty result after filtering (for example --include-sp-types Compute --exclude-sp-types Compute, or --services savingsplans-compute --include-sp-types EC2Instance) exits 1 with the generic No valid services specified. Fails loud, but the message does not name the SP filters. Optional follow-up.
  • Behaviour change: an unknown SP type name now exits 1 (previously silently ignored), including when no SP service is selected. Consistent with fail-loud; docs/cli/filtering.md could mention it.
  • --input-csv mode does not go through determineServicesToProcess, so the SP type filters do not apply there. Pre-existing, out of scope.
  • spTypeWanted is a single-caller helper; acceptable (keeps the loop flat for gocyclo).
  • The base is 00d82c3; main has since gained fix(sp): stop filtering Savings Plan recommendations on the endpoint region #2182 (0a28aa4), which touches the same file in a different hunk. git merge-tree is clean.

Local verification (macOS, exact head)

  • go vet ./...: clean. go test ./...: 1358 passed, 0 failed.
  • Fail-before: new test kept, cmd/multi_service_helpers.go reverted to the parent. TestDetermineServicesToProcess_SavingsPlanTypeFilters: 5 of 6 subtests FAIL (include, exclude, include-then-exclude, non-SP untouched, unknown fails loud); only "no filter keeps all four" passes. Example: expected: [savings-plans-ec2instance], actual: [savings-plans-compute savings-plans-ec2instance savings-plans-sagemaker savings-plans-database].
  • Mutation 1 (include set short-circuits, exclude ignored): include_then_exclude FAILS (actual: [compute ec2instance]). Mutation 2 (drop strings.ToLower): include_narrows,_case-insensitive and exclude_drops FAIL.
  • Real dry runs against the shift-prd account (SSO creds exported to env, so --profile omitted), --services savingsplans --term 1 --payment no-upfront --rec-lookback-period 30d --coverage 100 --regions us-east-1, no --purchase/--yes:
    • head --include-sp-types EC2Instance: Processing services: EC2 Instance Savings Plans, one fetch line Fetching EC2 Instance Savings Plans recommendations.
    • head --exclude-sp-types Compute,SageMaker: Processing services: EC2 Instance Savings Plans, Database Savings Plans, two fetch lines (EC2 Instance, Database).
    • head --include-sp-types "compute, database": Compute and Database fetched.
    • head --include-sp-types bogus: exit 1, Invalid services: unknown Savings Plan type "bogus" in --include-sp-types; valid types: Compute, EC2Instance, SageMaker, Database.
    • parent --include-sp-types EC2Instance and parent --include-sp-types bogus: all four types fetched (Compute, EC2 Instance, SageMaker, Database), exit 0.
  • Coverage gap: the account returned 0 SP recommendations for every type in this window, so the runs prove which plan types are queried, not the content of returned recommendations.

CI is green on this SHA and the PR is mergeable.

@cristim
cristim merged commit d4010ba into main Oct 13, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(sp): --include-sp-types / --exclude-sp-types do not narrow the fetch

1 participant