From b18b6561cd8e6a412dd3970cbd49b63c58371571 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 18:58:05 +0200 Subject: [PATCH 1/2] feat(aws/recommendations): selectable account scope for Savings Plan recommendations Add RecommendationParams.SPAccountScope (linked or payer, empty is linked). Savings Plan recommendations were hardcoded to LINKED, which returns nothing for Compute SP in a management account where PAYER returns the org-wide recommendation. Unknown values are an error. Closes #356 Co-Authored-By: Claude Opus 5.5 --- pkg/common/types.go | 4 ++ .../aws/recommendations/collection_sp.go | 20 +++++++++- .../parser_sp_account_scope_test.go | 38 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 providers/aws/recommendations/parser_sp_account_scope_test.go diff --git a/pkg/common/types.go b/pkg/common/types.go index 8b62a9d0..9565fe9d 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -525,6 +525,10 @@ type RecommendationParams struct { // Savings Plans specific filters IncludeSPTypes []string // Compute, EC2Instance, SageMaker, Database ExcludeSPTypes []string + // SPAccountScope selects the Cost Explorer account scope for Savings Plans + // recommendations: "linked" or "payer" (case-insensitive). Empty means + // linked. Payer scope only applies to a management account. + SPAccountScope string } // Account represents a cloud account/subscription/project. diff --git a/providers/aws/recommendations/collection_sp.go b/providers/aws/recommendations/collection_sp.go index faa904a2..96ed4b23 100644 --- a/providers/aws/recommendations/collection_sp.go +++ b/providers/aws/recommendations/collection_sp.go @@ -3,6 +3,7 @@ package recommendations import ( "context" "fmt" + "strings" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/costexplorer" @@ -32,11 +33,15 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params *com if err != nil { return nil, fmt.Errorf("invalid lookback period for Savings Plans recommendation: %w", err) } + scope, err := convertSavingsPlansAccountScope(params.SPAccountScope) + if err != nil { + return nil, err + } results := make([]serviceResult, 0, len(planTypes)) for _, planType := range planTypes { input := &costexplorer.GetSavingsPlansPurchaseRecommendationInput{ SavingsPlansType: planType, PaymentOption: payment, TermInYears: term, - LookbackPeriodInDays: lookback, AccountScope: types.AccountScopeLinked, + LookbackPeriodInDays: lookback, AccountScope: scope, } recs, fetchErr := c.fetchSPAllPages(ctx, input, params, planType) if canceled := collectionCancellation(fetchErr); canceled != nil { @@ -53,6 +58,19 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params *com return mergeServiceResults(results...) } +// convertSavingsPlansAccountScope maps RecommendationParams.SPAccountScope to +// the Cost Explorer scope. Empty is linked, the long-standing default; an +// unknown value is an error rather than a silent fallback. +func convertSavingsPlansAccountScope(s string) (types.AccountScope, error) { + switch strings.ToLower(s) { + case "", "linked": + return types.AccountScopeLinked, nil + case "payer": + return types.AccountScopePayer, nil + } + return "", fmt.Errorf("invalid Savings Plans account scope %q: want linked or payer", s) +} + func (c *Client) fetchSPAllPages( ctx context.Context, input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, diff --git a/providers/aws/recommendations/parser_sp_account_scope_test.go b/providers/aws/recommendations/parser_sp_account_scope_test.go new file mode 100644 index 00000000..fe072943 --- /dev/null +++ b/providers/aws/recommendations/parser_sp_account_scope_test.go @@ -0,0 +1,38 @@ +package recommendations + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func TestSPAccountScope(t *testing.T) { + t.Parallel() + for _, tc := range []struct{ in, want string }{ + {"", "LINKED"}, {"linked", "LINKED"}, {"payer", "PAYER"}, {"PAYER", "PAYER"}, + } { + t.Run(tc.in, func(t *testing.T) { + t.Parallel() + fixture := &spSDKFixture{respond: func(spSDKRequest) (map[string]any, error) { return spSDKPage(""), nil }} + params := spSDKParams(common.ServiceSavingsPlansCompute) + params.SPAccountScope = tc.in + _, err := spSDKClient(fixture).GetRecommendations(context.Background(), params) + require.NoError(t, err) + require.Len(t, fixture.requests, 1) + require.Equal(t, tc.want, fixture.requests[0].AccountScope) + }) + } +} + +func TestSPAccountScopeRejectsUnknown(t *testing.T) { + t.Parallel() + fixture := &spSDKFixture{respond: func(spSDKRequest) (map[string]any, error) { return spSDKPage(""), nil }} + params := spSDKParams(common.ServiceSavingsPlansCompute) + params.SPAccountScope = "org" + _, err := spSDKClient(fixture).GetRecommendations(context.Background(), params) + require.ErrorContains(t, err, "invalid Savings Plans account scope") + require.Empty(t, fixture.requests) +} From 5fb81bac66ea771bab9f682940a484ebed2df5ae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 19:58:31 +0200 Subject: [PATCH 2/2] refactor: land only the pkg field in this PR; provider change moves to a follow-up providers/aws builds against the pinned pkg module, so a new pkg field cannot be used in the same PR. Step 1 of 2 for #356: pkg first, then the provider change with a pkg pin bump. Refs #356 Co-Authored-By: Claude Opus 5.5 --- .../aws/recommendations/collection_sp.go | 20 +--------- .../parser_sp_account_scope_test.go | 38 ------------------- 2 files changed, 1 insertion(+), 57 deletions(-) delete mode 100644 providers/aws/recommendations/parser_sp_account_scope_test.go diff --git a/providers/aws/recommendations/collection_sp.go b/providers/aws/recommendations/collection_sp.go index 96ed4b23..faa904a2 100644 --- a/providers/aws/recommendations/collection_sp.go +++ b/providers/aws/recommendations/collection_sp.go @@ -3,7 +3,6 @@ package recommendations import ( "context" "fmt" - "strings" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/costexplorer" @@ -33,15 +32,11 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params *com if err != nil { return nil, fmt.Errorf("invalid lookback period for Savings Plans recommendation: %w", err) } - scope, err := convertSavingsPlansAccountScope(params.SPAccountScope) - if err != nil { - return nil, err - } results := make([]serviceResult, 0, len(planTypes)) for _, planType := range planTypes { input := &costexplorer.GetSavingsPlansPurchaseRecommendationInput{ SavingsPlansType: planType, PaymentOption: payment, TermInYears: term, - LookbackPeriodInDays: lookback, AccountScope: scope, + LookbackPeriodInDays: lookback, AccountScope: types.AccountScopeLinked, } recs, fetchErr := c.fetchSPAllPages(ctx, input, params, planType) if canceled := collectionCancellation(fetchErr); canceled != nil { @@ -58,19 +53,6 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params *com return mergeServiceResults(results...) } -// convertSavingsPlansAccountScope maps RecommendationParams.SPAccountScope to -// the Cost Explorer scope. Empty is linked, the long-standing default; an -// unknown value is an error rather than a silent fallback. -func convertSavingsPlansAccountScope(s string) (types.AccountScope, error) { - switch strings.ToLower(s) { - case "", "linked": - return types.AccountScopeLinked, nil - case "payer": - return types.AccountScopePayer, nil - } - return "", fmt.Errorf("invalid Savings Plans account scope %q: want linked or payer", s) -} - func (c *Client) fetchSPAllPages( ctx context.Context, input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, diff --git a/providers/aws/recommendations/parser_sp_account_scope_test.go b/providers/aws/recommendations/parser_sp_account_scope_test.go deleted file mode 100644 index fe072943..00000000 --- a/providers/aws/recommendations/parser_sp_account_scope_test.go +++ /dev/null @@ -1,38 +0,0 @@ -package recommendations - -import ( - "context" - "testing" - - "github.com/stretchr/testify/require" - - "github.com/LeanerCloud/cloud-commitments-go/pkg/common" -) - -func TestSPAccountScope(t *testing.T) { - t.Parallel() - for _, tc := range []struct{ in, want string }{ - {"", "LINKED"}, {"linked", "LINKED"}, {"payer", "PAYER"}, {"PAYER", "PAYER"}, - } { - t.Run(tc.in, func(t *testing.T) { - t.Parallel() - fixture := &spSDKFixture{respond: func(spSDKRequest) (map[string]any, error) { return spSDKPage(""), nil }} - params := spSDKParams(common.ServiceSavingsPlansCompute) - params.SPAccountScope = tc.in - _, err := spSDKClient(fixture).GetRecommendations(context.Background(), params) - require.NoError(t, err) - require.Len(t, fixture.requests, 1) - require.Equal(t, tc.want, fixture.requests[0].AccountScope) - }) - } -} - -func TestSPAccountScopeRejectsUnknown(t *testing.T) { - t.Parallel() - fixture := &spSDKFixture{respond: func(spSDKRequest) (map[string]any, error) { return spSDKPage(""), nil }} - params := spSDKParams(common.ServiceSavingsPlansCompute) - params.SPAccountScope = "org" - _, err := spSDKClient(fixture).GetRecommendations(context.Background(), params) - require.ErrorContains(t, err, "invalid Savings Plans account scope") - require.Empty(t, fixture.requests) -}