Skip to content

Commit ee2b597

Browse files
cristimclaude
andauthored
chore(cli): delete the unused legacy processService purchase path (#2254)
processService and processRegionRecommendations (plus the regionRecommendations type) had no production caller; only tests drove them. Since #2105 moved confirmation to confirmPurchaseRun they would purchase without confirmation if anything called them again. Delete them and the tests that only exercised them. Also drop the test-only applyCommonCoverage wrapper (its assertions now call ApplyCoverage directly) and the never-read Config.Providers field. The override-count assertion moves to a test of applyCoverageAndOverrides, which the live pipeline shares. processPurchaseLoop stays: the live pipeline and --input-csv use it. Closes #2110 Closes #2053 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 7e885a2 commit ee2b597

6 files changed

Lines changed: 18 additions & 584 deletions

‎cmd/main.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ type Config struct {
5151
Regions []string
5252
Services []string
5353
ExcludeAccounts []string
54-
Providers []string
5554
IncludeRegions []string
5655
ExcludeRegions []string
5756
IncludeInstanceTypes []string

‎cmd/multi_service.go‎

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -960,38 +960,6 @@ func requireFilterInputs(recs []common.Recommendation, cfg Config, engineData en
960960
return requireResolvedAccountNames(recs, &cfg)
961961
}
962962

963-
// processService processes a single service and returns recommendations and results.
964-
// Used by legacy callers; new code should use fetchAllRecs + executePurchasePipeline.
965-
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
966-
regionsToProcess, err := determineRegionsForService(ctx, recClient, service, cfg)
967-
if err != nil {
968-
log.Printf("❌ Failed to determine regions: %v", err)
969-
return nil, nil
970-
}
971-
972-
serviceRecs := make([]common.Recommendation, 0)
973-
serviceResults := make([]common.PurchaseResult, 0)
974-
975-
for i, region := range regionsToProcess {
976-
// Legacy single-service entry point — no coverage map is fetched here,
977-
// so sizing falls back to the no-existing-commitments formula. The new
978-
// path (runToolMultiService) fetches coverage once and threads it through.
979-
regionResult := processRegionRecommendations(
980-
ctx, awsCfg, recClient, accountCache,
981-
service, region, i+1, len(regionsToProcess),
982-
engineData, isDryRun, cfg, nil,
983-
)
984-
serviceRecs = append(serviceRecs, regionResult.recommendations...)
985-
serviceResults = append(serviceResults, regionResult.results...)
986-
if regionResult.auditErr != nil {
987-
log.Printf("%s", auditStopMessage(len(serviceResults), len(serviceRecs), serviceResults, regionResult.auditErr))
988-
break
989-
}
990-
}
991-
992-
return serviceRecs, serviceResults
993-
}
994-
995963
// processPurchaseLoop processes purchases for a single region (used by CSV
996964
// mode). runID groups every recommendation processed across the whole CSV
997965
// run into one audit trail, matching how executePurchasePipeline (the main

‎cmd/multi_service_coverage_test.go‎

Lines changed: 0 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
awsrds "github.com/aws/aws-sdk-go-v2/service/rds"
1414
rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types"
1515
"github.com/stretchr/testify/assert"
16-
"github.com/stretchr/testify/mock"
1716
"github.com/stretchr/testify/require"
1817
)
1918

@@ -624,98 +623,6 @@ func TestValidateFlags_CSVPaths(t *testing.T) {
624623
})
625624
}
626625

