Provisioning: Refactor to combine validation and test endpoint logic (#111965)

Provisioning: Refactor test endpoint

apps/provisioning/pkg/repository/tester.go

@@ -0,0 +1,84 @@
+package repository
+
+import (
+	"context"
+	"net/http"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+)
+
+// SimpleRepositoryTester will validate the repository configuration, and then proceed to test the connection to the repository
+type SimpleRepositoryTester struct {
+	validator RepositoryValidator
+}
+
+func NewSimpleRepositoryTester(validator RepositoryValidator) SimpleRepositoryTester {
+	return SimpleRepositoryTester{
+		validator: validator,
+	}
+}
+
+// TestRepository validates the repository and then runs a health check
+func (t *SimpleRepositoryTester) TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
+	errors := t.validator.ValidateRepository(repo)
+	if len(errors) > 0 {
+		rsp := &provisioning.TestResults{
+			Code:    http.StatusUnprocessableEntity, // Invalid
+			Success: false,
+			Errors:  make([]provisioning.ErrorDetails, len(errors)),
+		}
+		for i, err := range errors {
+			rsp.Errors[i] = provisioning.ErrorDetails{
+				Type:   metav1.CauseType(err.Type),
+				Field:  err.Field,
+				Detail: err.Detail,
+			}
+		}
+		return rsp, nil
+	}
+
+	return repo.Test(ctx)
+}
+
+type VerifyAgainstExistingRepositories func(ctx context.Context, cfg *provisioning.Repository) *field.Error // defined this way to prevent an import cycle
+
+// RepositoryTesterWithExistingChecker will validate the repository configuration, run a health check, and then compare it against existing repositories
+type RepositoryTesterWithExistingChecker struct {
+	tester SimpleRepositoryTester
+	verify VerifyAgainstExistingRepositories
+}
+
+func NewRepositoryTesterWithExistingChecker(tester SimpleRepositoryTester, verify VerifyAgainstExistingRepositories) RepositoryTesterWithExistingChecker {
+	return RepositoryTesterWithExistingChecker{
+		tester: tester,
+		verify: verify,
+	}
+}
+
+// TestRepositoryAndCheckExisting validates the repository, runs a health check, and then compares it against existing repositories
+func (c *RepositoryTesterWithExistingChecker) TestRepositoryAndCheckExisting(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
+	rsp, err := c.tester.TestRepository(ctx, repo)
+	if err != nil {
+		return nil, err
+	}
+
+	if rsp.Success {
+		cfg := repo.Config()
+		if validationErr := c.verify(ctx, cfg); validationErr != nil {
+			rsp = &provisioning.TestResults{
+				Success: false,
+				Code:    http.StatusUnprocessableEntity,
+				Errors: []provisioning.ErrorDetails{{
+					Type:   metav1.CauseType(validationErr.Type),
+					Field:  validationErr.Field,
+					Detail: validationErr.Detail,
+				}},
+			}
+		}
+	}
+
+	return rsp, nil
+}

apps/provisioning/pkg/repository/tester_test.go

