Provisioning: Cleanup tester interface (#110640)

* Provisioning: Cleanup tester interface

* undo accidental change

* cleanup

* cleanup test

apps/provisioning/pkg/repository/test.go

@@ -12,15 +12,6 @@ import (
 	provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
 )
 
-// Tester is a struct that implements the Tester interface
-// it's temporary
-// FIXME: remove as soon as controller and jobs refactoring PRs are merged
-type Tester struct{}
-
-func (t *Tester) TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
-	return TestRepository(ctx, repo)
-}
-
 func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestResults, error) {
 	errors := ValidateRepository(repo)
 	if len(errors) > 0 {

apps/provisioning/pkg/repository/test_test.go

@@ -360,9 +360,6 @@ func TestTestRepository(t *testing.T) {
 }
 
 func TestTester_TestRepository(t *testing.T) {
-	tester := &Tester{}
-
-	// Test that it properly delegates to TestRepository
 	repository := NewMockRepository(t)
 	repository.On("Config").Return(&provisioning.Repository{
 		Spec: provisioning.RepositorySpec{
@@ -375,7 +372,7 @@ func TestTester_TestRepository(t *testing.T) {
 		Success: true,
 	}, nil)
 
-	results, err := tester.TestRepository(context.Background(), repository)
+	results, err := 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

@@ -20,7 +20,6 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 
 	informer "github.com/grafana/grafana/apps/provisioning/pkg/generated/informers/externalversions"
-	"github.com/grafana/grafana/apps/provisioning/pkg/repository"
 )
 
 func RunRepoController(opts standalone.BuildInfo, c *cli.Context, cfg *setting.Cfg) error {
@@ -63,12 +62,8 @@ func RunRepoController(opts standalone.BuildInfo, c *cli.Context, cfg *setting.C
 	if err != nil {
 		return fmt.Errorf("create API client job store: %w", err)
 	}
-	tester := &repository.Tester{}
 	statusPatcher := appcontroller.NewRepositoryStatusPatcher(controllerCfg.provisioningClient.ProvisioningV0alpha1())
-	healthChecker := controller.NewHealthChecker(
+	healthChecker := controller.NewHealthChecker(statusPatcher)
-		tester,
-		statusPatcher,
-	)
 
 	repoInformer := informerFactory.Provisioning().V0alpha1().Repositories()
 	controller, err := controller.NewRepositoryController(
@@ -77,7 +72,6 @@ func RunRepoController(opts standalone.BuildInfo, c *cli.Context, cfg *setting.C
 		controllerCfg.repoFactory,
 		nil, // resourceLister -- TODO: needed for finalizers
 		nil, // clients -- TODO: needed for finalizers
-		tester,
 		jobs,
 		nil, // dualwrite -- standalone operator assumes it is backed by unified storage
 		healthChecker,

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

@@ -18,21 +18,12 @@ type StatusPatcher interface {
 
 // HealthChecker provides unified health checking for repositories
 type HealthChecker struct {
-	tester        RepositoryTester
 	statusPatcher StatusPatcher
 }
 
-// RepositoryTester defines the interface for testing repository connectivity
-//
-//go:generate mockery --name=RepositoryTester
-type RepositoryTester interface {
-	TestRepository(ctx context.Context, repo repository.Repository) (*provisioning.TestResults, error)
-}
-
 // NewHealthChecker creates a new health checker
-func NewHealthChecker(tester RepositoryTester, statusPatcher StatusPatcher) *HealthChecker {
+func NewHealthChecker(statusPatcher StatusPatcher) *HealthChecker {
 	return &HealthChecker{
-		tester:        tester,
 		statusPatcher: statusPatcher,
 	}
 }
@@ -172,7 +163,7 @@ func (hc *HealthChecker) RefreshTimestamp(ctx context.Context, repo *provisionin
 // refreshHealth performs a comprehensive health check
 // Returns test results, health status, and any error
 func (hc *HealthChecker) refreshHealth(ctx context.Context, repo repository.Repository, existingStatus provisioning.HealthStatus) (*provisioning.TestResults, provisioning.HealthStatus, error) {
-	res, err := hc.tester.TestRepository(ctx, repo)
+	res, err := repository.TestRepository(ctx, repo)
 	if err != nil {
 		return nil, existingStatus, fmt.Errorf("failed to test repository: %w", err)
 	}

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

@@ -16,13 +16,11 @@ import (
 )
 
 func TestNewHealthChecker(t *testing.T) {
-	mockTester := mocks.NewRepositoryTester(t)
 	mockPatcher := mocks.NewStatusPatcher(t)
 
-	hc := NewHealthChecker(mockTester, mockPatcher)
+	hc := NewHealthChecker(mockPatcher)
 
 	assert.NotNil(t, hc)
-	assert.Equal(t, mockTester, hc.tester)
 	assert.Equal(t, mockPatcher, hc.statusPatcher)
 }
 
@@ -136,9 +134,8 @@ func TestShouldCheckHealth(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			mockTester := mocks.NewRepositoryTester(t)
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockTester, mockPatcher)
+			hc := NewHealthChecker(mockPatcher)
 
 			result := hc.ShouldCheckHealth(tt.repo)
 			assert.Equal(t, tt.expected, result)
@@ -224,9 +221,8 @@ func TestHasRecentFailure(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			mockTester := mocks.NewRepositoryTester(t)
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockTester, mockPatcher)
+			hc := NewHealthChecker(mockPatcher)
 
 			result := hc.HasRecentFailure(tt.healthStatus, tt.failureType)
 			assert.Equal(t, tt.expected, result)
@@ -267,9 +263,8 @@ func TestRecordFailure(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			mockTester := mocks.NewRepositoryTester(t)
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockTester, mockPatcher)
+			hc := NewHealthChecker(mockPatcher)
 
 			repo := &provisioning.Repository{
 				Status: provisioning.RepositoryStatus{
@@ -313,9 +308,8 @@ func TestRecordFailure(t *testing.T) {
 }
 
 func TestRecordFailureFunction(t *testing.T) {
-	mockTester := mocks.NewRepositoryTester(t)
 	mockPatcher := mocks.NewStatusPatcher(t)
-	hc := NewHealthChecker(mockTester, mockPatcher)
+	hc := NewHealthChecker(mockPatcher)
 
 	testErr := errors.New("test error")
 	result := hc.recordFailure(provisioning.HealthFailureHook, testErr)
@@ -437,23 +431,22 @@ func TestRefreshHealth(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			mockTester := mocks.NewRepositoryTester(t)
 			mockPatcher := mocks.NewStatusPatcher(t)
 			mockRepo := &mockRepository{
 				config: &provisioning.Repository{
+					Spec: provisioning.RepositorySpec{
+						Title: "Test Repository",
+						Type:  provisioning.LocalRepositoryType,
+					},
 					Status: provisioning.RepositoryStatus{
 						Health: tt.existingStatus,
 					},
 				},
+				testResult: tt.testResult,
+				testError:  tt.testError,
 			}
 
-			hc := NewHealthChecker(mockTester, mockPatcher)
+			hc := NewHealthChecker(mockPatcher)
-
-			if tt.testError != nil {
-				mockTester.On("TestRepository", mock.Anything, mockRepo).Return(tt.testResult, tt.testError)
-			} else {
-				mockTester.On("TestRepository", mock.Anything, mockRepo).Return(tt.testResult, nil)
-			}
 
 			if tt.expectPatch {
 				if tt.patchError != nil {
@@ -484,8 +477,6 @@ func TestRefreshHealth(t *testing.T) {
 					assert.Equal(t, tt.testResult, testResult)
 				}
 			}
-
-			mockTester.AssertExpectations(t)
 			if tt.expectPatch {
 				mockPatcher.AssertExpectations(t)
 			}
@@ -564,9 +555,8 @@ func TestHasHealthStatusChanged(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			mockTester := mocks.NewRepositoryTester(t)
 			mockPatcher := mocks.NewStatusPatcher(t)
-			hc := NewHealthChecker(mockTester, mockPatcher)
+			hc := NewHealthChecker(mockPatcher)
 
 			result := hc.hasHealthStatusChanged(tt.old, tt.new)
 			assert.Equal(t, tt.expected, result)
@@ -576,7 +566,9 @@ func TestHasHealthStatusChanged(t *testing.T) {
 
 // mockRepository implements repository.Repository interface for testing
 type mockRepository struct {
-	config *provisioning.Repository
+	config     *provisioning.Repository
+	testResult *provisioning.TestResults
+	testError  error
 }
 
 func (m *mockRepository) Config() *provisioning.Repository {
@@ -588,5 +580,11 @@ func (m *mockRepository) Validate() field.ErrorList {
 }
 
 func (m *mockRepository) Test(ctx context.Context) (*provisioning.TestResults, error) {
-	return &provisioning.TestResults{Success: true}, nil
+	if m.testError != nil {
+		return m.testResult, m.testError
+	}
+	if m.testResult != nil {
+		return m.testResult, nil
+	}
+	return &provisioning.TestResults{Success: true, Code: 200}, nil
 }

pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go

@@ -1,62 +0,0 @@
-// Code generated by mockery v2.52.4. DO NOT EDIT.
-
-package mocks
-
-import (
-	context "context"
-
-	mock "github.com/stretchr/testify/mock"
-
-	repository "github.com/grafana/grafana/apps/provisioning/pkg/repository"
-
-	v0alpha1 "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1"
-)
-
-// RepositoryTester is an autogenerated mock type for the RepositoryTester type
-type RepositoryTester struct {
-	mock.Mock
-}
-
-// TestRepository provides a mock function with given fields: ctx, repo
-func (_m *RepositoryTester) TestRepository(ctx context.Context, repo repository.Repository) (*v0alpha1.TestResults, error) {
-	ret := _m.Called(ctx, repo)
-
-	if len(ret) == 0 {
-		panic("no return value specified for TestRepository")
-	}
-
-	var r0 *v0alpha1.TestResults
-	var r1 error
-	if rf, ok := ret.Get(0).(func(context.Context, repository.Repository) (*v0alpha1.TestResults, error)); ok {
-		return rf(ctx, repo)
-	}
-	if rf, ok := ret.Get(0).(func(context.Context, repository.Repository) *v0alpha1.TestResults); ok {
-		r0 = rf(ctx, repo)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*v0alpha1.TestResults)
-		}
-	}
-
-	if rf, ok := ret.Get(1).(func(context.Context, repository.Repository) error); ok {
-		r1 = rf(ctx, repo)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// NewRepositoryTester creates a new instance of RepositoryTester. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
-// The first argument is typically a *testing.T value.
-func NewRepositoryTester(t interface {
-	mock.TestingT
-	Cleanup(func())
-}) *RepositoryTester {
-	mock := &RepositoryTester{}
-	mock.Mock.Test(t)
-
-	t.Cleanup(func() { mock.AssertExpectations(t) })
-
-	return mock
-}

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

@@ -68,7 +68,6 @@ func NewRepositoryController(
 	repoFactory repository.Factory,
 	resourceLister resources.ResourceLister,
 	clients resources.ClientFactory,
-	tester RepositoryTester,
 	jobs jobs.Queue,
 	dualwrite dualwrite.Service,
 	healthChecker *HealthChecker,

pkg/registry/apis/provisioning/register.go

@@ -430,7 +430,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, b.repoFactory, &repository.Tester{}, b)
+	storage[provisioning.RepositoryResourceInfo.StoragePath("test")] = NewTestConnector(b, b.repoFactory, b)
 	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{
@@ -637,7 +637,7 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH
 			}
 
 			b.statusPatcher = appcontroller.NewRepositoryStatusPatcher(b.GetClient())
-			b.healthChecker = controller.NewHealthChecker(&repository.Tester{}, b.statusPatcher)
+			b.healthChecker = controller.NewHealthChecker(b.statusPatcher)
 
 			// if running solely CRUD, skip the rest of the setup
 			if b.onlyApiServer {
@@ -759,7 +759,6 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH
 				b.repoFactory,
 				b.resourceLister,
 				b.clients,
-				&repository.Tester{},
 				b.jobs,
 				b.storageStatus,
 				b.GetHealthChecker(),

pkg/registry/apis/provisioning/test.go

@@ -31,20 +31,17 @@ type HealthCheckerProvider interface {
 type testConnector struct {
 	getter         RepoGetter
 	factory        repository.Factory
-	tester         controller.RepositoryTester
 	healthProvider HealthCheckerProvider
 }
 
 func NewTestConnector(
 	getter RepoGetter,
 	factory repository.Factory,
-	tester controller.RepositoryTester,
 	healthProvider HealthCheckerProvider,
 ) *testConnector {
 	return &testConnector{
 		factory:        factory,
 		getter:         getter,
-		tester:         tester,
 		healthProvider: healthProvider,
 	}
 }
@@ -184,7 +181,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 = s.tester.TestRepository(ctx, repo)
+			rsp, err = repository.TestRepository(ctx, repo)
 			if err != nil {
 				responder.Error(err)
 				return