refactor(gcp): one strict commitment term parser for every GCP service

CHANGELOG.md

@@ -140,6 +140,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Changed
 
+- **GCP: one strict commitment term parser** (#15). Compute Engine now uses
+  `skumatch.TermYears` like Cloud SQL, Memorystore and Cloud Storage; it accepts
+  `1yr`/`1`/`12mo`/`3yr`/`3`/`36mo` case-insensitively. An unrecognized term
+  now errors in `GetOfferingDetails` and skips pricing in `enrichRecWithPricing`
+  instead of being priced as one year.
 - **`aws/recommendations`:** the four Cost Explorer coverage and utilization page fetchers share one generic `fetchWithRetry` helper (rate limiter plus concurrency cap); behaviour and error messages are unchanged (#120).
 - **`aws/rds`:** the RDS purchase path uses the shared `purchaseguard` idempotency guard and AlreadyExists recovery, like ElastiCache. Behavior for valid purchases is unchanged (#10).
 - **`aws/memorydb`:** the MemoryDB purchase path uses the shared `purchaseguard` idempotency guard and AlreadyExists recovery, like ElastiCache. Behavior for valid purchases is unchanged; only the lookup-failure error wording changes to "before MemoryDB reservation purchase" (#10).

providers/gcp/internal/skumatch/term.go

@@ -1,16 +1,20 @@
 package skumatch
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+)
 
-// TermYears parses a recommendation term label into commitment years. It
-// accepts "1yr", "1" and "" (an unset term has always meant one year) for one
-// year and "3yr", "3" for three. Anything else is an error: guessing a term
+// TermYears parses a recommendation term label into commitment years. It is
+// case-insensitive, ignores surrounding whitespace, and accepts "1yr", "1",
+// "12mo" and "" (an unset term has always meant one year) for one year and
+// "3yr", "3", "36mo" for three. Anything else is an error: guessing a term
 // would price the wrong commitment.
 func TermYears(term string) (int, error) {
-	switch term {
-	case "", "1yr", "1":
+	switch strings.ToLower(strings.TrimSpace(term)) {
+	case "", "1yr", "1", "12mo":
 		return 1, nil
-	case "3yr", "3":
+	case "3yr", "3", "36mo":
 		return 3, nil
 	}
 	return 0, fmt.Errorf("unsupported commitment term %q, want 1yr or 3yr", term)

providers/gcp/internal/skumatch/term_test.go

@@ -8,12 +8,16 @@ import (
 )
 
 func TestTermYears(t *testing.T) {
-	for term, want := range map[string]int{"": 1, "1yr": 1, "1": 1, "3yr": 3, "3": 3} {
+	accepted := map[string]int{
+		"": 1, "1yr": 1, "1": 1, "12mo": 1, "1YR": 1, "1Yr": 1, " 1yr ": 1, "12MO": 1, " 12mo": 1,
+		"3yr": 3, "3": 3, "36mo": 3, "3YR": 3, " 3yr": 3, "36MO": 3, " 36mo ": 3, " 3 ": 3,
+	}
+	for term, want := range accepted {
 		got, err := TermYears(term)
 		require.NoError(t, err, term)
 		assert.Equal(t, want, got, term)
 	}
-	for _, term := range []string{"2yr", "2", "5yr", "3yrs", "1Yr", " 3yr", "monthly"} {
+	for _, term := range []string{"2yr", "2", "5yr", "3yrs", "24mo", "P1Y", "abc", "monthly"} {
 		_, err := TermYears(term)
 		require.Error(t, err, term)
 		assert.Contains(t, err.Error(), "unsupported commitment term")

providers/gcp/services/computeengine/client.go

@@ -31,6 +31,7 @@ import (
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/retry"
 	"github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist"
+	"github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch"
 )
 
 // maxRecsPages caps GCP Recommender API iteration to avoid burning a Lambda
@@ -44,23 +45,21 @@ const maxCommitmentsPages = 50
 const maxMachineTypeItems = 20
 
 // termPlan converts a commitment term string to the canonical GCP Compute API
-// commitment plan value derived from the SDK enum constants.
-//
-// Accepted forms:
-//   - 1-year: "1yr", "1", "12mo"
-//   - 3-year: "3yr", "3", "36mo"
-//
-// An empty or unrecognized term returns an error rather than silently defaulting
-// to 12 months; a silent mis-default can purchase the wrong term and waste money.
+// commitment plan value derived from the SDK enum constants. Term parsing is
+// delegated to skumatch.TermYears; unlike it, an empty term is an error here
+// because a purchase must name its plan explicitly.
 func termPlan(term string) (string, error) {
-	switch strings.ToLower(strings.TrimSpace(term)) {
-	case "3yr", "3", "36mo":
+	if strings.TrimSpace(term) == "" {
+		return "", fmt.Errorf("termPlan: unrecognized commitment term %q (empty)", term)
+	}
+	years, err := skumatch.TermYears(term)
+	if err != nil {
+		return "", fmt.Errorf("termPlan: unrecognized commitment term %q: %w", term, err)
+	}
+	if years == 3 {
 		return computepb.Commitment_THIRTY_SIX_MONTH.String(), nil
-	case "1yr", "1", "12mo":
-		return computepb.Commitment_TWELVE_MONTH.String(), nil
-	default:
-		return "", fmt.Errorf("termPlan: unrecognized commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term)
 	}
+	return computepb.Commitment_TWELVE_MONTH.String(), nil
 }
 
 // machineFamilyCommitmentType maps a GCP machine series (the segment of a
@@ -202,17 +201,6 @@ func commitmentTypeForMachineType(machineType string) (computepb.Commitment_Type
 	return commitType, nil
 }
 
-// termYearsFromTerm returns the commitment length in years for a term label,
-// defaulting to 1 for any 1-year or unrecognized value and 3 for 3-year labels.
-func termYearsFromTerm(term string) int {
-	switch strings.ToLower(strings.TrimSpace(term)) {
-	case "3yr", "3", "36mo":
-		return 3
-	default:
-		return 1
-	}
-}
-
 // CommitmentsService interface for commitments operations (enables mocking).
 type CommitmentsService interface {
 	List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator
@@ -897,9 +885,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation
 
 // GetOfferingDetails retrieves CUD offering details from GCP Billing API.
 func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) {
-	termYears := 1
-	if rec.Term == "3yr" || rec.Term == "3" {
-		termYears = 3
+	termYears, err := skumatch.TermYears(rec.Term)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get pricing: %w", err)
 	}
 
 	res, err := resourcesFromRecommendation(rec)
@@ -1079,7 +1067,11 @@ func (c *Client) enrichRecWithPricing(ctx context.Context, rec *common.Recommend
 	if rec.ResourceType == "" {
 		return
 	}
-	termYears := termYearsFromTerm(rec.Term)
+	termYears, err := skumatch.TermYears(rec.Term)
+	if err != nil {
+		log.Printf("computeengine: pricing unavailable for %s in %s: %v", rec.ResourceType, c.region, err)
+		return
+	}
 	res, err := resourcesFromRecommendation(*rec)
 	if err != nil {
 		log.Printf("computeengine: pricing unavailable for %s in %s: %v", rec.ResourceType, c.region, err)

providers/gcp/services/computeengine/client_test.go

@@ -1035,6 +1035,38 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) {
 	assert.InDelta(t, details.TotalCost/12, details.RecurringCost, 1e-6)
 }
 
+func TestComputeEngineClient_GetOfferingDetails_Terms(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	client, _ := NewClient("test-project", "us-central1")
+	client.SetBillingService(mockBillingWithCommitment())
+	rec := common.Recommendation{ResourceType: "n1-standard-4", Count: 4, Details: common.ComputeDetails{MemoryGB: 15}}
+
+	rec.Term = "1yr"
+	one, err := client.GetOfferingDetails(ctx, rec)
+	require.NoError(t, err)
+	for _, term := range []string{"12mo", "12MO", "1"} {
+		rec.Term = term
+		got, err := client.GetOfferingDetails(ctx, rec)
+		require.NoError(t, err, term)
+		assert.InDelta(t, one.TotalCost, got.TotalCost, 1e-6, term)
+	}
+	// The fixture has no 3-year SKUs, so a recognized 3-year label fails on
+	// missing pricing, not on the term (3-year pricing is covered by the enrich test).
+	for _, term := range []string{"36mo", "36MO", " 3 ", "3yr"} {
+		rec.Term = term
+		_, err := client.GetOfferingDetails(ctx, rec)
+		require.Error(t, err, term)
+		assert.NotContains(t, err.Error(), "unsupported commitment term", term)
+	}
+	for _, term := range []string{"2yr", "24mo", "P1Y"} {
+		rec.Term = term
+		_, err := client.GetOfferingDetails(ctx, rec)
+		require.Error(t, err, term)
+		assert.Contains(t, err.Error(), "unsupported commitment term", term)
+	}
+}
+
 func TestComputeEngineClient_GetOfferingDetails_NoPricing(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()

providers/gcp/services/computeengine/plan_test.go

@@ -10,6 +10,7 @@ import (
 	"google.golang.org/protobuf/types/known/structpb"
 
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch"
 )
 
 func planClient(t *testing.T, recs ...*recommenderpb.Recommendation) (*Client, *MockCommitmentsService) {
@@ -142,7 +143,9 @@ func TestExplicitTermIsCanonicalized(t *testing.T) {
 			require.NoError(t, err)
 			require.Len(t, recs, 1)
 			assert.Equal(t, "3yr", recs[0].Term, "raw %q plan %q", raw, plan)
-			assert.Equal(t, 3, termYearsFromTerm(recs[0].Term))
+			years, yErr := skumatch.TermYears(recs[0].Term)
+			require.NoError(t, yErr)
+			assert.Equal(t, 3, years)
 		}
 	}
 	assert.Equal(t, "3yr", canonicalTerm("36mo"))

providers/gcp/services/computeengine/pricing_test.go

@@ -168,6 +168,33 @@ func TestEnrichRecWithPricing_MonthlyFiguresPerTerm(t *testing.T) {
 	}
 }
 
+func TestEnrichRecWithPricing_TermSpellingsAndUnknown(t *testing.T) {
+	t.Parallel()
+	price := func(term string) *float64 {
+		client, _ := NewClient("p", "us-central1")
+		client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: n2AmericasSKUs()}})
+		rec := newRec("n2-standard-8", 8, 32)
+		rec.Term = term
+		client.enrichRecWithPricing(context.Background(), &rec)
+		return rec.RecurringMonthlyCost
+	}
+	one, three := price("1yr"), price("3yr")
+	require.NotNil(t, one)
+	require.NotNil(t, three)
+	for _, term := range []string{"12mo", "12MO", " 1 "} {
+		got := price(term)
+		require.NotNil(t, got, term)
+		assert.InDelta(t, *one, *got, 1e-9, term)
+	}
+	for _, term := range []string{"36mo", "36MO", " 3 "} {
+		got := price(term)
+		require.NotNil(t, got, term)
+		assert.InDelta(t, *three, *got, 1e-9, term)
+	}
+	assert.Nil(t, price("2yr"), "unknown term must not be priced as 1yr")
+	assert.Nil(t, price("24mo"), "unknown term must not be priced as 1yr")
+}
+
 func TestEnrichRecWithPricing_InvalidCurrency(t *testing.T) {
 	t.Parallel()
 	for _, currency := range []string{"", "EUR"} {