feat(gcp): populate RecurringMonthlyCost in GCP rec parsers (closes #264) - #811
Merged
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR extends GCP recommendation converters for Cloud SQL, Cloud Storage, Memorystore, and Compute Engine to populate ChangesGCP Recommendation RecurringMonthlyCost Population
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
Rate Limit Exceeded
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/gcp/services/cloudsql/client.go (1)
4-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun
gofmton changed Go files before merging.CI is currently failing on the
go-fmtpre-commit hook.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/gcp/services/cloudsql/client.go` around lines 4 - 19, CI is failing due to unformatted Go code in the changed file (the import block in providers/gcp/services/cloudsql/client.go); run gofmt (or goimports) on that file to fix formatting and import grouping, save the changes, and commit the formatted file so the go-fmt pre-commit hook and CI pass.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/gcp/services/cloudsql/client_test.go`:
- Around line 849-915: The test fails because getSQLPricing now requires a
commitment-price SKU but the MockBillingService fixture only provides an
on-demand tier; update the mock (MockBillingService.skus / the Sku in the test)
to include a commitment SKU entry (with appropriate
PricingInfo/PricingExpression/TieredRates representing the 1yr commitment hourly
or total price) so convertGCPRecommendation can find commitment pricing, and
change the expected value computation (used in the assertion comparing against
rec.RecurringMonthlyCost) to derive the monthly commitment from that commitment
SKU rate (or reuse estimateSQLCommitmentPrice logic) instead of relying on the
stale hardcoded 0.85 multiplier.
In `@providers/gcp/services/cloudsql/client.go`:
- Around line 553-568: convertGCPRecommendation is calling getSQLPricing twice:
once indirectly via fillSQLPricing(rec, termYearsFromLabel(rec.Term)) and again
directly when computing RecurringMonthlyCost, causing duplicate external SKU
calls and warnings for empty rec.ResourceType. Fix by removing the second
getSQLPricing call in convertGCPRecommendation and instead have fillSQLPricing
either return the pricing or populate RecurringMonthlyCost (using
termYearsFromLabel(rec.Term) and pricing.CommitmentPrice/float64(termYears*12));
then in convertGCPRecommendation rely on the value set by fillSQLPricing (or use
its returned pricing) to set rec.RecurringMonthlyCost and avoid any extra
getSQLPricing invocation for empty rec.ResourceType.
In `@providers/gcp/services/cloudstorage/client_test.go`:
- Around line 868-926: The test currently only provides an on-demand SKU so
convertGCPRecommendation/getStoragePricing falls into the non-fatal path and
leaves RecurringMonthlyCost nil; update the mock billing response (the
MockBillingService instance used by client.SetBillingService, e.g., mockBilling)
to include an additional commitment SKU with a PricingInfo that contains a
PricingExpression -> TieredRates -> UnitPrice (Money) and appropriate
ServiceRegions so that getStoragePricing can find commitment pricing; re-run the
test to assert RecurringMonthlyCost is non-nil and > 0 for the successful path.
- Around line 922-923: The tests call convertGCPRecommendation with only (ctx,
gcpRec) but the function signature requires a third parameter of type
common.RecommendationParams; update the test invocations (the calls to
convertGCPRecommendation in providers/gcp/services/cloudstorage/client_test.go)
to pass an appropriate common.RecommendationParams value (e.g., a
default/constructed params struct or the existing test params variable) so the
call matches convertGCPRecommendation(ctx, gcpRec, params) and recompile.
In `@providers/gcp/services/cloudstorage/client.go`:
- Around line 545-560: The code is making a redundant billing lookup:
c.fillStoragePricing already performs the pricing lookup, so remove the second
call to c.getStoragePricing and instead use the pricing data populated by
fillStoragePricing to set rec.RecurringMonthlyCost (and ensure you only attempt
to compute monthly when rec.ResourceType is non-empty and the pricing data
populated by fillStoragePricing exists); update the block around
c.fillStoragePricing / RecurringMonthlyCost to rely on the filled pricing rather
than calling getStoragePricing again (references: fillStoragePricing,
getStoragePricing, rec.RecurringMonthlyCost, rec.ResourceType).
In `@providers/gcp/services/computeengine/client_test.go`:
- Around line 903-906: The test is asserting RecurringMonthlyCost is non-nil and
zero for an "upfront" case but the call uses default (monthly) params; either
make the test actually request an upfront recommendation or change assertions to
match monthly-default semantics. Update the test in
providers/gcp/services/computeengine/client_test.go so that the request params
explicitly set PaymentOption = "upfront" when asserting rec.RecurringMonthlyCost
is non-nil and exactly 0, or alternatively (preferred if testing monthly
default) change the assertions to allow RecurringMonthlyCost to be nil (meaning
unknown) and not assert zero; locate references to rec.RecurringMonthlyCost and
the test helper that constructs the request parameters and modify accordingly.
In `@providers/gcp/services/computeengine/client.go`:
- Around line 948-953: The code unconditionally sets rec.RecurringMonthlyCost to
a pointer-to-zero which misrepresents monthly costs; change this so you only set
RecurringMonthlyCost = &zero when the recommendation truly represents an upfront
payment option (i.e., when rec.PaymentOption == "upfront" or the original
source/payment flag indicates upfront), otherwise leave RecurringMonthlyCost
nil; update the logic around RecurringMonthlyCost (and the related PaymentOption
handling) so the nil-vs-zero contract is preserved and only upfront CUDs get the
explicit $0 pointer.
In `@providers/gcp/services/memorystore/client_test.go`:
- Around line 827-828: Tests call convertGCPRecommendation with only (ctx,
gcpRec) but the function signature requires a third common.RecommendationParams
parameter; update the test calls to pass an appropriate RecommendationParams
(e.g., zero-value or constructed params relevant to the test) into
client.convertGCPRecommendation so calls become (ctx, gcpRec, params) where
params is a common.RecommendationParams instance set for the test; update both
occurrences (the call assigning rec and the other call at line ~854) to supply
this third argument.
In `@providers/gcp/services/memorystore/client.go`:
- Around line 536-551: fillRedisPricing is already performing the billing lookup
so avoid calling c.getRedisPricing again; change fillRedisPricing to return the
pricing (or a pointer/struct containing CommitmentPrice) and an error (e.g.,
func (c *Client) fillRedisPricing(ctx context.Context, rec *Record, termYears
int) (*Pricing, error)), update call sites to capture the returned pricing and
error, and use that returned pricing to compute RecurringMonthlyCost (using
termYearsFromLabel(rec.Term)) instead of calling c.getRedisPricing here; also
ensure you skip all pricing work when rec.ResourceType == "" so no duplicate
lookup or warnings occur.
---
Outside diff comments:
In `@providers/gcp/services/cloudsql/client.go`:
- Around line 4-19: CI is failing due to unformatted Go code in the changed file
(the import block in providers/gcp/services/cloudsql/client.go); run gofmt (or
goimports) on that file to fix formatting and import grouping, save the changes,
and commit the formatted file so the go-fmt pre-commit hook and CI pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c77e36a8-5091-484d-8d4a-b97586e84bed
📒 Files selected for processing (8)
providers/gcp/services/cloudsql/client.goproviders/gcp/services/cloudsql/client_test.goproviders/gcp/services/cloudstorage/client.goproviders/gcp/services/cloudstorage/client_test.goproviders/gcp/services/computeengine/client.goproviders/gcp/services/computeengine/client_test.goproviders/gcp/services/memorystore/client.goproviders/gcp/services/memorystore/client_test.go
cristim
added a commit
that referenced
this pull request
Jun 8, 2026
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
Member
Author
|
Addressed all 9 review threads in b84c7ce. Per-finding summary:
Build green ( @coderabbitai review |
Contributor
|
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 10, 2026
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
) Set RecurringMonthlyCost on all three GCP service recommendation parsers: - computeengine: PaymentOption "upfront" -> &0 (no monthly charge) - cloudsql: monthly -> CommitmentPrice/termMonths via billing SKU lookup - memorystore: monthly -> CommitmentPrice/termMonths via billing SKU lookup Billing lookup failure is non-fatal: log a warning and leave the field nil so the frontend renders "—" rather than a stale value. Adds 5 new tests (2 per monthly service + 1 on computeengine) asserting the field is non-nil and matches the expected pricing formula.
Extract extractGCPResourceType, extractGCPSavings, and termYearsFromLabel helpers from convertGCPRecommendation in both the memorystore and cloudsql packages. Complexity drops from 13 to 5, clearing the gocyclo pre-commit hook failure on CI.
… (refs #264) The three other GCP service clients (cloudsql, memorystore, computeengine) already populate RecurringMonthlyCost in convertGCPRecommendation via their respective pricing helpers. cloudstorage was the only remaining client that left the field nil, causing the frontend to render "--" for all Cloud Storage rows. The fix calls getStoragePricing (which already existed for GetOfferingDetails) and sets RecurringMonthlyCost = CommitmentPrice / (termYears * 12). A pricing lookup failure is non-fatal: log a warning and leave the field nil so the frontend renders "--" rather than a stale value -- matching the sibling pattern. Tests added: - success case: non-nil RecurringMonthlyCost > 0 when billing returns a SKU - billing-failure case: field stays nil when billing call errors out (must not coerce to 0)
…mendation Extract extractGCPResourceType, extractGCPSavings, and termYearsFromLabel helpers from convertGCPRecommendation in the cloudstorage package, mirroring the existing refactor in the cloudsql and memorystore packages. Complexity drops from 13 to under the limit, clearing the gocyclo pre-commit hook failure on CI. Behavior is unchanged.
…undant lookups Address CodeRabbit findings on PR #811: - cloudsql/cloudstorage/memorystore converters issued a second getSQLPricing/getStoragePricing/getRedisPricing call after fill*Pricing already performed the lookup, doubling SKU API calls and emitting warnings even for empty ResourceType. Reuse the CommitmentCost the fill* helpers populate and gate the whole block on a non-empty ResourceType. - computeengine unconditionally set RecurringMonthlyCost to a pointer-to-zero, mislabeling monthly-billed CUDs as a known-zero recurring fee and breaking the nil-vs-zero contract. Derive the monthly charge from CommitmentCost and leave the field nil when pricing is unavailable. Add termYearsFromTerm and reuse it in enrichRecWithPricing, which also makes 36mo terms map to 3 years. - Fix the success-path test fixtures: getSQLPricing/getRedisPricing now require a commitment SKU, so the on-demand-only fixtures drove the failure branch and left RecurringMonthlyCost nil. Add commitment SKUs and replace the stale hardcoded discount multipliers with the commitment-rate-derived expectation. - Fix compile errors in cloudstorage and memorystore tests that still called convertGCPRecommendation with two arguments; pass common.RecommendationParams. - Align the compute test assertion with the monthly-default path: empty params yield no computed monthly pricing, so RecurringMonthlyCost stays nil (unknown), not an explicit zero.
golangci-lint misspell (locale US) flagged "unrecognised" in the new termYearsFromLabel/termYearsFromTerm doc comments added by this branch. Use "unrecognized" so the PR introduces no new lint findings.
Use a raw string literal (backtick) for the assert message that contains square-bracket quotes around "gcp" to fix the "missing ',' in argument list" compile error in the pre-commit gofmt/go-vet pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RecurringMonthlyCost(*float64) on all three GCP recommendation parsers, making GCP rows consistent with AWS and Azure in the frontend's monthly-cost column (closes feat(gcp): populate RecurringMonthlyCost in GCP recommendation parsers #264).computeengine: PaymentOption is "upfront" (all-upfront commitment), so setsRecurringMonthlyCost = &0-- no per-month charge.cloudsqlandmemorystore: PaymentOption is "monthly", so calls the existing Billing SKU API (getSQLPricing/getRedisPricing) to deriveCommitmentPrice / termMonths. Billing lookup failure is non-fatal: logs a warning and leaves the fieldnilso the frontend renders "—" rather than a stale value.Test plan
go test github.com/LeanerCloud/CUDly/providers/gcp/services/computeengine/...--TestComputeEngineClient_ConvertGCPRecommendationassertsRecurringMonthlyCostis a non-nil pointer to 0go test github.com/LeanerCloud/CUDly/providers/gcp/services/cloudsql/...-- two new tests: success path (positive value matchingCommitmentPrice/12) and billing-failure path (nil)go test github.com/LeanerCloud/CUDly/providers/gcp/services/memorystore/...-- same two new tests for Memorystorego build ./...cleanTestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancellationfailure is unrelated to this PR (was failing on base branch before these changes)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes