[cristim]

Summary

Follow-up to #646, covering the non-blocking review findings there:

• Each service's GetOfferingDetails test now asserts OfferingID with its prefix (gcp-cloudsql-, gcp-memorystore-, gcp-storage-, gcp-compute-), plus EffectiveHourlyRate and TotalCost. Before this, swapping the commitment total and hourly rate at a call site went undetected outside Cloud Storage, and so did changing the Cloud SQL or Compute Engine prefix.
• TestGetStoragePricing_CachesSKUsPerClient runs the Cloud Storage catalog cache through getStoragePricing and asserts one catalog read across three lookups (CodeRabbit nitpick on refactor(gcp): share term price scaling and cache the Cloud Storage catalog #646).
• Compute Engine ValidateOffering uses slices.Contains, like the other three clients. The error text is unchanged.

Tests

• Mutation probes, reverted: swapping the two float arguments at each of the four call sites, changing the Cloud SQL and Compute Engine prefixes, and listing without the cache each fail a test.
• providers/gcp: go test ./... passes with and without the workspace (GOWORK=off), and golangci-lint reports 0 issues.

Refs #95

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.50.}

• 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 1 minute for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 51 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-go
• Branch: test/gcp-offering-details-pins
• File: .coderabbit.yaml
• Commit: a78a1fe
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: d341732e-caee-47bd-b9bc-3dff0a13ff0f

Commits

Reviewing files that changed from the base of the PR and between eb546af and a78a1fe.

Files selected for processing: 6

• providers/gcp/services/cloudsql/client_test.go
• providers/gcp/services/cloudstorage/cache_test.go
• providers/gcp/services/cloudstorage/client_test.go
• providers/gcp/services/computeengine/client.go
• providers/gcp/services/computeengine/client_test.go
• providers/gcp/services/memorystore/offering_pins_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[cristim]

Cold review (claude-autospotting) at a78a1fe: APPROVE

Mutation probes. Each was applied alone and reverted, and every one fails a test:

• swapping the commitmentTotal and hourlyRate arguments at each of the 4 OfferingDetails call sites (cloudsql:180, memorystore:167, cloudstorage:163, computeengine:904);
• changing each OfferingID prefix (gcp-cloudsql-, gcp-memorystore-, gcp-storage-, gcp-compute-);
• making getStoragePricing call svc.ListSKUs directly, which fails TestGetStoragePricing_CachesSKUsPerClient;
• making computeengine ValidateOffering accept an invalid value, or reject a valid one.

No tautologies. Expected values are literal inputs and constants.

• Cloud Storage production divides by common.HoursPerMonth (730, cloudstorage/client.go:236) and scales by HoursPerYear * termYears (skumatch/savings.go:37-38).
• That gives hourly 0.0195/730 and a 1yr total of 0.0195*12. The test asserts exactly these values (cloudstorage/client_test.go:526-528).
• The cache test goes through getStoragePricing and c.skus.Get (client.go:197,258-260), which adds coverage beyond TestListSKUs_CachedPerClient.

a78a1fe. slices.Contains accepts the same set as the loop, and the error text invalid GCP machine type: %s is unchanged.

Scope. c09c488 changes only _test.go files. The commits are +60/-2 and +3/-4.

Races. Each new test builds its own client and mock, there are no package-level variables, and the touched packages pass under -race.

Build at a78a1fe (detached scratch worktree, under the heavy lock, -count=1):

• GOWORK=off go test ./...: ok
• workspace-mode go test ./...: ok
• golangci-lint: 0 issues

[cristim]

Cold review (claude-cc-libs reviewer agent) at a78a1fe: APPROVE

Findings: no actionable findings.

What I attacked and how (scratch worktree detached at a78a1fe, base main eb546af):

• Build/test: GOWORK=off go test ./... -count=1, workspace go test ./... -count=1 and golangci-lint run in providers/gcp all pass (0 lint issues). -race -count=1 on cloudsql, memorystore, cloudstorage and computeengine also passes.
• Mutation probes, each reverted right after its run (worktree left clean):
  • Swapping pricing.CommitmentPrice, pricing.HourlyRate at all four skumatch.OfferingDetails call sites (cloudsql/client.go:180, memorystore/client.go:167, cloudstorage/client.go:163, computeengine/client.go:904) is KILLED by the new rate and total pins in each service.
  • Changing each OfferingID prefix (cloudsql/client.go:179, memorystore/client.go:166, cloudstorage/client.go:162, computeengine/client.go:903) is KILLED by the new OfferingID equality asserts.
  • Making getStoragePricing call svc.ListSKUs(cloudStorageServiceID) directly instead of c.listSKUs (cloudstorage/client.go:197) is KILLED by TestGetStoragePricing_CachesSKUsPerClient (1 call expected, 3 seen). The existing TestListSKUs_CachedPerClient calls listSKUs directly, so it cannot catch that bypass. The new test is not redundant.
  • Extra probe: changing the GiBy.mo divisor at cloudstorage/client.go:236 from HoursPerMonth (730) to 720 is KILLED by the Cloud Storage rate and total pins.
• Expected values checked against production instead of being taken on trust. pkg/common/hours.go:5,9 gives HoursPerYear=8760 and HoursPerMonth=730. skumatch.ScaleToTerm (internal/skumatch/savings.go:30-44) returns HourlyRate as the commitment per-hour price and CommitmentPrice as hourly8760termYears. So Cloud Storage at 0.0195/GiB-month gives 0.0195/730 per hour and 0.0195/7308760 = 0.019512 for the term. Cloud SQL and Memorystore at 0.042/h give 0.0428760. For Compute Engine, 40.019915+15*0.002669 per hour matches the existing TotalCost pin divided by 8760. The fixtures (storageMockSkus GiBy.mo at 19500000 nanos, negPriceSkus 42000000 nanos) feed these numbers through the real extraction path. None of the expectations is computed by calling production code, so none is tautological.
• Production scope: the only non-test change is a78a1fe (computeengine/client.go:13,880-882). The slices.Contains form keeps the same semantics, and the error text "invalid GCP machine type: %s" at :884 is unchanged. ValidateOffering_Valid and ValidateOffering_Invalid (client_test.go:495,513) still cover both branches.
• Commit sizes: c09c488 is +60/-2 and a78a1fe is +3/-4, both well under 400.
• t.Parallel: both new tests build their own client and mock. countingBilling.calls is touched only from the test's serial loop, and the race detector is clean.

Nit, non-blocking: the Memorystore pins are in a new offering_pins_test.go instead of the existing GetOfferingDetails test. That is fine as is.