From cc5e24bccaa0972020f067a50e68e4aa4b21215e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 13:37:30 +0200 Subject: [PATCH 1/5] test(coverage): assert CE lookback window width + add --rec-lookback-period flag (refs #360) Gap 1: extend mockCoverageCE to capture the last CE TimePeriod and add TestGetRICoverageMap_LookbackWindowWidth, which parses Start/End YYYY-MM-DD strings and asserts end-start == lookbackDays. Test fails if the window math regresses to a hardcoded value. Gap 2: replace the tautological "custom lookback stored in Config" subcase in TestFetchExistingCoverage_LookbackDays (which asserted a struct field equals what was just assigned) with a comment pointing to the new discriminating test. Gap 3: implement the second flag from issue #360: --rec-lookback-period (7d/30d/60d, default 7d) controls LookbackPeriodInDays in GetReservationPurchaseRecommendation. Wired via Config.RecLookbackPeriod through fetchRecommendationsForRegion (CLI path) and Client.SetRecLookbackPeriod (discovery/UI path). Validated in validateFlags. TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts all three enum values reach the actual CE input. TestValidateRecLookbackPeriod guards the validation logic. --- cmd/main.go | 9 +++ cmd/multi_service.go | 3 + cmd/multi_service_coverage_test.go | 12 ++-- cmd/multi_service_helpers.go | 6 +- cmd/validators.go | 13 +++++ cmd/validators_test.go | 36 ++++++++++++ providers/aws/recommendations/client.go | 19 ++++++- providers/aws/recommendations/client_test.go | 34 +++++++++++ .../aws/recommendations/coverage_test.go | 56 +++++++++++++++++++ providers/aws/service_client.go | 6 ++ 10 files changed, 186 insertions(+), 8 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index f6c4c9cc7..2a8eded0a 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -69,6 +69,11 @@ type Config struct { ActualPurchase bool DryRun bool SkipConfirmation bool + // RecLookbackPeriod controls the LookbackPeriodInDays passed to + // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d". + // A longer window smooths seasonal spikes; a shorter window weights + // recent demand more heavily. Default "7d" matches the CE console default. + RecLookbackPeriod string } func main() { @@ -146,6 +151,10 @@ func init() { "below this threshold. Useful with --target-coverage to skip tiny pools "+ "that integer arithmetic forces above target (e.g. avg=1 cannot hit 80%%). "+ "Default 0 = no filter.") + rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", "7d", + "Historical window for GetReservationPurchaseRecommendation. "+ + "Valid values: 7d, 30d, 60d. A longer window smooths seasonal spikes; "+ + "a shorter window weights recent demand more heavily. Default 7d.") } // Package-level Config that cobra flags bind to. diff --git a/cmd/multi_service.go b/cmd/multi_service.go index eb3939cbd..05c2077f2 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -117,6 +117,9 @@ func runToolMultiService(ctx context.Context, cfg Config) { accountCache := NewAccountAliasCache(awsCfg) recClient := awsprovider.NewRecommendationsClient(awsCfg) + if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" { + adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod) + } engineData := fetchEngineVersionData(ctx, cfg) // Fetch existing-RI coverage so --target-coverage can subtract what diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 833c93d24..2fa4a676c 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -797,9 +797,11 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) { assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)") }) - t.Run("custom lookback stored in Config", func(t *testing.T) { - cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}} - // CoverageLookbackDays field value is preserved in the struct. - assert.Equal(t, 60, cfg.CoverageLookbackDays) - }) + // The previous "custom lookback stored in Config" subcase asserted only that + // a struct field equals what was just assigned -- a tautology that passes even + // if CoverageLookbackDays is never forwarded to GetRICoverageMap. The real + // assertion -- that the lookback value reaches the CE TimePeriod -- is covered + // by TestGetRICoverageMap_LookbackWindowWidth in + // providers/aws/recommendations/coverage_test.go, which directly verifies + // end-start == lookbackDays on the actual CE input. No redundant subcase here. } diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 387fa58c4..a2576b2b6 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -423,12 +423,16 @@ func fetchRecommendationsForRegion( termStr = "3yr" } + lookback := cfg.RecLookbackPeriod + if lookback == "" { + lookback = "7d" + } params := common.RecommendationParams{ Service: service, Region: region, PaymentOption: cfg.PaymentOption, Term: termStr, - LookbackPeriod: "7d", + LookbackPeriod: lookback, // Savings Plans specific filters IncludeSPTypes: cfg.IncludeSPTypes, ExcludeSPTypes: cfg.ExcludeSPTypes, diff --git a/cmd/validators.go b/cmd/validators.go index 9277e6836..a97e71bcc 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -29,6 +29,19 @@ func validateFlags(cmd *cobra.Command, args []string) error { return err } + if err := validateRecLookbackPeriod(); err != nil { + return err + } + + return nil +} + +// validateRecLookbackPeriod validates the --rec-lookback-period flag. +func validateRecLookbackPeriod() error { + valid := map[string]bool{"7d": true, "30d": true, "60d": true} + if !valid[toolCfg.RecLookbackPeriod] { + return fmt.Errorf("invalid rec-lookback-period %q: must be one of 7d, 30d, 60d", toolCfg.RecLookbackPeriod) + } return nil } diff --git a/cmd/validators_test.go b/cmd/validators_test.go index 98dcd5728..05c1b817c 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -537,3 +537,39 @@ func TestValidateCoverageLookbackDays(t *testing.T) { }) } } + +// TestValidateRecLookbackPeriod verifies that validateRecLookbackPeriod accepts +// the three valid values and rejects anything else, including empty string. +func TestValidateRecLookbackPeriod(t *testing.T) { + tests := []struct { + name string + period string + wantErr bool + errSubstr string + }{ + {name: "7d valid", period: "7d", wantErr: false}, + {name: "30d valid", period: "30d", wantErr: false}, + {name: "60d valid", period: "60d", wantErr: false}, + {name: "empty rejected", period: "", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "14d rejected", period: "14d", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "90d rejected", period: "90d", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + {name: "SEVEN_DAYS rejected", period: "SEVEN_DAYS", wantErr: true, errSubstr: "invalid rec-lookback-period"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.RecLookbackPeriod = tt.period + err := validateRecLookbackPeriod() + if tt.wantErr { + if err == nil { + t.Errorf("validateRecLookbackPeriod() expected error containing %q, got nil", tt.errSubstr) + } else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("validateRecLookbackPeriod() error = %v, want substring %q", err, tt.errSubstr) + } + } else if err != nil { + t.Errorf("validateRecLookbackPeriod() unexpected error = %v", err) + } + }) + } +} diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 7a1274f7f..e874910e9 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -55,6 +55,10 @@ type Client struct { // lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes // fan-out per scheduler tick). skuCatalog skuCatalog + + // recLookbackPeriod is forwarded to GetReservationPurchaseRecommendation + // as LookbackPeriodInDays. Defaults to "7d" when empty. + recLookbackPeriod string } // NewClient creates a new recommendations client @@ -100,7 +104,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 { @@ -109,6 +113,13 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory) } +// SetRecLookbackPeriod configures the LookbackPeriodInDays used by +// GetRecommendationsForService. Valid values: "7d", "30d", "60d". +// An empty or unrecognised value falls back to "7d" at call time. +func (c *Client) SetRecLookbackPeriod(period string) { + c.recLookbackPeriod = period +} + // GetRecommendations fetches Reserved Instance recommendations for any service func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { // Handle Savings Plans separately — they use a different Cost Explorer API @@ -254,11 +265,15 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo return nil, ctx.Err() } attempts++ + lookback := c.recLookbackPeriod + if lookback == "" { + lookback = "7d" + } params := common.RecommendationParams{ Service: service, PaymentOption: payment, Term: term, - LookbackPeriod: "7d", + LookbackPeriod: lookback, Region: "", } recs, err := c.GetRecommendations(ctx, params) diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 54beed2de..66551ce6b 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -917,3 +917,37 @@ func TestMergeServiceResults_AllFailIsError(t *testing.T) { require.NoError(t, err) assert.Empty(t, recs) } + +// TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts +// that SetRecLookbackPeriod propagates the chosen period into the +// LookbackPeriodInDays field of every GetReservationPurchaseRecommendation +// call issued by GetRecommendationsForService (refs #360). This test is +// intentionally discriminating: it would fail if recLookbackPeriod were +// ignored and the hardcoded "7d" default were sent instead. +func TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation(t *testing.T) { + cases := []struct { + period string + wantEnum types.LookbackPeriodInDays + }{ + {"7d", types.LookbackPeriodInDaysSevenDays}, + {"30d", types.LookbackPeriodInDaysThirtyDays}, + {"60d", types.LookbackPeriodInDaysSixtyDays}, + } + for _, tc := range cases { + t.Run(tc.period, func(t *testing.T) { + mock := &mockCostExplorerAPI{ + riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{}, + } + client := NewClientWithAPI(mock, "us-east-1") + client.SetRecLookbackPeriod(tc.period) + + _, err := client.GetRecommendationsForService(context.Background(), common.ServiceEC2) + require.NoError(t, err) + require.NotEmpty(t, mock.riCalls, "GetReservationPurchaseRecommendation must have been called") + for i, call := range mock.riCalls { + assert.Equal(t, tc.wantEnum, call.LookbackPeriodInDays, + "call[%d]: LookbackPeriodInDays must match --rec-lookback-period=%s", i, tc.period) + } + }) + } +} diff --git a/providers/aws/recommendations/coverage_test.go b/providers/aws/recommendations/coverage_test.go index 685d20802..beacf2e16 100644 --- a/providers/aws/recommendations/coverage_test.go +++ b/providers/aws/recommendations/coverage_test.go @@ -3,6 +3,7 @@ package recommendations import ( "context" "testing" + "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/costexplorer" @@ -15,15 +16,21 @@ import ( // mockCoverageCE extends the test mock with a configurable GetReservationCoverage // response so the coverage path can be exercised without hitting AWS. +// lastTimePeriod captures the TimePeriod from the most recent call so tests can +// assert that the CE window matches the requested lookbackDays. type mockCoverageCE struct { mockCostExplorerAPI coverageOutput *costexplorer.GetReservationCoverageOutput coverageError error coverageCalls int + lastTimePeriod *types.DateInterval } func (m *mockCoverageCE) GetReservationCoverage(ctx context.Context, params *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) { m.coverageCalls++ + if params != nil { + m.lastTimePeriod = params.TimePeriod + } if m.coverageError != nil { return nil, m.coverageError } @@ -118,6 +125,55 @@ func TestGetRICoverageMap_LookbackDefault(t *testing.T) { assert.Equal(t, wantCalls, mock.coverageCalls) } +// TestGetRICoverageMap_LookbackWindowWidth asserts that the CE TimePeriod +// sent to GetReservationCoverage spans exactly lookbackDays calendar days. +// This test is deliberately discriminating: it parses the YYYY-MM-DD Start +// and End strings and verifies end-start == N days so a regression in the +// window math (e.g. reverting to a hardcoded 30) causes a test failure. +func TestGetRICoverageMap_LookbackWindowWidth(t *testing.T) { + cases := []struct { + name string + lookbackDays int + }{ + {"14-day window", 14}, + {"60-day window", 60}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + mock := &mockCoverageCE{coverageOutput: &costexplorer.GetReservationCoverageOutput{}} + client := NewClientWithAPI(mock, "us-east-1") + + before := time.Now().UTC() + _, err := client.GetRICoverageMap(context.Background(), tc.lookbackDays, []string{"us-east-1"}) + after := time.Now().UTC() + require.NoError(t, err) + require.NotNil(t, mock.lastTimePeriod, "GetReservationCoverage must have been called with a TimePeriod") + require.NotNil(t, mock.lastTimePeriod.Start) + require.NotNil(t, mock.lastTimePeriod.End) + + start, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.Start)) + require.NoError(t, err, "TimePeriod.Start must be a valid YYYY-MM-DD date") + end, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.End)) + require.NoError(t, err, "TimePeriod.End must be a valid YYYY-MM-DD date") + + // CE dates are truncated to day; allow for midnight-boundary + // skew of at most 1 day when the test runs near UTC midnight. + spanDays := int(end.Sub(start).Hours() / 24) + assert.Equal(t, tc.lookbackDays, spanDays, + "CE TimePeriod must span exactly lookbackDays days (start=%s end=%s)", + aws.ToString(mock.lastTimePeriod.Start), + aws.ToString(mock.lastTimePeriod.End)) + + // Sanity: the End date must fall within the test's execution window. + endUTC := end + assert.True(t, !endUTC.Before(before.Truncate(24*time.Hour)), + "TimePeriod.End must not be before test start") + assert.True(t, !endUTC.After(after.Add(24*time.Hour)), + "TimePeriod.End must not be more than a day after test completion") + }) + } +} + // TestApplyCoverageMapToRecommendations covers the org-wide pool matching: // recs look up by (region, instance_type, [engine, deployment]) so any // linked account in the org sees the same coverage % for the same pool diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index a9d5acff9..902187860 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -174,6 +174,12 @@ func (r *RecommendationsClientAdapter) GetRICoverageMap(ctx context.Context, loo return r.client.GetRICoverageMap(ctx, lookbackDays, regions) } +// SetRecLookbackPeriod configures the LookbackPeriodInDays forwarded to +// GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d". +func (r *RecommendationsClientAdapter) SetRecLookbackPeriod(period string) { + r.client.SetRecLookbackPeriod(period) +} + // NewRecommendationsClientDirect creates a new recommendations client returning the concrete type // (needed for GetRIUtilization which is not part of the generic provider interface). func NewRecommendationsClientDirect(cfg aws.Config) *RecommendationsClientAdapter { From 603fbbe9b04ec2dcd1d2ccee4ecc4ebfb2edf517 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:48:06 +0200 Subject: [PATCH 2/5] chore(coverage): gofmt alignment fix in test struct literal --- providers/aws/recommendations/coverage_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/aws/recommendations/coverage_test.go b/providers/aws/recommendations/coverage_test.go index beacf2e16..c346fc8a7 100644 --- a/providers/aws/recommendations/coverage_test.go +++ b/providers/aws/recommendations/coverage_test.go @@ -132,7 +132,7 @@ func TestGetRICoverageMap_LookbackDefault(t *testing.T) { // window math (e.g. reverting to a hardcoded 30) causes a test failure. func TestGetRICoverageMap_LookbackWindowWidth(t *testing.T) { cases := []struct { - name string + name string lookbackDays int }{ {"14-day window", 14}, From edf071b156fe5f15dc7f8b0c0008faea8b7cb5cc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 14:46:45 -0700 Subject: [PATCH 3/5] refactor(cyclo): extract helpers to fix gocyclo failures on PR 992 Extract fetchSingleComboRecs from GetRecommendationsForService (complexity 11->8) and runPurchaseAndReport from runToolMultiService (complexity 12->9) to satisfy the pre-commit cyclomatic-complexity hook (limit 10). Also rebase onto origin/feat/multicloud-web-frontend resolving the client_test.go conflict by keeping both tests added by each side. No behaviour change. --- cmd/multi_service.go | 11 +++-- providers/aws/recommendations/client.go | 61 ++++++++++++++++--------- 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 05c2077f2..9c4cf5e94 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -140,7 +140,14 @@ func runToolMultiService(ctx context.Context, cfg Config) { return } - // Phase 3: confirm (skipped in dry-run). + // Phases 3-4: confirm, purchase, and produce summary outputs. + runPurchaseAndReport(ctx, awsCfg, scoredResult, isDryRun, cfg) +} + +// runPurchaseAndReport handles the confirm, execute, and report phases of +// the multi-service pipeline. It is a separate function to keep +// runToolMultiService within the cyclomatic-complexity limit. +func runPurchaseAndReport(ctx context.Context, awsCfg aws.Config, scoredResult scorer.ScoredResult, isDryRun bool, cfg Config) { runID := uuid.New().String() if !isDryRun { totalInstances, totalSavings := sumPassedRecs(scoredResult.Passed) @@ -150,10 +157,8 @@ func runToolMultiService(ctx context.Context, cfg Config) { } } - // Phase 4: purchase each recommendation and write audit records. allResults := executePurchasePipeline(ctx, awsCfg, scoredResult.Passed, isDryRun, runID, cfg) - // Produce summary outputs. serviceStats := buildServiceStats(scoredResult.Passed, allResults) finalCSVOutput := generateCSVFilename(isDryRun, cfg) if err := writeMultiServiceCSVReport(allResults, finalCSVOutput); err != nil { diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index e874910e9..ef989ed0e 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -254,6 +254,44 @@ var defaultDiscoveryPaymentOptions = []string{"all-upfront", "partial-upfront", // throttle on one (term, payment) combo doesn't suppress the others; // only an error where every combo fails is propagated. This mirrors // the "continue on per-service error" tolerance in GetAllRecommendations. +// fetchSingleComboRecs fetches recommendations for one (term, payment) pair. +// If the context is already done before the call, it returns (nil, ctx.Err()). +// If GetRecommendations returns an error after ctx cancellation, it also +// returns (nil, ctx.Err()) so the caller exits the sweep immediately. Per-combo +// errors (throttle, 5xx) return (nil, err) with ctx.Err() == nil, signalling +// skip-and-continue tolerance in the outer loop. +func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.ServiceType, term string, payment string) ([]common.Recommendation, error) { + if ctx.Err() != nil { + return nil, ctx.Err() + } + lookback := c.recLookbackPeriod + if lookback == "" { + lookback = "7d" + } + params := common.RecommendationParams{ + Service: service, + PaymentOption: payment, + Term: term, + LookbackPeriod: lookback, + Region: "", + } + recs, err := c.GetRecommendations(ctx, params) + if err != nil { + // A canceled / deadline-exceeded ctx is NOT a per-combo + // failure to be tolerated -- every subsequent combo + // would just hit the same dead context and waste time + // while we accumulate "failures" that hide the real + // reason. Short-circuit so the caller sees the ctx + // error verbatim. Per-combo errors (throttle, 5xx) + // keep the existing skip-and-continue tolerance. + if ctx.Err() != nil { + return nil, ctx.Err() + } + return nil, err + } + return recs, nil +} + func (c *Client) GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) { allRecs := make([]common.Recommendation, 0) var lastErr error @@ -261,30 +299,9 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo attempts := 0 for _, term := range defaultDiscoveryTerms { for _, payment := range defaultDiscoveryPaymentOptions { - if ctx.Err() != nil { - return nil, ctx.Err() - } attempts++ - lookback := c.recLookbackPeriod - if lookback == "" { - lookback = "7d" - } - params := common.RecommendationParams{ - Service: service, - PaymentOption: payment, - Term: term, - LookbackPeriod: lookback, - Region: "", - } - recs, err := c.GetRecommendations(ctx, params) + recs, err := c.fetchSingleComboRecs(ctx, service, term, payment) if err != nil { - // A canceled / deadline-exceeded ctx is NOT a per-combo - // failure to be tolerated — every subsequent combo - // would just hit the same dead context and waste time - // while we accumulate "failures" that hide the real - // reason. Short-circuit so the caller sees the ctx - // error verbatim. Per-combo errors (throttle, 5xx) - // keep the existing skip-and-continue tolerance. if ctx.Err() != nil { return nil, ctx.Err() } From 77790f53bc023a44fb434514ed559de8c70d9ce4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 17:19:58 +0200 Subject: [PATCH 4/5] fix(coverage): restore GetRecommendationsForService docstring to correct function After extracting fetchSingleComboRecs from GetRecommendationsForService, the docstring for GetRecommendationsForService was left attached to the extracted helper rather than to the function it describes. Move the docstring back to GetRecommendationsForService and give fetchSingleComboRecs its own focused comment. --- providers/aws/recommendations/client.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index ef989ed0e..8248a1605 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -242,18 +242,6 @@ var defaultDiscoveryTerms = []string{"1yr", "3yr"} // rows and render as distinct UI rows for free. var defaultDiscoveryPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"} -// GetRecommendationsForService fetches recommendations for a specific -// service across the full Cartesian product of defaultDiscoveryTerms × -// defaultDiscoveryPaymentOptions (currently 2 × 3 = 6 Cost Explorer -// calls per service). Each call returns the recs for that single -// (term, payment) cell and the parser tags them with params.Term / -// params.PaymentOption so the resulting slice contains every combo -// for the user to choose from in the UI. -// -// A per-call Cost Explorer error is tolerated and skipped so a single -// throttle on one (term, payment) combo doesn't suppress the others; -// only an error where every combo fails is propagated. This mirrors -// the "continue on per-service error" tolerance in GetAllRecommendations. // fetchSingleComboRecs fetches recommendations for one (term, payment) pair. // If the context is already done before the call, it returns (nil, ctx.Err()). // If GetRecommendations returns an error after ctx cancellation, it also @@ -292,6 +280,18 @@ func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.Servic return recs, nil } +// GetRecommendationsForService fetches recommendations for a specific +// service across the full Cartesian product of defaultDiscoveryTerms x +// defaultDiscoveryPaymentOptions (currently 2 x 3 = 6 Cost Explorer +// calls per service). Each call returns the recs for that single +// (term, payment) cell and the parser tags them with params.Term / +// params.PaymentOption so the resulting slice contains every combo +// for the user to choose from in the UI. +// +// A per-call Cost Explorer error is tolerated and skipped so a single +// throttle on one (term, payment) combo doesn't suppress the others; +// only an error where every combo fails is propagated. This mirrors +// the "continue on per-service error" tolerance in GetAllRecommendations. func (c *Client) GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) { allRecs := make([]common.Recommendation, 0) var lastErr error From bee2aa534ea2c92fcee94e2b129a6bee6caccdc5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 19:52:00 +0200 Subject: [PATCH 5/5] refactor(coverage): hoist 7d default into shared DefaultRecLookbackPeriod constant The "7d" default was hardcoded in three places after PR #992 added --rec-lookback-period (cmd flag default, cmd-side fallback in fetchRecommendationsForRegion, client-side fallback in fetchSingleComboRecs). Per feedback_no_hardcoded_magic_values.md, centralise it into recommendations.DefaultRecLookbackPeriod so the cmd flag default, the cmd-side fallback, and the client-side fallback all reference one source of truth, and a future change to the default only needs to flip one value. No behavioural change: same "7d" value, same call sites. refs #360 --- cmd/main.go | 8 +++++--- cmd/multi_service_helpers.go | 2 +- providers/aws/recommendations/client.go | 12 +++++++++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 2a8eded0a..118141e61 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -11,6 +11,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/pkg/provider" _ "github.com/LeanerCloud/CUDly/providers/aws" + "github.com/LeanerCloud/CUDly/providers/aws/recommendations" "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" "github.com/LeanerCloud/CUDly/providers/aws/services/elasticache" "github.com/LeanerCloud/CUDly/providers/aws/services/memorydb" @@ -70,9 +71,10 @@ type Config struct { DryRun bool SkipConfirmation bool // RecLookbackPeriod controls the LookbackPeriodInDays passed to - // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d". + // GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d" + // (recommendations.DefaultRecLookbackPeriod is the shared default). // A longer window smooths seasonal spikes; a shorter window weights - // recent demand more heavily. Default "7d" matches the CE console default. + // recent demand more heavily. RecLookbackPeriod string } @@ -151,7 +153,7 @@ func init() { "below this threshold. Useful with --target-coverage to skip tiny pools "+ "that integer arithmetic forces above target (e.g. avg=1 cannot hit 80%%). "+ "Default 0 = no filter.") - rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", "7d", + rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", recommendations.DefaultRecLookbackPeriod, "Historical window for GetReservationPurchaseRecommendation. "+ "Valid values: 7d, 30d, 60d. A longer window smooths seasonal spikes; "+ "a shorter window weights recent demand more heavily. Default 7d.") diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index a2576b2b6..581f4200c 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -425,7 +425,7 @@ func fetchRecommendationsForRegion( lookback := cfg.RecLookbackPeriod if lookback == "" { - lookback = "7d" + lookback = recommendations.DefaultRecLookbackPeriod } params := common.RecommendationParams{ Service: service, diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 8248a1605..8ea6c2bea 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -22,6 +22,16 @@ import ( // payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692). const maxRecommendationPages = 20 +// DefaultRecLookbackPeriod is the LookbackPeriod string forwarded to +// GetReservationPurchaseRecommendation when --rec-lookback-period is not +// specified. Kept in the recommendations package so the cmd flag default, +// the cmd-side fallback, and the client-side fallback all refer to a single +// source of truth (avoids the magic-value duplication called out by +// feedback_no_hardcoded_magic_values.md). Valid CE values are 7d/30d/60d +// (see convertLookbackPeriodE); 7d matches the prior hardcoded behaviour +// from before --rec-lookback-period existed. +const DefaultRecLookbackPeriod = "7d" + // 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) @@ -254,7 +264,7 @@ func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.Servic } lookback := c.recLookbackPeriod if lookback == "" { - lookback = "7d" + lookback = DefaultRecLookbackPeriod } params := common.RecommendationParams{ Service: service,