feat(azure/savingsplans): implement Savings Plan recommendations from benefitRecommendations

[cristim]

Closes #371

Implements savingsplans.Client.GetRecommendations with Cost Management Benefit Recommendations - List, api-version 2024-08-01 (subscription scope, properties/scope eq 'Shared'). Design and mapping are in #371. Honours params.Term and params.LookbackPeriod; no new params.

Mapping: hourly = commitmentAmount; RecurringMonthlyCost = hourly x 730; CommitmentCost 0; PaymentOption monthly; savings and on-demand cost scaled from totalHours to 730 h; term P1Y/P3Y. Unknown kind/sku/scope/term/granularity/currency, or a missing number, returns an error; nextLink is followed only on https management.azure.com.

Evidence (fixture-based; live probe az rest on the test subscription returned HTTP 200 {"value": []}):

• Fail before (stub restored in client.go, new tests kept): TestGetRecommendations_MapsRow, _EmptyTermRequestsBoth, _Pagination fail with "[]" should have N item(s), but has 0; _RejectsForeignNextLink and _SelfReferentialNextLink fail with An error is expected but got nil.
• Pass after: go test ./services/savingsplans/ ./ ok; GOWORK=off go build ./... ok; pre-commit hooks (vet, gocyclo, gosec) pass. golangci-lint panics in this environment (tool crash, not a finding).

Not in this PR: the one-line guard flag in providers/azure/recommendations.go (owned by azure-advisor-consumption). It is a separate local commit (lines ~188-192 and ~230-237), to be sequenced after their PR.

Deferred: PlanType is "Compute" (matches GetValidResourceTypes), but PurchaseCommitment sends it as sku.name, where the API expects Compute_Savings_Plan. Advisor's SavingsPlan rows may duplicate these.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added Azure Savings Plan benefit recommendations, including 7-, 30-, and 60-day lookbacks and one- or three-year terms.
  • Recommendations now include estimated savings and costs, with results retrieved across multiple pages.
  • Unsupported parameters and invalid or incomplete recommendation data return errors.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 4 billable files and costs up to $1.00.}

• 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 29 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-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: d162b390-e9ed-48a9-a18a-13a2f877a8b5

📥 Commits

Reviewing files that changed from the base of the PR and between 1ccf620 and 72145a4.

📒 Files selected for processing: 4

• providers/azure/services/savingsplans/benefit_recommendations.go
• providers/azure/services/savingsplans/benefit_recommendations_test.go
• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_test.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: 5c135f18-36ac-45e4-87ed-b14db7a818f0

📥 Commits

Reviewing files that changed from the base of the PR and between e9a33bb and 1ccf620.

📒 Files selected for processing: 4

• providers/azure/services/savingsplans/benefit_recommendations.go
• providers/azure/services/savingsplans/benefit_recommendations_test.go
• providers/azure/services/savingsplans/client.go
• providers/azure/services/savingsplans/client_test.go

💤 Files with no reviewable changes: 1

• providers/azure/services/savingsplans/client_test.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 Azure Savings Plan client now retrieves benefit recommendations through Azure Cost Management, validates and converts supported response rows, and returns errors for invalid parameters or responses.

Changes: Azure Savings Plan recommendations

Layer / File(s)	Summary
Recommendation retrieval and client integration providers/azure/services/savingsplans/client.go, providers/azure/services/savingsplans/benefit_recommendations.go, providers/azure/services/savingsplans/benefit_recommendations_test.go, providers/azure/services/savingsplans/client_test.go	GetRecommendations delegates to a new API flow. It supports 7-, 30-, and 60-day lookbacks and one- or three-year terms. The flow authenticates, paginates with HTTPS and host checks, validates response rows, and converts supported rows. Tests cover mapping, request parameters, pagination, and error cases. The test for the previous empty-success stub was removed.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant GetRecommendations
  participant ARM credential
  participant Benefit Recommendations API
  Client->>GetRecommendations: request recommendations
  GetRecommendations->>ARM credential: acquire ARM token
  GetRecommendations->>Benefit Recommendations API: request recommendation pages
  Benefit Recommendations API-->>GetRecommendations: return recommendation rows
  GetRecommendations-->>Client: return converted recommendations or error

Loading

Merge Risk: ⚪ Minimal · up to 1ccf6