@@ -0,0 +1,204 @@
+package repository
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
+)
+
+func TestTestRepository(t *testing.T) {
+	tests := []struct {
+		name          string
+		repository    *MockRepository
+		expectedCode  int
+		expectedErrs  []provisioning.ErrorDetails
+		expectedError error
+	}{
+		{
+			name: "validation fails",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						// Missing required title
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				return m
+			}(),
+			expectedCode: http.StatusUnprocessableEntity,
+			expectedErrs: []provisioning.ErrorDetails{{
+				Type:   metav1.CauseTypeFieldValueRequired,
+				Field:  "spec.title",
+				Detail: "a repository title must be given",
+			}},
+		},
+		{
+			name: "test passes",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repo",
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				m.On("Test", mock.Anything).Return(&provisioning.TestResults{
+					Code:    http.StatusOK,
+					Success: true,
+				}, nil)
+				return m
+			}(),
+			expectedCode: http.StatusOK,
+			expectedErrs: nil,
+		},
+		{
+			name: "test fails with error",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repo",
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				m.On("Test", mock.Anything).Return(nil, fmt.Errorf("test error"))
+				return m
+			}(),
+			expectedError: fmt.Errorf("test error"),
+		},
+		{
+			name: "test fails with results",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repo",
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				m.On("Test", mock.Anything).Return(&provisioning.TestResults{
+					Code:    http.StatusBadRequest,
+					Success: false,
+					Errors: []provisioning.ErrorDetails{{
+						Type:  metav1.CauseTypeFieldValueInvalid,
+						Field: "spec.property",
+					}},
+				}, nil)
+				return m
+			}(),
+			expectedCode: http.StatusBadRequest,
+			expectedErrs: []provisioning.ErrorDetails{{
+				Type:  metav1.CauseTypeFieldValueInvalid,
+				Field: "spec.property",
+			}},
+		},
+	}
+
+	tester := NewSimpleRepositoryTester(NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true))
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			results, err := tester.TestRepository(context.Background(), tt.repository)
+
+			if tt.expectedError != nil {
+				require.Error(t, err)
+				require.Equal(t, tt.expectedError.Error(), err.Error())
+				return
+			}
+
+			require.NoError(t, err)
+			require.NotNil(t, results)
+			require.Equal(t, tt.expectedCode, results.Code)
+
+			if tt.expectedErrs != nil {
+				require.Equal(t, tt.expectedErrs, results.Errors)
+				require.False(t, results.Success)
+			} else {
+				require.True(t, results.Success)
+				require.Empty(t, results.Errors)
+			}
+		})
+	}
+}
+
+func TestTester_TestRepository(t *testing.T) {
+	repository := NewMockRepository(t)
+	repository.On("Config").Return(&provisioning.Repository{
+		Spec: provisioning.RepositorySpec{
+			Title: "Test Repo",
+		},
+	})
+	repository.On("Validate").Return(field.ErrorList{})
+	repository.On("Test", mock.Anything).Return(&provisioning.TestResults{
+		Code:    http.StatusOK,
+		Success: true,
+	}, nil)
+
+	tester := NewSimpleRepositoryTester(NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true))
+	results, err := tester.TestRepository(context.Background(), repository)
+	require.NoError(t, err)
+	require.NotNil(t, results)
+	require.Equal(t, http.StatusOK, results.Code)
+	require.True(t, results.Success)
+}
+
+func TestFromFieldError(t *testing.T) {
+	tests := []struct {
+		name           string
+		fieldError     *field.Error
+		expectedCode   int
+		expectedField  string
+		expectedType   metav1.CauseType
+		expectedDetail string
+	}{
+		{
+			name: "required field error",
+			fieldError: &field.Error{
+				Type:   field.ErrorTypeRequired,
+				Field:  "spec.title",
+				Detail: "a repository title must be given",
+			},
+			expectedCode:   http.StatusBadRequest,
+			expectedField:  "spec.title",
+			expectedType:   metav1.CauseTypeFieldValueRequired,
+			expectedDetail: "a repository title must be given",
+		},
+		{
+			name: "not supported field error",
+			fieldError: &field.Error{
+				Type:   field.ErrorTypeNotSupported,
+				Field:  "spec.workflow",
+				Detail: "branch is only supported on git repositories",
+			},
+			expectedCode:   http.StatusBadRequest,
+			expectedField:  "spec.workflow",
+			expectedType:   metav1.CauseTypeFieldValueNotSupported,
+			expectedDetail: "branch is only supported on git repositories",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := FromFieldError(tt.fieldError)
+
+			require.NotNil(t, result)
+			require.Equal(t, tt.expectedCode, result.Code)
+			require.False(t, result.Success)
+			require.Len(t, result.Errors, 1)
+
+			errorDetail := result.Errors[0]
+			require.Equal(t, tt.expectedField, errorDetail.Field)
+			require.Equal(t, tt.expectedType, errorDetail.Type)
+			require.Equal(t, tt.expectedDetail, errorDetail.Detail)
+		})
+	}
+}

apps/provisioning/pkg/repository/validator.go

@@ -1,10 +1,10 @@
 package repository
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 	"slices"
+	"time"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -12,57 +12,27 @@ import (
 	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
 )
 
-// RepositoryValidator interface for validating repositories against existing ones
+type RepositoryValidator struct {
-type RepositoryValidator interface {
+	allowedTargets      []provisioning.SyncTargetType
-	VerifyAgainstExistingRepositories(ctx context.Context, cfg *provisioning.Repository) *field.Error
+	allowImageRendering bool
+	minSyncInterval     time.Duration
 }
 
-func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
+func NewValidator(minSyncInterval time.Duration, allowedTargets []provisioning.SyncTargetType, allowImageRendering bool) RepositoryValidator {
-	return TestRepositoryWithValidator(ctx, repo, nil)
+	// do not allow minsync interval to be less than 10
+	if minSyncInterval <= 10*time.Second {
+		minSyncInterval = 10 * time.Second
+	}
+
+	return RepositoryValidator{
+		allowedTargets:      allowedTargets,
+		allowImageRendering: allowImageRendering,
+		minSyncInterval:     minSyncInterval,
+	}
 }
 
-func TestRepositoryWithValidator(ctx context.Context, repo Repository, validator RepositoryValidator) (*provisioning.TestResults, error) {
+// ValidateRepository solely does configuration checks on the repository object. It does not run a health check or compare against existing repositories.
-	errors := ValidateRepository(repo)
+func (v *RepositoryValidator) ValidateRepository(repo Repository) field.ErrorList {
-	if len(errors) > 0 {
-		rsp := &provisioning.TestResults{
-			Code:    http.StatusUnprocessableEntity, // Invalid
-			Success: false,
-			Errors:  make([]provisioning.ErrorDetails, len(errors)),
-		}
-		for i, err := range errors {
-			rsp.Errors[i] = provisioning.ErrorDetails{
-				Type:   metav1.CauseType(err.Type),
-				Field:  err.Field,
-				Detail: err.Detail,
-			}
-		}
-		return rsp, nil
-	}
-
-	rsp, err := repo.Test(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	if rsp.Success && validator != nil {
-		cfg := repo.Config()
-		if validationErr := validator.VerifyAgainstExistingRepositories(ctx, cfg); validationErr != nil {
-			rsp = &provisioning.TestResults{
-				Success: false,
-				Code:    http.StatusUnprocessableEntity,
-				Errors: []provisioning.ErrorDetails{{
-					Type:   metav1.CauseType(validationErr.Type),
-					Field:  validationErr.Field,
-					Detail: validationErr.Detail,
-				}},
-			}
-		}
-	}
-
-	return rsp, nil
-}
-
-func ValidateRepository(repo Repository) field.ErrorList {
 	list := repo.Validate()
 	cfg := repo.Config()
 
@@ -70,9 +40,22 @@ func ValidateRepository(repo Repository) field.ErrorList {
 		list = append(list, field.Required(field.NewPath("spec", "title"), "a repository title must be given"))
 	}
 
-	if cfg.Spec.Sync.Enabled && cfg.Spec.Sync.Target == "" {
+	if cfg.Spec.Sync.Enabled {
-		list = append(list, field.Required(field.NewPath("spec", "sync", "target"),
+		if cfg.Spec.Sync.Target == "" {
-			"The target type is required when sync is enabled"))
+			list = append(list, field.Required(field.NewPath("spec", "sync", "target"),
+				"The target type is required when sync is enabled"))
+		} else if !slices.Contains(v.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.IntervalSeconds < int64(v.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.minSyncInterval.Seconds()))))
+		}
 	}
 
 	// Reserved names (for now)
@@ -131,6 +114,13 @@ func ValidateRepository(repo Repository) field.ErrorList {
 		}
 	}
 
+	if !v.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/validator_test.go

@@ -1,12 +1,9 @@
 package repository
 
 import (
-	"context"
-	"fmt"
-	"net/http"
 	"testing"
+	"time"
 
-	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -74,6 +71,28 @@ func TestValidateRepository(t *testing.T) {
 				require.Contains(t, errors.ToAggregate().Error(), "spec.sync.target: Required value")
 			},
 		},
+		{
+			name: "sync interval too low",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repo",
+						Sync: provisioning.SyncOptions{
+							Enabled:         true,
+							Target:          provisioning.SyncTargetTypeFolder,
+							IntervalSeconds: 5,
+						},
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				return m
+			}(),
+			expectedErrs: 1,
+			validateError: func(t *testing.T, errors field.ErrorList) {
+				require.Contains(t, errors.ToAggregate().Error(), "spec.sync.intervalSeconds: Invalid value")
+			},
+		},
 		{
 			name: "reserved name",
 			repository: func() *MockRepository {
@@ -132,6 +151,27 @@ func TestValidateRepository(t *testing.T) {
 				require.Contains(t, errors.ToAggregate().Error(), "spec.github: Invalid value")
 			},
 		},
+		{
+			name: "github enabled when image rendering is not allowed",
+			repository: func() *MockRepository {
+				m := NewMockRepository(t)
+				m.On("Config").Return(&provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repo",
+						Type:  provisioning.GitHubRepositoryType,
+						GitHub: &provisioning.GitHubRepositoryConfig{
+							GenerateDashboardPreviews: true,
+						},
+					},
+				})
+				m.On("Validate").Return(field.ErrorList{})
+				return m
+			}(),
+			expectedErrs: 1,
+			validateError: func(t *testing.T, errors field.ErrorList) {
+				require.Contains(t, errors.ToAggregate().Error(), "spec.generateDashboardPreviews: Invalid value")
+			},
+		},
 		{
 			name: "mismatched git config",
 			repository: func() *MockRepository {
@@ -163,16 +203,18 @@ func TestValidateRepository(t *testing.T) {
 						Sync: provisioning.SyncOptions{
 							Enabled:         true,
 							IntervalSeconds: 5,
+							Target:          provisioning.SyncTargetTypeInstance,
 						},
 					},
 				})
 				m.On("Validate").Return(field.ErrorList{})
 				return m
 			}(),
