From afbb304ff794e9a77cbe69457b5d6c9743a98770 Mon Sep 17 00:00:00 2001 From: Gonzalo Trigueros <242162051+gttrigger@users.noreply.github.com> Date: Mon, 29 Dec 2025 14:00:53 +0100 Subject: [PATCH] provisioning: change job result struct to use a factory method for better error handling/ownership. --- .../apis/provisioning/jobs/delete/worker.go | 19 +- .../provisioning/jobs/delete/worker_test.go | 40 ++-- .../apis/provisioning/jobs/export/folders.go | 15 +- .../provisioning/jobs/export/folders_test.go | 12 +- .../provisioning/jobs/export/resources.go | 27 +-- .../jobs/export/resources_test.go | 36 ++-- .../provisioning/jobs/job_resource_result.go | 93 ++++++++ .../jobs/job_resource_result_test.go | 63 ++++++ .../apis/provisioning/jobs/migrate/clean.go | 20 +- .../provisioning/jobs/migrate/clean_test.go | 38 ++-- .../apis/provisioning/jobs/move/worker.go | 19 +- .../provisioning/jobs/move/worker_test.go | 24 +-- .../apis/provisioning/jobs/progress.go | 34 ++- .../apis/provisioning/jobs/progress_test.go | 72 +++---- .../apis/provisioning/jobs/sync/full.go | 51 ++--- .../apis/provisioning/jobs/sync/full_test.go | 199 +++++++++--------- .../provisioning/jobs/sync/incremental.go | 42 ++-- .../jobs/sync/incremental_test.go | 95 +++++---- 18 files changed, 497 insertions(+), 402 deletions(-) create mode 100644 pkg/registry/apis/provisioning/jobs/job_resource_result.go create mode 100644 pkg/registry/apis/provisioning/jobs/job_resource_result_test.go diff --git a/pkg/registry/apis/provisioning/jobs/delete/worker.go b/pkg/registry/apis/provisioning/jobs/delete/worker.go index e06ae627917..ae1e7c924a6 100644 --- a/pkg/registry/apis/provisioning/jobs/delete/worker.go +++ b/pkg/registry/apis/provisioning/jobs/delete/worker.go @@ -131,15 +131,12 @@ func (w *Worker) Process(ctx context.Context, repo repository.Repository, job pr func (w *Worker) deleteFiles(ctx context.Context, rw repository.ReaderWriter, progress jobs.JobProgressRecorder, opts provisioning.DeleteJobOptions, paths ...string) error { for _, path := range paths { - result := jobs.JobResourceResult{ - Path: path, - Action: repository.FileActionDeleted, - } - progress.SetMessage(ctx, "Deleting "+path) + var resultErr error if err := rw.Delete(ctx, path, opts.Ref, "Delete "+path); err != nil { - result.Error = fmt.Errorf("deleting file %s: %w", path, err) + resultErr = fmt.Errorf("deleting file %s: %w", path, err) } + result := jobs.NewJobResourceResultWithoutKind(path, repository.FileActionDeleted, resultErr) progress.Record(ctx, result) if err := progress.TooManyErrors(); err != nil { return err @@ -163,12 +160,6 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read resolvedPaths := make([]string, 0, len(resources)) for _, resource := range resources { - result := jobs.JobResourceResult{ - Name: resource.Name, - Group: resource.Group, - Action: repository.FileActionDeleted, // Will be used for deletion later - } - gvk := schema.GroupVersionKind{ Group: resource.Group, Kind: resource.Kind, @@ -178,7 +169,8 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read progress.SetMessage(ctx, fmt.Sprintf("Finding path for resource %s/%s/%s", resource.Group, resource.Kind, resource.Name)) resourcePath, err := repositoryResources.FindResourcePath(ctx, resource.Name, gvk) if err != nil { - result.Error = fmt.Errorf("find path for resource %s/%s/%s: %w", resource.Group, resource.Kind, resource.Name, err) + resultErr := fmt.Errorf("find path for resource %s/%s/%s: %w", resource.Group, resource.Kind, resource.Name, err) + result := jobs.NewJobResourceResult(resource.Name, resource.Group, resource.Kind, "", repository.FileActionDeleted, resultErr) progress.Record(ctx, result) // Continue with next resource instead of failing fast if err := progress.TooManyErrors(); err != nil { @@ -187,7 +179,6 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read continue } - result.Path = resourcePath resolvedPaths = append(resolvedPaths, resourcePath) } diff --git a/pkg/registry/apis/provisioning/jobs/delete/worker_test.go b/pkg/registry/apis/provisioning/jobs/delete/worker_test.go index 07cc855f194..3de4632890a 100644 --- a/pkg/registry/apis/provisioning/jobs/delete/worker_test.go +++ b/pkg/registry/apis/provisioning/jobs/delete/worker_test.go @@ -184,10 +184,10 @@ func TestDeleteWorker_ProcessDeleteFilesSuccess(t *testing.T) { mockRepo.On("Delete", mock.Anything, "test/path2", "main", "Delete test/path2").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "test/path1" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path2" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "test/path2" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, nil, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) @@ -224,7 +224,7 @@ func TestDeleteWorker_ProcessDeleteFilesWithError(t *testing.T) { mockRepo.On("Delete", mock.Anything, "test/path1", "main", "Delete test/path1").Return(deleteError) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionDeleted && errors.Is(result.Error, deleteError) + return result.Path() == "test/path1" && result.Action() == repository.FileActionDeleted && errors.Is(result.Error(), deleteError) })).Return() mockProgress.On("TooManyErrors").Return(errors.New("too many errors")) @@ -263,7 +263,7 @@ func TestDeleteWorker_ProcessWithSyncWorker(t *testing.T) { mockRepo.On("Delete", mock.Anything, "test/path", "", "Delete test/path").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "test/path" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("ResetResults").Return() @@ -371,7 +371,7 @@ func TestDeleteWorker_deleteFiles(t *testing.T) { mockRepo.On("Delete", mock.Anything, path, "main", "Delete "+path).Return(tt.deleteResults[i]).Once() mockProgress.On("SetMessage", mock.Anything, "Deleting "+path).Return().Once() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == path && result.Action == repository.FileActionDeleted + return result.Path() == path && result.Action() == repository.FileActionDeleted })).Return().Once() if tt.tooManyErrors != nil && i == 0 { @@ -469,13 +469,13 @@ func TestDeleteWorker_ProcessWithResourceRefs(t *testing.T) { // Mock progress records mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "test/path1" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboards/test-dashboard.json" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "dashboards/test-dashboard.json" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "folders/test-folder.json" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "folders/test-folder.json" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, mockResourcesFactory, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) @@ -534,7 +534,7 @@ func TestDeleteWorker_ProcessResourceRefsOnly(t *testing.T) { mockRepo.On("Delete", mock.Anything, "dashboards/test-dashboard.json", "main", "Delete dashboards/test-dashboard.json").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboards/test-dashboard.json" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "dashboards/test-dashboard.json" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, mockResourcesFactory, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) @@ -587,10 +587,10 @@ func TestDeleteWorker_ProcessResourceResolutionError(t *testing.T) { // Expect error to be recorded, not thrown mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "nonexistent-dashboard" && - result.Group == "dashboard.grafana.app" && - result.Action == repository.FileActionDeleted && - result.Error != nil + return result.Name() == "nonexistent-dashboard" && + result.Group() == "dashboard.grafana.app" && + result.Action() == repository.FileActionDeleted && + result.Error() != nil })).Return() mockProgress.On("TooManyErrors").Return(nil) @@ -727,7 +727,7 @@ func TestDeleteWorker_ProcessResourceResolutionTooManyErrors(t *testing.T) { // Mock recording error and TooManyErrors returning error mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "nonexistent-dashboard" && result.Error != nil + return result.Name() == "nonexistent-dashboard" && result.Error() != nil })).Return() mockProgress.On("TooManyErrors").Return(errors.New("too many errors")) @@ -810,7 +810,7 @@ func TestDeleteWorker_ProcessMixedResourcesWithPartialFailure(t *testing.T) { mockProgress.On("Complete", mock.Anything, mock.Anything).Return(v0alpha1.JobStatus{}) // Record the error for the failed resource mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "nonexistent-dashboard" && result.Error != nil + return result.Name() == "nonexistent-dashboard" && result.Error() != nil })).Return() // Allow continuing after error @@ -822,10 +822,10 @@ func TestDeleteWorker_ProcessMixedResourcesWithPartialFailure(t *testing.T) { // Record successful deletions mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboards/valid-dashboard.json" && result.Error == nil + return result.Path() == "dashboards/valid-dashboard.json" && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "folders/valid-folder.json" && result.Error == nil + return result.Path() == "folders/valid-folder.json" && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, mockResourcesFactory, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) @@ -910,20 +910,20 @@ func TestDeleteWorker_ProcessWithPathDeduplication(t *testing.T) { mockProgress.On("SetMessage", mock.Anything, "Deleting dashboards/test-dashboard.json").Return() mockRepo.On("Delete", mock.Anything, "dashboards/test-dashboard.json", "main", "Delete dashboards/test-dashboard.json").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboards/test-dashboard.json" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "dashboards/test-dashboard.json" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("TooManyErrors").Return(nil) mockProgress.On("SetMessage", mock.Anything, "Deleting folders/test-folder/").Return() mockRepo.On("Delete", mock.Anything, "folders/test-folder/", "main", "Delete folders/test-folder/").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "folders/test-folder/" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "folders/test-folder/" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() mockProgress.On("SetMessage", mock.Anything, "Deleting dashboards/unique-dashboard.json").Return() mockRepo.On("Delete", mock.Anything, "dashboards/unique-dashboard.json", "main", "Delete dashboards/unique-dashboard.json").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboards/unique-dashboard.json" && result.Action == repository.FileActionDeleted && result.Error == nil + return result.Path() == "dashboards/unique-dashboard.json" && result.Action() == repository.FileActionDeleted && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, mockResourcesFactory, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) diff --git a/pkg/registry/apis/provisioning/jobs/export/folders.go b/pkg/registry/apis/provisioning/jobs/export/folders.go index 4b11f03172a..2437bf43eec 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders.go @@ -44,22 +44,17 @@ func ExportFolders(ctx context.Context, repoName string, options provisioning.Ex progress.SetMessage(ctx, "write folders to repository") err := repositoryResources.EnsureFolderTreeExists(ctx, options.Branch, options.Path, tree, func(folder resources.Folder, created bool, err error) error { - result := jobs.JobResourceResult{ - Action: repository.FileActionCreated, - Name: folder.ID, - Group: resources.FolderResource.Group, - Kind: resources.FolderKind.Kind, - Path: folder.Path, - } + action := repository.FileActionCreated + resultErr := error(nil) if err != nil { - result.Error = fmt.Errorf("creating folder %s at path %s: %w", folder.ID, folder.Path, err) + resultErr = fmt.Errorf("creating folder %s at path %s: %w", folder.ID, folder.Path, err) } if !created { - result.Action = repository.FileActionIgnored + action = repository.FileActionIgnored } - + result := jobs.NewJobResourceResult(folder.ID, resources.FolderResource.Group, resources.FolderKind.Kind, folder.Path, action, resultErr) progress.Record(ctx, result) if err := progress.TooManyErrors(); err != nil { return err diff --git a/pkg/registry/apis/provisioning/jobs/export/folders_test.go b/pkg/registry/apis/provisioning/jobs/export/folders_test.go index 4790f637a07..8f304b366ad 100644 --- a/pkg/registry/apis/provisioning/jobs/export/folders_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/folders_test.go @@ -128,10 +128,10 @@ 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 == "folder-1-uid" && result.Action == repository.FileActionCreated + return result.Name() == "folder-1-uid" && result.Action() == repository.FileActionCreated })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "folder-2-uid" && result.Action == repository.FileActionCreated + return result.Name() == "folder-2-uid" && result.Action() == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) @@ -189,10 +189,10 @@ 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 == "folder-1-uid" && result.Action == repository.FileActionIgnored && result.Error != nil && result.Error.Error() == "creating folder folder-1-uid at path grafana/folder-1: didn't work" + return result.Name() == "folder-1-uid" && result.Action() == repository.FileActionIgnored && result.Error() != nil && result.Error().Error() == "creating folder folder-1-uid at path grafana/folder-1: didn't work" })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "folder-2-uid" && result.Action == repository.FileActionCreated + return result.Name() == "folder-2-uid" && result.Action() == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) @@ -298,10 +298,10 @@ 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-folder" && 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-folder" && result.Action == repository.FileActionCreated + return result.Name() == "child-folder" && result.Action() == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) diff --git a/pkg/registry/apis/provisioning/jobs/export/resources.go b/pkg/registry/apis/provisioning/jobs/export/resources.go index f1b8cd078de..f2e5458b1ca 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources.go @@ -104,18 +104,17 @@ func exportResource(ctx context.Context, // this will work well enough for now, but needs to be revisted as we have a bigger mix of active versions return resources.ForEach(ctx, client, func(item *unstructured.Unstructured) (err error) { gvk := item.GroupVersionKind() - result := jobs.JobResourceResult{ - Name: item.GetName(), - Group: gvk.Group, - Kind: gvk.Kind, - Action: repository.FileActionCreated, - } + name := item.GetName() + action := repository.FileActionCreated + path := "" + result_err := error(nil) // 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("extracting meta accessor for resource %s: %w", result.Name, err) + action = repository.FileActionIgnored + meta_error := fmt.Errorf("extracting meta accessor for resource %s: %w", name, err) + result := jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, path, action, meta_error) progress.Record(ctx, result) return nil } @@ -123,7 +122,8 @@ func exportResource(ctx context.Context, manager, _ := meta.GetManagerProperties() // Skip if already managed by any manager (repository, file provisioning, etc.) if manager.Identity != "" { - result.Action = repository.FileActionIgnored + action = repository.FileActionIgnored + result := jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, path, action, result_err) progress.Record(ctx, result) return nil } @@ -133,19 +133,20 @@ func exportResource(ctx context.Context, } if err == nil { - result.Path, err = repositoryResources.WriteResourceFileFromObject(ctx, item, resources.WriteOptions{ + path, err = repositoryResources.WriteResourceFileFromObject(ctx, item, resources.WriteOptions{ Path: options.Path, Ref: options.Branch, }) } if errors.Is(err, resources.ErrAlreadyInRepository) { - result.Action = repository.FileActionIgnored + action = repository.FileActionIgnored } else if err != nil { - result.Action = repository.FileActionIgnored - result.Error = fmt.Errorf("writing resource file for %s: %w", result.Name, err) + action = repository.FileActionIgnored + result_err = fmt.Errorf("writing resource file for %s: %w", name, err) } + result := jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, path, action, result_err) progress.Record(ctx, result) if err := progress.TooManyErrors(); err != nil { return err diff --git a/pkg/registry/apis/provisioning/jobs/export/resources_test.go b/pkg/registry/apis/provisioning/jobs/export/resources_test.go index 36bf304cc70..8972cc39948 100644 --- a/pkg/registry/apis/provisioning/jobs/export/resources_test.go +++ b/pkg/registry/apis/provisioning/jobs/export/resources_test.go @@ -88,10 +88,10 @@ func TestExportResources_Dashboards_Success(t *testing.T) { 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 == "dashboard-1" && result.Action == repository.FileActionCreated + return result.Name() == "dashboard-1" && result.Action() == repository.FileActionCreated })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "dashboard-2" && result.Action == repository.FileActionCreated + return result.Name() == "dashboard-2" && result.Action() == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) @@ -141,10 +141,10 @@ func TestExportResources_Dashboards_WithErrors(t *testing.T) { 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 == "dashboard-1" && result.Action == repository.FileActionIgnored && result.Error != nil && result.Error.Error() == "writing resource file for dashboard-1: failed to export dashboard" + return result.Name() == "dashboard-1" && result.Action() == repository.FileActionIgnored && result.Error() != nil && result.Error().Error() == "writing resource file for dashboard-1: failed to export dashboard" })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "dashboard-2" && result.Action == repository.FileActionCreated + return result.Name() == "dashboard-2" && result.Action() == repository.FileActionCreated })).Return() progress.On("TooManyErrors").Return(nil) progress.On("TooManyErrors").Return(nil) @@ -179,7 +179,7 @@ func TestExportResources_Dashboards_TooManyErrors(t *testing.T) { 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 == "dashboard-1" && result.Action == repository.FileActionIgnored && result.Error != nil && result.Error.Error() == "writing resource file for dashboard-1: failed to export dashboard" + return result.Name() == "dashboard-1" && result.Action() == repository.FileActionIgnored && result.Error() != nil && result.Error().Error() == "writing resource file for dashboard-1: failed to export dashboard" })).Return() progress.On("TooManyErrors").Return(fmt.Errorf("too many errors encountered")) } @@ -209,7 +209,7 @@ func TestExportResources_Dashboards_IgnoresExisting(t *testing.T) { 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 == "existing-dashboard" && result.Action == repository.FileActionIgnored + return result.Name() == "existing-dashboard" && result.Action() == repository.FileActionIgnored })).Return() progress.On("TooManyErrors").Return(nil) } @@ -256,7 +256,7 @@ func TestExportResources_Dashboards_SavedVersion(t *testing.T) { 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 == "existing-dashboard" && result.Action == repository.FileActionIgnored + return result.Name() == "existing-dashboard" && result.Action() == repository.FileActionIgnored })).Return() progress.On("TooManyErrors").Return(nil) } @@ -320,9 +320,9 @@ func TestExportResources_Dashboards_FailedConversionNoStoredVersion(t *testing.T 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 == "dashboard-no-stored-version" && - result.Action == repository.FileActionIgnored && - result.Error != nil + return result.Name() == "dashboard-no-stored-version" && + result.Action() == repository.FileActionIgnored && + result.Error() != nil })).Return() progress.On("TooManyErrors").Return(nil) } @@ -469,20 +469,20 @@ func TestExportResources_Dashboards_Versions(t *testing.T) { progress.On("SetMessage", mock.Anything, "export dashboards").Return() if tt.expectSuccess { progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == tt.dashboardName && result.Action == repository.FileActionCreated + return result.Name() == tt.dashboardName && result.Action() == repository.FileActionCreated })).Return() } else { progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - if result.Name != tt.dashboardName { + if result.Name() != tt.dashboardName { return false } - if result.Action != repository.FileActionIgnored { + if result.Action() != repository.FileActionIgnored { return false } - if result.Error == nil { + if result.Error() == nil { return false } - return result.Error.Error() == tt.expectedError + return result.Error().Error() == tt.expectedError })).Return() } progress.On("TooManyErrors").Return(nil) @@ -540,7 +540,7 @@ func TestExportResources_Dashboards_SkipsManagedResources(t *testing.T) { 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 result.Name() == "managed-dashboard" && result.Action() == repository.FileActionIgnored })).Return() progress.On("TooManyErrors").Return(nil).Maybe() } @@ -608,8 +608,8 @@ func TestExportResources_Dashboards_MultipleVersions(t *testing.T) { 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 == "v2alpha-dashboard" || result.Name == "v2beta-dashboard" || result.Name == "v3-dashboard") && - result.Action == repository.FileActionCreated + return (result.Name() == "v2alpha-dashboard" || result.Name() == "v2beta-dashboard" || result.Name() == "v3-dashboard") && + result.Action() == repository.FileActionCreated })).Return().Times(3) progress.On("TooManyErrors").Return(nil).Times(3) } diff --git a/pkg/registry/apis/provisioning/jobs/job_resource_result.go b/pkg/registry/apis/provisioning/jobs/job_resource_result.go new file mode 100644 index 00000000000..5122f4c50a9 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/job_resource_result.go @@ -0,0 +1,93 @@ +package jobs + +import ( + "github.com/grafana/grafana/apps/provisioning/pkg/repository" +) + +// JobResourceResult represents the result of a resource operation in a job. +type JobResourceResult struct { + name string + group string + kind string + path string + action repository.FileAction + err error + warning error +} + +// NewJobResourceResultWithoutKind creates a new JobResourceResult without a name, group, and kind. +// This is used for operations that cannot be references to a Grafana resource or when the resource is not yet known. +func NewJobResourceResultWithoutKind(path string, action repository.FileAction, err error) JobResourceResult { + return NewJobResourceResult("", "", "", path, action, err) +} + +// Builder for a skipped resource operation. It takes care of assign the right action and job error type. +func NewSkippedJobResourceResult(name, group, kind, path string, err error) JobResourceResult { + return JobResourceResult{ + name: name, + group: group, + kind: kind, + path: path, + action: repository.FileActionIgnored, + warning: err, + err: nil, + } +} + +func isWarningError(err error) bool { + return false +} + +// newJobResourceResult creates a new JobResourceResult. +// err is the error associated with the resource operation (can be nil). +func NewJobResourceResult(name, group, kind, path string, action repository.FileAction, err error) JobResourceResult { + result := JobResourceResult{ + name: name, + group: group, + kind: kind, + path: path, + action: action, + } + if isWarningError(err) { + result.warning = err + } else { + result.err = err + } + + return result +} + +// Name returns the name of the resource. +func (r JobResourceResult) Name() string { + return r.name +} + +// Group returns the group of the resource. +func (r JobResourceResult) Group() string { + return r.group +} + +// Kind returns the kind of the resource. +func (r JobResourceResult) Kind() string { + return r.kind +} + +// Path returns the path of the resource. +func (r JobResourceResult) Path() string { + return r.path +} + +// Action returns the action performed on the resource. +func (r JobResourceResult) Action() repository.FileAction { + return r.action +} + +// Error returns the error associated with the resource operation. +func (r JobResourceResult) Error() error { + return r.err +} + +// Warning returns the warning associated with the resource operation. +func (r JobResourceResult) Warning() error { + return r.warning +} diff --git a/pkg/registry/apis/provisioning/jobs/job_resource_result_test.go b/pkg/registry/apis/provisioning/jobs/job_resource_result_test.go new file mode 100644 index 00000000000..97e4f68bcd7 --- /dev/null +++ b/pkg/registry/apis/provisioning/jobs/job_resource_result_test.go @@ -0,0 +1,63 @@ +package jobs + +import ( + "errors" + "testing" + + "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/stretchr/testify/assert" +) + +func TestNewSkippedJobResourceResult(t *testing.T) { + name := "test-resource" + group := "test-group" + kind := "test-kind" + path := "/test/path" + err := errors.New("skip reason") + + result := NewSkippedJobResourceResult(name, group, kind, path, err) + + assert.Equal(t, name, result.Name()) + assert.Equal(t, group, result.Group()) + assert.Equal(t, kind, result.Kind()) + assert.Equal(t, path, result.Path()) + assert.Equal(t, repository.FileActionIgnored, result.Action()) + assert.Equal(t, err, result.Warning()) + assert.Nil(t, result.Error()) +} + +func TestNewJobResourceResult_WithError(t *testing.T) { + name := "test-resource" + group := "test-group" + kind := "test-kind" + path := "/test/path" + action := repository.FileActionCreated + err := errors.New("operation failed") + + result := NewJobResourceResult(name, group, kind, path, action, err) + + assert.Equal(t, name, result.Name()) + assert.Equal(t, group, result.Group()) + assert.Equal(t, kind, result.Kind()) + assert.Equal(t, path, result.Path()) + assert.Equal(t, action, result.Action()) + assert.NotNil(t, result.Error()) + assert.Equal(t, err, result.Error()) +} + +func TestNewJobResourceResult_WithoutError(t *testing.T) { + name := "test-resource" + group := "test-group" + kind := "test-kind" + path := "/test/path" + action := repository.FileActionUpdated + + result := NewJobResourceResult(name, group, kind, path, action, nil) + + assert.Equal(t, name, result.Name()) + assert.Equal(t, group, result.Group()) + assert.Equal(t, kind, result.Kind()) + assert.Equal(t, path, result.Path()) + assert.Equal(t, action, result.Action()) + assert.Nil(t, result.Error()) +} diff --git a/pkg/registry/apis/provisioning/jobs/migrate/clean.go b/pkg/registry/apis/provisioning/jobs/migrate/clean.go index c05be914bf0..b77c07947e9 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/clean.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/clean.go @@ -40,17 +40,15 @@ func (c *namespaceCleaner) Clean(ctx context.Context, namespace string, progress } if err = resources.ForEach(ctx, client, func(item *unstructured.Unstructured) error { - result := jobs.JobResourceResult{ - Name: item.GetName(), - Kind: item.GetKind(), - Group: item.GroupVersionKind().Group, - Action: repository.FileActionDeleted, - } + name := item.GetName() + kind := item.GetKind() + group := item.GroupVersionKind().Group // Skip provisioned resources - only delete unprovisioned (unmanaged) resources meta, err := utils.MetaAccessor(item) if err != nil { - result.Error = fmt.Errorf("extracting meta accessor for resource %s: %w", result.Name, err) + resultErr := fmt.Errorf("extracting meta accessor for resource %s: %w", name, err) + result := jobs.NewJobResourceResult(name, group, kind, "", repository.FileActionDeleted, resultErr) progress.Record(ctx, result) return nil // Continue with next resource } @@ -58,18 +56,20 @@ func (c *namespaceCleaner) Clean(ctx context.Context, namespace string, progress manager, _ := meta.GetManagerProperties() // Skip if resource is managed by any provisioning system if manager.Identity != "" { - result.Action = repository.FileActionIgnored + result := jobs.NewJobResourceResult(name, group, kind, "", repository.FileActionIgnored, nil) progress.Record(ctx, result) return nil // Skip this resource } // Deletion works by name, so we can use any client regardless of version - if err := client.Delete(ctx, item.GetName(), metav1.DeleteOptions{}); err != nil { - result.Error = fmt.Errorf("deleting resource %s/%s %s: %w", result.Group, result.Kind, result.Name, err) + if err := client.Delete(ctx, name, metav1.DeleteOptions{}); err != nil { + resultErr := fmt.Errorf("deleting resource %s/%s %s: %w", group, kind, name, err) + result := jobs.NewJobResourceResult(name, group, kind, "", repository.FileActionDeleted, resultErr) progress.Record(ctx, result) return fmt.Errorf("delete resource: %w", err) } + result := jobs.NewJobResourceResult(name, group, kind, "", repository.FileActionDeleted, nil) progress.Record(ctx, result) return nil }); err != nil { diff --git a/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go b/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go index 1a454151a87..6540e0feda7 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/clean_test.go @@ -124,10 +124,10 @@ func TestNamespaceCleaner_Clean(t *testing.T) { 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.FileActionDeleted && - result.Name == "test-folder" && - result.Error != nil && - result.Error.Error() == "deleting resource folder.grafana.app/Folder test-folder: delete failed" + return result.Action() == repository.FileActionDeleted && + result.Name() == "test-folder" && + result.Error() != nil && + result.Error().Error() == "deleting resource folder.grafana.app/Folder test-folder: delete failed" })).Return() err := cleaner.Clean(context.Background(), "test-namespace", progress) @@ -194,21 +194,21 @@ func TestNamespaceCleaner_Clean(t *testing.T) { // Expect only unprovisioned resources to be deleted (2 deletions) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Name == "unprovisioned-folder" && - result.Error == nil + return result.Action() == repository.FileActionDeleted && + result.Name() == "unprovisioned-folder" && + result.Error() == nil })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Name == "unprovisioned-dashboard" && - result.Error == nil + return result.Action() == repository.FileActionDeleted && + result.Name() == "unprovisioned-dashboard" && + result.Error() == nil })).Return() // Expect provisioned resource to be ignored (1 ignore) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionIgnored && - result.Name == "provisioned-dashboard" && - result.Error == nil + return result.Action() == repository.FileActionIgnored && + result.Name() == "provisioned-dashboard" && + result.Error() == nil })).Return() err := cleaner.Clean(context.Background(), "test-namespace", progress) @@ -267,14 +267,14 @@ func TestNamespaceCleaner_Clean(t *testing.T) { // Expect both resources to be ignored (no deletions) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionIgnored && - result.Name == "repo-managed-dashboard" && - result.Error == nil + return result.Action() == repository.FileActionIgnored && + result.Name() == "repo-managed-dashboard" && + result.Error() == nil })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionIgnored && - result.Name == "file-provisioned-folder" && - result.Error == nil + return result.Action() == repository.FileActionIgnored && + result.Name() == "file-provisioned-folder" && + result.Error() == nil })).Return() err := cleaner.Clean(context.Background(), "test-namespace", progress) diff --git a/pkg/registry/apis/provisioning/jobs/move/worker.go b/pkg/registry/apis/provisioning/jobs/move/worker.go index 1ec3d817b3e..45b0594049f 100644 --- a/pkg/registry/apis/provisioning/jobs/move/worker.go +++ b/pkg/registry/apis/provisioning/jobs/move/worker.go @@ -142,18 +142,15 @@ func (w *Worker) Process(ctx context.Context, repo repository.Repository, job pr func (w *Worker) moveFiles(ctx context.Context, rw repository.ReaderWriter, progress jobs.JobProgressRecorder, opts provisioning.MoveJobOptions, paths ...string) error { for _, path := range paths { - result := jobs.JobResourceResult{ - Path: path, - Action: repository.FileActionRenamed, - } - // Construct the target path by combining the job's target path with the file/folder name targetPath := w.constructTargetPath(opts.TargetPath, path) progress.SetMessage(ctx, "Moving "+path+" to "+targetPath) + var resultErr error if err := rw.Move(ctx, path, targetPath, opts.Ref, "Move "+path+" to "+targetPath); err != nil { - result.Error = fmt.Errorf("moving file %s to %s: %w", path, targetPath, err) + resultErr = fmt.Errorf("moving file %s to %s: %w", path, targetPath, err) } + result := jobs.NewJobResourceResultWithoutKind(path, repository.FileActionRenamed, resultErr) progress.Record(ctx, result) if err := progress.TooManyErrors(); err != nil { return err @@ -191,12 +188,6 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read resolvedPaths := make([]string, 0, len(resources)) for _, resource := range resources { - result := jobs.JobResourceResult{ - Name: resource.Name, - Group: resource.Group, - Action: repository.FileActionRenamed, // Will be used for move later - } - gvk := schema.GroupVersionKind{ Group: resource.Group, Kind: resource.Kind, @@ -206,7 +197,8 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read progress.SetMessage(ctx, fmt.Sprintf("Finding path for resource %s/%s/%s", resource.Group, resource.Kind, resource.Name)) resourcePath, err := repositoryResources.FindResourcePath(ctx, resource.Name, gvk) if err != nil { - result.Error = fmt.Errorf("find path for resource %s/%s/%s: %w", resource.Group, resource.Kind, resource.Name, err) + resultErr := fmt.Errorf("find path for resource %s/%s/%s: %w", resource.Group, resource.Kind, resource.Name, err) + result := jobs.NewJobResourceResult(resource.Name, resource.Group, resource.Kind, "", repository.FileActionRenamed, resultErr) progress.Record(ctx, result) // Continue with next resource instead of failing fast if err := progress.TooManyErrors(); err != nil { @@ -215,7 +207,6 @@ func (w *Worker) resolveResourcesToPaths(ctx context.Context, rw repository.Read continue } - result.Path = resourcePath resolvedPaths = append(resolvedPaths, resourcePath) } diff --git a/pkg/registry/apis/provisioning/jobs/move/worker_test.go b/pkg/registry/apis/provisioning/jobs/move/worker_test.go index f5129350e6e..8e85ef12b0d 100644 --- a/pkg/registry/apis/provisioning/jobs/move/worker_test.go +++ b/pkg/registry/apis/provisioning/jobs/move/worker_test.go @@ -220,10 +220,10 @@ func TestMoveWorker_ProcessMoveFilesSuccess(t *testing.T) { mockRepo.On("Move", mock.Anything, "test/path2", "new/location/path2", "main", "Move test/path2 to new/location/path2").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionRenamed && result.Error == nil + return result.Path() == "test/path1" && result.Action() == repository.FileActionRenamed && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path2" && result.Action == repository.FileActionRenamed && result.Error == nil + return result.Path() == "test/path2" && result.Action() == repository.FileActionRenamed && result.Error() == nil })).Return() worker := NewWorker(nil, mockWrapFn.Execute, nil, jobs.RegisterJobMetrics(prometheus.NewPedanticRegistry())) @@ -261,7 +261,7 @@ func TestMoveWorker_ProcessMoveFilesWithError(t *testing.T) { mockRepo.On("Move", mock.Anything, "test/path1", "new/location/path1", "main", "Move test/path1 to new/location/path1").Return(moveError) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionRenamed && errors.Is(result.Error, moveError) + return result.Path() == "test/path1" && result.Action() == repository.FileActionRenamed && errors.Is(result.Error(), moveError) })).Return() mockProgress.On("TooManyErrors").Return(errors.New("too many errors")) @@ -300,7 +300,7 @@ func TestMoveWorker_ProcessWithSyncWorker(t *testing.T) { mockRepo.On("Move", mock.Anything, "test/path", "new/location/path", "", "Move test/path to new/location/path").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path" && result.Action == repository.FileActionRenamed && result.Error == nil + return result.Path() == "test/path" && result.Action() == repository.FileActionRenamed && result.Error() == nil })).Return() mockProgress.On("ResetResults").Return() @@ -422,7 +422,7 @@ func TestMoveWorker_moveFiles(t *testing.T) { mockRepo.On("Move", mock.Anything, path, expectedTarget, "main", "Move "+path+" to "+expectedTarget).Return(tt.moveResults[i]).Once() mockProgress.On("SetMessage", mock.Anything, "Moving "+path+" to "+expectedTarget).Return().Once() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == path && result.Action == repository.FileActionRenamed + return result.Path() == path && result.Action() == repository.FileActionRenamed })).Return().Once() if tt.tooManyErrors != nil && i == 0 { @@ -545,10 +545,10 @@ func TestMoveWorker_ProcessWithResourceReferences(t *testing.T) { mockRepo.On("Move", mock.Anything, "dashboard/file.yaml", "new/location/file.yaml", "", "Move dashboard/file.yaml to new/location/file.yaml").Return(nil) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "test/path1" && result.Action == repository.FileActionRenamed && result.Error == nil + return result.Path() == "test/path1" && result.Action() == repository.FileActionRenamed && result.Error() == nil })).Return() mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Path == "dashboard/file.yaml" && result.Action == repository.FileActionRenamed && result.Error == nil + return result.Path() == "dashboard/file.yaml" && result.Action() == repository.FileActionRenamed && result.Error() == nil })).Return() // Add expectations for sync worker (called when ref is empty) @@ -607,9 +607,9 @@ func TestMoveWorker_ProcessResourceReferencesError(t *testing.T) { }).Return("", resourceError) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == "non-existent-uid" && result.Group == "dashboard.grafana.app" && - result.Action == repository.FileActionRenamed && - result.Error != nil && result.Error.Error() == "find path for resource dashboard.grafana.app/Dashboard/non-existent-uid: resource not found" + return result.Name() == "non-existent-uid" && result.Group() == "dashboard.grafana.app" && + result.Action() == repository.FileActionRenamed && + result.Error() != nil && result.Error().Error() == "find path for resource dashboard.grafana.app/Dashboard/non-existent-uid: resource not found" })).Return() // Add expectations for sync worker (called when ref is empty) @@ -766,8 +766,8 @@ func TestMoveWorker_resolveResourcesToPaths(t *testing.T) { } else if err, ok := tt.resourceErrors[resource.Name]; ok { mockRepoResources.On("FindResourcePath", mock.Anything, resource.Name, gvk).Return("", err) mockProgress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Name == resource.Name && result.Group == resource.Group && - result.Action == repository.FileActionRenamed && result.Error != nil + return result.Name() == resource.Name && result.Group() == resource.Group && + result.Action() == repository.FileActionRenamed && result.Error() != nil })).Return() if tt.tooManyErrors != nil { mockProgress.On("TooManyErrors").Return(tt.tooManyErrors) diff --git a/pkg/registry/apis/provisioning/jobs/progress.go b/pkg/registry/apis/provisioning/jobs/progress.go index 2cb9dc9ddcf..5c7d308c48b 100644 --- a/pkg/registry/apis/provisioning/jobs/progress.go +++ b/pkg/registry/apis/provisioning/jobs/progress.go @@ -34,16 +34,6 @@ func maybeNotifyProgress(threshold time.Duration, fn ProgressFn) ProgressFn { } // FIXME: ProgressRecorder should be initialized in the queue -type JobResourceResult struct { - Name string - Group string - Kind string - Path string - Action repository.FileAction - Error error - Warning error -} - type jobProgressRecorder struct { mu sync.RWMutex started time.Time @@ -81,11 +71,11 @@ func (r *jobProgressRecorder) Record(ctx context.Context, result JobResourceResu r.mu.Lock() r.resultCount++ - if result.Error != nil { + if result.Error() != nil { shouldLogError = true - logErr = result.Error + logErr = result.Error() if len(r.errors) < 20 { - r.errors = append(r.errors, result.Error.Error()) + r.errors = append(r.errors, result.Error().Error()) } r.errorCount++ } @@ -93,7 +83,7 @@ func (r *jobProgressRecorder) Record(ctx context.Context, result JobResourceResu r.updateSummary(result) r.mu.Unlock() - logger := logging.FromContext(ctx).With("path", result.Path, "group", result.Group, "kind", result.Kind, "action", result.Action, "name", result.Name) + logger := logging.FromContext(ctx).With("path", result.Path(), "group", result.Group(), "kind", result.Kind(), "action", result.Action(), "name", result.Name()) if shouldLogError { logger.Error("job resource operation failed", "err", logErr) } else { @@ -180,26 +170,26 @@ func (r *jobProgressRecorder) summary() []*provisioning.JobResourceSummary { func (r *jobProgressRecorder) updateSummary(result JobResourceResult) { // Note: This method is called from Record() which already holds the lock - key := result.Group + ":" + result.Kind + key := result.Group() + ":" + result.Kind() summary, exists := r.summaries[key] if !exists { summary = &provisioning.JobResourceSummary{ - Group: result.Group, - Kind: result.Kind, + Group: result.Group(), + Kind: result.Kind(), } r.summaries[key] = summary } - if result.Error != nil { - errorMsg := fmt.Sprintf("%s (file: %s, name: %s, action: %s)", result.Error.Error(), result.Path, result.Name, result.Action) + if result.Error() != nil { + errorMsg := fmt.Sprintf("%s (file: %s, name: %s, action: %s)", result.Error().Error(), result.Path(), result.Name(), result.Action()) summary.Errors = append(summary.Errors, errorMsg) summary.Error++ - } else if result.Warning != nil { - warningMsg := fmt.Sprintf("%s (file: %s, name: %s, action: %s)", result.Warning.Error(), result.Path, result.Name, result.Action) + } else if result.Warning() != nil { + warningMsg := fmt.Sprintf("%s (file: %s, name: %s, action: %s)", result.Warning().Error(), result.Path(), result.Name(), result.Action()) summary.Warnings = append(summary.Warnings, warningMsg) summary.Warning++ } else { - switch result.Action { + switch result.Action() { case repository.FileActionDeleted: summary.Delete++ case repository.FileActionUpdated: diff --git a/pkg/registry/apis/provisioning/jobs/progress_test.go b/pkg/registry/apis/provisioning/jobs/progress_test.go index 7e849491bbe..adca2122eec 100644 --- a/pkg/registry/apis/provisioning/jobs/progress_test.go +++ b/pkg/registry/apis/provisioning/jobs/progress_test.go @@ -98,36 +98,36 @@ func TestJobProgressRecorderWarningStatus(t *testing.T) { // Record a result with a warning warningErr := errors.New("deprecated API used") result := JobResourceResult{ - Name: "test-resource", - Group: "test.grafana.app", - Kind: "Dashboard", - Path: "dashboards/test.json", - Action: repository.FileActionUpdated, - Warning: warningErr, + name: "test-resource", + group: "test.grafana.app", + kind: "Dashboard", + path: "dashboards/test.json", + action: repository.FileActionUpdated, + warning: warningErr, } recorder.Record(ctx, result) // Record another result with a different warning warningErr2 := errors.New("missing optional field") result2 := JobResourceResult{ - Name: "test-resource-2", - Group: "test.grafana.app", - Kind: "Dashboard", - Path: "dashboards/test2.json", - Action: repository.FileActionCreated, - Warning: warningErr2, + name: "test-resource-2", + group: "test.grafana.app", + kind: "Dashboard", + path: "dashboards/test2.json", + action: repository.FileActionCreated, + warning: warningErr2, } recorder.Record(ctx, result2) // Record a result with a warning from a different resource type warningErr3 := errors.New("validation warning") result3 := JobResourceResult{ - Name: "test-resource-3", - Group: "test.grafana.app", - Kind: "DataSource", - Path: "datasources/test.yaml", - Action: repository.FileActionCreated, - Warning: warningErr3, + name: "test-resource-3", + group: "test.grafana.app", + kind: "DataSource", + path: "datasources/test.yaml", + action: repository.FileActionCreated, + warning: warningErr3, } recorder.Record(ctx, result3) @@ -184,24 +184,24 @@ func TestJobProgressRecorderWarningWithErrors(t *testing.T) { // Record a result with an error (errors take precedence) errorErr := errors.New("failed to process") result := JobResourceResult{ - Name: "test-resource", - Group: "test.grafana.app", - Kind: "Dashboard", - Path: "dashboards/test.json", - Action: repository.FileActionUpdated, - Error: errorErr, + name: "test-resource", + group: "test.grafana.app", + kind: "Dashboard", + path: "dashboards/test.json", + action: repository.FileActionUpdated, + err: errorErr, } recorder.Record(ctx, result) // Record a result with only a warning warningErr := errors.New("deprecated API used") result2 := JobResourceResult{ - Name: "test-resource-2", - Group: "test.grafana.app", - Kind: "Dashboard", - Path: "dashboards/test2.json", - Action: repository.FileActionCreated, - Warning: warningErr, + name: "test-resource-2", + group: "test.grafana.app", + kind: "Dashboard", + path: "dashboards/test2.json", + action: repository.FileActionCreated, + warning: warningErr, } recorder.Record(ctx, result2) @@ -233,12 +233,12 @@ func TestJobProgressRecorderWarningOnlyNoErrors(t *testing.T) { // Record only warnings, no errors warningErr := errors.New("deprecated API used") result := JobResourceResult{ - Name: "test-resource", - Group: "test.grafana.app", - Kind: "Dashboard", - Path: "dashboards/test.json", - Action: repository.FileActionUpdated, - Warning: warningErr, + name: "test-resource", + group: "test.grafana.app", + kind: "Dashboard", + path: "dashboards/test.json", + action: repository.FileActionUpdated, + warning: warningErr, } recorder.Record(ctx, result) diff --git a/pkg/registry/apis/provisioning/jobs/sync/full.go b/pkg/registry/apis/provisioning/jobs/sync/full.go index 10aad46693b..a592b539f67 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/full.go +++ b/pkg/registry/apis/provisioning/jobs/sync/full.go @@ -82,20 +82,14 @@ func applyChange(ctx context.Context, change ResourceFileChange, clients resourc if change.Action == repository.FileActionDeleted { deleteCtx, deleteSpan := tracer.Start(ctx, "provisioning.sync.full.apply_changes.delete") - result := jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - } if change.Existing == nil || change.Existing.Name == "" { - result.Error = fmt.Errorf("processing deletion for file %s: missing existing reference", change.Path) + result := jobs.NewJobResourceResultWithoutKind(change.Path, change.Action, fmt.Errorf("processing deletion for file %s: missing existing reference", change.Path)) progress.Record(deleteCtx, result) - deleteSpan.RecordError(result.Error) + deleteSpan.RecordError(result.Error()) deleteSpan.End() return } - result.Name = change.Existing.Name - result.Group = change.Existing.Group versionlessGVR := schema.GroupVersionResource{ Group: change.Existing.Group, @@ -105,17 +99,17 @@ func applyChange(ctx context.Context, change ResourceFileChange, clients resourc // TODO: should we use the clients or the resource manager instead? client, gvk, err := clients.ForResource(deleteCtx, versionlessGVR) if err != nil { - result.Kind = versionlessGVR.Resource // could not find a kind - result.Error = fmt.Errorf("get client for deleted object: %w", err) + result := jobs.NewJobResourceResult(change.Existing.Name, change.Existing.Group, versionlessGVR.Resource, change.Path, change.Action, fmt.Errorf("get client for deleted object: %w", err)) progress.Record(deleteCtx, result) deleteSpan.End() return } - result.Kind = gvk.Kind + var deleteErr error if err := client.Delete(deleteCtx, change.Existing.Name, metav1.DeleteOptions{}); err != nil { - result.Error = fmt.Errorf("deleting resource %s/%s %s: %w", change.Existing.Group, gvk.Kind, change.Existing.Name, err) + deleteErr = fmt.Errorf("deleting resource %s/%s %s: %w", change.Existing.Group, gvk.Kind, change.Existing.Name, err) } + result := jobs.NewJobResourceResult(change.Existing.Name, change.Existing.Group, gvk.Kind, change.Path, change.Action, deleteErr) progress.Record(deleteCtx, result) deleteSpan.End() return @@ -125,23 +119,17 @@ func applyChange(ctx context.Context, change ResourceFileChange, clients resourc if safepath.IsDir(change.Path) { // For non-deletions, ensure folder exists ensureFolderCtx, ensureFolderSpan := tracer.Start(ctx, "provisioning.sync.full.apply_changes.ensure_folder_exists") - result := jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - Group: resources.FolderKind.Group, - Kind: resources.FolderKind.Kind, - } folder, err := repositoryResources.EnsureFolderPathExist(ensureFolderCtx, change.Path) if err != nil { - result.Error = fmt.Errorf("ensuring folder exists at path %s: %w", change.Path, err) + result := jobs.NewJobResourceResult("", resources.FolderKind.Group, resources.FolderKind.Kind, change.Path, change.Action, fmt.Errorf("ensuring folder exists at path %s: %w", change.Path, err)) ensureFolderSpan.RecordError(err) ensureFolderSpan.End() progress.Record(ctx, result) return } - result.Name = folder + result := jobs.NewJobResourceResult(folder, resources.FolderKind.Group, resources.FolderKind.Kind, change.Path, change.Action, nil) progress.Record(ensureFolderCtx, result) ensureFolderSpan.End() return @@ -149,18 +137,13 @@ func applyChange(ctx context.Context, change ResourceFileChange, clients resourc writeCtx, writeSpan := tracer.Start(ctx, "provisioning.sync.full.apply_changes.write_resource_from_file") name, gvk, err := repositoryResources.WriteResourceFromFile(writeCtx, change.Path, "") - result := jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - Name: name, - Group: gvk.Group, - Kind: gvk.Kind, - } + var writeErr error if err != nil { writeSpan.RecordError(err) - result.Error = fmt.Errorf("writing resource from file %s: %w", change.Path, err) + writeErr = fmt.Errorf("writing resource from file %s: %w", change.Path, err) } + result := jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, change.Path, change.Action, writeErr) progress.Record(writeCtx, result) writeSpan.End() } @@ -272,11 +255,7 @@ func applyFoldersSerially(ctx context.Context, folders []ResourceFileChange, cli logger.Error("operation timed out after 15 seconds", "path", folder.Path, "action", folder.Action) recordCtx, recordCancel := context.WithTimeout(context.Background(), 15*time.Second) - progress.Record(recordCtx, jobs.JobResourceResult{ - Path: folder.Path, - Action: folder.Action, - Error: fmt.Errorf("operation timed out after 15 seconds"), - }) + progress.Record(recordCtx, jobs.NewJobResourceResultWithoutKind(folder.Path, folder.Action, fmt.Errorf("operation timed out after 15 seconds"))) recordCancel() } @@ -341,11 +320,7 @@ func applyChangeWithTimeout(ctx context.Context, change ResourceFileChange, clie logger.Error("operation timed out after 15 seconds", "path", change.Path, "action", change.Action) recordCtx, recordCancel := context.WithTimeout(context.Background(), 15*time.Second) - progress.Record(recordCtx, jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - Error: fmt.Errorf("operation timed out after 15 seconds"), - }) + progress.Record(recordCtx, jobs.NewJobResourceResultWithoutKind(change.Path, change.Action, fmt.Errorf("operation timed out after 15 seconds"))) recordCancel() } } diff --git a/pkg/registry/apis/provisioning/jobs/sync/full_test.go b/pkg/registry/apis/provisioning/jobs/sync/full_test.go index aaa61ee61db..4f70c2a0e80 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/full_test.go +++ b/pkg/registry/apis/provisioning/jobs/sync/full_test.go @@ -218,8 +218,8 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo }), "").Return("test-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil).Maybe() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - (result.Path == "dashboards/one.json" || result.Path == "dashboards/two.json" || result.Path == "dashboards/three.json") + return result.Action() == repository.FileActionCreated && + (result.Path() == "dashboards/one.json" || result.Path() == "dashboards/two.json" || result.Path() == "dashboards/three.json") })).Return().Maybe() }, expectedError: "too many errors", @@ -239,13 +239,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo repoResources.On("WriteResourceFromFile", mock.Anything, "dashboards/test.json", ""). Return("test-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionCreated, - Path: "dashboards/test.json", - Name: "test-dashboard", - Kind: "Dashboard", - Group: "dashboards", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-dashboard", + "dashboards", + "Dashboard", + "dashboards/test.json", + repository.FileActionCreated, + nil, + )).Return() }, }, { @@ -264,13 +265,13 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Return("test-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, fmt.Errorf("write error")) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Path == "dashboards/test.json" && - result.Name == "test-dashboard" && - result.Kind == "Dashboard" && - result.Group == "dashboards" && - result.Error != nil && - result.Error.Error() == "writing resource from file dashboards/test.json: write error" + return result.Action() == repository.FileActionCreated && + result.Path() == "dashboards/test.json" && + result.Name() == "test-dashboard" && + result.Kind() == "Dashboard" && + result.Group() == "dashboards" && + result.Error() != nil && + result.Error().Error() == "writing resource from file dashboards/test.json: write error" })).Return() }, }, @@ -289,13 +290,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo repoResources.On("WriteResourceFromFile", mock.Anything, "dashboards/test.json", ""). Return("test-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionUpdated, - Path: "dashboards/test.json", - Name: "test-dashboard", - Kind: "Dashboard", - Group: "dashboards", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-dashboard", + "dashboards", + "Dashboard", + "dashboards/test.json", + repository.FileActionUpdated, + nil, + )).Return() }, }, { @@ -314,13 +316,13 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Return("test-dashboard", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, fmt.Errorf("write error")) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionUpdated && - result.Path == "dashboards/test.json" && - result.Name == "test-dashboard" && - result.Kind == "Dashboard" && - result.Group == "dashboards" && - result.Error != nil && - result.Error.Error() == "writing resource from file dashboards/test.json: write error" + return result.Action() == repository.FileActionUpdated && + result.Path() == "dashboards/test.json" && + result.Name() == "test-dashboard" && + result.Kind() == "Dashboard" && + result.Group() == "dashboards" && + result.Error() != nil && + result.Error().Error() == "writing resource from file dashboards/test.json: write error" })).Return() }, }, @@ -337,13 +339,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo progress.On("TooManyErrors").Return(nil) repoResources.On("EnsureFolderPathExist", mock.Anything, "one/two/three/").Return("some-folder", nil) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionCreated, - Path: "one/two/three/", - Name: "some-folder", - Kind: "Folder", - Group: "folder.grafana.app", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "some-folder", + "folder.grafana.app", + "Folder", + "one/two/three/", + repository.FileActionCreated, + nil, + )).Return() }, }, { @@ -364,13 +367,13 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo "one/two/three/", ).Return("some-folder", errors.New("folder creation error")) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Path == "one/two/three/" && - result.Name == "" && - result.Kind == "Folder" && - result.Group == "folder.grafana.app" && - result.Error != nil && - result.Error.Error() == "ensuring folder exists at path one/two/three/: folder creation error" + return result.Action() == repository.FileActionCreated && + result.Path() == "one/two/three/" && + result.Name() == "" && + result.Kind() == "Folder" && + result.Group() == "folder.grafana.app" && + result.Error() != nil && + result.Error().Error() == "ensuring folder exists at path one/two/three/: folder creation error" })).Return() }, }, @@ -423,14 +426,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Version: "v1", }, nil) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionDeleted, - Path: "dashboards/test.json", - Name: "test-dashboard", - Kind: "Dashboard", - Group: "dashboards", - Error: nil, - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-dashboard", + "dashboards", + "Dashboard", + "dashboards/test.json", + repository.FileActionDeleted, + nil, + )).Return() }, }, { @@ -483,13 +486,13 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo }, nil) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Path == "dashboards/test.json" && - result.Name == "test-dashboard" && - result.Kind == "Dashboard" && - result.Group == "dashboards" && - result.Error != nil && - result.Error.Error() == "deleting resource dashboards/Dashboard test-dashboard: delete failed" + return result.Action() == repository.FileActionDeleted && + result.Path() == "dashboards/test.json" && + result.Name() == "test-dashboard" && + result.Kind() == "Dashboard" && + result.Group() == "dashboards" && + result.Error() != nil && + result.Error().Error() == "deleting resource dashboards/Dashboard test-dashboard: delete failed" })).Return() }, }, @@ -506,10 +509,10 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo setupMocks: func(repo *repository.MockRepository, repoResources *resources.MockRepositoryResources, clients *resources.MockResourceClients, progress *jobs.MockJobProgressRecorder, compareFn *MockCompareFn) { progress.On("TooManyErrors").Return(nil) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Path == "dashboards/test.json" && - result.Error != nil && - result.Error.Error() == "processing deletion for file dashboards/test.json: missing existing reference" + return result.Action() == repository.FileActionDeleted && + result.Path() == "dashboards/test.json" && + result.Error() != nil && + result.Error().Error() == "processing deletion for file dashboards/test.json: missing existing reference" })).Return() }, }, @@ -526,10 +529,10 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo setupMocks: func(repo *repository.MockRepository, repoResources *resources.MockRepositoryResources, clients *resources.MockResourceClients, progress *jobs.MockJobProgressRecorder, compareFn *MockCompareFn) { progress.On("TooManyErrors").Return(nil) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Path == "dashboards/test.json" && - result.Error != nil && - result.Error.Error() == "processing deletion for file dashboards/test.json: missing existing reference" + return result.Action() == repository.FileActionDeleted && + result.Path() == "dashboards/test.json" && + result.Error() != nil && + result.Error().Error() == "processing deletion for file dashboards/test.json: missing existing reference" })).Return() }, }, @@ -555,14 +558,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Resource: "dashboards", }).Return(nil, schema.GroupVersionKind{}, errors.New("didn't work")) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Name: "test-dashboard", - Group: "dashboards", - Kind: "dashboards", // could not find a real kind - Action: repository.FileActionDeleted, - Path: "dashboards/test.json", - Error: fmt.Errorf("get client for deleted object: %w", errors.New("didn't work")), - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-dashboard", + "dashboards", + "dashboards", // could not find a real kind + "dashboards/test.json", + repository.FileActionDeleted, + fmt.Errorf("get client for deleted object: %w", errors.New("didn't work")), + )).Return() }, }, { @@ -614,14 +617,14 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Version: "v1", }, nil) - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionDeleted, - Path: "to-be-deleted/", - Name: "test-folder", - Kind: "Folder", - Group: "folders", - Error: nil, - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-folder", + "folders", + "Folder", + "to-be-deleted/", + repository.FileActionDeleted, + nil, + )).Return() }, }, { @@ -674,13 +677,13 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo }, nil) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Path == "to-be-deleted/" && - result.Name == "test-folder" && - result.Kind == "Folder" && - result.Group == "folders" && - result.Error != nil && - result.Error.Error() == "deleting resource folders/Folder test-folder: delete failed" + return result.Action() == repository.FileActionDeleted && + result.Path() == "to-be-deleted/" && + result.Name() == "test-folder" && + result.Kind() == "Folder" && + result.Group() == "folders" && + result.Error() != nil && + result.Error().Error() == "deleting resource folders/Folder test-folder: delete failed" })).Return() }, }, @@ -709,17 +712,17 @@ func TestFullSync_ApplyChanges(t *testing.T) { //nolint:gocyclo Return("", schema.GroupVersionKind{}, context.DeadlineExceeded) progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Path == "dashboards/slow.json" && - result.Error != nil && - result.Error.Error() == "writing resource from file dashboards/slow.json: context deadline exceeded" + return result.Action() == repository.FileActionCreated && + result.Path() == "dashboards/slow.json" && + result.Error() != nil && + result.Error().Error() == "writing resource from file dashboards/slow.json: context deadline exceeded" })).Return().Once() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Path == "dashboards/slow.json" && - result.Error != nil && - result.Error.Error() == "operation timed out after 15 seconds" + return result.Action() == repository.FileActionCreated && + result.Path() == "dashboards/slow.json" && + result.Error() != nil && + result.Error().Error() == "operation timed out after 15 seconds" })).Return().Once() }, }, diff --git a/pkg/registry/apis/provisioning/jobs/sync/incremental.go b/pkg/registry/apis/provisioning/jobs/sync/incremental.go index daa94d94636..2e977f17e22 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/incremental.go +++ b/pkg/registry/apis/provisioning/jobs/sync/incremental.go @@ -101,52 +101,40 @@ func applyIncrementalChanges(ctx context.Context, diff []repository.VersionedFil return nil, tracing.Error(span, fmt.Errorf("unable to create empty file folder: %w", err)) } - progress.Record(ensureFolderCtx, jobs.JobResourceResult{ - Path: safeSegment, - Action: repository.FileActionCreated, - Group: resources.FolderResource.Group, - Kind: resources.FolderKind.Kind, - Name: folder, - }) + result := jobs.NewJobResourceResult(folder, resources.FolderResource.Group, resources.FolderKind.Kind, safeSegment, repository.FileActionCreated, nil) + progress.Record(ensureFolderCtx, result) ensureFolderSpan.End() continue } - progress.Record(ensureFolderCtx, jobs.JobResourceResult{ - Path: change.Path, - Action: repository.FileActionIgnored, - }) + result := jobs.NewJobResourceResultWithoutKind(change.Path, repository.FileActionIgnored, nil) + progress.Record(ensureFolderCtx, result) ensureFolderSpan.End() continue } - result := jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - } + var result jobs.JobResourceResult switch change.Action { case repository.FileActionCreated, repository.FileActionUpdated: writeCtx, writeSpan := tracer.Start(ctx, "provisioning.sync.incremental.write_resource_from_file") name, gvk, err := repositoryResources.WriteResourceFromFile(writeCtx, change.Path, change.Ref) + var resultErr error if err != nil { writeSpan.RecordError(err) - result.Error = fmt.Errorf("writing resource from file %s: %w", change.Path, err) + resultErr = fmt.Errorf("writing resource from file %s: %w", change.Path, err) } - result.Name = name - result.Kind = gvk.Kind - result.Group = gvk.Group + result = jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, change.Path, change.Action, resultErr) writeSpan.End() case repository.FileActionDeleted: removeCtx, removeSpan := tracer.Start(ctx, "provisioning.sync.incremental.remove_resource_from_file") name, folderName, gvk, err := repositoryResources.RemoveResourceFromFile(removeCtx, change.Path, change.PreviousRef) + var resultErr error if err != nil { removeSpan.RecordError(err) - result.Error = fmt.Errorf("removing resource from file %s: %w", change.Path, err) + resultErr = fmt.Errorf("removing resource from file %s: %w", change.Path, err) } - result.Name = name - result.Kind = gvk.Kind - result.Group = gvk.Group + result = jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, change.Path, change.Action, resultErr) if folderName != "" { affectedFolders[safepath.Dir(change.Path)] = folderName @@ -156,13 +144,12 @@ func applyIncrementalChanges(ctx context.Context, diff []repository.VersionedFil case repository.FileActionRenamed: renameCtx, renameSpan := tracer.Start(ctx, "provisioning.sync.incremental.rename_resource_file") name, oldFolderName, gvk, err := repositoryResources.RenameResourceFile(renameCtx, change.PreviousPath, change.PreviousRef, change.Path, change.Ref) + var resultErr error if err != nil { renameSpan.RecordError(err) - result.Error = fmt.Errorf("renaming resource file from %s to %s: %w", change.PreviousPath, change.Path, err) + resultErr = fmt.Errorf("renaming resource file from %s to %s: %w", change.PreviousPath, change.Path, err) } - result.Name = name - result.Kind = gvk.Kind - result.Group = gvk.Group + result = jobs.NewJobResourceResult(name, gvk.Group, gvk.Kind, change.Path, change.Action, resultErr) if oldFolderName != "" { affectedFolders[safepath.Dir(change.Path)] = oldFolderName @@ -171,6 +158,7 @@ func applyIncrementalChanges(ctx context.Context, diff []repository.VersionedFil renameSpan.End() case repository.FileActionIgnored: // do nothing + result = jobs.NewJobResourceResultWithoutKind(change.Path, change.Action, nil) } progress.Record(ctx, result) } diff --git a/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go b/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go index f694d7f5068..30e94c24811 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go +++ b/pkg/registry/apis/provisioning/jobs/sync/incremental_test.go @@ -100,10 +100,10 @@ func TestIncrementalSync(t *testing.T) { // Mock progress recording progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && result.Path == "dashboards/test.json" + return result.Action() == repository.FileActionCreated && result.Path() == "dashboards/test.json" })).Return() progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionUpdated && result.Path == "alerts/alert.yaml" + return result.Action() == repository.FileActionUpdated && result.Path() == "alerts/alert.yaml" })).Return() progress.On("TooManyErrors").Return(nil) @@ -132,13 +132,14 @@ func TestIncrementalSync(t *testing.T) { Return("test-folder", nil) // Mock progress recording - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionCreated, - Path: "unsupported/path/", - Kind: resources.FolderKind.Kind, - Group: resources.FolderResource.Group, - Name: "test-folder", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "test-folder", + resources.FolderResource.Group, + resources.FolderKind.Kind, + "unsupported/path/", + repository.FileActionCreated, + nil, + )).Return() progress.On("TooManyErrors").Return(nil) }, @@ -161,10 +162,11 @@ func TestIncrementalSync(t *testing.T) { progress.On("SetMessage", mock.Anything, "replicating versioned changes").Return() progress.On("SetMessage", mock.Anything, "versioned changes replicated").Return() - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionIgnored, - Path: ".unsupported/path/file.txt", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResultWithoutKind( + ".unsupported/path/file.txt", + repository.FileActionIgnored, + nil, + )).Return() progress.On("TooManyErrors").Return(nil) }, previousRef: "old-ref", @@ -191,13 +193,14 @@ func TestIncrementalSync(t *testing.T) { Return("old-dashboard", "", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) // Mock progress recording - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionDeleted, - Path: "dashboards/old.json", - Name: "old-dashboard", - Kind: "Dashboard", - Group: "dashboards", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "old-dashboard", + "dashboards", + "Dashboard", + "dashboards/old.json", + repository.FileActionDeleted, + nil, + )).Return() progress.On("TooManyErrors").Return(nil) }, @@ -227,13 +230,14 @@ func TestIncrementalSync(t *testing.T) { Return("renamed-dashboard", "", schema.GroupVersionKind{Kind: "Dashboard", Group: "dashboards"}, nil) // Mock progress recording - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionRenamed, - Path: "dashboards/new.json", - Name: "renamed-dashboard", - Kind: "Dashboard", - Group: "dashboards", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResult( + "renamed-dashboard", + "dashboards", + "Dashboard", + "dashboards/new.json", + repository.FileActionRenamed, + nil, + )).Return() progress.On("TooManyErrors").Return(nil) }, @@ -254,10 +258,11 @@ func TestIncrementalSync(t *testing.T) { 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() - progress.On("Record", mock.Anything, jobs.JobResourceResult{ - Action: repository.FileActionIgnored, - Path: "dashboards/ignored.json", - }).Return() + progress.On("Record", mock.Anything, jobs.NewJobResourceResultWithoutKind( + "dashboards/ignored.json", + repository.FileActionIgnored, + nil, + )).Return() progress.On("TooManyErrors").Return(nil) }, previousRef: "old-ref", @@ -309,13 +314,13 @@ func TestIncrementalSync(t *testing.T) { // Mock progress recording with error progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionCreated && - result.Path == "dashboards/test.json" && - result.Name == "test-dashboard" && - result.Kind == "Dashboard" && - result.Group == "dashboards" && - result.Error != nil && - result.Error.Error() == "writing resource from file dashboards/test.json: write failed" + return result.Action() == repository.FileActionCreated && + result.Path() == "dashboards/test.json" && + result.Name() == "test-dashboard" && + result.Kind() == "Dashboard" && + result.Group() == "dashboards" && + result.Error() != nil && + result.Error().Error() == "writing resource from file dashboards/test.json: write failed" })).Return() progress.On("TooManyErrors").Return(nil) @@ -345,13 +350,13 @@ func TestIncrementalSync(t *testing.T) { // Mock progress recording with error progress.On("Record", mock.Anything, mock.MatchedBy(func(result jobs.JobResourceResult) bool { - return result.Action == repository.FileActionDeleted && - result.Path == "dashboards/old.json" && - result.Name == "old-dashboard" && - result.Kind == "Dashboard" && - result.Group == "dashboards" && - result.Error != nil && - result.Error.Error() == "removing resource from file dashboards/old.json: delete failed" + return result.Action() == repository.FileActionDeleted && + result.Path() == "dashboards/old.json" && + result.Name() == "old-dashboard" && + result.Kind() == "Dashboard" && + result.Group() == "dashboards" && + result.Error() != nil && + result.Error().Error() == "removing resource from file dashboards/old.json: delete failed" })).Return() progress.On("TooManyErrors").Return(nil) },