Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ type Config struct {
Regions []string
Services []string
ExcludeAccounts []string
Providers []string
IncludeRegions []string
ExcludeRegions []string
IncludeInstanceTypes []string
Expand Down
32 changes: 0 additions & 32 deletions cmd/multi_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -960,38 +960,6 @@ func requireFilterInputs(recs []common.Recommendation, cfg Config, engineData en
return requireResolvedAccountNames(recs, &cfg)
}

// processService processes a single service and returns recommendations and results.
// Used by legacy callers; new code should use fetchAllRecs + executePurchasePipeline.
func processService(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, accountCache *AccountAliasCache, service common.ServiceType, isDryRun bool, cfg Config, engineData engineVersionData) ([]common.Recommendation, []common.PurchaseResult) { //nolint:unparam // engineData always nil at current callsites but param is part of the API
regionsToProcess, err := determineRegionsForService(ctx, recClient, service, cfg)
if err != nil {
log.Printf("❌ Failed to determine regions: %v", err)
return nil, nil
}

serviceRecs := make([]common.Recommendation, 0)
serviceResults := make([]common.PurchaseResult, 0)

for i, region := range regionsToProcess {
// Legacy single-service entry point — no coverage map is fetched here,
// so sizing falls back to the no-existing-commitments formula. The new
// path (runToolMultiService) fetches coverage once and threads it through.
regionResult := processRegionRecommendations(
ctx, awsCfg, recClient, accountCache,
service, region, i+1, len(regionsToProcess),
engineData, isDryRun, cfg, nil,
)
serviceRecs = append(serviceRecs, regionResult.recommendations...)
serviceResults = append(serviceResults, regionResult.results...)
if regionResult.auditErr != nil {
log.Printf("%s", auditStopMessage(len(serviceResults), len(serviceRecs), serviceResults, regionResult.auditErr))
break
}
}

return serviceRecs, serviceResults
}

// processPurchaseLoop processes purchases for a single region (used by CSV
// mode). runID groups every recommendation processed across the whole CSV
// run into one audit trail, matching how executePurchasePipeline (the main
Expand Down
93 changes: 0 additions & 93 deletions cmd/multi_service_coverage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
awsrds "github.com/aws/aws-sdk-go-v2/service/rds"
rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -624,98 +623,6 @@ func TestValidateFlags_CSVPaths(t *testing.T) {
})
}

// ==================== Tests for processService Error Paths ====================

func TestProcessService_GetRegionsError(t *testing.T) {
ctx := context.Background()
awsCfg := aws.Config{Region: "us-east-1"}

origCfg := toolCfg
defer func() { toolCfg = origCfg }()

toolCfg.Coverage = 100.0
toolCfg.PaymentOption = "all-upfront"
toolCfg.TermYears = 3
toolCfg.Regions = []string{} // Empty - will trigger auto-discovery

mockClient := &MockRecommendationsClient{}
accountCache := NewAccountAliasCache(awsCfg)

// This test verifies behavior when region discovery is needed
// Since getAllAWSRegions requires real AWS config, we test with explicit regions instead
toolCfg.Regions = []string{"us-east-1"}

// Setup mock to return empty recommendations
mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation{}, nil)

recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{})

// Should return empty recommendations
assert.Empty(t, recs)
assert.Empty(t, results)

mockClient.AssertExpectations(t)
}

func TestProcessService_GetRecommendationsError(t *testing.T) {
ctx := context.Background()
awsCfg := aws.Config{Region: "us-east-1"}

origCfg := toolCfg
defer func() { toolCfg = origCfg }()

toolCfg.Coverage = 100.0
toolCfg.PaymentOption = "partial-upfront"
toolCfg.TermYears = 1
toolCfg.Regions = []string{"us-east-1"}

mockClient := &MockRecommendationsClient{}
accountCache := NewAccountAliasCache(awsCfg)

// Setup mock to return 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{})

// Should continue with empty results after error
assert.Empty(t, recs)
assert.Empty(t, results)

mockClient.AssertExpectations(t)
}

