From d5d1851bc1cc7dd928878c3ce20931505bc8011b Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Sat, 4 Oct 2025 15:17:42 -0600 Subject: [PATCH] Provisioning: Cleanup folders properly with webhooks (#112031) --- .../testdata/webhook-push-keep_file_only.json | 105 ++++++++++++++ .../webhook-push-keep_file_with_others.json | 109 +++++++++++++++ .../webhook-push-multiple_keep_files.json | 109 +++++++++++++++ .../pkg/repository/github/webhook.go | 29 +++- .../pkg/repository/github/webhook_test.go | 30 ++++ .../provisioning/jobs/sync/incremental.go | 68 ++++++++- .../jobs/sync/incremental_test.go | 129 +++++++++++++++++- .../apis/provisioning/resources/folders.go | 4 + .../apis/provisioning/resources/repository.go | 5 +- .../resources/repository_resources_mock.go | 109 +++++++++++---- .../apis/provisioning/resources/resources.go | 43 ++++-- 11 files changed, 696 insertions(+), 44 deletions(-) create mode 100644 apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_only.json create mode 100644 apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_with_others.json create mode 100644 apps/provisioning/pkg/repository/github/testdata/webhook-push-multiple_keep_files.json diff --git a/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_only.json b/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_only.json new file mode 100644 index 00000000000..f4ab95c170e --- /dev/null +++ b/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_only.json @@ -0,0 +1,105 @@ +{ + "ref": "refs/heads/main", + "before": "72096e3adc646c5a5b8a91744f962b12bac06045", + "after": "1234567890abcdef1234567890abcdef12345678", + "repository": { + "id": 888020043, + "node_id": "R_kgDONO4cSw", + "name": "git-ui-sync-demo", + "full_name": "grafana/git-ui-sync-demo", + "private": true, + "owner": { + "name": "grafana", + "email": "hello@grafana.com", + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/grafana", + "html_url": "https://github.com/grafana", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/grafana/git-ui-sync-demo", + "description": "A repository containing Grafana dashboards to demo the Github Sync feature in Grafana.", + "fork": false, + "url": "https://github.com/grafana/git-ui-sync-demo", + "default_branch": "main", + "master_branch": "main", + "organization": "grafana" + }, + "pusher": { + "name": "testuser", + "email": "test@grafana.com" + }, + "organization": { + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "url": "https://api.github.com/orgs/grafana", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4" + }, + "sender": { + "login": "testuser", + "id": 123456, + "node_id": "MDQ6VXNlcjEyMzQ1Ng==", + "avatar_url": "https://avatars.githubusercontent.com/u/123456?v=4", + "type": "User", + "site_admin": false + }, + "created": false, + "deleted": false, + "forced": false, + "base_ref": null, + "compare": "https://github.com/grafana/git-ui-sync-demo/compare/72096e3adc64...1234567890ab", + "commits": [ + { + "id": "1234567890abcdef1234567890abcdef12345678", + "tree_id": "abcdef1234567890abcdef1234567890abcdef12", + "distinct": true, + "message": "Remove empty folder by deleting .keep file", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/1234567890abcdef1234567890abcdef12345678", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "empty-folder/.keep" + ], + "modified": [] + } + ], + "head_commit": { + "id": "1234567890abcdef1234567890abcdef12345678", + "tree_id": "abcdef1234567890abcdef1234567890abcdef12", + "distinct": true, + "message": "Remove empty folder by deleting .keep file", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/1234567890abcdef1234567890abcdef12345678", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "empty-folder/.keep" + ], + "modified": [] + } +} + diff --git a/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_with_others.json b/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_with_others.json new file mode 100644 index 00000000000..9a70ca4997c --- /dev/null +++ b/apps/provisioning/pkg/repository/github/testdata/webhook-push-keep_file_with_others.json @@ -0,0 +1,109 @@ +{ + "ref": "refs/heads/main", + "before": "72096e3adc646c5a5b8a91744f962b12bac06045", + "after": "2345678901bcdef2345678901bcdef2345678901", + "repository": { + "id": 888020043, + "node_id": "R_kgDONO4cSw", + "name": "git-ui-sync-demo", + "full_name": "grafana/git-ui-sync-demo", + "private": true, + "owner": { + "name": "grafana", + "email": "hello@grafana.com", + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/grafana", + "html_url": "https://github.com/grafana", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/grafana/git-ui-sync-demo", + "description": "A repository containing Grafana dashboards to demo the Github Sync feature in Grafana.", + "fork": false, + "url": "https://github.com/grafana/git-ui-sync-demo", + "default_branch": "main", + "master_branch": "main", + "organization": "grafana" + }, + "pusher": { + "name": "testuser", + "email": "test@grafana.com" + }, + "organization": { + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "url": "https://api.github.com/orgs/grafana", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4" + }, + "sender": { + "login": "testuser", + "id": 123456, + "node_id": "MDQ6VXNlcjEyMzQ1Ng==", + "avatar_url": "https://avatars.githubusercontent.com/u/123456?v=4", + "type": "User", + "site_admin": false + }, + "created": false, + "deleted": false, + "forced": false, + "base_ref": null, + "compare": "https://github.com/grafana/git-ui-sync-demo/compare/72096e3adc64...2345678901bc", + "commits": [ + { + "id": "2345678901bcdef2345678901bcdef2345678901", + "tree_id": "bcdef2345678901bcdef2345678901bcdef23456", + "distinct": true, + "message": "Remove folder with .keep and dashboard files", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/2345678901bcdef2345678901bcdef2345678901", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "dashboards/.keep", + "dashboards/dashboard1.json", + "dashboards/dashboard2.json" + ], + "modified": [] + } + ], + "head_commit": { + "id": "2345678901bcdef2345678901bcdef2345678901", + "tree_id": "bcdef2345678901bcdef2345678901bcdef23456", + "distinct": true, + "message": "Remove folder with .keep and dashboard files", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/2345678901bcdef2345678901bcdef2345678901", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "dashboards/.keep", + "dashboards/dashboard1.json", + "dashboards/dashboard2.json" + ], + "modified": [] + } +} + diff --git a/apps/provisioning/pkg/repository/github/testdata/webhook-push-multiple_keep_files.json b/apps/provisioning/pkg/repository/github/testdata/webhook-push-multiple_keep_files.json new file mode 100644 index 00000000000..cbb82242aa3 --- /dev/null +++ b/apps/provisioning/pkg/repository/github/testdata/webhook-push-multiple_keep_files.json @@ -0,0 +1,109 @@ +{ + "ref": "refs/heads/main", + "before": "72096e3adc646c5a5b8a91744f962b12bac06045", + "after": "3456789012cdef3456789012cdef3456789012cd", + "repository": { + "id": 888020043, + "node_id": "R_kgDONO4cSw", + "name": "git-ui-sync-demo", + "full_name": "grafana/git-ui-sync-demo", + "private": true, + "owner": { + "name": "grafana", + "email": "hello@grafana.com", + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4", + "gravatar_id": "", + "url": "https://api.github.com/users/grafana", + "html_url": "https://github.com/grafana", + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/grafana/git-ui-sync-demo", + "description": "A repository containing Grafana dashboards to demo the Github Sync feature in Grafana.", + "fork": false, + "url": "https://github.com/grafana/git-ui-sync-demo", + "default_branch": "main", + "master_branch": "main", + "organization": "grafana" + }, + "pusher": { + "name": "testuser", + "email": "test@grafana.com" + }, + "organization": { + "login": "grafana", + "id": 7195757, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjcxOTU3NTc=", + "url": "https://api.github.com/orgs/grafana", + "avatar_url": "https://avatars.githubusercontent.com/u/7195757?v=4" + }, + "sender": { + "login": "testuser", + "id": 123456, + "node_id": "MDQ6VXNlcjEyMzQ1Ng==", + "avatar_url": "https://avatars.githubusercontent.com/u/123456?v=4", + "type": "User", + "site_admin": false + }, + "created": false, + "deleted": false, + "forced": false, + "base_ref": null, + "compare": "https://github.com/grafana/git-ui-sync-demo/compare/72096e3adc64...3456789012cd", + "commits": [ + { + "id": "3456789012cdef3456789012cdef3456789012cd", + "tree_id": "cdef3456789012cdef3456789012cdef34567890", + "distinct": true, + "message": "Remove multiple folders, some with only .keep files", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/3456789012cdef3456789012cdef3456789012cd", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "empty-folder1/.keep", + "dashboards-to-delete/.keep", + "dashboards-to-delete/dashboard.json" + ], + "modified": [] + } + ], + "head_commit": { + "id": "3456789012cdef3456789012cdef3456789012cd", + "tree_id": "cdef3456789012cdef3456789012cdef34567890", + "distinct": true, + "message": "Remove multiple folders, some with only .keep files", + "timestamp": "2024-12-09T11:00:48+03:00", + "url": "https://github.com/grafana/git-ui-sync-demo/commit/3456789012cdef3456789012cdef3456789012cd", + "author": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "committer": { + "name": "Test User", + "email": "test@grafana.com", + "username": "testuser" + }, + "added": [], + "removed": [ + "empty-folder1/.keep", + "dashboards-to-delete/.keep", + "dashboards-to-delete/dashboard.json" + ], + "modified": [] + } +} + diff --git a/apps/provisioning/pkg/repository/github/webhook.go b/apps/provisioning/pkg/repository/github/webhook.go index c2e13681f33..40f87ddc93d 100644 --- a/apps/provisioning/pkg/repository/github/webhook.go +++ b/apps/provisioning/pkg/repository/github/webhook.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "slices" + "strings" "github.com/google/go-github/v70/github" "github.com/google/uuid" @@ -15,6 +16,7 @@ import ( "github.com/grafana/grafana-app-sdk/logging" provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/grafana/grafana/apps/provisioning/pkg/safepath" common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" ) @@ -119,13 +121,38 @@ func (r *githubWebhookRepository) parsePushEvent(event *github.PushEvent) (*prov return &provisioning.WebhookResponse{Code: http.StatusOK}, nil } + // whenever possible, we want to do incremental syncs to keep things performant. + // however, if we get an event where just a .keep file is being deleted, and no other files in the folder + // are being deleted, the folder could be gone from git, but not from grafana and we do not have a way + // to get the grafana uid to delete the folder. so, instead, we will queue a full sync to clean things up. + dirsWithKeepDeletes := make(map[string]struct{}) + dirsWithOtherDeletes := make(map[string]struct{}) + for _, change := range event.GetCommits() { + for _, removedFile := range change.Removed { + dir := safepath.Dir(removedFile) + if strings.HasSuffix(removedFile, ".keep") { + dirsWithKeepDeletes[dir] = struct{}{} + } else { + dirsWithOtherDeletes[dir] = struct{}{} + } + } + } + // if there are any keep files deleted that do not have other files deleted in the same folder, we need to queue a full sync + incremental := true + for dir := range dirsWithKeepDeletes { + if _, exists := dirsWithOtherDeletes[dir]; !exists { + incremental = false + break + } + } + return &provisioning.WebhookResponse{ Code: http.StatusAccepted, Job: &provisioning.JobSpec{ Repository: r.config.GetName(), Action: provisioning.JobActionPull, Pull: &provisioning.SyncJobOptions{ - Incremental: true, + Incremental: incremental, }, }, }, nil diff --git a/apps/provisioning/pkg/repository/github/webhook_test.go b/apps/provisioning/pkg/repository/github/webhook_test.go index 49faf733378..5dc23c70e34 100644 --- a/apps/provisioning/pkg/repository/github/webhook_test.go +++ b/apps/provisioning/pkg/repository/github/webhook_test.go @@ -69,6 +69,36 @@ func TestParseWebhooks(t *testing.T) { }, }, }}, + {"push", "keep_file_only", provisioning.WebhookResponse{ + Code: http.StatusAccepted, + Job: &provisioning.JobSpec{ + Repository: "unit-test-repo", + Action: provisioning.JobActionPull, + Pull: &provisioning.SyncJobOptions{ + Incremental: false, + }, + }, + }}, + {"push", "keep_file_with_others", provisioning.WebhookResponse{ + Code: http.StatusAccepted, + Job: &provisioning.JobSpec{ + Repository: "unit-test-repo", + Action: provisioning.JobActionPull, + Pull: &provisioning.SyncJobOptions{ + Incremental: true, + }, + }, + }}, + {"push", "multiple_keep_files", provisioning.WebhookResponse{ + Code: http.StatusAccepted, + Job: &provisioning.JobSpec{ + Repository: "unit-test-repo", + Action: provisioning.JobActionPull, + Pull: &provisioning.SyncJobOptions{ + Incremental: false, + }, + }, + }}, {"issue_comment", "created", provisioning.WebhookResponse{ Code: http.StatusNotImplemented, }}, diff --git a/pkg/registry/apis/provisioning/jobs/sync/incremental.go b/pkg/registry/apis/provisioning/jobs/sync/incremental.go index 92639eee103..0c4ddde659b 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/incremental.go +++ b/pkg/registry/apis/provisioning/jobs/sync/incremental.go @@ -2,6 +2,7 @@ package sync import ( "context" + "errors" "fmt" "github.com/grafana/grafana/apps/provisioning/pkg/repository" @@ -9,6 +10,9 @@ import ( "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/registry/apis/provisioning/jobs" "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" + apierrors "k8s.io/apimachinery/pkg/api/errors" ) // Convert git changes into resource file changes @@ -38,6 +42,11 @@ func IncrementalSync(ctx context.Context, repo repository.Versioned, previousRef progress.SetTotal(ctx, len(diff)) progress.SetMessage(ctx, "replicating versioned changes") + // this will keep track of any folders that had resources deleted from it + // with key-value as path:grafana uid. + // after cleaning up all resources, we will look to see if the foldrs are + // now empty, and if so, delete them. + affectedFolders := make(map[string]string) for _, change := range diff { if ctx.Err() != nil { return ctx.Err() @@ -100,7 +109,7 @@ func IncrementalSync(ctx context.Context, repo repository.Versioned, previousRef writeSpan.End() case repository.FileActionDeleted: removeCtx, removeSpan := tracer.Start(ctx, "provisioning.sync.incremental.remove_resource_from_file") - name, gvk, err := repositoryResources.RemoveResourceFromFile(removeCtx, change.Path, change.PreviousRef) + name, folderName, gvk, err := repositoryResources.RemoveResourceFromFile(removeCtx, change.Path, change.PreviousRef) if err != nil { removeSpan.RecordError(err) result.Error = fmt.Errorf("removing resource from file %s: %w", change.Path, err) @@ -108,10 +117,15 @@ func IncrementalSync(ctx context.Context, repo repository.Versioned, previousRef result.Name = name result.Kind = gvk.Kind result.Group = gvk.Group + + if folderName != "" { + affectedFolders[safepath.Dir(change.Path)] = folderName + } + removeSpan.End() case repository.FileActionRenamed: renameCtx, renameSpan := tracer.Start(ctx, "provisioning.sync.incremental.rename_resource_file") - name, gvk, err := repositoryResources.RenameResourceFile(renameCtx, change.PreviousPath, change.PreviousRef, change.Path, change.Ref) + name, oldFolderName, gvk, err := repositoryResources.RenameResourceFile(renameCtx, change.PreviousPath, change.PreviousRef, change.Path, change.Ref) if err != nil { renameSpan.RecordError(err) result.Error = fmt.Errorf("renaming resource file from %s to %s: %w", change.PreviousPath, change.Path, err) @@ -119,6 +133,11 @@ func IncrementalSync(ctx context.Context, repo repository.Versioned, previousRef result.Name = name result.Kind = gvk.Kind result.Group = gvk.Group + + if oldFolderName != "" { + affectedFolders[safepath.Dir(change.Path)] = oldFolderName + } + renameSpan.End() case repository.FileActionIgnored: // do nothing @@ -128,5 +147,50 @@ func IncrementalSync(ctx context.Context, repo repository.Versioned, previousRef progress.SetMessage(ctx, "versioned changes replicated") + if len(affectedFolders) > 0 { + span.AddEvent("checking if impacted folders should be deleted", trace.WithAttributes(attribute.Int("affected_folders", len(affectedFolders)))) + if err := cleanupOrphanedFolders(ctx, repo, affectedFolders, repositoryResources, tracer); err != nil { + return tracing.Error(span, fmt.Errorf("cleanup orphaned folders: %w", err)) + } + } + + return nil +} + +// cleanupOrphanedFolders removes folders that no longer contain any resources in git after deletions have occurred. +func cleanupOrphanedFolders( + ctx context.Context, + repo repository.Versioned, + affectedFolders map[string]string, + repositoryResources resources.RepositoryResources, + tracer tracing.Tracer, +) error { + ctx, span := tracer.Start(ctx, "provisioning.sync.incremental.cleanup_orphaned_folders") + defer span.End() + + readerRepo, ok := repo.(repository.Reader) + if !ok { + span.RecordError(fmt.Errorf("repository does not implement Reader")) + return nil + } + + for path, folderName := range affectedFolders { + span.SetAttributes(attribute.String("folder", folderName)) + + // if we can no longer find the folder in git, then we can delete it from grafana + _, err := readerRepo.Read(ctx, path, "") + if err != nil && (errors.Is(err, repository.ErrFileNotFound) || apierrors.IsNotFound(err)) { + span.AddEvent("folder not found in git, removing from grafana") + if err := repositoryResources.RemoveFolder(ctx, folderName); err != nil { + span.RecordError(err) + } else { + span.AddEvent("successfully deleted") + } + continue + } + + span.AddEvent("folder still exists in git, continuing") + } + return nil } diff --git a/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go b/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go index 8b98f357695..95d595e8c6c 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go +++ b/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go @@ -187,7 +187,7 @@ func TestIncrementalSync(t *testing.T) { // Mock resource deletion repoResources.On("RemoveResourceFromFile", mock.Anything, "dashboards/old.json", "old-ref"). - Return("old-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) + Return("old-dashboard", "", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) // Mock progress recording progress.On("Record", mock.Anything, jobs.JobResourceResult{ @@ -223,7 +223,7 @@ func TestIncrementalSync(t *testing.T) { // Mock resource rename repoResources.On("RenameResourceFile", mock.Anything, "dashboards/old.json", "old-ref", "dashboards/new.json", "new-ref"). - Return("renamed-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) + Return("renamed-dashboard", "", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) // Mock progress recording progress.On("Record", mock.Anything, jobs.JobResourceResult{ @@ -340,7 +340,7 @@ func TestIncrementalSync(t *testing.T) { // Mock resource deletion error repoResources.On("RemoveResourceFromFile", mock.Anything, "dashboards/old.json", "old-ref"). - Return("old-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, fmt.Errorf("delete failed")) + Return("old-dashboard", "", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, fmt.Errorf("delete failed")) // Mock progress recording with error progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { @@ -398,3 +398,126 @@ func TestIncrementalSync(t *testing.T) { }) } } + +type compositeRepo struct { + *repository.MockVersioned + *repository.MockReader +} + +func TestIncrementalSync_CleanupOrphanedFolders(t *testing.T) { + tests := []struct { + name string + setupMocks func(*compositeRepo, *resources.MockRepositoryResources, *jobs.MockJobProgressRecorder) + expectedError string + }{ + { + name: "delete folder when it no longer exists in git", + setupMocks: func(repo *compositeRepo, repoResources *resources.MockRepositoryResources, progress *jobs.MockJobProgressRecorder) { + changes := []repository.VersionedFileChange{ + { + Action: repository.FileActionDeleted, + Path: "dashboards/old.json", + PreviousRef: "old-ref", + }, + } + repo.MockVersioned.On("CompareFiles", mock.Anything, "old-ref", "new-ref").Return(changes, nil) + progress.On("SetTotal", mock.Anything, 1).Return() + progress.On("SetMessage", mock.Anything, "replicating versioned changes").Return() + progress.On("SetMessage", mock.Anything, "versioned changes replicated").Return() + repoResources.On("RemoveResourceFromFile", mock.Anything, "dashboards/old.json", "old-ref"). + Return("old-dashboard", "folder-uid", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) + + // if the folder is not found in git, there should be a call to remove the folder from grafana + repo.MockReader.On("Read", mock.Anything, "dashboards/", ""). + Return((*repository.FileInfo)(nil), repository.ErrFileNotFound) + repoResources.On("RemoveFolder", mock.Anything, "folder-uid").Return(nil) + + progress.On("Record", mock.Anything, mock.Anything).Return() + progress.On("TooManyErrors").Return(nil) + }, + }, + { + name: "keep folder when it still exists in git", + setupMocks: func(repo *compositeRepo, repoResources *resources.MockRepositoryResources, progress *jobs.MockJobProgressRecorder) { + changes := []repository.VersionedFileChange{ + { + Action: repository.FileActionDeleted, + Path: "dashboards/old.json", + PreviousRef: "old-ref", + }, + } + repo.MockVersioned.On("CompareFiles", mock.Anything, "old-ref", "new-ref").Return(changes, nil) + progress.On("SetTotal", mock.Anything, 1).Return() + progress.On("SetMessage", mock.Anything, "replicating versioned changes").Return() + progress.On("SetMessage", mock.Anything, "versioned changes replicated").Return() + repoResources.On("RemoveResourceFromFile", mock.Anything, "dashboards/old.json", "old-ref"). + Return("old-dashboard", "folder-uid", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) + // if the folder still exists in git, there should not be a call to delete it from grafana + repo.MockReader.On("Read", mock.Anything, "dashboards/", ""). + Return(&repository.FileInfo{}, nil) + + progress.On("Record", mock.Anything, mock.Anything).Return() + progress.On("TooManyErrors").Return(nil) + }, + }, + { + name: "delete multiple folders when they no longer exist in git", + setupMocks: func(repo *compositeRepo, repoResources *resources.MockRepositoryResources, progress *jobs.MockJobProgressRecorder) { + changes := []repository.VersionedFileChange{ + { + Action: repository.FileActionDeleted, + Path: "dashboards/old.json", + PreviousRef: "old-ref", + }, + { + Action: repository.FileActionDeleted, + Path: "alerts/old-alert.yaml", + PreviousRef: "old-ref", + }, + } + repo.MockVersioned.On("CompareFiles", mock.Anything, "old-ref", "new-ref").Return(changes, nil) + progress.On("SetTotal", mock.Anything, 2).Return() + progress.On("SetMessage", mock.Anything, "replicating versioned changes").Return() + progress.On("SetMessage", mock.Anything, "versioned changes replicated").Return() + repoResources.On("RemoveResourceFromFile", mock.Anything, "dashboards/old.json", "old-ref"). + Return("old-dashboard", "folder-uid-1", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) + repoResources.On("RemoveResourceFromFile", mock.Anything, "alerts/old-alert.yaml", "old-ref"). + Return("old-alert", "folder-uid-2", schema.GroupVersionKind{Kind: "Alert", Group: "alerts"}, nil) + + // both not found in git, both should be deleted + repo.MockReader.On("Read", mock.Anything, "dashboards/", ""). + Return((*repository.FileInfo)(nil), repository.ErrFileNotFound) + repo.MockReader.On("Read", mock.Anything, "alerts/", ""). + Return((*repository.FileInfo)(nil), repository.ErrFileNotFound) + repoResources.On("RemoveFolder", mock.Anything, "folder-uid-1").Return(nil) + repoResources.On("RemoveFolder", mock.Anything, "folder-uid-2").Return(nil) + + progress.On("Record", mock.Anything, mock.Anything).Return() + progress.On("TooManyErrors").Return(nil) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockVersioned := repository.NewMockVersioned(t) + mockReader := repository.NewMockReader(t) + repo := &compositeRepo{ + MockVersioned: mockVersioned, + MockReader: mockReader, + } + repoResources := resources.NewMockRepositoryResources(t) + progress := jobs.NewMockJobProgressRecorder(t) + + tt.setupMocks(repo, repoResources, progress) + + err := IncrementalSync(context.Background(), repo, "old-ref", "new-ref", repoResources, progress, tracing.NewNoopTracerService()) + + if tt.expectedError != "" { + require.EqualError(t, err, tt.expectedError) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/resources/folders.go b/pkg/registry/apis/provisioning/resources/folders.go index ba6388b31cf..b74ab67976a 100644 --- a/pkg/registry/apis/provisioning/resources/folders.go +++ b/pkg/registry/apis/provisioning/resources/folders.go @@ -175,6 +175,10 @@ func (fm *FolderManager) GetFolder(ctx context.Context, name string) (*unstructu return fm.client.Get(ctx, name, metav1.GetOptions{}) } +func (fm *FolderManager) RemoveFolder(ctx context.Context, name string) error { + return fm.client.Delete(ctx, name, metav1.DeleteOptions{}) +} + // ReplicateTree replicates the folder tree to the repository. // The function fn is called for each folder. // If the folder already exists, the function is called with created set to false. diff --git a/pkg/registry/apis/provisioning/resources/repository.go b/pkg/registry/apis/provisioning/resources/repository.go index 67236d93cd5..6a9e16fc5a2 100644 --- a/pkg/registry/apis/provisioning/resources/repository.go +++ b/pkg/registry/apis/provisioning/resources/repository.go @@ -28,13 +28,14 @@ type RepositoryResources interface { EnsureFolderPathExist(ctx context.Context, filePath string) (parent string, err error) EnsureFolderExists(ctx context.Context, folder Folder, parentID string) error EnsureFolderTreeExists(ctx context.Context, ref, path string, tree FolderTree, fn func(folder Folder, created bool, err error) error) error + RemoveFolder(ctx context.Context, folderName string) error // File from Resource WriteResourceFileFromObject(ctx context.Context, obj *unstructured.Unstructured, options WriteOptions) (string, error) // Resource from file WriteResourceFromFile(ctx context.Context, path, ref string) (string, schema.GroupVersionKind, error) - RemoveResourceFromFile(ctx context.Context, path, ref string) (string, schema.GroupVersionKind, error) + RemoveResourceFromFile(ctx context.Context, path, ref string) (string, string, schema.GroupVersionKind, error) FindResourcePath(ctx context.Context, name string, gvk schema.GroupVersionKind) (string, error) - RenameResourceFile(ctx context.Context, path, previousRef, newPath, newRef string) (string, schema.GroupVersionKind, error) + RenameResourceFile(ctx context.Context, path, previousRef, newPath, newRef string) (string, string, schema.GroupVersionKind, error) // Stats Stats(ctx context.Context) (*provisioning.ResourceStats, error) List(ctx context.Context) (*provisioning.ResourceList, error) diff --git a/pkg/registry/apis/provisioning/resources/repository_resources_mock.go b/pkg/registry/apis/provisioning/resources/repository_resources_mock.go index 978d91f1d4d..1628971ff9e 100644 --- a/pkg/registry/apis/provisioning/resources/repository_resources_mock.go +++ b/pkg/registry/apis/provisioning/resources/repository_resources_mock.go @@ -297,8 +297,55 @@ func (_c *MockRepositoryResources_List_Call) RunAndReturn(run func(context.Conte return _c } +// RemoveFolder provides a mock function with given fields: ctx, folderName +func (_m *MockRepositoryResources) RemoveFolder(ctx context.Context, folderName string) error { + ret := _m.Called(ctx, folderName) + + if len(ret) == 0 { + panic("no return value specified for RemoveFolder") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { + r0 = rf(ctx, folderName) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MockRepositoryResources_RemoveFolder_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveFolder' +type MockRepositoryResources_RemoveFolder_Call struct { + *mock.Call +} + +// RemoveFolder is a helper method to define mock.On call +// - ctx context.Context +// - folderName string +func (_e *MockRepositoryResources_Expecter) RemoveFolder(ctx interface{}, folderName interface{}) *MockRepositoryResources_RemoveFolder_Call { + return &MockRepositoryResources_RemoveFolder_Call{Call: _e.mock.On("RemoveFolder", ctx, folderName)} +} + +func (_c *MockRepositoryResources_RemoveFolder_Call) Run(run func(ctx context.Context, folderName string)) *MockRepositoryResources_RemoveFolder_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string)) + }) + return _c +} + +func (_c *MockRepositoryResources_RemoveFolder_Call) Return(_a0 error) *MockRepositoryResources_RemoveFolder_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockRepositoryResources_RemoveFolder_Call) RunAndReturn(run func(context.Context, string) error) *MockRepositoryResources_RemoveFolder_Call { + _c.Call.Return(run) + return _c +} + // RemoveResourceFromFile provides a mock function with given fields: ctx, path, ref -func (_m *MockRepositoryResources) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, schema.GroupVersionKind, error) { +func (_m *MockRepositoryResources) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, string, schema.GroupVersionKind, error) { ret := _m.Called(ctx, path, ref) if len(ret) == 0 { @@ -306,9 +353,10 @@ func (_m *MockRepositoryResources) RemoveResourceFromFile(ctx context.Context, p } var r0 string - var r1 schema.GroupVersionKind - var r2 error - if rf, ok := ret.Get(0).(func(context.Context, string, string) (string, schema.GroupVersionKind, error)); ok { + var r1 string + var r2 schema.GroupVersionKind + var r3 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) (string, string, schema.GroupVersionKind, error)); ok { return rf(ctx, path, ref) } if rf, ok := ret.Get(0).(func(context.Context, string, string) string); ok { @@ -317,19 +365,25 @@ func (_m *MockRepositoryResources) RemoveResourceFromFile(ctx context.Context, p r0 = ret.Get(0).(string) } - if rf, ok := ret.Get(1).(func(context.Context, string, string) schema.GroupVersionKind); ok { + if rf, ok := ret.Get(1).(func(context.Context, string, string) string); ok { r1 = rf(ctx, path, ref) } else { - r1 = ret.Get(1).(schema.GroupVersionKind) + r1 = ret.Get(1).(string) } - if rf, ok := ret.Get(2).(func(context.Context, string, string) error); ok { + if rf, ok := ret.Get(2).(func(context.Context, string, string) schema.GroupVersionKind); ok { r2 = rf(ctx, path, ref) } else { - r2 = ret.Error(2) + r2 = ret.Get(2).(schema.GroupVersionKind) } - return r0, r1, r2 + if rf, ok := ret.Get(3).(func(context.Context, string, string) error); ok { + r3 = rf(ctx, path, ref) + } else { + r3 = ret.Error(3) + } + + return r0, r1, r2, r3 } // MockRepositoryResources_RemoveResourceFromFile_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RemoveResourceFromFile' @@ -352,18 +406,18 @@ func (_c *MockRepositoryResources_RemoveResourceFromFile_Call) Run(run func(ctx return _c } -func (_c *MockRepositoryResources_RemoveResourceFromFile_Call) Return(_a0 string, _a1 schema.GroupVersionKind, _a2 error) *MockRepositoryResources_RemoveResourceFromFile_Call { - _c.Call.Return(_a0, _a1, _a2) +func (_c *MockRepositoryResources_RemoveResourceFromFile_Call) Return(_a0 string, _a1 string, _a2 schema.GroupVersionKind, _a3 error) *MockRepositoryResources_RemoveResourceFromFile_Call { + _c.Call.Return(_a0, _a1, _a2, _a3) return _c } -func (_c *MockRepositoryResources_RemoveResourceFromFile_Call) RunAndReturn(run func(context.Context, string, string) (string, schema.GroupVersionKind, error)) *MockRepositoryResources_RemoveResourceFromFile_Call { +func (_c *MockRepositoryResources_RemoveResourceFromFile_Call) RunAndReturn(run func(context.Context, string, string) (string, string, schema.GroupVersionKind, error)) *MockRepositoryResources_RemoveResourceFromFile_Call { _c.Call.Return(run) return _c } // RenameResourceFile provides a mock function with given fields: ctx, path, previousRef, newPath, newRef -func (_m *MockRepositoryResources) RenameResourceFile(ctx context.Context, path string, previousRef string, newPath string, newRef string) (string, schema.GroupVersionKind, error) { +func (_m *MockRepositoryResources) RenameResourceFile(ctx context.Context, path string, previousRef string, newPath string, newRef string) (string, string, schema.GroupVersionKind, error) { ret := _m.Called(ctx, path, previousRef, newPath, newRef) if len(ret) == 0 { @@ -371,9 +425,10 @@ func (_m *MockRepositoryResources) RenameResourceFile(ctx context.Context, path } var r0 string - var r1 schema.GroupVersionKind - var r2 error - if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) (string, schema.GroupVersionKind, error)); ok { + var r1 string + var r2 schema.GroupVersionKind + var r3 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) (string, string, schema.GroupVersionKind, error)); ok { return rf(ctx, path, previousRef, newPath, newRef) } if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) string); ok { @@ -382,19 +437,25 @@ func (_m *MockRepositoryResources) RenameResourceFile(ctx context.Context, path r0 = ret.Get(0).(string) } - if rf, ok := ret.Get(1).(func(context.Context, string, string, string, string) schema.GroupVersionKind); ok { + if rf, ok := ret.Get(1).(func(context.Context, string, string, string, string) string); ok { r1 = rf(ctx, path, previousRef, newPath, newRef) } else { - r1 = ret.Get(1).(schema.GroupVersionKind) + r1 = ret.Get(1).(string) } - if rf, ok := ret.Get(2).(func(context.Context, string, string, string, string) error); ok { + if rf, ok := ret.Get(2).(func(context.Context, string, string, string, string) schema.GroupVersionKind); ok { r2 = rf(ctx, path, previousRef, newPath, newRef) } else { - r2 = ret.Error(2) + r2 = ret.Get(2).(schema.GroupVersionKind) } - return r0, r1, r2 + if rf, ok := ret.Get(3).(func(context.Context, string, string, string, string) error); ok { + r3 = rf(ctx, path, previousRef, newPath, newRef) + } else { + r3 = ret.Error(3) + } + + return r0, r1, r2, r3 } // MockRepositoryResources_RenameResourceFile_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RenameResourceFile' @@ -419,12 +480,12 @@ func (_c *MockRepositoryResources_RenameResourceFile_Call) Run(run func(ctx cont return _c } -func (_c *MockRepositoryResources_RenameResourceFile_Call) Return(_a0 string, _a1 schema.GroupVersionKind, _a2 error) *MockRepositoryResources_RenameResourceFile_Call { - _c.Call.Return(_a0, _a1, _a2) +func (_c *MockRepositoryResources_RenameResourceFile_Call) Return(_a0 string, _a1 string, _a2 schema.GroupVersionKind, _a3 error) *MockRepositoryResources_RenameResourceFile_Call { + _c.Call.Return(_a0, _a1, _a2, _a3) return _c } -func (_c *MockRepositoryResources_RenameResourceFile_Call) RunAndReturn(run func(context.Context, string, string, string, string) (string, schema.GroupVersionKind, error)) *MockRepositoryResources_RenameResourceFile_Call { +func (_c *MockRepositoryResources_RenameResourceFile_Call) RunAndReturn(run func(context.Context, string, string, string, string) (string, string, schema.GroupVersionKind, error)) *MockRepositoryResources_RenameResourceFile_Call { _c.Call.Return(run) return _c } diff --git a/pkg/registry/apis/provisioning/resources/resources.go b/pkg/registry/apis/provisioning/resources/resources.go index 027f04671ff..b66e16917da 100644 --- a/pkg/registry/apis/provisioning/resources/resources.go +++ b/pkg/registry/apis/provisioning/resources/resources.go @@ -238,44 +238,63 @@ func (r *ResourcesManager) WriteResourceFromFile(ctx context.Context, path strin return parsed.Obj.GetName(), parsed.GVK, err } -func (r *ResourcesManager) RenameResourceFile(ctx context.Context, previousPath, previousRef, newPath, newRef string) (string, schema.GroupVersionKind, error) { - name, gvk, err := r.RemoveResourceFromFile(ctx, previousPath, previousRef) +func (r *ResourcesManager) RenameResourceFile(ctx context.Context, previousPath, previousRef, newPath, newRef string) (string, string, schema.GroupVersionKind, error) { + name, oldFolderName, gvk, err := r.RemoveResourceFromFile(ctx, previousPath, previousRef) if err != nil { - return name, gvk, fmt.Errorf("failed to remove resource: %w", err) + return name, oldFolderName, gvk, fmt.Errorf("failed to remove resource: %w", err) } - return r.WriteResourceFromFile(ctx, newPath, newRef) + newName, gvk, err := r.WriteResourceFromFile(ctx, newPath, newRef) + if err != nil { + return name, oldFolderName, gvk, fmt.Errorf("failed to write resource: %w", err) + } + + return newName, oldFolderName, gvk, nil } -func (r *ResourcesManager) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, schema.GroupVersionKind, error) { +func (r *ResourcesManager) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, string, schema.GroupVersionKind, error) { info, err := r.repo.Read(ctx, path, ref) if err != nil { - return "", schema.GroupVersionKind{}, fmt.Errorf("failed to read file: %w", err) + return "", "", schema.GroupVersionKind{}, fmt.Errorf("failed to read file: %w", err) } obj, gvk, _ := DecodeYAMLObject(bytes.NewBuffer(info.Data)) if obj == nil { - return "", schema.GroupVersionKind{}, fmt.Errorf("no object found") + return "", "", schema.GroupVersionKind{}, fmt.Errorf("no object found") } objName := obj.GetName() if objName == "" { - return "", schema.GroupVersionKind{}, ErrMissingName + return "", "", schema.GroupVersionKind{}, ErrMissingName } client, _, err := r.clients.ForKind(ctx, *gvk) if err != nil { - return "", schema.GroupVersionKind{}, fmt.Errorf("unable to get client for deleted object: %w", err) + return "", "", schema.GroupVersionKind{}, fmt.Errorf("unable to get client for deleted object: %w", err) } + // the folder annotation is not stored in the git file, so we need to get it from grafana + grafanaObj, err := client.Get(ctx, objName, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return objName, "", schema.GroupVersionKind{}, nil // Already deleted or simply non-existing, nothing to do + } + return "", "", schema.GroupVersionKind{}, fmt.Errorf("unable to get grafana object: %w", err) + } + meta, err := utils.MetaAccessor(grafanaObj) + if err != nil { + return "", "", schema.GroupVersionKind{}, fmt.Errorf("unable to get meta accessor: %w", err) + } + folderName := meta.GetFolder() + err = client.Delete(ctx, objName, metav1.DeleteOptions{}) if err != nil { if apierrors.IsNotFound(err) { - return objName, schema.GroupVersionKind{}, nil // Already deleted or simply non-existing, nothing to do + return objName, folderName, schema.GroupVersionKind{}, nil // Already deleted or simply non-existing, nothing to do } - return "", schema.GroupVersionKind{}, fmt.Errorf("failed to delete: %w", err) + return "", "", schema.GroupVersionKind{}, fmt.Errorf("failed to delete: %w", err) } - return objName, schema.GroupVersionKind{}, nil + return objName, folderName, schema.GroupVersionKind{}, nil }