diff --git a/internal/scheduler/retry/convert.go b/internal/scheduler/retry/convert.go index 7aa16ce6a43..995171ce23a 100644 --- a/internal/scheduler/retry/convert.go +++ b/internal/scheduler/retry/convert.go @@ -6,9 +6,10 @@ import ( "github.com/armadaproject/armada/pkg/api" ) -// ConvertPolicy translates an api.RetryPolicy proto into the internal Policy, -// validating fields. Returns an error for any malformed field (unknown action, -// missing category, etc.). +// ConvertPolicy translates an api.RetryPolicy proto into the internal Policy. +// It only parses. The CRUD service validates policies at write time, so +// conversion assumes the stored policy is valid. It still returns an error +// for data it cannot map, for example an unknown action enum value. func ConvertPolicy(p *api.RetryPolicy) (*Policy, error) { if p == nil { return nil, fmt.Errorf("retry policy is nil") @@ -34,9 +35,6 @@ func ConvertPolicy(p *api.RetryPolicy) (*Policy, error) { DefaultAction: defaultAction, Rules: rules, } - if err := ValidatePolicy(*policy); err != nil { - return nil, fmt.Errorf("policy %q: %w", p.Name, err) - } return policy, nil } diff --git a/internal/scheduler/retry/engine_test.go b/internal/scheduler/retry/engine_test.go index 2744fb8828f..cbb760343ab 100644 --- a/internal/scheduler/retry/engine_test.go +++ b/internal/scheduler/retry/engine_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/armadaproject/armada/pkg/armadaevents" ) @@ -243,58 +242,3 @@ func TestEngine_Evaluate(t *testing.T) { }) } } - -func TestValidatePolicy(t *testing.T) { - tests := map[string]struct { - policy Policy - expectError string - }{ - "valid policy with Retry default": { - policy: Policy{ - Name: "test", - DefaultAction: ActionRetry, - }, - }, - "valid policy with OnCategory rule": { - policy: Policy{ - Name: "test", - DefaultAction: ActionFail, - Rules: []Rule{ - {Action: ActionRetry, OnCategory: "transient"}, - }, - }, - }, - "empty name rejected": { - policy: Policy{Name: "", DefaultAction: ActionRetry}, - expectError: "policy name must not be empty", - }, - "empty DefaultAction rejected": { - policy: Policy{Name: "test", DefaultAction: ""}, - expectError: "DefaultAction must be", - }, - "unknown DefaultAction rejected": { - policy: Policy{Name: "test", DefaultAction: "Skip"}, - expectError: "DefaultAction must be", - }, - "rule without OnCategory rejected": { - policy: Policy{ - Name: "test", - DefaultAction: ActionRetry, - Rules: []Rule{{Action: ActionRetry, OnCategory: ""}}, - }, - expectError: "rule 0: OnCategory must be set", - }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - err := ValidatePolicy(tc.policy) - if tc.expectError != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tc.expectError) - } else { - assert.NoError(t, err) - } - }) - } -} diff --git a/internal/scheduler/retry/types.go b/internal/scheduler/retry/types.go index 6d6117def63..7300ac9799d 100644 --- a/internal/scheduler/retry/types.go +++ b/internal/scheduler/retry/types.go @@ -1,9 +1,5 @@ package retry -import ( - "fmt" -) - // Action is what a rule or a policy's default prescribes for a failed run: // retry it or fail it permanently. type Action string @@ -53,29 +49,3 @@ type Result struct { // always set. Decision Decision } - -// ValidatePolicy checks that a policy has valid fields. -func ValidatePolicy(p Policy) error { - if p.Name == "" { - return fmt.Errorf("policy name must not be empty") - } - if p.DefaultAction != ActionFail && p.DefaultAction != ActionRetry { - return fmt.Errorf("DefaultAction must be %q or %q, got %q", ActionFail, ActionRetry, p.DefaultAction) - } - for i := range p.Rules { - if err := validateRule(i, p.Rules[i]); err != nil { - return err - } - } - return nil -} - -func validateRule(index int, rule Rule) error { - if rule.Action != ActionFail && rule.Action != ActionRetry { - return fmt.Errorf("rule %d: Action must be %q or %q, got %q", index, ActionFail, ActionRetry, rule.Action) - } - if rule.OnCategory == "" { - return fmt.Errorf("rule %d: OnCategory must be set", index) - } - return nil -}