func TestProcessService_AllRecommendationsFilteredOut(t *testing.T) {
ctx := context.Background()
awsCfg := aws.Config{Region: "us-east-1"}

origCfg := toolCfg
defer func() { toolCfg = origCfg }()

toolCfg.Coverage = 100.0
toolCfg.PaymentOption = "no-upfront"
toolCfg.TermYears = 1
toolCfg.Regions = []string{"us-east-1"}
toolCfg.IncludeInstanceTypes = []string{"db.r5.large"} // Filter to specific type

mockClient := &MockRecommendationsClient{}
accountCache := NewAccountAliasCache(awsCfg)

// 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, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil)

recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{})

// All recommendations should be filtered out
assert.Empty(t, recs)
assert.Empty(t, results)

mockClient.AssertExpectations(t)
}

// ==================== Tests for filterAndAdjustRecommendations Edge Cases ====================

func TestFilterAndAdjustRecommendations_ZeroCoverage(t *testing.T) {
Expand Down
124 changes: 2 additions & 122 deletions cmd/multi_service_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"log"
"maps"
"slices"
"sort"
Expand All @@ -18,7 +17,6 @@ import (
azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure"
"github.com/aws/aws-sdk-go-v2/aws"
awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2"
"github.com/google/uuid"
)

// EC2ClientInterface defines the interface for EC2 operations.
Expand Down Expand Up @@ -137,11 +135,6 @@ func discoverRegionsForService(ctx context.Context, client provider.Recommendati
return regions, nil
}

// applyCommonCoverage applies coverage percentage to recommendations.
func applyCommonCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation {
return ApplyCoverage(recs, coverage)
}

// determineServicesToProcess returns the list of services to process based on flags.
func determineServicesToProcess(cfg Config) ([]common.ServiceType, error) {
var services []common.ServiceType
Expand Down Expand Up @@ -458,118 +451,6 @@ func requireRegionInventory(recs []common.Recommendation, cfg Config, data engin
return fmt.Errorf("extended-support exclusion cannot be applied: RDS inventory unavailable in %s (re-run, or pass --include-extended-support to skip the check)", describeFailedRegions(affected))
}

// regionRecommendations holds the processed recommendations for a single region.
type regionRecommendations struct {
recommendations []common.Recommendation
results []common.PurchaseResult
auditErr error // errAuditTrailLost: stop buying
}

// processRegionRecommendations fetches and processes recommendations for a single region.
func processRegionRecommendations(
ctx context.Context,
awsCfg aws.Config,
recClient provider.RecommendationsClient,
accountCache *AccountAliasCache,
service common.ServiceType,
region string,
regionIndex, totalRegions int,
engineData engineVersionData,
isDryRun bool,
cfg Config,
coverageMap recommendations.PoolCoverageMap,
) regionRecommendations {
result := regionRecommendations{
recommendations: make([]common.Recommendation, 0),
results: make([]common.PurchaseResult, 0),
}

AppLogger.Printf("\n 📍 [%d/%d] Region: %s\n", regionIndex, totalRegions, region)

// Fetch recommendations
recs, fetchErr := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg)
if fetchErr != nil {
// Legacy test-only path: the CLI run goes through fetchAndFilterRegionRecs,
// which returns this error to the caller.
AppLogger.Printf(" ❌ Failed to fetch recommendations: %v\n", fetchErr)
return result
}
if len(recs) == 0 {
AppLogger.Printf(" ℹ️ No recommendations found\n")
return result
}

AppLogger.Printf(" ✅ Found %d recommendations\n", len(recs))

// Populate account names
if err := populateRecommendationAccountNames(ctx, recs, accountCache, cfg); err != nil {
AppLogger.Printf(" ❌ %v\n", err)
return result
}

// Apply filters (drops not tracked on this legacy test-only path).
recs = applyRegionFilters(recs, engineData, region, cfg, nil)
if len(recs) == 0 {
AppLogger.Printf(" ℹ️ No recommendations after applying filters\n")
return result
}

// Apply coverage and overrides. This legacy path (test-only) doesn't
// fetch expiring commitments — expiry-aware sizing only runs via the
// main pipeline in fetchAndFilterRegionRecs, which has access to the
// regional service client at the right moment. Drop tracking skipped (nil).
// nil commitments: no expiry deduction, so family sizing is identical to
// ApplyFamilyNUSizingRDS (documented on applyCoverageAndOverrides).
filteredRecs := applyCoverageAndOverrides(recs, cfg, coverageMap, nil, nil)

result.recommendations = filteredRecs

// --max-instances is a run-wide cap, and this legacy per-region entry point
// has no view of the other services and regions in the run, so it cannot
// evaluate the cap. Refuse to spend rather than purchase uncapped: an
// over-purchase of reserved capacity is not reversible. Dry runs continue
// so the recommendations are still reported.
//
// This is deliberately the first thing after the recommendations are
// recorded, ahead of building the service client and of the duplicate
// check. The decision depends only on cfg.MaxInstances and isDryRun, both
// already known, so reaching it through a cloud API call would be work
// done to arrive at an answer that was already determined -- and it would
// make the refusal path fail differently depending on whether the describe
// call happened to succeed.
if cfg.MaxInstances > 0 && !isDryRun {
log.Printf("❌ Refusing to purchase %s/%s: --max-instances is a run-wide cap and cannot be enforced on the per-region path. Use the multi-service pipeline (the default entry point).",
getServiceDisplayName(service), region)
return result
}

// Get service client and process purchases
regionalCfg := awsCfg.Copy()
regionalCfg.Region = region
serviceClient := createServiceClient(service, regionalCfg)

if serviceClient == nil {
AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service))
AppLogger.Printf(" (Skipping purchase phase for this service)\n")
return result
}

