From 9ca0750134cb583b1ac0b88477766195aaa53e0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Fri, 1 Aug 2025 13:17:15 +0200 Subject: [PATCH] Provisioning: skip export of already managed resources and parent folder export (#108893) * Skip export on already managed resources * Add integration test * Add integration test * Handle nothing to commit error * Fix leaky abstraction issue * Handle the no commit error on commit and not on push * Fix linting * Some fixes for integration test * Improve tree to work with a root * Some fixes with hacks * Add additional checks * Fix comment * Fix path problems in test * Fix more stuff * Revert to use empty tree * Remove changes in tree * Finally fix the tests work * Remove stale comment * Fix linting * Revert changes in test * Fix error message for folder not found in resource tree Co-authored-by: roberto.jimenez --------- Co-authored-by: Cursor Agent --- .../apis/provisioning/jobs/export/folders.go | 5 +- .../provisioning/jobs/export/folders_test.go | 89 ++++---- .../provisioning/jobs/export/resources.go | 18 ++ .../jobs/export/resources_test.go | 35 ++++ .../provisioning/repository/git/repository.go | 4 + .../provisioning/repository/git/staged.go | 14 +- .../repository/git/staged_test.go | 56 ++++- .../apis/provisioning/repository/staged.go | 9 +- .../provisioning/repository/staged_test.go | 79 +++++++ .../apis/provisioning/resources/resources.go | 18 +- pkg/tests/apis/provisioning/helper_test.go | 48 ++++- .../apis/provisioning/provisioning_test.go | 193 ++++++++++++++++++ 12 files changed, 518 insertions(+), 50 deletions(-) diff --git a/pkg/registry/apis/provisioning/jobs/export/folders.go b/pkg/registry/apis/provisioning/jobs/export/folders.go index afc7db05a87..c8b108d6023 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders.go @@ -32,8 +32,9 @@ func ExportFolders(ctx context.Context, repoName string, options provisioning.Ex } manager, _ := meta.GetManagerProperties() - if manager.Identity == repoName { - return nil // skip it... already in tree? + // Skip if already managed by any manager (repository, file provisioning, etc.) + if manager.Identity != "" { + return nil } return tree.AddUnstructured(item) diff --git a/pkg/registry/apis/provisioning/jobs/export/folders_test.go b/pkg/registry/apis/provisioning/jobs/export/folders_test.go index bec0214d7ef..49c07a64347 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders_test.go @@ -298,38 +298,21 @@ func TestExportFolders(t *testing.T) { progress.On("SetMessage", mock.Anything, "read folder tree from API server").Return() progress.On("SetMessage", mock.Anything, "write folders to repository").Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "parent-uid" && result.Action == repository.FileActionCreated + return result.Name == "parent-folder" && result.Action == repository.FileActionCreated })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "child-uid" && result.Action == repository.FileActionCreated + return result.Name == "child-folder" && result.Action == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) }, setupResources: func(repoResources *resources.MockRepositoryResources) { repoResources.On("EnsureFolderTreeExists", mock.Anything, "feature/branch", "grafana", mock.MatchedBy(func(tree resources.FolderTree) bool { - expectedFolders := []resources.Folder{ - {ID: "parent-folder", Path: "parent-folder"}, - {ID: "child-folder", Path: "parent-folder/child-folder"}, - } - - if tree.Count() != len(expectedFolders) { - return false - } - - for _, folder := range expectedFolders { - dir, ok := tree.DirPath(folder.ID, "") - if !ok || dir.Path != folder.Path { - return false - } - } - - return true + return tree.Count() == 2 }), mock.MatchedBy(func(fn func(folder resources.Folder, created bool, err error) error) bool { - // Parent folder should be processed first - require.NoError(t, fn(resources.Folder{ID: "parent-uid", Path: "grafana/parent-folder"}, true, nil)) - // Then child folder with nested path - require.NoError(t, fn(resources.Folder{ID: "child-uid", Path: "grafana/parent-folder/child-folder"}, true, nil)) + require.NoError(t, fn(resources.Folder{ID: "parent-folder", Path: "grafana/parent-folder"}, true, nil)) + require.NoError(t, fn(resources.Folder{ID: "child-folder", Path: "grafana/parent-folder/child-folder"}, true, nil)) + return true })).Return(nil) }, @@ -380,7 +363,7 @@ func TestExportFolders(t *testing.T) { } func TestFolderMetaAccessor(t *testing.T) { - t.Run("should export folders from another manager", func(t *testing.T) { + t.Run("should skip folders from another manager", func(t *testing.T) { obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -405,21 +388,12 @@ func TestFolderMetaAccessor(t *testing.T) { mockRepoResources := resources.NewMockRepositoryResources(t) mockRepoResources.On("EnsureFolderTreeExists", mock.Anything, "feature/branch", "grafana", mock.MatchedBy(func(tree resources.FolderTree) bool { - return tree.Count() == 1 - }), mock.MatchedBy(func(fn func(folder resources.Folder, created bool, err error) error) bool { - require.NoError(t, fn(resources.Folder{ID: "test-folder-uid", Path: "grafana/test-folder"}, true, nil)) - return true - })).Return(nil) + return tree.Count() == 0 // Should be 0 since folder is managed by other manager + }), mock.Anything).Return(nil) progress := jobs.NewMockJobProgressRecorder(t) - progress.On("SetMessage", mock.Anything, mock.Anything).Return() - progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Name == "test-folder-uid" && - result.Error == nil && - result.Path == "grafana/test-folder" - })).Return() - progress.On("TooManyErrors").Return(nil) + progress.On("SetMessage", mock.Anything, mock.Anything).Return().Twice() + // No Record calls expected since folder should be skipped err = ExportFolders(context.Background(), "test-repo", v0alpha1.ExportJobOptions{ Path: "grafana", Branch: "feature/branch", @@ -430,7 +404,7 @@ func TestFolderMetaAccessor(t *testing.T) { mockRepoResources.AssertExpectations(t) progress.AssertExpectations(t) }) - t.Run("should skip if repo is the manager", func(t *testing.T) { + t.Run("should skip if current repo is the manager", func(t *testing.T) { obj := &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": map[string]interface{}{ @@ -492,6 +466,45 @@ func TestFolderMetaAccessor(t *testing.T) { mockRepoResources.AssertExpectations(t) progress.AssertExpectations(t) }) + t.Run("should skip if managed by any other manager", func(t *testing.T) { + obj := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "metadata": map[string]interface{}{ + "name": "test-folder", + "annotations": map[string]interface{}{ + "folder.grafana.app/uid": "test-folder-uid", + }, + }, + }, + } + meta, err := utils.MetaAccessor(obj) + require.NoError(t, err) + meta.SetManagerProperties(utils.ManagerProperties{ + Kind: utils.ManagerKindTerraform, + Identity: "terraform-provisioning", + AllowsEdits: false, + Suspended: false, + }) + fakeFolderClient := &mockDynamicInterface{ + items: []unstructured.Unstructured{*obj}, + } + + mockRepoResources := resources.NewMockRepositoryResources(t) + progress := jobs.NewMockJobProgressRecorder(t) + progress.On("SetMessage", mock.Anything, mock.Anything).Return().Twice() + mockRepoResources.On("EnsureFolderTreeExists", mock.Anything, "feature/branch", "grafana", mock.MatchedBy(func(tree resources.FolderTree) bool { + return tree.Count() == 0 // Should be empty since folder was skipped + }), mock.Anything).Return(nil) + + err = ExportFolders(context.Background(), "test-repo", v0alpha1.ExportJobOptions{ + Path: "grafana", + Branch: "feature/branch", + }, fakeFolderClient, mockRepoResources, progress) + + require.NoError(t, err) + mockRepoResources.AssertExpectations(t) + progress.AssertExpectations(t) + }) } // mockDynamicInterface implements a simplified version of the dynamic.ResourceInterface diff --git a/pkg/registry/apis/provisioning/jobs/export/resources.go b/pkg/registry/apis/provisioning/jobs/export/resources.go index b986392c3d6..54805784cee 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources.go @@ -10,6 +10,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/dynamic" + "github.com/grafana/grafana/pkg/apimachinery/utils" provisioning "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" @@ -103,6 +104,23 @@ func exportResource(ctx context.Context, Action: repository.FileActionCreated, } + // Check if resource is already managed by a repository + meta, err := utils.MetaAccessor(item) + if err != nil { + result.Action = repository.FileActionIgnored + result.Error = fmt.Errorf("extract meta accessor: %w", err) + progress.Record(ctx, result) + return nil + } + + manager, _ := meta.GetManagerProperties() + // Skip if already managed by any manager (repository, file provisioning, etc.) + if manager.Identity != "" { + result.Action = repository.FileActionIgnored + progress.Record(ctx, result) + return nil + } + if shim != nil { item, err = shim(ctx, item) } diff --git a/pkg/registry/apis/provisioning/jobs/export/resources_test.go b/pkg/registry/apis/provisioning/jobs/export/resources_test.go index db8209cf414..e77ae6105ed 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/grafana/grafana/pkg/apimachinery/utils" mock "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -532,3 +533,37 @@ func TestExportResources_Dashboards_V2beta1_ClientError(t *testing.T) { err := runExportTest(t, mockItems, setupProgress, setupResources) require.NoError(t, err) } + +func TestExportResources_Dashboards_SkipsManagedResources(t *testing.T) { + // Create a dashboard managed by file provisioning + dashboard := createDashboardObject("managed-dashboard") + + // Add manager metadata using utils package + meta, err := utils.MetaAccessor(&dashboard) + require.NoError(t, err) + meta.SetManagerProperties(utils.ManagerProperties{ + Kind: utils.ManagerKindTerraform, + Identity: "terraform-provisioning", + AllowsEdits: false, + Suspended: false, + }) + + mockItems := []unstructured.Unstructured{dashboard} + + setupProgress := func(progress *jobs.MockJobProgressRecorder) { + progress.On("SetMessage", mock.Anything, "start resource export").Return() + progress.On("SetMessage", mock.Anything, "export dashboards").Return() + progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { + return result.Name == "managed-dashboard" && result.Action == repository.FileActionIgnored + })).Return() + progress.On("TooManyErrors").Return(nil).Maybe() + } + + setupResources := func(repoResources *resources.MockRepositoryResources, resourceClients *resources.MockResourceClients, mockClient *mockDynamicInterface, gvk schema.GroupVersionKind) { + resourceClients.On("ForResource", resources.DashboardResource).Return(mockClient, gvk, nil) + // No WriteResourceFileFromObject call expected since resource should be skipped + } + + err = runExportTest(t, mockItems, setupProgress, setupResources) + require.NoError(t, err) +} diff --git a/pkg/registry/apis/provisioning/repository/git/repository.go b/pkg/registry/apis/provisioning/repository/git/repository.go index 5bcdb12cd94..7ca9ed8998e 100644 --- a/pkg/registry/apis/provisioning/repository/git/repository.go +++ b/pkg/registry/apis/provisioning/repository/git/repository.go @@ -790,6 +790,10 @@ func (r *gitRepository) createSignature(ctx context.Context) (nanogit.Author, na func (r *gitRepository) commit(ctx context.Context, writer nanogit.StagedWriter, comment string) error { author, committer := r.createSignature(ctx) if _, err := writer.Commit(ctx, comment, author, committer); err != nil { + if errors.Is(err, nanogit.ErrNothingToCommit) { + return repository.ErrNothingToCommit + } + return fmt.Errorf("commit changes: %w", err) } return nil diff --git a/pkg/registry/apis/provisioning/repository/git/staged.go b/pkg/registry/apis/provisioning/repository/git/staged.go index a3f79567029..f55eb709341 100644 --- a/pkg/registry/apis/provisioning/repository/git/staged.go +++ b/pkg/registry/apis/provisioning/repository/git/staged.go @@ -219,12 +219,24 @@ func (r *stagedGitRepository) Push(ctx context.Context) error { if message == "" { message = "Staged changes" } + if err := r.commit(ctx, r.writer, message); err != nil { return err } } - return r.writer.Push(ctx) + err := r.writer.Push(ctx) + if err != nil { + // Convert nanogit-specific errors to repository-level errors to avoid leaky abstraction + if errors.Is(err, nanogit.ErrNothingToPush) { + return repository.ErrNothingToPush + } + if errors.Is(err, nanogit.ErrNothingToCommit) { + return repository.ErrNothingToCommit + } + return err + } + return nil } func (r *stagedGitRepository) Remove(ctx context.Context) error { diff --git a/pkg/registry/apis/provisioning/repository/git/staged_test.go b/pkg/registry/apis/provisioning/repository/git/staged_test.go index 4501a087e7a..77c1d8b5edb 100644 --- a/pkg/registry/apis/provisioning/repository/git/staged_test.go +++ b/pkg/registry/apis/provisioning/repository/git/staged_test.go @@ -3,6 +3,7 @@ package git import ( "context" "errors" + "fmt" "strings" "testing" "time" @@ -995,6 +996,54 @@ func TestStagedGitRepository_Push(t *testing.T) { expectPushCalls: 1, expectCommitCalls: 1, }, + { + name: "returns repository ErrNothingToPush when nanogit returns ErrNothingToPush", + opts: repository.StageOptions{}, + setupMock: func(mockWriter *mocks.FakeStagedWriter) { + mockWriter.PushReturns(nanogit.ErrNothingToPush) + }, + wantError: repository.ErrNothingToPush, + expectPushCalls: 1, + expectCommitCalls: 0, + }, + { + name: "returns repository ErrNothingToCommit when nanogit returns ErrNothingToCommit", + opts: repository.StageOptions{ + Mode: repository.StageModeCommitOnlyOnce, + }, + setupMock: func(mockWriter *mocks.FakeStagedWriter) { + mockWriter.CommitReturns(nil, nanogit.ErrNothingToCommit) + }, + wantError: repository.ErrNothingToCommit, + expectPushCalls: 0, + expectCommitCalls: 1, + }, + { + name: "returns repository ErrNothingToPush when nanogit returns wrapped ErrNothingToPush", + opts: repository.StageOptions{}, + setupMock: func(mockWriter *mocks.FakeStagedWriter) { + // Use fmt.Errorf with %w to create a wrapped error that errors.Is can detect + wrappedErr := fmt.Errorf("git operation failed: %w", nanogit.ErrNothingToPush) + mockWriter.PushReturns(wrappedErr) + }, + wantError: repository.ErrNothingToPush, + expectPushCalls: 1, + expectCommitCalls: 0, + }, + { + name: "returns repository ErrNothingToCommit when nanogit returns wrapped ErrNothingToCommit", + opts: repository.StageOptions{ + Mode: repository.StageModeCommitOnlyOnce, + }, + setupMock: func(mockWriter *mocks.FakeStagedWriter) { + // Use fmt.Errorf with %w to create a wrapped error that errors.Is can detect + wrappedErr := fmt.Errorf("git operation failed: %w", nanogit.ErrNothingToCommit) + mockWriter.CommitReturns(nil, wrappedErr) + }, + wantError: repository.ErrNothingToCommit, + expectPushCalls: 0, + expectCommitCalls: 1, + }, } for _, tt := range tests { @@ -1007,7 +1056,12 @@ func TestStagedGitRepository_Push(t *testing.T) { err := stagedRepo.Push(context.Background()) if tt.wantError != nil { - require.EqualError(t, err, tt.wantError.Error()) + // For nanogit error conversion tests, use ErrorIs to verify type conversion + if errors.Is(tt.wantError, repository.ErrNothingToPush) || errors.Is(tt.wantError, repository.ErrNothingToCommit) { + require.ErrorIs(t, err, tt.wantError) + } else { + require.EqualError(t, err, tt.wantError.Error()) + } } else { require.NoError(t, err) } diff --git a/pkg/registry/apis/provisioning/repository/staged.go b/pkg/registry/apis/provisioning/repository/staged.go index ca5b59c72c9..1861d66336c 100644 --- a/pkg/registry/apis/provisioning/repository/staged.go +++ b/pkg/registry/apis/provisioning/repository/staged.go @@ -7,9 +7,14 @@ import ( "time" "github.com/grafana/grafana-app-sdk/logging" - "github.com/grafana/nanogit" ) +// ErrNothingToPush indicates that there are no changes to push to the remote repository +var ErrNothingToPush = errors.New("nothing to push") + +// ErrNothingToCommit indicates that there are no changes to commit +var ErrNothingToCommit = errors.New("nothing to commit") + //go:generate mockery --name WrapWithStageFn --structname MockWrapWithStageFn --inpackage --filename mock_wrap_with_stage_fn.go --with-expecter type WrapWithStageFn func(ctx context.Context, repo Repository, stageOptions StageOptions, fn func(repo Repository, staged bool) error) error @@ -83,7 +88,7 @@ func WrapWithStageAndPushIfPossible( } if err = staged.Push(ctx); err != nil { - if errors.Is(err, nanogit.ErrNothingToPush) { + if errors.Is(err, ErrNothingToPush) || errors.Is(err, ErrNothingToCommit) { return nil // OK, already pushed } return fmt.Errorf("wrapped push error: %w", err) diff --git a/pkg/registry/apis/provisioning/repository/staged_test.go b/pkg/registry/apis/provisioning/repository/staged_test.go index 3efe8ebbd5e..c640e2cd0db 100644 --- a/pkg/registry/apis/provisioning/repository/staged_test.go +++ b/pkg/registry/apis/provisioning/repository/staged_test.go @@ -3,6 +3,7 @@ package repository import ( "context" "errors" + "fmt" "testing" "github.com/stretchr/testify/mock" @@ -127,6 +128,84 @@ func TestWrapWithStageAndPushIfPossible(t *testing.T) { return nil }, }, + { + name: "nothing to push - should not error", + setupMocks: func(t *testing.T) *mockStagedRepo { + mockRepo := NewMockStageableRepository(t) + mockStaged := NewMockStagedRepository(t) + + mockRepo.EXPECT().Stage(mock.Anything, StageOptions{}).Return(mockStaged, nil) + mockStaged.EXPECT().Push(mock.Anything).Return(ErrNothingToPush) + mockStaged.EXPECT().Remove(mock.Anything).Return(nil) + + return &mockStagedRepo{ + MockStageableRepository: mockRepo, + MockStagedRepository: mockStaged, + } + }, + operation: func(repo Repository, staged bool) error { + return nil + }, + }, + { + name: "nothing to commit - should not error", + setupMocks: func(t *testing.T) *mockStagedRepo { + mockRepo := NewMockStageableRepository(t) + mockStaged := NewMockStagedRepository(t) + + mockRepo.EXPECT().Stage(mock.Anything, StageOptions{}).Return(mockStaged, nil) + mockStaged.EXPECT().Push(mock.Anything).Return(ErrNothingToCommit) + mockStaged.EXPECT().Remove(mock.Anything).Return(nil) + + return &mockStagedRepo{ + MockStageableRepository: mockRepo, + MockStagedRepository: mockStaged, + } + }, + operation: func(repo Repository, staged bool) error { + return nil + }, + }, + { + name: "wrapped nothing to push error - should not error", + setupMocks: func(t *testing.T) *mockStagedRepo { + mockRepo := NewMockStageableRepository(t) + mockStaged := NewMockStagedRepository(t) + + wrappedErr := fmt.Errorf("some wrapper: %w", ErrNothingToPush) + mockRepo.EXPECT().Stage(mock.Anything, StageOptions{}).Return(mockStaged, nil) + mockStaged.EXPECT().Push(mock.Anything).Return(wrappedErr) + mockStaged.EXPECT().Remove(mock.Anything).Return(nil) + + return &mockStagedRepo{ + MockStageableRepository: mockRepo, + MockStagedRepository: mockStaged, + } + }, + operation: func(repo Repository, staged bool) error { + return nil + }, + }, + { + name: "wrapped nothing to commit error - should not error", + setupMocks: func(t *testing.T) *mockStagedRepo { + mockRepo := NewMockStageableRepository(t) + mockStaged := NewMockStagedRepository(t) + + wrappedErr := fmt.Errorf("some wrapper: %w", ErrNothingToCommit) + mockRepo.EXPECT().Stage(mock.Anything, StageOptions{}).Return(mockStaged, nil) + mockStaged.EXPECT().Push(mock.Anything).Return(wrappedErr) + mockStaged.EXPECT().Remove(mock.Anything).Return(nil) + + return &mockStagedRepo{ + MockStageableRepository: mockRepo, + MockStagedRepository: mockStaged, + } + }, + operation: func(repo Repository, staged bool) error { + return nil + }, + }, } for _, tt := range tests { diff --git a/pkg/registry/apis/provisioning/resources/resources.go b/pkg/registry/apis/provisioning/resources/resources.go index 28dd06ea43a..ec5942c21ec 100644 --- a/pkg/registry/apis/provisioning/resources/resources.go +++ b/pkg/registry/apis/provisioning/resources/resources.go @@ -92,19 +92,29 @@ func (r *ResourcesManager) WriteResourceFileFromObject(ctx context.Context, obj if title == "" { title = name } - folder := meta.GetFolder() + folder := meta.GetFolder() // Get the absolute path of the folder rootFolder := RootFolder(r.repo.Config()) - fid, ok := r.folders.Tree().DirPath(folder, rootFolder) - if !ok { - return "", fmt.Errorf("folder not found in tree: %s", folder) + + // If no folder is specified in the file, set it to the root to ensure everything is written under it + var fid Folder + if folder == "" { + fid = Folder{ID: rootFolder} + meta.SetFolder(rootFolder) // Set the folder in the metadata to the root folder + } else { + var ok bool + fid, ok = r.folders.Tree().DirPath(folder, rootFolder) + if !ok { + return "", fmt.Errorf("folder %s NOT found in tree with root: %s", folder, rootFolder) + } } fileName := slugify.Slugify(title) + ".json" if fid.Path != "" { fileName = safepath.Join(fid.Path, fileName) } + if options.Path != "" { fileName = safepath.Join(options.Path, fileName) } diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index 2d5585b819f..03ca9342f31 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -112,11 +112,11 @@ func (h *provisioningTestHelper) AwaitJobSuccess(t *testing.T, ctx context.Conte require.NoError(t, err) require.NotNil(t, result) + errors := mustNestedStringSlice(result.Object, "status", "errors") + require.Empty(t, errors, "historic job '%s' has errors: %v", job.GetName(), errors) state := mustNestedString(result.Object, "status", "state") require.Equal(t, string(provisioning.JobStateSuccess), state, "historic job '%s' was not successful", job.GetName()) - errors := mustNestedStringSlice(result.Object, "status", "errors") - require.Empty(t, errors, "historic job '%s' has errors: %v", job.GetName(), errors) }, time.Second*10, time.Millisecond*25) { // We also want to add the job details to the error when it fails. job, err := h.Jobs.Resource.Get(ctx, job.GetName(), metav1.GetOptions{}) @@ -163,6 +163,50 @@ func (h *provisioningTestHelper) AwaitJobs(t *testing.T, repoName string) { } } +// AwaitJobsWithStates waits for all jobs for a repository to complete and accepts multiple valid end states +func (h *provisioningTestHelper) AwaitJobsWithStates(t *testing.T, repoName string, acceptedStates []string) { + t.Helper() + + // First, we wait for all jobs for the repository to disappear (i.e. complete/fail). + require.EventuallyWithT(t, func(collect *assert.CollectT) { + list, err := h.Jobs.Resource.List(context.Background(), metav1.ListOptions{}) + if assert.NoError(collect, err, "failed to list active jobs") { + for _, elem := range list.Items { + repo, _, err := unstructured.NestedString(elem.Object, "spec", "repository") + require.NoError(t, err) + if repo == repoName { + collect.Errorf("there are still remaining jobs for %s: %+v", repoName, elem) + return + } + } + } + }, time.Second*10, time.Millisecond*25, "job queue must be empty") + + // Then, as all jobs are now historic jobs, we make sure they are in an accepted state. + result, err := h.Repositories.Resource.Get(context.Background(), repoName, metav1.GetOptions{}, "jobs") + require.NoError(t, err, "failed to list historic jobs") + + list, err := result.ToList() + require.NoError(t, err, "results should be a list") + require.NotEmpty(t, list.Items, "expect at least one job") + + for _, elem := range list.Items { + require.Equal(t, repoName, elem.GetLabels()[jobs.LabelRepository], "should have repo label") + + state := mustNestedString(elem.Object, "status", "state") + + // Check if state is in accepted states + found := false + for _, acceptedState := range acceptedStates { + if state == acceptedState { + found = true + break + } + } + require.True(t, found, "job %s completed with unexpected state %s (expected one of %v): %+v", elem.GetName(), state, acceptedStates, elem.Object) + } +} + // RenderObject reads the filePath and renders it as a template with the given values. // The template is expected to be a YAML or JSON file. // diff --git a/pkg/tests/apis/provisioning/provisioning_test.go b/pkg/tests/apis/provisioning/provisioning_test.go index cdf210e2133..e04406787fe 100644 --- a/pkg/tests/apis/provisioning/provisioning_test.go +++ b/pkg/tests/apis/provisioning/provisioning_test.go @@ -2107,3 +2107,196 @@ func TestIntegrationProvisioning_MoveResources(t *testing.T) { }, time.Second*10, time.Millisecond*100, "Expected move job to handle non-existent resource") }) } + +func TestIntegrationProvisioning_SecondRepositoryOnlyExportsNewDashboards(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := runGrafana(t) + ctx := context.Background() + + // Create some unmanaged dashboards directly in Grafana first + dashboard1 := helper.LoadYAMLOrJSONFile("exportunifiedtorepository/dashboard-test-v1.yaml") + dashboard1Obj, err := helper.DashboardsV1.Resource.Create(ctx, dashboard1, metav1.CreateOptions{}) + require.NoError(t, err, "should be able to create first dashboard") + dashboard1Name := dashboard1Obj.GetName() + + dashboard2 := helper.LoadYAMLOrJSONFile("exportunifiedtorepository/dashboard-test-v2beta1.yaml") + dashboard2Obj, err := helper.DashboardsV2beta1.Resource.Create(ctx, dashboard2, metav1.CreateOptions{}) + require.NoError(t, err, "should be able to create second dashboard") + dashboard2Name := dashboard2Obj.GetName() + + // Create the first repository with sync enabled + const repo1 = "first-repository" + repo1Path := filepath.Join(helper.ProvisioningPath, repo1) + err = os.MkdirAll(repo1Path, 0750) + require.NoError(t, err, "should be able to create repository path") + + createBody1 := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": repo1, + "SyncEnabled": true, + "SyncTarget": "folder", + "Path": repo1Path, + }) + _, err = helper.Repositories.Resource.Create(ctx, createBody1, metav1.CreateOptions{}) + require.NoError(t, err, "should be able to create first repository") + + // Print file tree before export + printFileTree(t, helper.ProvisioningPath) + + // Initial export + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo1). + SubResource("jobs"). + SetHeader("Content-Type", "application/json"). + Body(asJSON(&provisioning.JobSpec{ + Push: &provisioning.ExportJobOptions{ + Folder: "", // export entire instance + Path: "", // no prefix necessary for testing + }, + })). + Do(ctx) + require.NoError(t, result.Error(), "should be able to create export job for first repo") + helper.AwaitJobsWithStates(t, repo1, []string{"success"}) + // Wait for first repository to sync + helper.SyncAndWait(t, repo1, nil) + + printFileTree(t, helper.ProvisioningPath) + // Verify that the first repository has claimed ownership of the dashboards + managedDash1, err := helper.DashboardsV1.Resource.Get(ctx, dashboard1Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, repo1, managedDash1.GetAnnotations()[utils.AnnoKeyManagerIdentity], "dashboard1 should be managed by first repo") + + managedDash2, err := helper.DashboardsV2beta1.Resource.Get(ctx, dashboard2Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, repo1, managedDash2.GetAnnotations()[utils.AnnoKeyManagerIdentity], "dashboard2 should be managed by first repo") + + // Create second repository - enable sync and set different target + + const repo2 = "second-repository" + repo2Path := filepath.Join(helper.ProvisioningPath, repo2) + err = os.MkdirAll(repo2Path, 0750) + require.NoError(t, err, "should be able to create seconrd repository path") + + printFileTree(t, helper.ProvisioningPath) + + createBody2 := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": repo2, + "SyncEnabled": true, + "SyncTarget": "folder", + "Path": repo2Path, + }) + + _, err = helper.Repositories.Resource.Create(ctx, createBody2, metav1.CreateOptions{}) + require.NoError(t, err, "should be able to create second repository") + + // Wait for second repository to sync + helper.SyncAndWait(t, repo2, nil) + + // Validate that folders for both repositories exist + folders, err := helper.Folders.Resource.List(ctx, metav1.ListOptions{}) + require.NoError(t, err, "should be able to list folders") + + var repo1FolderFound, repo2FolderFound bool + for _, folder := range folders.Items { + if folder.GetName() == repo1 { + repo1FolderFound = true + } + if folder.GetName() == repo2 { + repo2FolderFound = true + } + } + require.True(t, repo1FolderFound, "folder for first repository %s should exist after sync", repo1) + require.True(t, repo2FolderFound, "folder for second repository %s should exist after sync", repo2) + + // Create a third dashboard that won't be claimed by the first repo + dashboard3 := helper.LoadYAMLOrJSONFile("exportunifiedtorepository/dashboard-test-v0.yaml") + dashboard3Obj, err := helper.DashboardsV0.Resource.Create(ctx, dashboard3, metav1.CreateOptions{}) + require.NoError(t, err, "should be able to create third dashboard") + dashboard3Name := dashboard3Obj.GetName() + + // Verify dashboard3 is not managed by anyone initially + unmanagedDash3, err := helper.DashboardsV0.Resource.Get(ctx, dashboard3Name, metav1.GetOptions{}) + require.NoError(t, err) + manager, found := unmanagedDash3.GetAnnotations()[utils.AnnoKeyManagerIdentity] + require.True(t, !found || manager == "", "dashboard3 should not be managed initially") + + printFileTree(t, helper.ProvisioningPath) + // Count files in first repo before second export + files1Before, err := countFilesInDir(repo1Path) + require.NoError(t, err) + + // Export from second repository - this should only export the unmanaged dashboard3 + result = helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo2). + SubResource("jobs"). + SetHeader("Content-Type", "application/json"). + Body(asJSON(&provisioning.JobSpec{ + Push: &provisioning.ExportJobOptions{ + Folder: "", // export entire instance + Path: "", // no prefix necessary for testing + }, + })). + Do(ctx) + require.NoError(t, result.Error(), "should be able to create export job for second repo") + + // Wait for second repository export to complete + helper.AwaitJobsWithStates(t, repo2, []string{"success"}) + + // Wait for second repository to sync + helper.SyncAndWait(t, repo1, nil) + helper.SyncAndWait(t, repo2, nil) + + printFileTree(t, helper.ProvisioningPath) + files1After, err := countFilesInDir(repo1Path) + require.NoError(t, err) + + actualNewFiles := files1After - files1Before + require.Equal(t, 0, actualNewFiles, + "second repository should skip managed dashboards and had folder issues with unmanaged dashboard (expected %d new files, got %d)", + 0, actualNewFiles) + + // Verify files in the second repository + files2After, err := countFilesInDir(repo2Path) + require.NoError(t, err) + require.Equal(t, 1, files2After, + "second repository should only export the unmanaged dashboard (expected %d new files, got %d)", + 1, files2After) + + // Verify dashboard1 and dashboard2 are still managed by repo1 (unchanged) + stillManagedDash1, err := helper.DashboardsV1.Resource.Get(ctx, dashboard1Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, repo1, stillManagedDash1.GetAnnotations()[utils.AnnoKeyManagerIdentity], + "dashboard1 should still be managed by first repo") + + stillManagedDash2, err := helper.DashboardsV2beta1.Resource.Get(ctx, dashboard2Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, repo1, stillManagedDash2.GetAnnotations()[utils.AnnoKeyManagerIdentity], + "dashboard2 should still be managed by first repo") + + // Verify dashboard3 is now managed by repo2 + stillManagedDash3, err := helper.DashboardsV0.Resource.Get(ctx, dashboard3Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, repo2, stillManagedDash3.GetAnnotations()[utils.AnnoKeyManagerIdentity], + "dashboard3 should now be managed by second repo") +} + +// Helper function to count files in a directory recursively +func countFilesInDir(rootPath string) (int, error) { + count := 0 + err := filepath.WalkDir(rootPath, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() { + count++ + } + return nil + }) + return count, err +}