From 0ba381bb6a8707e950d0937f7aab5973da626384 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 27 Oct 2026 07:22:23 +0100 Subject: [PATCH 1/3] refactor(gcp): share term price scaling across Cloud SQL, Memorystore and Cloud Storage Move the price guards and per-hour to term-total scaling into skumatch.ScaleToTerm; error strings are unchanged. Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/internal/skumatch/savings.go | 34 +++++++++++++++++++ .../gcp/internal/skumatch/savings_test.go | 34 +++++++++++++++++++ providers/gcp/services/cloudsql/client.go | 23 ++++--------- providers/gcp/services/cloudstorage/client.go | 20 ++++------- providers/gcp/services/memorystore/client.go | 23 ++++--------- 5 files changed, 89 insertions(+), 45 deletions(-) diff --git a/providers/gcp/internal/skumatch/savings.go b/providers/gcp/internal/skumatch/savings.go index efcc4add..73e9a5c4 100644 --- a/providers/gcp/internal/skumatch/savings.go +++ b/providers/gcp/internal/skumatch/savings.go @@ -1,5 +1,11 @@ package skumatch +import ( + "fmt" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + // SavingsPercent returns the percentage a commitment saves against on-demand // over a term. commitmentTotal must already be a term total (per-hour price * // hoursInTerm), on the same basis as onDemandPrice * hoursInTerm. A zero @@ -9,3 +15,31 @@ func SavingsPercent(onDemandPrice, hoursInTerm, commitmentTotal float64) float64 onDemandTotal := onDemandPrice * hoursInTerm return ((onDemandTotal - commitmentTotal) / onDemandTotal) * 100 } + +// TermPrices holds per-hour catalog prices scaled to a commitment term. +type TermPrices struct { + HourlyRate float64 + CommitmentPrice float64 + OnDemandPrice float64 + SavingsPercentage float64 +} + +// ScaleToTerm rejects missing catalog prices and scales both per-hour prices +// to term totals, so the savings percentage compares like with like (a +// per-hour commitment against a term-total on-demand would read near 100%). +func ScaleToTerm(what, region string, onDemandHourly, commitmentHourly float64, termYears int) (TermPrices, error) { + if onDemandHourly <= 0 { + return TermPrices{}, fmt.Errorf("no pricing found for %s", what) + } + if commitmentHourly <= 0 { + return TermPrices{}, fmt.Errorf("no commitment pricing found for %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", what, region) + } + hoursInTerm := common.HoursPerYear * float64(termYears) + commitmentTotal := commitmentHourly * hoursInTerm + return TermPrices{ + HourlyRate: commitmentHourly, + CommitmentPrice: commitmentTotal, + OnDemandPrice: onDemandHourly * hoursInTerm, + SavingsPercentage: SavingsPercent(onDemandHourly, hoursInTerm, commitmentTotal), + }, nil +} diff --git a/providers/gcp/internal/skumatch/savings_test.go b/providers/gcp/internal/skumatch/savings_test.go index 07500bee..3da41b67 100644 --- a/providers/gcp/internal/skumatch/savings_test.go +++ b/providers/gcp/internal/skumatch/savings_test.go @@ -69,3 +69,37 @@ func TestMatchesTier(t *testing.T) { }) } } + +func TestScaleToTerm(t *testing.T) { + t.Parallel() + tests := []struct { + name string + onDemand float64 + commitment float64 + years int + want TermPrices + wantErrText string + }{ + {name: "zero on-demand", onDemand: 0, commitment: 0.05, years: 1, wantErrText: "no pricing found for Thing x"}, + {name: "negative on-demand", onDemand: -1, commitment: 0.05, years: 1, wantErrText: "no pricing found for Thing x"}, + {name: "zero commitment", onDemand: 0.1, commitment: 0, years: 1, wantErrText: "no commitment pricing found for Thing x in region r1: catalog has no CUD SKU; cannot compute savings percentage"}, + {name: "negative commitment", onDemand: 0.1, commitment: -0.05, years: 3, wantErrText: "no commitment pricing found for Thing x in region r1: catalog has no CUD SKU; cannot compute savings percentage"}, + {name: "1 year", onDemand: 0.1, commitment: 0.05, years: 1, want: TermPrices{HourlyRate: 0.05, CommitmentPrice: 0.05 * 8760, OnDemandPrice: 0.1 * 8760, SavingsPercentage: 50}}, + {name: "3 year", onDemand: 0.2, commitment: 0.1, years: 3, want: TermPrices{HourlyRate: 0.1, CommitmentPrice: 0.1 * 26280, OnDemandPrice: 0.2 * 26280, SavingsPercentage: 50}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := ScaleToTerm("Thing x", "r1", tt.onDemand, tt.commitment, tt.years) + if tt.wantErrText != "" { + assert.EqualError(t, err, tt.wantErrText) + return + } + assert.NoError(t, err) + assert.InDelta(t, tt.want.HourlyRate, got.HourlyRate, 1e-9) + assert.InDelta(t, tt.want.CommitmentPrice, got.CommitmentPrice, 1e-6) + assert.InDelta(t, tt.want.OnDemandPrice, got.OnDemandPrice, 1e-6) + assert.InDelta(t, tt.want.SavingsPercentage, got.SavingsPercentage, 1e-9) + }) + } +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 48042b07..2bd1956a 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -266,26 +266,17 @@ func (c *Client) getSQLPricing(ctx context.Context, tier, region string, termYea if err != nil { return nil, err } - if onDemandPrice <= 0 { - return nil, fmt.Errorf("no pricing found for Cloud SQL tier %s", tier) - } - if commitmentPrice <= 0 { - return nil, fmt.Errorf("no commitment pricing found for Cloud SQL tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) + prices, err := skumatch.ScaleToTerm("Cloud SQL tier "+tier, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * float64(termYears) - // Scale the per-hour commitment price to a term total so it is on the - // same basis as onDemandPrice * hoursInTerm. Without this, the savings - // percentage would be nearly 100% (per-hour price vs term total). - commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &SQLPricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 93cea1f5..e06731f2 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -226,23 +226,17 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str if err != nil { return nil, err } - if onDemandPrice <= 0 { - return nil, fmt.Errorf("no pricing found for Cloud Storage class %s", storageClass) - } - if commitmentPrice <= 0 { - return nil, fmt.Errorf("no commitment pricing found for Cloud Storage class %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", storageClass, region) + prices, err := skumatch.ScaleToTerm("Cloud Storage class "+storageClass, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * float64(termYears) - commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &StoragePricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index 7424c241..df2dc864 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -229,26 +229,17 @@ func (c *Client) getRedisPricing(ctx context.Context, tier, region string, termY if err != nil { return nil, err } - if onDemandPrice <= 0 { - return nil, fmt.Errorf("no pricing found for Memorystore tier %s", tier) - } - if commitmentPrice <= 0 { - return nil, fmt.Errorf("no commitment pricing found for Memorystore tier %s in region %s: catalog has no CUD SKU; cannot compute savings percentage", tier, region) + prices, err := skumatch.ScaleToTerm("Memorystore tier "+tier, region, onDemandPrice, commitmentPrice, termYears) + if err != nil { + return nil, err } - hoursInTerm := common.HoursPerYear * float64(termYears) - // Scale the per-hour commitment price to a term total so it is on the - // same basis as onDemandPrice * hoursInTerm. Without this, the savings - // percentage would be nearly 100% (per-hour price vs term total). - commitmentPriceTerm := commitmentPrice * hoursInTerm - savingsPercentage := skumatch.SavingsPercent(onDemandPrice, hoursInTerm, commitmentPriceTerm) - return &RedisPricing{ - HourlyRate: commitmentPrice, - CommitmentPrice: commitmentPriceTerm, - OnDemandPrice: onDemandPrice * hoursInTerm, + HourlyRate: prices.HourlyRate, + CommitmentPrice: prices.CommitmentPrice, + OnDemandPrice: prices.OnDemandPrice, Currency: currency, - SavingsPercentage: savingsPercentage, + SavingsPercentage: prices.SavingsPercentage, }, nil } From 5c8f194fc4549955527d4fce4bc559487edbca5b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 27 Oct 2026 07:26:27 +0100 Subject: [PATCH 2/3] perf(gcp): list the Cloud Storage SKU catalog once per client Cloud Storage was the only GCP service listing its catalog on every pricing lookup; cache it per client like Cloud SQL and Memorystore. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 2 + providers/gcp/services/cloudstorage/client.go | 9 +++- .../services/cloudstorage/pagination_test.go | 51 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 providers/gcp/services/cloudstorage/pagination_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cb5536e..82724b42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- GCP Cloud Storage pricing lists the SKU catalog once per client, like Cloud + SQL and Memorystore. - Azure multi-subscription collection keeps skipped recommendations when a subscription also fails. They are returned by the new `PartialSubscriptionFailureError.Skips()` (a `*PartialResultError`, or nil) diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index e06731f2..fa14f411 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -54,6 +54,7 @@ type RecommenderIterator interface { // Client handles GCP Cloud Storage commitments. type Client struct { + skus skulist.Cache ctx context.Context projectID string region string @@ -217,7 +218,7 @@ func (c *Client) getStoragePricing(ctx context.Context, storageClass, region str return nil, err } - skus, err := svc.ListSKUs(cloudStorageServiceID) + skus, err := c.listSKUs(svc) if err != nil { return nil, fmt.Errorf("failed to list SKUs: %w", err) } @@ -276,5 +277,11 @@ func skuMatchesStorageClass(sku *cloudbilling.Sku, storageClass, region string) return skumatch.InRegion(sku, region) } +// listSKUs returns the service's SKU catalog, listing it once per client so +// that pricing N recommendations costs one catalog read instead of N. +func (c *Client) listSKUs(svc BillingService) (*cloudbilling.ListSkusResponse, error) { + return c.skus.Get(svc, cloudStorageServiceID) +} + // cloudStorageServiceID is the Cloud Billing service for Cloud Storage. const cloudStorageServiceID = "services/95FF-2EF5-5EA1" diff --git a/providers/gcp/services/cloudstorage/pagination_test.go b/providers/gcp/services/cloudstorage/pagination_test.go new file mode 100644 index 00000000..5f5e6296 --- /dev/null +++ b/providers/gcp/services/cloudstorage/pagination_test.go @@ -0,0 +1,51 @@ +package cloudstorage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pricing N recommendations lists the SKU catalog once. +type countingBilling struct { + BillingService + calls int +} + +func (b *countingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.calls++ + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_CachedPerClient(t *testing.T) { + t.Parallel() + b := &countingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + for i := 0; i < 3; i++ { + _, err := c.listSKUs(b) + require.NoError(t, err) + } + assert.Equal(t, 1, b.calls) +} + +type idRecordingBilling struct { + BillingService + id string +} + +func (b *idRecordingBilling) ListSKUs(id string) (*cloudbilling.ListSkusResponse, error) { + b.id = id + return b.BillingService.ListSKUs(id) +} + +func TestListSKUs_UsesCloudStorageBillingService(t *testing.T) { + t.Parallel() + b := &idRecordingBilling{BillingService: &MockBillingService{skus: &cloudbilling.ListSkusResponse{}}} + c, _ := NewClient(context.Background(), "p", "us-central1") + _, err := c.listSKUs(b) + require.NoError(t, err) + assert.Equal(t, "services/95FF-2EF5-5EA1", b.id) +} From e877154af3f0247b7f85b71c760d9f28e00cd978 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 27 Oct 2026 07:46:54 +0100 Subject: [PATCH 3/3] refactor(gcp): share offering details across the GCP CUD clients Move the payment split and OfferingDetails build repeated in the Cloud SQL, Memorystore, Cloud Storage and Compute Engine clients into skumatch.OfferingDetails; an unknown payment option still pays upfront. Use slices.Contains in the ValidateOffering bodies and pin the service label and region in the ScaleToTerm errors per service. Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/internal/skumatch/savings.go | 28 +++++++++++++ .../gcp/internal/skumatch/savings_test.go | 41 +++++++++++++++++++ providers/gcp/services/cloudsql/client.go | 34 +++------------ .../services/cloudsql/scale_errors_test.go | 40 ++++++++++++++++++ providers/gcp/services/cloudstorage/client.go | 34 +++------------ .../cloudstorage/scale_errors_test.go | 40 ++++++++++++++++++ .../gcp/services/computeengine/client.go | 27 +----------- providers/gcp/services/memorystore/client.go | 34 +++------------ .../services/memorystore/scale_errors_test.go | 40 ++++++++++++++++++ 9 files changed, 206 insertions(+), 112 deletions(-) create mode 100644 providers/gcp/services/cloudsql/scale_errors_test.go create mode 100644 providers/gcp/services/cloudstorage/scale_errors_test.go create mode 100644 providers/gcp/services/memorystore/scale_errors_test.go diff --git a/providers/gcp/internal/skumatch/savings.go b/providers/gcp/internal/skumatch/savings.go index 73e9a5c4..0113c9fb 100644 --- a/providers/gcp/internal/skumatch/savings.go +++ b/providers/gcp/internal/skumatch/savings.go @@ -43,3 +43,31 @@ func ScaleToTerm(what, region string, onDemandHourly, commitmentHourly float64, SavingsPercentage: SavingsPercent(onDemandHourly, hoursInTerm, commitmentTotal), }, nil } + +// OfferingDetails builds the offering for a commitment total, paying it up +// front or spreading it monthly by payment option. An unknown option pays the +// total up front. +func OfferingDetails(rec common.Recommendation, offeringID string, termYears int, commitmentTotal, hourlyRate float64, currency string) *common.OfferingDetails { + var upfrontCost, recurringCost float64 + + switch rec.PaymentOption { + case "all-upfront", "upfront": + upfrontCost = commitmentTotal + case "monthly", "no-upfront": + recurringCost = commitmentTotal / (float64(termYears) * 12) + default: + upfrontCost = commitmentTotal + } + + return &common.OfferingDetails{ + OfferingID: offeringID, + ResourceType: rec.ResourceType, + Term: rec.Term, + PaymentOption: rec.PaymentOption, + UpfrontCost: upfrontCost, + RecurringCost: recurringCost, + TotalCost: commitmentTotal, + EffectiveHourlyRate: hourlyRate, + Currency: currency, + } +} diff --git a/providers/gcp/internal/skumatch/savings_test.go b/providers/gcp/internal/skumatch/savings_test.go index 3da41b67..800b3811 100644 --- a/providers/gcp/internal/skumatch/savings_test.go +++ b/providers/gcp/internal/skumatch/savings_test.go @@ -1,9 +1,11 @@ package skumatch import ( + "fmt" "math" "testing" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/stretchr/testify/assert" "google.golang.org/api/cloudbilling/v1" ) @@ -103,3 +105,42 @@ func TestScaleToTerm(t *testing.T) { }) } } + +func TestOfferingDetails(t *testing.T) { + t.Parallel() + tests := []struct { + option string + years int + wantUpfront float64 + wantRecurring float64 + }{ + {"all-upfront", 1, 1200, 0}, + {"upfront", 3, 1200, 0}, + {"monthly", 1, 0, 100}, + {"monthly", 3, 0, 1200.0 / 36}, + {"no-upfront", 1, 0, 100}, + {"no-upfront", 3, 0, 1200.0 / 36}, + {"bogus", 1, 1200, 0}, + {"bogus", 3, 1200, 0}, + {"", 1, 1200, 0}, + {"", 3, 1200, 0}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%q/%dyr", tt.option, tt.years), func(t *testing.T) { + t.Parallel() + rec := common.Recommendation{ResourceType: "rt", Term: "term-x", PaymentOption: tt.option} + got := OfferingDetails(rec, "id-1", tt.years, 1200, 0.5, "EUR") + assert.Equal(t, &common.OfferingDetails{ + OfferingID: "id-1", + ResourceType: "rt", + Term: "term-x", + PaymentOption: tt.option, + UpfrontCost: tt.wantUpfront, + RecurringCost: tt.wantRecurring, + TotalCost: 1200, + EffectiveHourlyRate: 0.5, + Currency: "EUR", + }, got) + }) + } +} diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 2bd1956a..4da2ce5d 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -4,6 +4,7 @@ package cloudsql import ( "context" "fmt" + "slices" "strings" "time" @@ -156,10 +157,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid tiers: %w", err) } - for _, tier := range validTiers { - if tier == rec.ResourceType { - return nil - } + if slices.Contains(validTiers, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Cloud SQL tier: %s", rec.ResourceType) @@ -177,31 +176,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-cloudsql-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-cloudsql-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Cloud SQL tiers. diff --git a/providers/gcp/services/cloudsql/scale_errors_test.go b/providers/gcp/services/cloudsql/scale_errors_test.go new file mode 100644 index 00000000..15d8dda4 --- /dev/null +++ b/providers/gcp/services/cloudsql/scale_errors_test.go @@ -0,0 +1,40 @@ +package cloudsql + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetSQLPricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Cloud SQL tier db-n1-standard-1"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Cloud SQL tier db-n1-standard-1 in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := negPriceSkus(tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getSQLPricing(ctx, "db-n1-standard-1", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index fa14f411..5dd46e13 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -4,6 +4,7 @@ package cloudstorage import ( "context" "fmt" + "slices" "strings" "time" @@ -139,10 +140,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid storage classes: %w", err) } - for _, class := range validClasses { - if class == rec.ResourceType { - return nil - } + if slices.Contains(validClasses, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Cloud Storage class: %s", rec.ResourceType) @@ -160,31 +159,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-storage-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-storage-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Cloud Storage classes. diff --git a/providers/gcp/services/cloudstorage/scale_errors_test.go b/providers/gcp/services/cloudstorage/scale_errors_test.go new file mode 100644 index 00000000..f2ea3b18 --- /dev/null +++ b/providers/gcp/services/cloudstorage/scale_errors_test.go @@ -0,0 +1,40 @@ +package cloudstorage + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetStoragePricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Cloud Storage class STANDARD"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Cloud Storage class STANDARD in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := storageMockSkus("STANDARD", tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getStoragePricing(ctx, "STANDARD", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +} diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index dfa459c2..0b846965 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -901,31 +901,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-compute-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-compute-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid machine types from GCP Compute API. diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index df2dc864..969625d2 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -4,6 +4,7 @@ package memorystore import ( "context" "fmt" + "slices" "time" "cloud.google.com/go/recommender/apiv1/recommenderpb" @@ -143,10 +144,8 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation return fmt.Errorf("failed to get valid tiers: %w", err) } - for _, tier := range validTiers { - if tier == rec.ResourceType { - return nil - } + if slices.Contains(validTiers, rec.ResourceType) { + return nil } return fmt.Errorf("invalid Memorystore tier: %s", rec.ResourceType) @@ -164,31 +163,8 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati return nil, fmt.Errorf("failed to get pricing: %w", err) } - var upfrontCost, recurringCost float64 - totalCost := pricing.CommitmentPrice - - switch rec.PaymentOption { - case "all-upfront", "upfront": - upfrontCost = totalCost - recurringCost = 0 - case "monthly", "no-upfront": - upfrontCost = 0 - recurringCost = totalCost / (float64(termYears) * 12) - default: - upfrontCost = totalCost - } - - return &common.OfferingDetails{ - OfferingID: fmt.Sprintf("gcp-memorystore-%s-%s-%s", rec.ResourceType, c.region, rec.Term), - ResourceType: rec.ResourceType, - Term: rec.Term, - PaymentOption: rec.PaymentOption, - UpfrontCost: upfrontCost, - RecurringCost: recurringCost, - TotalCost: totalCost, - EffectiveHourlyRate: pricing.HourlyRate, - Currency: pricing.Currency, - }, nil + offeringID := fmt.Sprintf("gcp-memorystore-%s-%s-%s", rec.ResourceType, c.region, rec.Term) + return skumatch.OfferingDetails(rec, offeringID, termYears, pricing.CommitmentPrice, pricing.HourlyRate, pricing.Currency), nil } // GetValidResourceTypes returns valid Memorystore tiers. diff --git a/providers/gcp/services/memorystore/scale_errors_test.go b/providers/gcp/services/memorystore/scale_errors_test.go new file mode 100644 index 00000000..e1ea6ee6 --- /dev/null +++ b/providers/gcp/services/memorystore/scale_errors_test.go @@ -0,0 +1,40 @@ +package memorystore + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/cloudbilling/v1" +) + +// Pins the service label and region that ScaleToTerm puts in its errors. +func TestGetRedisPricing_ScaleErrorsNameLabelAndRegion(t *testing.T) { + t.Parallel() + const region = "europe-west4" + tests := []struct { + name string + onDemand, commit int64 + wantErr string + }{ + {"no on-demand", -50000000, 42000000, "no pricing found for Memorystore tier STANDARD_HA"}, + {"no commitment", 50000000, -42000000, "no commitment pricing found for Memorystore tier STANDARD_HA in region europe-west4: catalog has no CUD SKU; cannot compute savings percentage"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, err := NewClient(ctx, "test-project", region) + require.NoError(t, err) + skus := negPriceSkus(tt.onDemand, tt.commit) + for _, sku := range skus { + sku.ServiceRegions = []string{region} + } + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: skus}}) + + _, err = client.getRedisPricing(ctx, "STANDARD_HA", region, 1) + assert.EqualError(t, err, tt.wantErr) + }) + } +}