// Check for duplicate RIs. Drop tracking skipped (nil).
adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, isDryRun, nil)

// Process purchases. This legacy per-region entry point has no run-wide
// runID of its own (unlike runToolMultiService/runToolFromCSV, which mint
// one per invocation), so each call gets its own -- every recommendation
// it processes still ends up in cfg.AuditLog with a durable, groupable
// record; it is simply not grouped with a sibling region's run.
runID := uuid.New().String()
regionResults, auditErr := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg, runID)
result.results = regionResults
result.auditErr = auditErr

return result
}

// errRecommendationFetch marks a failure to obtain recommendations from the
// provider, as opposed to a successful but empty answer.
var errRecommendationFetch = errors.New("recommendation fetch failed")
Expand Down Expand Up @@ -786,9 +667,8 @@ func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays, look
// purchase run: that would buy reserved capacity the account already owns.
// isDryRun selects the behavior on error: a dry run logs a loud warning and
// continues (nothing is bought, so reporting fidelity wins), while a purchase
// run refuses to spend and drops these recommendations entirely, mirroring
// the "refuse to spend rather than purchase uncapped" stance already taken
// for --max-instances in processRegionRecommendations.
// run refuses to spend and drops these recommendations entirely: an
// over-purchase of reserved capacity is not reversible.
//
// drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip.
//
Expand Down
18 changes: 16 additions & 2 deletions cmd/multi_service_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func TestGetServiceDisplayName(t *testing.T) {
}
}

func TestApplyCommonCoverage(t *testing.T) {
func TestApplyCoverage_FloorsPerRecommendationCounts(t *testing.T) {
recs := []common.Recommendation{
{Count: 10, EstimatedSavings: 100},
{Count: 5, EstimatedSavings: 50},
Expand Down Expand Up @@ -264,7 +264,7 @@ func TestApplyCommonCoverage(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := applyCommonCoverage(recs, tt.coverage)
result := ApplyCoverage(recs, tt.coverage)
assert.Equal(t, tt.expectedCount, len(result))

for i, rec := range result {
Expand Down Expand Up @@ -883,3 +883,17 @@ func TestDetermineServicesToProcess_SavingsPlanTypeFilters(t *testing.T) {
})
}
}

// Moved from the deleted TestProcessService_WithOverrideCount: the override is
// applied by applyCoverageAndOverrides, which the live pipeline also calls.
func TestApplyCoverageAndOverrides_OverrideCount(t *testing.T) {
recs := []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},
}
got := applyCoverageAndOverrides(recs, Config{Coverage: 100, OverrideCount: 3}, nil, nil, nil)
assert.Len(t, got, 2)
for _, rec := range got {
assert.Equal(t, 3, rec.Count)
}
}
Loading
Loading