refactor(azure): consolidate AzureRetailPrice into shared pricing package (closes #594) by cristim · Pull Request #843 · LeanerCloud/cloud-commitments-cli

cristim · 2026-05-28T19:56:02Z

Summary

Add pricing.RetailPriceItem to providers/azure/internal/pricing/types.go as the canonical union of all per-service retail price item fields
Replace seven duplicate AzureRetailPrice struct definitions (compute, cache, database, cosmosdb, search, savingsplans, managedredis) with type aliases pointing to pricing.Page[pricing.RetailPriceItem]
Migrate the three manual pagination loops in search, savingsplans, and managedredis to use pricing.FetchAll, aligning them with the other four services

Test plan

go build ./... passes
go test github.com/LeanerCloud/CUDly/providers/azure/... passes (660 tests across 14 packages)
No per-service item struct definitions remain in any service client

Summary by CodeRabbit

Refactor
- Consolidated internal pricing data structures across Azure services (Cache, Compute, Cosmos DB, Database, Managed Redis, Savings Plans, and Search) to improve code maintainability and consistency. This standardization streamlines how pricing information is handled uniformly across all Azure service clients.

coderabbitai · 2026-05-28T19:56:08Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9804d715-c339-4eca-a6fd-be8b532ff0b5

📥 Commits

Reviewing files that changed from the base of the PR and between dd7a023 and 72195d9.

📒 Files selected for processing (12)

providers/azure/internal/pricing/retail_prices.go
providers/azure/internal/pricing/types.go
providers/azure/services/cache/client.go
providers/azure/services/cache/client_test.go
providers/azure/services/compute/client.go
providers/azure/services/cosmosdb/client.go
providers/azure/services/database/client.go
providers/azure/services/database/client_test.go
providers/azure/services/managedredis/client.go
providers/azure/services/savingsplans/client.go
providers/azure/services/search/client.go
providers/azure/services/search/client_test.go

📝 Walkthrough

Walkthrough

This PR consolidates Azure Retail Prices API type definitions across six service clients by extracting duplicated struct definitions into a shared canonical type in the pricing package, then replacing each service's local types with type aliases. The search service additionally refactors its pricing fetch logic to use the shared pagination helper instead of manual HTTP/JSON handling.

Changes

Azure Retail Pricing Type Consolidation

Layer / File(s)	Summary
Canonical pricing types definition `providers/azure/internal/pricing/types.go`	New exported `RetailPriceItem` struct with JSON-tagged fields (currency, pricing amounts, region, product identifiers, SKU, terms) serves as the canonical Azure Retail Prices API item shape for all services.
Simple type alias migrations `providers/azure/services/cache/client.go`, `providers/azure/services/compute/client.go`, `providers/azure/services/cosmosdb/client.go`, `providers/azure/services/database/client.go`	Cache, compute, cosmosdb, and database services replace locally defined pricing item and envelope structs with type aliases to `pricing.RetailPriceItem` and `pricing.Page[pricing.RetailPriceItem]`.
Managed Redis migration `providers/azure/services/managedredis/client.go`	Adds `AzureRetailPrice` type alias and updates `getRedisPricing` and `parsePriceItems` to consume `pricing.RetailPriceItem` directly, removing the local `redisPriceItem` struct.
Savings Plans migration `providers/azure/services/savingsplans/client.go`	Introduces `AzureRetailPrice` type alias and updates `fetchOnDemandRate` to fetch pages using `pricing.FetchAll` with `pricing.RetailPriceItem`.
Search service refactor `providers/azure/services/search/client.go`, `providers/azure/services/search/client_test.go`	Imports shared pricing helper, converts `AzureRetailPrice` to type alias, refactors `fetchAzurePricing` from manual HTTP/JSON decoding to helper-based pagination via `pricing.FetchAll`, updates `extractSearchPricing` to accept `[]pricing.RetailPriceItem`, and aligns test fixture to use shared type.

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

priority/p2, severity/medium, urgency/this-quarter, impact/many, effort/l, type/feat

Poem

🐰 Seven services spoke in tongues untrue,
Each pricing struct a copy through and through!
Now one shared shape brings harmony to all,
While search hops forth to answer the call,
No more duplication—just aliases clean and bright! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main change: consolidating duplicate AzureRetailPrice types across seven Azure services into a shared pricing package using type aliases.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/594-wave13

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

cristim · 2026-05-28T19:56:11Z

@coderabbitai review

coderabbitai · 2026-05-28T19:56:17Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T10:22:03Z

@coderabbitai full review

coderabbitai · 2026-05-30T10:22:08Z

✅ Actions performed

Full review triggered.

cristim · 2026-06-04T14:03:41Z

@coderabbitai review

coderabbitai · 2026-06-04T14:03:50Z

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 45 seconds before sending another message.

cristim · 2026-06-08T01:35:44Z

@coderabbitai full review

Generated by Claude Code

coderabbitai · 2026-06-08T01:37:36Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details
gh pr view 843 --repo LeanerCloud/CUDly

echo "=== PR FILES ==="
gh pr diff 843 --repo LeanerCloud/CUDly

Length of output: 25104

🏁 Script executed:

#!/bin/bash
echo "=== pricing package files ==="
find providers/azure/internal/pricing -type f | sort
echo ""

echo "=== pricing package contents ==="
find providers/azure/internal/pricing -type f -name "*.go" | sort | while read f; do
  echo "--- $f ---"
  cat "$f"
  echo ""
done

echo "=== savingsplans credential usage ==="
grep -n "cred\|credential\|Bearer\|GetToken\|policy\." providers/azure/services/savingsplans/client.go

echo "=== managedredis parsePriceItems termStr ==="
grep -n "termStr\|termYears\|azureTermString\|Years\|Term" providers/azure/services/managedredis/client.go

echo "=== search encoding/json usage ==="
grep -n "json\." providers/azure/services/search/client.go | head -20

echo "=== search fetchAzurePricing callers ==="
grep -n "fetchAzurePricing\|\.Items" providers/azure/services/search/client.go | head -20

echo "=== pricing.FetchAll signature ==="
grep -rn "func FetchAll" providers/azure/internal/pricing/

echo "=== pricing.Page definition ==="
grep -rn "type Page\|Page\[" providers/azure/internal/pricing/

echo "=== DefaultPageTimeout DefaultMaxPages ==="
grep -rn "DefaultPageTimeout\|DefaultMaxPages" providers/azure/internal/pricing/

echo "=== azureTermString usage ==="
grep -rn "azureTermString" providers/azure/

Length of output: 19008

@cristim: Clean, well-structured refactor. The FetchAll implementation is solid — per-page timeout, max-pages cap, seen-URL guard, and all defensive paths are covered by the existing test suite. A few findings below.

Medium — `managedredis/client.go` Line 489: singular/plural term mismatch carried forward

termStr := fmt.Sprintf("%d Years", termYears)   // produces "1 Years", never matches "1 Year"

This pre-existing bug is not introduced by this PR, but parsePriceItems is refactored here (new signature: []pricing.RetailPriceItem) so the bug is worth flagging again. Per the long-term memory note, the fix (azureTermString helper) lives in PR #1045 which hasn't landed on this branch yet — rg azureTermString returns nothing in the working tree. If #1045 is going to merge after this one, the conflict will surface; if before, this line should be updated here too.

Minor — `retail_prices.go` package comment is stale

The package-level comment on retail_prices.go reads:

"used by every service client (compute, database, cache, cosmosdb)"

After this PR, search, savingsplans, and managedredis are also consumers. Suggested update:

// Package pricing provides a shared NextPageLink-driven walker for the
// Azure Retail Prices API, used by every Azure service client
// (compute, database, cache, cosmosdb, search, savingsplans, managedredis).

Nit — `search.fetchAzurePricing`: `Count` field is always 0 in the returned value

return &AzureRetailPrice{Items: items}, nil   // Count: 0, NextPageLink: ""

FetchAll returns a flat slice, so the wrapper Page struct has Count=0. Callers only access .Items (lines 431, 435), so this is functionally harmless, but TestAzureRetailPriceStructure explicitly sets Count: 2 which may give a false impression that Count is populated at runtime. A comment on fetchAzurePricing noting that Count is not populated would help future readers.

Positive notes

The savingsplans Bearer-token removal in fetchOnDemandRate is correct — prices.azure.com/api/retail/prices is a public API that doesn't require auth.
Type aliases (CacheRetailPriceItem = pricing.RetailPriceItem, etc.) are the right tool here: zero diff for callers, zero risk of JSON tag drift.
FetchAll is notably better than the three ad-hoc loops it replaces: the original managedredis loop had no DefaultMaxPages cap and no seen-URL guard.
The test plan looks complete in scope; the checkboxes just need to be ticked before merge.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:34:23.754Z
Learning: In LeanerCloud/CUDly Azure pricing extractors (compute, cache, database, cosmosdb, search, managedredis), Azure's Retail Prices API returns "1 Year" (singular) for 1-year reservation terms and "3 Years" (plural) for 3-year terms. All 6 pricing extractors use `azureTermString(termYears int) string` helper (added in PR `#1045` commit 5e938d9) to produce the correct singular/plural form for term matching. Prior to this fix, "%d Years" format produced "1 Years" which never matched Azure API responses, causing all 1-year reservation pricing lookups to silently fail. Regression tests: `TestExtractVMPricing_SingularOneYear`, `TestExtractVMPricing_PluralThreeYears`, `TestExtractRedisPricing_SingularOneYear`, `TestExtractRedisPricing_PluralThreeYears`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-04T14:28:19.903Z
Learning: In providers/azure/recommendations.go (Go, Azure provider), the service constructors (compute.NewClient, cache.NewClient, etc.) build concrete Azure SDK types with no interface injection seam. Introducing a concurrency timing test that asserts elapsed ≈ max-single-service latency would require adding package-level function variables — a refactor that is out of scope for small performance PRs.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-25T21:45:57.471Z
Learning: In this repository (CUDly), Azure service clients alias payment options in pairs: `{all-upfront, upfront}` and `{monthly, no-upfront}`. All 7 Azure service-client switches (compute, cache, cosmosdb, database, search, synapse, managedredis client.go) use these aliases. The canonical Azure payment option set in `ValidPaymentOptionsByProvider` is `{upfront, monthly}` (with synonyms `all-upfront → upfront`, `no-upfront → monthly`, `partial-upfront → upfront`).

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` no longer hardcodes `Term = "1yr"` (fixed in PR `#1047` commit 8f9a787, H-3 audit finding). It propagates `params.Term` with a `"1yr"` default, validates via `termPlan`, and returns nil on unknown term. This is stricter than memorystore/cloudstorage/cloudsql (which default and continue): computeengine returns nil on unknown term because it is on the purchase path. Regression tests: `TestConvertGCPRecommendation_PropagatesParamsTerm`, `TestConvertGCPRecommendation_RejectsUnknownTerm`.

cristim · 2026-06-08T04:53:29Z

@coderabbitai full review

coderabbitai · 2026-06-08T04:53:33Z

✅ Action performed

Full review finished.

cristim · 2026-06-08T07:02:20Z

@coderabbitai full review

coderabbitai · 2026-06-08T07:02:26Z

✅ Action performed

Full review finished.

cristim · 2026-06-19T21:32:51Z

@coderabbitai review

coderabbitai · 2026-06-19T21:32:57Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-07-09T21:21:32Z

@coderabbitai review

coderabbitai · 2026-07-09T21:21:38Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-07-10T15:08:05Z

@coderabbitai review

coderabbitai · 2026-07-10T15:08:11Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…kage (closes #594) Add pricing.RetailPriceItem (union of all per-service item fields) to providers/azure/internal/pricing/types.go and alias AzureRetailPrice to pricing.Page[pricing.RetailPriceItem] in all six service clients (compute, cache, database, cosmosdb, search, savingsplans) plus managedredis. Replace the three manual pagination loops in search, savingsplans, and managedredis with pricing.FetchAll calls. Remove the seven per-service struct definitions and their duplicated field lists.

…dalignment - Remove CacheRetailPriceItem and DatabaseRetailPriceItem type aliases that triggered revive var-naming; replace all call sites with pricing.RetailPriceItem directly and add azpricing import alias to test files that shadow "pricing" as a local variable name - Fix em-dash and double-space in RetailPriceItem doc comment (types.go) - Reorder struct fields for optimal alignment per betteralign (RetailPriceItem: 184->168 bytes, Page[T]: 32->24 pointer bytes)

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible labels May 28, 2026

cristim force-pushed the fix/594-wave13 branch from 9f4d0d9 to dd7a023 Compare June 8, 2026 04:18

cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:44

cristim force-pushed the fix/594-wave13 branch from dd7a023 to 3b525b3 Compare June 19, 2026 21:32

cristim force-pushed the fix/594-wave13 branch from 3b525b3 to 72195d9 Compare July 10, 2026 15:07

cristim added 2 commits July 17, 2026 21:11

cristim force-pushed the fix/594-wave13 branch from 72195d9 to d2689f3 Compare July 17, 2026 18:11

cristim merged commit f6f1377 into main Jul 19, 2026
19 checks passed

cristim deleted the fix/594-wave13 branch July 27, 2026 11:09

Conversation

cristim commented May 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Suggested labels

Poem

Uh oh!

cristim commented May 28, 2026

Uh oh!

coderabbitai Bot commented May 28, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Rate Limit Exceeded

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026

Medium — managedredis/client.go Line 489: singular/plural term mismatch carried forward

Minor — retail_prices.go package comment is stale

Nit — search.fetchAzurePricing: Count field is always 0 in the returned value

Positive notes

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

coderabbitai Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 9, 2026

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented May 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

Medium — `managedredis/client.go` Line 489: singular/plural term mismatch carried forward

Minor — `retail_prices.go` package comment is stale

Nit — `search.fetchAzurePricing`: `Count` field is always 0 in the returned value

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading