[cristim]

Refs #95

Moves the duplicated GCP helpers into shared internal packages, parameterising only the service name where needed.

Lifted:

• skumatch.SavingsPercent (was calculateSQLSavingsPercentage, calculateStorageSavingsPercentage, calculateSavingsPercentage) and skumatch.MatchesTier (was skuMatchesTier in cloudsql and memorystore).
• skulist.BillingService, the real Cloud Billing wrapper and skulist.Resolve(ctx, existing, name, opts...) (was BillingService, realBillingService and getOrCreateBillingService x3). Each client keeps type BillingService = skulist.BillingService, so its mocks are unchanged.
• skumatch.UnitPrice (was extractSQLPriceFromSKU in cloudsql and extractPriceFromSKU in memorystore).

All copies were identical, so there are no edge-case differences. A zero on-demand total still yields NaN/Inf in SavingsPercent; callers reject non-positive prices first.

Hardening (separate commit): the on-demand and commitment guards in all three clients rejected only == 0; they now reject <= 0, because a negative catalog price flips the savings sign. One test per service.

Still duplicated, so this does not close the issue:

• the per-client listSKUs catalog cache (cloudsql, memorystore, cloudstorage)
• the extract*PricingFromSKUs slot loops (cloudsql and memorystore nearly identical; cloudstorage differs)
• cloudstorage extractStoragePriceFromSKU, which also converts usage units, so it is not a copy of UnitPrice

Tests: new table tests for SavingsPercent, MatchesTier, UnitPrice, Resolve, and the negative-price guards. Existing tests changed only to call the moved helpers (skumatch.MatchesTier, skumatch.UnitPrice); assertions are unchanged. go test -race -count=1 ./... passes in providers/gcp with GOWORK=off and in the workspace; golangci-lint v2.10.1: 0 issues.

Mutation probes, each reverted: flipping the SavingsPercent subtraction, && to || in MatchesTier, sign and no-pricing branch in UnitPrice, the existing check and the error label in Resolve, and each of the six <= 0 guards all fail tests.

Note: the first commit message contains Closes #95; if this is merged without squashing, that would close the issue.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Cloud SQL, Cloud Storage, and Memorystore pricing lookups now reject zero or negative on-demand and commitment prices instead of returning invalid pricing results.
  • SKU pricing now consistently accounts for paid rates, free tiers, tier names, and applicable regions across these services.
  • Commitment savings percentages are calculated consistently using the on-demand total for the commitment term.
• Improved Error Handling
  • Billing lookup errors now include the owning service and billing service details.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: refactor/95-gcp-shared-helpers
• File: .coderabbit.yaml
• Commit: b80010f
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 438cbde4-1eb8-402d-9ff1-58c2dadeecb2

Commits

Reviewing files that changed from the base of the PR and between f744566 and b80010f.

Files selected for processing: 17

• providers/gcp/internal/skulist/billing.go
• providers/gcp/internal/skulist/billing_test.go
• providers/gcp/internal/skumatch/paid_rate_test.go
• providers/gcp/internal/skumatch/savings.go
• providers/gcp/internal/skumatch/savings_test.go
• providers/gcp/internal/skumatch/skumatch.go
• providers/gcp/services/cloudsql/client.go
• providers/gcp/services/cloudsql/client_test.go
• providers/gcp/services/cloudsql/negative_price_test.go
• providers/gcp/services/cloudsql/tiered_rates_test.go
• providers/gcp/services/cloudstorage/client.go
• providers/gcp/services/cloudstorage/negative_price_test.go
• providers/gcp/services/memorystore/client.go
• providers/gcp/services/memorystore/client_coverage_test.go
• providers/gcp/services/memorystore/client_test.go
• providers/gcp/services/memorystore/negative_price_test.go
• providers/gcp/services/memorystore/tiered_rates_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 change adds shared GCP billing resolution and SKU pricing helpers. Cloud SQL, Cloud Storage, and Memorystore use these helpers and reject negative on-demand or commitment prices.

Changes: GCP SKU Pricing

Layer / File(s)	Summary
Shared billing resolution providers/gcp/internal/skulist/*	Adds a shared billing-service interface and resolver. The real service lists SKU pages and wraps client-creation and listing errors.
Shared SKU matching and pricing helpers providers/gcp/internal/skumatch/*	Adds shared tier-and-region matching, unit-price extraction, and savings calculation, with tests for their behavior.
Cloud SQL pricing integration providers/gcp/services/cloudsql/*	Uses shared billing and SKU helpers, rejects non-positive prices, and adds negative-price tests.
Cloud Storage pricing integration providers/gcp/services/cloudstorage/*	Uses shared billing resolution and savings calculation, rejects non-positive prices, and adds negative-price tests.
Memorystore pricing integration providers/gcp/services/memorystore/*	Uses shared billing and SKU helpers, rejects non-positive prices, and updates related pricing tests.

Priority: Low

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

Change: Refactor

Possibly related PRs

• LeanerCloud/cloud-commitments-go#632: Names billing service IDs used by Cloud SQL and Cloud Storage SKU listing.
• LeanerCloud/cloud-commitments-go#471: Adds shared SKU matching and price aggregation code used by this change.
• LeanerCloud/cloud-commitments-go#628: Adds a commitment-price API used by the pricing extractors.

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to b8001

No actionable pricing regression is identified; the change appears mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 43.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 17 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title is concise and accurately describes the main refactoring goal: sharing savings and tier-matching helpers across GCP services.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

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]

Review verdicts at 9575b16: two independent cold reviews (non-author), both APPROVE, after one fix round.

• Each removed helper matches its shared replacement line for line: skumatch.SavingsPercent, MatchesTier and UnitPrice, and skulist.Resolve with its wrapper. Service names and error wording are unchanged, and argument order is preserved at all six call sites.
• The only exported change in the three service packages is BillingService becoming a type alias. A mock in a separate module still compiles against all three aliases and their SetBillingService methods.
• All six price checks reject <= 0, with a negative-price test per service.
• Mutation probes each fail tests: reverting a guard to == 0, swapping commitment and on-demand arguments, swapping the MatchesTier arguments, dropping the region check, wrong nanos scale, a flipped savings sign, and the wrapper's name and service ID swapped. go test -race -count=1 ./... passes in providers/gcp, with and without the workspace. Lint is clean.
• Notes, not blocking: the service-name label in the billing wrapper is only used in error text, and no test covers it (as on main). A negative on-demand price reports "no pricing found".

Refs #95: the issue stays open for the remaining duplication listed in the PR body.

[claude-cc-libs]