Repository navigation
fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) - #829
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 58 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 (2)
📝 WalkthroughWalkthroughStandardizes “unrecognized” terminology across GCP Compute Engine recommendation handling and tests, adds a monthly payment-option assertion, and reorganizes test mocks without changing their behavior. ChangesGCP recommendation terminology
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
Address CodeRabbit review on PR #848: - F1 (types.go): remove the misleading "GCP all-upfront" example from the MonthlyCost field comment. In this repo GCP CUD commitments are always monthly-billed (per #709, #717, #718, #829), so the only provider that can legitimately produce a NULL recurring breakdown at the commitment layer is Azure all-upfront. Comment now mentions only Azure and explicitly notes that GCP always populates MonthlyCost. - F2 (store_postgres_db_test.go): add a "MonthlyCost nil round-trip" subtest that inserts a PurchaseHistoryRecord with MonthlyCost: nil and asserts it reads back as nil. This is the contract migration 000063 was meant to establish (NULL -> nil, distinct from 0.0 -> &0), and the frontend renders the dash glyph vs "$0.00" based on it. Also strengthens the existing 0.0 subtest to assert the pointer is non-nil with *MonthlyCost == 0.0, so both sides of the contract are pinned end-to-end. No production code changes; comment + test only.
cristim
added a commit
that referenced
this pull request
Jun 3, 2026
…#848) * feat(db): nullable MonthlyCost on PurchaseHistoryRecord (closes #255) Migration 000063 drops NOT NULL on purchase_history.monthly_cost so rows from providers without a monthly recurring breakdown (Azure/GCP all-upfront) can store NULL instead of a misleading 0.0. - PurchaseHistoryRecord.MonthlyCost: float64 -> *float64 - InventoryCommitment.MonthlyCost: float64 -> *float64 - All scan loops use sql.NullFloat64; nil DB value -> nil pointer - execution.go: remove derefFloat64; pass pointer straight through - handler_inventory.go: skip nil MonthlyCost in coverage aggregation - handler_history.go: sumRecommendationMonthlyCostPtr returns nil when every rec has nil MonthlyCost (renders as -- not $0.00) - Frontend: monthly_cost typed number|null; inventory cell renders -- - All test fixtures and assertions updated to use *float64 (pf helper) * style(api): apply gofmt alignment to InventoryCommitment and related types * fix(api/tests): wrap MonthlyCost literals in float64Ptr after nullable refactor The PurchaseHistoryRecord.MonthlyCost type changed to *float64 in 3cd63c8 but four test literals in handler_inventory_test.go (lines 186, 196, 499, 508) still assigned untyped float constants, breaking go vet with: cannot use 80.0 (untyped float constant 80) as *float64 value in struct literal Switch them to float64Ptr(...) consistent with the rest of the file (lines 77, 90, 145, 430 already use the helper). * docs(db): clarify GCP commitments are monthly + nil round-trip test Address CodeRabbit review on PR #848: - F1 (types.go): remove the misleading "GCP all-upfront" example from the MonthlyCost field comment. In this repo GCP CUD commitments are always monthly-billed (per #709, #717, #718, #829), so the only provider that can legitimately produce a NULL recurring breakdown at the commitment layer is Azure all-upfront. Comment now mentions only Azure and explicitly notes that GCP always populates MonthlyCost. - F2 (store_postgres_db_test.go): add a "MonthlyCost nil round-trip" subtest that inserts a PurchaseHistoryRecord with MonthlyCost: nil and asserts it reads back as nil. This is the contract migration 000063 was meant to establish (NULL -> nil, distinct from 0.0 -> &0), and the frontend renders the dash glyph vs "$0.00" based on it. Also strengthens the existing 0.0 subtest to assert the pointer is non-nil with *MonthlyCost == 0.0, so both sides of the contract are pinned end-to-end. No production code changes; comment + test only.
Member
Author
|
@coderabbitai review |
Contributor
Rate Limit Exceeded
|
This was referenced Jun 7, 2026
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
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
cristim
added a commit
that referenced
this pull request
Jun 7, 2026
…rices (#1047) * fix(gcp): wire Count/pricing into GCP converters; refuse fabricated prices 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). * fix(gcp/computeengine): default PaymentOption to "monthly" (10-M5) 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 * refactor(gcp): extract helpers to fix cyclomatic complexity in converters 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. * fix(gcp): FOLD-1047 correctness findings (10-M2,M3,M7,L1,L2,L3,L4,N3) 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 * fix(gcp/computeengine): use valid MEMORY resource enum on CUD insert (#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. * fix(gcp): address CR post-commit findings on payment option, term, and 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. * fix(gcp/cloudsql): correct savings unit, term propagation (closes #1078) 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. * refactor(gcp): extract memory from payload and use Commitment_Plan enum (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. * fix(gcp): filter non-ACTIVE recs and wire cache/storage into fan-out 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. * fix(gcp): reduce gocyclo complexity on collectRegion and convertGCPRecommendation 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 full review Generated by Claude Code |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and diff
gh pr view 829 --repo LeanerCloud/CUDly --json title,body,files,commits,state
echo "=== PR DIFF ==="
gh pr diff 829 --repo LeanerCloud/CUDlyLength of output: 3966 Review of PR
|
| Area | Status |
|---|---|
| Correctness of fix | ✅ |
| Consistency with peer GCP services | ✅ |
Consistency with ValidPaymentOptionsByProvider |
✅ |
| Test coverage for the new contract | ✅ |
| Scope / blast radius | ✅ Surgical — 1 literal, 1 test assertion |
| Unintended side-effects | None detected |
Note:
Term: "1yr"remains hardcoded on Line 803 in this PR. That is out of scope here and is addressed separately in PR#1047.
The PR looks good — approved.
[approve]
🧠 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-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-05-25T21:45:57.471Z
Learning: In this repository (CUDly), GCP CUD commitments are inherently monthly-billed. The GCP CUD purchase API (`providers/gcp/services/computeengine/client.go:350-373`, `buildCommitmentRequests`) takes only a `Plan` field (TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field. Therefore, the only valid payment option for GCP in `ValidPaymentOptionsByProvider` is `"monthly"`. The `NormalizePaymentOption` GCP branch should collapse any non-monthly token (including legacy `"upfront"`) to `"monthly"`, with a WARN log at the `scheduler.convertRecommendations` boundary.
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: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`.
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.
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-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, the `memMBPerVCPU = 4096` const was removed in PR `#1047` commit 8f9a787. MEMORY MB is now extracted directly from the Recommender payload via `memoryMBFromOperationGroups` (sibling of `vcpuCountFromOperationGroups`). `buildInsertRequest` and `GroupCommitments` call `memoryMBFromDetails(rec)` which returns an error (no silent fallback) when `Details.MemoryGB` is absent. This ensures high-memory families (e.g. N2-highmem) get the correct MB from the payload rather than the GENERAL_PURPOSE approximation. Regression test: `TestBuildInsertRequest_RefusesMissingMemory`.
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`, `termPlan` returns `(string, error)` using `computepb.Commitment_TWELVE_MONTH.String()` and `computepb.Commitment_THIRTY_SIX_MONTH.String()` as the canonical plan strings (fixed in PR `#1047` commit 8f9a787). It accepts 1yr/1/12mo and 3yr/3/36mo; it errors on empty or unrecognized input (no silent 12-month default). Regression tests: `TestTermPlan_UsesSdkEnumConstants`, `TestTermPlan_RejectsUnknownTerm`, `TestTermPlan_AcceptsAllDocumentedForms`.
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-05-25T21:45:57.471Z
Learning: In this repository (CUDly), `internal/commitmentopts/normalizePayment` is unexported and AWS-spelling-focused (parses "All Upfront"/"ALL_UPFRONT" → canonical AWS tokens). `internal/api/validation.go:purchasePaymentWhitelist` is the purchase-execute boundary. `pkg/common/reservation_name.go:normalizeReservationPayment` produces short-form name segments. These are all distinct concerns from the cross-provider `NormalizePaymentOption` helper in `internal/config/validation.go`.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
Rebased onto origin/main (89658a3) to pick up the #1437 tflint-cache and #1438 trivy-skip fixes. New tip: 2b19f80. Local gates: go build/vet exit=0 (gcp module), gocyclo -over 10 on touched files exit=0, golangci-lint ./... from root exit=0 (no issues), go test ./providers/gcp/services/computeengine/... 59 passed. |
GCP CUDs are billed monthly; there is no upfront billing tier.
The "upfront" literal was leftover from AWS-style modelling.
Peer services (cloudsql, memorystore, cloudstorage) already emit
"monthly". This makes computeengine consistent with them and with
ValidPaymentOptionsByProvider["gcp"] = {"monthly"}, silencing the
NormalizePaymentOption WARN that fired on every healthy GCP rec.
Also adds a PaymentOption assertion to
TestComputeEngineClient_ConvertGCPRecommendation to pin the contract.
…t_test.go Add period to mock type doc comments (godot); fix British-spelling misspellings in test comments (behaviour->behavior, cancelled->canceled, unrecognised->unrecognized); reorder mock struct fields for optimal alignment (govet/fieldalignment); remove unused index field from MockCommitmentsService. String literal in assert.Contains that matches the production error spelling is nolint-suppressed.
…P CUD client US-spelling fix across production error messages and comments in computeengine client; update matching test assertion to "unrecognized". Removes misspell nolint that was suppressing the lint warning.
30 tasks
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
convertGCPRecommendationinproviders/gcp/services/computeengine/client.gowas stampingPaymentOption = "upfront"on every GCP Compute Engine CUD recommendation. GCP CUDs have no upfront billing tier; they are always billed monthly.NormalizePaymentOption(introduced in PR fix(config): accept provider-canonical payment options in plan validator #709) to fire a WARN log on every healthy GCP Compute Engine recommendation, polluting ops dashboards."upfront"to"monthly", matchingValidPaymentOptionsByProvider["gcp"] = {"monthly"}and the existing behaviour of peer services (cloudsql,memorystore,cloudstorage).PaymentOptionassertion toTestComputeEngineClient_ConvertGCPRecommendationto pin the contract.Test plan
go build ./...cleango test github.com/LeanerCloud/CUDly/providers/gcp/... github.com/LeanerCloud/CUDly/internal/api/...passes (pre-existing unrelated failure inTestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancellationconfirmed present on base branch before this change)TestComputeEngineClient_ConvertGCPRecommendationnow assertsPaymentOption == "monthly"cloudsqlandmemorystorealready stamp"monthly"-- no change neededSummary by CodeRabbit