Skip to content

refactor(gcp): consolidate duplicated termYearsFromLabel into shared helper and add 36mo support #15

Description

@cristim

Context

PR LeanerCloud/cloud-commitments-cli#811 introduced termYearsFromLabel independently in three packages (cloudsql, cloudstorage, memorystore) with identical signatures and bodies. The computeengine package has a functionally equivalent termYearsFromTerm that handles one extra variant: "36mo" -> 3.

Problem

  • termYearsFromLabel is copy-pasted across providers/gcp/services/cloudsql/client.go, cloudstorage/client.go, and memorystore/client.go (violates DRY; maintenance hazard).
  • termYearsFromLabel does not handle "36mo", so a caller passing "36mo" for a Cloud SQL / Memorystore / Cloud Storage CUD gets 1 year instead of 3 (wrong RecurringMonthlyCost).
  • GetOfferingDetails in cloudsql and memorystore also has the old inline if rec.Term == "3yr" || rec.Term == "3" pattern instead of using the new helper.

Proposed fix

  1. Add a termYears(term string) int function to providers/gcp/services/common (or a new providers/gcp/internal/gcputil package) that handles all known forms: "1yr", "1", "12mo", "3yr", "3", "36mo".
  2. Replace the three per-package copies and the two GetOfferingDetails inline branches with calls to the shared helper.
  3. Add a table-driven test for all six input forms.

This is a pure refactor; no behavior change for the current production paths (which only use "1yr" or "3yr").

Findings from the 2026-09-02 codebase audit

Added by an automated audit of 3c0f8ac94048a2c36fce5ccddee54e6c4849a5cd (tip of origin/main). Each item below was reported by one reviewer and independently confirmed by a second that did not write it. Full report: docs/audits/codebase-audit-2026-09-02.md.

A08b-034 (medium)

Worth folding into the consolidation: the computeengine package parses the same rec.Term string with two opposite policies. termPlan refuses an unrecognised term with an explicit error and a doc comment stating that 'a silent mis-default can purchase the wrong term and waste money' (providers/gcp/services/computeengine/client.go:46-56), while termYearsFromTerm silently returns 1 for anything it does not recognise (:200-207). The lenient one feeds the pricing lookup at :1157 and RecurringMonthlyCost at :1139; the strict one runs inside GroupCommitments at :568 and on the purchase path at :773. A rec carrying "P3Y" therefore displays one-year economics right up until the purchase fails at termPlan. Having the shared helper return (int, error) and sharing termPlan's switch would resolve both this and the copy-paste. Audit finding A08b-034.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions