Advisor: Refactor check interfaces (#100043)

apps/advisor/pkg/app/app.go

@@ -37,7 +37,7 @@ func New(cfg app.Config) (app.App, error) {
 	// Initialize checks
 	checkMap := map[string]checks.Check{}
 	for _, c := range checkRegistry.Checks() {
-		checkMap[c.Type()] = c
+		checkMap[c.ID()] = c
 	}
 
 	simpleConfig := simple.AppConfig{

apps/advisor/pkg/app/checks/datasourcecheck/check.go

@@ -15,6 +15,13 @@ import (
 	"k8s.io/klog/v2"
 )
 
+type check struct {
+	DatasourceSvc         datasources.DataSourceService
+	PluginStore           pluginstore.Store
+	PluginContextProvider pluginContextProvider
+	PluginClient          plugins.Client
+}
+
 func New(
 	datasourceSvc datasources.DataSourceService,
 	pluginStore pluginstore.Store,
@@ -29,28 +36,53 @@ func New(
 	}
 }
 
-type check struct {
-	DatasourceSvc         datasources.DataSourceService
-	PluginStore           pluginstore.Store
-	PluginContextProvider pluginContextProvider
-	PluginClient          plugins.Client
+func (c *check) Items(ctx context.Context) ([]any, error) {
+	dss, err := c.DatasourceSvc.GetAllDataSources(ctx, &datasources.GetAllDataSourcesQuery{})
+	if err != nil {
+		return nil, err
+	}
+	res := make([]any, len(dss))
+	for i, ds := range dss {
+		res[i] = ds
+	}
+	return res, nil
 }
 
-func (c *check) Type() string {
+func (c *check) ID() string {
 	return "datasource"
 }
 
-func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.CheckV0alpha1StatusReport, error) {
-	// Optionally read the check input encoded in the object
-	// fmt.Println(obj.Data)
-
-	dss, err := c.DatasourceSvc.GetAllDataSources(ctx, &datasources.GetAllDataSourcesQuery{})
-	if err != nil {
-		return nil, err
+func (c *check) Steps() []checks.Step {
+	return []checks.Step{
+		&uidValidationStep{},
+		&healthCheckStep{
+			PluginContextProvider: c.PluginContextProvider,
+			PluginClient:          c.PluginClient,
+		},
 	}
+}
+
+type uidValidationStep struct{}
 
+func (s *uidValidationStep) ID() string {
+	return "uid-validation"
+}
+
+func (s *uidValidationStep) Title() string {
+	return "UID validation"
+}
+
+func (s *uidValidationStep) Description() string {
+	return "Check if the UID of each data source is valid."
+}
+
+func (s *uidValidationStep) Run(ctx context.Context, obj *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) {
 	dsErrs := []advisor.CheckReportError{}
-	for _, ds := range dss {
+	for _, i := range items {
+		ds, ok := i.(*datasources.DataSource)
+		if !ok {
+			return nil, fmt.Errorf("invalid item type %T", i)
+		}
 		// Data source UID validation
 		err := util.ValidateUID(ds.UID)
 		if err != nil {
@@ -60,13 +92,41 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check
 				Action:   "Check the <a href='https://grafana.com/docs/grafana/latest/upgrade-guide/upgrade-v11.2/#grafana-data-source-uid-format-enforcement' target=_blank>documentation</a> for more information.",
 			})
 		}
+	}
+	return dsErrs, nil
+}
+
+type healthCheckStep struct {
+	PluginContextProvider pluginContextProvider
+	PluginClient          plugins.Client
+}
+
+func (s *healthCheckStep) Title() string {
+	return "Health check"
+}
+
+func (s *healthCheckStep) Description() string {
+	return "Check if all data sources are healthy."
+}
+
+func (s *healthCheckStep) ID() string {
+	return "health-check"
+}
+
+func (s *healthCheckStep) Run(ctx context.Context, obj *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) {
+	dsErrs := []advisor.CheckReportError{}
+	for _, i := range items {
+		ds, ok := i.(*datasources.DataSource)
+		if !ok {
+			return nil, fmt.Errorf("invalid item type %T", i)
+		}
 
 		// Health check execution
 		requester, err := identity.GetRequester(ctx)
 		if err != nil {
 			return nil, err
 		}
-		pCtx, err := c.PluginContextProvider.GetWithDataSource(ctx, ds.Type, requester, ds)
+		pCtx, err := s.PluginContextProvider.GetWithDataSource(ctx, ds.Type, requester, ds)
 		if err != nil {
 			klog.ErrorS(err, "Error creating plugin context", "datasource", ds.Name)
 			continue
@@ -75,7 +135,7 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check
 			PluginContext: pCtx,
 			Headers:       map[string]string{},
 		}
-		resp, err := c.PluginClient.CheckHealth(ctx, req)
+		resp, err := s.PluginClient.CheckHealth(ctx, req)
 		if err != nil {
 			fmt.Println("Error checking health", err)
 			continue
@@ -90,11 +150,7 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check
 			})
 		}
 	}
-
-	return &advisor.CheckV0alpha1StatusReport{
-		Count:  int64(len(dss)),
-		Errors: dsErrs,
-	}, nil
+	return dsErrs, nil
 }
 
 type pluginContextProvider interface {

apps/advisor/pkg/app/checks/datasourcecheck/check_test.go

@@ -31,11 +31,18 @@ func TestCheck_Run(t *testing.T) {
 		}
 
 		ctx := identity.WithRequester(context.Background(), &user.SignedInUser{})
-		report, err := check.Run(ctx, &advisor.CheckSpec{})
+		items, err := check.Items(ctx)
+		assert.NoError(t, err)
+		errs := []advisor.CheckReportError{}
+		for _, step := range check.Steps() {
+			stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items)
+			assert.NoError(t, err)
+			errs = append(errs, stepErrs...)
+		}
 
 		assert.NoError(t, err)
-		assert.Equal(t, int64(2), report.Count)
-		assert.Empty(t, report.Errors)
+		assert.Equal(t, 2, len(items))
+		assert.Empty(t, errs)
 	})
 
 	t.Run("should return errors when datasource UID is invalid", func(t *testing.T) {
@@ -54,12 +61,19 @@ func TestCheck_Run(t *testing.T) {
 		}
 
 		ctx := identity.WithRequester(context.Background(), &user.SignedInUser{})
-		report, err := check.Run(ctx, &advisor.CheckSpec{})
+		items, err := check.Items(ctx)
+		assert.NoError(t, err)
+		errs := []advisor.CheckReportError{}
+		for _, step := range check.Steps() {
+			stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items)
+			assert.NoError(t, err)
+			errs = append(errs, stepErrs...)
+		}
 
 		assert.NoError(t, err)
-		assert.Equal(t, int64(1), report.Count)
-		assert.Len(t, report.Errors, 1)
-		assert.Equal(t, "Invalid UID 'invalid uid' for data source Prometheus", report.Errors[0].Reason)
+		assert.Equal(t, 1, len(items))
+		assert.Len(t, errs, 1)
+		assert.Equal(t, "Invalid UID 'invalid uid' for data source Prometheus", errs[0].Reason)
 	})
 
 	t.Run("should return errors when datasource health check fails", func(t *testing.T) {
@@ -78,12 +92,19 @@ func TestCheck_Run(t *testing.T) {
 		}
 
 		ctx := identity.WithRequester(context.Background(), &user.SignedInUser{})
-		report, err := check.Run(ctx, &advisor.CheckSpec{})
+		items, err := check.Items(ctx)
+		assert.NoError(t, err)
+		errs := []advisor.CheckReportError{}
+		for _, step := range check.Steps() {
+			stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items)
+			assert.NoError(t, err)
+			errs = append(errs, stepErrs...)
+		}
 
 		assert.NoError(t, err)
-		assert.Equal(t, int64(1), report.Count)
-		assert.Len(t, report.Errors, 1)
-		assert.Equal(t, "Health check failed for Prometheus", report.Errors[0].Reason)
+		assert.Equal(t, 1, len(items))
+		assert.Len(t, errs, 1)
+		assert.Equal(t, "Health check failed for Prometheus", errs[0].Reason)
 	})
 }
 