Azure Savings Plan recommendations now come from the Cost Management Benefit Recommendations API, with input validation and bounded, host-restricted pagination. The provider wiring is still deferred, so errors from this client do not interrupt existing Azure recommendation runs. No merge-blocking issue was identified.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #371 requires pagination with both a page cap and loop protection. The implementation enforces benefitRecsMaxPages, but fetchBenefitPages does not track previously visited nextLink values.…	Add a visited-URL set in fetchBenefitPages. Return an error when a nextLink repeats before issuing another request. Add a test that a self-referential link fails after one request.
Docstring Coverage		Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: implementing Azure Savings Plan recommendations from the benefitRecommendations API.
Out of Scope Changes check		The changed files implement the GetRecommendations client, benefit-recommendation conversion, and fixture-based tests for issue #371. The client delegation is directly related. The separate `providers…

Full details: Linked Issues check

Explanation

Issue #371 requires pagination with both a page cap and loop protection. The implementation enforces benefitRecsMaxPages, but fetchBenefitPages does not track previously visited nextLink values. A self-referential link therefore causes 50 requests before failure. The subscription scope, Shared filter, term and lookback handling, row mapping, validation, and ARM-host restriction are implemented. The added tests verify the page cap, not early loop detection.

✨ 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]

Independent review (claude-opus-5-5) of 1ccf620: APPROVE

Re-review after my block of 05ff804 (lint failure). Review only.

Source of truth: Benefit Recommendations - List, api-version 2024-08-01 (learn.microsoft.com/rest/api/cost-management/benefit-recommendations/list?view=rest-cost-management-2024-08-01).

• URI Parameters, $filter: properties/scope (Single, Shared; default Shared), properties/lookBackPeriod (Last7Days, Last30Days, Last60Days; default Last60Days), properties/term (P1Y, P3Y; default P3Y). Matches the code.
• allSavingsBenefitDetails: commitmentAmount is "the commitment amount at the commitmentGranularity" (only Hourly supported); benefitCost = commitmentAmount x totalHours; savingsAmount is "for the 'totalHours' in the look-back period". sharedScopeBenefitRecommendationProperties: costWithoutBenefit "corresponds to 'totalHours'". So hourly = commitmentAmount and scaling savings/on-demand by month-hours / totalHours is correct.

Claims verified at this head:

• "honoured" changed to "honored" (client.go:128); "Lint Code (providers/azure)" is now green.
• hoursPerMonth = recommendations.DaysPerMonth * 24 (730.5), shared with the RI converters.
• Lookback is always sent; empty maps to Last7Days, matching the RI path's explicit consumptionLookBack.
• TestGetRecommendations_RejectsHTTPNextLink added. The loop guard is gone, and TestGetRecommendations_PageCapIsAnError asserts exactly 50 requests.
• benefitTerms uses reservations.ParseTermYears, which errors on unknown values.

Evidence (local, fresh clone, macOS):

• go vet ./... and go test ./... in providers/azure pass. GOWORK=off go build ./... passes for pkg, providers/aws, providers/azure and providers/gcp.
• Mutations: drop the https scheme check, and RejectsHTTPNextLink fails (it survived before). Map commitmentAmount as monthly, and MapsRow fails. Omit the default lookback, and EmptyTermRequestsBoth fails. Remove the page cap, and the package fails: the walk never ends and hits the test timeout.
• Live read-only probe (subscription 24d185cc-...): the exact encoded default filter (Shared, P3Y, Last7Days) returns 200 {"value":[]}. Earlier negative controls (P9Y, an unknown property) return 400, so the filter is really parsed. Mapping evidence is fixture-based, because this subscription has no rows.
• nextLink host check: 13 crafted URLs refused (http, :443/:8443 ports, trailing dot, suffix host, userinfo-before-host, %2e, backslash, IPv6). https://x@management.azure.com passes but still targets the ARM host.
• CI: every check on this head is green, including Lint Code (providers/azure), Unit Tests, Integration Tests, gosec, Trivy, Snyk and CodeRabbit.

Non-blocking:

1. MapsRow asserts against hoursPerMonth itself, so changing the constant (for example to 730) passes. Asserting against a literal 730.5 would pin it.
2. ParseTermYears does not accept "p1y"/"p3y", which the previous benefitTerms accepted. Callers send "1yr"/"3yr", so there is no current impact.
3. io.ReadAll is unbounded. This is consistent with the rest of providers/azure.
4. A single row with commitmentAmount <= 0 fails the whole call. It is unverified whether Azure ever emits such rows.

Known and out of scope: Advisor SP duplicates (handled in #378), the purchase path's PlanType "Compute" versus Compute_Savings_Plan, and the held-back recommendations.go wiring commit (its stub comments at :188-192 and :229-235 are now stale).

[cristim]

Independent review (claude-opus-5-5) of 72145a4: APPROVE
Update-branch of the approved 1ccf620: only main's commits came in, as clean merges with no conflict resolution; the PR's own diff content is unchanged; GOWORK=off vet and tests pass in: providers/azure.