From f7cb446c361683187ecbc7a00cad7fbcfc2c1e1b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 06:26:21 -0700 Subject: [PATCH 1/4] test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled ctx; drop em-dashes The ComputeDetails VCPU/MemoryGB implementation landed via the merged duplicate PR #816 (closes #218). This commit grafts the two test-coverage gaps that #816 missed, plus a comment cleanup: - TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but SizeInMiB nil" -- expects VCPU populated, MemGB 0.0. - TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount remains 0 after a pre-canceled ctx returns nil (the stub already tracks the count via atomic; this makes the no-NextPage contract explicit). - Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the client.go instanceTypeLookup comment. parser_services.go em-dashes left for a follow-up (out of scope here). Closes #99 --- providers/aws/recommendations/client.go | 2 +- providers/aws/recommendations/sku.go | 4 ++-- providers/aws/recommendations/sku_test.go | 11 +++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 95102165f..914c7c1c8 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -96,7 +96,7 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) { // instanceTypeLookup returns the cached SKU entry for instanceType. // On the first call the catalogue is built by calling the pager factory. // ok=false when no factory is configured, the catalogue fetch failed, or -// the instance type was not in the catalogue — the caller falls back to +// the instance type was not in the catalogue -- the caller falls back to // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810). func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) { if c.instanceTypePagerFactory == nil { diff --git a/providers/aws/recommendations/sku.go b/providers/aws/recommendations/sku.go index d5f4f7da5..c528c6e65 100644 --- a/providers/aws/recommendations/sku.go +++ b/providers/aws/recommendations/sku.go @@ -70,12 +70,12 @@ func fetchInstanceTypeCatalogue(ctx context.Context, pager InstanceTypePager) ma out := make(map[string]instanceTypeSKUEntry) for pager.HasMorePages() { if err := ctx.Err(); err != nil { - logging.Warnf("aws ec2: instance type catalogue fetch interrupted: %v — Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalogue fetch interrupted: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } page, err := pager.NextPage(ctx) if err != nil { - logging.Warnf("aws ec2: instance type catalogue page fetch failed: %v — Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalogue page fetch failed: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } populateInstanceTypeSKUMap(out, page.InstanceTypes) diff --git a/providers/aws/recommendations/sku_test.go b/providers/aws/recommendations/sku_test.go index 64842842b..e3ea02f89 100644 --- a/providers/aws/recommendations/sku_test.go +++ b/providers/aws/recommendations/sku_test.go @@ -111,6 +111,15 @@ func TestExtractInstanceTypeSKUEntry(t *testing.T) { wantVCPU: 1, wantMemGB: 0.5, }, + { + name: "MemoryInfo present but SizeInMiB nil -- MemGB zero", + info: ec2types.InstanceTypeInfo{ + VCpuInfo: &ec2types.VCpuInfo{DefaultVCpus: aws.Int32(8)}, + MemoryInfo: &ec2types.MemoryInfo{}, + }, + wantVCPU: 8, + wantMemGB: 0.0, + }, } for _, tt := range tests { @@ -157,6 +166,8 @@ func TestFetchInstanceTypeCatalogue_ContextCanceled(t *testing.T) { pager := newStubPager(knownInstanceTypes()...) m := fetchInstanceTypeCatalogue(ctx, pager) assert.Nil(t, m, "catalogue must be nil when ctx is already canceled") + // NextPage must not have been called on a pre-canceled ctx. + assert.Equal(t, int32(0), atomic.LoadInt32(&pager.callCount)) } // TestInstanceTypeLookup_CachedOnce asserts that a single GetRecommendations From 2162cbc067c2fdfac65b59509b700d90a50518fb Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 23:31:48 +0200 Subject: [PATCH 2/4] style(aws/recs): fold golangci-lint fixes into PR-touched files Fix all lint issues in client.go, sku.go, and sku_test.go per the 3b step in the review brief: - misspell: catalogue -> catalog in comments and string literals (function identifiers fetchInstanceTypeCatalogue/skuCatalog untouched) - misspell: Behaviour -> Behavior in GetAllRecommendations comment - godot: add trailing periods to CostExplorerAPI, Client, NewClient, NewClientWithAPI, and GetRecommendations doc comments - govet/fieldalignment: reorder skuCatalog fields (m before once: 24 -> 20 bytes) and serviceResult fields (err before name before recs: 56 -> 40 bytes); Client and serviceResult updated accordingly - gocritic/rangeValCopy: populateInstanceTypeSKUMap now iterates by index to avoid copying the large InstanceTypeInfo struct; change extractInstanceTypeSKUEntry signature to *ec2types.InstanceTypeInfo - gocritic/hugeParam: suppress via //nolint:gocritic on NewClient (aws.Config 696 bytes, SDK value-type convention) and GetRecommendations (RecommendationParams 200 bytes, public API) - gocritic/rangeValCopy: mergeServiceResults range loops converted to index-based to avoid copying the 56-byte serviceResult struct --- providers/aws/recommendations/client.go | 50 ++++++++++++----------- providers/aws/recommendations/sku.go | 21 +++++----- providers/aws/recommendations/sku_test.go | 20 ++++----- 3 files changed, 47 insertions(+), 44 deletions(-) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 914c7c1c8..f5b640934 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -22,7 +22,7 @@ import ( // payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692). const maxRecommendationPages = 20 -// CostExplorerAPI defines the interface for Cost Explorer operations +// CostExplorerAPI defines the interface for Cost Explorer operations. type CostExplorerAPI interface { GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) GetSavingsPlansPurchaseRecommendation(ctx context.Context, params *costexplorer.GetSavingsPlansPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) @@ -32,29 +32,31 @@ type CostExplorerAPI interface { GetSavingsPlansUtilization(ctx context.Context, params *costexplorer.GetSavingsPlansUtilizationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansUtilizationOutput, error) } -// Client wraps the AWS Cost Explorer client for RI recommendations +// Client wraps the AWS Cost Explorer client for RI recommendations. type Client struct { costExplorerClient CostExplorerAPI - region string - rateLimiter *RateLimiter // ec2API is the EC2 client used to build the DescribeInstanceTypes paginator. // Populated by NewClient from aws.Config; nil when created via NewClientWithAPI. ec2API DescribeInstanceTypesAPI + rateLimiter *RateLimiter + // instanceTypePagerFactory creates a new InstanceTypePager on demand. // Set by NewClient to wrap ec2API; overridable via SetInstanceTypePagerFactory // for hermetic tests. When nil, instanceTypeLookup returns (0,0). instanceTypePagerFactory func() InstanceTypePager - // skuCatalog caches the per-instance-type vCPU/memory catalogue, fetched + region string + + // skuCatalog caches the per-instance-type vCPU/memory catalog, fetched // lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes // fan-out per scheduler tick). skuCatalog skuCatalog } -// NewClient creates a new recommendations client -func NewClient(cfg aws.Config) *Client { +// NewClient creates a new recommendations client. +func NewClient(cfg aws.Config) *Client { //nolint:gocritic // aws.Config is the SDK's value-type convention; pointer form is not idiomatic here. // Force Cost Explorer to use us-east-1 with explicit endpoint ceConfig := cfg.Copy() ceConfig.Region = "us-east-1" @@ -74,7 +76,7 @@ func NewClient(cfg aws.Config) *Client { } } -// NewClientWithAPI creates a new recommendations client with a custom Cost Explorer API (for testing) +// NewClientWithAPI creates a new recommendations client with a custom Cost Explorer API (for testing). func NewClientWithAPI(api CostExplorerAPI, region string) *Client { return &Client{ costExplorerClient: api, @@ -86,7 +88,7 @@ func NewClientWithAPI(api CostExplorerAPI, region string) *Client { } // SetInstanceTypePagerFactory injects a pager factory for the instance-type -// SKU catalogue. Must be called before the first GetRecommendations call. +// SKU catalog. Must be called before the first GetRecommendations call. // Intended for tests that need to verify the one-fetch-per-lifetime invariant // without hitting AWS. func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) { @@ -94,9 +96,9 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) { } // instanceTypeLookup returns the cached SKU entry for instanceType. -// On the first call the catalogue is built by calling the pager factory. -// ok=false when no factory is configured, the catalogue fetch failed, or -// the instance type was not in the catalogue -- the caller falls back to +// On the first call the catalog is built by calling the pager factory. +// ok=false when no factory is configured, the catalog fetch failed, or +// the instance type was not in the catalog -- the caller falls back to // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810). func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) { if c.instanceTypePagerFactory == nil { @@ -105,8 +107,8 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory) } -// GetRecommendations fetches Reserved Instance recommendations for any service -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +// GetRecommendations fetches Reserved Instance recommendations for any service. +func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { //nolint:gocritic // RecommendationParams is a value type by convention; pointer form would widen the public API surface // Handle Savings Plans separately — they use a different Cost Explorer API // (GetSavingsPlansPurchaseRecommendation, not GetReservationPurchaseRecommendation). // Match any SP slug — the legacy umbrella plus the four per-plan-type slugs — @@ -299,7 +301,7 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo // the canonical order EC2 → RDS → ElastiCache → OpenSearch → Redshift after // all goroutines finish so order-sensitive consumers stay stable. // -// Behaviour change vs the previous sequential loop: per-service errors are +// Behavior change vs the previous sequential loop: per-service errors are // now logged at WARN via mergeServiceResults — the previous loop swallowed // them silently with a bare `continue`, leaving operators no signal when a // single service was misbehaving. Mirrors the Azure parallelisation in @@ -375,9 +377,9 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda // stays under the gocyclo gate (.golangci.yml min-complexity: 15) after the // post-Wait ctx.Err() block was added. type serviceResult struct { + err error name string recs []common.Recommendation - err error } // mergeServiceResults logs per-service errors at WARN and appends successful @@ -398,20 +400,20 @@ func mergeServiceResults(results ...serviceResult) ([]common.Recommendation, err total := 0 failures := 0 var lastErr error - for _, r := range results { - total += len(r.recs) - if r.err != nil { + for i := range results { + total += len(results[i].recs) + if results[i].err != nil { failures++ - lastErr = r.err + lastErr = results[i].err } } out := make([]common.Recommendation, 0, total) - for _, r := range results { - if r.err != nil { - logging.Warnf("AWS %s recommendations: %v", r.name, r.err) + for i := range results { + if results[i].err != nil { + logging.Warnf("AWS %s recommendations: %v", results[i].name, results[i].err) continue } - out = append(out, r.recs...) + out = append(out, results[i].recs...) } if failures == len(results) && failures > 0 { return nil, fmt.Errorf("all %d AWS recommendation services failed: %w", failures, lastErr) diff --git a/providers/aws/recommendations/sku.go b/providers/aws/recommendations/sku.go index c528c6e65..f73c2e920 100644 --- a/providers/aws/recommendations/sku.go +++ b/providers/aws/recommendations/sku.go @@ -32,17 +32,17 @@ type instanceTypeSKUEntry struct { memoryGB float64 } -// skuCatalog holds a lazily-built per-Client instance-type catalogue. -// The catalogue is fetched ONCE per client lifetime via sync.Once +// skuCatalog holds a lazily-built per-Client instance-type catalog. +// The catalog is fetched ONCE per client lifetime via sync.Once // so a single recommendations refresh issues at most one // DescribeInstanceTypes fan-out regardless of how many EC2 recs are returned. type skuCatalog struct { + m map[string]instanceTypeSKUEntry once sync.Once - m map[string]instanceTypeSKUEntry // nil means fetch failed } -// lookup returns the catalogue entry for instanceType, building the -// catalogue on the first call. ok=false on cache miss or fetch failure; +// lookup returns the catalog entry for instanceType, building the +// catalog on the first call. ok=false on cache miss or fetch failure; // the caller falls back to VCPU=0 / MemoryGB=0 and does NOT fail the // conversion (graceful-degradation contract from Azure PR #810). func (s *skuCatalog) lookup(ctx context.Context, instanceType string, newPager func() InstanceTypePager) (instanceTypeSKUEntry, bool) { @@ -65,17 +65,17 @@ func (s *skuCatalog) lookup(ctx context.Context, instanceType string, newPager f // WARN so operators can detect it. // // Any page-fetch error also returns nil (partial results are discarded so -// callers never see a half-populated catalogue). +// callers never see a half-populated catalog). func fetchInstanceTypeCatalogue(ctx context.Context, pager InstanceTypePager) map[string]instanceTypeSKUEntry { out := make(map[string]instanceTypeSKUEntry) for pager.HasMorePages() { if err := ctx.Err(); err != nil { - logging.Warnf("aws ec2: instance type catalogue fetch interrupted: %v -- Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalog fetch interrupted: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } page, err := pager.NextPage(ctx) if err != nil { - logging.Warnf("aws ec2: instance type catalogue page fetch failed: %v -- Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalog page fetch failed: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } populateInstanceTypeSKUMap(out, page.InstanceTypes) @@ -86,7 +86,8 @@ func fetchInstanceTypeCatalogue(ctx context.Context, pager InstanceTypePager) ma // populateInstanceTypeSKUMap writes one instanceTypeSKUEntry per item in // instanceTypes into out. First-write-wins on duplicate names. func populateInstanceTypeSKUMap(out map[string]instanceTypeSKUEntry, instanceTypes []ec2types.InstanceTypeInfo) { - for _, info := range instanceTypes { + for i := range instanceTypes { + info := &instanceTypes[i] name := string(info.InstanceType) if name == "" { continue @@ -101,7 +102,7 @@ func populateInstanceTypeSKUMap(out map[string]instanceTypeSKUEntry, instanceTyp // extractInstanceTypeSKUEntry reads the vCPU count and memory size from // InstanceTypeInfo. Returns (0, 0) when either field is absent or nil; // callers treat 0 as "unknown". -func extractInstanceTypeSKUEntry(info ec2types.InstanceTypeInfo) instanceTypeSKUEntry { +func extractInstanceTypeSKUEntry(info *ec2types.InstanceTypeInfo) instanceTypeSKUEntry { var vCPUs int var memoryGB float64 diff --git a/providers/aws/recommendations/sku_test.go b/providers/aws/recommendations/sku_test.go index e3ea02f89..54f612c46 100644 --- a/providers/aws/recommendations/sku_test.go +++ b/providers/aws/recommendations/sku_test.go @@ -45,7 +45,7 @@ func newStubPager(entries ...ec2types.InstanceTypeInfo) *stubInstanceTypePager { } // knownInstanceTypes returns a slice with two well-known instance types for -// use in tests that need a populated catalogue. +// use in tests that need a populated catalog. func knownInstanceTypes() []ec2types.InstanceTypeInfo { return []ec2types.InstanceTypeInfo{ { @@ -124,7 +124,7 @@ func TestExtractInstanceTypeSKUEntry(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - entry := extractInstanceTypeSKUEntry(tt.info) + entry := extractInstanceTypeSKUEntry(&tt.info) assert.Equal(t, tt.wantVCPU, entry.vCPUs) assert.InDelta(t, tt.wantMemGB, entry.memoryGB, 0.001) }) @@ -141,12 +141,12 @@ func TestFetchInstanceTypeCatalogue_PopulatesMap(t *testing.T) { assert.Len(t, m, 2) m5, ok := m["m5.large"] - require.True(t, ok, "m5.large must be in catalogue") + require.True(t, ok, "m5.large must be in catalog") assert.Equal(t, 2, m5.vCPUs) assert.InDelta(t, 8.0, m5.memoryGB, 0.001) r5, ok := m["r5.xlarge"] - require.True(t, ok, "r5.xlarge must be in catalogue") + require.True(t, ok, "r5.xlarge must be in catalog") assert.Equal(t, 4, r5.vCPUs) assert.InDelta(t, 32.0, r5.memoryGB, 0.001) } @@ -155,7 +155,7 @@ func TestFetchInstanceTypeCatalogue_PopulatesMap(t *testing.T) { func TestFetchInstanceTypeCatalogue_PageError(t *testing.T) { errPager := &errorOnFirstPagePager{} m := fetchInstanceTypeCatalogue(context.Background(), errPager) - assert.Nil(t, m, "catalogue must be nil on page fetch error") + assert.Nil(t, m, "catalog must be nil on page fetch error") } // TestFetchInstanceTypeCatalogue_ContextCanceled returns nil when ctx is canceled. @@ -165,7 +165,7 @@ func TestFetchInstanceTypeCatalogue_ContextCanceled(t *testing.T) { pager := newStubPager(knownInstanceTypes()...) m := fetchInstanceTypeCatalogue(ctx, pager) - assert.Nil(t, m, "catalogue must be nil when ctx is already canceled") + assert.Nil(t, m, "catalog must be nil when ctx is already canceled") // NextPage must not have been called on a pre-canceled ctx. assert.Equal(t, int32(0), atomic.LoadInt32(&pager.callCount)) } @@ -194,7 +194,7 @@ func TestInstanceTypeLookup_CachedOnce(t *testing.T) { } // TestParseEC2Details_VCPUAndMemoryPopulated asserts that parseEC2Details -// enriches ComputeDetails.VCPU and MemoryGB from the catalogue. +// enriches ComputeDetails.VCPU and MemoryGB from the catalog. func TestParseEC2Details_VCPUAndMemoryPopulated(t *testing.T) { pager := newStubPager(knownInstanceTypes()...) client := NewClientWithAPI(&mockCostExplorerAPI{}, "us-east-1") @@ -239,12 +239,12 @@ func TestParseEC2Details_CatalogueMiss(t *testing.T) { rec := &common.Recommendation{} err := client.parseEC2Details(context.Background(), rec, details) - require.NoError(t, err, "catalogue miss must not fail the conversion") + require.NoError(t, err, "catalog miss must not fail the conversion") cd, ok := rec.Details.(*common.ComputeDetails) require.True(t, ok) - assert.Equal(t, 0, cd.VCPU, "VCPU must be 0 on catalogue miss") - assert.InDelta(t, 0.0, cd.MemoryGB, 0.001, "MemoryGB must be 0 on catalogue miss") + assert.Equal(t, 0, cd.VCPU, "VCPU must be 0 on catalog miss") + assert.InDelta(t, 0.0, cd.MemoryGB, 0.001, "MemoryGB must be 0 on catalog miss") } // TestParseEC2Details_NoCatalogueConfigured leaves VCPU/MemoryGB at zero From d1552ae4896568803771092fc57df62e3e6c5d75 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 20 Jun 2026 13:50:59 +0200 Subject: [PATCH 3/4] fix(aws/recs): pointer-ize RecommendationParams and aws.Config to remove gocritic nolints Remove two gocritic hugeParam nolints from providers/aws/recommendations/client.go by changing both hotspot parameters to pointer types: - NewClient(cfg *aws.Config): cascaded to all 2 callers in aws/service_client.go and aws/recommendations/client_test.go - GetRecommendations(ctx, params *common.RecommendationParams): cascaded through the ServiceClient and RecommendationsClient interfaces in pkg/provider/interface.go to all 25+ implementations across AWS, Azure, and GCP modules, plus all mock/test callers in cmd/ and internal/ No behaviour change; internal helpers that receive a value copy dereference the pointer at the function boundary (params := *p). --- cmd/multi_service_coverage_test.go | 35 ++-------- cmd/multi_service_helpers.go | 2 +- cmd/multi_service_test.go | 64 +++---------------- cmd/multi_service_test_common_test.go | 4 +- internal/purchase/mocks_test.go | 2 +- internal/scheduler/scheduler.go | 2 +- internal/scheduler/scheduler_test.go | 4 +- pkg/provider/interface.go | 4 +- providers/aws/recommendations/client.go | 12 ++-- providers/aws/recommendations/client_test.go | 22 +++---- providers/aws/service_client.go | 8 +-- providers/aws/service_client_test.go | 6 +- providers/aws/services/ec2/client.go | 2 +- providers/aws/services/ec2/client_test.go | 2 +- providers/aws/services/elasticache/client.go | 2 +- .../aws/services/elasticache/client_test.go | 2 +- providers/aws/services/memorydb/client.go | 2 +- .../aws/services/memorydb/client_test.go | 2 +- providers/aws/services/opensearch/client.go | 2 +- .../aws/services/opensearch/client_test.go | 2 +- providers/aws/services/rds/client.go | 2 +- providers/aws/services/rds/client_test.go | 2 +- providers/aws/services/redshift/client.go | 2 +- .../aws/services/redshift/client_test.go | 2 +- providers/aws/services/savingsplans/client.go | 2 +- .../aws/services/savingsplans/client_test.go | 2 +- providers/azure/recommendations.go | 18 +++--- providers/azure/recommendations_test.go | 3 +- providers/azure/services/cache/client.go | 2 +- providers/azure/services/cache/client_test.go | 4 +- providers/azure/services/compute/client.go | 2 +- .../azure/services/compute/client_test.go | 4 +- providers/azure/services/cosmosdb/client.go | 2 +- .../azure/services/cosmosdb/client_test.go | 6 +- providers/azure/services/database/client.go | 2 +- .../azure/services/database/client_test.go | 4 +- .../azure/services/managedredis/client.go | 2 +- .../services/managedredis/client_test.go | 4 +- .../azure/services/savingsplans/client.go | 2 +- .../services/savingsplans/client_test.go | 2 +- providers/azure/services/search/client.go | 2 +- .../azure/services/search/client_test.go | 2 +- providers/azure/services/synapse/client.go | 2 +- .../azure/services/synapse/client_test.go | 12 ++-- providers/gcp/recommendations.go | 15 +++-- providers/gcp/recommendations_test.go | 2 +- providers/gcp/services/cloudsql/client.go | 3 +- .../gcp/services/cloudsql/client_test.go | 6 +- providers/gcp/services/cloudstorage/client.go | 3 +- .../gcp/services/cloudstorage/client_test.go | 6 +- .../gcp/services/computeengine/client.go | 3 +- .../gcp/services/computeengine/client_test.go | 14 ++-- providers/gcp/services/memorystore/client.go | 3 +- .../gcp/services/memorystore/client_test.go | 2 +- 54 files changed, 128 insertions(+), 197 deletions(-) diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 0ba0d0b78..3be671126 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -10,6 +10,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/aws/aws-sdk-go-v2/aws" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) // ==================== Tests for coverage improvement ==================== @@ -583,16 +584,7 @@ func TestProcessService_GetRegionsError(t *testing.T) { toolCfg.Regions = []string{"us-east-1"} // Setup mock to return empty recommendations - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return([]common.Recommendation{}, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation{}, nil) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) @@ -619,16 +611,7 @@ func TestProcessService_GetRecommendationsError(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) // Setup mock to return error - params := common.RecommendationParams{ - Service: common.ServiceEC2, - Region: "us-east-1", - PaymentOption: "partial-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return([]common.Recommendation(nil), errors.New("API error")) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation(nil), errors.New("API error")) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceEC2, true, toolCfg, engineVersionData{}) @@ -655,22 +638,12 @@ func TestProcessService_AllRecommendationsFilteredOut(t *testing.T) { mockClient := &MockRecommendationsClient{} accountCache := NewAccountAliasCache(awsCfg) - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "no-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - // Return recommendations that don't match the filter mockRecs := []common.Recommendation{ {ResourceType: "db.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "db.t3.medium", Count: 3, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index b30426820..5c5338f2e 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -432,7 +432,7 @@ func fetchRecommendationsForRegion( ExcludeSPTypes: cfg.ExcludeSPTypes, } - recs, err := recClient.GetRecommendations(ctx, params) + recs, err := recClient.GetRecommendations(ctx, ¶ms) if err != nil { AppLogger.Printf(" ❌ Failed to fetch recommendations: %v\n", err) return nil diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index fc28c92e7..215cdb616 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -267,21 +267,8 @@ func TestProcessServiceWithMocks(t *testing.T) { mockClient := &MockRecommendationsClient{} // Setup expectations - termStr := "1yr" - if toolCfg.TermYears == 3 { - termStr = "3yr" - } - for _, region := range tt.testRegions { - params := common.RecommendationParams{ - Service: tt.service, - Region: region, - PaymentOption: toolCfg.PaymentOption, - Term: termStr, - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return(tt.mockRecs, nil) + for range tt.testRegions { + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(tt.mockRecs, nil) } // Set regions in toolCfg for this test @@ -337,19 +324,10 @@ func TestProcessService_SavingsPlansAccountLevel(t *testing.T) { // Savings Plans should only query us-east-1 once (account-level). Use the // per-plan-type Compute slug now that the legacy umbrella has been // retired from createServiceClient dispatch. - params := common.RecommendationParams{ - Service: common.ServiceSavingsPlansCompute, - Region: "us-east-1", - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {Service: common.ServiceSavingsPlansCompute, ResourceType: "ComputeSP", Count: 1, Region: "us-east-1", EstimatedSavings: 1000}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceSavingsPlansCompute, true, toolCfg, engineVersionData{}) @@ -383,20 +361,11 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { mockClient := &MockRecommendationsClient{} - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "partial-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "db.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "db.t3.small", Count: 10, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, _ := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) @@ -425,20 +394,11 @@ func TestProcessService_WithOverrideCount(t *testing.T) { mockClient := &MockRecommendationsClient{} - params := common.RecommendationParams{ - Service: common.ServiceElastiCache, - Region: "us-east-1", - PaymentOption: "no-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "cache.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "cache.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, _ := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceElastiCache, true, toolCfg, engineVersionData{}) @@ -465,21 +425,13 @@ func TestProcessService_MultipleRegions(t *testing.T) { mockClient := &MockRecommendationsClient{} - // Setup mock for each region + // Setup mock for each region call in order; Once() ensures testify cycles + // through the returns so each region's call gets region-appropriate results. for _, region := range toolCfg.Regions { - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: region, - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "db.t3.small", Count: 2, Region: region, EstimatedSavings: 100}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil).Once() } accountCache := NewAccountAliasCache(awsCfg) diff --git a/cmd/multi_service_test_common_test.go b/cmd/multi_service_test_common_test.go index cdf65a9ad..2878c142c 100644 --- a/cmd/multi_service_test_common_test.go +++ b/cmd/multi_service_test_common_test.go @@ -31,7 +31,7 @@ type MockRecommendationsClient struct { mock.Mock } -func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) @@ -70,7 +70,7 @@ func (m *MockServiceClient) GetRegion() string { return args.String(0) } -func (m *MockServiceClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 0e18717d8..7888f0735 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -110,7 +110,7 @@ func (m *MockServiceClient) PurchaseCommitment(ctx context.Context, rec common.R return args.Get(0).(common.PurchaseResult), args.Error(1) } -func (m *MockServiceClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index b0300f5ba..b09b1587c 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -905,7 +905,7 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, LookbackPeriod: fmt.Sprintf("%dd", lookbackDays), } var recErr error - recs, recErr = recClient.GetRecommendations(ctx, params) + recs, recErr = recClient.GetRecommendations(ctx, ¶ms) if recErr != nil { logging.Warnf("fetchAndConvert: %s GetRecommendations fallback failed: %v", providerName, recErr) } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 2a380201d..2e59ffa53 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -697,7 +697,7 @@ type MockRecommendationsClient struct { mock.Mock } -func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) @@ -1589,7 +1589,7 @@ func TestScheduler_CollectAWSRecommendations_FallbackToFiltered(t *testing.T) { mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil) mockRecClient.On("GetAllRecommendations", ctx).Return([]common.Recommendation{}, nil) // Empty - mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("common.RecommendationParams")).Return(filteredRecommendations, nil) + mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(filteredRecommendations, nil) scheduler := &Scheduler{ config: mockStore, diff --git a/pkg/provider/interface.go b/pkg/provider/interface.go index f2a02159c..5554fd2fe 100644 --- a/pkg/provider/interface.go +++ b/pkg/provider/interface.go @@ -41,7 +41,7 @@ type ServiceClient interface { GetRegion() string // Recommendations - GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) // Commitments (RI/SP/CUD/etc) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) @@ -56,7 +56,7 @@ type ServiceClient interface { // RecommendationsClient provides centralized recommendations across all services type RecommendationsClient interface { // Get recommendations with filtering - GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) // Get recommendations for a specific service GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index f5b640934..d3cdb1df5 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -56,13 +56,13 @@ type Client struct { } // NewClient creates a new recommendations client. -func NewClient(cfg aws.Config) *Client { //nolint:gocritic // aws.Config is the SDK's value-type convention; pointer form is not idiomatic here. +func NewClient(cfg *aws.Config) *Client { // Force Cost Explorer to use us-east-1 with explicit endpoint ceConfig := cfg.Copy() ceConfig.Region = "us-east-1" ceConfig.BaseEndpoint = aws.String("https://ce.us-east-1.amazonaws.com") - ec2Client := awsec2.NewFromConfig(cfg) + ec2Client := awsec2.NewFromConfig(*cfg) return &Client{ costExplorerClient: costexplorer.NewFromConfig(ceConfig), region: cfg.Region, @@ -108,14 +108,14 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i } // GetRecommendations fetches Reserved Instance recommendations for any service. -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { //nolint:gocritic // RecommendationParams is a value type by convention; pointer form would widen the public API surface +func (c *Client) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { // Handle Savings Plans separately — they use a different Cost Explorer API // (GetSavingsPlansPurchaseRecommendation, not GetReservationPurchaseRecommendation). // Match any SP slug — the legacy umbrella plus the four per-plan-type slugs — // via the IsSavingsPlan family predicate so the dispatch keeps working as // callers migrate. if common.IsSavingsPlan(params.Service) { - return c.getSavingsPlansRecommendations(ctx, params) + return c.getSavingsPlansRecommendations(ctx, *params) } input := &costexplorer.GetReservationPurchaseRecommendationInput{ @@ -131,7 +131,7 @@ func (c *Client) GetRecommendations(ctx context.Context, params common.Recommend return nil, err } - return c.parseRecommendations(ctx, allRecs, params) + return c.parseRecommendations(ctx, allRecs, *params) } // fetchRIAllPages paginates over all pages of RI recommendations for a single @@ -259,7 +259,7 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo LookbackPeriod: "7d", Region: "", } - recs, err := c.GetRecommendations(ctx, params) + recs, err := c.GetRecommendations(ctx, ¶ms) if err != nil { // A canceled / deadline-exceeded ctx is NOT a per-combo // failure to be tolerated — every subsequent combo diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 2d83b44ab..af49f5064 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -65,7 +65,7 @@ func TestNewClient(t *testing.T) { Region: "us-west-2", } - client := NewClient(cfg) + client := NewClient(&cfg) assert.NotNil(t, client) assert.NotNil(t, client.costExplorerClient) @@ -120,7 +120,7 @@ func TestGetRecommendations_EC2_Success(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -164,7 +164,7 @@ func TestGetRecommendations_RDS_Success(t *testing.T) { LookbackPeriod: "30d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -210,7 +210,7 @@ func TestGetRecommendations_ElastiCache_Success(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -249,7 +249,7 @@ func TestGetRecommendations_SavingsPlans_Success(t *testing.T) { IncludeSPTypes: []string{"Compute"}, } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -281,7 +281,7 @@ func TestGetRecommendations_Error(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) assert.Error(t, err) assert.Nil(t, recs) @@ -305,7 +305,7 @@ func TestGetRecommendations_EmptyResult(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Empty(t, recs) @@ -526,7 +526,7 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(ctx, params) + recs, err := client.GetRecommendations(ctx, ¶ms) // With the pagination loop added (issue #692), ctx.Err() is checked at // the top of the first page iteration before the rate-limiter runs. A @@ -750,7 +750,7 @@ func TestGetRecommendations_RI_Paginates(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) // 2 + 3 + 4 = 9 recommendation details, each yielding one rec assert.Len(t, recs, 9, "must accumulate recs across all 3 pages") @@ -779,7 +779,7 @@ func TestGetRecommendations_RI_EmptyTokenTerminates(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) assert.Equal(t, 1, mock.calls, "empty-string token must terminate pagination after page 1") @@ -845,7 +845,7 @@ func TestGetRecommendations_RI_PaginationCapError(t *testing.T) { LookbackPeriod: "7d", } - _, err := client.GetRecommendations(context.Background(), params) + _, err := client.GetRecommendations(context.Background(), ¶ms) require.Error(t, err) assert.Contains(t, err.Error(), "pagination cap reached") assert.Equal(t, maxRecommendationPages, mock.calls, diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index a9d5acff9..d79d8cde3 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -66,18 +66,18 @@ type RecommendationsClientAdapter struct { // NewRecommendationsClient creates a new recommendations client func NewRecommendationsClient(cfg aws.Config) provider.RecommendationsClient { return &RecommendationsClientAdapter{ - client: recommendations.NewClient(cfg), + client: recommendations.NewClient(&cfg), } } // GetRecommendations gets recommendations with filtering -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { recs, err := r.client.GetRecommendations(ctx, params) if err != nil { return nil, err } - recs = applyRecommendationFilters(recs, params) + recs = applyRecommendationFilters(recs, *params) return recs, nil } @@ -178,7 +178,7 @@ func (r *RecommendationsClientAdapter) GetRICoverageMap(ctx context.Context, loo // (needed for GetRIUtilization which is not part of the generic provider interface). func NewRecommendationsClientDirect(cfg aws.Config) *RecommendationsClientAdapter { return &RecommendationsClientAdapter{ - client: recommendations.NewClient(cfg), + client: recommendations.NewClient(&cfg), } } diff --git a/providers/aws/service_client_test.go b/providers/aws/service_client_test.go index 61b5c5b7b..6489e6194 100644 --- a/providers/aws/service_client_test.go +++ b/providers/aws/service_client_test.go @@ -134,12 +134,12 @@ func TestRecommendationsClientAdapter_GetRecommendationsForService(t *testing.T) // testRecommendationsClientAdapter is a test-only version of RecommendationsClientAdapter // that uses an interface for easier mocking type testRecommendationsClientAdapter struct { - getRecommendationsFunc func(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + getRecommendationsFunc func(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) getRecommendationsForServiceFunc func(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) getAllRecommendationsFunc func(ctx context.Context) ([]common.Recommendation, error) } -func (t *testRecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (t *testRecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if t.getRecommendationsFunc != nil { return t.getRecommendationsFunc(ctx, params) } @@ -179,7 +179,7 @@ func TestRecommendationsClientAdapter_GetRecommendations_Integration(t *testing. // This will call the real adapter method which exercises the filtering code // Even though the underlying client returns no recommendations, // this test ensures the adapter's GetRecommendations method is covered - _, err := adapter.GetRecommendations(context.Background(), params) + _, err := adapter.GetRecommendations(context.Background(), ¶ms) // We expect no error even with empty results require.NoError(t, err) }) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 2b31d1cf1..12d42b7f0 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -63,7 +63,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as EC2 uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { // EC2 recommendations come from Cost Explorer API via RecommendationsClient return []common.Recommendation{}, nil } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 44ecee07d..c0427cb40 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -103,7 +103,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { t.Parallel() client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 987e32e96..c8aff0471 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -57,7 +57,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as ElastiCache uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 69db4d4d9..8d8546393 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -68,7 +68,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 2e4542fe1..61a223da6 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -57,7 +57,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as MemoryDB uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index 769954e81..42cbd54be 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -68,7 +68,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index e4071aedf..5847dd0c1 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -76,7 +76,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as OpenSearch uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 8edcb4a05..e6e43b6d5 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -90,7 +90,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 58e16a3dd..ad22f102b 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -59,7 +59,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as RDS uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 6c4f4bb74..fe9f42d83 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -69,7 +69,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index a7b49b864..d0bee9f4e 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -80,7 +80,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as Redshift uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 9920f575b..7c207d2cb 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -97,7 +97,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 4cea975ee..fdc69d864 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -101,7 +101,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as Savings Plans uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index e52191a1e..a726512f0 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -90,7 +90,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 8fe3719cd..fedfd4581 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -65,7 +65,7 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID // its own error and returns nil to the group so that a single service failure // does not cancel sibling calls. Results are appended in a deterministic order // (compute → database → cache → cosmosdb → savingsplans → advisor) after all goroutines finish. -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { var ( computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs []common.Recommendation computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr error @@ -98,7 +98,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // Compute (VM) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceCompute) { + if shouldIncludeService(*params, common.ServiceCompute) { goService(&computeErr, func() { computeClient := compute.NewClient(r.cred, r.subscriptionID, "") computeRecs, computeErr = computeClient.GetRecommendations(gctx, params) @@ -106,7 +106,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // Database (SQL) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceRelationalDB) { + if shouldIncludeService(*params, common.ServiceRelationalDB) { goService(&dbErr, func() { dbClient := database.NewClient(r.cred, r.subscriptionID, "") dbRecs, dbErr = dbClient.GetRecommendations(gctx, params) @@ -114,7 +114,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // Cache (Redis) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceCache) { + if shouldIncludeService(*params, common.ServiceCache) { goService(&cacheErr, func() { cacheClient := cache.NewClient(r.cred, r.subscriptionID, "") cacheRecs, cacheErr = cacheClient.GetRecommendations(gctx, params) @@ -122,7 +122,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p } // CosmosDB (NoSQL) recommendations — subscription-wide. - if shouldIncludeService(params, common.ServiceNoSQL) { + if shouldIncludeService(*params, common.ServiceNoSQL) { goService(&cosmosErr, func() { cosmosClient := cosmosdb.NewClient(r.cred, r.subscriptionID, "") cosmosRecs, cosmosErr = cosmosClient.GetRecommendations(gctx, params) @@ -134,7 +134,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // The call returns an empty slice so the service appears in the fan-out // and will start returning data once the API stabilises without requiring // a scheduler change. - if shouldIncludeService(params, common.ServiceSavingsPlans) { + if shouldIncludeService(*params, common.ServiceSavingsPlans) { goService(&spErr, func() { spClient := savingsplans.NewClient(r.cred, r.subscriptionID, "") spRecs, spErr = spClient.GetRecommendations(gctx, params) @@ -145,7 +145,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // per-service Reservation API. Failures here are non-fatal — the per-service // results above are still useful on their own. goService(&advisorErr, func() { - advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, params) + advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, *params) }) // Wait for all goroutines. g.Wait() always returns nil because every @@ -209,13 +209,13 @@ func (r *RecommendationsClientAdapter) GetRecommendationsForService(ctx context. params := common.RecommendationParams{ Service: service, } - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // GetAllRecommendations retrieves all Azure reservation recommendations across all services func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { params := common.RecommendationParams{} - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // getAdvisorRecommendations retrieves cost optimization recommendations from Azure Advisor diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index efdca18dc..92573bea3 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -277,7 +277,8 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := adapter.GetRecommendations(ctx, common.RecommendationParams{}) + emptyParams := common.RecommendationParams{} + _, err := adapter.GetRecommendations(ctx, &emptyParams) require.Error(t, err, "expected context.Canceled to propagate from GetRecommendations") assert.ErrorIs(t, err, context.Canceled, "GetRecommendations must propagate the parent ctx error after g.Wait()") diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 266e27816..4d2ccb5b8 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -155,7 +155,7 @@ type AzureRetailPrice struct { } // GetRecommendations gets Redis Cache reservation recommendations from Azure Consumption API -func (c *CacheClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CacheClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 14397c747..d770e0b54 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -483,7 +483,7 @@ func TestCacheClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -510,7 +510,7 @@ func TestCacheClient_GetRecommendations_MultiplePages(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 77d175b4a..7458a8b6a 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -175,7 +175,7 @@ type AzureRetailPrice struct { } // GetRecommendations gets VM RI recommendations from Azure Consumption API -func (c *ComputeClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ComputeClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 266c8020a..97862ea94 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -214,7 +214,7 @@ func TestComputeClient_GetRecommendations_WithMock(t *testing.T) { Region: "eastus", } - recommendations, err := client.GetRecommendations(ctx, params) + recommendations, err := client.GetRecommendations(ctx, ¶ms) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -242,7 +242,7 @@ func TestComputeClient_GetRecommendations_EmitsBothPaymentVariants(t *testing.T) } client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 2, "one API rec must expand to two payment-variant entries") diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 5c619e3b5..52808d08b 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -149,7 +149,7 @@ type AzureRetailPrice struct { } // GetRecommendations gets Cosmos DB reservation recommendations from Azure Consumption API -func (c *CosmosDBClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CosmosDBClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 1b77ddfba..90a050fca 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -421,7 +421,7 @@ func TestCosmosDBClient_GetRecommendations_WithMockPager(t *testing.T) { } client.SetRecommendationsPager(mockPager) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.NotNil(t, recommendations) } @@ -437,7 +437,7 @@ func TestCosmosDBClient_GetRecommendations_PagerError(t *testing.T) { client.SetRecommendationsPager(mockPager) - _, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + _, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get Cosmos DB recommendations") } @@ -463,7 +463,7 @@ func TestCosmosDBClient_GetRecommendations_MultiplePages(t *testing.T) { } client.SetRecommendationsPager(mockPager) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.NotNil(t, recommendations) assert.Equal(t, 2, mockPager.index) // Verify both pages were consumed diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index acd598904..b02ea0c4b 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -154,7 +154,7 @@ type AzureRetailPrice struct { } // GetRecommendations gets SQL Database reservation recommendations from Azure Consumption API -func (c *DatabaseClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *DatabaseClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 95d2d8351..1023fb606 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -434,7 +434,7 @@ func TestDatabaseClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -461,7 +461,7 @@ func TestDatabaseClient_GetRecommendations_MultiplePages(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index e38722c8c..d87cd35e2 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -127,7 +127,7 @@ type redisPriceItem struct { } // GetRecommendations gets Redis Cache reservation recommendations from the Azure Consumption API. -func (c *ManagedRedisClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ManagedRedisClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) var pager RecommendationsPager diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index de1a391b2..96092f000 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -341,7 +341,7 @@ func TestGetRecommendations_EmptyPager(t *testing.T) { }}, }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -358,7 +358,7 @@ func TestGetRecommendations_MultiplePages(t *testing.T) { }}, }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 8d98cf6b3..1671fc5b7 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -126,7 +126,7 @@ func (c *Client) GetRegion() string { // recommendations (the Benefits Recommendations API is still in preview). // This mirrors the AWS Savings Plans client which also returns empty here and // delegates to Cost Explorer centrally. -func (c *Client) GetRecommendations(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index aa0b624e0..a2aca979e 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -114,7 +114,7 @@ func TestGetRegion(t *testing.T) { func TestGetRecommendations_AlwaysEmpty(t *testing.T) { c := NewClient(nil, "sub", "eastus") - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 0ec7fb77b..ec29f3440 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -130,7 +130,7 @@ type AzureRetailPrice struct { } // GetRecommendations gets Azure Search reservation recommendations from Azure Consumption API -func (c *SearchClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *SearchClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 766570204..02a2aed00 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -440,7 +440,7 @@ func TestSearchClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 44110ac88..8ed9c8789 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -114,7 +114,7 @@ type SynapseRetailPriceItem struct { // GetRecommendations retrieves Synapse reservation recommendations from the // Azure Consumption API. -func (c *SynapseClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *SynapseClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recs := make([]common.Recommendation, 0) var pager RecommendationsPager diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index a3c8c49c6..d2b4fd6f1 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -167,7 +167,7 @@ func TestGetRecommendations_empty(t *testing.T) { c := newTestClient() c.SetRecommendationsPager(&fakeRecommendationsPager{}) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -191,7 +191,7 @@ func TestGetRecommendations_singlePage(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) @@ -243,7 +243,7 @@ func TestGetRecommendations_multiPage(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recs, 2) } @@ -252,7 +252,7 @@ func TestGetRecommendations_pagerError(t *testing.T) { c := newTestClient() c.SetRecommendationsPager(&errorRecommendationsPager{}) - _, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.Error(t, err) } @@ -281,7 +281,7 @@ func TestGetRecommendations_regionFilter(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "DW500c", recs[0].ResourceType) @@ -307,7 +307,7 @@ func TestGetRecommendations_modernShape(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "DW2000c", recs[0].ResourceType) diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go index 2a8c1cecc..67b88b287 100644 --- a/providers/gcp/recommendations.go +++ b/providers/gcp/recommendations.go @@ -94,7 +94,8 @@ type RecommendationsClientAdapter struct { // Mirrors the Azure parallelisation in // providers/azure/recommendations.go (closes #258, commit b10326c5) and the // AWS service-loop parallelisation (closes #266). -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + params := *p // Get list of regions to check regions, err := r.getRegions(ctx) if err != nil { @@ -175,7 +176,7 @@ func (r *RecommendationsClientAdapter) collectComputeRecs(ctx context.Context, p if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectSQLRecs fetches Cloud SQL CUD recommendations for one region. @@ -188,7 +189,7 @@ func (r *RecommendationsClientAdapter) collectSQLRecs(ctx context.Context, param if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectCacheRecs fetches Memorystore recommendations for one region. @@ -201,7 +202,7 @@ func (r *RecommendationsClientAdapter) collectCacheRecs(ctx context.Context, par if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectStorageRecs fetches Cloud Storage recommendations for one region. @@ -214,7 +215,7 @@ func (r *RecommendationsClientAdapter) collectStorageRecs(ctx context.Context, p if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectRegion fetches recommendations for all four GCP services @@ -286,13 +287,13 @@ func (r *RecommendationsClientAdapter) GetRecommendationsForService(ctx context. params := common.RecommendationParams{ Service: service, } - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // GetAllRecommendations retrieves all GCP commitment recommendations across all services func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { params := common.RecommendationParams{} - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // getRegions retrieves available GCP regions for the project diff --git a/providers/gcp/recommendations_test.go b/providers/gcp/recommendations_test.go index d0fe486b6..076c893dd 100644 --- a/providers/gcp/recommendations_test.go +++ b/providers/gcp/recommendations_test.go @@ -154,7 +154,7 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := adapter.GetRecommendations(ctx, common.RecommendationParams{}) + _, err := adapter.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err, "expected context.Canceled to propagate from GetRecommendations") assert.ErrorIs(t, err, context.Canceled, "GetRecommendations must propagate the parent ctx error") diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 8f443e3eb..bf2b7b777 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -139,7 +139,8 @@ func (c *CloudSQLClient) GetRegion() string { } // GetRecommendations gets Cloud SQL recommendations from GCP Recommender API -func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CloudSQLClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + params := *p recommendations := make([]common.Recommendation, 0) // Use injected client if available (for testing) diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index 6a93b81ae..0e3bbe32d 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/providers/gcp/services/cloudsql/client_test.go @@ -617,7 +617,7 @@ func TestCloudSQLClient_GetRecommendations_WithMock(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -635,7 +635,7 @@ func TestCloudSQLClient_GetRecommendations_IteratorError(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "cloudsql: iterate recommendations") assert.Nil(t, recs, "partial data must not leak on iterator failure") @@ -655,7 +655,7 @@ func TestCloudSQLClient_GetRecommendations_Empty(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index bcae9ae1a..7848d6f20 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -159,7 +159,8 @@ func (c *CloudStorageClient) GetRegion() string { } // GetRecommendations gets Cloud Storage recommendations from GCP Recommender API -func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CloudStorageClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + params := *p recommendations := make([]common.Recommendation, 0) // Use injected client if available (for testing) diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go index 78b874d0b..b46a64219 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -376,7 +376,7 @@ func TestCloudStorageClient_GetRecommendations_WithMock(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -394,7 +394,7 @@ func TestCloudStorageClient_GetRecommendations_Empty(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -411,7 +411,7 @@ func TestCloudStorageClient_GetRecommendations_IteratorError(t *testing.T) { // Iterator errors now propagate (issue #1022 H2 fix) -- they must not be // silently swallowed, as that would mask auth/quota failures and cause callers // to act on a partial (empty) recommendation list. - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "cloudstorage: iterate recommendations") assert.Nil(t, recommendations) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 2af9cfa83..ec730f727 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -214,7 +214,8 @@ func (c *ComputeEngineClient) GetRegion() string { } // GetRecommendations gets CUD recommendations from GCP Recommender API -func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + params := *p recommendations := make([]common.Recommendation, 0) // Use injected client if available (for testing) diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index bb3e0d534..7754b8864 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -791,7 +791,7 @@ func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -815,7 +815,7 @@ func TestComputeEngineClient_GetRecommendations_IteratorError(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "computeengine: iterate recommendations") assert.Nil(t, recs, "partial data must not leak on iterator failure") @@ -833,7 +833,7 @@ func TestComputeEngineClient_GetRecommendations_Empty(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -930,7 +930,7 @@ func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing require.NoError(t, err) client.SetRecommenderClient(&infiniteRecommenderClient{}) - _, err = client.GetRecommendations(ctx, common.RecommendationParams{}) + _, err = client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err, "cancelled context must surface an error, not a partial result set") } @@ -941,7 +941,7 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { require.NoError(t, err) client.SetRecommenderClient(&infiniteRecommenderClient{}) - _, err = client.GetRecommendations(context.Background(), common.RecommendationParams{}) + _, err = client.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.Error(t, err, "page cap must surface an error when the iterator never terminates") } @@ -1556,7 +1556,7 @@ func TestGetRecommendations_FiltersNonActiveStates(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + results, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, results, 1, "only the ACTIVE recommendation must be returned; CLAIMED/SUCCEEDED/FAILED/DISMISSED must be filtered (H-1)") @@ -1589,7 +1589,7 @@ func TestGetRecommendations_ActiveRecIncluded(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + results, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, results, 1, "an ACTIVE recommendation must be included") } diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index ba6a2946a..aa2956d78 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -152,7 +152,8 @@ func (c *MemorystoreClient) GetRegion() string { } // GetRecommendations gets Memorystore Redis recommendations from GCP Recommender API -func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *MemorystoreClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + params := *p recClient := c.recommenderClient if recClient == nil { client, err := recommender.NewClient(ctx, c.clientOpts...) diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index 81eff3f53..0f8a2ffab 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -574,7 +574,7 @@ func TestMemorystoreClient_GetRecommendations_WithMockClient(t *testing.T) { } client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) if tt.wantErr { require.Error(t, err) From 41caec2cb8ef5d21b99a41f71de1549d72ac027d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 15:56:45 +0200 Subject: [PATCH 4/4] fix(gcp/recs): nil-guard p before dereference in all GetRecommendations Pointer-izing RecommendationParams (prev commit) exposed five GCP GetRecommendations implementations that dereference p without a nil check, which would panic if a caller passes nil. Add a nil guard at the function boundary in all five GCP service clients: RecommendationsClientAdapter, CloudSQLClient, CloudStorageClient, ComputeEngineClient, and MemorystoreClient. Extract resolveRecommenderClient helpers from the four leaf clients to keep GetRecommendations cyclomatic complexity at 10 (pre-commit limit) after the nil-guard branch is added. --- providers/gcp/recommendations.go | 3 ++ providers/gcp/services/cloudsql/client.go | 29 ++++++++++++------- providers/gcp/services/cloudstorage/client.go | 29 ++++++++++++------- .../gcp/services/computeengine/client.go | 29 ++++++++++++------- providers/gcp/services/memorystore/client.go | 26 ++++++++++++----- 5 files changed, 79 insertions(+), 37 deletions(-) diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go index 67b88b287..cb849bddd 100644 --- a/providers/gcp/recommendations.go +++ b/providers/gcp/recommendations.go @@ -95,6 +95,9 @@ type RecommendationsClientAdapter struct { // providers/azure/recommendations.go (closes #258, commit b10326c5) and the // AWS service-loop parallelisation (closes #266). func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } params := *p // Get list of regions to check regions, err := r.getRegions(ctx) diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index bf2b7b777..f1ededded 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -138,21 +138,30 @@ func (c *CloudSQLClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *CloudSQLClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Cloud SQL recommendations from GCP Recommender API func (c *CloudSQLClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 7848d6f20..2aa13fb2d 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -158,21 +158,30 @@ func (c *CloudStorageClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *CloudStorageClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Cloud Storage recommendations from GCP Recommender API func (c *CloudStorageClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index ec730f727..8171f1a8a 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -213,21 +213,30 @@ func (c *ComputeEngineClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *ComputeEngineClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets CUD recommendations from GCP Recommender API func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index aa2956d78..31eeeeb19 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -151,16 +151,28 @@ func (c *MemorystoreClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *MemorystoreClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Memorystore Redis recommendations from GCP Recommender API func (c *MemorystoreClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } params := *p - recClient := c.recommenderClient - if recClient == nil { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close()