apps/advisor/pkg/app/checks/ifaces.go

@@ -6,8 +6,24 @@ import (
 	advisorv0alpha1 "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1"
 )
 
-// Check defines the methods that a check must implement to be executed.
+// Check returns metadata about the check being executed and the list of Steps
 type Check interface {
-	Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec) (*advisorv0alpha1.CheckV0alpha1StatusReport, error)
-	Type() string
+	// ID returns the unique identifier of the check
+	ID() string
+	// Items returns the list of items that will be checked
+	Items(ctx context.Context) ([]any, error)
+	// Steps returns the list of steps that will be executed
+	Steps() []Step
+}
+
+// Step is a single step in a check, including its metadata
+type Step interface {
+	// ID returns the unique identifier of the step
+	ID() string
+	// Title returns the title of the step
+	Title() string
+	// Description returns the description of the step
+	Description() string
+	// Run executes the step and returns a list of errors
+	Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec, items []any) ([]advisorv0alpha1.CheckReportError, error)
 }

apps/advisor/pkg/app/checks/plugincheck/check.go

@@ -36,22 +36,63 @@ type check struct {
 	ManagedPlugins   managedplugins.Manager
 }
 
-func (c *check) Type() string {
+func (c *check) ID() string {
 	return "plugin"
 }
 
-func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0alpha1StatusReport, error) {
+func (c *check) Items(ctx context.Context) ([]any, error) {
 	ps := c.PluginStore.Plugins(ctx)
+	res := make([]any, len(ps))
+	for i, p := range ps {
+		res[i] = p
+	}
+	return res, nil
+}
+
+func (c *check) Steps() []checks.Step {
+	return []checks.Step{
+		&deprecationStep{
+			PluginRepo: c.PluginRepo,
+		},
+		&updateStep{
+			PluginRepo:       c.PluginRepo,
+			PluginPreinstall: c.PluginPreinstall,
+			ManagedPlugins:   c.ManagedPlugins,
+		},
+	}
+}
 
+type deprecationStep struct {
+	PluginRepo repo.Service
+}
+
+func (s *deprecationStep) Title() string {
+	return "Deprecation check"
+}
+
+func (s *deprecationStep) Description() string {
+	return "Check if any installed plugins are deprecated."
+}
+
+func (s *deprecationStep) ID() string {
+	return "deprecation"
+}
+
+func (s *deprecationStep) Run(ctx context.Context, _ *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) {
 	errs := []advisor.CheckReportError{}
-	for _, p := range ps {
+	for _, i := range items {
+		p, ok := i.(pluginstore.Plugin)
+		if !ok {
+			return nil, fmt.Errorf("invalid item type %T", i)
+		}
+
 		// Skip if it's a core plugin
 		if p.IsCorePlugin() {
 			continue
 		}
 
 		// Check if plugin is deprecated
-		i, err := c.PluginRepo.PluginInfo(ctx, p.ID)
+		i, err := s.PluginRepo.PluginInfo(ctx, p.ID)
 		if err != nil {
 			continue
 		}
@@ -62,13 +103,49 @@ func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0
 				Action:   "Check the <a href='https://grafana.com/legal/plugin-deprecation/#a-plugin-i-use-is-deprecated-what-should-i-do' target=_blank>documentation</a> for recommended steps.",
 			})
 		}
+	}
+	return errs, nil
+}
 
