Provisioning: small pending refactoring of pull request job (#103876)

* WIP: Refactor into evaluator and commenter

* Add mocks

* Fix existing tests comment

* Fix existing changes tests

* Use the extracted config

* Fix linting

* Remove trailing line

pkg/registry/apis/provisioning/jobs/pullrequest/changes.go

@@ -50,54 +50,67 @@ type fileChangeInfo struct {
 	PreviewScreenshotURL string
 }
 
-type changeOptions struct {
-	grafanaBaseURL string
-	pullRequest    provisioning.PullRequestJobOptions
-	changes        []repository.VersionedFileChange
-	parser         resources.Parser
-	reader         repository.Reader
-	progress       jobs.JobProgressRecorder
-	render         ScreenshotRenderer // from config
+type evaluator struct {
+	render      ScreenshotRenderer
+	parsers     resources.ParserFactory
+	urlProvider func(namespace string) string
+}
+
+func NewEvaluator(render ScreenshotRenderer, parsers resources.ParserFactory, urlProvider func(namespace string) string) Evaluator {
+	return &evaluator{
+		render:      render,
+		parsers:     parsers,
+		urlProvider: urlProvider,
+	}
 }
 
 // This will process the list of versioned file changes into changeInfo
-func processChangedFiles(ctx context.Context, opts changeOptions) (changeInfo, error) {
-	info := changeInfo{
-		GrafanaBaseURL: opts.grafanaBaseURL,
+func (e *evaluator) Evaluate(ctx context.Context, repo repository.Reader, opts provisioning.PullRequestJobOptions, changes []repository.VersionedFileChange, progress jobs.JobProgressRecorder) (changeInfo, error) {
+	cfg := repo.Config()
+	parser, err := e.parsers.GetParser(ctx, repo)
+	if err != nil {
+		return changeInfo{}, fmt.Errorf("failed to get parser for %s: %w", cfg.Name, err)
 	}
 
-	if opts.render != nil {
-		if !opts.render.IsAvailable(ctx) {
-			info.MissingImageRenderer = true
-			opts.render = nil
-		}
+	baseURL := e.urlProvider(cfg.Namespace)
+	info := changeInfo{
+		GrafanaBaseURL: baseURL,
+	}
 
+	var shouldRender bool
+	switch {
+	case e.render == nil:
+		shouldRender = false
+	case !e.render.IsAvailable(ctx):
+		info.MissingImageRenderer = true
+		shouldRender = false
+	case len(changes) > 1 || !cfg.Spec.GitHub.GenerateDashboardPreviews:
 		// Only render images when there is just one change
-		if len(opts.changes) > 1 {
-			opts.render = nil
-		}
+		shouldRender = false
+	default:
+		shouldRender = true
 	}
 
 	logger := logging.FromContext(ctx)
-	for i, change := range opts.changes {
+	for i, change := range changes {
 		// process maximum 10 files
 		if i >= 10 {
-			info.SkippedFiles = len(opts.changes) - i
+			info.SkippedFiles = len(changes) - i
 			break
 		}
 
-		opts.progress.SetMessage(ctx, fmt.Sprintf("processing: %s", change.Path))
+		progress.SetMessage(ctx, fmt.Sprintf("processing: %s", change.Path))
 		logger.With("action", change.Action).With("path", change.Path)
 
-		v, err := calculateFileChangeInfo(ctx, info.GrafanaBaseURL, change, opts)
+		v, err := calculateFileChangeInfo(ctx, repo, info.GrafanaBaseURL, change, opts, parser)
 		if err != nil {
 			return info, fmt.Errorf("error calculating changes %w", err)
 		}
 
 		// If everything applied OK, then render screenshots
-		if opts.render != nil && v.GrafanaURL != "" && v.Parsed != nil && v.Parsed.DryRunResponse != nil {
-			opts.progress.SetMessage(ctx, fmt.Sprintf("rendering screenshots: %s", change.Path))
-			if err = v.renderScreenshots(ctx, info.GrafanaBaseURL, opts.render); err != nil {
+		if shouldRender && v.GrafanaURL != "" && v.Parsed != nil && v.Parsed.DryRunResponse != nil {
+			progress.SetMessage(ctx, fmt.Sprintf("rendering screenshots: %s", change.Path))
+			if err = v.renderScreenshots(ctx, info.GrafanaBaseURL, e.render); err != nil {
 				info.MissingImageRenderer = true
 				if v.Error == "" {
 					v.Error = "Error running image rendering"
@@ -116,13 +129,13 @@ func processChangedFiles(ctx context.Context, opts changeOptions) (changeInfo, e
 
 var dashboardKind = dashboard.DashboardResourceInfo.GroupVersionKind().Kind
 
-func calculateFileChangeInfo(ctx context.Context, baseURL string, change repository.VersionedFileChange, opts changeOptions) (fileChangeInfo, error) {
+func calculateFileChangeInfo(ctx context.Context, repo repository.Reader, baseURL string, change repository.VersionedFileChange, opts provisioning.PullRequestJobOptions, parser resources.Parser) (fileChangeInfo, error) {
 	if change.Action == repository.FileActionDeleted {
-		return calculateFileDeleteInfo(ctx, baseURL, change, opts)
+		return calculateFileDeleteInfo(ctx, baseURL, change)
 	}
 
 	info := fileChangeInfo{Change: change}
-	fileInfo, err := opts.reader.Read(ctx, change.Path, change.Ref)
+	fileInfo, err := repo.Read(ctx, change.Path, change.Ref)
 	if err != nil {
 		logger.Info("unable to read file", "err", err)
 		info.Error = err.Error()
@@ -130,7 +143,7 @@ func calculateFileChangeInfo(ctx context.Context, baseURL string, change reposit
 	}
 
 	// Read the file as a resource
-	info.Parsed, err = opts.parser.Parse(ctx, fileInfo)
+	info.Parsed, err = parser.Parse(ctx, fileInfo)
 	if err != nil {
 		info.Error = err.Error()
 		return info, nil
@@ -161,8 +174,8 @@ func calculateFileChangeInfo(ctx context.Context, baseURL string, change reposit
 
 		query := url.Values{}
 		query.Set("ref", info.Parsed.Info.Ref)
-		if opts.pullRequest.URL != "" {
-			query.Set("pull_request_url", url.QueryEscape(opts.pullRequest.URL))
+		if opts.URL != "" {
+			query.Set("pull_request_url", url.QueryEscape(opts.URL))
 		}
 		info.PreviewURL += "?" + query.Encode()
 	}
@@ -170,7 +183,7 @@ func calculateFileChangeInfo(ctx context.Context, baseURL string, change reposit
 	return info, nil
 }
 
-func calculateFileDeleteInfo(_ context.Context, _ string, change repository.VersionedFileChange, opts changeOptions) (fileChangeInfo, error) {
+func calculateFileDeleteInfo(_ context.Context, _ string, change repository.VersionedFileChange) (fileChangeInfo, error) {
 	// TODO -- read the old and verify
 	return fileChangeInfo{Change: change, Error: "delete feedback not yet implemented"}, nil
 }

pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go

@@ -7,16 +7,16 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/stretchr/testify/mock"
-	"github.com/stretchr/testify/require"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-
 	"github.com/grafana/grafana/pkg/apimachinery/utils"
 	"github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/resources"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 func TestCalculateChanges(t *testing.T) {
@@ -45,6 +45,17 @@ func TestCalculateChanges(t *testing.T) {
 
 	progress.On("SetMessage", mock.Anything, mock.Anything).Return()
 	reader.On("Read", mock.Anything, "path/to/file.json", "ref").Return(finfo, nil)
+	reader.On("Config").Return(&v0alpha1.Repository{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-repo",
+			Namespace: "x",
+		},
+		Spec: v0alpha1.RepositorySpec{
+			GitHub: &v0alpha1.GitHubRepositoryConfig{
+				GenerateDashboardPreviews: true,
+			},
+		},
+	})
 	parser.On("Parse", mock.Anything, finfo).Return(&resources.ParsedResource{
 		Info: finfo,
 		Repo: v0alpha1.ResourceRepositoryInfo{
@@ -76,18 +87,15 @@ func TestCalculateChanges(t *testing.T) {
 		renderer.On("IsAvailable", mock.Anything, mock.Anything).Return(true)
 		renderer.On("RenderScreenshot", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
 			Return(getDummyRenderedURL("x"), nil)
+		changes := []repository.VersionedFileChange{createdFileChange}
 
-		options := changeOptions{
-			grafanaBaseURL: "http://host/",
-			pullRequest:    pullRequest,
-			changes:        []repository.VersionedFileChange{createdFileChange},
-			parser:         parser,
-			reader:         reader,
-			progress:       progress,
-			render:         renderer,
-		}
+		parserFactory := resources.NewMockParserFactory(t)
+		parserFactory.On("GetParser", mock.Anything, mock.Anything).Return(parser, nil)
+		evaluator := NewEvaluator(renderer, parserFactory, func(_ string) string {
+			return "http://host/"
+		})
 
-		info, err := processChangedFiles(context.Background(), options)
+		info, err := evaluator.Evaluate(context.Background(), reader, pullRequest, changes, progress)
 		require.NoError(t, err)
 
 		require.False(t, info.MissingImageRenderer)
@@ -107,17 +115,14 @@ func TestCalculateChanges(t *testing.T) {
 	t.Run("without-screenshot", func(t *testing.T) {
 		renderer := NewMockScreenshotRenderer(t)
 		renderer.On("IsAvailable", mock.Anything, mock.Anything).Return(false)
-		options := changeOptions{
-			grafanaBaseURL: "http://host/",
-			pullRequest:    pullRequest,
-			changes:        []repository.VersionedFileChange{createdFileChange},
-			parser:         parser,
-			reader:         reader,
-			progress:       progress,
-			render:         renderer,
-		}
+		changes := []repository.VersionedFileChange{createdFileChange}
+		parserFactory := resources.NewMockParserFactory(t)
+		parserFactory.On("GetParser", mock.Anything, mock.Anything).Return(parser, nil)
+		evaluator := NewEvaluator(renderer, parserFactory, func(_ string) string {
+			return "http://host/"
+		})
 
-		info, err := processChangedFiles(context.Background(), options)
+		info, err := evaluator.Evaluate(context.Background(), reader, pullRequest, changes, progress)
 		require.NoError(t, err)
 
 		require.True(t, info.MissingImageRenderer)
@@ -138,19 +143,18 @@ func TestCalculateChanges(t *testing.T) {
 		renderer := NewMockScreenshotRenderer(t)
 		renderer.On("IsAvailable", mock.Anything, mock.Anything).Return(true)
 
-		options := changeOptions{
-			grafanaBaseURL: "http://host/",
-			pullRequest:    pullRequest,
-			parser:         parser,
-			reader:         reader,
-			progress:       progress,
-			render:         renderer, // not used
-		}
+		changes := []repository.VersionedFileChange{}
 		for range 15 {
-			options.changes = append(options.changes, createdFileChange)
+			changes = append(changes, createdFileChange)
 		}
 
-		info, err := processChangedFiles(context.Background(), options)
+		parserFactory := resources.NewMockParserFactory(t)
+		parserFactory.On("GetParser", mock.Anything, mock.Anything).Return(parser, nil)
+		evaluator := NewEvaluator(renderer, parserFactory, func(_ string) string {
+			return "http://host/"
+		})
+
+		info, err := evaluator.Evaluate(context.Background(), reader, pullRequest, changes, progress)
 		require.NoError(t, err)
 
 		require.False(t, info.MissingImageRenderer)

pkg/registry/apis/provisioning/jobs/pullrequest/comment.go

@@ -9,21 +9,21 @@ import (
 	"strings"
 )
 
-type commentBuilder struct {
+type commenter struct {
 	templateDashboard  *template.Template
 	templateTable      *template.Template
 	templateRenderInfo *template.Template
 }
 
-func newCommentBuilder() *commentBuilder {
-	return &commentBuilder{
+func NewCommenter() Commenter {
+	return &commenter{
 		templateDashboard:  template.Must(template.New("dashboard").Parse(commentTemplateSingleDashboard)),
 		templateTable:      template.Must(template.New("table").Parse(commentTemplateTable)),
 		templateRenderInfo: template.Must(template.New("setup").Parse(commentTemplateMissingImageRenderer)),
 	}
 }
 
-func (c *commentBuilder) Comment(ctx context.Context, prRepo PullRequestRepo, pr int, info changeInfo) error {
+func (c *commenter) Comment(ctx context.Context, prRepo PullRequestRepo, pr int, info changeInfo) error {
 	comment, err := c.generateComment(ctx, info)
 	if err != nil {
 		return fmt.Errorf("unable to generate comment text: %w", err)
@@ -32,16 +32,16 @@ func (c *commentBuilder) Comment(ctx context.Context, prRepo PullRequestRepo, pr
 	if err := prRepo.CommentPullRequest(ctx, pr, comment); err != nil {
 		return fmt.Errorf("comment pull request: %w", err)
 	}
+
 	return nil
 }
 
-func (c *commentBuilder) generateComment(_ context.Context, info changeInfo) (string, error) {
+func (c *commenter) generateComment(_ context.Context, info changeInfo) (string, error) {
 	if len(info.Changes) == 0 {
 		return "no changes found", nil
 	}
 
 	var buf bytes.Buffer
-
 	if len(info.Changes) == 1 && info.Changes[0].Parsed.GVK.Kind == dashboardKind {
 		if err := c.templateDashboard.Execute(&buf, info.Changes[0]); err != nil {
 			return "", fmt.Errorf("unable to execute template: %w", err)
@@ -63,7 +63,7 @@ func (c *commentBuilder) generateComment(_ context.Context, info changeInfo) (st
 
 const commentTemplateSingleDashboard = `Hey there! 🎉
 Grafana spotted some changes to your dashboard.
- 
+
 {{- if and .GrafanaScreenshotURL .PreviewScreenshotURL}}
 ### Side by Side Comparison of {{.Parsed.Info.Path}}
 | Before | After |

pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go

@@ -7,7 +7,6 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
@@ -17,8 +16,6 @@ import (
 )
 
 func TestGenerateComment(t *testing.T) {
-	builder := newCommentBuilder()
-
 	for _, tc := range []struct {
 		Name  string
 		Input changeInfo
@@ -119,27 +116,19 @@ func TestGenerateComment(t *testing.T) {
 		}},
 	} {
 		t.Run(tc.Name, func(t *testing.T) {
-			comment, err := builder.generateComment(context.Background(), tc.Input)
-			require.NoError(t, err)
+			repo := NewMockPullRequestRepo(t)
 
+			// expectation on the comment
 			fpath := filepath.Join("testdata", strings.ReplaceAll(tc.Name, " ", "-")+".md")
-			update := false
-
 			// We can ignore the gosec G304 because this is only for tests
 			// nolint:gosec
 			expect, err := os.ReadFile(fpath)
-			if err != nil || len(expect) < 1 {
-				update = true
-				t.Error("missing " + fpath)
-			} else {
-				if diff := cmp.Diff(string(expect), comment); diff != "" {
-					t.Errorf("%s: %s", fpath, diff)
-					update = true
-				}
-			}
-			if update {
-				_ = os.WriteFile(fpath, []byte(comment), 0777)
-			}
+			require.NoError(t, err)
+			repo.On("CommentPullRequest", context.Background(), 1, string(expect)).Return(nil)
+
+			commenter := NewCommenter()
+			err = commenter.Comment(context.Background(), repo, 1, tc.Input)
+			require.NoError(t, err)
 		})
 	}
 }

pkg/registry/apis/provisioning/jobs/pullrequest/mock_commenter.go

@@ -0,0 +1,85 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package pullrequest
+
+import (
+	context "context"
+
+	mock "github.com/stretchr/testify/mock"
+)
+
+// MockCommenter is an autogenerated mock type for the Commenter type
+type MockCommenter struct {
+	mock.Mock
+}
+
+type MockCommenter_Expecter struct {
+	mock *mock.Mock
+}
+
+func (_m *MockCommenter) EXPECT() *MockCommenter_Expecter {
+	return &MockCommenter_Expecter{mock: &_m.Mock}
+}
+
+// Comment provides a mock function with given fields: ctx, repo, pr, changeInfo3
+func (_m *MockCommenter) Comment(ctx context.Context, repo PullRequestRepo, pr int, changeInfo3 changeInfo) error {
+	ret := _m.Called(ctx, repo, pr, changeInfo3)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Comment")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, PullRequestRepo, int, changeInfo) error); ok {
+		r0 = rf(ctx, repo, pr, changeInfo3)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// MockCommenter_Comment_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Comment'
+type MockCommenter_Comment_Call struct {
+	*mock.Call
+}
+
+// Comment is a helper method to define mock.On call
+//   - ctx context.Context
+//   - repo PullRequestRepo
+//   - pr int
+//   - changeInfo3 changeInfo
+func (_e *MockCommenter_Expecter) Comment(ctx interface{}, repo interface{}, pr interface{}, changeInfo3 interface{}) *MockCommenter_Comment_Call {
+	return &MockCommenter_Comment_Call{Call: _e.mock.On("Comment", ctx, repo, pr, changeInfo3)}
+}
+
+func (_c *MockCommenter_Comment_Call) Run(run func(ctx context.Context, repo PullRequestRepo, pr int, changeInfo3 changeInfo)) *MockCommenter_Comment_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(PullRequestRepo), args[2].(int), args[3].(changeInfo))
+	})
+	return _c
+}
+
+func (_c *MockCommenter_Comment_Call) Return(_a0 error) *MockCommenter_Comment_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockCommenter_Comment_Call) RunAndReturn(run func(context.Context, PullRequestRepo, int, changeInfo) error) *MockCommenter_Comment_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// NewMockCommenter creates a new instance of MockCommenter. 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 NewMockCommenter(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockCommenter {
+	mock := &MockCommenter{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/jobs/pullrequest/mock_evaluator.go

@@ -0,0 +1,101 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package pullrequest
+
+import (
+	context "context"
+
+	jobs "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs"
+	mock "github.com/stretchr/testify/mock"
+
+	repository "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
+
+	v0alpha1 "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
+)
+
+// MockEvaluator is an autogenerated mock type for the Evaluator type
+type MockEvaluator struct {
+	mock.Mock
+}
+
+type MockEvaluator_Expecter struct {
+	mock *mock.Mock
+}
+
+func (_m *MockEvaluator) EXPECT() *MockEvaluator_Expecter {
+	return &MockEvaluator_Expecter{mock: &_m.Mock}
+}
+
+// Evaluate provides a mock function with given fields: ctx, repo, opts, changes, progress
+func (_m *MockEvaluator) Evaluate(ctx context.Context, repo repository.Reader, opts v0alpha1.PullRequestJobOptions, changes []repository.VersionedFileChange, progress jobs.JobProgressRecorder) (changeInfo, error) {
+	ret := _m.Called(ctx, repo, opts, changes, progress)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Evaluate")
+	}
+
+	var r0 changeInfo
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, repository.Reader, v0alpha1.PullRequestJobOptions, []repository.VersionedFileChange, jobs.JobProgressRecorder) (changeInfo, error)); ok {
+		return rf(ctx, repo, opts, changes, progress)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, repository.Reader, v0alpha1.PullRequestJobOptions, []repository.VersionedFileChange, jobs.JobProgressRecorder) changeInfo); ok {
+		r0 = rf(ctx, repo, opts, changes, progress)
+	} else {
+		r0 = ret.Get(0).(changeInfo)
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, repository.Reader, v0alpha1.PullRequestJobOptions, []repository.VersionedFileChange, jobs.JobProgressRecorder) error); ok {
+		r1 = rf(ctx, repo, opts, changes, progress)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockEvaluator_Evaluate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Evaluate'
+type MockEvaluator_Evaluate_Call struct {
+	*mock.Call
+}
+
+// Evaluate is a helper method to define mock.On call
+//   - ctx context.Context
+//   - repo repository.Reader
+//   - opts v0alpha1.PullRequestJobOptions
+//   - changes []repository.VersionedFileChange
+//   - progress jobs.JobProgressRecorder
+func (_e *MockEvaluator_Expecter) Evaluate(ctx interface{}, repo interface{}, opts interface{}, changes interface{}, progress interface{}) *MockEvaluator_Evaluate_Call {
+	return &MockEvaluator_Evaluate_Call{Call: _e.mock.On("Evaluate", ctx, repo, opts, changes, progress)}
+}
+
+func (_c *MockEvaluator_Evaluate_Call) Run(run func(ctx context.Context, repo repository.Reader, opts v0alpha1.PullRequestJobOptions, changes []repository.VersionedFileChange, progress jobs.JobProgressRecorder)) *MockEvaluator_Evaluate_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(repository.Reader), args[2].(v0alpha1.PullRequestJobOptions), args[3].([]repository.VersionedFileChange), args[4].(jobs.JobProgressRecorder))
+	})
+	return _c
+}
+
+func (_c *MockEvaluator_Evaluate_Call) Return(_a0 changeInfo, _a1 error) *MockEvaluator_Evaluate_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockEvaluator_Evaluate_Call) RunAndReturn(run func(context.Context, repository.Reader, v0alpha1.PullRequestJobOptions, []repository.VersionedFileChange, jobs.JobProgressRecorder) (changeInfo, error)) *MockEvaluator_Evaluate_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// NewMockEvaluator creates a new instance of MockEvaluator. 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 NewMockEvaluator(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockEvaluator {
+	mock := &MockEvaluator{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/jobs/pullrequest/mock_pullrequest_repo.go

@@ -0,0 +1,351 @@
+// Code generated by mockery v2.52.4. DO NOT EDIT.
+
+package pullrequest
+
+import (
+	context "context"
+
+	repository "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository"
+	mock "github.com/stretchr/testify/mock"
+
+	v0alpha1 "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
+)
+
+// MockPullRequestRepo is an autogenerated mock type for the PullRequestRepo type
+type MockPullRequestRepo struct {
+	mock.Mock
+}
+
+type MockPullRequestRepo_Expecter struct {
+	mock *mock.Mock
+}
+
+func (_m *MockPullRequestRepo) EXPECT() *MockPullRequestRepo_Expecter {
+	return &MockPullRequestRepo_Expecter{mock: &_m.Mock}
+}
+
+// ClearAllPullRequestFileComments provides a mock function with given fields: ctx, pr
+func (_m *MockPullRequestRepo) ClearAllPullRequestFileComments(ctx context.Context, pr int) error {
+	ret := _m.Called(ctx, pr)
+
+	if len(ret) == 0 {
+		panic("no return value specified for ClearAllPullRequestFileComments")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, int) error); ok {
+		r0 = rf(ctx, pr)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// MockPullRequestRepo_ClearAllPullRequestFileComments_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'ClearAllPullRequestFileComments'
+type MockPullRequestRepo_ClearAllPullRequestFileComments_Call struct {
+	*mock.Call
+}
+
+// ClearAllPullRequestFileComments is a helper method to define mock.On call
+//   - ctx context.Context
+//   - pr int
+func (_e *MockPullRequestRepo_Expecter) ClearAllPullRequestFileComments(ctx interface{}, pr interface{}) *MockPullRequestRepo_ClearAllPullRequestFileComments_Call {
+	return &MockPullRequestRepo_ClearAllPullRequestFileComments_Call{Call: _e.mock.On("ClearAllPullRequestFileComments", ctx, pr)}
+}
+
+func (_c *MockPullRequestRepo_ClearAllPullRequestFileComments_Call) Run(run func(ctx context.Context, pr int)) *MockPullRequestRepo_ClearAllPullRequestFileComments_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(int))
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_ClearAllPullRequestFileComments_Call) Return(_a0 error) *MockPullRequestRepo_ClearAllPullRequestFileComments_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_ClearAllPullRequestFileComments_Call) RunAndReturn(run func(context.Context, int) error) *MockPullRequestRepo_ClearAllPullRequestFileComments_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// CommentPullRequest provides a mock function with given fields: ctx, pr, comment
+func (_m *MockPullRequestRepo) CommentPullRequest(ctx context.Context, pr int, comment string) error {
+	ret := _m.Called(ctx, pr, comment)
+
+	if len(ret) == 0 {
+		panic("no return value specified for CommentPullRequest")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, int, string) error); ok {
+		r0 = rf(ctx, pr, comment)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// MockPullRequestRepo_CommentPullRequest_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CommentPullRequest'
+type MockPullRequestRepo_CommentPullRequest_Call struct {
+	*mock.Call
+}
+
+// CommentPullRequest is a helper method to define mock.On call
+//   - ctx context.Context
+//   - pr int
+//   - comment string
+func (_e *MockPullRequestRepo_Expecter) CommentPullRequest(ctx interface{}, pr interface{}, comment interface{}) *MockPullRequestRepo_CommentPullRequest_Call {
+	return &MockPullRequestRepo_CommentPullRequest_Call{Call: _e.mock.On("CommentPullRequest", ctx, pr, comment)}
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequest_Call) Run(run func(ctx context.Context, pr int, comment string)) *MockPullRequestRepo_CommentPullRequest_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(int), args[2].(string))
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequest_Call) Return(_a0 error) *MockPullRequestRepo_CommentPullRequest_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequest_Call) RunAndReturn(run func(context.Context, int, string) error) *MockPullRequestRepo_CommentPullRequest_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// CommentPullRequestFile provides a mock function with given fields: ctx, pr, path, ref, comment
+func (_m *MockPullRequestRepo) CommentPullRequestFile(ctx context.Context, pr int, path string, ref string, comment string) error {
+	ret := _m.Called(ctx, pr, path, ref, comment)
+
+	if len(ret) == 0 {
+		panic("no return value specified for CommentPullRequestFile")
+	}
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context, int, string, string, string) error); ok {
+		r0 = rf(ctx, pr, path, ref, comment)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// MockPullRequestRepo_CommentPullRequestFile_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CommentPullRequestFile'
+type MockPullRequestRepo_CommentPullRequestFile_Call struct {
+	*mock.Call
+}
+
+// CommentPullRequestFile is a helper method to define mock.On call
+//   - ctx context.Context
+//   - pr int
+//   - path string
+//   - ref string
+//   - comment string
+func (_e *MockPullRequestRepo_Expecter) CommentPullRequestFile(ctx interface{}, pr interface{}, path interface{}, ref interface{}, comment interface{}) *MockPullRequestRepo_CommentPullRequestFile_Call {
+	return &MockPullRequestRepo_CommentPullRequestFile_Call{Call: _e.mock.On("CommentPullRequestFile", ctx, pr, path, ref, comment)}
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequestFile_Call) Run(run func(ctx context.Context, pr int, path string, ref string, comment string)) *MockPullRequestRepo_CommentPullRequestFile_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(int), args[2].(string), args[3].(string), args[4].(string))
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequestFile_Call) Return(_a0 error) *MockPullRequestRepo_CommentPullRequestFile_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CommentPullRequestFile_Call) RunAndReturn(run func(context.Context, int, string, string, string) error) *MockPullRequestRepo_CommentPullRequestFile_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// CompareFiles provides a mock function with given fields: ctx, base, ref
+func (_m *MockPullRequestRepo) CompareFiles(ctx context.Context, base string, ref string) ([]repository.VersionedFileChange, error) {
+	ret := _m.Called(ctx, base, ref)
+
+	if len(ret) == 0 {
+		panic("no return value specified for CompareFiles")
+	}
+
+	var r0 []repository.VersionedFileChange
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, string, string) ([]repository.VersionedFileChange, error)); ok {
+		return rf(ctx, base, ref)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, string, string) []repository.VersionedFileChange); ok {
+		r0 = rf(ctx, base, ref)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]repository.VersionedFileChange)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok {
+		r1 = rf(ctx, base, ref)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockPullRequestRepo_CompareFiles_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CompareFiles'
+type MockPullRequestRepo_CompareFiles_Call struct {
+	*mock.Call
+}
+
+// CompareFiles is a helper method to define mock.On call
+//   - ctx context.Context
+//   - base string
+//   - ref string
+func (_e *MockPullRequestRepo_Expecter) CompareFiles(ctx interface{}, base interface{}, ref interface{}) *MockPullRequestRepo_CompareFiles_Call {
+	return &MockPullRequestRepo_CompareFiles_Call{Call: _e.mock.On("CompareFiles", ctx, base, ref)}
+}
+
+func (_c *MockPullRequestRepo_CompareFiles_Call) Run(run func(ctx context.Context, base string, ref string)) *MockPullRequestRepo_CompareFiles_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(string), args[2].(string))
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CompareFiles_Call) Return(_a0 []repository.VersionedFileChange, _a1 error) *MockPullRequestRepo_CompareFiles_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_CompareFiles_Call) RunAndReturn(run func(context.Context, string, string) ([]repository.VersionedFileChange, error)) *MockPullRequestRepo_CompareFiles_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// Config provides a mock function with no fields
+func (_m *MockPullRequestRepo) Config() *v0alpha1.Repository {
+	ret := _m.Called()
+
+	if len(ret) == 0 {
+		panic("no return value specified for Config")
+	}
+
+	var r0 *v0alpha1.Repository
+	if rf, ok := ret.Get(0).(func() *v0alpha1.Repository); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*v0alpha1.Repository)
+		}
+	}
+
+	return r0
+}
+
+// MockPullRequestRepo_Config_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Config'
+type MockPullRequestRepo_Config_Call struct {
+	*mock.Call
+}
+
+// Config is a helper method to define mock.On call
+func (_e *MockPullRequestRepo_Expecter) Config() *MockPullRequestRepo_Config_Call {
+	return &MockPullRequestRepo_Config_Call{Call: _e.mock.On("Config")}
+}
+
+func (_c *MockPullRequestRepo_Config_Call) Run(run func()) *MockPullRequestRepo_Config_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run()
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_Config_Call) Return(_a0 *v0alpha1.Repository) *MockPullRequestRepo_Config_Call {
+	_c.Call.Return(_a0)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_Config_Call) RunAndReturn(run func() *v0alpha1.Repository) *MockPullRequestRepo_Config_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// Read provides a mock function with given fields: ctx, path, ref
+func (_m *MockPullRequestRepo) Read(ctx context.Context, path string, ref string) (*repository.FileInfo, error) {
+	ret := _m.Called(ctx, path, ref)
+
+	if len(ret) == 0 {
+		panic("no return value specified for Read")
+	}
+
+	var r0 *repository.FileInfo
+	var r1 error
+	if rf, ok := ret.Get(0).(func(context.Context, string, string) (*repository.FileInfo, error)); ok {
+		return rf(ctx, path, ref)
+	}
+	if rf, ok := ret.Get(0).(func(context.Context, string, string) *repository.FileInfo); ok {
+		r0 = rf(ctx, path, ref)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*repository.FileInfo)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(context.Context, string, string) error); ok {
+		r1 = rf(ctx, path, ref)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
+// MockPullRequestRepo_Read_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Read'
+type MockPullRequestRepo_Read_Call struct {
+	*mock.Call
+}
+
+// Read is a helper method to define mock.On call
+//   - ctx context.Context
+//   - path string
+//   - ref string
+func (_e *MockPullRequestRepo_Expecter) Read(ctx interface{}, path interface{}, ref interface{}) *MockPullRequestRepo_Read_Call {
+	return &MockPullRequestRepo_Read_Call{Call: _e.mock.On("Read", ctx, path, ref)}
+}
+
+func (_c *MockPullRequestRepo_Read_Call) Run(run func(ctx context.Context, path string, ref string)) *MockPullRequestRepo_Read_Call {
+	_c.Call.Run(func(args mock.Arguments) {
+		run(args[0].(context.Context), args[1].(string), args[2].(string))
+	})
+	return _c
+}
+
+func (_c *MockPullRequestRepo_Read_Call) Return(_a0 *repository.FileInfo, _a1 error) *MockPullRequestRepo_Read_Call {
+	_c.Call.Return(_a0, _a1)
+	return _c
+}
+
+func (_c *MockPullRequestRepo_Read_Call) RunAndReturn(run func(context.Context, string, string) (*repository.FileInfo, error)) *MockPullRequestRepo_Read_Call {
+	_c.Call.Return(run)
+	return _c
+}
+
+// NewMockPullRequestRepo creates a new instance of MockPullRequestRepo. 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 NewMockPullRequestRepo(t interface {
+	mock.TestingT
+	Cleanup(func())
+}) *MockPullRequestRepo {
+	mock := &MockPullRequestRepo{}
+	mock.Mock.Test(t)
+
+	t.Cleanup(func() { mock.AssertExpectations(t) })
+
+	return mock
+}

pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go

@@ -1,4 +1,4 @@
-// Code generated by mockery v2.53.3. DO NOT EDIT.
+// Code generated by mockery v2.52.4. DO NOT EDIT.
 
 package pullrequest
 

pkg/registry/apis/provisioning/jobs/pullrequest/worker.go

@@ -14,6 +14,7 @@ import (
 	"github.com/grafana/grafana/pkg/registry/apis/provisioning/resources"
 )
 
+//go:generate mockery --name=PullRequestRepo --structname=MockPullRequestRepo --inpackage --filename=mock_pullrequest_repo.go --with-expecter
 type PullRequestRepo interface {
 	Config() *provisioning.Repository
 	Read(ctx context.Context, path, ref string) (*repository.FileInfo, error)
@@ -23,23 +24,25 @@ type PullRequestRepo interface {
 	CommentPullRequest(ctx context.Context, pr int, comment string) error
 }
 
-type PullRequestWorker struct {
-	parsers     resources.ParserFactory
-	renderer    ScreenshotRenderer
-	urlProvider func(namespace string) string
-	commenter   *commentBuilder
+//go:generate mockery --name=Evaluator --structname=MockEvaluator --inpackage --filename=mock_evaluator.go --with-expecter
+type Evaluator interface {
+	Evaluate(ctx context.Context, repo repository.Reader, opts provisioning.PullRequestJobOptions, changes []repository.VersionedFileChange, progress jobs.JobProgressRecorder) (changeInfo, error)
 }
 
-func NewPullRequestWorker(
-	parsers resources.ParserFactory,
-	renderer ScreenshotRenderer,
-	urlProvider func(namespace string) string,
-) *PullRequestWorker {
+//go:generate mockery --name=Commenter --structname=MockCommenter --inpackage --filename=mock_commenter.go --with-expecter
+type Commenter interface {
+	Comment(ctx context.Context, repo PullRequestRepo, pr int, changeInfo changeInfo) error
+}
+
+type PullRequestWorker struct {
+	evaluator Evaluator
+	commenter Commenter
+}
+
+func NewPullRequestWorker(evaluator Evaluator, commenter Commenter) *PullRequestWorker {
 	return &PullRequestWorker{
-		parsers:     parsers,
-		renderer:    renderer,
-		urlProvider: urlProvider,
-		commenter:   newCommentBuilder(),
+		evaluator: evaluator,
+		commenter: commenter,
 	}
 }
 
@@ -53,15 +56,16 @@ func (c *PullRequestWorker) Process(ctx context.Context,
 	progress jobs.JobProgressRecorder,
 ) error {
 	cfg := repo.Config().Spec
-	options := job.Spec.PullRequest
-	if options == nil {
+	opts := job.Spec.PullRequest
+	if opts == nil {
 		return apierrors.NewBadRequest("missing spec.pr")
 	}
 
-	if options.Ref == "" {
+	if opts.Ref == "" {
 		return apierrors.NewBadRequest("missing spec.ref")
 	}
 
+	// FIXME: this is leaky because it's supposed to be already a PullRequestRepo
 	if cfg.GitHub == nil {
 		return apierrors.NewBadRequest("expecting github configuration")
 	}
@@ -76,51 +80,34 @@ func (c *PullRequestWorker) Process(ctx context.Context,
 		return errors.New("pull request job submitted targeting repository that is not a Reader")
 	}
 
-	logger := logging.FromContext(ctx).With("pr", options.PR)
+	logger := logging.FromContext(ctx).With("pr", opts.PR)
 	logger.Info("process pull request")
 	defer logger.Info("pull request processed")
 
 	progress.SetMessage(ctx, "listing pull request files")
+	// FIXME: this is leaky because it's supposed to be already a PullRequestRepo
 	base := cfg.GitHub.Branch
-	files, err := prRepo.CompareFiles(ctx, base, options.Ref)
+	files, err := prRepo.CompareFiles(ctx, base, opts.Ref)
 	if err != nil {
-		return fmt.Errorf("failed to list pull request files: %s", err.Error())
+		return fmt.Errorf("failed to list pull request files: %w", err)
 	}
 
 	files = onlySupportedFiles(files)
-
 	if len(files) == 0 {
 		progress.SetFinalMessage(ctx, "no files to process")
 		return nil
 	}
 
-	parser, err := c.parsers.GetParser(ctx, reader)
+	changeInfo, err := c.evaluator.Evaluate(ctx, reader, *opts, files, progress)
 	if err != nil {
-		return fmt.Errorf("failed to get parser for %s: %w", repo.Config().Name, err)
+		return fmt.Errorf("calculate changes: %w", err)
 	}
 
-	var render ScreenshotRenderer
-	if cfg.GitHub.GenerateDashboardPreviews {
-		render = c.renderer
-	}
-
-	changeInfo, err := processChangedFiles(ctx, changeOptions{
-		grafanaBaseURL: c.urlProvider(repo.Config().Namespace),
-		pullRequest:    *options,
-		changes:        files,
-		parser:         parser,
-		reader:         reader,
-		progress:       progress,
-		render:         render,
-	})
-	if err != nil {
-		return fmt.Errorf("unable to calculate changes: %w", err)
-	}
-
-	if err := c.commenter.Comment(ctx, prRepo, options.PR, changeInfo); err != nil {
+	if err := c.commenter.Comment(ctx, prRepo, opts.PR, changeInfo); err != nil {
 		return fmt.Errorf("comment pull request: %w", err)
 	}
 	logger.Info("preview comment added")
+
 	return nil
 }
 

pkg/registry/apis/provisioning/register.go

@@ -559,7 +559,9 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH
 
 			// Pull request worker
 			renderer := pullrequest.NewScreenshotRenderer(b.render, b.unified)
-			pullRequestWorker := pullrequest.NewPullRequestWorker(b.parsers, renderer, b.urlProvider)
+			evaluator := pullrequest.NewEvaluator(renderer, b.parsers, b.urlProvider)
+			commenter := pullrequest.NewCommenter()
+			pullRequestWorker := pullrequest.NewPullRequestWorker(evaluator, commenter)
 
 			driver := jobs.NewJobDriver(time.Second*28, time.Second*30, time.Second*30, b.jobs, b, b.jobHistory,
 				exportWorker, syncWorker, migrationWorker, pullRequestWorker)