Skip to content

feat(sp): add --sp-account-scope to choose linked or payer recommendations - #2185

Merged
cristim merged 3 commits into
mainfrom
feat/2180-sp-account-scope
Oct 13, 2026
Merged

cristim merged 3 commits into
mainfrom
feat/2180-sp-account-scope

Conversation

@cristim

@cristim cristim commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Closes #2180

What

New --sp-account-scope linked|payer flag (default linked, so current behaviour is unchanged). Validated up front (validateSPAccountScope), carried in Config.SPAccountScope, and passed to the provider as RecommendationParams.SPAccountScope. Documented in docs/cli/README.md. RI recommendations are not affected; whether they need the same is tracked in #2181.

Dependency (read before merging)

Depends on LeanerCloud/cloud-commitments-go#357, which adds RecommendationParams.SPAccountScope. This branch does not build against the currently pinned library, so CI will fail until the go.mod bump to the merged library commit is pushed to this branch. Order: merge go#357, then bump github.com/LeanerCloud/cloud-commitments-go/{pkg,providers/aws} here, re-run CI, then merge this PR. No replace or go.work change is committed.

Evidence

Built against the go#357 branch via a go.work kept outside the repo.

  • TestFetchRecommendationsForRegion_PassesSPAccountScope: without the cmd/multi_service_helpers.go line, FAIL (expected "payer" actual ""); with it PASS. TestValidateSPAccountScope covers empty/linked/payer/PAYER and rejects org. Full go test ./... passes (1353); go vet clean.
  • Real data, dry run only, management account , --services savingsplans --include-sp-types Compute --term 1 --payment no-upfront --rec-lookback-period 30d --coverage 100:
    • --sp-account-scope payer: 1 recommendation, HourlyCommitment 2.787, $166.80/mo, 3.2%, matching aws ce ... --account-scope PAYER.
    • default (linked): "No recommendations found".

🤖 Generated with Claude Code

…tions

Pass the new RecommendationParams.SPAccountScope through to the provider.
Default stays linked. Requires cloud-commitments-go#357; the go.mod bump
lands once that is merged.

Closes #2180

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/feat New capability 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 6 billable files and costs up to $1.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 22 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 61 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: fb7e989c-5dbd-4918-84c0-9f6b28f89b09
📥 Commits

Reviewing files that changed from the base of the PR and between 68f6b77 and bbeec06.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • cmd/main.go
  • cmd/multi_service_helpers.go
  • cmd/sp_account_scope_test.go
  • cmd/validators.go
  • docs/cli/README.md
  • go.mod
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

cristim and others added 2 commits October 13, 2026 20:33
Brings RecommendationParams.SPAccountScope and its provider support.

Refs #2180

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 13, 2026 •

Copy link
Copy Markdown
Member Author

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

Dependency pin

  • go.mod pins pkg and providers/aws at v0.0.0-20261013183209-f794d75afb69. In cloud-commitments-go, f794d75 is the merge commit of cli: make --target-coverage CE lookback window configurable #360 and sits on main's first-parent line. Its parent chain includes cdcdc7f (fix(api/auth): base64-decode new_password in resetPassword handler (P0 — invited users locked out, closes #356) #357).
  • Only those four go.sum lines change. go mod verify reports "all modules verified", go mod tidy -diff is clean, and GOWORK=off go build ./cmd succeeds.
  • go.mod has no replace. The tracked go.work is pre-existing (use . only) and this PR leaves it unchanged.
  • Blast radius of the bump, 218912b..f794d75: five library commits. Besides the scope change, they bring a ladder refactor, the EC2 coverage scope fix and the SP coverage series. The CLI does not import pkg/ladder or providers/aws/ladder. Its recommendations.* call sites (PoolCoverageMap, ApplyCoverageMapToRecommendations, the RDS family sizing functions) were changed only by a coverageWindow extraction. The full suite passes.

Behaviour against the library

  • providers/aws/recommendations/collection_sp.go:36-39,61-72 (convertSavingsPlansAccountScope): "" and "linked" map to LINKED, "payer" maps to PAYER, matching is case-insensitive, and an unknown value is an error. The CLI's validateSPAccountScope (cmd/validators.go:53-60) accepts the same set, so the two agree.
  • The validator runs in root PreRunE (cmd/main.go:96) and fails before any AWS call.
  • fetchRecommendationsForRegion is the only GetRecommendations call site, and the wiring is at cmd/multi_service_helpers.go:610.

Local verification

  • gofmt and go vet ./... are clean, go test -count=1 ./... passes, and the git-secrets self-test reports 44 PASS, 0 FAIL.
  • Fail-before (fixtures): with cmd/main.go, cmd/validators.go and cmd/multi_service_helpers.go reverted to base 68f6b77 and the new tests kept, the package does not build: toolCfg.SPAccountScope undefined, undefined: validateSPAccountScope, unknown field SPAccountScope in struct literal of type Config.
  • Mutation (fixtures):
    • Deleting SPAccountScope: cfg.SPAccountScope, makes TestFetchRecommendationsForRegion_PassesSPAccountScope fail (expected "payer", actual "").
    • Adding "org" to the accepted set makes TestValidateSPAccountScope fail (error expected, got nil).
  • Real dry run, management account (describe-organization MasterAccountId matches). No --purchase. Flags: --services savingsplans --include-sp-types Compute --term 1 --payment no-upfront --rec-lookback-period 30d --coverage 100.
    • aws ce get-savings-plans-purchase-recommendation --savings-plans-type COMPUTE_SP ... --account-scope PAYER: 2.787/hr, $166.7975/mo, 3.197%. With LINKED: no details.
    • --sp-account-scope payer: one row, <payer-account-name>, savings-plans-compute, $166.80, 3.2%. CSV HourlyCommitment is 2.787 and Region is empty. The dry-run ID uses global.
    • --sp-account-scope linked: "No recommendations found".
    • --sp-account-scope org: exit 1, invalid sp-account-scope "org": must be linked or payer. No output files are written.

Non-blocking findings

  1. docs/cli/README.md:76: "A management account may only get recommendations at payer scope" is not accurate. LINKED from the management account is valid; here it simply returned no recommendations, without an error. The page also does not say what payer means. Suggested wording: "payer sizes one plan against usage across the whole organization and is meaningful only from the management account; linked sizes against each account's own usage." The flag help text at cmd/main.go:137 could say the same.
  2. Under payer, the row is labelled with the payer account's name and ID, which is the account Cost Explorer returns and the one the plan would be bought in, but the plan covers the whole organization. This is acceptable as is. A short note in the docs would stop readers taking the savings figure as that account's alone.

CI at review time: every check passes except Unit Tests, which is still pending, so the CI Success gate has not reported yet. git merge-tree against main reports no conflicts.

@cristim
cristim merged commit 8ddb891 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/feat New capability urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(sp): allow PAYER account scope for Savings Plan recommendations

1 participant