-		// Check if plugin has a newer version, only if it's not managed or pinned
-		if c.isManaged(ctx, p.ID) || c.PluginPreinstall.IsPinned(p.ID) {
+type updateStep struct {
+	PluginRepo       repo.Service
+	PluginPreinstall plugininstaller.Preinstall
+	ManagedPlugins   managedplugins.Manager
+}
+
+func (s *updateStep) Title() string {
+	return "Update check"
+}
+
+func (s *updateStep) Description() string {
+	return "Check if any installed plugins have a newer version available."
+}
+
+func (s *updateStep) ID() string {
+	return "update"
+}
+
+func (s *updateStep) Run(ctx context.Context, _ *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) {
+	errs := []advisor.CheckReportError{}
+	for _, i := range items {
+		p, ok := i.(pluginstore.Plugin)
+		if !ok {
+			return nil, fmt.Errorf("invalid item type %T", i)
+		}
+
+		// Skip if it's a core plugin
+		if p.IsCorePlugin() {
 			continue
 		}
+
+		// Skip if it's managed or pinned
+		if s.isManaged(ctx, p.ID) || s.PluginPreinstall.IsPinned(p.ID) {
+			continue
+		}
+
+		// Check if plugin has a newer version available
 		compatOpts := repo.NewCompatOpts(services.GrafanaVersion, sysruntime.GOOS, sysruntime.GOARCH)
-		info, err := c.PluginRepo.GetPluginArchiveInfo(ctx, p.ID, "", compatOpts)
+		info, err := s.PluginRepo.GetPluginArchiveInfo(ctx, p.ID, "", compatOpts)
 		if err != nil {
 			continue
 		}
@@ -83,10 +160,7 @@ func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0
 		}
 	}
 
-	return &advisor.CheckV0alpha1StatusReport{
-		Count:  int64(len(ps)),
-		Errors: errs,
-	}, nil
+	return errs, nil
 }
 
 func hasUpdate(current pluginstore.Plugin, latest *repo.PluginArchiveInfo) bool {
@@ -100,8 +174,8 @@ func hasUpdate(current pluginstore.Plugin, latest *repo.PluginArchiveInfo) bool
 	return current.Info.Version != latest.Version
 }
 