627-
// ==================== Tests for processService Error Paths ====================
628-
629-
func TestProcessService_GetRegionsError(t *testing.T) {
630-
ctx := context.Background()
631-
awsCfg := aws.Config{Region: "us-east-1"}
632-
633-
origCfg := toolCfg
634-
defer func() { toolCfg = origCfg }()
635-
636-
toolCfg.Coverage = 100.0
637-
toolCfg.PaymentOption = "all-upfront"
638-
toolCfg.TermYears = 3
639-
toolCfg.Regions = []string{} // Empty - will trigger auto-discovery
640-
641-
mockClient := &MockRecommendationsClient{}
642-
accountCache := NewAccountAliasCache(awsCfg)
643-
644-
// This test verifies behavior when region discovery is needed
645-
// Since getAllAWSRegions requires real AWS config, we test with explicit regions instead
646-
toolCfg.Regions = []string{"us-east-1"}
647-
648-
// Setup mock to return empty recommendations
649-
mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation{}, nil)
650-
651-
recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{})
652-
653-
// Should return empty recommendations
654-
assert.Empty(t, recs)
655-
assert.Empty(t, results)
656-
657-
mockClient.AssertExpectations(t)
658-
}
659-
660-
func TestProcessService_GetRecommendationsError(t *testing.T) {
661-
ctx := context.Background()
662-
awsCfg := aws.Config{Region: "us-east-1"}
663-
664-
origCfg := toolCfg
665-
defer func() { toolCfg = origCfg }()
666-
667-
toolCfg.Coverage = 100.0
668-
toolCfg.PaymentOption = "partial-upfront"
669-
toolCfg.TermYears = 1
670-
toolCfg.Regions = []string{"us-east-1"}
671-
672-
mockClient := &MockRecommendationsClient{}
673-
accountCache := NewAccountAliasCache(awsCfg)
674-
675-
// Setup mock to return error
676-
mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation(nil), errors.New("API error"))
677-
678-
recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceEC2, true, toolCfg, engineVersionData{})
679-
680-
// Should continue with empty results after error
681-
assert.Empty(t, recs)
682-
assert.Empty(t, results)
683-
684-
mockClient.AssertExpectations(t)
685-
}
686-
687-
func TestProcessService_AllRecommendationsFilteredOut(t *testing.T) {
688-
ctx := context.Background()
689-
awsCfg := aws.Config{Region: "us-east-1"}
690-
691-
origCfg := toolCfg
692-
defer func() { toolCfg = origCfg }()
693-
694-
toolCfg.Coverage = 100.0
695-
toolCfg.PaymentOption = "no-upfront"
696-
toolCfg.TermYears = 1
697-
toolCfg.Regions = []string{"us-east-1"}
698-
toolCfg.IncludeInstanceTypes = []string{"db.r5.large"} // Filter to specific type
699-
700-
mockClient := &MockRecommendationsClient{}
701-
accountCache := NewAccountAliasCache(awsCfg)
702-
703-
// Return recommendations that don't match the filter
704-
mockRecs := []common.Recommendation{
705-
{ResourceType: "db.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 100},
706-
{ResourceType: "db.t3.medium", Count: 3, Region: "us-east-1", EstimatedSavings: 200},
707-
}
708-
mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil)
709-
710-
recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{})
711-
712-
// All recommendations should be filtered out
713-
assert.Empty(t, recs)
714-
assert.Empty(t, results)
715-
716-
mockClient.AssertExpectations(t)
717-
}
718-
719626
// ==================== Tests for filterAndAdjustRecommendations Edge Cases ====================
720627

721628
func TestFilterAndAdjustRecommendations_ZeroCoverage(t *testing.T) {

‎cmd/multi_service_helpers.go‎

Lines changed: 2 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"context"
55
"errors"
66
"fmt"
7-
"log"
87
"maps"
98
"slices"
109
"sort"
@@ -18,7 +17,6 @@ import (
1817
azureprovider "github.com/LeanerCloud/cloud-commitments-go/providers/azure"
1918
"github.com/aws/aws-sdk-go-v2/aws"
2019
awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2"
21-
"github.com/google/uuid"
2220
)
2321

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

140-
// applyCommonCoverage applies coverage percentage to recommendations.
141-
func applyCommonCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation {
142-
return ApplyCoverage(recs, coverage)
143-
}
144-
145138
// determineServicesToProcess returns the list of services to process based on flags.
146139
func determineServicesToProcess(cfg Config) ([]common.ServiceType, error) {
147140
var services []common.ServiceType
@@ -458,118 +451,6 @@ func requireRegionInventory(recs []common.Recommendation, cfg Config, data engin
458451
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))
459452
}
460453

