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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions frontend/src/__tests__/serviceFilters.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,19 @@ describe('injectServiceFilterControls', () => {
expect(minEl.min).toBe('0');
});

it('labels min-count as not applied for Savings Plans services only', () => {
const labelOf = (t: ServiceFilterTarget): string =>
document.getElementById(`${t.provider}-${t.service}-min-count`)!.closest('label')!.textContent ?? '';
document.body.innerHTML = `
<div class="service-default-card"><select id="aws-savings-plans-compute-term"></select></div>
<div class="service-default-card"><select id="aws-rds-term"></select></div>`;
const sp = { provider: 'aws', service: 'savings-plans-compute', termId: 'aws-savings-plans-compute-term' };
injectServiceFilterControls(sp);
injectServiceFilterControls(target);
expect(labelOf(sp)).toContain('not applied to Savings Plans');
expect(labelOf(target)).not.toContain('Savings Plans');
});

it('is idempotent (no duplicate panel on re-run)', () => {
expect(injectServiceFilterControls(target)).toBe(true);
expect(injectServiceFilterControls(target)).toBe(false);
Expand Down
7 changes: 6 additions & 1 deletion frontend/src/serviceFilters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

import type { ServiceConfig } from './api/types';
import { isSavingsPlanService } from './lib/purchase-compatibility';

/** Identifies a single service card. Subset of a SERVICE_FIELDS entry. */
export interface ServiceFilterTarget {
Expand Down Expand Up @@ -132,7 +133,11 @@ export function injectServiceFilterControls(t: ServiceFilterTarget): boolean {
panel.appendChild(
makeLabeledInput(
filterInputId(t.provider, t.service, MIN_COUNT_KEY),
'Min count (0 = no filter)',
// Savings Plans are dollar-denominated (Count is always 1), so the
// backend exempts them from the floor.
isSavingsPlanService(t.service)
? 'Min count (not applied to Savings Plans)'
: 'Min count (0 = no filter)',
'number',
'',
),
Expand Down
7 changes: 5 additions & 2 deletions internal/scheduler/scheduler_overrides.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"

"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
)

Expand Down Expand Up @@ -76,7 +77,7 @@ func filterRecsByResolvedConfigs(
if !inListRule(rec.ResourceType, cfg.IncludeTypes, cfg.ExcludeTypes) {
continue
}
if !meetsMinCount(rec.Count, cfg) {
if !common.IsSavingsPlan(common.ServiceType(rec.Service)) && !meetsMinCount(rec.Count, cfg) {
continue
}
out = append(out, rec)
Expand All @@ -89,7 +90,9 @@ func filterRecsByResolvedConfigs(
// 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).
// applies to the net count the user would actually see). Callers skip it
// for Savings Plans: their Count is always 1 (dollar-denominated), not an
// instance count.
func meetsMinCount(count int, cfg *config.ServiceConfig) bool {
if cfg.MinCount <= 0 {
return true
Expand Down
31 changes: 31 additions & 0 deletions internal/scheduler/scheduler_overrides_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,3 +514,34 @@ func TestGetRecommendationByID_AbsentRec(t *testing.T) {
assert.Nil(t, got, "absent rec must return nil")
assert.Nil(t, hiddenBy)
}

func TestFilterRecsByResolvedConfigs_MinCount_ExemptsSavingsPlans(t *testing.T) {
// An SP rec's Count is always 1 (not an instance count), so the min-count
// floor must not drop it; non-SP recs with the same Count still drop (#1146).
a := "acct-A"
cases := []struct {
provider, service string
keep bool
}{
{"aws", "savingsplans", true},
{"aws", "savings-plans-compute", true},
{"aws", "savings-plans-ec2instance", true},
{"aws", "savings-plans-sagemaker", true},
{"aws", "savings-plans-database", true},
{"azure", "savingsplans", true},
{"aws", "ec2", false},
}
for _, tc := range cases {
t.Run(tc.provider+"/"+tc.service, func(t *testing.T) {
rec := config.RecommendationRecord{
ID: "r", Provider: tc.provider, Service: tc.service, Region: "us-east-1",
ResourceType: "x", Count: 1, CloudAccountID: &a,
}
resolved := map[string]*config.ServiceConfig{
config.AccountConfigKey(a, tc.provider, tc.service): {Provider: tc.provider, Service: tc.service, Enabled: true, MinCount: 5},
}
out := filterRecsByResolvedConfigs([]config.RecommendationRecord{rec}, resolved)
assert.Equal(t, tc.keep, len(out) == 1)
})
}
}
Loading