From d99296b4a8e32dae749fb7bf09a5cb4329d8c92b Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 08:57:15 +0000 Subject: [PATCH 1/2] chore(deps): bump cloud-commitments-go to main 80e9b846 The platform pinned pkg to a commit from before go#614, so Allocate's NetExistingUSDPerHour was not available to the ladder handler (#243). Move pkg and providers/{aws,azure,gcp} to go main, which also brings in go#441, #471, #491, #439, #627, #628 and #629. The bump needs three mechanical follow-ups: - go#575 dropped the ctx argument from computeengine.NewClient; the GCP history-cost test is updated to the new signature. - go#603/#609 renamed provider.ProviderConfig to provider.Config and left the old name as a deprecated alias. staticcheck SA1019 flags every use, so all references move to provider.Config. mock.AnythingOfType reports the real type name, so the zonal-recs mock now matches "*provider.Config". - The providerfactory bypass guard matched only the ProviderConfig name; it now recognises both names so a Profile literal is still caught. Co-Authored-By: Claude Opus 5.5 --- go.mod | 8 +++--- go.sum | 16 ++++++------ ...combined_plan_purchase_integration_test.go | 2 +- ..._purchases_pause_claim_integration_test.go | 2 +- .../handler_purchases_retry_fanout_test.go | 2 +- ...dler_purchases_run_now_integration_test.go | 2 +- ...dations_global_filters_integration_test.go | 2 +- ...ecommendation_identity_integration_test.go | 2 +- internal/providerfactory/bypass_guard_test.go | 12 ++++++--- internal/providerfactory/providerfactory.go | 6 ++--- .../providerfactory/providerfactory_test.go | 8 +++--- .../azure_savings_plans_alias_proof_test.go | 2 +- internal/purchase/execution.go | 26 +++++++++---------- internal/purchase/execution_test.go | 6 ++--- internal/purchase/gcp_history_cost_test.go | 2 +- internal/purchase/mocks_test.go | 2 +- internal/purchase/plan_step_proofs_test.go | 2 +- ...mendation_completeness_integration_test.go | 4 +-- internal/scheduler/scheduler.go | 4 +-- internal/scheduler/scheduler_test.go | 10 +++---- internal/scheduler/zonal_recs_test.go | 6 ++--- 21 files changed, 65 insertions(+), 61 deletions(-) diff --git a/go.mod b/go.mod index 0844b6cb3..b44d9e598 100644 --- a/go.mod +++ b/go.mod @@ -84,10 +84,10 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020053130-ff6225d5ff04 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020071945-92722e673741 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026081621-80e9b8464d2a + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026081621-80e9b8464d2a + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026081621-80e9b8464d2a + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026081621-80e9b8464d2a github.com/aws/aws-lambda-go v1.47.0 github.com/aws/aws-sdk-go-v2/service/kms v1.50.4 github.com/aws/aws-sdk-go-v2/service/lambda v1.89.0 diff --git a/go.sum b/go.sum index 021297bb2..a0875fe60 100644 --- a/go.sum +++ b/go.sum @@ -92,14 +92,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020053130-ff6225d5ff04 h1:D595qtoMszl7dkETbmVFzn+WPECQ7HuiKnsMlFI7bLA= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020053130-ff6225d5ff04/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020071945-92722e673741 h1:AlfTl8EpfcRIWjU1FIx/XN+4ZgJjktjP/gPlRylPoF8= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020071945-92722e673741/go.mod h1:cx+GF1J6RKy0CTOs2qhy+DM9rwFbKf3g0JIhYwqWz9k= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617 h1:K9wBX9ac1QwhkjW7BvDt0gl2Kt9fGdDytJyndoPRcBU= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617/go.mod h1:iOwl8e9EKGb3VQ4u/YjiqVzglhD6/GX1ABC3tBkqu0A= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617 h1:VYVbQyoR+tIcPoXhkviclexKuU5H+A21hSXudB+kZag= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617/go.mod h1:03vjHt5wlHtZEFAFOhosA+paDSqacWnKg9cVa68QnJ0= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026081621-80e9b8464d2a h1:AdVEkJsrrE1i7a7J5pWh3fUZZ65VYacs8dq69EpMGWk= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261026081621-80e9b8464d2a/go.mod h1:rEJErEsL40Np/ttf0RDoNNC2XQsN1GmWyFf34y0JLD8= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026081621-80e9b8464d2a h1:buZASfVGddhmva+kSO3QbwIw0USNo0zILx64TP0tN44= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261026081621-80e9b8464d2a/go.mod h1:yVHEL5ceNS/CsDatzLH0+Qmwtoi7yY4tIHvqyvhdtig= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026081621-80e9b8464d2a h1:Ih+d9iCRAQErjoJHnlJlFB3zlgVZF1O7WFgwM/xB5PA= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261026081621-80e9b8464d2a/go.mod h1:zA51HKXtBvHHMYd+owRDmn4p8ialZrLFzI9I052uUmo= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026081621-80e9b8464d2a h1:nnyJsdoxgR+tqmMbh5L5/HtT6xn5p6Axz2OhphbCa+Y= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261026081621-80e9b8464d2a/go.mod h1:/HxBqXBb5v69+EHLsibHnekkEYRP8TvyCmT6d9QRxt0= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= github.com/Microsoft/go-winio v0.6.2/go.mod h1:yd8OoFMLzJbo9gZq8j5qaps8bJ9aShtEA8Ipt1oGCvU= github.com/agext/levenshtein v1.2.1 h1:QmvMAjj2aEICytGiWzmxoE0x2KZvE0fvmqMOfy2tjT8= diff --git a/internal/api/combined_plan_purchase_integration_test.go b/internal/api/combined_plan_purchase_integration_test.go index c35a15cec..f3078c803 100644 --- a/internal/api/combined_plan_purchase_integration_test.go +++ b/internal/api/combined_plan_purchase_integration_test.go @@ -86,7 +86,7 @@ func (p *combinedProvider) GetServiceClient(_ context.Context, service common.Se type combinedFactory struct{ provider *combinedProvider } -func (f combinedFactory) CreateAndValidateProvider(_ context.Context, name string, _ *provider.ProviderConfig) (provider.Provider, error) { +func (f combinedFactory) CreateAndValidateProvider(_ context.Context, name string, _ *provider.Config) (provider.Provider, error) { if name != "azure" { return nil, fmt.Errorf("fixture refuses unexpected provider %q", name) } diff --git a/internal/api/handler_purchases_pause_claim_integration_test.go b/internal/api/handler_purchases_pause_claim_integration_test.go index f327b5e4f..4c8ba032a 100644 --- a/internal/api/handler_purchases_pause_claim_integration_test.go +++ b/internal/api/handler_purchases_pause_claim_integration_test.go @@ -52,7 +52,7 @@ func (p *pauseClaimProvider) GetServiceClient(context.Context, common.ServiceTyp type pauseClaimFactory struct{ provider *pauseClaimProvider } -func (f pauseClaimFactory) CreateAndValidateProvider(context.Context, string, *provider.ProviderConfig) (provider.Provider, error) { +func (f pauseClaimFactory) CreateAndValidateProvider(context.Context, string, *provider.Config) (provider.Provider, error) { return f.provider, nil } diff --git a/internal/api/handler_purchases_retry_fanout_test.go b/internal/api/handler_purchases_retry_fanout_test.go index 4e6d2f6ff..829a1317d 100644 --- a/internal/api/handler_purchases_retry_fanout_test.go +++ b/internal/api/handler_purchases_retry_fanout_test.go @@ -162,7 +162,7 @@ func (p *fanoutProvider) GetRecommendationsClient(_ context.Context) (provider.R type fanoutProviderFactory struct{ prov *fanoutProvider } -func (f *fanoutProviderFactory) CreateAndValidateProvider(_ context.Context, _ string, _ *provider.ProviderConfig) (provider.Provider, error) { +func (f *fanoutProviderFactory) CreateAndValidateProvider(_ context.Context, _ string, _ *provider.Config) (provider.Provider, error) { return f.prov, nil } diff --git a/internal/api/handler_purchases_run_now_integration_test.go b/internal/api/handler_purchases_run_now_integration_test.go index 0660e806c..572efed06 100644 --- a/internal/api/handler_purchases_run_now_integration_test.go +++ b/internal/api/handler_purchases_run_now_integration_test.go @@ -51,7 +51,7 @@ func (p *runNowProvider) GetServiceClient(context.Context, common.ServiceType, s type runNowFactory struct{ provider *runNowProvider } -func (f runNowFactory) CreateAndValidateProvider(context.Context, string, *provider.ProviderConfig) (provider.Provider, error) { +func (f runNowFactory) CreateAndValidateProvider(context.Context, string, *provider.Config) (provider.Provider, error) { return f.provider, nil } diff --git a/internal/api/handler_recommendations_global_filters_integration_test.go b/internal/api/handler_recommendations_global_filters_integration_test.go index 4ef76c2dd..9eca9f713 100644 --- a/internal/api/handler_recommendations_global_filters_integration_test.go +++ b/internal/api/handler_recommendations_global_filters_integration_test.go @@ -29,7 +29,7 @@ type globalFilterProvider struct { provider.RecommendationsClient } -func (p globalFilterProvider) CreateAndValidateProvider(_ context.Context, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { +func (p globalFilterProvider) CreateAndValidateProvider(_ context.Context, name string, cfg *provider.Config) (provider.Provider, error) { if name != "aws" || cfg != nil { return nil, fmt.Errorf("unexpected provider request: %s", name) } diff --git a/internal/api/recommendation_identity_integration_test.go b/internal/api/recommendation_identity_integration_test.go index 5f563627d..84b1c64f3 100644 --- a/internal/api/recommendation_identity_integration_test.go +++ b/internal/api/recommendation_identity_integration_test.go @@ -44,7 +44,7 @@ type identityProviderFactory struct { recs map[string][]common.Recommendation } -func (f identityProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { +func (f identityProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.Config) (provider.Provider, error) { if name != "aws" || cfg == nil || cfg.AWSCredentialsProvider == nil { return nil, fmt.Errorf("expected registered AWS fixture credentials") } diff --git a/internal/providerfactory/bypass_guard_test.go b/internal/providerfactory/bypass_guard_test.go index 78e6f9fc3..d15ab4d70 100644 --- a/internal/providerfactory/bypass_guard_test.go +++ b/internal/providerfactory/bypass_guard_test.go @@ -29,7 +29,7 @@ var bypassConstructors = map[string]map[string]bool{ } // No non-test file under cmd/ or internal/ may call the concrete Azure/GCP -// provider constructors or set the deprecated ProviderConfig.Profile field. +// provider constructors or set the deprecated provider.Config Profile field. func TestNoRegistryBypassInProductionCode(t *testing.T) { root := filepath.Join("..", "..") var violations []string @@ -50,7 +50,7 @@ func TestNoRegistryBypassInProductionCode(t *testing.T) { } require.Greater(t, scanned, 100, "the walk must actually reach the production sources") assert.Empty(t, violations, - "build credentialed Azure/GCP providers with providerfactory.NewAzure/NewGCP and use the typed ProviderConfig fields") + "build credentialed Azure/GCP providers with providerfactory.NewAzure/NewGCP and use the typed provider.Config fields") } // registryBypasses returns one entry per bypassing call or deprecated @@ -80,7 +80,7 @@ func registryBypasses(t *testing.T, path string) []string { out = append(out, fset.Position(node.Pos()).String()+": "+node.Sel.Name) } case *ast.CompositeLit: - if sel, ok := node.Type.(*ast.SelectorExpr); ok && sel.Sel.Name == "ProviderConfig" && selectorPkg(sel, imports) == providerPkg { + if sel, ok := node.Type.(*ast.SelectorExpr); ok && providerConfigTypes[sel.Sel.Name] && selectorPkg(sel, imports) == providerPkg { out = append(out, profileKeys(fset, node)...) } } @@ -89,6 +89,10 @@ func registryBypasses(t *testing.T, path string) []string { return out } +// providerConfigTypes are the names a provider.Config literal can be written +// with: Config, and its deprecated alias ProviderConfig (go#603). +var providerConfigTypes = map[string]bool{"Config": true, "ProviderConfig": true} + func selectorPkg(sel *ast.SelectorExpr, imports map[string]string) string { id, ok := sel.X.(*ast.Ident) if !ok { @@ -105,7 +109,7 @@ func profileKeys(fset *token.FileSet, lit *ast.CompositeLit) []string { continue } if id, ok := kv.Key.(*ast.Ident); ok && id.Name == "Profile" { - out = append(out, fset.Position(kv.Pos()).String()+": ProviderConfig.Profile") + out = append(out, fset.Position(kv.Pos()).String()+": provider.Config.Profile") } } return out diff --git a/internal/providerfactory/providerfactory.go b/internal/providerfactory/providerfactory.go index bc351ef8f..516d50407 100644 --- a/internal/providerfactory/providerfactory.go +++ b/internal/providerfactory/providerfactory.go @@ -59,7 +59,7 @@ func NewGCP(projectID string, ts oauth2.TokenSource) (provider.Provider, error) func newAzure(reg *provider.Registry, subscriptionID string, cred azcore.TokenCredential) (provider.Provider, error) { // A nil cred converts to a nil `any`, which the Azure factory treats as // "no credential supplied". - return build(reg, azureName, &provider.ProviderConfig{ + return build(reg, azureName, &provider.Config{ Name: azureName, AzureSubscriptionID: subscriptionID, AzureTokenCredential: cred, @@ -73,14 +73,14 @@ func newGCP(reg *provider.Registry, projectID string, ts oauth2.TokenSource) (pr if ts == nil { return nil, ErrMissingGCPTokenSource } - return build(reg, gcpName, &provider.ProviderConfig{ + return build(reg, gcpName, &provider.Config{ Name: gcpName, GCPProjectID: projectID, GCPTokenSource: ts, }) } -func build(reg *provider.Registry, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { +func build(reg *provider.Registry, name string, cfg *provider.Config) (provider.Provider, error) { prov, err := reg.GetProviderWithConfig(name, cfg) if err != nil { return nil, fmt.Errorf("providerfactory: build %s provider: %w", name, err) diff --git a/internal/providerfactory/providerfactory_test.go b/internal/providerfactory/providerfactory_test.go index 063ee506f..7d4e28a01 100644 --- a/internal/providerfactory/providerfactory_test.go +++ b/internal/providerfactory/providerfactory_test.go @@ -36,11 +36,11 @@ func (f *fakeTokenSource) Token() (*oauth2.Token, error) { // recordingRegistry returns a private registry whose name factory records // the config it receives and returns want. -func recordingRegistry(t *testing.T, name string, want provider.Provider, wantErr error) (*provider.Registry, *[]*provider.ProviderConfig) { +func recordingRegistry(t *testing.T, name string, want provider.Provider, wantErr error) (*provider.Registry, *[]*provider.Config) { t.Helper() reg := provider.NewRegistry() - var got []*provider.ProviderConfig - require.NoError(t, reg.Register(name, func(cfg *provider.ProviderConfig) (provider.Provider, error) { + var got []*provider.Config + require.NoError(t, reg.Register(name, func(cfg *provider.Config) (provider.Provider, error) { got = append(got, cfg) return want, wantErr })) @@ -147,7 +147,7 @@ func TestBuild_PropagatesFactoryErrorsAndNilProviders(t *testing.T) { func TestNewAzure_MatchesLegacyConstruction(t *testing.T) { cred := &fakeAzureCred{id: "legacy"} - legacy, err := azureprovider.NewAzureProvider(&provider.ProviderConfig{Profile: testSubscriptionID}) //nolint:staticcheck // SA1019: this is the legacy baseline being compared against + legacy, err := azureprovider.NewAzureProvider(&provider.Config{Profile: testSubscriptionID}) //nolint:staticcheck // SA1019: this is the legacy baseline being compared against require.NoError(t, err) legacy.SetCredential(cred) diff --git a/internal/purchase/azure_savings_plans_alias_proof_test.go b/internal/purchase/azure_savings_plans_alias_proof_test.go index 8453c1788..68150e229 100644 --- a/internal/purchase/azure_savings_plans_alias_proof_test.go +++ b/internal/purchase/azure_savings_plans_alias_proof_test.go @@ -107,7 +107,7 @@ func purchaseThroughPinnedAzureClient(t *testing.T, exec *config.PurchaseExecuti prov.On("GetServiceClient", mock.Anything, common.ServiceSavingsPlansAll, mock.Anything).Return(provider.ServiceClient(client), nil) m := &Manager{config: store, email: sender, providerFactory: factory} - _, _, errs, err := m.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "p"}, "acct", (*provider.ProviderConfig)(nil), "") + _, _, errs, err := m.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "p"}, "acct", (*provider.Config)(nil), "") require.NoError(t, err) require.Empty(t, errs) for i, rec := range exec.Recommendations { diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 16679db4b..5dc938d83 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -159,7 +159,7 @@ func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.Purchas // (issue #97; see fan_out_shape.go). Every error it returns is before // submission. Split out of executeSingleAccount to keep that function under // the gocyclo budget. -func (m *Manager) prepareSingleAccountSubmission(ctx context.Context, exec *config.PurchaseExecution) (*provider.ProviderConfig, purchaseTarget, error) { +func (m *Manager) prepareSingleAccountSubmission(ctx context.Context, exec *config.PurchaseExecution) (*provider.Config, purchaseTarget, error) { if err := requireRecsMatchRowAccount(exec); err != nil { return nil, purchaseTarget{}, beforeSubmission(err) } @@ -445,7 +445,7 @@ func applyAccountOutcome(acctExec *config.PurchaseExecution, purchaseErrors []st // resolved (lookup failed, the account does not exist, or credentials could // not be derived); the caller must NOT fall back to ambient credentials on // error, as that would purchase/stamp against the wrong account (#646). -func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config.PurchaseExecution) (*provider.ProviderConfig, purchaseTarget, error) { +func (m *Manager) resolveSingleAccountProvider(ctx context.Context, exec *config.PurchaseExecution) (*provider.Config, purchaseTarget, error) { // Skip credential resolution when nothing is selected. Approval-only // flows may carry an account ID on the recs solely for the contact-email // gate; attempting resolution there would fail on accounts with no @@ -499,14 +499,14 @@ func (e *partialPurchaseError) Error() string { return "some purchases failed (partial success): " + strings.Join(e.errors, "; ") } -// resolveAccountProvider returns a *ProviderConfig with a pre-authenticated provider +// resolveAccountProvider returns a *provider.Config with a pre-authenticated provider // for the given account. Returns an error if credential resolution fails -- callers // must NOT fall back to ambient credentials on error. -func (m *Manager) resolveAccountProvider(ctx context.Context, account config.CloudAccount) (*provider.ProviderConfig, error) { +func (m *Manager) resolveAccountProvider(ctx context.Context, account config.CloudAccount) (*provider.Config, error) { t0 := time.Now() logging.Infof("purchase[resolveAccountProvider]: resolving credentials for provider=%s account=%s", account.Provider, account.ID) - var cfg *provider.ProviderConfig + var cfg *provider.Config var err error switch account.Provider { case "aws": @@ -528,7 +528,7 @@ func (m *Manager) resolveAccountProvider(ctx context.Context, account config.Clo return cfg, nil } -func (m *Manager) resolveAWSProvider(ctx context.Context, account config.CloudAccount) (*provider.ProviderConfig, error) { +func (m *Manager) resolveAWSProvider(ctx context.Context, account config.CloudAccount) (*provider.Config, error) { t0 := time.Now() logging.Infof("purchase[resolveAWSProvider]: resolving AWS credentials for account=%s authMode=%s", account.ID, account.AWSAuthMode) @@ -544,10 +544,10 @@ func (m *Manager) resolveAWSProvider(ctx context.Context, account config.CloudAc } logging.Infof("purchase[resolveAWSProvider]: AWS credentials resolved for account=%s in %s", account.ID, time.Since(t0)) - return &provider.ProviderConfig{Name: "aws", AWSCredentialsProvider: awsCreds}, nil + return &provider.Config{Name: "aws", AWSCredentialsProvider: awsCreds}, nil } -func (m *Manager) resolveAzureProvider(ctx context.Context, account config.CloudAccount) (*provider.ProviderConfig, error) { +func (m *Manager) resolveAzureProvider(ctx context.Context, account config.CloudAccount) (*provider.Config, error) { t0 := time.Now() logging.Infof("purchase[resolveAzureProvider]: resolving Azure credentials for account=%s authMode=%s", account.ID, account.AzureAuthMode) @@ -571,10 +571,10 @@ func (m *Manager) resolveAzureProvider(ctx context.Context, account config.Cloud } logging.Infof("purchase[resolveAzureProvider]: Azure credentials resolved for account=%s in %s", account.ID, time.Since(t0)) - return &provider.ProviderConfig{ProviderOverride: azProv}, nil + return &provider.Config{ProviderOverride: azProv}, nil } -func (m *Manager) resolveGCPProvider(ctx context.Context, account config.CloudAccount) (*provider.ProviderConfig, error) { +func (m *Manager) resolveGCPProvider(ctx context.Context, account config.CloudAccount) (*provider.Config, error) { t0 := time.Now() logging.Infof("purchase[resolveGCPProvider]: resolving GCP credentials for account=%s authMode=%s", account.ID, account.GCPAuthMode) @@ -604,7 +604,7 @@ func (m *Manager) resolveGCPProvider(ctx context.Context, account config.CloudAc } logging.Infof("purchase[resolveGCPProvider]: GCP credentials resolved for account=%s in %s", account.ID, time.Since(t0)) - return &provider.ProviderConfig{ProviderOverride: gcpProv}, nil + return &provider.Config{ProviderOverride: gcpProv}, nil } // getMaxAccountParallelism is a thin alias over the shared @@ -624,7 +624,7 @@ type recPurchaseOutcome struct { index int } -func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig, awsPartition string) (totalSavings, totalUpfront float64, purchaseErrors []string, procErr error) { +func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.Config, awsPartition string) (totalSavings, totalUpfront float64, purchaseErrors []string, procErr error) { // ExecutionID is carried into PurchaseOptions so executeSinglePurchase // can tag every per-rec log line with the owning exec UUID. Without // this, CloudWatch filtering by exec ID returns zero hits and a stuck @@ -1111,7 +1111,7 @@ func logRecCtxErr(executionID, recTuple string, elapsed time.Duration, recCtxErr // provCfg carries optional per-account credentials; pass nil to use ambient credentials. // opts carries execution-level metadata (the source surface) that providers stamp // onto the commitment they create. -func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.ProviderConfig, opts common.PurchaseOptions) (common.PurchaseResult, error) { +func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.Config, opts common.PurchaseOptions) (common.PurchaseResult, error) { // Per-purchase Info logs tagged with the owning execution ID so a // CloudWatch filter on the execution UUID surfaces every step of the // purchase attempt -- provider construction, service-client lookup, diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 3a75bf064..1728fcacf 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1322,7 +1322,7 @@ func TestExecuteMultiAccount_RunsAccountsInParallel(t *testing.T) { // // After the fix, singleCloudAccountIDFromRecs derives the account from the recs, // GetCloudAccount fetches the CloudAccount record, and resolveAccountProvider -// constructs a provider.ProviderConfig with explicit creds. The factory receives +// constructs a provider.Config with explicit creds. The factory receives // a non-nil provCfg, which the assertion captures via MatchedBy. func TestExecutePurchase_SingleAccount_AzureUsesResolvedCreds(t *testing.T) { ctx := context.Background() @@ -1376,7 +1376,7 @@ func TestExecutePurchase_SingleAccount_AzureUsesResolvedCreds(t *testing.T) { // Before the fix, nil was passed and Azure SDK fell back to DefaultAzureCredential. // After the fix, resolveAzureProvider populates ProviderOverride on the config. mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), "azure", - mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil && cfg.ProviderOverride != nil }), ).Return(mockProviderInst, nil) @@ -1505,7 +1505,7 @@ func TestExecutePurchase_AzureCanonicalServiceTypes(t *testing.T) { }, nil) mockFactory.On("CreateAndValidateProvider", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), "azure", - mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil && cfg.ProviderOverride != nil }), ).Return(mockProviderInst, nil) diff --git a/internal/purchase/gcp_history_cost_test.go b/internal/purchase/gcp_history_cost_test.go index 89bd76ec7..85a80bd98 100644 --- a/internal/purchase/gcp_history_cost_test.go +++ b/internal/purchase/gcp_history_cost_test.go @@ -45,7 +45,7 @@ func (fakeCommitmentsService) Close() error { return nil } // cloud-commitments-go pin before a32fd1a returned the term total (1000 here). func TestSavePurchaseHistory_GCPRecordsNoUpfront(t *testing.T) { ctx := context.Background() - client, err := computeengine.NewClient(ctx, "project", "us-central1") + client, err := computeengine.NewClient("project", "us-central1") require.NoError(t, err) client.SetCommitmentsService(fakeCommitmentsService{}) diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 729a0fc35..8d6b92631 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -22,7 +22,7 @@ type MockProviderFactory struct { mock.Mock } -func (m *MockProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { +func (m *MockProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.Config) (provider.Provider, error) { args := m.Called(ctx, name, cfg) if args.Get(0) == nil { return nil, args.Error(1) diff --git a/internal/purchase/plan_step_proofs_test.go b/internal/purchase/plan_step_proofs_test.go index c1a6ca27f..1c9c20336 100644 --- a/internal/purchase/plan_step_proofs_test.go +++ b/internal/purchase/plan_step_proofs_test.go @@ -104,7 +104,7 @@ func tokenCapture(t *testing.T, providerName string, exec *config.PurchaseExecut mu.Unlock() }).Return(common.PurchaseResult{Success: true, CommitmentID: "c"}, nil) m := &Manager{config: store, email: sender, providerFactory: factory} - _, _, errs, err := m.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "p"}, "acct", (*provider.ProviderConfig)(nil), "") + _, _, errs, err := m.processPurchaseRecommendations(ctx, exec, &config.PurchasePlan{Name: "p"}, "acct", (*provider.Config)(nil), "") require.NoError(t, err) require.Empty(t, errs) return tokens diff --git a/internal/scheduler/recommendation_completeness_integration_test.go b/internal/scheduler/recommendation_completeness_integration_test.go index 93bc72789..34c9b07da 100644 --- a/internal/scheduler/recommendation_completeness_integration_test.go +++ b/internal/scheduler/recommendation_completeness_integration_test.go @@ -233,7 +233,7 @@ func TestAWSRecommendationCompletenessPersistence(t *testing.T) { store := config.NewPostgresStore(container.DB) factory := new(MockProviderFactory) fixture := &completenessHTTP{resourceType: "db.t3.medium", details: tc.details, fallback: tc.fallback, failScope: tc.failScope, spMode: tc.spMode, spFallback: tc.spFallback} - factory.On("CreateAndValidateProvider", mock.Anything, "aws", (*provider.ProviderConfig)(nil)). + factory.On("CreateAndValidateProvider", mock.Anything, "aws", (*provider.Config)(nil)). Return(completenessProvider(t, fixture), nil).Once() var accountID *string var completeAccountID string @@ -257,7 +257,7 @@ func TestAWSRecommendationCompletenessPersistence(t *testing.T) { if tc.spMode != "" { cleanFixture = &completenessHTTP{spMode: "valid"} } - factory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { return cfg != nil })). + factory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil })). Return(completenessProvider(t, cleanFixture), nil).Once() } seed := []config.RecommendationRecord{ diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 6e67335e2..76331a701 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -786,7 +786,7 @@ func (s *Scheduler) resolveAmbientAccountID(ctx context.Context, providerName, e // subscriptionID is passed explicitly to avoid an unnecessary Azure API round-trip // to auto-discover subscriptions — the caller already resolved it from env. func (s *Scheduler) collectAzureAmbient(ctx context.Context, subscriptionID string) ([]config.RecommendationRecord, *sweepGap, error) { - prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "azure", &provider.ProviderConfig{ + prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "azure", &provider.Config{ AzureSubscriptionID: subscriptionID, }) if err != nil { @@ -851,7 +851,7 @@ func (s *Scheduler) collectAWSForAccount(ctx context.Context, globalCfg *config. if err != nil { return nil, nil, fmt.Errorf("resolve credentials: %w", err) } - cfg := &provider.ProviderConfig{Name: "aws", AWSCredentialsProvider: awsCreds} + cfg := &provider.Config{Name: "aws", AWSCredentialsProvider: awsCreds} prov, err := s.providerFactory.CreateAndValidateProvider(ctx, "aws", cfg) if err != nil { return nil, nil, fmt.Errorf("create provider: %w", err) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 937109f59..69f4d547d 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -33,7 +33,7 @@ type MockProviderFactory struct { mock.Mock } -func (m *MockProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.ProviderConfig) (provider.Provider, error) { +func (m *MockProviderFactory) CreateAndValidateProvider(ctx context.Context, name string, cfg *provider.Config) (provider.Provider, error) { args := m.Called(ctx, name, cfg) if args.Get(0) == nil { return nil, args.Error(1) @@ -2123,7 +2123,7 @@ func TestScheduler_CollectAWSForAccount_SelfAccountRoleARNModeUsesAmbient(t *tes EstimatedSavings: 100.0, }, } - mockFactory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(mockProvider, nil) + mockFactory.On("CreateAndValidateProvider", ctx, "aws", (*provider.Config)(nil)).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil) mockRecClient.On("GetAllRecommendations", ctx).Return(recommendations, nil) @@ -2592,7 +2592,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_HappyPath(t *testi EstimatedSavings: 20.0, }, } - mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil && cfg.AzureSubscriptionID == "sub-abc-123" })).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", mock.Anything).Return(mockRecClient, nil) @@ -2639,7 +2639,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_NoRegisteredAccoun recommendations := []common.Recommendation{ {Provider: common.ProviderAzure, Service: common.ServiceCompute, Region: "westus", ResourceType: "Standard_B2s", Count: 1, Term: "1yr"}, } - mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil && cfg.AzureSubscriptionID == "sub-unregistered" })).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", mock.Anything).Return(mockRecClient, nil) @@ -2676,7 +2676,7 @@ func TestScheduler_CollectAzureRecommendations_AmbientTagging_StoreError(t *test recommendations := []common.Recommendation{ {Provider: common.ProviderAzure, Service: common.ServiceCompute, Region: "eastus", ResourceType: "Standard_D2s_v3", Count: 1, Term: "1yr"}, } - mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.ProviderConfig) bool { + mockFactory.On("CreateAndValidateProvider", mock.Anything, "azure", mock.MatchedBy(func(cfg *provider.Config) bool { return cfg != nil && cfg.AzureSubscriptionID == "sub-abc-123" })).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", mock.Anything).Return(mockRecClient, nil) diff --git a/internal/scheduler/zonal_recs_test.go b/internal/scheduler/zonal_recs_test.go index 8fd3f4ee2..e9087a510 100644 --- a/internal/scheduler/zonal_recs_test.go +++ b/internal/scheduler/zonal_recs_test.go @@ -142,7 +142,7 @@ func TestCollectAWSAmbient_PassesHostAccount(t *testing.T) { factory := new(MockProviderFactory) client := new(MockRecommendationsClient) client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) - factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.Config)(nil)).Return(newRecProvider(client), nil) s := &Scheduler{providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} recs, gap, err := s.collectAWSAmbient(ctx, nil) require.NoError(t, err) @@ -155,7 +155,7 @@ func TestCollectAWSForAccount_SelfAccountPassesExternalID(t *testing.T) { factory := new(MockProviderFactory) client := new(MockRecommendationsClient) client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("111111111111"), nil) - factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.ProviderConfig)(nil)).Return(newRecProvider(client), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", (*provider.Config)(nil)).Return(newRecProvider(client), nil) s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} recs, gap, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ ID: "self", Provider: "aws", AWSAuthMode: "role_arn", ExternalID: "111111111111", Enabled: true, @@ -171,7 +171,7 @@ func TestCollectAWSForAccount_AssumedRolePassesExternalID(t *testing.T) { factory := new(MockProviderFactory) client := new(MockRecommendationsClient) client.On("GetAllRecommendations", mock.Anything).Return(mixedAccountRecs("333333333333"), nil) - factory.On("CreateAndValidateProvider", ctx, "aws", mock.AnythingOfType("*provider.ProviderConfig")).Return(newRecProvider(client), nil) + factory.On("CreateAndValidateProvider", ctx, "aws", mock.AnythingOfType("*provider.Config")).Return(newRecProvider(client), nil) s := &Scheduler{config: new(MockConfigStore), providerFactory: factory, stsClient: &fakeSTSClient{accountID: "111111111111"}} recs, gap, err := s.collectAWSForAccount(ctx, nil, config.CloudAccount{ ID: "member", Provider: "aws", AWSAuthMode: "role_arn", AWSRoleARN: "arn:aws:iam::333333333333:role/cudly", From 8b64a354ce1c8a6fb82bae44c93b55cc86049a2c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 08:57:15 +0000 Subject: [PATCH 2/2] fix(server): report the net existing commitment in the ladder plan assembleLadderPlan summed each layer's gross ExistingUSDPerHour into the plan, ladder_runs.existing_usd_hr and the approval email, while Allocate sized the gap against existing minus expiring. A layer with $8/hr OD of existing of which $4/hr expires showed Existing $8/hr next to a gap of $6/hr against a $10/hr target, which does not reconcile. Report Allocate's NetExistingUSDPerHour instead (go#614), which is set on every result including the hold-only exits, so the plan shows the figure the engine used rather than a recomputed one. Closes #243 Co-Authored-By: Claude Opus 5.5 --- internal/server/handler_ladder.go | 25 +++++------ internal/server/ladder_existing_units_test.go | 45 +++++++++++++++++++ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/internal/server/handler_ladder.go b/internal/server/handler_ladder.go index c2e17a53b..22a72f005 100644 --- a/internal/server/handler_ladder.go +++ b/internal/server/handler_ladder.go @@ -456,7 +456,7 @@ func (p *ladderRunPlanner) plan(rows []config.LadderInFlightRow) (*config.Ladder if err != nil { return nil, nil, err } - plan := assembleLadderPlan(p.engineCfg.Scope, p.now, allocResult, immediate, p.layerStates, p.baseline, p.engineCfg.TargetCoveragePct, p.term, p.paymentOpt) + plan := assembleLadderPlan(p.engineCfg.Scope, p.now, allocResult, immediate, p.baseline, p.engineCfg.TargetCoveragePct, p.term, p.paymentOpt) p.allocations, p.tranches, p.holds = len(allocResult.Allocations), len(trancheResult.Tranches), len(allocResult.Holds) return p.buildRows(runID, plan, trancheResult) } @@ -601,20 +601,22 @@ func ladderConfigToEngine(dbCfg *config.LadderConfigDB, accountID string) (pkgla // The monetary snapshot is derived to stay consistent with the authoritative // allocation rather than recomputed independently: // - target = baseline low-water * targetCoveragePct/100 (the coverage goal), -// - existing = sum of per-layer ExistingUSDPerHour, already converted to -// on-demand-equivalent dollars by ladderLayerStatesOnDemand, +// - existing = Allocate's NetExistingUSDPerHour, the net (existing minus +// expiring) commitment over the configured layers that the gap was sized +// against, in the on-demand-equivalent dollars ladderLayerStatesOnDemand +// converted the layer states to. Reporting the gross per-layer sum instead +// would show a gap that does not reconcile with target - existing, // - gap = sum of the planned allocations' on-demand-equivalent gaps (the unit // of target and existing), NOT target - existing. // // When the baseline low-water is nil the run is a Hold-only no-op (Allocate // returns no allocations), so target and gap stay nil (never 0-coerced); -// existing is still reported from the observed layer states. +// existing is still reported, since Allocate sets it on every result. func assembleLadderPlan( scope pkgladder.Scope, now time.Time, allocResult *pkgladder.AllocateResult, immediate map[pkgladder.LayerType]*big.Rat, - layerStates map[pkgladder.LayerType]pkgladder.LayerState, baseline pkgladder.UsageBaseline, targetCoveragePct float64, term pkgladder.Term, @@ -646,15 +648,12 @@ func assembleLadderPlan( // Holds are informational; append last. actions = append(actions, allocResult.Holds...) - // existing = sum of per-layer on-demand-equivalent existing coverage - // (always reportable). - var existingTotal float64 - for _, ls := range layerStates { - if ls.ExistingUSDPerHour != nil { - existingTotal += *ls.ExistingUSDPerHour - } + // existing is the net figure Allocate sized the gap against (#243), copied + // so the plan does not alias the result. + var existing *big.Rat + if allocResult.NetExistingUSDPerHour != nil { + existing = new(big.Rat).Set(allocResult.NetExistingUSDPerHour) } - existing := ratFromFloat(existingTotal) // target and gap are only meaningful when the baseline is available; nil // stays nil (a Hold-only no-op has no purchase target/gap to report). diff --git a/internal/server/ladder_existing_units_test.go b/internal/server/ladder_existing_units_test.go index c38864396..faa6d2221 100644 --- a/internal/server/ladder_existing_units_test.go +++ b/internal/server/ladder_existing_units_test.go @@ -164,6 +164,51 @@ func TestExecuteLadderRun_ExpiringCoverageIsOnDemandEquivalent(t *testing.T) { assert.InDelta(t, 3.0, store.savedRun.TotalHourlyCommit, 1e-9) } +// #243: the run reports the net existing coverage Allocate sized the gap +// against, not the gross sum. Existing 4 and expiring 2 commitment at rate 0.5 +// is 8 OD gross, 4 OD net; target 10 - existing 4 = gap 6 must reconcile in +// ladder_runs and in the plan JSON the approval email is built from, on an +// allocating run and on a hold-only run alike. +func TestExecuteLadderRun_ReportsNetExistingCoverage(t *testing.T) { + cases := []struct { + name string + baseline pkgladder.UsageBaseline + wantGap bool + }{ + {"allocating run", testBaselineLowWater(12.5), true}, + {"baseline-unavailable hold", pkgladder.UsageBaseline{LookbackDays: 30, Percentile: 5}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + store := &ladderTestStore{} + cap := &fakeLadderCapability{t: t, baseline: tc.baseline, + rates: map[pkgladder.LayerType]float64{pkgladder.LayerConvertibleRI: 0.5, pkgladder.LayerEC2InstanceSP: 0.5}, + layerStates: map[pkgladder.LayerType]pkgladder.LayerState{ + pkgladder.LayerConvertibleRI: {Layer: pkgladder.LayerConvertibleRI, ExistingUSDPerHour: f64(0), ExpiringUSDPerHour: f64(0)}, + pkgladder.LayerEC2InstanceSP: spState(f64(4), f64(2)), + }} + require.NoError(t, runWithRates(t, store, cap)) + + run := store.savedRun + require.NotNil(t, run) + require.NotNil(t, run.ExistingUSDHr) + assert.InDelta(t, 4.0, *run.ExistingUSDHr, 1e-9, "existing_usd_hr is net of expiring, not the gross 8") + + var dto ladderPlanJSONDTO + require.NoError(t, json.Unmarshal(run.Plan, &dto)) + require.NotNil(t, dto.ExistingUSDHr) + assert.InDelta(t, 4.0, *dto.ExistingUSDHr, 1e-9, "plan JSON existing is the net figure") + if !tc.wantGap { + assert.Nil(t, run.GapUSDHr) + return + } + require.NotNil(t, run.GapUSDHr) + require.NotNil(t, run.TargetUSDHr) + assert.InDelta(t, *run.TargetUSDHr-*run.ExistingUSDHr, *run.GapUSDHr, 1e-9, "gap reconciles with target - existing") + }) + } +} + // Existing or expiring commitment on a layer with no rate cannot be // converted. Every such layer is reported, in layer order, with no partial // conversion for Allocate; layers that hold nothing still need no rate.