Repository navigation
fix(gcp): wire Count/pricing into GCP converters; refuse fabricated prices - #1047
Merged
cristim merged 10 commits intoJun 7, 2026
Merged
Conversation
…rices Closes #1022. Addresses the GCP portion of #1020. **C1 (issue #1022):** `convertGCPRecommendation` (all 4 services) never set `Count`, causing `buildInsertRequest` to request a 0-vCPU / 0-MB commitment. Now extracts the vCPU count from the Recommender operation's structpb numeric value (or falls back to the overview `numericValue`). `buildInsertRequest` and `PurchaseCommitment` now refuse `rec.Count <= 0` with an explicit error before reaching the GCP API. **C2 (issue #1022):** All four `convertGCPRecommendation` implementations now call the service-specific pricing function to populate `CommitmentCost`, `OnDemandCost`, `SavingsPercentage`, and `BreakEvenMonths`. A scorer with any `MinSavingsPct` filter will no longer silently drop all GCP recommendations. Pricing failures are logged and the recommendation is still returned (the Recommender-derived `EstimatedSavings` is the authoritative figure). **M4 (issue #1020, GCP side):** All four `get*Pricing` functions previously fabricated a commitment price from hardcoded discount multipliers when no commitment SKU existed in the catalog, surfacing it as a real `OfferingDetails` figure. Following the `managedredis` precedent, these now return an error ("no commitment pricing found") instead of a fabricated number, consistent with `feedback_nullable_not_zero` and `feedback_empty_string_vs_error`. **M5 (issue #1022):** `PaymentOption` is now derived from `RecommendationParams.PaymentOption` (with a service-appropriate default when empty) rather than being hardcoded per service. **H2 (cloudstorage):** Fixed the iterator error swallow (`break` on error -> `return nil, err`) to match the other three service clients. Regression tests added in `computeengine/client_test.go`: - `TestConverterToInsert_CountNonZero_VCPUAmountSet`: realistic Recommender payload -> converter -> buildInsertRequest -> VCPU Amount > 0. - `TestConverterFillsPricingForScorer`: converter fills SavingsPercentage; scorer with MinSavingsPct=5 passes the recommendation. - `TestBuildInsertRequest_RefusesZeroCount`: Count <= 0 -> error, no Insert. - `TestGetComputePricing_NoCommitmentSKUReturnsError`: missing commitment SKU -> error (no fabricated price). Related: #718, #829 (payment-option monthly stamp).
cristim
added
bug
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reached
More reviews will be available in 14 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR refactors GCP provider and service clients to remove synthesized commitments, thread RecommendationParams through recommendation conversion, require commitment SKUs in billing catalog (erroring when absent), enforce pagination/cancellation behavior, and populate pricing/savings fields while logging catalog failures. ChangesGCP Pricing and Commitment Model Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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 |
Contributor
✅ Action performedReview finished.
|
This was referenced Jun 7, 2026
Closed
GCP CUDs have no upfront-payment option; the previous default of "upfront" caused downstream purchase-body construction to silently use a mode that does not exist for CUDs. Changes to align with cloudsql/memorystore/ cloudstorage which already default to "monthly" and with the monthly stamp introduced by PRs #718/#829 (which this supersedes in the converter layer). Regression tests: - TestConvertGCPRecommendation_EmptyParamsDefaultsToMonthly: FAILS on pre-fix code (paymentOption == "upfront"); PASSES after. - TestConvertGCPRecommendation_ParamPaymentOptionRespected: caller-supplied value is forwarded unchanged. Closes #1062
…ters The four convertGCPRecommendation implementations (cloudsql, cloudstorage, memorystore) scored 18 and extractVCPUCountFromRecommendation scored 17 on gocyclo, failing the pre-commit hook. Refactor: - Add extractResourceTypeFromContent/extractEstimatedSavings helpers shared by all three service converters; reduce each converter body. - Add fillRedisPricing/fillSQLPricing/fillStoragePricing per-service helpers that isolate the if-pricing-err-ok branching. - Add isMemoryAmountOp + vcpuCountFromOperationGroups to split the nested path-filter scan in extractVCPUCountFromRecommendation. All 235 GCP tests pass; gocyclo --over 10 finds no violations.
10-M2: Add termPlan(term) helper and memMBPerVCPU const in computeengine;
dedup the "TWELVE_MONTH"/"THIRTY_SIX_MONTH" and 4096 literals.
10-M3: Replace substring-match isResourceExhausted with typed checks:
errors.As(*googleapi.Error, Code==429) for REST, status.FromError
codes.ResourceExhausted for gRPC; substring match kept as fallback
for non-standard wrapped errors.
10-M7: GetAccounts returns empty slice when no ACTIVE projects are visible
instead of synthesising a fallback account from p.projectID, which
would silently hide credential/permission errors from callers. Test
updated to assert empty (not 1-element) on zero-projects input.
10-L1: Remove the no-op commitmentType if-branch in convertGCPCommitmentToCommon
(both arms assigned CommitmentCUD).
10-L2: CloudStorage GetExistingCommitments returns empty; enumerating
regional buckets is not a commitment indicator (GCS has no CUD API).
Test updated accordingly.
10-L3: CloudSQL GetExistingCommitments returns empty; PricingPlan "PACKAGE"
is a legacy per-instance billing mode, not a spend-based CUD. Test
updated accordingly.
10-L4: Wrap memorystore realRecommenderClient.ListRecommendations in
realRecommenderIterator for diffability with the other 3 service
clients.
10-N3: Rename local var 'provider' in recommendations.getRegions to 'p' to
avoid shadowing the imported 'provider' package.
Deferred (logged to docs/code-review/open-questions/fold-1047.md):
- 10-M1: dead GroupCommitments types -- no-delete: pending owner confirmation
- 10-M6: struct-stored ctx removal -- tests assert on ctx field; needs test
update before removal (correctness impact: none, field is never read)
- 10-N1: common.Ptr[T] -- helper does not yet exist; lands with #1035
Closes #1062
Member
Author
FOLD-1047 additions summaryThree additional commits on this PR address the remaining FOLD-1047 findings from docs/code-review/16-remaining-findings-plan.md: Commit 1: fix(gcp/computeengine): default PaymentOption to "monthly" (10-M5)
Commit 2: refactor(gcp): extract helpers to fix cyclomatic complexity (CI fix)
Commit 3: fix(gcp): FOLD-1047 correctness findings (10-M2,M3,M7,L1,L2,L3,L4,N3)
Deferred to docs/code-review/open-questions/fold-1047.md: 10-M1 (no-delete pending owner confirm), 10-M6 (ctx field, tests assert on it), 10-N1 (common.Ptr[T] not yet available). Closes #1062 @coderabbitai review |
Contributor
|
Kicking off the incremental review of the new commits now. 🧠 Learnings used✅ Action performedReview finished.
|
…1022) GCP's ResourceCommitment.Type enum is VCPU/MEMORY/LOCAL_SSD/ACCELERATOR. buildInsertRequest and the public GroupCommitments helper emitted the invalid type string "MEMORY_MB" for the memory member, so every Compute Engine CUD purchase would be rejected by RegionCommitments.Insert. The Amount unit (MB) was already correct; only the enum string was wrong. Switch the outbound type to "MEMORY", make the inbound Recommender memory detector (isMemoryAmountOp) tolerate both "MEMORY" and the legacy "MEMORY_MB" spelling since the path_filter wire format is not documented, and update the resource-pair doc comments. Regression tests: TestGroupCommitments_UsesValidMemoryEnum and TestIsMemoryAmountOp_MatchesBothSpellings added; the converter->insert test now rejects any invalid ResourceCommitment.Type. All fail on the pre-fix "MEMORY_MB" code and pass after.
Member
Author
Money-path verification + fixIndependent deep correctness verification of the GCP recommendation -> purchase Fixed (commit on this PR): Verified CORRECT (no change needed):
Verification: providers/gcp |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
providers/gcp/services/computeengine/client.go (1)
867-885:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize non-monthly GCP payment options instead of passing them through.
Line 867-Line 875 currently preserves any non-empty
params.PaymentOption. If the scheduler provides"upfront", the recommendation carries an invalid GCP payment mode instead of canonical"monthly".💡 Suggested fix
- paymentOption := params.PaymentOption - if paymentOption == "" { - // GCP CUDs are billed monthly with no upfront option; "monthly" is - // the only valid default. Aligns with cloudsql/memorystore/cloudstorage - // and supersedes the monthly stamp introduced in PR `#829` (which - // stamped "monthly" in the purchase body but not in the converter - // default -- see PR `#1047` for the supersession note). - paymentOption = "monthly" - } + paymentOption := "monthly" + if params.PaymentOption != "" && !strings.EqualFold(params.PaymentOption, "monthly") { + log.Printf("computeengine: unsupported GCP payment option %q; forcing monthly", params.PaymentOption) + }Based on learnings, GCP CUDs in this repository are monthly-only and non-monthly inputs (including legacy
"upfront") should be collapsed to"monthly"at conversion/scheduler boundaries.🤖 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/computeengine/client.go` around lines 867 - 885, The code currently preserves any non-empty params.PaymentOption; change the normalization so that only the canonical "monthly" is allowed: read params.PaymentOption into paymentOption, and if it is empty or not exactly "monthly" then set paymentOption = "monthly" before constructing the common.Recommendation (the block that sets PaymentOption using paymentOption). Ensure this logic touches the params.PaymentOption -> paymentOption handling in client.go so legacy values like "upfront" are collapsed to "monthly" at conversion time.Source: Learnings
providers/gcp/services/memorystore/client.go (2)
490-518:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the requested term instead of forcing
1yr.This converter now accepts
RecommendationParams, but it still hardcodesrec.Termto"1yr". Any caller requesting3yrMemorystore recommendations will get the wrong term and the wrong pricing path.Suggested fix
func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { paymentOption := params.PaymentOption if paymentOption == "" { paymentOption = "monthly" } + term := params.Term + if term == "" { + term = "1yr" + } rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceCache, Account: c.projectID, Region: c.region, CommitmentType: common.CommitmentCUD, Timestamp: time.Now(), - Term: "1yr", + Term: term, PaymentOption: paymentOption, }🤖 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/memorystore/client.go` around lines 490 - 518, The convertGCPRecommendation function currently hardcodes rec.Term = "1yr"; update it to propagate the requested term from the RecommendationParams (use params.Term) and default to "1yr" only when params.Term is empty, then use rec.Term when computing termYears for pricing and when calling fillRedisPricing; specifically, set rec.Term = params.Term || "1yr" (preserving existing handling of "3yr" and "3" to map to termYears=3) so callers requesting "3yr" get the correct term and pricing path in convertGCPRecommendation and fillRedisPricing.
312-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis is still mixing unit-price SKUs with term-total costs.
extractPriceFromSKUreturns a rawUnitPrice, but onlyonDemandPriceis expanded tohoursInTerm.commitmentPriceremains in single-unit form, so the returnedCommitmentPrice,HourlyRate,SavingsPercentage,BreakEvenMonths, andGetOfferingDetails.TotalCostare all off by the term multiplier. Normalize the commitment SKU to the same term-total basis before using it in any downstream math.🤖 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/memorystore/client.go` around lines 312 - 340, getRedisPricing is mixing per-unit SKUs with term-total costs: extractPricingFromSKUs / extractPriceFromSKU returns unit prices but only onDemandPrice is multiplied by hoursInTerm; commitmentPrice must be normalized to the same term-total basis before any math. Update getRedisPricing to convert commitmentPrice to term total (e.g., multiply the commitment unit price by hoursInTerm or the appropriate term multiplier returned by extractPricingFromSKUs) before computing HourlyRate, CommitmentPrice, SavingsPercentage, BreakEvenMonths and any GetOfferingDetails.TotalCost; keep OnDemandPrice computed as now (onDemand unit * hoursInTerm) and then derive HourlyRate as normalizedCommitmentTotal / hoursInTerm and pass the normalized commitment total into RedisPricing.providers/gcp/services/cloudstorage/client.go (2)
499-527:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
RecommendationParams.Termhere.
rec.Termis hardcoded to"1yr", so a caller requesting3yrstill gets a 1-year recommendation and 1-year pricing. Callers already pass the desired term throughRecommendationParams.Term, so this drops the user's selection for Cloud Storage recommendations.Suggested fix
func (c *CloudStorageClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { paymentOption := params.PaymentOption if paymentOption == "" { paymentOption = "monthly" } + term := params.Term + if term == "" { + term = "1yr" + } rec := &common.Recommendation{ Provider: common.ProviderGCP, Service: common.ServiceStorage, Account: c.projectID, Region: c.region, CommitmentType: common.CommitmentReservedCapacity, Timestamp: time.Now(), - Term: "1yr", + Term: term, PaymentOption: paymentOption, }🤖 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/cloudstorage/client.go` around lines 499 - 527, In convertGCPRecommendation, rec.Term is hardcoded to "1yr" which ignores params.RecommendationParams.Term; change it to derive rec.Term from params.Term (defaulting to "1yr" when empty) so callers requesting "3yr" are honored, and keep the existing termYears logic (which reads rec.Term) and the call to c.fillStoragePricing(ctx, rec, termYears) so pricing matches the selected term; update any normalization (e.g., accept "3" or "3yr") when mapping params.Term to rec.Term.
321-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize commitment pricing onto the same basis as on-demand pricing.
extractStoragePriceFromSKUreturns a rawUnitPrice, but this function only convertsonDemandPriceto a term-total.commitmentPricestays in unit-price form, soSavingsPercentage,CommitmentPrice,HourlyRate,BreakEvenMonths, andGetOfferingDetails.TotalCostare computed from mismatched units and materially understate the commitment cost. Scale both sides to the same term basis before comparing or returning them.🤖 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/cloudstorage/client.go` around lines 321 - 349, getStoragePricing mixes unit and term totals: onDemandPrice is multiplied by hoursInTerm but commitmentPrice is left as a unit price, causing all downstream math to be inconsistent. Multiply the raw commitmentPrice returned by extractStoragePricingFromSKUs by hoursInTerm (e.g., commitmentPriceTerm := commitmentPrice * hoursInTerm), then pass commitmentPriceTerm into calculateStorageSavingsPercentage and set StoragePricing.CommitmentPrice = commitmentPriceTerm; keep HourlyRate = commitmentPriceTerm / hoursInTerm (or the original unit rate) and leave OnDemandPrice as onDemandPrice * hoursInTerm so all comparisons and outputs in getStoragePricing, calculateStorageSavingsPercentage, and any callers use the same term basis.
🤖 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.
Outside diff comments:
In `@providers/gcp/services/cloudstorage/client.go`:
- Around line 499-527: In convertGCPRecommendation, rec.Term is hardcoded to
"1yr" which ignores params.RecommendationParams.Term; change it to derive
rec.Term from params.Term (defaulting to "1yr" when empty) so callers requesting
"3yr" are honored, and keep the existing termYears logic (which reads rec.Term)
and the call to c.fillStoragePricing(ctx, rec, termYears) so pricing matches the
selected term; update any normalization (e.g., accept "3" or "3yr") when mapping
params.Term to rec.Term.
- Around line 321-349: getStoragePricing mixes unit and term totals:
onDemandPrice is multiplied by hoursInTerm but commitmentPrice is left as a unit
price, causing all downstream math to be inconsistent. Multiply the raw
commitmentPrice returned by extractStoragePricingFromSKUs by hoursInTerm (e.g.,
commitmentPriceTerm := commitmentPrice * hoursInTerm), then pass
commitmentPriceTerm into calculateStorageSavingsPercentage and set
StoragePricing.CommitmentPrice = commitmentPriceTerm; keep HourlyRate =
commitmentPriceTerm / hoursInTerm (or the original unit rate) and leave
OnDemandPrice as onDemandPrice * hoursInTerm so all comparisons and outputs in
getStoragePricing, calculateStorageSavingsPercentage, and any callers use the
same term basis.
In `@providers/gcp/services/computeengine/client.go`:
- Around line 867-885: The code currently preserves any non-empty
params.PaymentOption; change the normalization so that only the canonical
"monthly" is allowed: read params.PaymentOption into paymentOption, and if it is
empty or not exactly "monthly" then set paymentOption = "monthly" before
constructing the common.Recommendation (the block that sets PaymentOption using
paymentOption). Ensure this logic touches the params.PaymentOption ->
paymentOption handling in client.go so legacy values like "upfront" are
collapsed to "monthly" at conversion time.
In `@providers/gcp/services/memorystore/client.go`:
- Around line 490-518: The convertGCPRecommendation function currently hardcodes
rec.Term = "1yr"; update it to propagate the requested term from the
RecommendationParams (use params.Term) and default to "1yr" only when
params.Term is empty, then use rec.Term when computing termYears for pricing and
when calling fillRedisPricing; specifically, set rec.Term = params.Term || "1yr"
(preserving existing handling of "3yr" and "3" to map to termYears=3) so callers
requesting "3yr" get the correct term and pricing path in
convertGCPRecommendation and fillRedisPricing.
- Around line 312-340: getRedisPricing is mixing per-unit SKUs with term-total
costs: extractPricingFromSKUs / extractPriceFromSKU returns unit prices but only
onDemandPrice is multiplied by hoursInTerm; commitmentPrice must be normalized
to the same term-total basis before any math. Update getRedisPricing to convert
commitmentPrice to term total (e.g., multiply the commitment unit price by
hoursInTerm or the appropriate term multiplier returned by
extractPricingFromSKUs) before computing HourlyRate, CommitmentPrice,
SavingsPercentage, BreakEvenMonths and any GetOfferingDetails.TotalCost; keep
OnDemandPrice computed as now (onDemand unit * hoursInTerm) and then derive
HourlyRate as normalizedCommitmentTotal / hoursInTerm and pass the normalized
commitment total into RedisPricing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c522817-8270-45f6-b35c-17ca46075ad6
📒 Files selected for processing (11)
providers/gcp/provider.goproviders/gcp/provider_test.goproviders/gcp/recommendations.goproviders/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
…d pricing math - computeengine: force paymentOption="monthly" unconditionally; log non-monthly inputs instead of passing them through (GCP CUDs are monthly-only). - memorystore/cloudstorage: propagate params.Term to rec.Term in convertGCPRecommendation; hardcoded "1yr" dropped 3yr caller requests. - memorystore/cloudstorage: fix unit-mismatch in getRedisPricing/getStoragePricing where per-hour commitmentPrice was passed to calculateSavingsPercentage as a term total, producing ~99.99% savings. Multiply by hoursInTerm before math; HourlyRate is now the raw per-hour SKU rate. Regression tests: 5 new tests that fail on pre-fix code. Follow-up for CloudSQL identical issue: closes #1078.
Member
Author
CR post-commit findings triage (review 4445458345, 2026-06-07T16:47:56Z)All 5 findings were in the "outside diff range" block but all touch files present in this PR's diff. All 5 are fixed in commit c6280c3. F1 - computeengine/client.go:867-885 - Non-monthly payment option passed throughDisposition: FIXED GCP CUDs are monthly-only. The prior code forwarded any non-empty F2 - memorystore/client.go:490-518 - rec.Term hardcoded to "1yr"Disposition: FIXED
F3 - memorystore/client.go:312-340 - commitmentPrice unit mismatchDisposition: FIXED
F4 - cloudstorage/client.go:499-527 - rec.Term hardcoded to "1yr"Disposition: FIXED Same as F2 for Cloud Storage. Regression test F5 - cloudstorage/client.go:321-349 - commitmentPrice unit mismatchDisposition: FIXED Same as F3 for Cloud Storage. Regression test Follow-upCloudSQL @coderabbitai review |
Contributor
|
Recording the commitmentPrice unit semantics and term-propagation patterns for future reviews. Triggering the incremental review of commit ✏️ Learnings added
🧠 Learnings used✅ Action performedReview finished.
|
Two bugs mirroring the Memorystore/CloudStorage fixes from PR #1047: 1. Unit mismatch in getSQLPricing: commitmentPrice (per-hour SKU rate) was passed directly to calculateSQLSavingsPercentage which expects a term total, producing ~99.99% savings. Now multiplies by hoursInTerm first (commitmentPriceTerm = commitmentPrice * hoursInTerm). HourlyRate is now the raw per-hour rate instead of per-hour / hours (near-zero). 2. Term propagation in convertGCPRecommendation: rec.Term was hardcoded to "1yr", ignoring params.Term. A caller requesting "3yr" now gets "3yr". PaymentOption was not affected: CloudSQL already defaults to "monthly" when params.PaymentOption is empty, matching the Memorystore/CloudStorage pattern. Regression tests: 2 new tests that fail pre-fix and pass after.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/gcp/services/memorystore/client.go (1)
494-514: ⚡ Quick winPaymentOption handling is inconsistent with computeengine client.
Per learnings, GCP CUDs are inherently monthly-billed, and the computeengine client forces
paymentOption = "monthly"unconditionally, logging a WARN when a non-monthly value is supplied. Here, memorystore only defaults to"monthly"whenparams.PaymentOptionis empty, allowing non-monthly values (e.g.,"upfront") to pass through silently.Consider aligning with the computeengine pattern to force monthly and log a warning for non-monthly input.
♻️ Suggested change to force monthly payment option
func (c *MemorystoreClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation { paymentOption := params.PaymentOption - if paymentOption == "" { - paymentOption = "monthly" + if paymentOption != "monthly" && paymentOption != "" { + log.Printf("WARN: memorystore: coercing non-monthly payment option %q to monthly (GCP CUDs are monthly-only)", paymentOption) } + paymentOption = "monthly" term := params.Term🤖 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/memorystore/client.go` around lines 494 - 514, In convertGCPRecommendation, force the PaymentOption to "monthly" like computeengine does: if params.PaymentOption is non-empty and not "monthly", emit a warning log mentioning the supplied value (use the client logger available in MemorystoreClient) and then set paymentOption = "monthly"; if empty, also set paymentOption = "monthly". Update references to params.PaymentOption/paymentOption in convertGCPRecommendation accordingly so no non-monthly values are allowed through.Source: Learnings
🤖 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.
Nitpick comments:
In `@providers/gcp/services/memorystore/client.go`:
- Around line 494-514: In convertGCPRecommendation, force the PaymentOption to
"monthly" like computeengine does: if params.PaymentOption is non-empty and not
"monthly", emit a warning log mentioning the supplied value (use the client
logger available in MemorystoreClient) and then set paymentOption = "monthly";
if empty, also set paymentOption = "monthly". Update references to
params.PaymentOption/paymentOption in convertGCPRecommendation accordingly so no
non-monthly values are allowed through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbc1bb1c-09d8-44ff-9552-e68421ce0f8d
📒 Files selected for processing (6)
providers/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
🚧 Files skipped from review as they are similar to previous changes (2)
- providers/gcp/services/computeengine/client_test.go
- providers/gcp/services/cloudstorage/client.go
Member
Author
CloudSQL #1078 folded in (commit 20590c6)Two bug classes fixed in Bug 1 -- Unit mismatch (getSQLPricing): Bug 2 -- Term propagation (convertGCPRecommendation): Not present: PaymentOption bug (F1 analog) -- CloudSQL already had Regression tests: 2 new tests ( Verification: @coderabbitai review |
Contributor
|
The two regression tests ( Updating the learning for CloudSQL to reflect the fix is now merged into this PR. Triggering the incremental review of commit ✏️ Learnings added
⛔ Learnings removed🧠 Learnings used✅ Action performedReview finished.
|
…um (de-hardcode) Target 1 - MEMORY amount from Recommender payload: - Add memoryMBFromOperationGroups (parallel to vcpuCountFromOperationGroups) to extract the MEMORY resource amount from the commitment operation group. - Add extractMemoryMBFromRecommendation to store the payload memory in ComputeDetails.MemoryGB on the Recommendation (called from convertGCPRecommendation). - Add memoryMBFromDetails to read the value back; returns an error (fail-loud) when absent -- no silent fallback to the 4096 MB/vCPU ratio. - Remove the memMBPerVCPU const (no longer used). - buildInsertRequest and GroupCommitments now use memoryMBFromDetails; both return/log an error for recs that lack the payload memory field. - Regression tests: TestBuildInsertRequest_RefusesMissingMemory, TestGroupCommitments_SkipsRecsWithoutMemory, and strengthened TestConverterToInsert_CountNonZero_VCPUAmountSet (asserts 6144 MB from payload, not ratio-derived 16384 MB). Target 2 - termPlan SDK enum constants: - termPlan now returns (string, error) using computepb.Commitment_TWELVE_MONTH.String() and computepb.Commitment_THIRTY_SIX_MONTH.String(). - Accepts 1yr/1/12mo (TWELVE_MONTH) and 3yr/3/36mo (THIRTY_SIX_MONTH). - Returns an error on unrecognised/empty term (no silent 12-month default). - Error threaded through buildInsertRequest, GroupCommitments, and convertGCPRecommendation (which returns nil on bad term). - Regression tests: TestTermPlan_UsesSdkEnumConstants, TestTermPlan_RejectsUnknownTerm, TestTermPlan_AcceptsAllDocumentedForms. H-3 audit finding - convertGCPRecommendation propagates params.Term: - No longer hardcodes Term: "1yr"; propagates params.Term (default "1yr"). - Validates the term via termPlan and returns nil (skips rec) when invalid. - Regression tests: TestConvertGCPRecommendation_PropagatesParamsTerm, TestConvertGCPRecommendation_RejectsUnknownTerm. All existing 247 GCP tests pass; 57 computeengine tests pass under -race.
Member
Author
Hardcode removal commit (8f9a787)Three items addressed in the latest push: TARGET 1 - MEMORY amount from Recommender payload (fail-loud, no ratio fallback)
TARGET 2 - termPlan uses Commitment_Plan SDK enum (fail-loud, no silent 12-month default)
H-3 audit finding - params.Term propagation in convertGCPRecommendation
Verification: @coderabbitai review |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Recording the new behaviors for future reviews. Triggering the incremental review of commit [review] (˳˘ ɜ˘)˳ ♡ ✏️ Learnings added
🧠 Learnings used |
H-1 (rec state never filtered): GetRecommendations in all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) now skip recommendations whose StateInfo.State is not ACTIVE. The GCP Recommender returns recommendations in ACTIVE/CLAIMED/SUCCEEDED/FAILED/ DISMISSED states; acting on a non-ACTIVE rec is a cross-run double- purchase vector and inflates actionable rec counts. Uses typed enum constant recommenderpb.RecommendationStateInfo_ACTIVE (not a string). The proto GetStateInfo() getter is nil-safe so nil-StateInfo recs (STATE_UNSPECIFIED) are also filtered. H-2 (cache/storage advertised but produce zero recs): collectRegion in recommendations.go now fans out to memorystore and cloudstorage in addition to compute and cloudsql, guarded by shouldIncludeService. Both services have complete GetRecommendations implementations; their PurchaseCommitment paths are advisory-only no-ops (documented) which is orthogonal to surfacing recommendations. regionResult gains cache and storage slices; GetRecommendations merge appends them per region. Decision log: docs/code-review/open-questions/fix-1047-h1h2.md. Regression tests: - TestGetRecommendations_FiltersNonActiveStates: five-rec mock (one ACTIVE, four non-ACTIVE); asserts only ACTIVE is returned. Fails pre-fix. - TestGetRecommendations_ActiveRecIncluded: positive case. - TestRegionResult_HasCacheAndStorageFields: compile-time + runtime check that regionResult carries all four service slices (H-2). - TestShouldIncludeService_Cache_Storage: routing logic for cache/ storage service types. - Existing WithMock tests in all four services updated to set StateInfo=ACTIVE (they failed post-fix; pre-fix they passed against STATE_UNSPECIFIED=0 which was not filtered). Follow-up: remaining Medium/Low/Nit findings from 10-gcp-and-pkg.md tracked in issue #1083. 257 GCP tests pass under -race. go build/vet ./... clean.
Member
Author
H-1 and H-2 fixes (commit 95a390f)Two additional High findings from the GCP broad audit addressed: H-1 -- Rec state filter (anti double-purchase)
H-2 -- Cache/storage wired into fan-out
257 GCP tests pass under Remaining Medium/Low/Nit findings tracked in issue LeanerCloud/cloud-commitments-go#6. @coderabbitai review |
Contributor
|
The Recording both patterns for future reviews. ✏️ Learnings added
🧠 Learnings used✅ Action performedReview finished.
|
…commendation Extract per-service fetch helpers (collectComputeRecs, collectSQLRecs, collectCacheRecs, collectStorageRecs) from collectRegion so the Acquire/Release and client-construction branches are in named functions rather than anonymous closures, bringing collectRegion from 17 to 9. Extract enrichRecWithPricing from convertGCPRecommendation to move the pricing lookup and BreakEvenMonths calculation into a dedicated method, bringing convertGCPRecommendation from 12 to 5. Apply gofmt to client.go (trailing blank line) and client_test.go (comment alignment). No logic changes. Fixes the failing "Check cyclomatic complexity" and "Run gofmt" pre-commit hooks in CI for PR #1047.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
5 tasks
This was referenced Jun 10, 2026
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
Verification
Notes
Scope expanded
Additional commits added as part of the FOLD-1047 pass from
docs/code-review/16-remaining-findings-plan.md:Also closes #1062 (grouped issue for findings 10-M2, 10-M3, 10-M5, 10-M7, 10-L1, 10-L2, 10-L3, 10-L4, 10-N3)
computeenginePaymentOption default from "upfront" to "monthly". GCP CUDs have no upfront-payment option; the prior default caused incorrect purchase-body construction. All four converters now consistently default to "monthly". This supersedes PR fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829 ("fix/committed-monthly"): fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829 stamped "monthly" in the scheduler purchase body but the converter-layer default remained "upfront", causing a mismatch whenRecommendationParams.PaymentOptionis empty. That mismatch is now closed at the converter layer. If fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829 is still open, it can be closed as superseded by this PR.convertGCPRecommendationimplementations (cloudsql, cloudstorage, memorystore -- scored 18 on gocyclo) andextractVCPUCountFromRecommendation(computeengine -- scored 17) by extractingextractResourceTypeFromContent,extractEstimatedSavings,fill*Pricing,isMemoryAmountOp, andvcpuCountFromOperationGroupshelpers.termPlan(term)helper andmemMBPerVCPUconst in computeengine; dedup the "TWELVE_MONTH"/"THIRTY_SIX_MONTH" and 4096 literals across all call sites.isResourceExhaustedwith typed checks (errors.As(*googleapi.Error)for REST,status.FromError(codes.ResourceExhausted)for gRPC); substring match retained as fallback.GetAccountsreturns empty slice when no ACTIVE projects are visible instead of synthesising a fallback account (which hid credential errors from callers).commitmentTypeif-branch inconvertGCPCommitmentToCommon(both arms wereCommitmentCUD).CloudStorage.GetExistingCommitmentsreturns empty; GCS has no commitment API and enumerating regional buckets was not a commitment indicator.CloudSQL.GetExistingCommitmentsreturns empty; PricingPlan "PACKAGE" is a legacy per-instance billing mode, not a spend-based CUD indicator.realRecommenderClient.ListRecommendationsinrealRecommenderIteratorfor diffability with the other three service clients.providerinrecommendations.getRegionstopto avoid shadowing the importedproviderpackage.Deferred (logged to
docs/code-review/open-questions/fold-1047.md):GroupCommitmentstypes -- no-delete pending owner confirmationctxfield removal -- tests assert on the field; needs test updates first (no correctness impact, field is never read)common.Ptr[T]-- helper does not exist yet; will land with refactor(arch): provider-module fragmentation forces copy-paste (fan-out, env reader, savings formula); + pkg/errors orphan, logging split #1035Also closes #1078: CloudSQL unit-mismatch + term-propagation (identical pattern to Memorystore/CloudStorage fixes above; folded into this PR per issue #1078).
Money-path verification pass (independent)
Deep correctness verification of the GCP recommendation->purchase path against the
vendored GCP SDK schema. Found and fixed one real money-path defect; all other
items verified CORRECT.
DEFECT (fixed in this PR):
buildInsertRequestand the publicGroupCommitmentshelper emitted
"MEMORY_MB"as theResourceCommitment.Type. GCP's enum isVCPU/MEMORY/LOCAL_SSD/ACCELERATOR(confirmed incomputepb.ResourceCommitment_Type),so
"MEMORY_MB"is invalid andRegionCommitments.Insertrejects it, failing everyCompute Engine CUD purchase. The Amount unit (MB) was already correct; only the enum
string was wrong. Fixed ->
"MEMORY"; inboundisMemoryAmountOpnow tolerates bothspellings. Regression tests added that fail on the pre-fix code.
Verified CORRECT: vCPU count extraction (units are a vCPU count, not core-seconds/
dollars, per the SDK ResourceCommitment doc); RequestId + deterministic Name
idempotency (no double-purchase on re-drive); term->plan mapping;
Count<=0guard;pricing functions return errors (no fabricated prices) when no commitment SKU exists;
divide-by-zero guarded upstream of the savings calc; computeengine
GetExistingCommitmentsconsumes all pages and errors (not truncates) at the cap;cloudsql/cloudstorage/memorystore empty-return doc comments accurate; scorer
integration (SavingsPercentage/BreakEvenMonths/Count all populated).
Scope expanded (hardcode removal)
Two hardcoded constructs and one audit finding addressed in the latest commit (8f9a787):
TARGET 1 - MEMORY from Recommender payload (fail-loud)
memMBPerVCPU = 4096const and its use as a ratio fallback.memoryMBFromOperationGroups(parallel tovcpuCountFromOperationGroups) to extract the MEMORY resource amount from the Recommender payload's commitment operation group.extractMemoryMBFromRecommendationto store the result inComputeDetails.MemoryGBon the Recommendation (called fromconvertGCPRecommendation).buildInsertRequestandGroupCommitmentsnow callmemoryMBFromDetailswhich returns an error (not a fallback) whenDetails.MemoryGBis absent. GCP Recommender does carry the MEMORY amount in the payload; the fail-loud policy ensures non-standard machine families (e.g. high-memory N2) never get an incorrect commitment.TestBuildInsertRequest_RefusesMissingMemory,TestGroupCommitments_SkipsRecsWithoutMemory,TestConverterToInsert_CountNonZero_VCPUAmountSet(asserts the exact payload value 6144 MB, not ratio-derived 16384 MB).TARGET 2 - termPlan uses SDK Commitment_Plan enum (fail-loud)
termPlannow returns(string, error)usingcomputepb.Commitment_TWELVE_MONTH.String()andcomputepb.Commitment_THIRTY_SIX_MONTH.String().buildInsertRequest,GroupCommitments, andconvertGCPRecommendation(returns nil on bad term).TestTermPlan_UsesSdkEnumConstants,TestTermPlan_RejectsUnknownTerm,TestTermPlan_AcceptsAllDocumentedForms.H-3 audit finding -
convertGCPRecommendationpropagatesparams.TermTerm: "1yr"regardless ofparams.Term; propagates the requested term (defaults to "1yr" when empty) and validates it viatermPlan, returning nil for unroutable recs.TestConvertGCPRecommendation_PropagatesParamsTerm,TestConvertGCPRecommendation_RejectsUnknownTerm.Verification: 247 GCP tests pass, 57 computeengine tests pass under
-race. Repo-rootgo build ./...clean.Scope expanded (H-1 and H-2 fixes)
Latest commit (95a390f) addresses two additional High findings from the GCP broad audit.
H-1 -- Rec state never filtered (double-purchase vector)
computeengine,cloudsql,memorystore,cloudstorage) now skip recommendations whoseStateInfo.Stateis notACTIVE. The GCP Recommender returns all states (ACTIVE/CLAIMED/SUCCEEDED/FAILED/DISMISSED) unless filtered at the API layer; acting on a non-ACTIVE rec is a cross-run double-purchase vector and inflates actionable rec counts.recommenderpb.RecommendationStateInfo_ACTIVE(not a string). The nil-safe proto getter means nil-StateInfo recs (STATE_UNSPECIFIED) are also filtered.TestGetRecommendations_FiltersNonActiveStates(five-rec mock, only ACTIVE passes) andTestGetRecommendations_ActiveRecIncluded.H-2 -- Cache/storage advertised but produce zero recs
GetRecommendationsimplementations; theirPurchaseCommitmentpaths being advisory-only no-ops is orthogonal to surfacing recommendations.collectRegionnow fans out to all four services concurrently (guarded byshouldIncludeService).regionResultgainscacheandstorageslices; the merge inGetRecommendationsappends them per region.TestRegionResult_HasCacheAndStorageFieldsandTestShouldIncludeService_Cache_Storage.257 GCP tests pass under
-race.go build/vet ./...clean.Remaining Medium/Low/Nit findings from the broad audit tracked in issue LeanerCloud/cloud-commitments-go#6.