Repository navigation
fix(gcp): fold FOLD-1047 code quality / correctness findings into fix/gcp-recs-count-pricing #1062
Description
Problem
This issue tracks the FOLD-1047 findings from the remaining-findings plan (docs/code-review/16-remaining-findings-plan.md). These are medium/low/nit-severity findings whose files are already modified by PR #1047.
Findings
| ID | File:Line | Sev | Fix |
|---|---|---|---|
| 10-M2 | computeengine/client.go:350-353,508-511,574-577 + cloudsql/memorystore/cloudstorage | Med | Add termPlan(term) helper + named memMBPerVCPU const; dedup the 4096/term mapping |
| 10-M3 | computeengine/client.go:377-384 | Med | Typed retry classification (errors.As *googleapi.Error{Code:429} + status.FromError codes.ResourceExhausted) instead of substring match |
| 10-M5 | gcp/{computeengine:804,cloudsql,memorystore,cloudstorage} | Med | Derive PaymentOption from params, default to "monthly" (GCP CUDs have no upfront option) -- supersedes #829/#718 which also stamp monthly |
| 10-M6 | gcp/{computeengine:71,cloudsql:46,memorystore:57,cloudstorage:56},provider.go:95 | Med | Remove struct-stored ctx field (containedctx lint); pass ctx per call |
| 10-M7 | gcp/provider.go:336-344 | Med | GetAccounts: return empty on zero ACTIVE projects instead of synthesizing an account |
| 10-L1 | computeengine/client.go:293-296 | Low | Fix no-op commitmentType branch (both arms assign CommitmentCUD) |
| 10-L2 | cloudstorage/client.go:228-242 | Low | GetExistingCommitments should return empty (GCS has no commitment API), not every regional bucket |
| 10-L3 | cloudsql/client.go:210-231 | Low | Don't treat legacy PricingPlan=="PACKAGE" as a commitment |
| 10-L4 | memorystore/client.go:122-124 | Low | Wrap in realRecommenderIterator like the other 3 clients for diffability |
| 10-N1 | computeengine/client.go:888-894 | Nit | Single common.Ptr[T] instead of per-package stringPtr/int64Ptr (deferred: common.Ptr[T] does not yet exist) |
| 10-N3 | recommendations.go:228 | Nit | Rename local var shadowing the imported provider package |
Also fixes CI (cyclomatic complexity > 10 in convertGCPRecommendation and extractVCPUCountFromRecommendation).
Fix
Additional commits on PR #1047 (fix/gcp-recs-count-pricing).
Notes
- 10-M1 (dead GroupCommitments/CommitmentRequest/ResourceCommitment): deferred pending owner confirmation (no-delete rule)
- 10-N1 deferred until common.Ptr[T] is introduced (likely by refactor(arch): provider-module fragmentation forces copy-paste (fan-out, env reader, savings formula); + pkg/errors orphan, logging split #1035 refactor)
- 10-N2 (cache SDK clients): skip, currently fine for Lambda as noted
- 10-M5 supersedes the monthly-stamp in PR fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829 (which stamps monthly in the purchase body but not in the default converter) -- see PR fix(gcp): wire Count/pricing into GCP converters; refuse fabricated prices #1047 body for detail
Activity
fix(gcp/computeengine): default PaymentOption to "monthly" (10-M5)
fix(gcp): FOLD-1047 correctness findings (10-M2,M3,M7,L1,L2,L3,L4,N3)
fix(gcp): wire Count/pricing into GCP converters; refuse fabricated p…