Skip to content

fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 3 commits into from
Jul 17, 2026
Merged

fix(gcp): stamp PaymentOption=monthly on all GCP recs (closes #718) #829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad758b9
fix(gcp/computeengine): stamp PaymentOption="monthly" (closes #718)
cristim May 28, 2026
da5b793
test(gcp/computeengine): fix godot, misspell, fieldalignment in clien…
cristim Jun 19, 2026
0969cd4
fix(lint): correct spelling of "unrecognised" to "unrecognized" in GC…
cristim Jun 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions providers/gcp/services/computeengine/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const maxMachineTypeItems = 20
// - 1-year: "1yr", "1", "12mo"
// - 3-year: "3yr", "3", "36mo"
//
// An empty or unrecognised term returns an error rather than silently defaulting
// 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.
func termPlan(term string) (string, error) {
switch strings.ToLower(strings.TrimSpace(term)) {
Expand All @@ -52,7 +52,7 @@ func termPlan(term string) (string, error) {
case "1yr", "1", "12mo":
return computepb.Commitment_TWELVE_MONTH.String(), nil
default:
return "", fmt.Errorf("termPlan: unrecognised commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term)
return "", fmt.Errorf("termPlan: unrecognized commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term)
}
}

Expand Down Expand Up @@ -390,7 +390,7 @@ type CommitmentRequest struct {
// single commitments.insert call.
// Each recommendation's Count is treated as vCPU count; the memory Amount is
// read from ComputeDetails.MemoryGB (populated by extractMemoryMBFromRecommendation).
// Recommendations with an unrecognised term or missing memory are skipped with a
// Recommendations with an unrecognized term or missing memory are skipped with a
// log warning so a bad rec never contaminates an otherwise valid group.
func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
type key struct{ account, region, term string }
Expand All @@ -407,7 +407,7 @@ func GroupCommitments(recs []common.Recommendation) []CommitmentRequest {
}
plan, err := termPlan(rec.Term)
if err != nil {
log.Printf("GroupCommitments: skipping recommendation with unrecognised term %q: %v", rec.Term, err)
log.Printf("GroupCommitments: skipping recommendation with unrecognized term %q: %v", rec.Term, err)
continue
}
recMemMB, err := memoryMBFromDetails(rec)
Expand Down Expand Up @@ -903,7 +903,7 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo
// BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly
// (issue #1022 C2). Pricing failures are logged but do not discard the recommendation:
// EstimatedSavings from the Recommender payload is the authoritative savings signal.
// Returns nil when the params.Term is unrecognised so the caller skips an
// Returns nil when the params.Term is unrecognized so the caller skips an
// unroutable recommendation rather than queuing a purchase with an invalid plan.
func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) *common.Recommendation {
// GCP CUDs are billed monthly with no upfront option; force "monthly"
Expand All @@ -917,13 +917,13 @@ func (c *ComputeEngineClient) convertGCPRecommendation(ctx context.Context, gcpR
paymentOption := "monthly"

// H-3: propagate params.Term (default "1yr") and validate it so an
// unrecognised term fails loud here rather than reaching buildInsertRequest.
// unrecognized term fails loud here rather than reaching buildInsertRequest.
term := params.Term
if term == "" {
term = "1yr"
}
if _, err := termPlan(term); err != nil {
log.Printf("computeengine: skipping recommendation with unrecognised term %q: %v", term, err)
log.Printf("computeengine: skipping recommendation with unrecognized term %q: %v", term, err)
return nil
}

Expand Down
51 changes: 26 additions & 25 deletions providers/gcp/services/computeengine/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@ import (
"github.com/LeanerCloud/CUDly/pkg/scorer"
)

// MockCommitmentsService mocks the CommitmentsService interface
// MockCommitmentsService mocks the CommitmentsService interface.
type MockCommitmentsService struct {
commitments []*computepb.Commitment
operation *MockOperation
listErr error
insertErr error
index int
lastInsertReq *computepb.InsertRegionCommitmentRequest // captured for assertions
lastInsertReq *computepb.InsertRegionCommitmentRequest // captured for assertions
operation *MockOperation
insertReqs []*computepb.InsertRegionCommitmentRequest // every Insert call (re-drive assertions)
commitments []*computepb.Commitment
}

func (m *MockCommitmentsService) List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator {
Expand All @@ -47,11 +46,11 @@ func (m *MockCommitmentsService) Close() error {
return nil
}

// MockCommitmentsIterator mocks the CommitmentsIterator interface
// MockCommitmentsIterator mocks the CommitmentsIterator interface.
type MockCommitmentsIterator struct {
err error
commitments []*computepb.Commitment
index int
err error
}

func (m *MockCommitmentsIterator) Next() (*computepb.Commitment, error) {
Expand All @@ -66,7 +65,7 @@ func (m *MockCommitmentsIterator) Next() (*computepb.Commitment, error) {
return c, nil
}

// MockOperation mocks the CommitmentsOperation interface
// MockOperation mocks the CommitmentsOperation interface.
type MockOperation struct {
err error
}
Expand All @@ -75,10 +74,10 @@ func (m *MockOperation) Wait(ctx context.Context, opts ...gax.CallOption) error
return m.err
}

// MockMachineTypesService mocks the MachineTypesService interface
// MockMachineTypesService mocks the MachineTypesService interface.
type MockMachineTypesService struct {
machineTypes []*computepb.MachineType
err error
machineTypes []*computepb.MachineType
}

func (m *MockMachineTypesService) List(ctx context.Context, req *computepb.ListMachineTypesRequest) MachineTypesIterator {
Expand All @@ -89,11 +88,11 @@ func (m *MockMachineTypesService) Close() error {
return nil
}

// MockMachineTypesIterator mocks the MachineTypesIterator interface
// MockMachineTypesIterator mocks the MachineTypesIterator interface.
type MockMachineTypesIterator struct {
err error
machineTypes []*computepb.MachineType
index int
err error
}

func (m *MockMachineTypesIterator) Next() (*computepb.MachineType, error) {
Expand All @@ -108,7 +107,7 @@ func (m *MockMachineTypesIterator) Next() (*computepb.MachineType, error) {
return mt, nil
}

// MockBillingService mocks the BillingService interface
// MockBillingService mocks the BillingService interface.
type MockBillingService struct {
skus *cloudbilling.ListSkusResponse
err error
Expand All @@ -121,11 +120,11 @@ func (m *MockBillingService) ListSKUs(serviceID string) (*cloudbilling.ListSkusR
return m.skus, nil
}

// MockRecommenderIterator mocks the RecommenderIterator interface
// MockRecommenderIterator mocks the RecommenderIterator interface.
type MockRecommenderIterator struct {
err error
recommendations []*recommenderpb.Recommendation
index int
err error
}

func (m *MockRecommenderIterator) Next() (*recommenderpb.Recommendation, error) {
Expand All @@ -140,7 +139,7 @@ func (m *MockRecommenderIterator) Next() (*recommenderpb.Recommendation, error)
return rec, nil
}

// MockRecommenderClient mocks the RecommenderClient interface
// MockRecommenderClient mocks the RecommenderClient interface.
type MockRecommenderClient struct {
iterator RecommenderIterator
closed bool
Expand Down Expand Up @@ -577,7 +576,7 @@ func TestComputeEngineClient_PurchaseCommitment_IdempotentReDrive(t *testing.T)

// TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID confirms the
// CLI path (no owning execution, empty token) keeps its prior non-idempotent
// behaviour: no RequestId is set and the name is the timestamp-based fallback.
// behavior: no RequestId is set and the name is the timestamp-based fallback.
func TestComputeEngineClient_PurchaseCommitment_EmptyTokenNoRequestID(t *testing.T) {
ctx := context.Background()
client, _ := NewClient(ctx, "test-project", "us-central1")
Expand Down Expand Up @@ -900,6 +899,8 @@ func TestComputeEngineClient_ConvertGCPRecommendation(t *testing.T) {
assert.Equal(t, "us-central1", rec.Region)
assert.Equal(t, "n1-standard-4", rec.ResourceType)
assert.Equal(t, 50.5, rec.EstimatedSavings)
assert.Equal(t, "monthly", rec.PaymentOption,
"GCP CUDs are billed monthly; PaymentOption must match ValidPaymentOptionsByProvider[\"gcp\"]")
}

// infiniteRecommenderIterator never signals iterator.Done, used to exercise
Expand All @@ -919,7 +920,7 @@ func (c *infiniteRecommenderClient) ListRecommendations(_ context.Context, _ *re
func (c *infiniteRecommenderClient) Close() error { return nil }

// TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError asserts
// that a cancelled context is treated as a terminal stop and returns an error
// that a canceled context is treated as a terminal stop and returns an error
// rather than silently producing a partial result set
// (feedback_ctx_cancel_terminal).
func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing.T) {
Expand All @@ -931,7 +932,7 @@ func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing
client.SetRecommenderClient(&infiniteRecommenderClient{})

_, err = client.GetRecommendations(ctx, common.RecommendationParams{})
require.Error(t, err, "cancelled context must surface an error, not a partial result set")
require.Error(t, err, "canceled context must surface an error, not a partial result set")
}

// TestComputeEngineClient_GetRecommendations_PageCapFires asserts that the
Expand Down Expand Up @@ -1425,16 +1426,16 @@ func TestTermPlan_UsesSdkEnumConstants(t *testing.T) {

// TestTermPlan_RejectsUnknownTerm is the regression test for the "fail loud" policy:
// termPlan must return an error rather than silently defaulting to 12 months when
// given an unrecognised or empty term. A silent mis-default can purchase the wrong
// given an unrecognized or empty term. A silent mis-default can purchase the wrong
// duration and waste money.
//
// This test fails on pre-fix code that silently returned TWELVE_MONTH for any
// unrecognised input.
// unrecognized input.
func TestTermPlan_RejectsUnknownTerm(t *testing.T) {
for _, badTerm := range []string{"", "2yr", "invalid", "24mo", "forever"} {
_, err := termPlan(badTerm)
require.Error(t, err, "termPlan must reject unrecognised term %q (no silent 12-month default)", badTerm)
assert.Contains(t, err.Error(), "unrecognised commitment term",
require.Error(t, err, "termPlan must reject unrecognized term %q (no silent 12-month default)", badTerm)
assert.Contains(t, err.Error(), "unrecognized commitment term",
"error for term %q must identify the bad input", badTerm)
}
}
Expand Down Expand Up @@ -1487,7 +1488,7 @@ func TestConvertGCPRecommendation_PropagatesParamsTerm(t *testing.T) {
}

// TestConvertGCPRecommendation_RejectsUnknownTerm asserts that convertGCPRecommendation
// returns nil when given an unrecognised params.Term (e.g. "5yr"). An unroutable
// returns nil when given an unrecognized params.Term (e.g. "5yr"). An unroutable
// recommendation must be dropped before it can reach buildInsertRequest and attempt
// to insert a commitment with an invalid plan.
//
Expand All @@ -1500,7 +1501,7 @@ func TestConvertGCPRecommendation_RejectsUnknownTerm(t *testing.T) {

rec := client.convertGCPRecommendation(ctx, gcpRec, common.RecommendationParams{Term: "5yr"})
assert.Nil(t, rec,
"convertGCPRecommendation must return nil for unrecognised term (not silently default to 12 months)")
"convertGCPRecommendation must return nil for unrecognized term (not silently default to 12 months)")
}

// TestGetRecommendations_FiltersNonActiveStates is a regression test for H-1
Expand Down
Loading