461-
// regionRecommendations holds the processed recommendations for a single region.
462-
type regionRecommendations struct {
463-
recommendations []common.Recommendation
464-
results []common.PurchaseResult
465-
auditErr error // errAuditTrailLost: stop buying
466-
}
467-
468-
// processRegionRecommendations fetches and processes recommendations for a single region.
469-
func processRegionRecommendations(
470-
ctx context.Context,
471-
awsCfg aws.Config,
472-
recClient provider.RecommendationsClient,
473-
accountCache *AccountAliasCache,
474-
service common.ServiceType,
475-
region string,
476-
regionIndex, totalRegions int,
477-
engineData engineVersionData,
478-
isDryRun bool,
479-
cfg Config,
480-
coverageMap recommendations.PoolCoverageMap,
481-
) regionRecommendations {
482-
result := regionRecommendations{
483-
recommendations: make([]common.Recommendation, 0),
484-
results: make([]common.PurchaseResult, 0),
485-
}
486-
487-
AppLogger.Printf("\n 📍 [%d/%d] Region: %s\n", regionIndex, totalRegions, region)
488-
489-
// Fetch recommendations
490-
recs, fetchErr := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg)
491-
if fetchErr != nil {
492-
// Legacy test-only path: the CLI run goes through fetchAndFilterRegionRecs,
493-
// which returns this error to the caller.
494-
AppLogger.Printf(" ❌ Failed to fetch recommendations: %v\n", fetchErr)
495-
return result
496-
}
497-
if len(recs) == 0 {
498-
AppLogger.Printf(" ℹ️ No recommendations found\n")
499-
return result
500-
}
501-
502-
AppLogger.Printf(" ✅ Found %d recommendations\n", len(recs))
503-
504-
// Populate account names
505-
if err := populateRecommendationAccountNames(ctx, recs, accountCache, cfg); err != nil {
506-
AppLogger.Printf(" ❌ %v\n", err)
507-
return result
508-
}
509-
510-
// Apply filters (drops not tracked on this legacy test-only path).
511-
recs = applyRegionFilters(recs, engineData, region, cfg, nil)
512-
if len(recs) == 0 {
513-
AppLogger.Printf(" ℹ️ No recommendations after applying filters\n")
514-
return result
515-
}
516-
517-
// Apply coverage and overrides. This legacy path (test-only) doesn't
518-
// fetch expiring commitments — expiry-aware sizing only runs via the
519-
// main pipeline in fetchAndFilterRegionRecs, which has access to the
520-
// regional service client at the right moment. Drop tracking skipped (nil).
521-
// nil commitments: no expiry deduction, so family sizing is identical to
522-
// ApplyFamilyNUSizingRDS (documented on applyCoverageAndOverrides).
523-
filteredRecs := applyCoverageAndOverrides(recs, cfg, coverageMap, nil, nil)
524-
525-
result.recommendations = filteredRecs
526-
527-
// --max-instances is a run-wide cap, and this legacy per-region entry point
528-
// has no view of the other services and regions in the run, so it cannot
529-
// evaluate the cap. Refuse to spend rather than purchase uncapped: an
530-
// over-purchase of reserved capacity is not reversible. Dry runs continue
531-
// so the recommendations are still reported.
532-
//
533-
// This is deliberately the first thing after the recommendations are
534-
// recorded, ahead of building the service client and of the duplicate
535-
// check. The decision depends only on cfg.MaxInstances and isDryRun, both
536-
// already known, so reaching it through a cloud API call would be work
537-
// done to arrive at an answer that was already determined -- and it would
538-
// make the refusal path fail differently depending on whether the describe
539-
// call happened to succeed.
540-
if cfg.MaxInstances > 0 && !isDryRun {
541-
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).",
542-
getServiceDisplayName(service), region)
543-
return result
544-
}
545-
546-
// Get service client and process purchases
547-
regionalCfg := awsCfg.Copy()
548-
regionalCfg.Region = region
549-
serviceClient := createServiceClient(service, regionalCfg)
550-
551-
if serviceClient == nil {
552-
AppLogger.Printf(" ⚠️ Service client not yet implemented for %s\n", getServiceDisplayName(service))
553-
AppLogger.Printf(" (Skipping purchase phase for this service)\n")
554-
return result
555-
}
556-
557-
// Check for duplicate RIs. Drop tracking skipped (nil).
558-
adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, isDryRun, nil)
559-
560-
// Process purchases. This legacy per-region entry point has no run-wide
561-
// runID of its own (unlike runToolMultiService/runToolFromCSV, which mint
562-
// one per invocation), so each call gets its own -- every recommendation
563-
// it processes still ends up in cfg.AuditLog with a durable, groupable
564-
// record; it is simply not grouped with a sibling region's run.
565-
runID := uuid.New().String()
566-
regionResults, auditErr := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg, runID)
567-
result.results = regionResults
568-
result.auditErr = auditErr
569-
570-
return result
571-
}
572-
573454
// errRecommendationFetch marks a failure to obtain recommendations from the
574455
// provider, as opposed to a successful but empty answer.
575456
var errRecommendationFetch = errors.New("recommendation fetch failed")
@@ -786,9 +667,8 @@ func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays, look
786667
// purchase run: that would buy reserved capacity the account already owns.
787668
// isDryRun selects the behavior on error: a dry run logs a loud warning and
788669
// continues (nothing is bought, so reporting fidelity wins), while a purchase
789-
// run refuses to spend and drops these recommendations entirely, mirroring
790-
// the "refuse to spend rather than purchase uncapped" stance already taken
791-
// for --max-instances in processRegionRecommendations.
670+
// run refuses to spend and drops these recommendations entirely: an
671+
// over-purchase of reserved capacity is not reversible.
792672
//
793673
// drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip.
794674
//

‎cmd/multi_service_helpers_test.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func TestGetServiceDisplayName(t *testing.T) {
223223
}
224224
}
225225

226-
func TestApplyCommonCoverage(t *testing.T) {
226+
func TestApplyCoverage_FloorsPerRecommendationCounts(t *testing.T) {
227227
recs := []common.Recommendation{
228228
{Count: 10, EstimatedSavings: 100},
229229
{Count: 5, EstimatedSavings: 50},
@@ -264,7 +264,7 @@ func TestApplyCommonCoverage(t *testing.T) {
264264

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

270270
for i, rec := range result {
@@ -883,3 +883,17 @@ func TestDetermineServicesToProcess_SavingsPlanTypeFilters(t *testing.T) {
883883
})
884884
}
885885
}
886+
887+
// Moved from the deleted TestProcessService_WithOverrideCount: the override is
888+
// applied by applyCoverageAndOverrides, which the live pipeline also calls.
889+
func TestApplyCoverageAndOverrides_OverrideCount(t *testing.T) {
890+
recs := []common.Recommendation{
891+
{ResourceType: "cache.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100},
892+
{ResourceType: "cache.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 200},
893+
}
894+
got := applyCoverageAndOverrides(recs, Config{Coverage: 100, OverrideCount: 3}, nil, nil, nil)
895+
assert.Len(t, got, 2)
896+
for _, rec := range got {
897+
assert.Equal(t, 3, rec.Count)
898+
}
899+
}

0 commit comments

Comments
 (0)