From 9ddc70423b01e4bcd9bd98ea7ec57d130b8319ad Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Thu, 4 Sep 2025 23:47:27 -0600 Subject: [PATCH] Provisioning: Cleanup tester interface (#110640) * Provisioning: Cleanup tester interface * undo accidental change * cleanup * cleanup test --- apps/provisioning/pkg/repository/test.go | 9 --- apps/provisioning/pkg/repository/test_test.go | 5 +- pkg/operators/provisioning/repo_operator.go | 8 +-- .../apis/provisioning/controller/health.go | 13 +--- .../provisioning/controller/health_test.go | 48 +++++++------- .../controller/mocks/RepositoryTester.go | 62 ------------------- .../provisioning/controller/repository.go | 1 - pkg/registry/apis/provisioning/register.go | 5 +- pkg/registry/apis/provisioning/test.go | 5 +- 9 files changed, 30 insertions(+), 126 deletions(-) delete mode 100644 pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go diff --git a/apps/provisioning/pkg/repository/test.go b/apps/provisioning/pkg/repository/test.go index 403ac24fa38..83a549851af 100644 --- a/apps/provisioning/pkg/repository/test.go +++ b/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 { diff --git a/apps/provisioning/pkg/repository/test_test.go b/apps/provisioning/pkg/repository/test_test.go index 56d73e13a90..5b7c8c6b815 100644 --- a/apps/provisioning/pkg/repository/test_test.go +++ b/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) diff --git a/pkg/operators/provisioning/repo_operator.go b/pkg/operators/provisioning/repo_operator.go index 7587689799d..1830b745849 100644 --- a/pkg/operators/provisioning/repo_operator.go +++ b/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( - tester, - statusPatcher, - ) + healthChecker := controller.NewHealthChecker(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, diff --git a/pkg/registry/apis/provisioning/controller/health.go b/pkg/registry/apis/provisioning/controller/health.go index eef3945e860..f6bad8fff6b 100644 --- a/pkg/registry/apis/provisioning/controller/health.go +++ b/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) } diff --git a/pkg/registry/apis/provisioning/controller/health_test.go b/pkg/registry/apis/provisioning/controller/health_test.go index a82cfea9076..a42cd35c7aa 100644 --- a/pkg/registry/apis/provisioning/controller/health_test.go +++ b/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) - - 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) - } + hc := NewHealthChecker(mockPatcher) 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 } diff --git a/pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go b/pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go deleted file mode 100644 index d7a25b906ff..00000000000 --- a/pkg/registry/apis/provisioning/controller/mocks/RepositoryTester.go +++ /dev/null @@ -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 -} diff --git a/pkg/registry/apis/provisioning/controller/repository.go b/pkg/registry/apis/provisioning/controller/repository.go index 27ee5f9e0a1..43bfecdfaa1 100644 --- a/pkg/registry/apis/provisioning/controller/repository.go +++ b/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, diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 0e2926ec138..d7c0d5c2343 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/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(), diff --git a/pkg/registry/apis/provisioning/test.go b/pkg/registry/apis/provisioning/test.go index a22cc35c77d..c1b87ab70ab 100644 --- a/pkg/registry/apis/provisioning/test.go +++ b/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