From c8118c17c91d6a22558a89f0fbe62128ccff32dd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 13:23:46 -0700 Subject: [PATCH 1/3] feat(config): persist + apply per-service min_count recommendation filter Wire the CLI --min-count filter into the server pipeline so it can be set per service and persisted, ahead of GUI exposure. - Add ServiceConfig.MinCount (migration 000068, postgres Get/Save/List, resolver). - Apply it at read time in scheduler.filterRecsByResolvedConfigs: drop recs whose net Count is below the floor; 0 disables (matches CLI 0-no-floor). - Validate MinCount (>= 0, <= MaxServiceMinCount=10000) to stop a typo silently suppressing every recommendation. - Make updateServiceConfig presence-aware: overlay the include/exclude engine/region/type lists and min_count from the request only when the body carries the key, so a partial PUT never zeroes a filter set out-of-band. - openapi: document min_count on ServiceConfig. Regression tests: scheduler drops/keeps recs by MinCount; config rejects out-of-range MinCount; merge overlays present filters, preserves omitted ones, and clears an explicitly-empty list. --- internal/api/coverage_gaps_test.go | 8 +- internal/api/handler_config.go | 102 +++++++++++++++--- internal/api/handler_config_test.go | 82 ++++++++++++++ internal/api/openapi.yaml | 7 ++ internal/config/constants.go | 7 ++ internal/config/resolver.go | 1 + internal/config/store_postgres.go | 12 ++- .../config/store_postgres_pgxmock_test.go | 11 +- internal/config/types.go | 7 ++ internal/config/validation.go | 13 +++ internal/config/validation_test.go | 34 ++++++ .../000068_service_configs_min_count.down.sql | 3 + .../000068_service_configs_min_count.up.sql | 12 +++ internal/scheduler/scheduler_overrides.go | 16 +++ .../scheduler/scheduler_overrides_test.go | 51 +++++++++ 15 files changed, 337 insertions(+), 29 deletions(-) create mode 100644 internal/database/postgres/migrations/000068_service_configs_min_count.down.sql create mode 100644 internal/database/postgres/migrations/000068_service_configs_min_count.up.sql diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index 27ed14419..fcdcd4a98 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -139,7 +139,7 @@ func TestMergeServiceConfig_NewRecord(t *testing.T) { Coverage: 80, } - result, err := mergeServiceConfig(ctx, mockStore, incoming) + result, err := mergeServiceConfig(ctx, mockStore, incoming, `{"enabled":true,"term":3,"coverage":80}`) require.NoError(t, err) assert.Equal(t, incoming, result) } @@ -166,7 +166,7 @@ func TestMergeServiceConfig_ExistingRecord(t *testing.T) { Coverage: 80.0, } - result, err := mergeServiceConfig(ctx, mockStore, incoming) + result, err := mergeServiceConfig(ctx, mockStore, incoming, `{"enabled":true,"term":3,"coverage":80}`) require.NoError(t, err) // UI-editable fields are updated @@ -186,7 +186,7 @@ func TestMergeServiceConfig_DBError(t *testing.T) { incoming := config.ServiceConfig{Provider: "aws", Service: "rds"} - _, err := mergeServiceConfig(ctx, mockStore, incoming) + _, err := mergeServiceConfig(ctx, mockStore, incoming, `{"provider":"aws","service":"rds"}`) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to read existing service config") } @@ -205,7 +205,7 @@ func TestMergeServiceConfig_NilExisting(t *testing.T) { Term: 3, } - result, err := mergeServiceConfig(ctx, mockStore, incoming) + result, err := mergeServiceConfig(ctx, mockStore, incoming, `{"enabled":true,"term":3}`) require.NoError(t, err) assert.Equal(t, incoming, result) } diff --git a/internal/api/handler_config.go b/internal/api/handler_config.go index db591f235..20946a140 100644 --- a/internal/api/handler_config.go +++ b/internal/api/handler_config.go @@ -160,15 +160,32 @@ func (h *Handler) checkCommitmentOptionCombo(ctx context.Context, cfg config.Ser return nil } -// mergeServiceConfig loads any existing service config and overlays the four -// UI-editable fields (Enabled, Term, Payment, Coverage) from cfg onto it, so -// that filter fields set outside the UI (RampSchedule, IncludeEngines, etc.) -// are not zeroed on every settings save. +// serviceConfigFilterKeys are the recommendation-filter JSON fields the +// Settings UI now edits. They are overlaid onto the existing row only when the +// request body actually contains the key, so a partial PUT from a non-UI +// client (or an older UI build) never zeroes a filter the operator set +// elsewhere. RampSchedule is intentionally absent: it is still set out-of-band +// and the UI does not own it. +var serviceConfigFilterKeys = []string{ + "include_engines", "exclude_engines", + "include_regions", "exclude_regions", + "include_types", "exclude_types", + "min_count", +} + +// mergeServiceConfig loads any existing service config and overlays the +// UI-editable fields from cfg onto it. The four always-present scalar fields +// (Enabled, Term, Payment, Coverage) are overlaid unconditionally; the +// recommendation-filter fields (include/exclude engines/regions/types, +// min_count) are overlaid only when the request body actually carried the +// corresponding key (per presentKeys), so a partial PUT cannot silently wipe a +// filter set out-of-band. body is the raw request JSON used to detect which +// filter keys are present. // // A "not found" error means no existing record — cfg is returned unchanged. // Any other DB error is returned to prevent a partial write from clobbering // previously configured filter fields. -func mergeServiceConfig(ctx context.Context, store config.StoreInterface, cfg config.ServiceConfig) (config.ServiceConfig, error) { +func mergeServiceConfig(ctx context.Context, store config.StoreInterface, cfg config.ServiceConfig, body string) (config.ServiceConfig, error) { existing, err := store.GetServiceConfig(ctx, cfg.Provider, cfg.Service) if err != nil { if strings.Contains(err.Error(), "not found") { @@ -176,14 +193,66 @@ func mergeServiceConfig(ctx context.Context, store config.StoreInterface, cfg co } return cfg, fmt.Errorf("failed to read existing service config before update: %w", err) } - if existing != nil { - existing.Enabled = cfg.Enabled - existing.Term = cfg.Term - existing.Payment = cfg.Payment - existing.Coverage = cfg.Coverage - return *existing, nil + if existing == nil { + return cfg, nil + } + + existing.Enabled = cfg.Enabled + existing.Term = cfg.Term + existing.Payment = cfg.Payment + existing.Coverage = cfg.Coverage + + present, perr := presentKeys(body, serviceConfigFilterKeys) + if perr != nil { + return cfg, perr + } + overlayPresentFilterFields(existing, &cfg, present) + return *existing, nil +} + +// presentKeys returns the subset of keys that appear as top-level fields in the +// JSON body. A malformed body is reported as a client error so the caller +// returns 400 rather than silently overlaying nothing. +func presentKeys(body string, keys []string) (map[string]bool, error) { + var raw map[string]json.RawMessage + if err := json.Unmarshal([]byte(body), &raw); err != nil { + return nil, NewClientError(400, "invalid request body") + } + present := make(map[string]bool, len(keys)) + for _, k := range keys { + if _, ok := raw[k]; ok { + present[k] = true + } + } + return present, nil +} + +// overlayPresentFilterFields copies each filter field from src onto dst only +// when its JSON key was present in the request body. Split out of +// mergeServiceConfig to keep that function under the cyclomatic-complexity +// gate. +func overlayPresentFilterFields(dst, src *config.ServiceConfig, present map[string]bool) { + if present["include_engines"] { + dst.IncludeEngines = src.IncludeEngines + } + if present["exclude_engines"] { + dst.ExcludeEngines = src.ExcludeEngines + } + if present["include_regions"] { + dst.IncludeRegions = src.IncludeRegions + } + if present["exclude_regions"] { + dst.ExcludeRegions = src.ExcludeRegions + } + if present["include_types"] { + dst.IncludeTypes = src.IncludeTypes + } + if present["exclude_types"] { + dst.ExcludeTypes = src.ExcludeTypes + } + if present["min_count"] { + dst.MinCount = src.MinCount } - return cfg, nil } func (h *Handler) getServiceConfig(ctx context.Context, req *events.LambdaFunctionURLRequest, service string) (any, error) { @@ -241,10 +310,11 @@ func (h *Handler) updateServiceConfig(ctx context.Context, req *events.LambdaFun cfg.Service = parts[1] } - // Merge: preserve existing filter fields, overlay the 4 UI-editable fields. - // The frontend only sends enabled/term/payment/coverage; a full UPSERT would - // zero out ramp_schedule, include_engines, etc. that were set elsewhere. - merged, mergeErr := mergeServiceConfig(ctx, h.config, cfg) + // Merge: overlay the scalar UI fields unconditionally and the + // recommendation-filter fields only when the body carried them, so a + // partial PUT never zeroes a filter (ramp_schedule, or a filter set + // out-of-band) the request didn't mean to touch. + merged, mergeErr := mergeServiceConfig(ctx, h.config, cfg, req.Body) if mergeErr != nil { return nil, mergeErr } diff --git a/internal/api/handler_config_test.go b/internal/api/handler_config_test.go index b47a791a7..547ed1f21 100644 --- a/internal/api/handler_config_test.go +++ b/internal/api/handler_config_test.go @@ -220,6 +220,88 @@ func TestHandler_updateServiceConfig(t *testing.T) { assert.Equal(t, "updated", result.Status) } +// TestMergeServiceConfig_PresenceAwareFilterOverlay verifies that the +// recommendation-filter fields are overlaid from the request only when the +// body actually carries the key, while the four scalar UI fields are always +// overlaid. A partial PUT that omits a filter must preserve the existing +// value; a PUT that includes it (even empty) must apply it. +func TestMergeServiceConfig_PresenceAwareFilterOverlay(t *testing.T) { + ctx := context.Background() + // Each subtest gets a fresh existing record: mergeServiceConfig overlays + // onto the pointer returned by GetServiceConfig (the production postgres + // store returns a fresh struct per call), so a shared fixture would leak + // mutations across subtests. + newExisting := func() *config.ServiceConfig { + return &config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: true, Term: 3, + Payment: "all-upfront", Coverage: 50, + IncludeEngines: []string{"mysql"}, + ExcludeTypes: []string{"db.t2.micro"}, + MinCount: 4, + } + } + + t.Run("body includes filter fields -> overlaid", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetServiceConfig", ctx, "aws", "rds").Return(newExisting(), nil) + t.Cleanup(func() { store.AssertExpectations(t) }) + + req := config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: false, Term: 1, + Payment: "no-upfront", Coverage: 90, + IncludeEngines: []string{"postgres"}, + MinCount: 7, + } + body := `{"enabled":false,"term":1,"payment":"no-upfront","coverage":90,"include_engines":["postgres"],"min_count":7}` + + merged, err := mergeServiceConfig(ctx, store, req, body) + require.NoError(t, err) + assert.False(t, merged.Enabled) + assert.Equal(t, 1, merged.Term) + assert.Equal(t, 90.0, merged.Coverage) + assert.Equal(t, []string{"postgres"}, merged.IncludeEngines) + assert.Equal(t, 7, merged.MinCount) + // exclude_types absent from body -> preserved from existing + assert.Equal(t, []string{"db.t2.micro"}, merged.ExcludeTypes) + }) + + t.Run("body omits filter fields -> preserved", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetServiceConfig", ctx, "aws", "rds").Return(newExisting(), nil) + t.Cleanup(func() { store.AssertExpectations(t) }) + + req := config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: true, Term: 3, + Payment: "all-upfront", Coverage: 80, + } + body := `{"enabled":true,"term":3,"payment":"all-upfront","coverage":80}` + + merged, err := mergeServiceConfig(ctx, store, req, body) + require.NoError(t, err) + assert.Equal(t, 80.0, merged.Coverage) + assert.Equal(t, []string{"mysql"}, merged.IncludeEngines, "omitted filter must be preserved") + assert.Equal(t, []string{"db.t2.micro"}, merged.ExcludeTypes) + assert.Equal(t, 4, merged.MinCount, "omitted min_count must be preserved") + }) + + t.Run("body includes empty filter -> cleared", func(t *testing.T) { + store := new(MockConfigStore) + store.On("GetServiceConfig", ctx, "aws", "rds").Return(newExisting(), nil) + t.Cleanup(func() { store.AssertExpectations(t) }) + + req := config.ServiceConfig{ + Provider: "aws", Service: "rds", Enabled: true, Term: 3, + Payment: "all-upfront", Coverage: 80, + IncludeEngines: []string{}, + } + body := `{"enabled":true,"term":3,"payment":"all-upfront","coverage":80,"include_engines":[]}` + + merged, err := mergeServiceConfig(ctx, store, req, body) + require.NoError(t, err) + assert.Empty(t, merged.IncludeEngines, "explicit empty list clears the filter") + }) +} + func TestHandler_updateServiceConfig_InvalidBody(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 104a8f99e..3e6a8ea0c 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2099,6 +2099,13 @@ components: type: array items: type: string + min_count: + type: integer + minimum: 0 + description: | + Minimum instance/node count a recommendation must carry to be + surfaced (GUI/persisted equivalent of the CLI --min-count flag). + 0 (default) disables the filter. CommitmentOptionsResponse: type: object diff --git a/internal/config/constants.go b/internal/config/constants.go index 598bf19a0..56ab21d92 100644 --- a/internal/config/constants.go +++ b/internal/config/constants.go @@ -40,6 +40,13 @@ const ( // MaxTotalSteps is the maximum number of ramp steps MaxTotalSteps = 100 + + // MaxServiceMinCount caps the per-service min-count recommendation + // filter. Mirrors the CLI's MaxReasonableInstances ceiling so a typo + // (e.g. a stray trailing zero) can't silently suppress every + // recommendation. 0 disables the filter; values above this are rejected + // at validation time. + MaxServiceMinCount = 10000 ) // Default values for new configurations diff --git a/internal/config/resolver.go b/internal/config/resolver.go index 474616b90..6e1bafce7 100644 --- a/internal/config/resolver.go +++ b/internal/config/resolver.go @@ -44,6 +44,7 @@ func ResolveServiceConfig(provider, service string, global *ServiceConfig, overr ExcludeRegions: copyStrSlice(baseline.ExcludeRegions), IncludeTypes: copyStrSlice(baseline.IncludeTypes), ExcludeTypes: copyStrSlice(baseline.ExcludeTypes), + MinCount: baseline.MinCount, } applyScalarOverrides(resolved, override) diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 447b031f1..924613221 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -227,7 +227,7 @@ func (s *PostgresStore) GetServiceConfig(ctx context.Context, provider, service query := ` SELECT provider, service, enabled, term, payment, coverage, ramp_schedule, include_engines, exclude_engines, include_regions, exclude_regions, - include_types, exclude_types + include_types, exclude_types, min_count FROM service_configs WHERE provider = $1 AND service = $2 ` @@ -249,6 +249,7 @@ func (s *PostgresStore) GetServiceConfig(ctx context.Context, provider, service &excludeRegions, &includeTypes, &excludeTypes, + &config.MinCount, ) if err != nil { @@ -275,8 +276,8 @@ func (s *PostgresStore) SaveServiceConfig(ctx context.Context, config *ServiceCo INSERT INTO service_configs ( provider, service, enabled, term, payment, coverage, ramp_schedule, include_engines, exclude_engines, include_regions, exclude_regions, - include_types, exclude_types - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) + include_types, exclude_types, min_count + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) ON CONFLICT (provider, service) DO UPDATE SET enabled = $3, term = $4, @@ -289,6 +290,7 @@ func (s *PostgresStore) SaveServiceConfig(ctx context.Context, config *ServiceCo exclude_regions = $11, include_types = $12, exclude_types = $13, + min_count = $14, updated_at = NOW() ` @@ -306,6 +308,7 @@ func (s *PostgresStore) SaveServiceConfig(ctx context.Context, config *ServiceCo config.ExcludeRegions, config.IncludeTypes, config.ExcludeTypes, + config.MinCount, ) if err != nil { @@ -327,7 +330,7 @@ func (s *PostgresStore) ListServiceConfigs(ctx context.Context) ([]ServiceConfig query := ` SELECT provider, service, enabled, term, payment, coverage, ramp_schedule, include_engines, exclude_engines, include_regions, exclude_regions, - include_types, exclude_types + include_types, exclude_types, min_count FROM service_configs ORDER BY provider, service LIMIT 1000 @@ -358,6 +361,7 @@ func (s *PostgresStore) ListServiceConfigs(ctx context.Context) ([]ServiceConfig &excludeRegions, &includeTypes, &excludeTypes, + &config.MinCount, ) if err != nil { return nil, fmt.Errorf("failed to scan service config: %w", err) diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index 8c6a5e3b3..689f1f488 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -169,11 +169,11 @@ func TestPGXMock_GetServiceConfig_Success(t *testing.T) { cols := []string{ "provider", "service", "enabled", "term", "payment", "coverage", "ramp_schedule", "include_engines", "exclude_engines", "include_regions", "exclude_regions", - "include_types", "exclude_types", + "include_types", "exclude_types", "min_count", } rows := pgxmock.NewRows(cols).AddRow( "aws", "ec2", true, 1, "no-upfront", 80.0, RampImmediate, - []string{"mysql"}, []string{}, []string{"us-east-1"}, []string{}, []string{}, []string{}, + []string{"mysql"}, []string{}, []string{"us-east-1"}, []string{}, []string{}, []string{}, 2, ) mock.ExpectQuery("SELECT").WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg()).WillReturnRows(rows) @@ -182,6 +182,7 @@ func TestPGXMock_GetServiceConfig_Success(t *testing.T) { assert.Equal(t, "aws", cfg.Provider) assert.Equal(t, "ec2", cfg.Service) assert.Equal(t, []string{"mysql"}, cfg.IncludeEngines) + assert.Equal(t, 2, cfg.MinCount) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -195,13 +196,13 @@ func TestPGXMock_ListServiceConfigs_Success(t *testing.T) { cols := []string{ "provider", "service", "enabled", "term", "payment", "coverage", "ramp_schedule", "include_engines", "exclude_engines", "include_regions", "exclude_regions", - "include_types", "exclude_types", + "include_types", "exclude_types", "min_count", } rows := pgxmock.NewRows(cols). AddRow("aws", "ec2", true, 1, "no-upfront", 80.0, RampImmediate, - []string{}, []string{}, []string{}, []string{}, []string{}, []string{}). + []string{}, []string{}, []string{}, []string{}, []string{}, []string{}, 0). AddRow("aws", "rds", true, 3, "all-upfront", 70.0, RampImmediate, - []string{}, []string{}, []string{}, []string{}, []string{}, []string{}) + []string{}, []string{}, []string{}, []string{}, []string{}, []string{}, 0) mock.ExpectQuery("SELECT").WillReturnRows(rows) cfgs, err := store.ListServiceConfigs(ctx) diff --git a/internal/config/types.go b/internal/config/types.go index 2c04a4f76..a8ea379c3 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -119,6 +119,13 @@ type ServiceConfig struct { ExcludeRegions []string `json:"exclude_regions,omitempty" dynamodbav:"exclude_regions,omitempty"` IncludeTypes []string `json:"include_types,omitempty" dynamodbav:"include_types,omitempty"` ExcludeTypes []string `json:"exclude_types,omitempty" dynamodbav:"exclude_types,omitempty"` + // MinCount is the GUI/persisted equivalent of the CLI --min-count flag: + // the minimum instance/node count a recommendation must carry to be + // surfaced. Applied at read time by + // scheduler.filterRecsByResolvedConfigs against the persisted + // RecommendationRecord.Count. 0 (the default) disables the filter, + // matching the CLI flag's 0-no-floor semantics. + MinCount int `json:"min_count,omitempty" dynamodbav:"min_count,omitempty"` } // PurchasePlan represents a saved purchase plan for automated execution diff --git a/internal/config/validation.go b/internal/config/validation.go index 959ccec62..c4323a82d 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -323,9 +323,22 @@ func (c *ServiceConfig) Validate() error { if err := c.validatePayment(); err != nil { return err } + if err := c.validateMinCount(); err != nil { + return err + } return c.validateConfigCoverage() } +func (c *ServiceConfig) validateMinCount() error { + if c.MinCount < 0 { + return fmt.Errorf("min_count must be 0 (no filter) or a positive number, got: %d", c.MinCount) + } + if c.MinCount > MaxServiceMinCount { + return fmt.Errorf("min_count (%d) exceeds reasonable limit of %d", c.MinCount, MaxServiceMinCount) + } + return nil +} + func (c *ServiceConfig) validateProvider() error { if c.Provider == "" { return fmt.Errorf("provider is required") diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 5e2d42190..e977f35f9 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -508,6 +508,40 @@ func TestServiceConfig_Validate(t *testing.T) { wantErr: true, errMsg: "coverage must be between 0 and 100", }, + { + name: "valid min_count", + config: ServiceConfig{ + Provider: "aws", + Service: "ec2", + Term: 3, + Coverage: 80, + Payment: "all-upfront", + MinCount: 5, + }, + wantErr: false, + }, + { + name: "negative min_count", + config: ServiceConfig{ + Provider: "aws", + Service: "ec2", + Coverage: 80, + MinCount: -1, + }, + wantErr: true, + errMsg: "min_count must be 0 (no filter) or a positive number", + }, + { + name: "min_count too high", + config: ServiceConfig{ + Provider: "aws", + Service: "ec2", + Coverage: 80, + MinCount: MaxServiceMinCount + 1, + }, + wantErr: true, + errMsg: "exceeds reasonable limit", + }, } for _, tt := range tests { diff --git a/internal/database/postgres/migrations/000068_service_configs_min_count.down.sql b/internal/database/postgres/migrations/000068_service_configs_min_count.down.sql new file mode 100644 index 000000000..56b299bc7 --- /dev/null +++ b/internal/database/postgres/migrations/000068_service_configs_min_count.down.sql @@ -0,0 +1,3 @@ +-- Reverse 000068: drop the per-service min-count filter column. +ALTER TABLE service_configs + DROP COLUMN IF EXISTS min_count; diff --git a/internal/database/postgres/migrations/000068_service_configs_min_count.up.sql b/internal/database/postgres/migrations/000068_service_configs_min_count.up.sql new file mode 100644 index 000000000..c0c8c6ada --- /dev/null +++ b/internal/database/postgres/migrations/000068_service_configs_min_count.up.sql @@ -0,0 +1,12 @@ +-- Per-service min-count recommendation filter (GUI exposure of the CLI +-- --min-count flag). +-- +-- min_count is the minimum instance/node count a recommendation must carry +-- to be surfaced. The scheduler read path +-- (scheduler.filterRecsByResolvedConfigs) drops recs whose persisted +-- RecommendationRecord.Count is below this threshold. 0 (the default and the +-- NULL backfill) means "no floor", matching the CLI flag's 0-disables +-- semantics. NOT NULL with a 0 default so existing rows and new inserts share +-- the disabled-by-default behaviour without a separate backfill pass. +ALTER TABLE service_configs + ADD COLUMN IF NOT EXISTS min_count INTEGER NOT NULL DEFAULT 0; diff --git a/internal/scheduler/scheduler_overrides.go b/internal/scheduler/scheduler_overrides.go index fb61c8b25..c9a2a6d3e 100644 --- a/internal/scheduler/scheduler_overrides.go +++ b/internal/scheduler/scheduler_overrides.go @@ -78,11 +78,27 @@ func filterRecsByResolvedConfigs( if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) { continue } + if !meetsMinCount(rec.Count, cfg) { + continue + } out = append(out, rec) } return out } +// meetsMinCount implements the read-time half of the GUI/CLI --min-count +// filter: a rec is dropped when its count is below cfg.MinCount. A MinCount +// of 0 (the default) disables the filter, matching the CLI flag's +// 0-no-floor semantics. count is the persisted RecommendationRecord.Count +// (already grace-suppression-adjusted by the time this runs, so the floor +// applies to the net count the user would actually see). +func meetsMinCount(count int, cfg *config.ServiceConfig) bool { + if cfg.MinCount <= 0 { + return true + } + return count >= cfg.MinCount +} + // engineMatches applies the engine include/exclude rule with a "lax for // engine-less services" carve-out. // diff --git a/internal/scheduler/scheduler_overrides_test.go b/internal/scheduler/scheduler_overrides_test.go index 9180e7cf0..c01cec255 100644 --- a/internal/scheduler/scheduler_overrides_test.go +++ b/internal/scheduler/scheduler_overrides_test.go @@ -241,6 +241,57 @@ func TestApplyAccountOverrides_RegionAndTypeFilters(t *testing.T) { assert.Equal(t, "db.t3.large", recs[0].ResourceType) } +func TestApplyAccountOverrides_MinCount_DropsRecsBelowFloor(t *testing.T) { + // The per-service MinCount filter (GUI/CLI --min-count) drops recs whose + // persisted Count is below the configured floor. MinCount=0 disables it. + ctx := context.Background() + mkRec := func(account string, count int) config.RecommendationRecord { + a := account + return config.RecommendationRecord{ + ID: account + "/" + string(rune('a'+count)), Provider: "aws", Service: "rds", + Region: "us-east-1", ResourceType: "db.t3.medium", Engine: "mysql", + Count: count, CloudAccountID: &a, + } + } + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{ + mkRec("acct-A", 1), + mkRec("acct-A", 2), + mkRec("acct-A", 5), + }, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true, MinCount: 2}, + }, + } + s := &Scheduler{config: store} + + recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, recs, 2, "count=1 must be dropped; count>=2 kept") + counts := []int{recs[0].Count, recs[1].Count} + assert.ElementsMatch(t, []int{2, 5}, counts) +} + +func TestApplyAccountOverrides_MinCountZero_KeepsAll(t *testing.T) { + ctx := context.Background() + a := "acct-A" + rec := config.RecommendationRecord{ + ID: "acct-A/1", Provider: "aws", Service: "rds", Region: "us-east-1", + ResourceType: "db.t3.medium", Engine: "mysql", Count: 1, CloudAccountID: &a, + } + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{rec}, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true, MinCount: 0}, + }, + } + s := &Scheduler{config: store} + + recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, recs, 1, "MinCount=0 disables the floor") +} + func TestApplyAccountOverrides_EmptyEngine_NotFilteredByIncludeEngines(t *testing.T) { // Savings Plans / Compute recs carry no engine; an IncludeEngines list // added on another service for the same account must NOT silently drop From fc5d5ec5cc8bee632c1c06a0471844f2dce5f670 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 13:33:06 -0700 Subject: [PATCH 2/3] feat(settings): expose per-service recommendation filters in the GUI Add the missing GUI surface for the CLI recommendation-filter knobs that the backend already persists and applies: include/exclude engines, instance types, and regions (config.ServiceConfig, applied server-side in scheduler.filterRecsByResolvedConfigs) plus the new per-service min-count. - New serviceFilters.ts: pure, testable inject / populate / read helpers that render a "Recommendation filters" panel into each Service Defaults card (keyed off SERVICE_FIELDS, one code path for all ~20 cards). DOM is built via createElement + .value only (no innerHTML), so user values cannot inject markup. - settings.ts load: inject (idempotent) + populate the controls from the service config. save: read + validate them up front (a bad min-count aborts the whole save with a targeted toast) and send them in the ServiceConfig payload so they round-trip through PUT /config/service/{p}/{s}. - api/types.ts: add ServiceConfig.min_count. The values flow end-to-end: GUI form -> updateServiceConfig -> persisted ServiceConfig -> scheduler read-time filter on the next recommendations fetch. Tests: serviceFilters.test.ts (15 cases: csv parse/join, idempotent inject, populate/read round-trip, min-count validation, no-panel fallback). Existing 182 settings tests still pass. --- frontend/src/__tests__/serviceFilters.test.ts | 157 ++++++++++++ frontend/src/api/types.ts | 4 + frontend/src/serviceFilters.ts | 231 ++++++++++++++++++ frontend/src/settings.ts | 46 +++- 4 files changed, 434 insertions(+), 4 deletions(-) create mode 100644 frontend/src/__tests__/serviceFilters.test.ts create mode 100644 frontend/src/serviceFilters.ts diff --git a/frontend/src/__tests__/serviceFilters.test.ts b/frontend/src/__tests__/serviceFilters.test.ts new file mode 100644 index 000000000..b778e9ce6 --- /dev/null +++ b/frontend/src/__tests__/serviceFilters.test.ts @@ -0,0 +1,157 @@ +/** + * Per-service recommendation-filter controls (serviceFilters.ts). + * + * Covers the GUI surface for the CLI filter knobs: + * - parseCsvList / joinCsvList round-trip + de-dup + trim + lowercase. + * - injectServiceFilterControls is idempotent and finds the card. + * - populate fills inputs from a ServiceConfig (or clears when absent). + * - read returns parsed values, validates min-count, and errors loudly on + * a bad min-count instead of silently coercing. + */ +import { + parseCsvList, + joinCsvList, + filterInputId, + serviceFilterInputIds, + injectServiceFilterControls, + populateServiceFilterControls, + readServiceFilterControls, + isServiceFilterError, + type ServiceFilterTarget, +} from '../serviceFilters'; +import type { ServiceConfig } from '../api/types'; + +const target: ServiceFilterTarget = { provider: 'aws', service: 'rds', termId: 'aws-rds-term' }; + +function seedCard(): void { + document.body.innerHTML = ` +
+
RDS
+ +
`; +} + +describe('parseCsvList / joinCsvList', () => { + it('splits on comma and whitespace, trims, lowercases, de-dups', () => { + expect(parseCsvList('MySQL, postgres mysql,, AURORA')).toEqual(['mysql', 'postgres', 'aurora']); + }); + it('returns [] for empty / whitespace-only input', () => { + expect(parseCsvList('')).toEqual([]); + expect(parseCsvList(' , , ')).toEqual([]); + }); + it('joins back to the comma-separated display form', () => { + expect(joinCsvList(['mysql', 'postgres'])).toBe('mysql, postgres'); + expect(joinCsvList(undefined)).toBe(''); + }); +}); + +describe('id helpers', () => { + it('builds deterministic input ids', () => { + expect(filterInputId('aws', 'rds', 'include-engines')).toBe('aws-rds-include-engines'); + }); + it('lists all owned input ids including min-count', () => { + const ids = serviceFilterInputIds(target); + expect(ids).toContain('aws-rds-include-engines'); + expect(ids).toContain('aws-rds-exclude-regions'); + expect(ids).toContain('aws-rds-min-count'); + expect(ids).toHaveLength(7); + }); +}); + +describe('injectServiceFilterControls', () => { + beforeEach(seedCard); + + it('injects the panel and all inputs once', () => { + expect(injectServiceFilterControls(target)).toBe(true); + for (const id of serviceFilterInputIds(target)) { + expect(document.getElementById(id)).not.toBeNull(); + } + // min-count input is a number input with min=0 + const minEl = document.getElementById('aws-rds-min-count') as HTMLInputElement; + expect(minEl.type).toBe('number'); + expect(minEl.min).toBe('0'); + }); + + it('is idempotent (no duplicate panel on re-run)', () => { + expect(injectServiceFilterControls(target)).toBe(true); + expect(injectServiceFilterControls(target)).toBe(false); + expect(document.querySelectorAll('.service-filter-panel')).toHaveLength(1); + }); + + it('returns false when the card is missing', () => { + document.body.innerHTML = ''; + expect(injectServiceFilterControls(target)).toBe(false); + }); +}); + +describe('populate + read round-trip', () => { + beforeEach(() => { + seedCard(); + injectServiceFilterControls(target); + }); + + it('populates inputs from a ServiceConfig', () => { + const svc: ServiceConfig = { + provider: 'aws', service: 'rds', enabled: true, term: 3, payment: 'all-upfront', coverage: 80, + include_engines: ['mysql', 'postgres'], + exclude_types: ['db.t3.micro'], + include_regions: ['us-east-1'], + min_count: 3, + }; + populateServiceFilterControls(target, svc); + expect((document.getElementById('aws-rds-include-engines') as HTMLInputElement).value).toBe('mysql, postgres'); + expect((document.getElementById('aws-rds-exclude-types') as HTMLInputElement).value).toBe('db.t3.micro'); + expect((document.getElementById('aws-rds-include-regions') as HTMLInputElement).value).toBe('us-east-1'); + expect((document.getElementById('aws-rds-min-count') as HTMLInputElement).value).toBe('3'); + }); + + it('clears inputs when no config (min-count -> 0)', () => { + populateServiceFilterControls(target, undefined); + expect((document.getElementById('aws-rds-include-engines') as HTMLInputElement).value).toBe(''); + expect((document.getElementById('aws-rds-min-count') as HTMLInputElement).value).toBe('0'); + }); + + it('reads inputs back into parsed values', () => { + (document.getElementById('aws-rds-include-engines') as HTMLInputElement).value = 'MySQL, mysql, postgres'; + (document.getElementById('aws-rds-min-count') as HTMLInputElement).value = '5'; + const v = readServiceFilterControls(target); + expect(isServiceFilterError(v)).toBe(false); + if (!isServiceFilterError(v)) { + expect(v.include_engines).toEqual(['mysql', 'postgres']); + expect(v.min_count).toBe(5); + expect(v.exclude_engines).toEqual([]); + } + }); + + it('treats blank min-count as 0', () => { + (document.getElementById('aws-rds-min-count') as HTMLInputElement).value = ' '; + const v = readServiceFilterControls(target); + expect(isServiceFilterError(v)).toBe(false); + if (!isServiceFilterError(v)) expect(v.min_count).toBe(0); + }); + + it('errors loudly on a fractional min-count', () => { + (document.getElementById('aws-rds-min-count') as HTMLInputElement).value = '1.5'; + const v = readServiceFilterControls(target); + expect(isServiceFilterError(v)).toBe(true); + if (isServiceFilterError(v)) expect(v.message).toContain('whole number'); + }); + + it('errors loudly on a negative min-count', () => { + (document.getElementById('aws-rds-min-count') as HTMLInputElement).value = '-2'; + const v = readServiceFilterControls(target); + expect(isServiceFilterError(v)).toBe(true); + }); +}); + +describe('read with no injected panel', () => { + it('returns empty lists and 0 min-count (no filters)', () => { + document.body.innerHTML = ''; + const v = readServiceFilterControls(target); + expect(isServiceFilterError(v)).toBe(false); + if (!isServiceFilterError(v)) { + expect(v.include_engines).toEqual([]); + expect(v.min_count).toBe(0); + } + }); +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 089903d58..a1956a9d2 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -303,6 +303,10 @@ export interface ServiceConfig { exclude_regions?: string[]; include_types?: string[]; exclude_types?: string[]; + /** Minimum instance/node count a recommendation must carry to be surfaced + * (GUI equivalent of the CLI --min-count flag). 0 / undefined disables the + * filter. */ + min_count?: number; } /** Response from GET /api/info (unauthenticated). Only safe-to-expose fields. */ diff --git a/frontend/src/serviceFilters.ts b/frontend/src/serviceFilters.ts new file mode 100644 index 000000000..58124ed3f --- /dev/null +++ b/frontend/src/serviceFilters.ts @@ -0,0 +1,231 @@ +/** + * Per-service recommendation-filter controls for the Settings → Service + * Defaults cards. + * + * Exposes the CLI filter knobs (include/exclude engines, instance types, + * regions, and the --min-count threshold) in the GUI. The controls are + * injected into each service card at runtime (keyed off SERVICE_FIELDS) so a + * single code path drives every card instead of ~20 hand-written HTML blocks, + * and so the inject / populate / read logic stays unit-testable in isolation. + * + * The backend already persists these fields on config.ServiceConfig and + * applies them server-side (engines/types/regions in + * scheduler.filterRecsByResolvedConfigs; min_count likewise). This module is + * purely the missing GUI surface. + * + * All DOM construction goes through createElement + .value assignment (never + * innerHTML), so user-entered values cannot inject markup. + */ + +import type { ServiceConfig } from './api/types'; + +/** Identifies a single service card. Subset of a SERVICE_FIELDS entry. */ +export interface ServiceFilterTarget { + provider: string; + service: string; + /** ID of the card's term