-			expectedErrs: 3,
+			expectedErrs: 4,
 			// 1. missing title
 			// 2. sync target missing
 			// 3. reserved name
+			// 4. sync target not supported
 		},
 		{
 			name: "branch workflow for non-github repository",
@@ -258,9 +300,10 @@ func TestValidateRepository(t *testing.T) {
 		},
 	}
 
+	validator := NewValidator(10*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder}, false)
 	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)
@@ -268,189 +311,3 @@ func TestValidateRepository(t *testing.T) {
 		})
 	}
 }
-
-func TestTestRepository(t *testing.T) {
-	tests := []struct {
-		name          string
-		repository    *MockRepository
-		expectedCode  int
-		expectedErrs  []provisioning.ErrorDetails
-		expectedError error
-	}{
-		{
-			name: "validation fails",
-			repository: func() *MockRepository {
-				m := NewMockRepository(t)
-				m.On("Config").Return(&provisioning.Repository{
-					Spec: provisioning.RepositorySpec{
-						// Missing required title
-					},
-				})
-				m.On("Validate").Return(field.ErrorList{})
-				return m
-			}(),
-			expectedCode: http.StatusUnprocessableEntity,
-			expectedErrs: []provisioning.ErrorDetails{{
-				Type:   metav1.CauseTypeFieldValueRequired,
-				Field:  "spec.title",
-				Detail: "a repository title must be given",
-			}},
-		},
-		{
-			name: "test passes",
-			repository: func() *MockRepository {
-				m := NewMockRepository(t)
-				m.On("Config").Return(&provisioning.Repository{
-					Spec: provisioning.RepositorySpec{
-						Title: "Test Repo",
-					},
-				})
-				m.On("Validate").Return(field.ErrorList{})
-				m.On("Test", mock.Anything).Return(&provisioning.TestResults{
-					Code:    http.StatusOK,
-					Success: true,
-				}, nil)
-				return m
-			}(),
-			expectedCode: http.StatusOK,
-			expectedErrs: nil,
-		},
-		{
-			name: "test fails with error",
-			repository: func() *MockRepository {
-				m := NewMockRepository(t)
-				m.On("Config").Return(&provisioning.Repository{
-					Spec: provisioning.RepositorySpec{
-						Title: "Test Repo",
-					},
-				})
-				m.On("Validate").Return(field.ErrorList{})
-				m.On("Test", mock.Anything).Return(nil, fmt.Errorf("test error"))
-				return m
-			}(),
-			expectedError: fmt.Errorf("test error"),
-		},
-		{
-			name: "test fails with results",
-			repository: func() *MockRepository {
-				m := NewMockRepository(t)
-				m.On("Config").Return(&provisioning.Repository{
-					Spec: provisioning.RepositorySpec{
-						Title: "Test Repo",
-					},
-				})
-				m.On("Validate").Return(field.ErrorList{})
-				m.On("Test", mock.Anything).Return(&provisioning.TestResults{
-					Code:    http.StatusBadRequest,
-					Success: false,
-					Errors: []provisioning.ErrorDetails{{
-						Type:  metav1.CauseTypeFieldValueInvalid,
-						Field: "spec.property",
-					}},
-				}, nil)
-				return m
-			}(),
-			expectedCode: http.StatusBadRequest,
-			expectedErrs: []provisioning.ErrorDetails{{
-				Type:  metav1.CauseTypeFieldValueInvalid,
-				Field: "spec.property",
-			}},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			results, err := TestRepository(context.Background(), tt.repository)
-
-			if tt.expectedError != nil {
-				require.Error(t, err)
-				require.Equal(t, tt.expectedError.Error(), err.Error())
-				return
-			}
-
-			require.NoError(t, err)
-			require.NotNil(t, results)
-			require.Equal(t, tt.expectedCode, results.Code)
-
-			if tt.expectedErrs != nil {
-				require.Equal(t, tt.expectedErrs, results.Errors)
-				require.False(t, results.Success)
-			} else {
-				require.True(t, results.Success)
-				require.Empty(t, results.Errors)
-			}
-		})
-	}
-}
-
-func TestTester_TestRepository(t *testing.T) {
-	repository := NewMockRepository(t)
-	repository.On("Config").Return(&provisioning.Repository{
-		Spec: provisioning.RepositorySpec{
-			Title: "Test Repo",
-		},
-	})
-	repository.On("Validate").Return(field.ErrorList{})
-	repository.On("Test", mock.Anything).Return(&provisioning.TestResults{
-		Code:    http.StatusOK,
-		Success: true,
-	}, nil)
-
-	results, err := TestRepository(context.Background(), repository)
-	require.NoError(t, err)
-	require.NotNil(t, results)
-	require.Equal(t, http.StatusOK, results.Code)
-	require.True(t, results.Success)
-}
-
-func TestFromFieldError(t *testing.T) {
-	tests := []struct {
-		name           string
-		fieldError     *field.Error
-		expectedCode   int
-		expectedField  string
-		expectedType   metav1.CauseType
-		expectedDetail string
-	}{
-		{
-			name: "required field error",
-			fieldError: &field.Error{
-				Type:   field.ErrorTypeRequired,
-				Field:  "spec.title",
-				Detail: "a repository title must be given",
-			},
-			expectedCode:   http.StatusBadRequest,
-			expectedField:  "spec.title",
-			expectedType:   metav1.CauseTypeFieldValueRequired,
-			expectedDetail: "a repository title must be given",
-		},
-		{
-			name: "not supported field error",
-			fieldError: &field.Error{
-				Type:   field.ErrorTypeNotSupported,
-				Field:  "spec.workflow",
-				Detail: "branch is only supported on git repositories",
-			},
-			expectedCode:   http.StatusBadRequest,
-			expectedField:  "spec.workflow",
-			expectedType:   metav1.CauseTypeFieldValueNotSupported,
-			expectedDetail: "branch is only supported on git repositories",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := FromFieldError(tt.fieldError)
-
-			require.NotNil(t, result)
-			require.Equal(t, tt.expectedCode, result.Code)
-			require.False(t, result.Success)
-			require.Len(t, result.Errors, 1)
-
-			errorDetail := result.Errors[0]
-			require.Equal(t, tt.expectedField, errorDetail.Field)
-			require.Equal(t, tt.expectedType, errorDetail.Type)
-			require.Equal(t, tt.expectedDetail, errorDetail.Detail)
-		})
-	}
-}

