[wip] Provisioning: Refactor test endpoint

apps/provisioning/pkg/repository/test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"slices"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -17,12 +18,33 @@ type RepositoryValidator interface {
 	VerifyAgainstExistingRepositories(ctx context.Context, cfg *provisioning.Repository) *field.Error
 }
 
-func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
+type ValidationOptions struct {
-	return TestRepositoryWithValidator(ctx, repo, nil)
+	AllowedTargets      []provisioning.SyncTargetType
+	AllowImageRendering bool
+	MinSyncInterval     time.Duration
 }
 
-func TestRepositoryWithValidator(ctx context.Context, repo Repository, validator RepositoryValidator) (*provisioning.TestResults, error) {
+type Validator struct {
-	errors := ValidateRepository(repo)
+	opts ValidationOptions
+}
+
+func NewValidator(opts ValidationOptions) Validator {
+	// do not allow minsync interval to be less than 10
+	if opts.MinSyncInterval <= 10*time.Second {
+		opts.MinSyncInterval = 10 * time.Second
+	}
+
+	return Validator{
+		opts: opts,
+	}
+}
+
+func (v *Validator) TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
+	return v.TestRepositoryWithValidator(ctx, repo, nil)
+}
+
+func (v *Validator) TestRepositoryWithValidator(ctx context.Context, repo Repository, validator RepositoryValidator) (*provisioning.TestResults, error) {
+	errors := v.ValidateRepository(repo)
 	if len(errors) > 0 {
 		rsp := &provisioning.TestResults{
 			Code:    http.StatusUnprocessableEntity, // Invalid
@@ -62,7 +84,7 @@ func TestRepositoryWithValidator(ctx context.Context, repo Repository, validator
 	return rsp, nil
 }
 
-func ValidateRepository(repo Repository) field.ErrorList {
+func (v *Validator) ValidateRepository(repo Repository) field.ErrorList {
 	list := repo.Validate()
 	cfg := repo.Config()
 
@@ -131,6 +153,26 @@ func ValidateRepository(repo Repository) field.ErrorList {
 		}
 	}
 
+	if !slices.Contains(v.opts.AllowedTargets, cfg.Spec.Sync.Target) {
+		list = append(list,
+			field.Invalid(
+				field.NewPath("spec", "target"),
+				cfg.Spec.Sync.Target,
+				"sync target is not supported"))
+	}
+
+	if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.IntervalSeconds < int64(v.opts.MinSyncInterval.Seconds()) {
+		list = append(list, field.Invalid(field.NewPath("spec", "sync", "intervalSeconds"),
+			cfg.Spec.Sync.IntervalSeconds, fmt.Sprintf("Interval must be at least %d seconds", int64(v.opts.MinSyncInterval.Seconds()))))
+	}
+
+	if !v.opts.AllowImageRendering && cfg.Spec.GitHub != nil && cfg.Spec.GitHub.GenerateDashboardPreviews {
+		list = append(list,
+			field.Invalid(field.NewPath("spec", "generateDashboardPreviews"),
+				cfg.Spec.GitHub.GenerateDashboardPreviews,
+				"image rendering is not enabled"))
+	}
+
 	return list
 }
 

apps/provisioning/pkg/repository/test_test.go

@@ -258,9 +258,11 @@ func TestValidateRepository(t *testing.T) {
 		},
 	}
 
+	// TODO: add more tests here
+	validator := NewValidator(ValidationOptions{})
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			errors := ValidateRepository(tt.repository)
+			errors := validator.ValidateRepository(tt.repository)
 			require.Len(t, errors, tt.expectedErrs)
 			if tt.validateError != nil {
 				tt.validateError(t, errors)
@@ -358,9 +360,10 @@ func TestTestRepository(t *testing.T) {
 		},
 	}
 
+	validator := NewValidator(ValidationOptions{})
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			results, err := TestRepository(context.Background(), tt.repository)
+			results, err := validator.TestRepository(context.Background(), tt.repository)
 
 			if tt.expectedError != nil {
 				require.Error(t, err)
@@ -396,7 +399,8 @@ func TestTester_TestRepository(t *testing.T) {
 		Success: true,
 	}, nil)
 
-	results, err := TestRepository(context.Background(), repository)
+	validator := NewValidator(ValidationOptions{})
+	results, err := validator.TestRepository(context.Background(), repository)
 	require.NoError(t, err)
 	require.NotNil(t, results)
 	require.Equal(t, http.StatusOK, results.Code)

pkg/operators/provisioning/repo_operator.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/grafana/grafana-app-sdk/logging"
 	appcontroller "github.com/grafana/grafana/apps/provisioning/pkg/controller"
+	"github.com/grafana/grafana/apps/provisioning/pkg/repository"
 	"github.com/prometheus/client_golang/prometheus"
 	"k8s.io/client-go/tools/cache"
 
@@ -66,8 +67,11 @@ func RunRepoController(deps server.OperatorDependencies) error {
 	if err != nil {
 		return fmt.Errorf("create API client job store: %w", err)
 	}
+
+	// TODO: fix this
+	validator := repository.NewValidator(repository.ValidationOptions{})
 	statusPatcher := appcontroller.NewRepositoryStatusPatcher(controllerCfg.provisioningClient.ProvisioningV0alpha1())
-	healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer)
+	healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer, validator)
 
 	repoInformer := informerFactory.Provisioning().V0alpha1().Repositories()
 	controller, err := controller.NewRepositoryController(

pkg/registry/apis/provisioning/controller/health.go

@@ -23,14 +23,16 @@ type StatusPatcher interface {
 type HealthChecker struct {
 	statusPatcher StatusPatcher
 	healthMetrics healthMetrics
+	validator     repository.Validator
 }
 
 // NewHealthChecker creates a new health checker
-func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer) *HealthChecker {
+func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer, validator repository.Validator) *HealthChecker {
 	healthMetrics := registerHealthMetrics(registry)
 	return &HealthChecker{
 		statusPatcher: statusPatcher,
 		healthMetrics: healthMetrics,
+		validator:     validator,
 	}
 }
 
@@ -176,7 +178,7 @@ func (hc *HealthChecker) refreshHealth(ctx context.Context, repo repository.Repo
 		hc.healthMetrics.RecordHealthCheck(outcome, time.Since(start).Seconds())
 	}()
 
-	res, err := repository.TestRepository(ctx, repo)
+	res, err := hc.validator.TestRepository(ctx, repo)
 	if err != nil {
 		outcome = utils.ErrorOutcome
 		logger.Error("failed to test repository", "error", err)

pkg/registry/apis/provisioning/controller/health_test.go

@@ -13,13 +13,14 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+	repository "github.com/grafana/grafana/apps/provisioning/pkg/repository"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/controller/mocks"
 )
 
 func TestNewHealthChecker(t *testing.T) {
 	mockPatcher := mocks.NewStatusPatcher(t)
 
-	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 	assert.NotNil(t, hc)
 	assert.Equal(t, mockPatcher, hc.statusPatcher)
@@ -223,7 +224,7 @@ func TestHasRecentFailure(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 			result := hc.HasRecentFailure(tt.healthStatus, tt.failureType)
 			assert.Equal(t, tt.expected, result)
@@ -265,7 +266,7 @@ func TestRecordFailure(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 			repo := &provisioning.Repository{
 				Status: provisioning.RepositoryStatus{
@@ -310,7 +311,7 @@ func TestRecordFailure(t *testing.T) {
 
 func TestRecordFailureFunction(t *testing.T) {
 	mockPatcher := mocks.NewStatusPatcher(t)
-	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 	testErr := errors.New("test error")
 	result := hc.recordFailure(provisioning.HealthFailureHook, testErr)
@@ -447,7 +448,7 @@ func TestRefreshHealth(t *testing.T) {
 				testError:  tt.testError,
 			}
 
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 			if tt.expectPatch {
 				if tt.patchError != nil {
@@ -557,7 +558,7 @@ func TestHasHealthStatusChanged(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewValidator(repository.ValidationOptions{}))
 
 			result := hc.hasHealthStatusChanged(tt.old, tt.new)
 			assert.Equal(t, tt.expected, result)

pkg/registry/apis/provisioning/register.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"slices"
 	"strings"
 	"time"
 
@@ -90,10 +89,6 @@ type APIBuilder struct {
 	// TODO: Set this up in the standalone API server
 	onlyApiServer bool
 
-	allowedTargets      []provisioning.SyncTargetType
-	allowImageRendering bool
-	minSyncInterval     time.Duration
-
 	features   featuremgmt.FeatureToggles
 	usageStats usagestats.Service
 
@@ -117,6 +112,7 @@ type APIBuilder struct {
 	access           authlib.AccessChecker
 	statusPatcher    *appcontroller.RepositoryStatusPatcher
 	healthChecker    *controller.HealthChecker
+	validator        repository.Validator
 	// Extras provides additional functionality to the API.
 	extras       []Extra
 	extraWorkers []jobs.Worker
@@ -158,10 +154,11 @@ func NewAPIBuilder(
 	parsers := resources.NewParserFactory(clients)
 	resourceLister := resources.NewResourceListerForMigrations(unified, legacyMigrator, storageStatus)
 
-	// do not allow minsync interval to be less than 10
+	validator := repository.NewValidator(repository.ValidationOptions{
-	if minSyncInterval <= 10*time.Second {
+		AllowedTargets:      allowedTargets,
-		minSyncInterval = 10 * time.Second
+		AllowImageRendering: allowImageRendering,
-	}
+		MinSyncInterval:     minSyncInterval,
+	})
 
 	b := &APIBuilder{
 		onlyApiServer:       onlyApiServer,
@@ -179,10 +176,8 @@ func NewAPIBuilder(
 		access:              access,
 		jobHistoryConfig:    jobHistoryConfig,
 		extraWorkers:        extraWorkers,
-		allowedTargets:      allowedTargets,
 		restConfigGetter:    restConfigGetter,
-		allowImageRendering: allowImageRendering,
+		validator:           validator,
-		minSyncInterval:     minSyncInterval,
 		registry:            registry,
 	}
 
@@ -484,7 +479,7 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI
 	storage[provisioning.RepositoryResourceInfo.StoragePath("status")] = repositoryStatusStorage
 
 	// TODO: Add some logic so that the connectors can registered themselves and we don't have logic all over the place
-	storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b)
+	storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b, b.validator)
 	storage[provisioning.RepositoryResourceInfo.StoragePath("files")] = NewFilesConnector(b, b.parsers, b.clients, b.access)
 	storage[provisioning.RepositoryResourceInfo.StoragePath("refs")] = NewRefsConnector(b)
 	storage[provisioning.RepositoryResourceInfo.StoragePath("resources")] = &listConnector{
@@ -585,29 +580,14 @@ func (b *APIBuilder) Validate(ctx context.Context, a admission.Attributes, o adm
 		return err
 	}
 
-	list := repository.ValidateRepository(repo)
+	// ALL configuration validations should be done in ValidateRepository -
+	// this is how the UI is able to show proper validation errors
+	//
+	// the only time to add configuration checks here is if you need to compare
+	// the incoming change to the current configuration
+	list := b.validator.ValidateRepository(repo)
 	cfg := repo.Config()
 
-	if !slices.Contains(b.allowedTargets, cfg.Spec.Sync.Target) {
-		list = append(list,
-			field.Invalid(
-				field.NewPath("spec", "target"),
-				cfg.Spec.Sync.Target,
-				"sync target is not supported"))
-	}
-
-	if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.IntervalSeconds < int64(b.minSyncInterval.Seconds()) {
-		list = append(list, field.Invalid(field.NewPath("spec", "sync", "intervalSeconds"),
-			cfg.Spec.Sync.IntervalSeconds, fmt.Sprintf("Interval must be at least %d seconds", int64(b.minSyncInterval.Seconds()))))
-	}
-
-	if !b.allowImageRendering && cfg.Spec.GitHub != nil && cfg.Spec.GitHub.GenerateDashboardPreviews {
-		list = append(list,
-			field.Invalid(field.NewPath("spec", "generateDashboardPreviews"),
-				cfg.Spec.GitHub.GenerateDashboardPreviews,
-				"image rendering is not enabled"))
-	}
-
 	if a.GetOperation() == admission.Update {
 		oldRepo, err := b.asRepository(ctx, a.GetOldObject(), nil)
 		if err != nil {
@@ -683,7 +663,7 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH
 			}
 
 			b.statusPatcher = appcontroller.NewRepositoryStatusPatcher(b.GetClient())
-			b.healthChecker = controller.NewHealthChecker(b.statusPatcher, b.registry)
+			b.healthChecker = controller.NewHealthChecker(b.statusPatcher, b.registry, b.validator)
 
 			// if running solely CRUD, skip the rest of the setup
 			if b.onlyApiServer {

pkg/registry/apis/provisioning/test.go

@@ -40,14 +40,16 @@ type testConnector struct {
 	factory        repository.Factory
 	healthProvider HealthCheckerProvider
 	validator      repository.RepositoryValidator
+	cfgValidator   repository.Validator
 }
 
-func NewTestConnector(deps ConnectorDependencies) *testConnector {
+func NewTestConnector(deps ConnectorDependencies, cfgValidator repository.Validator) *testConnector {
 	return &testConnector{
 		factory:        deps.GetRepoFactory(),
 		getter:         deps,
 		healthProvider: deps,
 		validator:      deps,
+		cfgValidator:   cfgValidator,
 	}
 }
 
@@ -186,7 +188,7 @@ func (s *testConnector) Connect(ctx context.Context, name string, opts runtime.O
 			}
 		} else {
 			// Testing temporary repository - just run test without status update
-			rsp, err = repository.TestRepositoryWithValidator(ctx, repo, s.validator)
+			rsp, err = s.cfgValidator.TestRepositoryWithValidator(ctx, repo, s.validator)
 			if err != nil {
 				responder.Error(err)
 				return