Skip to content

feat(aws): configurable account scope for RI purchase recommendations - #535

Merged
cristim merged 5 commits into
mainfrom
feat/508-ri-account-scope
Oct 19, 2026
Merged

cristim merged 5 commits into
mainfrom
feat/508-ri-account-scope

Conversation

@cristim

@cristim cristim commented Oct 19, 2026 •

Copy link
Copy Markdown
Member

Closes #508

Adds RecommendationParams.RIAccountScope (pkg/common), which mirrors the existing SPAccountScope:

  • It accepts "", "linked" or "payer", case-insensitively. Empty means LINKED, so existing callers are unchanged.
  • Any other value is rejected ("invalid RI account scope").

GetRecommendations (providers/aws/recommendations/client.go) passes the converted scope into GetReservationPurchaseRecommendationInput, which used to hardcode LINKED.

The SP validation switch (convertSavingsPlansAccountScope, collection_sp.go:64) is factored into a shared convertAccountScope(s, kind) that both paths use.

The consumer is cli#2181 (--ri-account-scope). It has not been verified against a real payer account.

Tests (client_ri_account_scope_test.go): a fake HTTP client captures AccountScope for unset, linked, payer and PAYER, plus the rejection of "org". With LINKED hardcoded again (main's behavior), the payer and PAYER cases fail.

The second commit bumps providers/aws's pkg pin to this branch's pkg commit, so the standalone (GOWORK=off) module builds, as in #515.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Reserved Instance recommendations can now use either linked-account or payer account scope. The default remains linked scope; values are case-insensitive, and unsupported values return an error.
  • Documentation
    • Updated the changelog to describe the new account-scope option.

cristim and others added 2 commits October 20, 2026 00:30
GetReservationPurchaseRecommendation was hardcoded to AccountScope=LINKED.
Add RecommendationParams.RIAccountScope (linked default, payer), validated
by the same converter as SPAccountScope, so a management account can request
payer-scope RI recommendations. Empty keeps linked; unknown values error.

Closes #508

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

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 42 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 87 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-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: aa1f2957-7ce9-4b06-999f-7dcdc420ba92
📥 Commits

Reviewing files that changed from the base of the PR and between 06f0eaf and 8e32780.

⛔ Files ignored due to path filters: 1
  • providers/aws/go.sum is excluded by !**/*.sum
📒 Files selected for processing: 6
  • CHANGELOG.md
  • pkg/common/types.go
  • providers/aws/go.mod
  • providers/aws/recommendations/client.go
  • providers/aws/recommendations/client_ri_account_scope_test.go
  • providers/aws/recommendations/collection_sp.go

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 68086c51-c748-49fa-b8fe-bcdf0cbfbd6a
📥 Commits

Reviewing files that changed from the base of the PR and between 71b12a6 and 06f0eaf.

⛔ Files ignored due to path filters: 1
  • providers/aws/go.sum is excluded by !**/*.sum
📒 Files selected for processing: 6
  • CHANGELOG.md
  • pkg/common/types.go
  • providers/aws/go.mod
  • providers/aws/recommendations/client.go
  • providers/aws/recommendations/client_ri_account_scope_test.go
  • providers/aws/recommendations/collection_sp.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

The change adds RIAccountScope to recommendation parameters. The AWS client converts this value before sending RI recommendation requests. Empty or linked values select linked scope; payer selects payer scope. Unknown values return an error without sending the request.

Changes: RI account scope

Layer / File(s) Summary
Account-scope contract and conversion
pkg/common/types.go, providers/aws/recommendations/collection_sp.go, CHANGELOG.md
RecommendationParams documents the RI scope values. The shared converter handles linked and payer values case-insensitively and reports invalid values using the recommendation kind. The changelog documents the option.
Configure and send RI scope
providers/aws/recommendations/client.go, providers/aws/go.mod, providers/aws/recommendations/client_ri_account_scope_test.go
GetRecommendations converts the RI scope before making the Cost Explorer request and uses the converted scope in the request. Tests cover empty, linked, payer, and unknown values. The AWS provider dependency version is updated.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Client
  participant convertAccountScope
  participant CostExplorer
  Caller->>Client: Call GetRecommendations with RIAccountScope
  Client->>convertAccountScope: Convert RI account scope
  convertAccountScope-->>Client: Return scope or conversion error
  Client->>CostExplorer: Send recommendation request with converted scope
Loading
Merge Risk: ⚪ Minimal · up to 06f0e

The change adds configurable linked or payer scope while preserving linked behavior for existing callers. No actionable merge-blocking risk is evident from the supplied context.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: configurable account scope for AWS Reserved Instance purchase recommendations.
Linked Issues check Passed Issue #508 requires an RI account-scope option, LINKED as the default, PAYER support, request forwarding, and tests for both scopes. RecommendationParams.RIAccountScope implements the option. `conve…
Out of Scope Changes check Passed The changes remain connected to issue #508. The shared converter refactor preserves existing Savings Plans validation. The RI tests verify the requested behavior. The changelog documents the feature, …
Full details: Linked Issues check

Explanation

Issue #508 requires an RI account-scope option, LINKED as the default, PAYER support, request forwarding, and tests for both scopes. RecommendationParams.RIAccountScope implements the option. convertAccountScope accepts empty or case-insensitive linked and payer, and GetRecommendations forwards the converted value to GetReservationPurchaseRecommendationInput. Tests cover unset, linked, payer, uppercase PAYER, and invalid input. The shared converter preserves the Savings Plans behavior.

Full details: Out of Scope Changes check

Explanation

The changes remain connected to issue #508. The shared converter refactor preserves existing Savings Plans validation. The RI tests verify the requested behavior. The changelog documents the feature, and the AWS pkg pin supports the standalone module build. No unrelated product behavior is shown.

Full details: Docstring Coverage

Explanation

Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • 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 19, 2026

Copy link
Copy Markdown
Member Author

Review I (independent, claude-cc-libs-rev-I): APPROVE at 0013799

  • (1) SP path unchanged. It delegates to convertAccountScope(s, "Savings Plans") with the same error text. The only match is parser_sp_account_scope_test.go:36 (ErrorContains), and the cli, mcp and platform don't match on it.
  • (2) One RI call site. client.go:229 fetchRIPageWithRetry. The other mentions are comments. sp_commitment_rate.go:138 is an SP rate lookup and stays LINKED.
  • (3) Zero value. It maps to LINKED, and TestRIAccountScope asserts it on the wire.
  • (4) Pin. pkg@37131464e21b resolves through the proxy and go mod verify passes. Unlike fix(aws/redshift): dedupe reserved-node purchases with DescribeReservedNodes instead of tags #515's pin, it is a branch-only commit, so it will be re-pinned to main's squash commit in a follow-up after merge.
  • (5) Mutation. Reverting to LINKED fails the payer/PAYER subtests.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Review J (independent, claude-cc-libs-rev-J): APPROVE at 0013799

  • (a) Symmetric with SPAccountScope.
    • The field sits next to it (types.go:557), with the same shared converter, the same accepted values and case handling, and rejection before any API call.
    • The cli's --sp-account-scope wiring (cmd/main.go:146, validators.go:50, multi_service_helpers.go:486) fits a --ri-account-scope the same way.
  • (b) AWS semantics.
    • AccountScope is generic for GetReservationPurchaseRecommendation, and there are no AccountId or linked filters that would conflict with PAYER.
    • Low: fetchSingleComboRecs (client.go:274) ignores RIAccountScope. Being fixed before merge.
  • (c) Build and tests. Build, vet and tests pass in pkg and providers/aws with GOWORK on and off.
  • (d) Size and CHANGELOG. Commits are about 82 and 6 lines, and the CHANGELOG entry is under Added.

Add Client.SetRIAccountScope, mirroring SetRecLookbackPeriod, so the
per-service discovery path no longer always requests LINKED.

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

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Review notes on f84cff4 (claude-cc-libs-rev-I and rev-J)

Both reviewers found SetRIAccountScope correct (a bad value fails loudly at use, and the test fails without the forwarding). But no consumer can reach it: RecommendationsClientAdapter doesn't wrap it, and the cli's --ri-account-scope path uses GetRecommendations with params, which this PR already covers. The setter is being reverted as YAGNI. GetRecommendationsForService stays LINKED until a consumer needs a scope there.

No reachable caller: the adapter does not wrap it and the cli path uses
GetRecommendations with params.

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

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Gate note (claude-cc-libs)

Head 06f0eaf reverts f84cff4. Its tree is byte-identical to 0013799 (git rev-parse 0013799^{tree} equals 06f0eaf^{tree}), which both reviewers approved. No new review is needed for an identical tree.

Resolve the CHANGELOG conflict by keeping both sides.

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

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Review J (independent, claude-cc-libs-rev-J): APPROVE at 8e32780

  • The pkg and providers patch-ids of the PR diff against main equal 0013799's.
  • 06f0eaf has an empty diff against 0013799.
  • There are no markers, and pkg plus providers/aws build with GOWORK=off.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Review I (independent, claude-cc-libs-rev-I): APPROVE at 8e32780

  • The pkg/ and providers/ diff against main is identical to 0013799's (149 lines, 7 files).
  • CHANGELOG keeps every line, with no markers.
  • go test -count=1 ./recommendations/ in providers/aws passes with GOWORK on and off.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Merge gate verdict (claude-cc-libs)

Every gate holds at head 8e32780:

  • Two independent reviews approve the reviewed head 8e32780. The branch was then updated from main, and the PR diff is byte-identical to the reviewed diff (compared mechanically).
  • CI is green on 8e32780, which is the reviewed change on top of current main.
  • 0 unresolved threads; MERGEABLE/CLEAN.

CodeRabbit is waived as a merge gate under the 2026-10-19 decision. Squash-merging.

@cristim
cristim merged commit 8937b9a into main Oct 19, 2026
15 checks passed
@cristim
cristim deleted the feat/508-ri-account-scope branch October 19, 2026 23:52
cristim added a commit that referenced this pull request Oct 20, 2026
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(recommendations): configurable account scope for RI purchase recommendations

1 participant