From 1254fb9b682731a679d209aaac154fdbb08378f3 Mon Sep 17 00:00:00 2001 From: Misi Date: Thu, 8 May 2025 10:42:38 +0200 Subject: [PATCH] Advisor: Return array of CheckReportFailures from checks (#104958) * Return array from Run * Change NewCheckReportFailure signature --- .../authchecks/list_format_validation.go | 14 +-- .../authchecks/list_format_validation_test.go | 91 +++++++++++++------ .../pkg/app/checks/datasourcecheck/check.go | 18 ++-- .../app/checks/datasourcecheck/check_test.go | 4 +- apps/advisor/pkg/app/checks/ifaces.go | 2 +- .../pkg/app/checks/plugincheck/check.go | 12 +-- .../pkg/app/checks/plugincheck/check_test.go | 4 +- apps/advisor/pkg/app/checks/utils.go | 4 +- .../checktyperegisterer_test.go | 2 +- apps/advisor/pkg/app/utils.go | 6 +- apps/advisor/pkg/app/utils_test.go | 4 +- 11 files changed, 97 insertions(+), 64 deletions(-) diff --git a/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go b/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go index 076286935aa..c2627d735b1 100644 --- a/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go +++ b/apps/advisor/pkg/app/checks/authchecks/list_format_validation.go @@ -48,12 +48,14 @@ func (s *listFormatValidation) Resolution() string { return "Configure the relevant SSO setting using a valid format, like space-separated (\"opt1 opt2\"), comma-separated values (\"opt1, opt2\") or JSON array format ([\"opt1\", \"opt2\"])." } -func (s *listFormatValidation) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, objToCheck any) (*advisor.CheckReportFailure, error) { +func (s *listFormatValidation) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, objToCheck any) ([]advisor.CheckReportFailure, error) { setting, ok := objToCheck.(*models.SSOSettings) if !ok { return nil, fmt.Errorf("invalid item type %T", objToCheck) } + reportIssues := make([]advisor.CheckReportFailure, 0, 3) + for _, settingKey := range listSettingKeys { currentSettingValue, exists := setting.Settings[settingKey] if !exists || currentSettingValue == nil { @@ -63,13 +65,13 @@ func (s *listFormatValidation) Run(ctx context.Context, log logging.Logger, _ *a currentSettingStr, ok := currentSettingValue.(string) if !ok { - return checks.NewCheckReportFailure( + reportIssues = append(reportIssues, checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, s.ID(), fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", login.GetAuthProviderLabel(setting.Provider), settingKey, currentSettingValue), setting.Provider, s.generateLinks(setting.Provider), - ), nil + )) } if currentSettingStr == "" { @@ -78,17 +80,17 @@ func (s *listFormatValidation) Run(ctx context.Context, log logging.Logger, _ *a _, err := util.SplitStringWithError(currentSettingStr) if err != nil { - return checks.NewCheckReportFailure( + reportIssues = append(reportIssues, checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, s.ID(), fmt.Sprintf("%s - Invalid format for '%s': %s", login.GetAuthProviderLabel(setting.Provider), settingKey, currentSettingStr), setting.Provider, s.generateLinks(setting.Provider), - ), nil + )) } } - return nil, nil + return reportIssues, nil } func (s *listFormatValidation) generateLinks(provider string) []advisor.CheckErrorLink { diff --git a/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go b/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go index c850b6c058c..80b7ec38d29 100644 --- a/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go +++ b/apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go @@ -31,10 +31,10 @@ func TestListFormatValidation_Run(t *testing.T) { providerLabel := login.GetAuthProviderLabel(provider) tests := []struct { - name string - objToCheck any - expectedError string - expectedFailure *advisor.CheckReportFailure + name string + objToCheck any + expectedError string + expectedFailures []advisor.CheckReportFailure }{ { name: "invalid object type", @@ -47,7 +47,7 @@ func TestListFormatValidation_Run(t *testing.T) { Provider: provider, Settings: map[string]any{"other_setting": "value"}, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is nil", @@ -57,7 +57,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": nil, }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is empty string", @@ -67,7 +67,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": "", }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is empty JSON array", @@ -77,7 +77,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": "[]", }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is valid (comma-separated)", @@ -87,7 +87,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": "group1, group2", }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is valid (JSON array)", @@ -97,7 +97,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_domains": `["domain1.com", "domain2.com"]`, }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is valid (space-separated)", @@ -107,7 +107,7 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": "group1 group2", }, }, - expectedFailure: nil, + expectedFailures: nil, }, { name: "one setting exists and is not a string", @@ -117,13 +117,13 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": 123, }, }, - expectedFailure: checks.NewCheckReportFailure( + expectedFailures: []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, ListFormatValidationStepID, fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", providerLabel, "allowed_groups", 123), provider, generateExpectedLinks(provider), - ), + )}, }, { name: "one setting exists and has invalid format (bad JSON)", @@ -133,13 +133,13 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": `["group1", "group2"`, }, }, - expectedFailure: checks.NewCheckReportFailure( + expectedFailures: []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, ListFormatValidationStepID, fmt.Sprintf("%s - Invalid format for '%s': %s", providerLabel, "allowed_groups", `["group1", "group2"`), provider, generateExpectedLinks(provider), - ), + )}, }, { name: "multiple settings exist, first one is invalid (type)", @@ -150,13 +150,13 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": "group1, group2", }, }, - expectedFailure: checks.NewCheckReportFailure( + expectedFailures: []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, ListFormatValidationStepID, fmt.Sprintf("%s - Invalid type for '%s': expected string, got %T", providerLabel, "allowed_domains", 123), provider, generateExpectedLinks(provider), - ), + )}, }, { name: "multiple settings exist, second one is invalid (format)", @@ -167,13 +167,13 @@ func TestListFormatValidation_Run(t *testing.T) { "allowed_groups": `["group1",`, }, }, - expectedFailure: checks.NewCheckReportFailure( + expectedFailures: []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, ListFormatValidationStepID, fmt.Sprintf("%s - Invalid format for '%s': %s", providerLabel, "allowed_groups", `["group1",`), provider, generateExpectedLinks(provider), - ), + )}, }, { name: "all settings exist and are valid", @@ -190,31 +190,62 @@ func TestListFormatValidation_Run(t *testing.T) { "role_values_viewer": "Viewer", }, }, - expectedFailure: nil, + expectedFailures: nil, + }, + { + name: "returns multiple errors", + objToCheck: &models.SSOSettings{ + Provider: provider, + Settings: map[string]any{ + "allowed_domains": `[\"group1\", \"group2\"]`, + "allowed_groups": `["group1", "group2"`, + "role_values_editor": `["group1-`, + }, + }, + expectedFailures: []advisor.CheckReportFailure{ + checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid format for '%s': %v", providerLabel, "allowed_domains", `[\"group1\", \"group2\"]`), + provider, + generateExpectedLinks(provider), + ), + checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid format for '%s': %s", providerLabel, "allowed_groups", `["group1", "group2"`), + provider, + generateExpectedLinks(provider), + ), + checks.NewCheckReportFailure( + advisor.CheckReportFailureSeverityHigh, + ListFormatValidationStepID, + fmt.Sprintf("%s - Invalid format for '%s': %v", providerLabel, "role_values_editor", `["group1-`), + provider, + generateExpectedLinks(provider), + ), + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - failure, err := validator.Run(ctx, logging.DefaultLogger, spec, tt.objToCheck) + failures, err := validator.Run(ctx, logging.DefaultLogger, spec, tt.objToCheck) if tt.expectedError != "" { require.Error(t, err) require.Contains(t, err.Error(), tt.expectedError) - require.Nil(t, failure) + require.Nil(t, failures) return } require.NoError(t, err) - if tt.expectedFailure != nil { - require.NotNil(t, failure, "Expected a failure report, but got nil") - require.Equal(t, tt.expectedFailure.Severity, failure.Severity) - require.Equal(t, tt.expectedFailure.StepID, failure.StepID) - require.Equal(t, tt.expectedFailure.Item, failure.Item) - require.Equal(t, tt.expectedFailure.ItemID, failure.ItemID) - require.ElementsMatch(t, tt.expectedFailure.Links, failure.Links) + if len(tt.expectedFailures) > 0 { + require.NotNil(t, failures) + require.Len(t, failures, len(tt.expectedFailures)) + require.ElementsMatch(t, tt.expectedFailures, failures) } else { - require.Nil(t, failure, "Expected no failure report, but got one: %+v", failure) + require.Empty(t, failures, "Expected no failure reports, but got some: %+v", failures) } }) } diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check.go b/apps/advisor/pkg/app/checks/datasourcecheck/check.go index df1223043f4..33589b85e8f 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check.go @@ -108,7 +108,7 @@ func (s *uidValidationStep) Resolution() string { "target=_blank>documentation for more information or delete the data source and create a new one." } -func (s *uidValidationStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) (*advisor.CheckReportFailure, error) { +func (s *uidValidationStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) ([]advisor.CheckReportFailure, error) { ds, ok := i.(*datasources.DataSource) if !ok { return nil, fmt.Errorf("invalid item type %T", i) @@ -116,13 +116,13 @@ func (s *uidValidationStep) Run(ctx context.Context, log logging.Logger, obj *ad // Data source UID validation err := util.ValidateUID(ds.UID) if err != nil { - return checks.NewCheckReportFailure( + return []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityLow, s.ID(), fmt.Sprintf("%s (%s)", ds.Name, ds.UID), ds.UID, []advisor.CheckErrorLink{}, - ), nil + )}, nil } return nil, nil } @@ -148,7 +148,7 @@ func (s *healthCheckStep) ID() string { return HealthCheckStepID } -func (s *healthCheckStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) (*advisor.CheckReportFailure, error) { +func (s *healthCheckStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) ([]advisor.CheckReportFailure, error) { ds, ok := i.(*datasources.DataSource) if !ok { return nil, fmt.Errorf("invalid item type %T", i) @@ -184,7 +184,7 @@ func (s *healthCheckStep) Run(ctx context.Context, log logging.Logger, obj *advi } else { log.Debug("Failed to check health", "datasource_uid", ds.UID, "status", resp.Status, "message", resp.Message) } - return checks.NewCheckReportFailure( + return []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, s.ID(), ds.Name, @@ -195,7 +195,7 @@ func (s *healthCheckStep) Run(ctx context.Context, log logging.Logger, obj *advi Url: fmt.Sprintf("/connections/datasources/edit/%s", ds.UID), }, }, - ), nil + )}, nil } return nil, nil } @@ -221,7 +221,7 @@ func (s *missingPluginStep) ID() string { return MissingPluginStepID } -func (s *missingPluginStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) (*advisor.CheckReportFailure, error) { +func (s *missingPluginStep) Run(ctx context.Context, log logging.Logger, obj *advisor.CheckSpec, i any) ([]advisor.CheckReportFailure, error) { ds, ok := i.(*datasources.DataSource) if !ok { return nil, fmt.Errorf("invalid item type %T", i) @@ -244,13 +244,13 @@ func (s *missingPluginStep) Run(ctx context.Context, log logging.Logger, obj *ad }) } // The plugin is not installed - return checks.NewCheckReportFailure( + return []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, s.ID(), ds.Name, ds.UID, links, - ), nil + )}, nil } return nil, nil } diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go index b1a47b9bcc6..26e106df68c 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go @@ -32,8 +32,8 @@ func runChecks(check *check) ([]advisor.CheckReportFailure, error) { if err != nil { return nil, err } - if stepFailures != nil { - failures = append(failures, *stepFailures) + if len(stepFailures) > 0 { + failures = append(failures, stepFailures...) } } } diff --git a/apps/advisor/pkg/app/checks/ifaces.go b/apps/advisor/pkg/app/checks/ifaces.go index 3e9bdda2d57..9e9f8529526 100644 --- a/apps/advisor/pkg/app/checks/ifaces.go +++ b/apps/advisor/pkg/app/checks/ifaces.go @@ -30,5 +30,5 @@ type Step interface { // Explains the action that needs to be taken to resolve the issue Resolution() string // Run executes the step for an item and returns a report - Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, item any) (*advisorv0alpha1.CheckReportFailure, error) + Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, item any) ([]advisorv0alpha1.CheckReportFailure, error) } diff --git a/apps/advisor/pkg/app/checks/plugincheck/check.go b/apps/advisor/pkg/app/checks/plugincheck/check.go index 94338c7245d..c54fd5b0d67 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check.go @@ -104,7 +104,7 @@ func (s *deprecationStep) ID() string { return DeprecationStepID } -func (s *deprecationStep) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, it any) (*advisor.CheckReportFailure, error) { +func (s *deprecationStep) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, it any) ([]advisor.CheckReportFailure, error) { p, ok := it.(pluginstore.Plugin) if !ok { return nil, fmt.Errorf("invalid item type %T", it) @@ -122,7 +122,7 @@ func (s *deprecationStep) Run(ctx context.Context, log logging.Logger, _ *adviso return nil, nil } if i.Status == "deprecated" { - return checks.NewCheckReportFailure( + return []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityHigh, s.ID(), p.Name, @@ -133,7 +133,7 @@ func (s *deprecationStep) Run(ctx context.Context, log logging.Logger, _ *adviso Url: fmt.Sprintf("/plugins/%s", p.ID), }, }, - ), nil + )}, nil } return nil, nil } @@ -162,7 +162,7 @@ func (s *updateStep) ID() string { return UpdateStepID } -func (s *updateStep) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, i any) (*advisor.CheckReportFailure, error) { +func (s *updateStep) Run(ctx context.Context, log logging.Logger, _ *advisor.CheckSpec, i any) ([]advisor.CheckReportFailure, error) { p, ok := i.(pluginstore.Plugin) if !ok { return nil, fmt.Errorf("invalid item type %T", i) @@ -194,7 +194,7 @@ func (s *updateStep) Run(ctx context.Context, log logging.Logger, _ *advisor.Che return nil, nil } if hasUpdate(p, info) { - return checks.NewCheckReportFailure( + return []advisor.CheckReportFailure{checks.NewCheckReportFailure( advisor.CheckReportFailureSeverityLow, s.ID(), p.Name, @@ -205,7 +205,7 @@ func (s *updateStep) Run(ctx context.Context, log logging.Logger, _ *advisor.Che Url: fmt.Sprintf("/plugins/%s?page=version-history", p.ID), }, }, - ), nil + )}, nil } return nil, nil diff --git a/apps/advisor/pkg/app/checks/plugincheck/check_test.go b/apps/advisor/pkg/app/checks/plugincheck/check_test.go index 5095ac2f08a..924036eb73a 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check_test.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check_test.go @@ -172,8 +172,8 @@ func TestRun(t *testing.T) { for _, item := range items { stepFailures, err := step.Run(context.Background(), logging.DefaultLogger, &advisor.CheckSpec{}, item) assert.NoError(t, err) - if stepFailures != nil { - failures = append(failures, *stepFailures) + if len(stepFailures) > 0 { + failures = append(failures, stepFailures...) } } } diff --git a/apps/advisor/pkg/app/checks/utils.go b/apps/advisor/pkg/app/checks/utils.go index 361f75b3d23..4fdf7b5b927 100644 --- a/apps/advisor/pkg/app/checks/utils.go +++ b/apps/advisor/pkg/app/checks/utils.go @@ -25,8 +25,8 @@ func NewCheckReportFailure( item string, itemID string, links []advisor.CheckErrorLink, -) *advisor.CheckReportFailure { - return &advisor.CheckReportFailure{ +) advisor.CheckReportFailure { + return advisor.CheckReportFailure{ Severity: severity, StepID: stepID, Item: item, diff --git a/apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go b/apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go index d8fe810c945..2edb5a27dec 100644 --- a/apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go +++ b/apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go @@ -180,7 +180,7 @@ func (m *mockStep) Resolution() string { return "" } -func (m *mockStep) Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, item any) (*advisorv0alpha1.CheckReportFailure, error) { +func (m *mockStep) Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, item any) ([]advisorv0alpha1.CheckReportFailure, error) { return nil, nil } diff --git a/apps/advisor/pkg/app/utils.go b/apps/advisor/pkg/app/utils.go index e9f0adaafcd..fefe646a421 100644 --- a/apps/advisor/pkg/app/utils.go +++ b/apps/advisor/pkg/app/utils.go @@ -159,7 +159,7 @@ func runStepsInParallel(ctx context.Context, log logging.Logger, spec *advisorv0 go func(step checks.Step, item any) { defer wg.Done() defer func() { <-limit }() - var stepErr *advisorv0alpha1.CheckReportFailure + var stepErr []advisorv0alpha1.CheckReportFailure var err error func() { defer func() { @@ -176,8 +176,8 @@ func runStepsInParallel(ctx context.Context, log logging.Logger, spec *advisorv0 internalErr = fmt.Errorf("error running step %s: %w", step.ID(), err) return } - if stepErr != nil { - reportFailures = append(reportFailures, *stepErr) + if len(stepErr) > 0 { + reportFailures = append(reportFailures, stepErr...) } }(step, item) } diff --git a/apps/advisor/pkg/app/utils_test.go b/apps/advisor/pkg/app/utils_test.go index 29b4474205b..f97ee3778ab 100644 --- a/apps/advisor/pkg/app/utils_test.go +++ b/apps/advisor/pkg/app/utils_test.go @@ -263,7 +263,7 @@ type mockStep struct { panics bool } -func (m *mockStep) Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, items any) (*advisorv0alpha1.CheckReportFailure, error) { +func (m *mockStep) Run(ctx context.Context, log logging.Logger, obj *advisorv0alpha1.CheckSpec, items any) ([]advisorv0alpha1.CheckReportFailure, error) { if m.panics { panic("panic") } @@ -271,7 +271,7 @@ func (m *mockStep) Run(ctx context.Context, log logging.Logger, obj *advisorv0al return nil, m.err } if _, ok := items.(error); ok { - return &advisorv0alpha1.CheckReportFailure{}, nil + return []advisorv0alpha1.CheckReportFailure{{}}, nil } return nil, nil }