-func (c *check) isManaged(ctx context.Context, pluginID string) bool {
-	for _, managedPlugin := range c.ManagedPlugins.ManagedPlugins(ctx) {
+func (s *updateStep) isManaged(ctx context.Context, pluginID string) bool {
+	for _, managedPlugin := range s.ManagedPlugins.ManagedPlugins(ctx) {
 		if managedPlugin == pluginID {
 			return true
 		}

apps/advisor/pkg/app/checks/plugincheck/check_test.go

@@ -126,10 +126,17 @@ func TestRun(t *testing.T) {
 			managedPlugins := &mockManagedPlugins{managed: tt.pluginManaged}
 			check := New(pluginStore, pluginRepo, pluginPreinstall, managedPlugins)
 
-			report, err := check.Run(context.Background(), nil)
+			items, err := check.Items(context.Background())
 			assert.NoError(t, err)
-			assert.Equal(t, int64(len(tt.plugins)), report.Count)
-			assert.Equal(t, tt.expectedErrors, report.Errors)
+			errs := []advisor.CheckReportError{}
+			for _, step := range check.Steps() {
+				stepErrs, err := step.Run(context.Background(), &advisor.CheckSpec{}, items)
+				assert.NoError(t, err)
+				errs = append(errs, stepErrs...)
+			}
+			assert.NoError(t, err)
+			assert.Equal(t, len(tt.plugins), len(items))
+			assert.Equal(t, tt.expectedErrors, errs)
 		})
 	}
 }

apps/advisor/pkg/app/utils.go

@@ -72,14 +72,32 @@ func processCheck(ctx context.Context, client resource.Client, obj resource.Obje
 		UserUID:      uid,
 		FallbackType: typ,
 	})
-	// Run the checks
-	report, err := check.Run(ctx, &c.Spec)
+	// Get the items to check
+	items, err := check.Items(ctx)
 	if err != nil {
 		setErr := setStatusAnnotation(ctx, client, obj, "error")
 		if setErr != nil {
 			return setErr
 		}
-		return err
+		return fmt.Errorf("error initializing check: %w", err)
+	}
+	// Run the steps
+	steps := check.Steps()
+	errs := []advisorv0alpha1.CheckReportError{}
+	for _, step := range steps {
+		stepErrs, err := step.Run(ctx, &c.Spec, items)
+		if err != nil {
+			setErr := setStatusAnnotation(ctx, client, obj, "error")
+			if setErr != nil {
+				return setErr
+			}
+			return fmt.Errorf("error running step %s: %w", step.Title(), err)
+		}
+		errs = append(errs, stepErrs...)
+	}
+	report := &advisorv0alpha1.CheckV0alpha1StatusReport{
+		Errors: errs,
+		Count:  int64(len(items)),
 	}
 	err = setStatusAnnotation(ctx, client, obj, "processed")
 	if err != nil {

apps/advisor/pkg/app/utils_test.go

@@ -115,10 +115,42 @@ func (m *mockClient) PatchInto(ctx context.Context, id resource.Identifier, req
 }
 
 type mockCheck struct {
-	checks.Check
 	err error
 }
 
-func (m *mockCheck) Run(ctx context.Context, spec *advisorv0alpha1.CheckSpec) (*advisorv0alpha1.CheckV0alpha1StatusReport, error) {
-	return &advisorv0alpha1.CheckV0alpha1StatusReport{}, m.err
+func (m *mockCheck) ID() string {
+	return "mock"
+}
+
+func (m *mockCheck) Items(ctx context.Context) ([]any, error) {
+	return []any{}, nil
+}
+
+func (m *mockCheck) Steps() []checks.Step {
+	return []checks.Step{
+		&mockStep{err: m.err},
+	}
+}
+
+type mockStep struct {
+	err error
+}
+
+func (m *mockStep) Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec, items []any) ([]advisorv0alpha1.CheckReportError, error) {
+	if m.err != nil {
+		return nil, m.err
+	}
+	return nil, nil
+}
+
+func (m *mockStep) Title() string {
+	return "mock"
+}
+
+func (m *mockStep) Description() string {
+	return "mock"
+}
+
+func (m *mockStep) ID() string {
+	return "mock"
 }