From 6e9f87deef555e38e18a27c3cca1e57947a6e8eb Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 01:17:21 +0100 Subject: [PATCH 1/4] fix(azure): count savings plans billed to another subscription in the duplicate check GetExistingCommitments keeps the #40/#207 rule that Account is the payer, so a plan billed to subscription B never appears in A's inventory, even when its applied scope covers A. DuplicateChecker only saw that inventory, so a recently bought plan billed to B and applied to A did not suppress A's matching recommendation, and the same commitment could be bought again while the benefit-recommendations API lagged. recfilter.DuplicateChecker now checks recommendations against GetCoveringCommitments when a client implements it, and falls back to GetExistingCommitments otherwise. The Azure savings plans client implements it as its inventory plus plans billed to another subscription that may apply to this one. Only a Single scope that resolves to a different subscription is left out. Shared, management-group, unknown and unresolvable scopes count as covering, because their containment cannot be established here. A covering plan without an ID fails the call. Account stays the payer, so ownership inventory is unchanged. Closes #466 Co-Authored-By: Claude Opus 5.5 --- pkg/recfilter/dedupe.go | 20 ++- pkg/recfilter/dedupe_test.go | 31 ++++ .../azure/services/savingsplans/client.go | 17 ++- .../azure/services/savingsplans/scope.go | 49 ++++++- .../azure/services/savingsplans/scope_test.go | 135 ++++++++++++++++++ 5 files changed, 244 insertions(+), 8 deletions(-) diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 0bd528ec..4d8e106d 100644 --- a/pkg/recfilter/dedupe.go +++ b/pkg/recfilter/dedupe.go @@ -17,6 +17,15 @@ type recentCommitmentFilter interface { FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) } +// coveringCommitmentLister is implemented by clients whose inventory +// (GetExistingCommitments, where Account is the payer) leaves out commitments +// billed elsewhere that still apply to the client's scope. The duplicate check +// uses this wider list instead, so a recent covering commitment suppresses a +// re-buy without being counted in a second owner's inventory. +type coveringCommitmentLister interface { + GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error) +} + // DuplicateChecker checks for existing commitments to avoid duplicates. type DuplicateChecker struct { LookbackHours int // How many hours to look back for recent purchases @@ -45,7 +54,7 @@ func NewDuplicateChecker(hours int) *DuplicateChecker { // Note: This is designed to prevent re-purchasing something you just bought, not to prevent // purchasing RIs in other accounts that happen to have the same characteristics. func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) { - existing, err := client.GetExistingCommitments(ctx) + existing, err := commitmentsForDedupe(ctx, client) if err != nil { return recs, nil, err } @@ -80,6 +89,15 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, return passed, filtered, nil } +// commitmentsForDedupe returns the commitments a recommendation is checked +// against: the covering list when the client provides one, else its inventory. +func commitmentsForDedupe(ctx context.Context, client provider.ServiceClient) ([]common.Commitment, error) { + if lister, ok := client.(coveringCommitmentLister); ok { + return lister.GetCoveringCommitments(ctx) + } + return client.GetExistingCommitments(ctx) +} + // filterRecentCommitments filters commitments to only recent purchases within the lookback window. func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment { now := time.Now diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go index f354b71e..20e8b0a7 100644 --- a/pkg/recfilter/dedupe_test.go +++ b/pkg/recfilter/dedupe_test.go @@ -629,3 +629,34 @@ func TestAdjustRecommendationsForExisting_PartialCoverageScalesMoney(t *testing. assert.InDelta(t, 40.0, *passed[0].RecurringMonthlyCost, 1e-9) assert.Equal(t, 100.0, monthly, "caller's pointer is not mutated") } + +type coveringClient struct { + fakeServiceClient + covering []common.Commitment + err error +} + +func (c *coveringClient) GetCoveringCommitments(context.Context) ([]common.Commitment, error) { + return c.covering, c.err +} + +// A commitment billed elsewhere is absent from the inventory but still covers +// the client; the check must suppress against the covering list (#466). +func TestAdjustRecommendationsForExisting_UsesCoveringCommitments(t *testing.T) { + t.Parallel() + recent := common.Commitment{ResourceType: "Compute", Count: 1, State: common.CommitmentStateActive, StartDate: time.Now().Add(-time.Hour)} + recs := []common.Recommendation{{ResourceType: "Compute", Count: 1}} + + client := &coveringClient{covering: []common.Commitment{recent}} + passed, filtered, err := NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client) + require.NoError(t, err) + assert.Empty(t, passed) + assert.Equal(t, recs, filtered) + + wantErr := errors.New("covering list unavailable") + client = &coveringClient{fakeServiceClient: fakeServiceClient{commitments: []common.Commitment{recent}}, err: wantErr} + passed, filtered, err = NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client) + require.ErrorIs(t, err, wantErr) + assert.Equal(t, recs, passed) + assert.Nil(t, filtered) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index a4235d0b..33e94c62 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -141,6 +141,13 @@ const maxSavingsPlanPages = 100 // that name this subscription (see planOwnedBy). Shared and management-group // plans billed to a billing account fail the call closed. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + return c.listCommitments(ctx, false) +} + +// listCommitments walks the savings plan inventory. covering also keeps plans +// billed to another subscription that may apply to this one (see +// GetCoveringCommitments). +func (c *Client) listCommitments(ctx context.Context, covering bool) ([]common.Commitment, error) { var pager SavingsPlanListAllPager if c.listAllPager != nil { @@ -167,7 +174,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen return nil, fmt.Errorf("failed to list savings plans: %w", err) } var skipped int - commitments, skipped, err = c.appendOwnedPlans(commitments, page.Value) + commitments, skipped, err = c.appendOwnedPlans(commitments, page.Value, covering) skippedSingle += skipped if err != nil { return nil, fmt.Errorf("incomplete savings plan inventory for subscription %s, page %d: %w", c.subscriptionID, pageIndex, err) @@ -181,7 +188,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen const subscriptionScopePrefix = "/subscriptions/" -func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel) ([]common.Commitment, int, error) { +func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel, covering bool) ([]common.Commitment, int, error) { skipped := 0 for i, sp := range plans { disposition, err := planOwnedBy(sp, c.subscriptionID) @@ -193,6 +200,12 @@ func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbe dst = append(dst, *convertSavingsPlan(sp, c.subscriptionID)) case planSkippedSingle: skipped++ + case planOtherSubscription: + if covering { + if dst, err = appendCoveringForeignPlan(dst, sp, c.subscriptionID); err != nil { + return nil, 0, fmt.Errorf("row %d: %w", i+1, err) + } + } } } return dst, skipped, nil diff --git a/providers/azure/services/savingsplans/scope.go b/providers/azure/services/savingsplans/scope.go index da0a7ea9..e58d231e 100644 --- a/providers/azure/services/savingsplans/scope.go +++ b/providers/azure/services/savingsplans/scope.go @@ -1,19 +1,23 @@ package savingsplans import ( + "context" "fmt" "strings" "unicode" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" ) type planDisposition int const ( - planNotOurs planDisposition = iota // billed to another subscription - planOwned // belongs in this subscription's inventory - planSkippedSingle // account-billed Single plan applied to another subscription + planNotOurs planDisposition = iota // unresolved; returned only with an error + planOwned // belongs in this subscription's inventory + planSkippedSingle // account-billed Single plan applied to another subscription + planOtherSubscription // billed to another subscription; may still apply to this one ) const billingAccountPrefix = "/providers/Microsoft.Billing/billingAccounts/" @@ -41,7 +45,7 @@ func subscriptionBilledPlan(sp *armbillingbenefits.SavingsPlanModel, billingScop return planNotOurs, fmt.Errorf("malformed billing subscription scope") } if !strings.EqualFold(billingScope, subscriptionScopePrefix+ownSub) { - return planNotOurs, nil + return planOtherSubscription, nil } return ownedWithID(sp) } @@ -53,9 +57,44 @@ func ownedWithID(sp *armbillingbenefits.SavingsPlanModel) (planDisposition, erro return planOwned, nil } +// GetCoveringCommitments returns GetExistingCommitments plus the plans billed +// to another subscription that may apply to this one, for the duplicate check +// only (#466). Account stays the payer, so such a plan is still owned only by +// the subscription that pays for it. +func (c *Client) GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error) { + return c.listCommitments(ctx, true) +} + +// appendCoveringForeignPlan appends a plan billed to another subscription +// unless it provably cannot cover ownSub. +func appendCoveringForeignPlan(dst []common.Commitment, sp *armbillingbenefits.SavingsPlanModel, ownSub string) ([]common.Commitment, error) { + if !foreignPlanMayCover(sp.Properties, ownSub) { + return dst, nil + } + if _, err := ownedWithID(sp); err != nil { + return nil, fmt.Errorf("covering savings plan has no ID") + } + payer := (*sp.Properties.BillingScopeID)[len(subscriptionScopePrefix):] + return append(dst, *convertSavingsPlan(sp, payer)), nil +} + +// foreignPlanMayCover reports whether a plan billed to another subscription +// may apply to ownSub. Only a Single scope that resolves to a different +// subscription rules it out. Shared and management-group scopes cover +// subscriptions this client cannot enumerate, so like unknown or unresolvable +// scopes they count as covering: skipping a purchase is recoverable, a +// duplicate commitment is not. +func foreignPlanMayCover(props *armbillingbenefits.SavingsPlanModelProperties, ownSub string) bool { + if props.AppliedScopeType == nil || *props.AppliedScopeType != armbillingbenefits.AppliedScopeTypeSingle { + return true + } + applied, err := singleScopeSubscription(props.AppliedScopeProperties) + return err != nil || strings.EqualFold(applied, ownSub) +} + // accountBilledPlan attributes a Single plan to the subscription it names, // because no subscription pays for it. Plans billed to another subscription -// but applied to this one are tracked in #466 and are not handled here. +// but applied to this one are handled by GetCoveringCommitments. func accountBilledPlan(sp *armbillingbenefits.SavingsPlanModel, ownSub string) (planDisposition, error) { if sp.Properties.AppliedScopeType == nil || *sp.Properties.AppliedScopeType != armbillingbenefits.AppliedScopeTypeSingle { return planNotOurs, fmt.Errorf("billing-account plan is not single-subscription scoped") diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go index dbbe35b1..7d95fb61 100644 --- a/providers/azure/services/savingsplans/scope_test.go +++ b/providers/azure/services/savingsplans/scope_test.go @@ -185,3 +185,138 @@ func TestGetExistingCommitments_SDKBillingAccountSingleScope(t *testing.T) { }) } } + +// subPlan is a plan billed to subscription sub-b with the given applied scope. +func subPlan(id string, typ *armbillingbenefits.AppliedScopeType, props *armbillingbenefits.AppliedScopeProperties) *armbillingbenefits.SavingsPlanModel { + sp := accountPlan("/subscriptions/sub-b", typ, props) + sp.ID = toPtr(id) + return sp +} + +func TestGetCoveringCommitments_ForeignSubscriptionPlans(t *testing.T) { + single := toPtr(armbillingbenefits.AppliedScopeTypeSingle) + own := accountPlan("/subscriptions/sub-a", toPtr(armbillingbenefits.AppliedScopeTypeShared), nil) + own.ID = toPtr("owned") + coveringNoID := subPlan("", single, scoped("/subscriptions/sub-a", "")) + coveringNoID.ID = nil + otherNoID := subPlan("", single, scoped("/subscriptions/sub-c", "")) + otherNoID.ID = nil + cases := []struct { + name string + row *armbillingbenefits.SavingsPlanModel + wantCover bool + wantErr bool + }{ + {"single-ours", subPlan("f", single, scoped("/subscriptions/sub-a", "")), true, false}, + {"single-ours-mixed-case", subPlan("f", single, scoped("/Subscriptions/SUB-A", "")), true, false}, + {"single-rg-ours", subPlan("f", single, scoped("", "/subscriptions/sub-a/resourceGroups/rg")), true, false}, + {"single-other", subPlan("f", single, scoped("/subscriptions/sub-c", "")), false, false}, + {"single-rg-other", subPlan("f", single, scoped("", "/subscriptions/sub-c/resourceGroups/rg")), false, false}, + {"single-unresolvable", subPlan("f", single, nil), true, false}, + {"single-disagreeing", subPlan("f", single, scoped("/subscriptions/sub-c", "/subscriptions/sub-a/resourceGroups/rg")), true, false}, + {"shared", subPlan("f", toPtr(armbillingbenefits.AppliedScopeTypeShared), nil), true, false}, + {"management-group", subPlan("f", toPtr(armbillingbenefits.AppliedScopeTypeManagementGroup), nil), true, false}, + {"nil-type", subPlan("f", nil, nil), true, false}, + {"unknown-type", subPlan("f", toPtr(armbillingbenefits.AppliedScopeType("Bogus")), scoped("/subscriptions/sub-c", "")), true, false}, + {"covering-no-id", coveringNoID, false, true}, + {"covering-blank-id", subPlan(" ", single, scoped("/subscriptions/sub-a", "")), false, true}, + {"other-no-id", otherNoID, false, false}, + {"account-shared", accountPlan(eaScope, toPtr(armbillingbenefits.AppliedScopeTypeShared), nil), false, true}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + c := NewClient(nil, "sub-a", "eastus") + c.SetListAllPager(&mockListAllPager{results: []*armbillingbenefits.SavingsPlanModel{own, tt.row}}) + got, err := c.GetCoveringCommitments(context.Background()) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, got) + return + } + require.NoError(t, err) + want := []common.Commitment{*convertSavingsPlan(own, "sub-a")} + if tt.wantCover { + want = append(want, *convertSavingsPlan(tt.row, "sub-b")) + } + assert.Equal(t, want, got) + + // Ownership is unchanged: the payer sub-b owns the plan, not sub-a. + c.SetListAllPager(&mockListAllPager{results: []*armbillingbenefits.SavingsPlanModel{own, tt.row}}) + owned, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + assert.Equal(t, want[:1], owned) + }) + } +} + +// coveringLister mirrors the optional interface recfilter.DuplicateChecker +// consults before GetExistingCommitments (pkg/recfilter/dedupe.go). +type coveringLister interface { + GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error) +} + +var _ coveringLister = (*Client)(nil) + +// coveringView hands DuplicateChecker the covering list as its inventory, so +// this test exercises the recfilter matching against the pinned pkg as well as +// the workspace one, which picks GetCoveringCommitments up on its own. +type coveringView struct{ *Client } + +func (v coveringView) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + return v.GetCoveringCommitments(ctx) +} + +// A plan billed to sub-b and bought an hour ago, decoded by the real SDK pager +// (#466): only sub-b owns it, but it suppresses sub-a's matching recommendation +// unless its Single scope names another subscription. +func TestDuplicateChecker_PlanBilledToAnotherSubscription(t *testing.T) { + row := func(applied, props string) string { + return fmt.Sprintf(`{"id":"p1","name":"Compute","properties":{"billingScopeId":"/subscriptions/sub-b","appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`, + applied, props, time.Now().UTC().Add(-time.Hour).Format(time.RFC3339)) + } + cases := []struct { + name string + row string + wantSuppress bool + }{ + {"single-ours", row("Single", `{"subscriptionId":"/subscriptions/sub-a"}`), true}, + {"single-rg-ours", row("Single", `{"resourceGroupId":"/subscriptions/sub-a/resourceGroups/rg"}`), true}, + {"shared", row("Shared", `{}`), true}, + {"management-group", row("ManagementGroup", `{"managementGroupId":"/providers/Microsoft.Management/managementGroups/mg"}`), true}, + {"single-other", row("Single", `{"subscriptionId":"/subscriptions/sub-c"}`), false}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, err := fmt.Fprintf(w, `{"value":[%s]}`, tt.row) + assert.NoError(t, err) + })) + t.Cleanup(server.Close) + sdk := newFixtureSavingsPlanClient(t, server) + client := func(sub string) *Client { + c := NewClient(nil, sub, "eastus") + c.SetListAllPager(sdk.NewListAllPager(nil)) + return c + } + ownedA, err := client("sub-a").GetExistingCommitments(context.Background()) + require.NoError(t, err) + assert.Empty(t, ownedA) + ownedB, err := client("sub-b").GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, ownedB, 1) + assert.Equal(t, "sub-b", ownedB[0].Account) + + recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1}} + passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, coveringView{client("sub-a")}) + require.NoError(t, err) + if tt.wantSuppress { + assert.Empty(t, passed) + assert.Equal(t, recs, filtered) + } else { + assert.Equal(t, recs, passed) + assert.Empty(t, filtered) + } + }) + } +} From c2c9989dc50646531f40229d2c37e761e85fcde2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 01:59:34 +0100 Subject: [PATCH 2/4] fix(azure): adapt covering savings plans to convertSavingsPlan's error return #494 made convertSavingsPlan return an error and require a SKU. Propagate the error for plans billed to another subscription too, and give the covering-path fixtures the Compute_Savings_Plan SKU. Co-Authored-By: Claude Opus 5.5 --- .../savingsplans/client_completeness_sdk_test.go | 2 +- providers/azure/services/savingsplans/scope.go | 6 +++++- providers/azure/services/savingsplans/scope_test.go | 13 ++++++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/providers/azure/services/savingsplans/client_completeness_sdk_test.go b/providers/azure/services/savingsplans/client_completeness_sdk_test.go index 6cd32095..c40b1dcd 100644 --- a/providers/azure/services/savingsplans/client_completeness_sdk_test.go +++ b/providers/azure/services/savingsplans/client_completeness_sdk_test.go @@ -52,7 +52,7 @@ func TestGetExistingCommitments_SDKPaginationCompleteness(t *testing.T) { if ambiguous { scope = "/providers/Microsoft.Billing/billingAccounts/ba-1" } - _, err := fmt.Fprintf(w, `{"value":[{"id":"second","properties":{"billingScopeId":%q,"appliedScopeType":"Shared"}}]}`, scope) + _, err := fmt.Fprintf(w, `{"value":[{"id":"second","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":%q,"appliedScopeType":"Shared"}}]}`, scope) assert.NoError(t, err) default: t.Errorf("unexpected page: %s", r.URL.RawQuery) diff --git a/providers/azure/services/savingsplans/scope.go b/providers/azure/services/savingsplans/scope.go index e58d231e..af945158 100644 --- a/providers/azure/services/savingsplans/scope.go +++ b/providers/azure/services/savingsplans/scope.go @@ -75,7 +75,11 @@ func appendCoveringForeignPlan(dst []common.Commitment, sp *armbillingbenefits.S return nil, fmt.Errorf("covering savings plan has no ID") } payer := (*sp.Properties.BillingScopeID)[len(subscriptionScopePrefix):] - return append(dst, *convertSavingsPlan(sp, payer)), nil + commitment, err := convertSavingsPlan(sp, payer) + if err != nil { + return nil, err + } + return append(dst, *commitment), nil } // foreignPlanMayCover reports whether a plan billed to another subscription diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go index 7bd7e613..c59e2195 100644 --- a/providers/azure/services/savingsplans/scope_test.go +++ b/providers/azure/services/savingsplans/scope_test.go @@ -234,9 +234,9 @@ func TestGetCoveringCommitments_ForeignSubscriptionPlans(t *testing.T) { return } require.NoError(t, err) - want := []common.Commitment{*convertSavingsPlan(own, "sub-a")} + want := []common.Commitment{mustConvertSavingsPlan(t, own, "sub-a")} if tt.wantCover { - want = append(want, *convertSavingsPlan(tt.row, "sub-b")) + want = append(want, mustConvertSavingsPlan(t, tt.row, "sub-b")) } assert.Equal(t, want, got) @@ -271,7 +271,7 @@ func (v coveringView) GetExistingCommitments(ctx context.Context) ([]common.Comm // unless its Single scope names another subscription. func TestDuplicateChecker_PlanBilledToAnotherSubscription(t *testing.T) { row := func(applied, props string) string { - return fmt.Sprintf(`{"id":"p1","name":"Compute","properties":{"billingScopeId":"/subscriptions/sub-b","appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`, + return fmt.Sprintf(`{"id":"p1","name":"Compute","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":"/subscriptions/sub-b","appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`, applied, props, time.Now().UTC().Add(-time.Hour).Format(time.RFC3339)) } cases := []struct { @@ -320,3 +320,10 @@ func TestDuplicateChecker_PlanBilledToAnotherSubscription(t *testing.T) { }) } } + +func mustConvertSavingsPlan(t *testing.T, sp *armbillingbenefits.SavingsPlanModel, sub string) common.Commitment { + t.Helper() + c, err := convertSavingsPlan(sp, sub) + require.NoError(t, err) + return *c +} From c8039e9d09141a114a3f90dfde64a22cbbe8c153 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 02:00:04 +0100 Subject: [PATCH 3/4] fix(azure): avoid shadowed err in appendOwnedPlans after the main merge Co-Authored-By: Claude Opus 5.5 --- providers/azure/services/savingsplans/client.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index e8b1e99c..374eb91b 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -197,9 +197,9 @@ func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbe } switch disposition { case planOwned: - commitment, err := convertSavingsPlan(sp, c.subscriptionID) - if err != nil { - return nil, 0, fmt.Errorf("row %d: %w", i+1, err) + commitment, convErr := convertSavingsPlan(sp, c.subscriptionID) + if convErr != nil { + return nil, 0, fmt.Errorf("row %d: %w", i+1, convErr) } dst = append(dst, *commitment) case planSkippedSingle: From 63a53275a189d95d39135aae61b1661750422ee2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 16:45:57 +0100 Subject: [PATCH 4/4] test(azure): date the covering-path pagination fixture for #498 #498 makes convertSavingsPlan reject a live plan without effectiveDateTime. The page-2 plan billed to another subscription is now in the covering list, so give it one. Co-Authored-By: Claude Opus 5.5 --- .../services/savingsplans/client_completeness_sdk_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/providers/azure/services/savingsplans/client_completeness_sdk_test.go b/providers/azure/services/savingsplans/client_completeness_sdk_test.go index b35202d8..06c2b32b 100644 --- a/providers/azure/services/savingsplans/client_completeness_sdk_test.go +++ b/providers/azure/services/savingsplans/client_completeness_sdk_test.go @@ -52,7 +52,8 @@ func TestGetExistingCommitments_SDKPaginationCompleteness(t *testing.T) { if ambiguous { scope = "/providers/Microsoft.Billing/billingAccounts/ba-1" } - _, err := fmt.Fprintf(w, `{"value":[{"id":"second","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":%q,"appliedScopeType":"Shared"}}]}`, scope) + _, err := fmt.Fprintf(w, `{"value":[{"id":"second","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":%q,"appliedScopeType":"Shared","effectiveDateTime":%q}}]}`, + scope, time.Now().UTC().Format(time.RFC3339)) assert.NoError(t, err) default: t.Errorf("unexpected page: %s", r.URL.RawQuery)