root
/
grafana
mirror of https://github.com/grafana/grafana.git
1
0
Fork 0
Code Issues Actions 83 Packages Projects Releases Wiki Activity

Advisor: Return array of CheckReportFailures from checks (#104958) 

* Return array from Run

* Change NewCheckReportFailure signature
This commit is contained in:
Misi 2025-05-08 10:42:38 +02:00 committed by GitHub
parent e88b3383bf
commit 1254fb9b68
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 97 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
apps/advisor/pkg/app/checks/authchecks/list_format_validation.go
View File

@ -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 {

91
apps/advisor/pkg/app/checks/authchecks/list_format_validation_test.go
View File

@ -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)
}
})
}

18
apps/advisor/pkg/app/checks/datasourcecheck/check.go
View File

@ -108,7 +108,7 @@ func (s *uidValidationStep) Resolution() string {
"target=_blank>documentation</a> 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
}

4
apps/advisor/pkg/app/checks/datasourcecheck/check_test.go
View File

@ -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...)
}
}
}

2
apps/advisor/pkg/app/checks/ifaces.go
View File

@ -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)
}

12
apps/advisor/pkg/app/checks/plugincheck/check.go
View File

@ -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

4
apps/advisor/pkg/app/checks/plugincheck/check_test.go
View File

@ -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...)
}
}
}

4
apps/advisor/pkg/app/checks/utils.go
View File

@ -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,

2
apps/advisor/pkg/app/checktyperegisterer/checktyperegisterer_test.go
View File

@ -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
}

6
apps/advisor/pkg/app/utils.go
View File

@ -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)
}

4
apps/advisor/pkg/app/utils_test.go
View File

@ -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
}