pkg/operators/provisioning/repo_operator.go

@@ -11,9 +11,11 @@ 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"
 
+	"github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
 	"github.com/grafana/grafana/pkg/infra/tracing"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/controller"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs"
@@ -66,8 +68,14 @@ func RunRepoController(deps server.OperatorDependencies) error {
 	if err != nil {
 		return fmt.Errorf("create API client job store: %w", err)
 	}
+
+	allowedTargets := []v0alpha1.SyncTargetType{}
+	for _, target := range controllerCfg.allowedTargets {
+		allowedTargets = append(allowedTargets, v0alpha1.SyncTargetType(target))
+	}
+	validator := repository.NewValidator(controllerCfg.minSyncInterval, allowedTargets, controllerCfg.allowImageRendering)
 	statusPatcher := appcontroller.NewRepositoryStatusPatcher(controllerCfg.provisioningClient.ProvisioningV0alpha1())
-	healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer)
+	healthChecker := controller.NewHealthChecker(statusPatcher, deps.Registerer, repository.NewSimpleRepositoryTester(validator))
 
 	repoInformer := informerFactory.Provisioning().V0alpha1().Repositories()
 	controller, err := controller.NewRepositoryController(
@@ -98,7 +106,10 @@ func RunRepoController(deps server.OperatorDependencies) error {
 
 type repoControllerConfig struct {
 	provisioningControllerConfig
-	workerCount int
+	workerCount         int
+	allowedTargets      []string
+	allowImageRendering bool
+	minSyncInterval     time.Duration
 }
 
 func getRepoControllerConfig(cfg *setting.Cfg, registry prometheus.Registerer) (*repoControllerConfig, error) {
@@ -106,8 +117,18 @@ func getRepoControllerConfig(cfg *setting.Cfg, registry prometheus.Registerer) (
 	if err != nil {
 		return nil, err
 	}
+
+	allowedTargets := []string{}
+	cfg.SectionWithEnvOverrides("provisioning").Key("allowed_targets").Strings("|")
+	if len(allowedTargets) == 0 {
+		allowedTargets = []string{"folder"}
+	}
+
 	return &repoControllerConfig{
 		provisioningControllerConfig: *controllerCfg,
+		allowedTargets:               allowedTargets,
 		workerCount:                  cfg.SectionWithEnvOverrides("operator").Key("worker_count").MustInt(1),
+		allowImageRendering:          cfg.SectionWithEnvOverrides("provisioning").Key("allow_image_rendering").MustBool(false),
+		minSyncInterval:              cfg.SectionWithEnvOverrides("provisioning").Key("min_sync_interval").MustDuration(1 * time.Minute),
 	}, nil
 }

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

@@ -23,14 +23,16 @@ type StatusPatcher interface {
 type HealthChecker struct {
 	statusPatcher StatusPatcher
 	healthMetrics healthMetrics
+	tester        repository.SimpleRepositoryTester
 }
 
 // NewHealthChecker creates a new health checker
-func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer) *HealthChecker {
+func NewHealthChecker(statusPatcher StatusPatcher, registry prometheus.Registerer, tester repository.SimpleRepositoryTester) *HealthChecker {
 	healthMetrics := registerHealthMetrics(registry)
 	return &HealthChecker{
 		statusPatcher: statusPatcher,
 		healthMetrics: healthMetrics,
+		tester:        tester,
 	}
 }
 
@@ -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.tester.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,15 @@ 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())
+	validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 	assert.NotNil(t, hc)
 	assert.Equal(t, mockPatcher, hc.statusPatcher)
@@ -136,7 +138,8 @@ func TestShouldCheckHealth(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 			result := hc.ShouldCheckHealth(tt.repo)
 			assert.Equal(t, tt.expected, result)
@@ -223,7 +226,8 @@ 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())
+			validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 			result := hc.HasRecentFailure(tt.healthStatus, tt.failureType)
 			assert.Equal(t, tt.expected, result)
@@ -265,7 +269,8 @@ 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())
+			validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 			repo := &provisioning.Repository{
 				Status: provisioning.RepositoryStatus{
@@ -310,7 +315,8 @@ func TestRecordFailure(t *testing.T) {
 
 func TestRecordFailureFunction(t *testing.T) {
 	mockPatcher := mocks.NewStatusPatcher(t)
-	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+	validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+	hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 	testErr := errors.New("test error")
 	result := hc.recordFailure(provisioning.HealthFailureHook, testErr)
@@ -447,7 +453,8 @@ func TestRefreshHealth(t *testing.T) {
 				testError:  tt.testError,
 			}
 
-			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry())
+			validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 			if tt.expectPatch {
 				if tt.patchError != nil {
@@ -557,7 +564,8 @@ 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())
+			validator := repository.NewValidator(30*time.Second, []provisioning.SyncTargetType{provisioning.SyncTargetTypeFolder, provisioning.SyncTargetTypeInstance}, true)
+			hc := NewHealthChecker(mockPatcher, prometheus.NewPedanticRegistry(), repository.NewSimpleRepositoryTester(validator))
 
 			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"
 
@@ -92,7 +91,6 @@ type APIBuilder struct {
 
 	allowedTargets      []provisioning.SyncTargetType
 	allowImageRendering bool
-	minSyncInterval     time.Duration
 
 	features   featuremgmt.FeatureToggles
 	usageStats usagestats.Service
@@ -117,6 +115,7 @@ type APIBuilder struct {
 	access           authlib.AccessChecker
 	statusPatcher    *appcontroller.RepositoryStatusPatcher
 	healthChecker    *controller.HealthChecker
+	validator        repository.RepositoryValidator
 	// Extras provides additional functionality to the API.
 	extras       []Extra
 	extraWorkers []jobs.Worker
@@ -158,11 +157,6 @@ func NewAPIBuilder(
 	parsers := resources.NewParserFactory(clients)
 	resourceLister := resources.NewResourceListerForMigrations(unified, legacyMigrator, storageStatus)
 
-	// do not allow minsync interval to be less than 10
-	if minSyncInterval <= 10*time.Second {
-		minSyncInterval = 10 * time.Second
-	}
-
 	b := &APIBuilder{
 		onlyApiServer:       onlyApiServer,
 		tracer:              tracer,
@@ -179,11 +173,11 @@ func NewAPIBuilder(
 		access:              access,
 		jobHistoryConfig:    jobHistoryConfig,
 		extraWorkers:        extraWorkers,
-		allowedTargets:      allowedTargets,
 		restConfigGetter:    restConfigGetter,
+		allowedTargets:      allowedTargets,
 		allowImageRendering: allowImageRendering,
-		minSyncInterval:     minSyncInterval,
 		registry:            registry,
+		validator:           repository.NewValidator(minSyncInterval, allowedTargets, allowImageRendering),
 	}
 
 	for _, builder := range extraBuilders {
@@ -484,7 +478,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, repository.NewRepositoryTesterWithExistingChecker(repository.NewSimpleRepositoryTester(b.validator), b.VerifyAgainstExistingRepositories))
 	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 +579,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 +662,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, repository.NewSimpleRepositoryTester(b.validator))
 
 			// if running solely CRUD, skip the rest of the setup
 			if b.onlyApiServer {

pkg/registry/apis/provisioning/register_validate_test.go

@@ -23,11 +23,12 @@ func TestAPIBuilderValidate(t *testing.T) {
 	mockRepo := repository.NewMockConfigRepository(t)
 	mockRepo.EXPECT().Validate().Return(nil)
 	factory.EXPECT().Build(mock.Anything, mock.Anything).Return(mockRepo, nil)
+	validator := repository.NewValidator(30*time.Second, []v0alpha1.SyncTargetType{v0alpha1.SyncTargetTypeFolder}, false)
 	b := &APIBuilder{
 		repoFactory:         factory,
 		allowedTargets:      []v0alpha1.SyncTargetType{v0alpha1.SyncTargetTypeFolder},
 		allowImageRendering: false,
-		minSyncInterval:     30 * time.Second,
+		validator:           validator,
 	}
 
 	t.Run("min sync interval is less than 10 seconds", func(t *testing.T) {

pkg/registry/apis/provisioning/test.go

@@ -31,7 +31,6 @@ type HealthCheckerProvider interface {
 type ConnectorDependencies interface {
 	RepoGetter
 	HealthCheckerProvider
-	repository.RepositoryValidator
 	GetRepoFactory() repository.Factory
 }
 
@@ -39,15 +38,15 @@ type testConnector struct {
 	getter         RepoGetter
 	factory        repository.Factory
 	healthProvider HealthCheckerProvider
-	validator      repository.RepositoryValidator
+	tester         repository.RepositoryTesterWithExistingChecker
 }
 
-func NewTestConnector(deps ConnectorDependencies) *testConnector {
+func NewTestConnector(deps ConnectorDependencies, tester repository.RepositoryTesterWithExistingChecker) *testConnector {
 	return &testConnector{
 		factory:        deps.GetRepoFactory(),
 		getter:         deps,
 		healthProvider: deps,
-		validator:      deps,
+		tester:         tester,
 	}
 }
 
@@ -186,7 +185,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.tester.TestRepositoryAndCheckExisting(ctx, repo)
 			if err != nil {
 				responder.Error(err)
 				return