From 4c21e7f8c203db5e25eca5d693756f3e3e140955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Fri, 11 Apr 2025 17:18:41 +0200 Subject: [PATCH] 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 --- .../provisioning/jobs/pullrequest/changes.go | 79 ++-- .../jobs/pullrequest/changes_test.go | 74 ++-- .../provisioning/jobs/pullrequest/comment.go | 14 +- .../jobs/pullrequest/comment_test.go | 27 +- .../jobs/pullrequest/mock_commenter.go | 85 +++++ .../jobs/pullrequest/mock_evaluator.go | 101 +++++ .../jobs/pullrequest/mock_pullrequest_repo.go | 351 ++++++++++++++++++ .../jobs/pullrequest/render_mock.go | 2 +- .../provisioning/jobs/pullrequest/worker.go | 71 ++-- pkg/registry/apis/provisioning/register.go | 4 +- 10 files changed, 670 insertions(+), 138 deletions(-) create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/mock_commenter.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/mock_evaluator.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/mock_pullrequest_repo.go diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go b/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go index 7946f969789..961485b1eb6 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go +++ b/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 } diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go b/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go index 9f0e04947f0..c45374cc1f4 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go +++ b/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) diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go b/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go index e7c098af776..12003b73305 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go +++ b/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 | diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go b/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go index 9d5271ed0a3..ec0d0da27bf 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go +++ b/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) }) } } diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/mock_commenter.go b/pkg/registry/apis/provisioning/jobs/pullrequest/mock_commenter.go new file mode 100644 index 00000000000..785e552f68d --- /dev/null +++ b/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 +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/mock_evaluator.go b/pkg/registry/apis/provisioning/jobs/pullrequest/mock_evaluator.go new file mode 100644 index 00000000000..54673cd92d8 --- /dev/null +++ b/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 +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/mock_pullrequest_repo.go b/pkg/registry/apis/provisioning/jobs/pullrequest/mock_pullrequest_repo.go new file mode 100644 index 00000000000..882ee50cbe9 --- /dev/null +++ b/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 +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go b/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go index 0bb99e8a740..f7aa5bb5039 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go +++ b/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 diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go b/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go index 0fea17d09b6..c95a37dbb7e 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go +++ b/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 } diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 9db18a4e9be..60650cfdd16 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/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)