From 0d206806958be88afa59ba9149d8ce8a13c5c649 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Fri, 11 Apr 2025 13:06:16 +0300 Subject: [PATCH] Provisioning: Improve PR feedback (#103537) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix ref * dryRun before save * reuse code for create vs update * update comments * can update PRs * return useful errors * update preview links * lint fix * update some wording * update mocks * fixed * Update pkg/registry/apis/provisioning/resources/dualwriter.go Co-authored-by: Roberto Jiménez Sánchez * more updates * fix link * error string * dry run * refactor galore * with template test * very basic tests * more test cases * remove generator * more tests * fix lint * multiple files now * merge main * merge main * fix snapshots * fix snapshots * slugify title --------- Co-authored-by: Roberto Jiménez Sánchez --- .../apis/provisioning/jobs/migrate/worker.go | 6 +- .../provisioning/jobs/pullrequest/changes.go | 220 ++++++++++++++++++ .../jobs/pullrequest/changes_test.go | 204 ++++++++++++++++ .../provisioning/jobs/pullrequest/comment.go | 124 ++++++++++ .../jobs/pullrequest/comment_test.go | 145 ++++++++++++ .../provisioning/jobs/pullrequest/preview.go | 192 --------------- .../jobs/pullrequest/preview_renderer_mock.go | 142 ----------- .../jobs/pullrequest/previewer_mock.go | 156 ------------- .../provisioning/jobs/pullrequest/render.go | 58 +++-- .../jobs/pullrequest/render_mock.go | 144 ++++++++++++ .../pullrequest/testdata/multiple-files.md | 11 + .../pullrequest/testdata/new-dashboard.md | 8 + .../update-dashboard-missing-renderer.md | 7 + .../pullrequest/testdata/update-dashboard.md | 9 + .../provisioning/jobs/pullrequest/worker.go | 117 +++++----- pkg/registry/apis/provisioning/register.go | 5 +- .../apis/provisioning/repository/local.go | 9 +- .../provisioning/repository/local_test.go | 4 +- .../provisioning/repository/repository.go | 9 +- .../apis/provisioning/resources/dualwriter.go | 111 ++++----- .../apis/provisioning/resources/parser.go | 12 +- .../apis/provisioning/provisioning_test.go | 16 +- .../GettingStarted/GettingStarted.tsx | 1 - 23 files changed, 1052 insertions(+), 658 deletions(-) create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/changes.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/comment.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go delete mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/preview.go delete mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/preview_renderer_mock.go delete mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/previewer_mock.go create mode 100644 pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go create mode 100755 pkg/registry/apis/provisioning/jobs/pullrequest/testdata/multiple-files.md create mode 100755 pkg/registry/apis/provisioning/jobs/pullrequest/testdata/new-dashboard.md create mode 100755 pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard-missing-renderer.md create mode 100755 pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard.md diff --git a/pkg/registry/apis/provisioning/jobs/migrate/worker.go b/pkg/registry/apis/provisioning/jobs/migrate/worker.go index b2992e6d261..657b4348731 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/worker.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/worker.go @@ -142,7 +142,7 @@ func (w *MigrationWorker) migrateFromLegacy(ctx context.Context, rw repository.R } namespace := rw.Config().Namespace - progress.SetMessage(ctx, "loading legacy folders") + progress.SetMessage(ctx, "loading folders from SQL") reader := NewLegacyFolderReader(w.legacyMigrator, rw.Config().Name, namespace) if err = reader.Read(ctx, w.legacyMigrator, rw.Config().Name, namespace); err != nil { return fmt.Errorf("error loading folder tree: %w", err) @@ -154,7 +154,7 @@ func (w *MigrationWorker) migrateFromLegacy(ctx context.Context, rw repository.R } folders := resources.NewFolderManager(rw, folderClient, resources.NewEmptyFolderTree()) - progress.SetMessage(ctx, "exporting legacy folders") + progress.SetMessage(ctx, "exporting folders from SQL") err = folders.EnsureFolderTreeExists(ctx, "", "", reader.Tree(), func(folder resources.Folder, created bool, err error) error { result := jobs.JobResourceResult{ Action: repository.FileActionCreated, @@ -176,7 +176,7 @@ func (w *MigrationWorker) migrateFromLegacy(ctx context.Context, rw repository.R return fmt.Errorf("error exporting legacy folders: %w", err) } - progress.SetMessage(ctx, "exporting legacy resources") + progress.SetMessage(ctx, "exporting resources from SQL") resourceManager := resources.NewResourcesManager(rw, folders, parser, clients, userInfo) for _, kind := range resources.SupportedProvisioningResources { if kind == resources.FolderResource { diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go b/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go new file mode 100644 index 00000000000..7946f969789 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/changes.go @@ -0,0 +1,220 @@ +package pullrequest + +import ( + "context" + "fmt" + "net/url" + "path" + "strings" + + "github.com/grafana/grafana-app-sdk/logging" + dashboard "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1alpha1" + provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" + "github.com/grafana/grafana/pkg/infra/slugify" + "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" +) + +type changeInfo struct { + GrafanaBaseURL string + + // Files we tried to read + Changes []fileChangeInfo + + // More files changed than we processed + SkippedFiles int + + // Requested image render, but it is not available + MissingImageRenderer bool + HasScreenshot bool +} + +type fileChangeInfo struct { + Change repository.VersionedFileChange + Error string + + // The parsed value + Parsed *resources.ParsedResource + + // The title from inside the resource (or name if not found) + Title string + + // The URL where this will appear (target) + GrafanaURL string + GrafanaScreenshotURL string + + // URL where we can see a preview of this particular change + PreviewURL string + 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 +} + +// 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, + } + + if opts.render != nil { + if !opts.render.IsAvailable(ctx) { + info.MissingImageRenderer = true + opts.render = nil + } + + // Only render images when there is just one change + if len(opts.changes) > 1 { + opts.render = nil + } + } + + logger := logging.FromContext(ctx) + for i, change := range opts.changes { + // process maximum 10 files + if i >= 10 { + info.SkippedFiles = len(opts.changes) - i + break + } + + opts.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) + 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 { + info.MissingImageRenderer = true + if v.Error == "" { + v.Error = "Error running image rendering" + } + + if v.GrafanaScreenshotURL != "" || v.PreviewScreenshotURL != "" { + info.HasScreenshot = true + } + } + } + + info.Changes = append(info.Changes, v) + } + return info, nil +} + +var dashboardKind = dashboard.DashboardResourceInfo.GroupVersionKind().Kind + +func calculateFileChangeInfo(ctx context.Context, baseURL string, change repository.VersionedFileChange, opts changeOptions) (fileChangeInfo, error) { + if change.Action == repository.FileActionDeleted { + return calculateFileDeleteInfo(ctx, baseURL, change, opts) + } + + info := fileChangeInfo{Change: change} + fileInfo, err := opts.reader.Read(ctx, change.Path, change.Ref) + if err != nil { + logger.Info("unable to read file", "err", err) + info.Error = err.Error() + return info, nil + } + + // Read the file as a resource + info.Parsed, err = opts.parser.Parse(ctx, fileInfo) + if err != nil { + info.Error = err.Error() + return info, nil + } + + // Find a name within the file + obj := info.Parsed.Obj + info.Title = info.Parsed.Meta.FindTitle(obj.GetName()) + + // Check what happens when we apply changes + // NOTE: this will also invoke any server side validation + err = info.Parsed.DryRun(ctx) + if err != nil { + info.Error = err.Error() + return info, nil + } + + // Dashboards get special handling + if info.Parsed.GVK.Kind == dashboardKind { + if info.Parsed.Existing != nil { + info.GrafanaURL = fmt.Sprintf("%sd/%s/%s", baseURL, obj.GetName(), + slugify.Slugify(info.Title)) + } + + // Load this file directly + info.PreviewURL = baseURL + path.Join("admin/provisioning", + info.Parsed.Repo.Name, "dashboard/preview", info.Parsed.Info.Path) + + query := url.Values{} + query.Set("ref", info.Parsed.Info.Ref) + if opts.pullRequest.URL != "" { + query.Set("pull_request_url", url.QueryEscape(opts.pullRequest.URL)) + } + info.PreviewURL += "?" + query.Encode() + } + + return info, nil +} + +func calculateFileDeleteInfo(_ context.Context, _ string, change repository.VersionedFileChange, opts changeOptions) (fileChangeInfo, error) { + // TODO -- read the old and verify + return fileChangeInfo{Change: change, Error: "delete feedback not yet implemented"}, nil +} + +// This will update render the linked screenshots and update the screenshotURLs +func (f *fileChangeInfo) renderScreenshots(ctx context.Context, baseURL string, renderer ScreenshotRenderer) (err error) { + if f.GrafanaURL != "" { + f.GrafanaScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, renderer, f.Parsed.Repo, f.GrafanaURL) + if err != nil { + return err + } + } + if f.PreviewURL != "" { + f.PreviewScreenshotURL, err = renderScreenshotFromGrafanaURL(ctx, baseURL, renderer, f.Parsed.Repo, f.PreviewURL) + if err != nil { + return err + } + } + return nil +} + +func renderScreenshotFromGrafanaURL(ctx context.Context, + baseURL string, + renderer ScreenshotRenderer, + repo provisioning.ResourceRepositoryInfo, + grafanaURL string, +) (string, error) { + parsed, err := url.Parse(grafanaURL) + if err != nil { + logging.FromContext(ctx).Warn("invalid", "url", grafanaURL, "err", err) + return "", err + } + snap, err := renderer.RenderScreenshot(ctx, repo, strings.TrimPrefix(parsed.Path, "/"), parsed.Query()) + if err != nil { + logging.FromContext(ctx).Warn("render failed", "url", grafanaURL, "err", err) + return "", fmt.Errorf("error rendering screenshot %w", err) + } + if strings.Contains(snap, "://") { + return snap, nil // it is a full URL already (can happen when the blob storage returns CDN urls) + } + base, err := url.Parse(baseURL) + if err != nil { + logger.Warn("invalid base", "url", baseURL, "err", err) + return "", err + } + return base.JoinPath(snap).String(), nil +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go b/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go new file mode 100644 index 00000000000..9f0e04947f0 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/changes_test.go @@ -0,0 +1,204 @@ +package pullrequest + +import ( + "context" + "crypto/sha256" + "encoding/binary" + "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" +) + +func TestCalculateChanges(t *testing.T) { + parser := resources.NewMockParser(t) + reader := repository.NewMockReader(t) + progress := jobs.NewMockJobProgressRecorder(t) + + finfo := &repository.FileInfo{ + Path: "path/to/file.json", + Ref: "ref", + Data: []byte("xxxx"), // not a valid JSON! + } + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": resources.DashboardResource.GroupVersion().String(), + "kind": dashboardKind, // will trigger creating a URL + "metadata": map[string]interface{}{ + "name": "the-uid", + }, + "spec": map[string]interface{}{ + "title": "hello world", // has spaces + }, + }, + } + meta, _ := utils.MetaAccessor(obj) + + progress.On("SetMessage", mock.Anything, mock.Anything).Return() + reader.On("Read", mock.Anything, "path/to/file.json", "ref").Return(finfo, nil) + parser.On("Parse", mock.Anything, finfo).Return(&resources.ParsedResource{ + Info: finfo, + Repo: v0alpha1.ResourceRepositoryInfo{ + Namespace: "x", + Name: "y", + }, + GVK: schema.GroupVersionKind{ + Kind: dashboardKind, + }, + Obj: obj, + Existing: obj, + Meta: meta, + DryRunResponse: obj, // avoid hitting the client + }, nil) + + pullRequest := v0alpha1.PullRequestJobOptions{ + Ref: "ref", + PR: 123, + URL: "http://github.com/pr/", + } + createdFileChange := repository.VersionedFileChange{ + Action: repository.FileActionCreated, + Path: "path/to/file.json", + Ref: "ref", + } + + t.Run("with-screenshot", func(t *testing.T) { + renderer := NewMockScreenshotRenderer(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) + + options := changeOptions{ + grafanaBaseURL: "http://host/", + pullRequest: pullRequest, + changes: []repository.VersionedFileChange{createdFileChange}, + parser: parser, + reader: reader, + progress: progress, + render: renderer, + } + + info, err := processChangedFiles(context.Background(), options) + require.NoError(t, err) + + require.False(t, info.MissingImageRenderer) + require.Equal(t, map[string]string{ + "Grafana": "http://host/d/the-uid/hello-world", + "GrafanaSnapshot": "https://cdn2.thecatapi.com/images/9e2.jpg", + "Preview": "http://host/admin/provisioning/y/dashboard/preview/path/to/file.json?pull_request_url=http%253A%252F%252Fgithub.com%252Fpr%252F&ref=ref", + "PreviewSnapshot": "https://cdn2.thecatapi.com/images/9e2.jpg", + }, map[string]string{ + "Grafana": info.Changes[0].GrafanaURL, + "GrafanaSnapshot": info.Changes[0].GrafanaScreenshotURL, + "Preview": info.Changes[0].PreviewURL, + "PreviewSnapshot": info.Changes[0].PreviewScreenshotURL, + }) + }) + + 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, + } + + info, err := processChangedFiles(context.Background(), options) + require.NoError(t, err) + + require.True(t, info.MissingImageRenderer) + require.Equal(t, map[string]string{ + "Grafana": "http://host/d/the-uid/hello-world", + "GrafanaSnapshot": "", + "Preview": "http://host/admin/provisioning/y/dashboard/preview/path/to/file.json?pull_request_url=http%253A%252F%252Fgithub.com%252Fpr%252F&ref=ref", + "PreviewSnapshot": "", + }, map[string]string{ + "Grafana": info.Changes[0].GrafanaURL, + "GrafanaSnapshot": info.Changes[0].GrafanaScreenshotURL, + "Preview": info.Changes[0].PreviewURL, + "PreviewSnapshot": info.Changes[0].PreviewScreenshotURL, + }) + }) + + t.Run("process first 10 files", func(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 + } + for range 15 { + options.changes = append(options.changes, createdFileChange) + } + + info, err := processChangedFiles(context.Background(), options) + require.NoError(t, err) + + require.False(t, info.MissingImageRenderer) + require.Equal(t, 10, len(info.Changes)) + require.Equal(t, 5, info.SkippedFiles) + + // Make sure we linked a URL, but no screenshot for each item + for _, change := range info.Changes { + require.NotEmpty(t, change.GrafanaURL) + require.Empty(t, change.GrafanaScreenshotURL) + } + }) +} + +func TestDummyImageURL(t *testing.T) { + urls := []string{} + for i := range 10 { + urls = append(urls, getDummyRenderedURL(fmt.Sprintf("http://%d", i))) + } + require.Equal(t, []string{ + "https://cdn2.thecatapi.com/images/9e2.jpg", + "https://cdn2.thecatapi.com/images/bhs.jpg", + "https://cdn2.thecatapi.com/images/d54.jpg", + "https://cdn2.thecatapi.com/images/99c.jpg", + "https://cdn2.thecatapi.com/images/9e2.jpg", + "https://cdn2.thecatapi.com/images/bhs.jpg", + "https://cdn2.thecatapi.com/images/d54.jpg", + "https://cdn2.thecatapi.com/images/99c.jpg", + "https://cdn2.thecatapi.com/images/9e2.jpg", + "https://cdn2.thecatapi.com/images/bhs.jpg", + }, urls) +} + +// Returns a random (but stable) image for a string +func getDummyRenderedURL(url string) string { + dummy := []string{ + "https://cdn2.thecatapi.com/images/9e2.jpg", + "https://cdn2.thecatapi.com/images/bhs.jpg", + "https://cdn2.thecatapi.com/images/d54.jpg", + "https://cdn2.thecatapi.com/images/99c.jpg", + } + + idx := 0 + hash := sha256.New() + bytes := hash.Sum([]byte(url)) + if len(bytes) > 8 { + v := binary.BigEndian.Uint64(bytes[0:8]) + idx = int(v) % len(dummy) + } + return dummy[idx] +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go b/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go new file mode 100644 index 00000000000..e7c098af776 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/comment.go @@ -0,0 +1,124 @@ +package pullrequest + +import ( + "bytes" + "context" + "fmt" + "html/template" + "path/filepath" + "strings" +) + +type commentBuilder struct { + templateDashboard *template.Template + templateTable *template.Template + templateRenderInfo *template.Template +} + +func newCommentBuilder() *commentBuilder { + return &commentBuilder{ + 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 { + comment, err := c.generateComment(ctx, info) + if err != nil { + return fmt.Errorf("unable to generate comment text: %w", err) + } + + 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) { + 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) + } + } else { + if err := c.templateTable.Execute(&buf, info); err != nil { + return "", fmt.Errorf("unable to execute template: %w", err) + } + } + + if info.MissingImageRenderer { + if err := c.templateRenderInfo.Execute(&buf, info); err != nil { + return "", fmt.Errorf("unable to execute template: %w", err) + } + } + + return strings.TrimSpace(buf.String()), nil +} + +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 | +|----------|---------| +| ![Before]({{.GrafanaScreenshotURL}}) | ![Preview]({{.PreviewScreenshotURL}}) | +{{- else if .GrafanaScreenshotURL}} +### Original of {{.Title}} +![Original]({{.GrafanaScreenshotURL}}) +{{- else if .PreviewScreenshotURL}} +### Preview of {{.Parsed.Info.Path}} +![Preview]({{.PreviewScreenshotURL}}) +{{ end}} + +{{ if and .GrafanaURL .PreviewURL}} +See the [original]({{.GrafanaURL}}) and [preview]({{.PreviewURL}}) of {{.Parsed.Info.Path}}. +{{- else if .GrafanaURL}} +See the [original]({{.GrafanaURL}}) of {{.Title}}. +{{- else if .PreviewURL}} +See the [preview]({{.PreviewURL}}) of {{.Parsed.Info.Path}}. +{{- end}} +` + +const commentTemplateTable = `Hey there! 🎉 +Grafana spotted some changes. + +| Action | Kind | Resource | Preview | +|--------|------|----------|---------| +{{- range .Changes}} +| {{.Parsed.Action}} | {{.Kind}} | {{.ExistingLink}} | {{ if .PreviewURL}}[preview]({{.PreviewURL}}){{ end }} | +{{- end}} + +{{ if .SkippedFiles }} +and {{ .SkippedFiles }} more files. +{{ end}} +` + +// TODO: this should expand and show links to setup docs +const commentTemplateMissingImageRenderer = ` +NOTE: The image renderer is not configured +` + +func (f *fileChangeInfo) Kind() string { + if f.Parsed == nil { + return filepath.Ext(f.Change.Path) + } + v := f.Parsed.GVK.Kind + if v == "" { + return filepath.Ext(f.Parsed.Info.Path) + } + return f.Parsed.GVK.Kind +} + +func (f *fileChangeInfo) ExistingLink() string { + if f.GrafanaURL != "" { + return fmt.Sprintf("[%s](%s)", f.Title, f.GrafanaURL) + } + return f.Title +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go b/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go new file mode 100644 index 00000000000..9d5271ed0a3 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/comment_test.go @@ -0,0 +1,145 @@ +package pullrequest + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" +) + +func TestGenerateComment(t *testing.T) { + builder := newCommentBuilder() + + for _, tc := range []struct { + Name string + Input changeInfo + }{ + {"new dashboard", changeInfo{ + GrafanaBaseURL: "http://host/", + Changes: []fileChangeInfo{ + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "file.json", + }, + GVK: schema.GroupVersionKind{Kind: "Dashboard"}, + Action: v0alpha1.ResourceActionCreate, + }, + Title: "New Dashboard", + PreviewURL: "http://grafana/admin/preview", + PreviewScreenshotURL: getDummyRenderedURL("http://grafana/admin/preview"), + }, + }, + }}, + {"update dashboard", changeInfo{ + GrafanaBaseURL: "http://host/", + Changes: []fileChangeInfo{ + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "file.json", + }, + Action: v0alpha1.ResourceActionUpdate, + GVK: schema.GroupVersionKind{Kind: "Dashboard"}, + }, + Title: "Existing Dashboard", + GrafanaURL: "http://grafana/d/uid", + PreviewURL: "http://grafana/admin/preview", + + GrafanaScreenshotURL: getDummyRenderedURL("http://grafana/d/uid"), + PreviewScreenshotURL: getDummyRenderedURL("http://grafana/admin/preview"), + }, + }, + }}, + {"update dashboard missing renderer", changeInfo{ + GrafanaBaseURL: "http://host/", + Changes: []fileChangeInfo{ + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "file.json", + }, + Action: v0alpha1.ResourceActionUpdate, + GVK: schema.GroupVersionKind{Kind: "Dashboard"}, + }, + Title: "Existing Dashboard", + GrafanaURL: "http://grafana/d/uid", + PreviewURL: "http://grafana/admin/preview", + }, + }, + MissingImageRenderer: true, + }}, + {"multiple files", changeInfo{ + GrafanaBaseURL: "http://host/", + SkippedFiles: 5, + Changes: []fileChangeInfo{ + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "aaa.json", + }, + Action: v0alpha1.ResourceActionCreate, + GVK: schema.GroupVersionKind{Kind: "Dashboard"}, + }, + Title: "Dash A", + PreviewURL: "http://grafana/admin/preview", + }, + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "bbb.json", + }, + Action: v0alpha1.ResourceActionUpdate, + GVK: schema.GroupVersionKind{Kind: "Dashboard"}, + }, + Title: "Dash B", + GrafanaURL: "http://grafana/d/bbb", + PreviewURL: "http://grafana/admin/preview", + }, + { + Parsed: &resources.ParsedResource{ + Info: &repository.FileInfo{ + Path: "bbb.json", + }, + Action: v0alpha1.ResourceActionCreate, + GVK: schema.GroupVersionKind{Kind: "Playlist"}, + }, + Title: "My Playlist", + }, + }, + }}, + } { + t.Run(tc.Name, func(t *testing.T) { + comment, err := builder.generateComment(context.Background(), tc.Input) + require.NoError(t, err) + + 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) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/preview.go b/pkg/registry/apis/provisioning/jobs/pullrequest/preview.go deleted file mode 100644 index 7a124f384cc..00000000000 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/preview.go +++ /dev/null @@ -1,192 +0,0 @@ -package pullrequest - -import ( - "bytes" - "context" - "fmt" - "html/template" - "net/url" - "path" - - "github.com/grafana/grafana-app-sdk/logging" - "github.com/grafana/grafana/pkg/cmd/grafana-cli/logger" - "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" -) - -// resourcePreview represents a resource that has changed in a pull request. -type resourcePreview struct { - Filename string - Path string - Action string - Kind string - OriginalURL string - OriginalScreenshotURL string - PreviewURL string - PreviewScreenshotURL string -} - -const previewsCommentTemplate = `Hey there! 🎉 -Grafana spotted some changes in your dashboard. - -{{- if and .OriginalScreenshotURL .PreviewScreenshotURL}} -### Side by Side Comparison of {{.Filename}} -| Original | Preview | -|----------|---------| -| ![Original]({{.OriginalScreenshotURL}}) | ![Preview]({{.PreviewScreenshotURL}}) | -{{- else if .OriginalScreenshotURL}} -### Original of {{.Filename}} -![Original]({{.OriginalScreenshotURL}}) -{{- else if .PreviewScreenshotURL}} -### Preview of {{.Filename}} -![Preview]({{.PreviewScreenshotURL}}) -{{ end}} - -{{ if and .OriginalURL .PreviewURL}} -See the [original]({{.OriginalURL}}) and [preview]({{.PreviewURL}}) of {{.Filename}}. -{{- else if .OriginalURL}} -See the [original]({{.OriginalURL}}) of {{.Filename}}. -{{- else if .PreviewURL}} -See the [preview]({{.PreviewURL}}) of {{.Filename}}. -{{- end}}` - -// PreviewRenderer is an interface for rendering a preview of a file -// -//go:generate mockery --name PreviewRenderer --structname MockPreviewRenderer --inpackage --filename preview_renderer_mock.go --with-expecter -type PreviewRenderer interface { - IsAvailable(ctx context.Context) bool - RenderDashboardPreview(ctx context.Context, namespace, repoName, path, ref string) (string, error) -} - -// Previewer is a service for previewing dashboard changes in a pull request -// -//go:generate mockery --name Previewer --structname MockPreviewer --inpackage --filename previewer_mock.go --with-expecter -type Previewer interface { - Preview(ctx context.Context, f repository.VersionedFileChange, namespace, repoName, base, ref, pullRequestURL string, generatePreview bool) (resourcePreview, error) - GenerateComment(preview resourcePreview) (string, error) -} - -type previewer struct { - template *template.Template - urlProvider func(namespace string) string - renderer PreviewRenderer -} - -func NewPreviewer(renderer PreviewRenderer, urlProvider func(namespace string) string) *previewer { - return &previewer{ - template: template.Must(template.New("comment").Parse(previewsCommentTemplate)), - urlProvider: urlProvider, - renderer: renderer, - } -} - -// GenerateComment creates a formatted comment for dashboard previews -func (p *previewer) GenerateComment(preview resourcePreview) (string, error) { - var buf bytes.Buffer - if err := p.template.Execute(&buf, preview); err != nil { - return "", fmt.Errorf("execute previews comment template: %w", err) - } - return buf.String(), nil -} - -// getOriginalURL returns the URL for the original version of the file based on the action -func (p *previewer) getOriginalURL(ctx context.Context, f repository.VersionedFileChange, baseURL *url.URL, repoName, base, pullRequestURL string) string { - switch f.Action { - case repository.FileActionCreated: - return "" // No original URL for new files - case repository.FileActionUpdated: - return p.previewURL(baseURL, repoName, base, f.Path, pullRequestURL) - case repository.FileActionRenamed: - return p.previewURL(baseURL, repoName, base, f.PreviousPath, pullRequestURL) - case repository.FileActionDeleted: - return p.previewURL(baseURL, repoName, base, f.Path, pullRequestURL) - default: - logging.FromContext(ctx).Error("unknown file action for original URL", "action", f.Action) - return "" - } -} - -// getPreviewURL returns the URL for the preview version of the file based on the action -func (p *previewer) getPreviewURL(ctx context.Context, f repository.VersionedFileChange, baseURL *url.URL, repoName, ref, pullRequestURL string) string { - switch f.Action { - case repository.FileActionCreated, repository.FileActionUpdated, repository.FileActionRenamed: - return p.previewURL(baseURL, repoName, ref, f.Path, pullRequestURL) - case repository.FileActionDeleted: - return "" // No preview URL for deleted files - default: - logging.FromContext(ctx).Error("unknown file action for preview URL", "action", f.Action) - return "" - } -} - -// previewURL returns the URL to preview the file in Grafana -func (p *previewer) previewURL(u *url.URL, repoName, ref, filePath, pullRequestURL string) string { - baseURL := *u - baseURL = *baseURL.JoinPath("/admin/provisioning", repoName, "dashboard/preview", filePath) - - query := baseURL.Query() - if ref != "" { - query.Set("ref", ref) - } - if pullRequestURL != "" { - query.Set("pull_request_url", url.QueryEscape(pullRequestURL)) - } - baseURL.RawQuery = query.Encode() - - return baseURL.String() -} - -// Preview creates a preview for a single file change -func (p *previewer) Preview( - ctx context.Context, - f repository.VersionedFileChange, - namespace string, - repoName string, - base string, - ref string, - pullRequestURL string, - generatePreview bool, -) (resourcePreview, error) { - baseURL, err := url.Parse(p.urlProvider(namespace)) - if err != nil { - return resourcePreview{}, fmt.Errorf("error parsing base url: %w", err) - } - - preview := resourcePreview{ - Filename: path.Base(f.Path), - Path: f.Path, - Kind: "dashboard", // TODO: add more kinds - Action: string(f.Action), - OriginalURL: p.getOriginalURL(ctx, f, baseURL, repoName, base, pullRequestURL), - PreviewURL: p.getPreviewURL(ctx, f, baseURL, repoName, ref, pullRequestURL), - } - - if !generatePreview { - logger.Info("skipping dashboard preview generation", "path", f.Path) - return preview, nil - } - - if preview.PreviewURL != "" { - screenshotURL, err := p.renderer.RenderDashboardPreview(ctx, namespace, repoName, f.Path, ref) - if err != nil { - return resourcePreview{}, fmt.Errorf("render dashboard preview: %w", err) - } - preview.PreviewScreenshotURL = screenshotURL - logger.Info("dashboard preview screenshot generated", "screenshotURL", screenshotURL) - } - - if preview.OriginalURL != "" { - originalPath := f.PreviousPath - if originalPath == "" { - originalPath = f.Path - } - - screenshotURL, err := p.renderer.RenderDashboardPreview(ctx, namespace, repoName, originalPath, base) - if err != nil { - return resourcePreview{}, fmt.Errorf("render dashboard preview: %w", err) - } - preview.OriginalScreenshotURL = screenshotURL - logger.Info("original dashboard screenshot generated", "screenshotURL", screenshotURL) - } - - return preview, nil -} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/preview_renderer_mock.go b/pkg/registry/apis/provisioning/jobs/pullrequest/preview_renderer_mock.go deleted file mode 100644 index 529430cc72c..00000000000 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/preview_renderer_mock.go +++ /dev/null @@ -1,142 +0,0 @@ -// Code generated by mockery v2.52.4. DO NOT EDIT. - -package pullrequest - -import ( - context "context" - - mock "github.com/stretchr/testify/mock" -) - -// MockPreviewRenderer is an autogenerated mock type for the PreviewRenderer type -type MockPreviewRenderer struct { - mock.Mock -} - -type MockPreviewRenderer_Expecter struct { - mock *mock.Mock -} - -func (_m *MockPreviewRenderer) EXPECT() *MockPreviewRenderer_Expecter { - return &MockPreviewRenderer_Expecter{mock: &_m.Mock} -} - -// IsAvailable provides a mock function with given fields: ctx -func (_m *MockPreviewRenderer) IsAvailable(ctx context.Context) bool { - ret := _m.Called(ctx) - - if len(ret) == 0 { - panic("no return value specified for IsAvailable") - } - - var r0 bool - if rf, ok := ret.Get(0).(func(context.Context) bool); ok { - r0 = rf(ctx) - } else { - r0 = ret.Get(0).(bool) - } - - return r0 -} - -// MockPreviewRenderer_IsAvailable_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'IsAvailable' -type MockPreviewRenderer_IsAvailable_Call struct { - *mock.Call -} - -// IsAvailable is a helper method to define mock.On call -// - ctx context.Context -func (_e *MockPreviewRenderer_Expecter) IsAvailable(ctx interface{}) *MockPreviewRenderer_IsAvailable_Call { - return &MockPreviewRenderer_IsAvailable_Call{Call: _e.mock.On("IsAvailable", ctx)} -} - -func (_c *MockPreviewRenderer_IsAvailable_Call) Run(run func(ctx context.Context)) *MockPreviewRenderer_IsAvailable_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context)) - }) - return _c -} - -func (_c *MockPreviewRenderer_IsAvailable_Call) Return(_a0 bool) *MockPreviewRenderer_IsAvailable_Call { - _c.Call.Return(_a0) - return _c -} - -func (_c *MockPreviewRenderer_IsAvailable_Call) RunAndReturn(run func(context.Context) bool) *MockPreviewRenderer_IsAvailable_Call { - _c.Call.Return(run) - return _c -} - -// RenderDashboardPreview provides a mock function with given fields: ctx, namespace, repoName, path, ref -func (_m *MockPreviewRenderer) RenderDashboardPreview(ctx context.Context, namespace string, repoName string, path string, ref string) (string, error) { - ret := _m.Called(ctx, namespace, repoName, path, ref) - - if len(ret) == 0 { - panic("no return value specified for RenderDashboardPreview") - } - - var r0 string - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) (string, error)); ok { - return rf(ctx, namespace, repoName, path, ref) - } - if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) string); ok { - r0 = rf(ctx, namespace, repoName, path, ref) - } else { - r0 = ret.Get(0).(string) - } - - if rf, ok := ret.Get(1).(func(context.Context, string, string, string, string) error); ok { - r1 = rf(ctx, namespace, repoName, path, ref) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// MockPreviewRenderer_RenderDashboardPreview_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RenderDashboardPreview' -type MockPreviewRenderer_RenderDashboardPreview_Call struct { - *mock.Call -} - -// RenderDashboardPreview is a helper method to define mock.On call -// - ctx context.Context -// - namespace string -// - repoName string -// - path string -// - ref string -func (_e *MockPreviewRenderer_Expecter) RenderDashboardPreview(ctx interface{}, namespace interface{}, repoName interface{}, path interface{}, ref interface{}) *MockPreviewRenderer_RenderDashboardPreview_Call { - return &MockPreviewRenderer_RenderDashboardPreview_Call{Call: _e.mock.On("RenderDashboardPreview", ctx, namespace, repoName, path, ref)} -} - -func (_c *MockPreviewRenderer_RenderDashboardPreview_Call) Run(run func(ctx context.Context, namespace string, repoName string, path string, ref string)) *MockPreviewRenderer_RenderDashboardPreview_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) - }) - return _c -} - -func (_c *MockPreviewRenderer_RenderDashboardPreview_Call) Return(_a0 string, _a1 error) *MockPreviewRenderer_RenderDashboardPreview_Call { - _c.Call.Return(_a0, _a1) - return _c -} - -func (_c *MockPreviewRenderer_RenderDashboardPreview_Call) RunAndReturn(run func(context.Context, string, string, string, string) (string, error)) *MockPreviewRenderer_RenderDashboardPreview_Call { - _c.Call.Return(run) - return _c -} - -// NewMockPreviewRenderer creates a new instance of MockPreviewRenderer. 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 NewMockPreviewRenderer(t interface { - mock.TestingT - Cleanup(func()) -}) *MockPreviewRenderer { - mock := &MockPreviewRenderer{} - mock.Mock.Test(t) - - t.Cleanup(func() { mock.AssertExpectations(t) }) - - return mock -} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/previewer_mock.go b/pkg/registry/apis/provisioning/jobs/pullrequest/previewer_mock.go deleted file mode 100644 index e8e8aedcdab..00000000000 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/previewer_mock.go +++ /dev/null @@ -1,156 +0,0 @@ -// 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" -) - -// MockPreviewer is an autogenerated mock type for the Previewer type -type MockPreviewer struct { - mock.Mock -} - -type MockPreviewer_Expecter struct { - mock *mock.Mock -} - -func (_m *MockPreviewer) EXPECT() *MockPreviewer_Expecter { - return &MockPreviewer_Expecter{mock: &_m.Mock} -} - -// GenerateComment provides a mock function with given fields: preview -func (_m *MockPreviewer) GenerateComment(preview resourcePreview) (string, error) { - ret := _m.Called(preview) - - if len(ret) == 0 { - panic("no return value specified for GenerateComment") - } - - var r0 string - var r1 error - if rf, ok := ret.Get(0).(func(resourcePreview) (string, error)); ok { - return rf(preview) - } - if rf, ok := ret.Get(0).(func(resourcePreview) string); ok { - r0 = rf(preview) - } else { - r0 = ret.Get(0).(string) - } - - if rf, ok := ret.Get(1).(func(resourcePreview) error); ok { - r1 = rf(preview) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// MockPreviewer_GenerateComment_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GenerateComment' -type MockPreviewer_GenerateComment_Call struct { - *mock.Call -} - -// GenerateComment is a helper method to define mock.On call -// - preview resourcePreview -func (_e *MockPreviewer_Expecter) GenerateComment(preview interface{}) *MockPreviewer_GenerateComment_Call { - return &MockPreviewer_GenerateComment_Call{Call: _e.mock.On("GenerateComment", preview)} -} - -func (_c *MockPreviewer_GenerateComment_Call) Run(run func(preview resourcePreview)) *MockPreviewer_GenerateComment_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(resourcePreview)) - }) - return _c -} - -func (_c *MockPreviewer_GenerateComment_Call) Return(_a0 string, _a1 error) *MockPreviewer_GenerateComment_Call { - _c.Call.Return(_a0, _a1) - return _c -} - -func (_c *MockPreviewer_GenerateComment_Call) RunAndReturn(run func(resourcePreview) (string, error)) *MockPreviewer_GenerateComment_Call { - _c.Call.Return(run) - return _c -} - -// Preview provides a mock function with given fields: ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview -func (_m *MockPreviewer) Preview(ctx context.Context, f repository.VersionedFileChange, namespace string, repoName string, base string, ref string, pullRequestURL string, generatePreview bool) (resourcePreview, error) { - ret := _m.Called(ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview) - - if len(ret) == 0 { - panic("no return value specified for Preview") - } - - var r0 resourcePreview - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, repository.VersionedFileChange, string, string, string, string, string, bool) (resourcePreview, error)); ok { - return rf(ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview) - } - if rf, ok := ret.Get(0).(func(context.Context, repository.VersionedFileChange, string, string, string, string, string, bool) resourcePreview); ok { - r0 = rf(ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview) - } else { - r0 = ret.Get(0).(resourcePreview) - } - - if rf, ok := ret.Get(1).(func(context.Context, repository.VersionedFileChange, string, string, string, string, string, bool) error); ok { - r1 = rf(ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - -// MockPreviewer_Preview_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Preview' -type MockPreviewer_Preview_Call struct { - *mock.Call -} - -// Preview is a helper method to define mock.On call -// - ctx context.Context -// - f repository.VersionedFileChange -// - namespace string -// - repoName string -// - base string -// - ref string -// - pullRequestURL string -// - generatePreview bool -func (_e *MockPreviewer_Expecter) Preview(ctx interface{}, f interface{}, namespace interface{}, repoName interface{}, base interface{}, ref interface{}, pullRequestURL interface{}, generatePreview interface{}) *MockPreviewer_Preview_Call { - return &MockPreviewer_Preview_Call{Call: _e.mock.On("Preview", ctx, f, namespace, repoName, base, ref, pullRequestURL, generatePreview)} -} - -func (_c *MockPreviewer_Preview_Call) Run(run func(ctx context.Context, f repository.VersionedFileChange, namespace string, repoName string, base string, ref string, pullRequestURL string, generatePreview bool)) *MockPreviewer_Preview_Call { - _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(repository.VersionedFileChange), args[2].(string), args[3].(string), args[4].(string), args[5].(string), args[6].(string), args[7].(bool)) - }) - return _c -} - -func (_c *MockPreviewer_Preview_Call) Return(_a0 resourcePreview, _a1 error) *MockPreviewer_Preview_Call { - _c.Call.Return(_a0, _a1) - return _c -} - -func (_c *MockPreviewer_Preview_Call) RunAndReturn(run func(context.Context, repository.VersionedFileChange, string, string, string, string, string, bool) (resourcePreview, error)) *MockPreviewer_Preview_Call { - _c.Call.Return(run) - return _c -} - -// NewMockPreviewer creates a new instance of MockPreviewer. 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 NewMockPreviewer(t interface { - mock.TestingT - Cleanup(func()) -}) *MockPreviewer { - mock := &MockPreviewer{} - mock.Mock.Test(t) - - t.Cleanup(func() { mock.AssertExpectations(t) }) - - return mock -} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/render.go b/pkg/registry/apis/provisioning/jobs/pullrequest/render.go index cf4a6b43275..644a8660973 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/render.go +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/render.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "mime" + "net/url" "os" "path/filepath" "strings" @@ -16,34 +17,45 @@ import ( "github.com/grafana/grafana/pkg/storage/unified/resource" ) -type screenshotRenderer struct { - render rendering.Service - blobstore resource.BlobStoreClient - urlProvider func(namespace string) string - isPublic bool +// ScreenshotRenderer is an interface for rendering a preview of a file +// +//go:generate mockery --name ScreenshotRenderer --structname MockScreenshotRenderer --inpackage --filename render_mock.go --with-expecter +type ScreenshotRenderer interface { + IsAvailable(ctx context.Context) bool + RenderScreenshot(ctx context.Context, repo provisioning.ResourceRepositoryInfo, path string, values url.Values) (string, error) } -func NewScreenshotRenderer(render rendering.Service, blobstore resource.BlobStoreClient, isPublic bool, urlProvider func(namespace string) string) *screenshotRenderer { +type screenshotRenderer struct { + render rendering.Service + blobstore resource.BlobStoreClient +} + +func NewScreenshotRenderer(render rendering.Service, blobstore resource.BlobStoreClient) ScreenshotRenderer { return &screenshotRenderer{ - render: render, - blobstore: blobstore, - urlProvider: urlProvider, - isPublic: isPublic, + render: render, + blobstore: blobstore, } } func (r *screenshotRenderer) IsAvailable(ctx context.Context) bool { - return r.render != nil && r.render.IsAvailable(ctx) && r.blobstore != nil && r.isPublic + return r.render != nil && r.render.IsAvailable(ctx) && r.blobstore != nil } -func (r *screenshotRenderer) RenderDashboardPreview(ctx context.Context, namespace, repoName, path, ref string) (string, error) { - url := fmt.Sprintf("admin/provisioning/%s/dashboard/preview/%s?kiosk&ref=%s", repoName, path, ref) - - // TODO: why were we using a different context? - // renderContext := identity.WithRequester(context.Background(), r.id) +func (r *screenshotRenderer) RenderScreenshot(ctx context.Context, repo provisioning.ResourceRepositoryInfo, path string, values url.Values) (string, error) { + if strings.Contains(path, "://") { + return "", fmt.Errorf("path should be relative to the system root url") + } + if strings.HasPrefix(path, "/") { + return "", fmt.Errorf("path should not start with slash") + } + if len(values) > 0 { + path = path + "?" + values.Encode() + "&kiosk" + } else { + path = path + "?kiosk" + } result, err := r.render.Render(ctx, rendering.RenderPNG, rendering.Opts{ CommonOpts: rendering.CommonOpts{ - Path: url, + Path: path, AuthOpts: rendering.AuthOpts{ OrgID: 1, // TODO!!!, use the worker identity UserID: 1, @@ -69,10 +81,10 @@ func (r *screenshotRenderer) RenderDashboardPreview(ctx context.Context, namespa rsp, err := r.blobstore.PutBlob(ctx, &resource.PutBlobRequest{ Resource: &resource.ResourceKey{ - Namespace: namespace, + Namespace: repo.Namespace, Group: provisioning.GROUP, Resource: provisioning.RepositoryResourceInfo.GroupResource().Resource, - Name: repoName, + Name: repo.Name, }, Method: resource.PutBlobRequest_GRPC, ContentType: mime.TypeByExtension(ext), // image/png @@ -84,10 +96,6 @@ func (r *screenshotRenderer) RenderDashboardPreview(ctx context.Context, namespa if rsp.Url != "" { return rsp.Url, nil } - base := r.urlProvider(namespace) - if !strings.HasSuffix(base, "/") { - base += "/" - } - return fmt.Sprintf("%sapis/%s/namespaces/%s/repositories/%s/render/%s", - base, provisioning.APIVERSION, namespace, repoName, rsp.Uid), nil + return fmt.Sprintf("apis/%s/namespaces/%s/repositories/%s/render/%s", + provisioning.APIVERSION, repo.Namespace, repo.Name, rsp.Uid), nil } diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go b/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go new file mode 100644 index 00000000000..0bb99e8a740 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/render_mock.go @@ -0,0 +1,144 @@ +// Code generated by mockery v2.53.3. DO NOT EDIT. + +package pullrequest + +import ( + context "context" + url "net/url" + + mock "github.com/stretchr/testify/mock" + + v0alpha1 "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" +) + +// MockScreenshotRenderer is an autogenerated mock type for the ScreenshotRenderer type +type MockScreenshotRenderer struct { + mock.Mock +} + +type MockScreenshotRenderer_Expecter struct { + mock *mock.Mock +} + +func (_m *MockScreenshotRenderer) EXPECT() *MockScreenshotRenderer_Expecter { + return &MockScreenshotRenderer_Expecter{mock: &_m.Mock} +} + +// IsAvailable provides a mock function with given fields: ctx +func (_m *MockScreenshotRenderer) IsAvailable(ctx context.Context) bool { + ret := _m.Called(ctx) + + if len(ret) == 0 { + panic("no return value specified for IsAvailable") + } + + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context) bool); ok { + r0 = rf(ctx) + } else { + r0 = ret.Get(0).(bool) + } + + return r0 +} + +// MockScreenshotRenderer_IsAvailable_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'IsAvailable' +type MockScreenshotRenderer_IsAvailable_Call struct { + *mock.Call +} + +// IsAvailable is a helper method to define mock.On call +// - ctx context.Context +func (_e *MockScreenshotRenderer_Expecter) IsAvailable(ctx interface{}) *MockScreenshotRenderer_IsAvailable_Call { + return &MockScreenshotRenderer_IsAvailable_Call{Call: _e.mock.On("IsAvailable", ctx)} +} + +func (_c *MockScreenshotRenderer_IsAvailable_Call) Run(run func(ctx context.Context)) *MockScreenshotRenderer_IsAvailable_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context)) + }) + return _c +} + +func (_c *MockScreenshotRenderer_IsAvailable_Call) Return(_a0 bool) *MockScreenshotRenderer_IsAvailable_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockScreenshotRenderer_IsAvailable_Call) RunAndReturn(run func(context.Context) bool) *MockScreenshotRenderer_IsAvailable_Call { + _c.Call.Return(run) + return _c +} + +// RenderScreenshot provides a mock function with given fields: ctx, repo, path, values +func (_m *MockScreenshotRenderer) RenderScreenshot(ctx context.Context, repo v0alpha1.ResourceRepositoryInfo, path string, values url.Values) (string, error) { + ret := _m.Called(ctx, repo, path, values) + + if len(ret) == 0 { + panic("no return value specified for RenderScreenshot") + } + + var r0 string + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, v0alpha1.ResourceRepositoryInfo, string, url.Values) (string, error)); ok { + return rf(ctx, repo, path, values) + } + if rf, ok := ret.Get(0).(func(context.Context, v0alpha1.ResourceRepositoryInfo, string, url.Values) string); ok { + r0 = rf(ctx, repo, path, values) + } else { + r0 = ret.Get(0).(string) + } + + if rf, ok := ret.Get(1).(func(context.Context, v0alpha1.ResourceRepositoryInfo, string, url.Values) error); ok { + r1 = rf(ctx, repo, path, values) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockScreenshotRenderer_RenderScreenshot_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RenderScreenshot' +type MockScreenshotRenderer_RenderScreenshot_Call struct { + *mock.Call +} + +// RenderScreenshot is a helper method to define mock.On call +// - ctx context.Context +// - repo v0alpha1.ResourceRepositoryInfo +// - path string +// - values url.Values +func (_e *MockScreenshotRenderer_Expecter) RenderScreenshot(ctx interface{}, repo interface{}, path interface{}, values interface{}) *MockScreenshotRenderer_RenderScreenshot_Call { + return &MockScreenshotRenderer_RenderScreenshot_Call{Call: _e.mock.On("RenderScreenshot", ctx, repo, path, values)} +} + +func (_c *MockScreenshotRenderer_RenderScreenshot_Call) Run(run func(ctx context.Context, repo v0alpha1.ResourceRepositoryInfo, path string, values url.Values)) *MockScreenshotRenderer_RenderScreenshot_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(v0alpha1.ResourceRepositoryInfo), args[2].(string), args[3].(url.Values)) + }) + return _c +} + +func (_c *MockScreenshotRenderer_RenderScreenshot_Call) Return(_a0 string, _a1 error) *MockScreenshotRenderer_RenderScreenshot_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockScreenshotRenderer_RenderScreenshot_Call) RunAndReturn(run func(context.Context, v0alpha1.ResourceRepositoryInfo, string, url.Values) (string, error)) *MockScreenshotRenderer_RenderScreenshot_Call { + _c.Call.Return(run) + return _c +} + +// NewMockScreenshotRenderer creates a new instance of MockScreenshotRenderer. 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 NewMockScreenshotRenderer(t interface { + mock.TestingT + Cleanup(func()) +}) *MockScreenshotRenderer { + mock := &MockScreenshotRenderer{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/multiple-files.md b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/multiple-files.md new file mode 100755 index 00000000000..b7ce21ef16b --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/multiple-files.md @@ -0,0 +1,11 @@ +Hey there! 🎉 +Grafana spotted some changes. + +| Action | Kind | Resource | Preview | +|--------|------|----------|---------| +| create | Dashboard | Dash A | [preview](http://grafana/admin/preview) | +| update | Dashboard | [Dash B](http://grafana/d/bbb) | [preview](http://grafana/admin/preview) | +| create | Playlist | My Playlist | | + + +and 5 more files. \ No newline at end of file diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/new-dashboard.md b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/new-dashboard.md new file mode 100755 index 00000000000..deedc18c04d --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/new-dashboard.md @@ -0,0 +1,8 @@ +Hey there! 🎉 +Grafana spotted some changes to your dashboard. +### Preview of file.json +![Preview](https://cdn2.thecatapi.com/images/99c.jpg) + + + +See the [preview](http://grafana/admin/preview) of file.json. \ No newline at end of file diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard-missing-renderer.md b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard-missing-renderer.md new file mode 100755 index 00000000000..79d31c86cee --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard-missing-renderer.md @@ -0,0 +1,7 @@ +Hey there! 🎉 +Grafana spotted some changes to your dashboard. + + +See the [original](http://grafana/d/uid) and [preview](http://grafana/admin/preview) of file.json. + +NOTE: The image renderer is not configured \ No newline at end of file diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard.md b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard.md new file mode 100755 index 00000000000..2597017e685 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/testdata/update-dashboard.md @@ -0,0 +1,9 @@ +Hey there! 🎉 +Grafana spotted some changes to your dashboard. +### Side by Side Comparison of file.json +| Before | After | +|----------|---------| +| ![Before](https://cdn2.thecatapi.com/images/99c.jpg) | ![Preview](https://cdn2.thecatapi.com/images/99c.jpg) | + + +See the [original](http://grafana/d/uid) and [preview](http://grafana/admin/preview) of file.json. \ No newline at end of file diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go b/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go index 485234e2598..0fea17d09b6 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go @@ -24,17 +24,22 @@ type PullRequestRepo interface { } type PullRequestWorker struct { - parsers resources.ParserFactory - previewer Previewer + parsers resources.ParserFactory + renderer ScreenshotRenderer + urlProvider func(namespace string) string + commenter *commentBuilder } func NewPullRequestWorker( parsers resources.ParserFactory, - previewer Previewer, + renderer ScreenshotRenderer, + urlProvider func(namespace string) string, ) *PullRequestWorker { return &PullRequestWorker{ - parsers: parsers, - previewer: previewer, + parsers: parsers, + renderer: renderer, + urlProvider: urlProvider, + commenter: newCommentBuilder(), } } @@ -42,7 +47,6 @@ func (c *PullRequestWorker) IsSupported(ctx context.Context, job provisioning.Jo return job.Spec.Action == provisioning.JobActionPullRequest } -//nolint:gocyclo func (c *PullRequestWorker) Process(ctx context.Context, repo repository.Repository, job provisioning.Job, @@ -54,6 +58,14 @@ func (c *PullRequestWorker) Process(ctx context.Context, return apierrors.NewBadRequest("missing spec.pr") } + if options.Ref == "" { + return apierrors.NewBadRequest("missing spec.ref") + } + + if cfg.GitHub == nil { + return apierrors.NewBadRequest("expecting github configuration") + } + prRepo, ok := repo.(PullRequestRepo) if !ok { return fmt.Errorf("repository is not a github repository") @@ -64,82 +76,71 @@ func (c *PullRequestWorker) Process(ctx context.Context, return errors.New("pull request job submitted targeting repository that is not a Reader") } - parser, err := c.parsers.GetParser(ctx, reader) - if err != nil { - return fmt.Errorf("failed to get parser for %s: %w", repo.Config().Name, err) - } - logger := logging.FromContext(ctx).With("pr", options.PR) logger.Info("process pull request") defer logger.Info("pull request processed") progress.SetMessage(ctx, "listing pull request files") base := cfg.GitHub.Branch - ref := options.Hash - files, err := prRepo.CompareFiles(ctx, base, ref) + files, err := prRepo.CompareFiles(ctx, base, options.Ref) if err != nil { return fmt.Errorf("failed to list pull request files: %s", err.Error()) } - progress.SetMessage(ctx, "clearing pull request comments") - if err := prRepo.ClearAllPullRequestFileComments(ctx, options.PR); err != nil { - return fmt.Errorf("failed to clear pull request comments: %+v", err) - } + files = onlySupportedFiles(files) if len(files) == 0 { progress.SetFinalMessage(ctx, "no files to process") return nil } - if len(files) > 1 { - progress.SetFinalMessage(ctx, "too many files to preview") - return nil - } - - f := files[0] - progress.SetMessage(ctx, "processing file preview") - - if err := resources.IsPathSupported(f.Path); err != nil { - progress.SetFinalMessage(ctx, "file path is not supported") - return nil - } - - fileInfo, err := prRepo.Read(ctx, f.Path, ref) + parser, err := c.parsers.GetParser(ctx, reader) if err != nil { - return fmt.Errorf("read file: %w", err) + return fmt.Errorf("failed to get parser for %s: %w", repo.Config().Name, err) } - _, err = parser.Parse(ctx, fileInfo) + 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 { - if errors.Is(err, resources.ErrUnableToReadResourceBytes) { - progress.SetFinalMessage(ctx, "file changes is not valid resource") - return nil - } else { - return fmt.Errorf("parse resource: %w", err) - } + return fmt.Errorf("unable to calculate changes: %w", err) } - // Preview should be the branch name if provided, otherwise use the commit hash - previewRef := options.Ref - if previewRef == "" { - previewRef = ref - } - - preview, err := c.previewer.Preview(ctx, f, job.Namespace, repo.Config().Name, cfg.GitHub.Branch, previewRef, options.URL, cfg.GitHub.GenerateDashboardPreviews) - if err != nil { - return fmt.Errorf("generate preview: %w", err) - } - - progress.SetMessage(ctx, "generating previews comment") - comment, err := c.previewer.GenerateComment(preview) - if err != nil { - return fmt.Errorf("generate comment: %w", err) - } - - if err := prRepo.CommentPullRequest(ctx, options.PR, comment); err != nil { + if err := c.commenter.Comment(ctx, prRepo, options.PR, changeInfo); err != nil { return fmt.Errorf("comment pull request: %w", err) } logger.Info("preview comment added") - return nil } + +// Remove files we should not try to process +func onlySupportedFiles(files []repository.VersionedFileChange) (ret []repository.VersionedFileChange) { + for _, file := range files { + if file.Action == repository.FileActionIgnored { + continue + } + + if err := resources.IsPathSupported(file.Path); err == nil { + ret = append(ret, file) + continue + } + if file.PreviousPath != "" { + if err := resources.IsPathSupported(file.PreviousPath); err != nil { + ret = append(ret, file) + continue + } + } + } + return +} diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index 26e6e617a67..9807476cbb4 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -558,9 +558,8 @@ func (b *APIBuilder) GetPostStartHooks() (map[string]genericapiserver.PostStartH ) // Pull request worker - renderer := pullrequest.NewScreenshotRenderer(b.render, b.unified, b.isPublic, b.urlProvider) - previewer := pullrequest.NewPreviewer(renderer, b.urlProvider) - pullRequestWorker := pullrequest.NewPullRequestWorker(b.parsers, previewer) + renderer := pullrequest.NewScreenshotRenderer(b.render, b.unified) + pullRequestWorker := pullrequest.NewPullRequestWorker(b.parsers, renderer, b.urlProvider) driver := jobs.NewJobDriver(time.Second*28, time.Second*30, time.Second*30, b.jobs, b, b.jobHistory, exportWorker, syncWorker, migrationWorker, pullRequestWorker) diff --git a/pkg/registry/apis/provisioning/repository/local.go b/pkg/registry/apis/provisioning/repository/local.go index c68da67d80f..d28663e4680 100644 --- a/pkg/registry/apis/provisioning/repository/local.go +++ b/pkg/registry/apis/provisioning/repository/local.go @@ -19,6 +19,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" @@ -312,18 +313,18 @@ func (r *localRepository) calculateFileHash(path string) (string, int64, error) return hex.EncodeToString(hasher.Sum(nil)), size, nil } -func (r *localRepository) Create(ctx context.Context, fpath string, ref string, data []byte, comment string) error { +func (r *localRepository) Create(ctx context.Context, filepath string, ref string, data []byte, comment string) error { if err := r.validateRequest(ref); err != nil { return err } - fpath = safepath.Join(r.path, fpath) + fpath := safepath.Join(r.path, filepath) _, err := os.Stat(fpath) if !errors.Is(err, os.ErrNotExist) { if err != nil { return apierrors.NewInternalError(fmt.Errorf("failed to check if file exists: %w", err)) } - return apierrors.NewAlreadyExists(provisioning.RepositoryResourceInfo.GroupResource(), fpath) + return apierrors.NewAlreadyExists(schema.GroupResource{}, filepath) } if safepath.IsDir(fpath) { @@ -356,7 +357,7 @@ func (r *localRepository) Update(ctx context.Context, path string, ref string, d } if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("file does not exist") + return ErrFileNotFound } return os.WriteFile(path, data, 0600) } diff --git a/pkg/registry/apis/provisioning/repository/local_test.go b/pkg/registry/apis/provisioning/repository/local_test.go index c2825ff4f84..4fc6c6545a1 100644 --- a/pkg/registry/apis/provisioning/repository/local_test.go +++ b/pkg/registry/apis/provisioning/repository/local_test.go @@ -2,11 +2,11 @@ package repository import ( "context" - "io/fs" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" ) @@ -69,7 +69,7 @@ func TestLocalResolver(t *testing.T) { // read unknown file _, err = r.Read(context.Background(), "testdata/missing", "") - require.ErrorIs(t, err, fs.ErrNotExist) + require.True(t, apierrors.IsNotFound(err)) // 404 error _, err = r.Read(context.Background(), "testdata/webhook-push-nested.json/", "") require.Error(t, err) // not a directory diff --git a/pkg/registry/apis/provisioning/repository/repository.go b/pkg/registry/apis/provisioning/repository/repository.go index db5f6a45b2a..0d15d28a9d8 100644 --- a/pkg/registry/apis/provisioning/repository/repository.go +++ b/pkg/registry/apis/provisioning/repository/repository.go @@ -5,10 +5,10 @@ import ( "errors" "fmt" "io" - "io/fs" "net/http" "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" @@ -29,7 +29,12 @@ type Repository interface { } // ErrFileNotFound indicates that a path could not be found in the repository. -var ErrFileNotFound error = fs.ErrNotExist +var ErrFileNotFound error = &apierrors.StatusError{ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Code: http.StatusNotFound, + Reason: metav1.StatusReasonNotFound, + Message: "file not found", +}} type FileInfo struct { // Path to the file on disk. diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index ea819149165..b8313aa99d7 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -170,60 +170,16 @@ func (r *DualReadWriter) CreateFolder(ctx context.Context, path string, ref stri // CreateResource creates a new resource in the repository func (r *DualReadWriter) CreateResource(ctx context.Context, path string, ref string, message string, data []byte) (*ParsedResource, error) { - if err := repository.IsWriteAllowed(r.repo.Config(), ref); err != nil { - return nil, err - } - - info := &repository.FileInfo{ - Data: data, - Path: path, - Ref: ref, - } - - parsed, err := r.parser.Parse(ctx, info) - if err != nil { - return nil, fmt.Errorf("parse file: %w", err) - } - - if err = r.authorize(ctx, parsed, utils.VerbCreate); err != nil { - return nil, err - } - - data, err = parsed.ToSaveBytes() - if err != nil { - return nil, err - } - - if err := r.repo.Create(ctx, path, ref, data, message); err != nil { - return nil, fmt.Errorf("create resource in repository: %w", err) - } - - // Directly update the grafana database - // Behaves the same running sync after writing - // FIXME: to make sure if behaves in the same way as in sync, we should - // we should refactor the code to use the same function. - if ref == "" { - if _, err := r.folders.EnsureFolderPathExist(ctx, path); err != nil { - return nil, fmt.Errorf("ensure folder path exists: %w", err) - } - - if err := parsed.Run(ctx); err != nil { - return nil, fmt.Errorf("run resource: %w", err) - } - } else { - if err := parsed.DryRun(ctx); err != nil { - logger := logging.FromContext(ctx).With("path", path, "name", parsed.Obj.GetName(), "ref", ref) - logger.Warn("failed to dry run resource on create", "error", err) - // Do not fail here as it's purely informational - parsed.Errors = append(parsed.Errors, err.Error()) - } - } - - return parsed, nil + return r.createOrUpdate(ctx, true, path, ref, message, data) } // UpdateResource updates a resource in the repository func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref string, message string, data []byte) (*ParsedResource, error) { + return r.createOrUpdate(ctx, false, path, ref, message, data) +} + +// Create or updates a resource in the repository +func (r *DualReadWriter) createOrUpdate(ctx context.Context, create bool, path string, ref string, message string, data []byte) (*ParsedResource, error) { if err := repository.IsWriteAllowed(r.repo.Config(), ref); err != nil { return nil, err } @@ -234,47 +190,68 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st Ref: ref, } - // TODO: improve parser to parse out of reader parsed, err := r.parser.Parse(ctx, info) if err != nil { - return nil, fmt.Errorf("parse file: %w", err) + return nil, err } - if err = r.authorize(ctx, parsed, utils.VerbUpdate); err != nil { + // Verify that we can create (or update) the referenced resource + verb := utils.VerbUpdate + if create { + verb = utils.VerbCreate + } + if err = r.authorize(ctx, parsed, verb); err != nil { return nil, err } + // Always use the provisioning identity when writing + ctx, _, err = identity.WithProvisioningIdentity(ctx, parsed.Obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("unable to use provisioning identity %w", err) + } + + // Make sure the value is valid + if err := parsed.DryRun(ctx); err != nil { + logger := logging.FromContext(ctx).With("path", path, "name", parsed.Obj.GetName(), "ref", ref) + logger.Warn("failed to dry run resource on create", "error", err) + + // TODO: return this as a 400 rather than 500 + return nil, fmt.Errorf("error running dryRun %w", err) + } + + if len(parsed.Errors) > 0 { + // TODO: return this as a 400 rather than 500 + return nil, fmt.Errorf("errors while parsing file [%v]", parsed.Errors) + } + data, err = parsed.ToSaveBytes() if err != nil { return nil, err } - if err = r.repo.Update(ctx, path, ref, data, message); err != nil { - return nil, fmt.Errorf("update resource in repository: %w", err) + // Create or update + if create { + err = r.repo.Create(ctx, path, ref, data, message) + } else { + err = r.repo.Update(ctx, path, ref, data, message) + } + if err != nil { + return nil, err // raw error is useful } // Directly update the grafana database // Behaves the same running sync after writing // FIXME: to make sure if behaves in the same way as in sync, we should // we should refactor the code to use the same function. - if ref == "" { + if ref == "" && parsed.Client != nil { if _, err := r.folders.EnsureFolderPathExist(ctx, path); err != nil { return nil, fmt.Errorf("ensure folder path exists: %w", err) } - if err := parsed.Run(ctx); err != nil { - return nil, fmt.Errorf("run resource: %w", err) - } - } else { - if err := parsed.DryRun(ctx); err != nil { - // Do not fail here as it's purely informational - logger := logging.FromContext(ctx).With("path", path, "name", parsed.Obj.GetName(), "ref", ref) - logger.Warn("failed to dry run resource on update", "error", err) - parsed.Errors = append(parsed.Errors, err.Error()) - } + err = parsed.Run(ctx) } - return parsed, nil + return parsed, err } func (r *DualReadWriter) authorize(ctx context.Context, parsed *ParsedResource, verb string) error { diff --git a/pkg/registry/apis/provisioning/resources/parser.go b/pkg/registry/apis/provisioning/resources/parser.go index 505ed7ba996..cdca95ea405 100644 --- a/pkg/registry/apis/provisioning/resources/parser.go +++ b/pkg/registry/apis/provisioning/resources/parser.go @@ -212,6 +212,10 @@ func (r *parser) Parse(ctx context.Context, info *repository.FileInfo) (parsed * } func (f *ParsedResource) DryRun(ctx context.Context) error { + if f.DryRunResponse != nil { + return nil // this already ran (and helpful for testing) + } + // FIXME: remove this check once we have better unit tests if f.Client == nil { return fmt.Errorf("no client configured") @@ -252,9 +256,13 @@ func (f *ParsedResource) Run(ctx context.Context) error { return err } - // FIXME: shouldn't we check for the specific error? + // We may have already called DryRun that also calls get + if f.DryRunResponse != nil && f.Action != "" { + // FIXME: shouldn't we check for the specific error? + f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + } + // Run update or create - f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) if f.Existing == nil { f.Action = provisioning.ResourceActionCreate f.Upsert, err = f.Client.Create(ctx, f.Obj, metav1.CreateOptions{}) diff --git a/pkg/tests/apis/provisioning/provisioning_test.go b/pkg/tests/apis/provisioning/provisioning_test.go index 2ffaba5ea53..6b531807381 100644 --- a/pkg/tests/apis/provisioning/provisioning_test.go +++ b/pkg/tests/apis/provisioning/provisioning_test.go @@ -270,7 +270,21 @@ func TestIntegrationProvisioning_RunLocalRepository(t *testing.T) { // Write a file -- this will create it *both* in the local file system, and in grafana t.Run("write all panels", func(t *testing.T) { code := 0 - result := helper.AdminREST.Post(). + + // Check that we can not (yet) UPDATE the target path + result := helper.AdminREST.Put(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", targetPath). + Body(helper.LoadFile("testdata/all-panels.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx).StatusCode(&code) + require.Equal(t, http.StatusNotFound, code) + require.True(t, apierrors.IsNotFound(result.Error())) + + // Now try again with POST + result = helper.AdminREST.Post(). Namespace("default"). Resource("repositories"). Name(repo). diff --git a/public/app/features/provisioning/GettingStarted/GettingStarted.tsx b/public/app/features/provisioning/GettingStarted/GettingStarted.tsx index ab4de428af7..d74719dcb5d 100644 --- a/public/app/features/provisioning/GettingStarted/GettingStarted.tsx +++ b/public/app/features/provisioning/GettingStarted/GettingStarted.tsx @@ -17,7 +17,6 @@ const featureIni = `# In your custom.ini file [feature_toggles] provisioning = true -unifiedStorageSearch = true kubernetesClientDashboardsFolders = true kubernetesDashboards = true ; use k8s from browser