From 16339b07887f109dc6f16ebe11e1a1fdb232fb6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Fri, 25 Jul 2025 06:28:59 +0200 Subject: [PATCH] Provisioning: Move repository file API (#108514) * Bump nanogit version * Embed git repository in the Github one --- go.mod | 2 +- go.sum | 4 +- pkg/apis/provisioning/v0alpha1/types.go | 1 + .../v0alpha1/zz_generated.openapi.go | 4 +- pkg/registry/apis/provisioning/files.go | 29 +- .../apis/provisioning/jobs/sync/sync_test.go | 5 + pkg/registry/apis/provisioning/register.go | 9 + .../repository/git/git_repository_mock.go | 50 ++ .../provisioning/repository/git/repository.go | 62 +++ .../repository/git/repository_test.go | 526 +++++++++++++++++- .../github/github_repository_mock.go | 50 ++ .../repository/github/repository.go | 63 +-- .../repository/github/repository_test.go | 147 ++++- .../provisioning/repository/local/local.go | 60 ++ .../repository/local/local_test.go | 284 ++++++++++ .../provisioning/repository/repository.go | 3 + .../repository/repository_mock.go | 18 + .../repository/staged_repository_mock.go | 50 ++ .../apis/provisioning/resources/dualwriter.go | 229 +++++++- .../provisioning/resources/dualwriter_test.go | 147 +++++ .../provisioning.grafana.app-v0alpha1.json | 27 +- pkg/tests/apis/provisioning/helper_test.go | 44 ++ .../apis/provisioning/provisioning_test.go | 284 ++++++++++ .../provisioning/v0alpha1/endpoints.gen.ts | 12 +- 24 files changed, 2011 insertions(+), 99 deletions(-) create mode 100644 pkg/registry/apis/provisioning/resources/dualwriter_test.go diff --git a/go.mod b/go.mod index 8bd1230bdc8..023caf5199a 100644 --- a/go.mod +++ b/go.mod @@ -104,7 +104,7 @@ require ( github.com/grafana/grafana-openapi-client-go v0.0.0-20231213163343-bd475d63fb79 // @grafana/grafana-backend-group github.com/grafana/grafana-plugin-sdk-go v0.278.0 // @grafana/plugins-platform-backend github.com/grafana/loki/v3 v3.2.1 // @grafana/observability-logs - github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e // @grafana-app-platform-squad + github.com/grafana/nanogit v0.0.0-20250723104447-68f58f5ecec0 // @grafana-app-platform-squad github.com/grafana/otel-profiling-go v0.5.1 // @grafana/grafana-backend-group github.com/grafana/pyroscope-go/godeltaprof v0.1.8 // @grafana/observability-traces-and-profiling github.com/grafana/pyroscope/api v1.2.1-0.20250415190842-3ff7247547ae // @grafana/observability-traces-and-profiling diff --git a/go.sum b/go.sum index 2eaadc3e257..11e8025c482 100644 --- a/go.sum +++ b/go.sum @@ -1642,8 +1642,8 @@ github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608 h1:ZYk42718k github.com/grafana/loki/pkg/push v0.0.0-20231124142027-e52380921608/go.mod h1:f3JSoxBTPXX5ec4FxxeC19nTBSxoTz+cBgS3cYLMcr0= github.com/grafana/loki/v3 v3.2.1 h1:VB7u+KHfvL5aHAxgoVBvz5wVhsdGuqKC7uuOFOOe7jw= github.com/grafana/loki/v3 v3.2.1/go.mod h1:WvdLl6wOS+yahaeQY+xhD2m2XzkHDfKr5FZaX7D/X2Y= -github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e h1:IcrC8SqJcNbGlNq0nqptJ4X8pyy21smMaFrhA7DrfYg= -github.com/grafana/nanogit v0.0.0-20250717084510-7027b3f0138e/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= +github.com/grafana/nanogit v0.0.0-20250723104447-68f58f5ecec0 h1:cS0SlJGIlZbmDLctNj5vIYGemrJDLy25wwoiIyZWVN8= +github.com/grafana/nanogit v0.0.0-20250723104447-68f58f5ecec0/go.mod h1:ToqLjIdvV3AZQa3K6e5m9hy/nsGaUByc2dWQlctB9iA= github.com/grafana/otel-profiling-go v0.5.1 h1:stVPKAFZSa7eGiqbYuG25VcqYksR6iWvF3YH66t4qL8= github.com/grafana/otel-profiling-go v0.5.1/go.mod h1:ftN/t5A/4gQI19/8MoWurBEtC6gFw8Dns1sJZ9W4Tls= github.com/grafana/prometheus-alertmanager v0.25.1-0.20250620093340-be61a673dee6 h1:oJnbhG6ZNy10AjsgNeAtAKeGHogIGOMfAsBH6fYYa5M= diff --git a/pkg/apis/provisioning/v0alpha1/types.go b/pkg/apis/provisioning/v0alpha1/types.go index 5ea21495add..edcf2f790fa 100644 --- a/pkg/apis/provisioning/v0alpha1/types.go +++ b/pkg/apis/provisioning/v0alpha1/types.go @@ -295,6 +295,7 @@ const ( ResourceActionCreate ResourceAction = "create" ResourceActionUpdate ResourceAction = "update" ResourceActionDelete ResourceAction = "delete" + ResourceActionMove ResourceAction = "move" ) // This is a container type for any resource type diff --git a/pkg/apis/provisioning/v0alpha1/zz_generated.openapi.go b/pkg/apis/provisioning/v0alpha1/zz_generated.openapi.go index 737d2cc29c0..37db6def7d7 100644 --- a/pkg/apis/provisioning/v0alpha1/zz_generated.openapi.go +++ b/pkg/apis/provisioning/v0alpha1/zz_generated.openapi.go @@ -1756,10 +1756,10 @@ func schema_pkg_apis_provisioning_v0alpha1_ResourceObjects(ref common.ReferenceC }, "action": { SchemaProps: spec.SchemaProps{ - Description: "The action required/used for dryRun\n\nPossible enum values:\n - `\"create\"`\n - `\"delete\"`\n - `\"update\"`", + Description: "The action required/used for dryRun\n\nPossible enum values:\n - `\"create\"`\n - `\"delete\"`\n - `\"move\"`\n - `\"update\"`", Type: []string{"string"}, Format: "", - Enum: []interface{}{"create", "delete", "update"}, + Enum: []interface{}{"create", "delete", "move", "update"}, }, }, "dryRun": { diff --git a/pkg/registry/apis/provisioning/files.go b/pkg/registry/apis/provisioning/files.go index a0548e1055b..4dd6604c989 100644 --- a/pkg/registry/apis/provisioning/files.go +++ b/pkg/registry/apis/provisioning/files.go @@ -93,9 +93,10 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. return WithTimeout(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { query := r.URL.Query() opts := resources.DualWriteOptions{ - Ref: query.Get("ref"), - Message: query.Get("message"), - SkipDryRun: query.Get("skipDryRun") == "true", + Ref: query.Get("ref"), + Message: query.Get("message"), + SkipDryRun: query.Get("skipDryRun") == "true", + OriginalPath: query.Get("originalPath"), } logger := logger.With("url", r.URL.Path, "ref", opts.Ref, "message", opts.Message) ctx := logging.Context(r.Context(), logger) @@ -139,7 +140,24 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. } obj = resource.AsResourceWrapper() case http.MethodPost: - if isDir { + // Check if this is a move operation first (originalPath query parameter is present) + if opts.OriginalPath != "" { + // For move operations, only read body for file moves (not directory moves) + if !isDir { + opts.Data, err = readBody(r, filesMaxBodySize) + if err != nil { + responder.Error(err) + return + } + } + + resource, err := dualReadWriter.MoveResource(ctx, opts) + if err != nil { + responder.Error(err) + return + } + obj = resource.AsResourceWrapper() + } else if isDir { obj, err = dualReadWriter.CreateFolder(ctx, opts) } else { opts.Data, err = readBody(r, filesMaxBodySize) @@ -148,7 +166,8 @@ func (c *filesConnector) Connect(ctx context.Context, name string, opts runtime. return } - resource, err := dualReadWriter.CreateResource(ctx, opts) + var resource *resources.ParsedResource + resource, err = dualReadWriter.CreateResource(ctx, opts) if err != nil { responder.Error(err) return diff --git a/pkg/registry/apis/provisioning/jobs/sync/sync_test.go b/pkg/registry/apis/provisioning/jobs/sync/sync_test.go index 943e32da35c..f7bb4e0c961 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/sync_test.go +++ b/pkg/registry/apis/provisioning/jobs/sync/sync_test.go @@ -33,6 +33,11 @@ func (m *mockReaderWriter) CompareFiles(ctx context.Context, base, ref string) ( return m.MockVersioned.CompareFiles(ctx, base, ref) } +func (m *mockReaderWriter) Move(ctx context.Context, oldPath, newPath, ref, message string) error { + args := m.MockRepository.Called(ctx, oldPath, newPath, ref, message) + return args.Error(0) +} + func TestSyncer_Sync(t *testing.T) { tests := []struct { name string diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index e3209fa7026..9b60c22df44 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -794,6 +794,15 @@ func (b *APIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAPI, err Required: false, }, }, + { + ParameterProps: spec3.ParameterProps{ + Name: "originalPath", + In: "query", + Description: "path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -> 'c/d.json') or folder-to-folder (e.g., 'some/' -> 'new/')", + Schema: spec.StringProperty(), + Required: false, + }, + }, } sub.Delete.Parameters = comment sub.Post.Parameters = comment diff --git a/pkg/registry/apis/provisioning/repository/git/git_repository_mock.go b/pkg/registry/apis/provisioning/repository/git/git_repository_mock.go index d16a5450326..4b914b50f19 100644 --- a/pkg/registry/apis/provisioning/repository/git/git_repository_mock.go +++ b/pkg/registry/apis/provisioning/repository/git/git_repository_mock.go @@ -451,6 +451,56 @@ func (_c *MockGitRepository_ListRefs_Call) RunAndReturn(run func(context.Context return _c } +// Move provides a mock function with given fields: ctx, oldPath, newPath, ref, message +func (_m *MockGitRepository) Move(ctx context.Context, oldPath string, newPath string, ref string, message string) error { + ret := _m.Called(ctx, oldPath, newPath, ref, message) + + if len(ret) == 0 { + panic("no return value specified for Move") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, oldPath, newPath, ref, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MockGitRepository_Move_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Move' +type MockGitRepository_Move_Call struct { + *mock.Call +} + +// Move is a helper method to define mock.On call +// - ctx context.Context +// - oldPath string +// - newPath string +// - ref string +// - message string +func (_e *MockGitRepository_Expecter) Move(ctx interface{}, oldPath interface{}, newPath interface{}, ref interface{}, message interface{}) *MockGitRepository_Move_Call { + return &MockGitRepository_Move_Call{Call: _e.mock.On("Move", ctx, oldPath, newPath, ref, message)} +} + +func (_c *MockGitRepository_Move_Call) Run(run func(ctx context.Context, oldPath string, newPath string, ref string, message string)) *MockGitRepository_Move_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) + }) + return _c +} + +func (_c *MockGitRepository_Move_Call) Return(_a0 error) *MockGitRepository_Move_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockGitRepository_Move_Call) RunAndReturn(run func(context.Context, string, string, string, string) error) *MockGitRepository_Move_Call { + _c.Call.Return(run) + return _c +} + // OnCreate provides a mock function with given fields: ctx func (_m *MockGitRepository) OnCreate(ctx context.Context) ([]map[string]interface{}, error) { ret := _m.Called(ctx) diff --git a/pkg/registry/apis/provisioning/repository/git/repository.go b/pkg/registry/apis/provisioning/repository/git/repository.go index a822b20eb8d..5bcdb12cd94 100644 --- a/pkg/registry/apis/provisioning/repository/git/repository.go +++ b/pkg/registry/apis/provisioning/repository/git/repository.go @@ -457,6 +457,30 @@ func (r *gitRepository) Delete(ctx context.Context, path, ref, comment string) e return r.commitAndPush(ctx, writer, comment) } +func (r *gitRepository) Move(ctx context.Context, oldPath, newPath, ref, comment string) error { + if ref == "" { + ref = r.gitConfig.Branch + } + ctx, _ = r.logger(ctx, ref) + + branchRef, err := r.ensureBranchExists(ctx, ref) + if err != nil { + return err + } + + // Create a staged writer + writer, err := r.client.NewStagedWriter(ctx, branchRef) + if err != nil { + return fmt.Errorf("create staged writer: %w", err) + } + + if err := r.move(ctx, oldPath, newPath, writer); err != nil { + return err + } + + return r.commitAndPush(ctx, writer, comment) +} + func (r *gitRepository) delete(ctx context.Context, path string, writer nanogit.StagedWriter) error { finalPath := safepath.Join(r.gitConfig.Path, path) // Check if it's a directory - use DeleteTree for directories, DeleteBlob for files @@ -480,6 +504,44 @@ func (r *gitRepository) delete(ctx context.Context, path string, writer nanogit. return nil } +func (r *gitRepository) move(ctx context.Context, oldPath, newPath string, writer nanogit.StagedWriter) error { + oldFinalPath := safepath.Join(r.gitConfig.Path, oldPath) + newFinalPath := safepath.Join(r.gitConfig.Path, newPath) + + // Check if moving directories + if safepath.IsDir(oldPath) && safepath.IsDir(newPath) { + // For directories, trim trailing slashes and use MoveTree + oldTrimmed := strings.TrimSuffix(oldFinalPath, "/") + newTrimmed := strings.TrimSuffix(newFinalPath, "/") + + if _, err := writer.MoveTree(ctx, oldTrimmed, newTrimmed); err != nil { + if errors.Is(err, nanogit.ErrObjectNotFound) { + return repository.ErrFileNotFound + } + if errors.Is(err, nanogit.ErrObjectAlreadyExists) { + return repository.ErrFileAlreadyExists + } + return fmt.Errorf("move tree: %w", err) + } + } else if !safepath.IsDir(oldPath) && !safepath.IsDir(newPath) { + // For files, use MoveBlob operation + if _, err := writer.MoveBlob(ctx, oldFinalPath, newFinalPath); err != nil { + if errors.Is(err, nanogit.ErrObjectNotFound) { + return repository.ErrFileNotFound + } + if errors.Is(err, nanogit.ErrObjectAlreadyExists) { + return repository.ErrFileAlreadyExists + } + return fmt.Errorf("move blob: %w", err) + } + } else { + // Mismatched types (file to directory or vice versa) + return apierrors.NewBadRequest("cannot move between file and directory types") + } + + return nil +} + func (r *gitRepository) History(_ context.Context, _ string, _ string) ([]provisioning.HistoryItem, error) { return nil, &apierrors.StatusError{ErrStatus: metav1.Status{ Status: metav1.StatusFailure, diff --git a/pkg/registry/apis/provisioning/repository/git/repository_test.go b/pkg/registry/apis/provisioning/repository/git/repository_test.go index 2c731b4c9f3..f70346825f6 100644 --- a/pkg/registry/apis/provisioning/repository/git/repository_test.go +++ b/pkg/registry/apis/provisioning/repository/git/repository_test.go @@ -3244,7 +3244,7 @@ func TestGitRepository_CompareFiles_EmptyBase(t *testing.T) { // Verify CompareCommits was called with empty base hash and feature hash require.Equal(t, 1, mockClient.CompareCommitsCallCount()) _, baseHash, refHash := mockClient.CompareCommitsArgsForCall(0) - require.Equal(t, hash.Zero, baseHash) // Empty hash for empty base + require.Equal(t, hash.Zero, baseHash) // Empty base should be zero hash require.Equal(t, hash.MustFromHex("0102030405060708090a0b0c0d0e0f1011121314"), refHash) } @@ -3812,3 +3812,527 @@ func TestGitRepository_OnDelete(t *testing.T) { }) } } + +func TestGitRepository_Move(t *testing.T) { + tests := []struct { + name string + oldPath string + newPath string + ref string + comment string + setupMock func(*mocks.FakeClient) + expectedError string + }{ + { + name: "successful move", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("1234567890abcdef1234567890abcdef12345678") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob - returns the hash of the moved blob + movedHash, _ := hash.FromHex("abcdef1234567890abcdef1234567890abcdef12") + mockWriter.MoveBlobReturns(movedHash, nil) + + // Mock commit and push + commitHash, _ := hash.FromHex("fedcba0987654321fedcba0987654321fedcba09") + mockWriter.CommitReturns(&nanogit.Commit{Hash: commitHash}, nil) + mockWriter.PushReturns(nil) + }, + }, + { + name: "move with empty ref uses default branch", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "", + comment: "move file", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior for default branch + refHash, _ := hash.FromHex("aaaa1111bbbb2222cccc3333dddd4444eeee5555") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob + moveHash, _ := hash.FromHex("bbbb2222cccc3333dddd4444eeee5555ffff6666") + mockWriter.MoveBlobReturns(moveHash, nil) + + // Mock commit and push + commitHash, _ := hash.FromHex("cccc3333dddd4444eeee5555ffff6666aaaa1111") + mockWriter.CommitReturns(&nanogit.Commit{Hash: commitHash}, nil) + mockWriter.PushReturns(nil) + }, + }, + { + name: "successful directory move", + oldPath: "old/", + newPath: "new/", + ref: "main", + comment: "move directory", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("dddd4444eeee5555ffff6666aaaa1111bbbb2222") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveTree (for directories, we trim trailing slashes) + treeHash, _ := hash.FromHex("eeee5555ffff6666aaaa1111bbbb2222cccc3333") + mockWriter.MoveTreeReturns(treeHash, nil) + + // Mock commit and push + commitHash, _ := hash.FromHex("ffff6666aaaa1111bbbb2222cccc3333dddd4444") + mockWriter.CommitReturns(&nanogit.Commit{Hash: commitHash}, nil) + mockWriter.PushReturns(nil) + }, + }, + { + name: "move file to directory type should fail", + oldPath: "file.yaml", + newPath: "directory/", + ref: "main", + comment: "move file to directory", + expectedError: "cannot move between file and directory types", + setupMock: func(mockClient *mocks.FakeClient) { + // No mocks needed as this should fail early during validation + }, + }, + { + name: "move non-existent file", + oldPath: "nonexistent.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move missing file", + expectedError: "file not found", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("aaaa0000bbbb1111cccc2222dddd3333eeee4444") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob to return not found error + mockWriter.MoveBlobReturns(hash.Hash{}, nanogit.ErrObjectNotFound) + }, + }, + { + name: "move to existing file should fail", + oldPath: "old.yaml", + newPath: "existing.yaml", + ref: "main", + comment: "move to existing", + expectedError: "file already exists", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("ffff0000eeee1111dddd2222cccc3333bbbb4444") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob to return already exists error + mockWriter.MoveBlobReturns(hash.Hash{}, nanogit.ErrObjectAlreadyExists) + }, + }, + { + name: "branch creation fails", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "nonexistent-branch", + comment: "move on nonexistent branch", + expectedError: "get source branch ref", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock branch not found + mockClient.GetRefReturnsOnCall(0, nanogit.Ref{}, nanogit.ErrObjectNotFound) + // Mock getting source branch for creation - also fails + mockClient.GetRefReturnsOnCall(1, nanogit.Ref{}, nanogit.ErrObjectNotFound) + }, + }, + { + name: "staged writer creation fails", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + expectedError: "create staged writer", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("1111aaaa2222bbbb3333cccc4444dddd5555eeee") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter failure + mockClient.NewStagedWriterReturns(nil, errors.New("writer creation failed")) + }, + }, + { + name: "commit fails", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + expectedError: "commit changes", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("2222bbbb3333cccc4444dddd5555eeee6666ffff") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob success + moveHash, _ := hash.FromHex("3333cccc4444dddd5555eeee6666ffff7777aaaa") + mockWriter.MoveBlobReturns(moveHash, nil) + + // Mock commit failure + mockWriter.CommitReturns(&nanogit.Commit{}, errors.New("commit failed")) + }, + }, + { + name: "push fails", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + expectedError: "push changes", + setupMock: func(mockClient *mocks.FakeClient) { + // Mock ensureBranchExists behavior + refHash, _ := hash.FromHex("4444dddd5555eeee6666ffff7777aaaa8888bbbb") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + // Mock NewStagedWriter + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + + // Mock MoveBlob success + moveHash, _ := hash.FromHex("5555eeee6666ffff7777aaaa8888bbbb9999cccc") + mockWriter.MoveBlobReturns(moveHash, nil) + + // Mock commit success + commitHash, _ := hash.FromHex("6666ffff7777aaaa8888bbbb9999cccc0000dddd") + mockWriter.CommitReturns(&nanogit.Commit{Hash: commitHash}, nil) + + // Mock push failure + mockWriter.PushReturns(errors.New("push failed")) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup mock + mockClient := &mocks.FakeClient{} + tt.setupMock(mockClient) + + // Create repository config + config := &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + }, + } + + gitConfig := RepositoryConfig{ + URL: "https://github.com/example/repo.git", + Branch: "main", + Token: "token123", + Path: "configs", + } + + gitRepo := &gitRepository{ + config: config, + gitConfig: gitConfig, + client: mockClient, + } + + // Execute move operation + err := gitRepo.Move(context.Background(), tt.oldPath, tt.newPath, tt.ref, tt.comment) + + // Verify results + if tt.expectedError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedError) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestGitRepository_Move_ErrorConditions(t *testing.T) { + tests := []struct { + name string + oldPath string + newPath string + ref string + comment string + setupMock func(*mocks.FakeClient) + expectedError string + errorType error + }{ + { + name: "MoveBlob - ErrObjectNotFound", + oldPath: "missing.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move missing file", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("1234567890abcdef1234567890abcdef12345678") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveBlobReturns(hash.Hash{}, nanogit.ErrObjectNotFound) + }, + expectedError: "file not found", + errorType: repository.ErrFileNotFound, + }, + { + name: "MoveBlob - ErrObjectAlreadyExists", + oldPath: "old.yaml", + newPath: "existing.yaml", + ref: "main", + comment: "move to existing file", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("abcdef1234567890abcdef1234567890abcdef12") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveBlobReturns(hash.Hash{}, nanogit.ErrObjectAlreadyExists) + }, + expectedError: "file already exists", + errorType: repository.ErrFileAlreadyExists, + }, + { + name: "MoveBlob - generic error", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file with generic error", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("fedcba0987654321fedcba0987654321fedcba09") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveBlobReturns(hash.Hash{}, errors.New("network error")) + }, + expectedError: "move blob: network error", + }, + { + name: "MoveTree - ErrObjectNotFound", + oldPath: "missing-dir/", + newPath: "new-dir/", + ref: "main", + comment: "move missing directory", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("1111222233334444555566667777888899990000") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveTreeReturns(hash.Hash{}, nanogit.ErrObjectNotFound) + }, + expectedError: "file not found", + errorType: repository.ErrFileNotFound, + }, + { + name: "MoveTree - ErrObjectAlreadyExists", + oldPath: "old-dir/", + newPath: "existing-dir/", + ref: "main", + comment: "move to existing directory", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("aaaa1111bbbb2222cccc3333dddd4444eeee5555") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveTreeReturns(hash.Hash{}, nanogit.ErrObjectAlreadyExists) + }, + expectedError: "file already exists", + errorType: repository.ErrFileAlreadyExists, + }, + { + name: "MoveTree - generic error", + oldPath: "old-dir/", + newPath: "new-dir/", + ref: "main", + comment: "move directory with generic error", + setupMock: func(mockClient *mocks.FakeClient) { + refHash, _ := hash.FromHex("ffff6666eeee5555dddd4444cccc3333bbbb2222") + mockClient.GetRefReturns(nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + + mockWriter := &mocks.FakeStagedWriter{} + mockClient.NewStagedWriterReturns(mockWriter, nil) + mockWriter.MoveTreeReturns(hash.Hash{}, errors.New("permission denied")) + }, + expectedError: "move tree: permission denied", + }, + { + name: "invalid branch name", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "invalid//branch", + comment: "move with invalid branch", + setupMock: func(mockClient *mocks.FakeClient) { + // No mock setup needed as error is caught during branch validation + }, + expectedError: "invalid branch name", + }, + { + name: "ensure branch exists fails - source branch not found", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "new-branch", + comment: "move to new branch when source doesn't exist", + setupMock: func(mockClient *mocks.FakeClient) { + // First call - new branch doesn't exist + mockClient.GetRefReturnsOnCall(0, nanogit.Ref{}, nanogit.ErrObjectNotFound) + // Second call - source branch also doesn't exist + mockClient.GetRefReturnsOnCall(1, nanogit.Ref{}, nanogit.ErrObjectNotFound) + }, + expectedError: "get source branch ref", + }, + { + name: "ensure branch exists fails - branch creation error", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "new-branch", + comment: "move to new branch with creation failure", + setupMock: func(mockClient *mocks.FakeClient) { + // First call - new branch doesn't exist + mockClient.GetRefReturnsOnCall(0, nanogit.Ref{}, nanogit.ErrObjectNotFound) + // Second call - get source branch succeeds + refHash, _ := hash.FromHex("7777aaaa8888bbbb9999cccc0000dddd1111eeee") + mockClient.GetRefReturnsOnCall(1, nanogit.Ref{ + Name: "refs/heads/main", + Hash: refHash, + }, nil) + // CreateRef fails + mockClient.CreateRefReturns(errors.New("create ref failed")) + }, + expectedError: "create branch", + }, + { + name: "directory to file move type mismatch", + oldPath: "directory/", + newPath: "file.yaml", + ref: "main", + comment: "move directory to file", + setupMock: func(mockClient *mocks.FakeClient) { + // No mock setup needed as error is caught during validation + }, + expectedError: "cannot move between file and directory types", + }, + { + name: "file to directory move type mismatch", + oldPath: "file.yaml", + newPath: "directory/", + ref: "main", + comment: "move file to directory", + setupMock: func(mockClient *mocks.FakeClient) { + // No mock setup needed as error is caught during validation + }, + expectedError: "cannot move between file and directory types", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockClient := &mocks.FakeClient{} + tt.setupMock(mockClient) + + config := &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitRepositoryType, + }, + } + + gitConfig := RepositoryConfig{ + URL: "https://github.com/example/repo.git", + Branch: "main", + Token: "token123", + Path: "configs", + } + + gitRepo := &gitRepository{ + config: config, + gitConfig: gitConfig, + client: mockClient, + } + + err := gitRepo.Move(context.Background(), tt.oldPath, tt.newPath, tt.ref, tt.comment) + + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedError) + + if tt.errorType != nil { + require.ErrorIs(t, err, tt.errorType) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/repository/github/github_repository_mock.go b/pkg/registry/apis/provisioning/repository/github/github_repository_mock.go index a2c2798819a..5ccd5d2e319 100644 --- a/pkg/registry/apis/provisioning/repository/github/github_repository_mock.go +++ b/pkg/registry/apis/provisioning/repository/github/github_repository_mock.go @@ -453,6 +453,56 @@ func (_c *MockGithubRepository_ListRefs_Call) RunAndReturn(run func(context.Cont return _c } +// Move provides a mock function with given fields: ctx, oldPath, newPath, ref, message +func (_m *MockGithubRepository) Move(ctx context.Context, oldPath string, newPath string, ref string, message string) error { + ret := _m.Called(ctx, oldPath, newPath, ref, message) + + if len(ret) == 0 { + panic("no return value specified for Move") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, oldPath, newPath, ref, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MockGithubRepository_Move_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Move' +type MockGithubRepository_Move_Call struct { + *mock.Call +} + +// Move is a helper method to define mock.On call +// - ctx context.Context +// - oldPath string +// - newPath string +// - ref string +// - message string +func (_e *MockGithubRepository_Expecter) Move(ctx interface{}, oldPath interface{}, newPath interface{}, ref interface{}, message interface{}) *MockGithubRepository_Move_Call { + return &MockGithubRepository_Move_Call{Call: _e.mock.On("Move", ctx, oldPath, newPath, ref, message)} +} + +func (_c *MockGithubRepository_Move_Call) Run(run func(ctx context.Context, oldPath string, newPath string, ref string, message string)) *MockGithubRepository_Move_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) + }) + return _c +} + +func (_c *MockGithubRepository_Move_Call) Return(_a0 error) *MockGithubRepository_Move_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockGithubRepository_Move_Call) RunAndReturn(run func(context.Context, string, string, string, string) error) *MockGithubRepository_Move_Call { + _c.Call.Return(run) + return _c +} + // OnCreate provides a mock function with given fields: ctx func (_m *MockGithubRepository) OnCreate(ctx context.Context) ([]map[string]interface{}, error) { ret := _m.Called(ctx) diff --git a/pkg/registry/apis/provisioning/repository/github/repository.go b/pkg/registry/apis/provisioning/repository/github/repository.go index e6cc5aa304d..d2da1de41d1 100644 --- a/pkg/registry/apis/provisioning/repository/github/repository.go +++ b/pkg/registry/apis/provisioning/repository/github/repository.go @@ -22,7 +22,7 @@ const githubTokenSecretSuffix = "-github-token" // Make sure all public functions of this struct call the (*githubRepository).logger function, to ensure the GH repo details are included. type githubRepository struct { - gitRepo git.GitRepository + git.GitRepository config *provisioning.Repository gh Client // assumes github.com base URL secrets secrets.RepositorySecrets @@ -62,19 +62,15 @@ func NewGitHub( } return &githubRepository{ - config: config, - gitRepo: gitRepo, - gh: factory.New(ctx, token), // TODO, baseURL from config - owner: owner, - repo: repo, - secrets: secrets, + config: config, + GitRepository: gitRepo, + gh: factory.New(ctx, token), // TODO, baseURL from config + owner: owner, + repo: repo, + secrets: secrets, }, nil } -func (r *githubRepository) Config() *provisioning.Repository { - return r.gitRepo.Config() -} - func (r *githubRepository) Owner() string { return r.owner } @@ -89,7 +85,7 @@ func (r *githubRepository) Client() Client { // Validate implements provisioning.Repository. func (r *githubRepository) Validate() (list field.ErrorList) { - cfg := r.gitRepo.Config() + cfg := r.Config() gh := cfg.Spec.GitHub if gh == nil { list = append(list, field.Required(field.NewPath("spec", "github"), "a github config is required")) @@ -110,7 +106,7 @@ func (r *githubRepository) Validate() (list field.ErrorList) { return list } - return r.gitRepo.Validate() + return r.GitRepository.Validate() } func ParseOwnerRepoGithub(giturl string) (owner string, repo string, err error) { @@ -139,32 +135,7 @@ func (r *githubRepository) Test(ctx context.Context) (*provisioning.TestResults, field.NewPath("spec", "github", "url"), url, err.Error())), nil } - return r.gitRepo.Test(ctx) -} - -// ReadResource implements provisioning.Repository. -func (r *githubRepository) Read(ctx context.Context, filePath, ref string) (*repository.FileInfo, error) { - return r.gitRepo.Read(ctx, filePath, ref) -} - -func (r *githubRepository) ReadTree(ctx context.Context, ref string) ([]repository.FileTreeEntry, error) { - return r.gitRepo.ReadTree(ctx, ref) -} - -func (r *githubRepository) Create(ctx context.Context, path, ref string, data []byte, comment string) error { - return r.gitRepo.Create(ctx, path, ref, data, comment) -} - -func (r *githubRepository) Update(ctx context.Context, path, ref string, data []byte, comment string) error { - return r.gitRepo.Update(ctx, path, ref, data, comment) -} - -func (r *githubRepository) Write(ctx context.Context, path string, ref string, data []byte, message string) error { - return r.gitRepo.Write(ctx, path, ref, data, message) -} - -func (r *githubRepository) Delete(ctx context.Context, path, ref, comment string) error { - return r.gitRepo.Delete(ctx, path, ref, comment) + return r.GitRepository.Test(ctx) } func (r *githubRepository) History(ctx context.Context, path, ref string) ([]provisioning.HistoryItem, error) { @@ -214,7 +185,7 @@ func (r *githubRepository) History(ctx context.Context, path, ref string) ([]pro // ListRefs list refs from the git repository and add the ref URL to the ref item func (r *githubRepository) ListRefs(ctx context.Context) ([]provisioning.RefItem, error) { - refs, err := r.gitRepo.ListRefs(ctx) + refs, err := r.GitRepository.ListRefs(ctx) if err != nil { return nil, fmt.Errorf("list refs: %w", err) } @@ -226,14 +197,6 @@ func (r *githubRepository) ListRefs(ctx context.Context) ([]provisioning.RefItem return refs, nil } -func (r *githubRepository) LatestRef(ctx context.Context) (string, error) { - return r.gitRepo.LatestRef(ctx) -} - -func (r *githubRepository) CompareFiles(ctx context.Context, base, ref string) ([]repository.VersionedFileChange, error) { - return r.gitRepo.CompareFiles(ctx, base, ref) -} - // ResourceURLs implements RepositoryWithURLs. func (r *githubRepository) ResourceURLs(ctx context.Context, file *repository.FileInfo) (*provisioning.ResourceURLs, error) { cfg := r.config.Spec.GitHub @@ -261,10 +224,6 @@ func (r *githubRepository) ResourceURLs(ctx context.Context, file *repository.Fi return urls, nil } -func (r *githubRepository) Stage(ctx context.Context, opts repository.StageOptions) (repository.StagedRepository, error) { - return r.gitRepo.Stage(ctx, opts) -} - func (r *githubRepository) OnCreate(_ context.Context) ([]map[string]interface{}, error) { return nil, nil } diff --git a/pkg/registry/apis/provisioning/repository/github/repository_test.go b/pkg/registry/apis/provisioning/repository/github/repository_test.go index 960b80e42d0..962a7a4e2b1 100644 --- a/pkg/registry/apis/provisioning/repository/github/repository_test.go +++ b/pkg/registry/apis/provisioning/repository/github/repository_test.go @@ -105,7 +105,7 @@ func TestNewGitHub(t *testing.T) { assert.Equal(t, tt.expectedRepo, repo.Repo()) concreteRepo, ok := repo.(*githubRepository) require.True(t, ok) - assert.Equal(t, gitRepo, concreteRepo.gitRepo) + assert.Equal(t, gitRepo, concreteRepo.GitRepository) } }) } @@ -301,8 +301,8 @@ func TestGitHubRepositoryValidate(t *testing.T) { } repo := &githubRepository{ - config: tt.config, - gitRepo: mockGitRepo, + config: tt.config, + GitRepository: mockGitRepo, } errors := repo.Validate() @@ -388,10 +388,10 @@ func TestGitHubRepositoryTest(t *testing.T) { } repo := &githubRepository{ - config: tt.config, - gitRepo: mockGitRepo, - owner: "grafana", - repo: "grafana", + config: tt.config, + GitRepository: mockGitRepo, + owner: "grafana", + repo: "grafana", } result, err := repo.Test(context.Background()) @@ -802,8 +802,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Config").Return(config) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result := repo.Config() @@ -822,8 +822,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Read", ctx, "test.yaml", "main").Return(expectedFileInfo, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.Read(ctx, "test.yaml", "main") @@ -840,8 +840,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("ReadTree", ctx, "main").Return(expectedEntries, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.ReadTree(ctx, "main") @@ -856,8 +856,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Create", ctx, "new-file.yaml", "main", data, "Create new file").Return(nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } err := repo.Create(ctx, "new-file.yaml", "main", data, "Create new file") @@ -871,8 +871,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Update", ctx, "existing-file.yaml", "main", data, "Update file").Return(nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } err := repo.Update(ctx, "existing-file.yaml", "main", data, "Update file") @@ -886,8 +886,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Write", ctx, "file.yaml", "main", data, "Write file").Return(nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } err := repo.Write(ctx, "file.yaml", "main", data, "Write file") @@ -900,8 +900,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Delete", ctx, "file.yaml", "main", "Delete file").Return(nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } err := repo.Delete(ctx, "file.yaml", "main", "Delete file") @@ -915,8 +915,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("LatestRef", ctx).Return(expectedRef, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.LatestRef(ctx) @@ -935,8 +935,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("ListRefs", ctx).Return(gitRepoRefs, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.ListRefs(ctx) @@ -971,8 +971,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("CompareFiles", ctx, "main", "feature-branch").Return(expectedChanges, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.CompareFiles(ctx, "main", "feature-branch") @@ -991,8 +991,8 @@ func TestGitHubRepositoryDelegation(t *testing.T) { mockGitRepo.On("Stage", ctx, opts).Return(mockStagedRepo, nil) repo := &githubRepository{ - config: config, - gitRepo: mockGitRepo, + config: config, + GitRepository: mockGitRepo, } result, err := repo.Stage(ctx, opts) @@ -1126,3 +1126,92 @@ func TestGitHubRepository_OnDelete(t *testing.T) { }) } } + +func TestGithubRepository_Move(t *testing.T) { + tests := []struct { + name string + oldPath string + newPath string + ref string + comment string + setupMock func(*git.MockGitRepository) + expectedErr error + }{ + { + name: "successful move delegates to git repository", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + setupMock: func(mockGitRepo *git.MockGitRepository) { + mockGitRepo.EXPECT().Move(context.Background(), "old.yaml", "new.yaml", "main", "move file").Return(nil) + }, + expectedErr: nil, + }, + { + name: "move error from git repository", + oldPath: "old.yaml", + newPath: "new.yaml", + ref: "main", + comment: "move file", + setupMock: func(mockGitRepo *git.MockGitRepository) { + mockGitRepo.EXPECT().Move(context.Background(), "old.yaml", "new.yaml", "main", "move file").Return(errors.New("git move failed")) + }, + expectedErr: errors.New("git move failed"), + }, + { + name: "successful directory move", + oldPath: "old/", + newPath: "new/", + ref: "main", + comment: "move directory", + setupMock: func(mockGitRepo *git.MockGitRepository) { + mockGitRepo.EXPECT().Move(context.Background(), "old/", "new/", "main", "move directory").Return(nil) + }, + expectedErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create mock git repository + mockGitRepo := git.NewMockGitRepository(t) + mockSecrets := &secrets.MockRepositorySecrets{} + + // Setup mock expectations + tt.setupMock(mockGitRepo) + + // Create GitHub repository + config := &provisioning.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-repo", + }, + Spec: provisioning.RepositorySpec{ + Type: provisioning.GitHubRepositoryType, + GitHub: &provisioning.GitHubRepositoryConfig{ + URL: "https://github.com/example/repo", + }, + }, + } + + githubRepo := &githubRepository{ + config: config, + GitRepository: mockGitRepo, + owner: "example", + repo: "repo", + secrets: mockSecrets, + } + + // Execute move operation + err := githubRepo.Move(context.Background(), tt.oldPath, tt.newPath, tt.ref, tt.comment) + + // Verify results + if tt.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tt.expectedErr.Error(), err.Error()) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/repository/local/local.go b/pkg/registry/apis/provisioning/repository/local/local.go index 5f63b83cfa1..0ab620e6639 100644 --- a/pkg/registry/apis/provisioning/repository/local/local.go +++ b/pkg/registry/apis/provisioning/repository/local/local.go @@ -372,3 +372,63 @@ func (r *localRepository) Delete(ctx context.Context, path string, ref string, c return os.Remove(fullPath) } + +func (r *localRepository) Move(ctx context.Context, oldPath, newPath, ref, comment string) error { + if err := r.validateRequest(ref); err != nil { + return err + } + + oldFullPath := safepath.Join(r.path, oldPath) + newFullPath := safepath.Join(r.path, newPath) + + // Check if source exists + sourceInfo, err := os.Stat(oldFullPath) + if errors.Is(err, os.ErrNotExist) { + return repository.ErrFileNotFound + } else if err != nil { + return fmt.Errorf("check source: %w", err) + } + + // Check if destination already exists + if _, err := os.Stat(newFullPath); !errors.Is(err, os.ErrNotExist) { + if err != nil { + return fmt.Errorf("check destination: %w", err) + } + return repository.ErrFileAlreadyExists + } + + // Validate move types + sourceIsDir := sourceInfo.IsDir() + targetIsDir := safepath.IsDir(newPath) + + if sourceIsDir != targetIsDir { + return apierrors.NewBadRequest("cannot move between file and directory types") + } + + // Create destination directory if needed + if !sourceIsDir { + // For file moves, create the directory containing the file + destParent := path.Dir(newFullPath) + if err := os.MkdirAll(destParent, 0700); err != nil { + return fmt.Errorf("create destination directory: %w", err) + } + } else { + // For directory moves, create the parent directory of the destination + // but not the destination directory itself (os.Rename will create it) + // We need to be careful with trailing slashes in directory paths + cleanNewPath := strings.TrimSuffix(newFullPath, "/") + destParent := path.Dir(cleanNewPath) + if destParent != "." && destParent != "/" && destParent != r.path { + if err := os.MkdirAll(destParent, 0700); err != nil { + return fmt.Errorf("create destination parent directory: %w", err) + } + } + } + + // Move the file or directory + if err := os.Rename(oldFullPath, newFullPath); err != nil { + return fmt.Errorf("move: %w", err) + } + + return nil +} diff --git a/pkg/registry/apis/provisioning/repository/local/local_test.go b/pkg/registry/apis/provisioning/repository/local/local_test.go index 7895da24995..77a0b99fa09 100644 --- a/pkg/registry/apis/provisioning/repository/local/local_test.go +++ b/pkg/registry/apis/provisioning/repository/local/local_test.go @@ -1517,3 +1517,287 @@ func TestLocalRepository_Config(t *testing.T) { }) } } + +func TestLocalRepository_Move(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) (string, *localRepository) + oldPath string + newPath string + ref string + comment string + expectedErr error + expectedContent string // Expected content of moved file (empty string means don't verify content) + }{ + { + name: "successful move", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create source file + sourceFile := filepath.Join(tempDir, "source.txt") + err := os.WriteFile(sourceFile, []byte("source content"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "source.txt", + newPath: "destination.txt", + ref: "", + comment: "move file", + expectedErr: nil, + expectedContent: "source content", + }, + { + name: "move to subdirectory", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create source file + sourceFile := filepath.Join(tempDir, "test.txt") + err := os.WriteFile(sourceFile, []byte("test content"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "test.txt", + newPath: "newdir/moved.txt", + ref: "", + comment: "move to subdir", + expectedErr: nil, + expectedContent: "test content", + }, + { + name: "move non-existent file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "nonexistent.txt", + newPath: "destination.txt", + ref: "", + comment: "move missing", + expectedErr: repository.ErrFileNotFound, + expectedContent: "", // No content verification for error cases + }, + { + name: "move to existing file", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create source and destination files + sourceFile := filepath.Join(tempDir, "source2.txt") + destFile := filepath.Join(tempDir, "dest2.txt") + + err := os.WriteFile(sourceFile, []byte("source"), 0600) + require.NoError(t, err) + err = os.WriteFile(destFile, []byte("destination"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "source2.txt", + newPath: "dest2.txt", + ref: "", + comment: "move to existing", + expectedErr: repository.ErrFileAlreadyExists, + expectedContent: "", // No content verification for error cases + }, + { + name: "successful directory move", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create directory with files + subdir := filepath.Join(tempDir, "subdir") + err := os.MkdirAll(subdir, 0700) + require.NoError(t, err) + + // Add a file inside the directory + err = os.WriteFile(filepath.Join(subdir, "file.txt"), []byte("dir content"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "subdir/", + newPath: "newsubdir/", + ref: "", + comment: "move directory", + expectedErr: nil, + expectedContent: "", // No content verification for directory moves + }, + { + name: "move file to directory type should fail", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create source file + sourceFile := filepath.Join(tempDir, "file.txt") + err := os.WriteFile(sourceFile, []byte("content"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "file.txt", + newPath: "directory/", + ref: "", + comment: "move file to directory", + expectedErr: apierrors.NewBadRequest("cannot move between file and directory types"), + expectedContent: "", // No content verification for error cases + }, + { + name: "move with ref should fail", + setup: func(t *testing.T) (string, *localRepository) { + tempDir := t.TempDir() + + // Create test file + testFile := filepath.Join(tempDir, "ref_test.txt") + err := os.WriteFile(testFile, []byte("ref test content"), 0600) + require.NoError(t, err) + + repo := &localRepository{ + config: &provisioning.Repository{ + Spec: provisioning.RepositorySpec{ + Local: &provisioning.LocalRepositoryConfig{ + Path: tempDir, + }, + }, + }, + resolver: &LocalFolderResolver{ + PermittedPrefixes: []string{tempDir}, + }, + path: tempDir, + } + + return tempDir, repo + }, + oldPath: "ref_test.txt", + newPath: "ref_dest.txt", + ref: "some-ref", + comment: "move with ref", + expectedErr: apierrors.NewBadRequest("local repository does not support ref"), + expectedContent: "", // No content verification for error cases + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup test environment + tempDir, repo := tc.setup(t) + + // Execute the move operation + err := repo.Move(context.Background(), tc.oldPath, tc.newPath, tc.ref, tc.comment) + + // Verify results + if tc.expectedErr != nil { + require.Error(t, err) + assert.Equal(t, tc.expectedErr.Error(), err.Error(), "Error message should match expected") + } else { + require.NoError(t, err) + + // Verify source file no longer exists + sourceFullPath := filepath.Join(tempDir, tc.oldPath) + _, err = os.Stat(sourceFullPath) + assert.True(t, errors.Is(err, os.ErrNotExist), "Source file should no longer exist") + + // Verify destination file exists + destFullPath := filepath.Join(tempDir, tc.newPath) + _, err = os.Stat(destFullPath) + require.NoError(t, err, "Destination file should exist") + + // Verify content if expectedContent is specified + if tc.expectedContent != "" { + //nolint:gosec // G304: is only for tests + content, err := os.ReadFile(destFullPath) + require.NoError(t, err) + assert.Equal(t, tc.expectedContent, string(content), "Content should be preserved") + } + } + }) + } +} diff --git a/pkg/registry/apis/provisioning/repository/repository.go b/pkg/registry/apis/provisioning/repository/repository.go index 0e72dabbf43..718660c9fab 100644 --- a/pkg/registry/apis/provisioning/repository/repository.go +++ b/pkg/registry/apis/provisioning/repository/repository.go @@ -114,6 +114,9 @@ type Writer interface { // Delete a file in the remote repository Delete(ctx context.Context, path, ref, message string) error + + // Move a file from one path to another in the remote repository + Move(ctx context.Context, oldPath, newPath, ref, message string) error } type ReaderWriter interface { diff --git a/pkg/registry/apis/provisioning/repository/repository_mock.go b/pkg/registry/apis/provisioning/repository/repository_mock.go index b7f5bda049b..05fc5a3832a 100644 --- a/pkg/registry/apis/provisioning/repository/repository_mock.go +++ b/pkg/registry/apis/provisioning/repository/repository_mock.go @@ -280,6 +280,24 @@ func (_m *MockRepository) Write(ctx context.Context, path string, ref string, da return r0 } +// Move provides a mock function with given fields: ctx, oldPath, newPath, ref, message +func (_m *MockRepository) Move(ctx context.Context, oldPath string, newPath string, ref string, message string) error { + ret := _m.Called(ctx, oldPath, newPath, ref, message) + + if len(ret) == 0 { + panic("no return value specified for Move") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, oldPath, newPath, ref, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // NewMockRepository creates a new instance of MockRepository. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewMockRepository(t interface { diff --git a/pkg/registry/apis/provisioning/repository/staged_repository_mock.go b/pkg/registry/apis/provisioning/repository/staged_repository_mock.go index 916b6cc678e..85bfb4f3012 100644 --- a/pkg/registry/apis/provisioning/repository/staged_repository_mock.go +++ b/pkg/registry/apis/provisioning/repository/staged_repository_mock.go @@ -170,6 +170,56 @@ func (_c *MockStagedRepository_Delete_Call) RunAndReturn(run func(context.Contex return _c } +// Move provides a mock function with given fields: ctx, oldPath, newPath, ref, message +func (_m *MockStagedRepository) Move(ctx context.Context, oldPath string, newPath string, ref string, message string) error { + ret := _m.Called(ctx, oldPath, newPath, ref, message) + + if len(ret) == 0 { + panic("no return value specified for Move") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, oldPath, newPath, ref, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// MockStagedRepository_Move_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Move' +type MockStagedRepository_Move_Call struct { + *mock.Call +} + +// Move is a helper method to define mock.On call +// - ctx context.Context +// - oldPath string +// - newPath string +// - ref string +// - message string +func (_e *MockStagedRepository_Expecter) Move(ctx interface{}, oldPath interface{}, newPath interface{}, ref interface{}, message interface{}) *MockStagedRepository_Move_Call { + return &MockStagedRepository_Move_Call{Call: _e.mock.On("Move", ctx, oldPath, newPath, ref, message)} +} + +func (_c *MockStagedRepository_Move_Call) Run(run func(ctx context.Context, oldPath string, newPath string, ref string, message string)) *MockStagedRepository_Move_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(string), args[2].(string), args[3].(string), args[4].(string)) + }) + return _c +} + +func (_c *MockStagedRepository_Move_Call) Return(_a0 error) *MockStagedRepository_Move_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *MockStagedRepository_Move_Call) RunAndReturn(run func(context.Context, string, string, string, string) error) *MockStagedRepository_Move_Call { + _c.Call.Return(run) + return _c +} + // Push provides a mock function with given fields: ctx func (_m *MockStagedRepository) Push(ctx context.Context) error { ret := _m.Called(ctx) diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index 2145285d52e..50efaa13048 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -28,11 +28,12 @@ type DualReadWriter struct { } type DualWriteOptions struct { - Path string - Ref string - Message string - Data []byte - SkipDryRun bool + Path string + Ref string + Message string + Data []byte + SkipDryRun bool + OriginalPath string // Used for move operations } func NewDualReadWriter(repo repository.ReaderWriter, parser Parser, folders *FolderManager, access authlib.AccessChecker) *DualReadWriter { @@ -278,6 +279,216 @@ func (r *DualReadWriter) createOrUpdate(ctx context.Context, create bool, opts D return parsed, err } +// MoveResource moves a resource from one path to another in the repository +func (r *DualReadWriter) MoveResource(ctx context.Context, opts DualWriteOptions) (*ParsedResource, error) { + if err := repository.IsWriteAllowed(r.repo.Config(), opts.Ref); err != nil { + return nil, err + } + + if opts.OriginalPath == "" { + return nil, fmt.Errorf("originalPath is required for move operations") + } + + // Validate that both paths are either files or directories (consistent types) + // Files should end without '/', directories should end with '/' + sourceIsDir := safepath.IsDir(opts.OriginalPath) + targetIsDir := safepath.IsDir(opts.Path) + if sourceIsDir != targetIsDir { + return nil, fmt.Errorf("cannot move between file and directory types - source is %s, target is %s", + getPathType(sourceIsDir), getPathType(targetIsDir)) + } + + // Handle directory moves separately (no parsing/authorization needed) + if sourceIsDir { + return r.moveDirectory(ctx, opts) + } + + // Handle file moves with parsing and authorization + return r.moveFile(ctx, opts) +} + +func (r *DualReadWriter) moveDirectory(ctx context.Context, opts DualWriteOptions) (*ParsedResource, error) { + // For directory moves, we just perform the repository move without parsing + // Always use the provisioning identity when writing + ctx, _, err := identity.WithProvisioningIdentity(ctx, r.repo.Config().Namespace) + if err != nil { + return nil, fmt.Errorf("unable to use provisioning identity: %w", err) + } + + // Perform the move operation in the repository + if err = r.repo.Move(ctx, opts.OriginalPath, opts.Path, opts.Ref, opts.Message); err != nil { + return nil, fmt.Errorf("move directory in repository: %w", err) + } + + // Create a basic parsed resource response for directories + cfg := r.repo.Config() + parsed := &ParsedResource{ + Action: provisioning.ResourceActionMove, + Info: &repository.FileInfo{ + Path: opts.Path, + Ref: opts.Ref, + }, + GVK: schema.GroupVersionKind{ + Group: FolderResource.Group, + Version: FolderResource.Version, + Kind: "Folder", + }, + GVR: FolderResource, + Repo: provisioning.ResourceRepositoryInfo{ + Type: cfg.Spec.Type, + Namespace: cfg.Namespace, + Name: cfg.Name, + Title: cfg.Spec.Title, + }, + } + + // Handle folder management for main branch + if opts.Ref == "" { + // Ensure destination folder path exists + if _, err := r.folders.EnsureFolderPathExist(ctx, opts.Path); err != nil { + return nil, fmt.Errorf("ensure destination folder path exists: %w", err) + } + + // Try to delete the old folder structure from grafana (if it exists) + // This handles cleanup when folders are moved to new locations + oldFolderName, err := r.folders.EnsureFolderPathExist(ctx, opts.OriginalPath) + if err != nil { + return nil, fmt.Errorf("ensure original folder path exists: %w", err) + } + + if oldFolderName != "" { + oldFolder, err := r.folders.GetFolder(ctx, oldFolderName) + if err != nil && !apierrors.IsNotFound(err) { + return nil, fmt.Errorf("get old folder for cleanup: %w", err) + } + + if err == nil { + err = r.folders.Client().Delete(ctx, oldFolder.GetName(), metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + return nil, fmt.Errorf("delete old folder from storage: %w", err) + } + } + } + } + + return parsed, nil +} + +func (r *DualReadWriter) moveFile(ctx context.Context, opts DualWriteOptions) (*ParsedResource, error) { + // Read the original file to get its content for parsing and authorization + originalFile, err := r.repo.Read(ctx, opts.OriginalPath, "") + if err != nil { + return nil, fmt.Errorf("read original file: %w", err) + } + + // Parse the original file to check permissions + parsed, err := r.parser.Parse(ctx, originalFile) + if err != nil { + return nil, fmt.Errorf("parse original file: %w", err) + } + + // Authorize delete on the original path + if err = r.authorize(ctx, parsed, utils.VerbDelete); err != nil { + return nil, fmt.Errorf("not authorized to delete original file: %w", err) + } + + // Determine the content to use for the destination + // If new content is provided in opts.Data, use it; otherwise use original content + var destinationData []byte + if len(opts.Data) > 0 { + destinationData = opts.Data + } else { + destinationData = originalFile.Data + } + + // Create new parsed resource with updated path and content + newInfo := &repository.FileInfo{ + Data: destinationData, + Path: opts.Path, + Ref: opts.Ref, + } + + newParsed, err := r.parser.Parse(ctx, newInfo) + if err != nil { + return nil, fmt.Errorf("parse new file: %w", err) + } + + // Make sure the new resource is valid + if !opts.SkipDryRun { + if err := newParsed.DryRun(ctx); err != nil { + logger := logging.FromContext(ctx).With("path", opts.Path, "originalPath", opts.OriginalPath, "name", newParsed.Obj.GetName(), "ref", opts.Ref) + logger.Warn("failed to dry run resource on move", "error", err) + return nil, fmt.Errorf("error running dryRun on moved resource: %w", err) + } + } + + if len(newParsed.Errors) > 0 { + return nil, fmt.Errorf("errors while parsing moved file [%v]", newParsed.Errors) + } + + // Authorize create on the new path + verb := utils.VerbCreate + if newParsed.Action == provisioning.ResourceActionUpdate { + verb = utils.VerbUpdate + } + if err = r.authorize(ctx, newParsed, verb); err != nil { + return nil, fmt.Errorf("not authorized to create new file: %w", err) + } + + data, err := newParsed.ToSaveBytes() + if err != nil { + return nil, err + } + + // Always use the provisioning identity when writing + ctx, _, err = identity.WithProvisioningIdentity(ctx, newParsed.Obj.GetNamespace()) + if err != nil { + return nil, fmt.Errorf("unable to use provisioning identity: %w", err) + } + + // Perform the move operation in the repository + // If we have new content, we need to update the file content as part of the move + if len(opts.Data) > 0 { + // For moves with content updates, we need to delete the old file and create the new one + if err = r.repo.Delete(ctx, opts.OriginalPath, opts.Ref, opts.Message); err != nil { + return nil, fmt.Errorf("delete original file in repository: %w", err) + } + if err = r.repo.Create(ctx, opts.Path, opts.Ref, data, opts.Message); err != nil { + return nil, fmt.Errorf("create moved file with new content in repository: %w", err) + } + } else { + // For simple moves without content changes, use the move operation + if err = r.repo.Move(ctx, opts.OriginalPath, opts.Path, opts.Ref, opts.Message); err != nil { + return nil, fmt.Errorf("move file in repository: %w", err) + } + } + + // Update the grafana database if this is the main branch + if opts.Ref == "" && newParsed.Client != nil { + if _, err := r.folders.EnsureFolderPathExist(ctx, opts.Path); err != nil { + return nil, fmt.Errorf("ensure folder path exists: %w", err) + } + + // Delete the old resource from grafana if name changed + if newParsed.Obj.GetName() != parsed.Obj.GetName() { + err = parsed.Client.Delete(ctx, parsed.Obj.GetName(), metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + return nil, fmt.Errorf("delete original resource from storage: %w", err) + } + } + + // Create/update the new resource in grafana + err = newParsed.Run(ctx) + if err != nil { + return nil, fmt.Errorf("create moved resource in storage: %w", err) + } + } + + newParsed.Action = provisioning.ResourceActionMove + + return newParsed, nil +} + func (r *DualReadWriter) authorize(ctx context.Context, parsed *ParsedResource, verb string) error { id, err := identity.GetRequester(ctx) if err != nil { @@ -376,6 +587,14 @@ func getFolderURLs(ctx context.Context, path, ref string, repo repository.Reposi return nil, nil } +// getPathType returns a human-readable description of the path type +func getPathType(isDir bool) string { + if isDir { + return "directory (ends with '/')" + } + return "file (no trailing '/')" +} + func folderDeleteResponse(ctx context.Context, path, ref string, repo repository.Repository) (*ParsedResource, error) { urls, err := getFolderURLs(ctx, path, ref, repo) if err != nil { diff --git a/pkg/registry/apis/provisioning/resources/dualwriter_test.go b/pkg/registry/apis/provisioning/resources/dualwriter_test.go new file mode 100644 index 00000000000..10ec8270655 --- /dev/null +++ b/pkg/registry/apis/provisioning/resources/dualwriter_test.go @@ -0,0 +1,147 @@ +package resources + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/grafana/grafana/pkg/registry/apis/provisioning/safepath" +) + +func TestGetPathType(t *testing.T) { + tests := []struct { + name string + isDir bool + expected string + }{ + { + name: "directory path", + isDir: true, + expected: "directory (ends with '/')", + }, + { + name: "file path", + isDir: false, + expected: "file (no trailing '/')", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getPathType(tt.isDir) + assert.Equal(t, tt.expected, result) + }) + } +} + +func TestMovePathValidation(t *testing.T) { + tests := []struct { + name string + originalPath string + newPath string + expectError bool + errorMessage string + }{ + { + name: "file to file move (valid)", + originalPath: "old/file.json", + newPath: "new/file.json", + expectError: false, + }, + { + name: "directory to directory move (valid)", + originalPath: "old/folder/", + newPath: "new/folder/", + expectError: false, + }, + { + name: "file to directory move (invalid)", + originalPath: "old/file.json", + newPath: "new/folder/", + expectError: true, + errorMessage: "cannot move between file and directory types", + }, + { + name: "directory to file move (invalid)", + originalPath: "old/folder/", + newPath: "new/file.json", + expectError: true, + errorMessage: "cannot move between file and directory types", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the path validation logic that would be used in MoveResource + sourceIsDir := safepath.IsDir(tt.originalPath) + targetIsDir := safepath.IsDir(tt.newPath) + + if tt.expectError { + assert.NotEqual(t, sourceIsDir, targetIsDir, "Path types should be different for invalid moves") + } else { + assert.Equal(t, sourceIsDir, targetIsDir, "Path types should be the same for valid moves") + } + }) + } +} + +func TestMoveOptionsContentHandling(t *testing.T) { + tests := []struct { + name string + opts DualWriteOptions + originalData []byte + expectedContentToUse []byte + expectedUseOriginal bool + }{ + { + name: "move with new content provided", + opts: DualWriteOptions{ + Path: "new/file.json", + OriginalPath: "old/file.json", + Data: []byte(`{"updated": "content"}`), + }, + originalData: []byte(`{"original": "content"}`), + expectedContentToUse: []byte(`{"updated": "content"}`), + expectedUseOriginal: false, + }, + { + name: "move without new content (nil)", + opts: DualWriteOptions{ + Path: "new/file.json", + OriginalPath: "old/file.json", + Data: nil, + }, + originalData: []byte(`{"original": "content"}`), + expectedContentToUse: []byte(`{"original": "content"}`), + expectedUseOriginal: true, + }, + { + name: "move without new content (empty slice)", + opts: DualWriteOptions{ + Path: "new/file.json", + OriginalPath: "old/file.json", + Data: []byte{}, + }, + originalData: []byte(`{"original": "content"}`), + expectedContentToUse: []byte(`{"original": "content"}`), + expectedUseOriginal: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Simulate the content selection logic from moveFile method + var destinationData []byte + useOriginal := len(tt.opts.Data) == 0 + + if useOriginal { + destinationData = tt.originalData + } else { + destinationData = tt.opts.Data + } + + assert.Equal(t, tt.expectedUseOriginal, useOriginal, "Should correctly determine whether to use original content") + assert.Equal(t, tt.expectedContentToUse, destinationData, "Should select correct content for destination") + }) + } +} diff --git a/pkg/tests/apis/openapi_snapshots/provisioning.grafana.app-v0alpha1.json b/pkg/tests/apis/openapi_snapshots/provisioning.grafana.app-v0alpha1.json index 4f1beaa5e67..0faaab04b78 100644 --- a/pkg/tests/apis/openapi_snapshots/provisioning.grafana.app-v0alpha1.json +++ b/pkg/tests/apis/openapi_snapshots/provisioning.grafana.app-v0alpha1.json @@ -1281,6 +1281,14 @@ "schema": { "type": "boolean" } + }, + { + "name": "originalPath", + "in": "query", + "description": "path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -\u003e 'c/d.json') or folder-to-folder (e.g., 'some/' -\u003e 'new/')", + "schema": { + "type": "string" + } } ], "requestBody": { @@ -1385,6 +1393,14 @@ "schema": { "type": "boolean" } + }, + { + "name": "originalPath", + "in": "query", + "description": "path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -\u003e 'c/d.json') or folder-to-folder (e.g., 'some/' -\u003e 'new/')", + "schema": { + "type": "string" + } } ], "requestBody": { @@ -1489,6 +1505,14 @@ "schema": { "type": "boolean" } + }, + { + "name": "originalPath", + "in": "query", + "description": "path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -\u003e 'c/d.json') or folder-to-folder (e.g., 'some/' -\u003e 'new/')", + "schema": { + "type": "string" + } } ], "responses": { @@ -3714,11 +3738,12 @@ ], "properties": { "action": { - "description": "The action required/used for dryRun\n\nPossible enum values:\n - `\"create\"`\n - `\"delete\"`\n - `\"update\"`", + "description": "The action required/used for dryRun\n\nPossible enum values:\n - `\"create\"`\n - `\"delete\"`\n - `\"move\"`\n - `\"update\"`", "type": "string", "enum": [ "create", "delete", + "move", "update" ] }, diff --git a/pkg/tests/apis/provisioning/helper_test.go b/pkg/tests/apis/provisioning/helper_test.go index d2787e68892..cff6dbeeabf 100644 --- a/pkg/tests/apis/provisioning/helper_test.go +++ b/pkg/tests/apis/provisioning/helper_test.go @@ -3,6 +3,9 @@ package provisioning import ( "context" "encoding/json" + "fmt" + "net/http" + "net/url" "os" "path" "strings" @@ -336,3 +339,44 @@ func unstructuredToRepository(t *testing.T, obj *unstructured.Unstructured) *pro return repo } + +// postFilesRequest performs a direct HTTP POST request to the files API. +// This bypasses Kubernetes REST client limitations with '/' characters in subresource names. +type filesPostOptions struct { + targetPath string // The target file/directory path + originalPath string // Source path for move operations (optional) + message string // Commit message (optional) + body string // Request body content (optional) + ref string // Git ref/branch (optional) +} + +func (h *provisioningTestHelper) postFilesRequest(t *testing.T, repo string, opts filesPostOptions) *http.Response { + addr := h.GetEnv().Server.HTTPServer.Listener.Addr().String() + baseUrl := fmt.Sprintf("http://admin:admin@%s/apis/provisioning.grafana.app/v0alpha1/namespaces/default/repositories/%s/files/%s", + addr, repo, opts.targetPath) + + // Build the URL with proper query parameter encoding + parsedUrl, err := url.Parse(baseUrl) + require.NoError(t, err) + params := parsedUrl.Query() + + if opts.originalPath != "" { + params.Set("originalPath", opts.originalPath) + } + if opts.message != "" { + params.Set("message", opts.message) + } + if opts.ref != "" { + params.Set("ref", opts.ref) + } + parsedUrl.RawQuery = params.Encode() + + req, err := http.NewRequest(http.MethodPost, parsedUrl.String(), strings.NewReader(opts.body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + + return resp +} diff --git a/pkg/tests/apis/provisioning/provisioning_test.go b/pkg/tests/apis/provisioning/provisioning_test.go index 7fa26a30fb6..a5edc751fbe 100644 --- a/pkg/tests/apis/provisioning/provisioning_test.go +++ b/pkg/tests/apis/provisioning/provisioning_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "io/fs" "net/http" "os" @@ -820,3 +821,286 @@ func TestIntegrationProvisioning_DeleteResources(t *testing.T) { require.Error(t, result.Error()) }) } + +func TestIntegrationProvisioning_MoveResources(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := runGrafana(t) + ctx := context.Background() + + const repo = "move-test-repo" + localTmp := helper.RenderObject(t, "testdata/local-write.json.tmpl", map[string]any{ + "Name": repo, + "SyncEnabled": true, + "SyncTarget": "instance", + }) + _, err := helper.Repositories.Resource.Create(ctx, localTmp, metav1.CreateOptions{}) + require.NoError(t, err) + + // Copy test dashboards to the repository path for initial setup + const originalDashboard = "all-panels.json" + helper.CopyToProvisioningPath(t, "testdata/all-panels.json", originalDashboard) + + // Wait for sync to ensure the dashboard is created in Grafana + helper.SyncAndWait(t, repo, nil) + + // Verify the original dashboard exists in Grafana (using the UID from all-panels.json) + const allPanelsUID = "n1jR8vnnz" // This is the UID from the all-panels.json file + obj, err := helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "original dashboard should exist in Grafana") + require.Equal(t, repo, obj.GetAnnotations()[utils.AnnoKeyManagerIdentity]) + + t.Run("move file without content change", func(t *testing.T) { + const targetPath = "moved/simple-move.json" + + // Perform the move operation using helper function + resp := helper.postFilesRequest(t, repo, filesPostOptions{ + targetPath: targetPath, + originalPath: originalDashboard, + message: "move file without content change", + }) + // nolint:errcheck + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode, "move operation should succeed") + + // Verify the file moved in the repository + movedObj, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "moved", "simple-move.json") + require.NoError(t, err, "moved file should exist in repository") + + // Check the content is preserved (verify it's still the all-panels dashboard) + resource, _, err := unstructured.NestedMap(movedObj.Object, "resource") + require.NoError(t, err) + dryRun, _, err := unstructured.NestedMap(resource, "dryRun") + require.NoError(t, err) + title, _, err := unstructured.NestedString(dryRun, "spec", "title") + require.NoError(t, err) + require.Equal(t, "Panel tests - All panels", title, "content should be preserved") + + // Verify original file no longer exists + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", originalDashboard) + require.Error(t, err, "original file should no longer exist") + + // Verify dashboard still exists in Grafana with same content but may have updated path references + helper.SyncAndWait(t, repo, nil) + _, err = helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.NoError(t, err, "dashboard should still exist in Grafana after move") + }) + + t.Run("move file to nested path without ref", func(t *testing.T) { + // Test a different scenario: Move a file that was never synced to Grafana + // This might reveal the issue if dashboard creation fails during move + const sourceFile = "never-synced.json" + helper.CopyToProvisioningPath(t, "testdata/timeline-demo.json", sourceFile) + + // DO NOT sync - move the file immediately without it ever being in Grafana + const targetPath = "deep/nested/timeline.json" + + // Perform the move operation without the file ever being synced to Grafana + resp := helper.postFilesRequest(t, repo, filesPostOptions{ + targetPath: targetPath, + originalPath: sourceFile, + message: "move never-synced file to nested path", + }) + // nolint:errcheck + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode, "move operation should succeed") + + // Check folders were created and validate hierarchy + folderList, err := helper.Folders.Resource.List(ctx, metav1.ListOptions{}) + require.NoError(t, err, "should be able to list folders") + + // Build a map of folder names to their objects for easier lookup + folders := make(map[string]*unstructured.Unstructured) + for _, folder := range folderList.Items { + title, _, _ := unstructured.NestedString(folder.Object, "spec", "title") + folders[title] = &folder + parent, _, _ := unstructured.NestedString(folder.Object, "metadata", "annotations", "grafana.app/folder") + t.Logf(" - %s: %s (parent: %s)", folder.GetName(), title, parent) + } + + // Validate expected folders exist with proper hierarchy + // Expected structure: deep -> deep/nested + deepFolderTitle := "deep" + nestedFolderTitle := "nested" + + // Validate "deep" folder exists and has no parent (is top-level) + require.Contains(t, folders, deepFolderTitle, "deep folder should exist") + f := folders[deepFolderTitle] + deepFolderName := f.GetName() + title, _, _ := unstructured.NestedString(f.Object, "spec", "title") + require.Equal(t, deepFolderTitle, title, "deep folder should have correct title") + parent, found, _ := unstructured.NestedString(f.Object, "metadata", "annotations", "grafana.app/folder") + require.True(t, !found || parent == "", "deep folder should be top-level (no parent)") + + // Validate "deep/nested" folder exists and has "deep" as parent + require.Contains(t, folders, nestedFolderTitle, "nested folder should exist") + f = folders[nestedFolderTitle] + nestedFolderName := f.GetName() + title, _, _ = unstructured.NestedString(f.Object, "spec", "title") + require.Equal(t, nestedFolderTitle, title, "nested folder should have correct title") + parent, _, _ = unstructured.NestedString(f.Object, "metadata", "annotations", "grafana.app/folder") + require.Equal(t, deepFolderName, parent, "nested folder should have deep folder as parent") + + // The key test: Check if dashboard was created in Grafana during move + const timelineUID = "mIJjFy8Kz" + dashboard, err := helper.DashboardsV1.Resource.Get(ctx, timelineUID, metav1.GetOptions{}) + require.NoError(t, err, "dashboard should exist in Grafana after moving never-synced file") + dashboardFolder, _, _ := unstructured.NestedString(dashboard.Object, "metadata", "annotations", "grafana.app/folder") + + // Validate dashboard is in the correct nested folder + require.Equal(t, nestedFolderName, dashboardFolder, "dashboard should be in the nested folder") + + // Verify the file moved in the repository + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "deep", "nested", "timeline.json") + require.NoError(t, err, "moved file should exist in nested repository path") + + // Verify the original file no longer exists in the repository + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", sourceFile) + require.Error(t, err, "original file should no longer exist in repository") + }) + + t.Run("move file with content update", func(t *testing.T) { + const sourcePath = "moved/simple-move.json" // Use the file from previous test + const targetPath = "updated/content-updated.json" + + // Use text-options.json content for the update + updatedContent := helper.LoadFile("testdata/text-options.json") + + // Perform move with content update using helper function + resp := helper.postFilesRequest(t, repo, filesPostOptions{ + targetPath: targetPath, + originalPath: sourcePath, + message: "move file with content update", + body: string(updatedContent), + }) + // nolint:errcheck + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode, "move with content update should succeed") + + // Verify the moved file has updated content (should now be text-options dashboard) + movedObj, err := helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "updated", "content-updated.json") + require.NoError(t, err, "moved file should exist in repository") + + resource, _, err := unstructured.NestedMap(movedObj.Object, "resource") + require.NoError(t, err) + dryRun, _, err := unstructured.NestedMap(resource, "dryRun") + require.NoError(t, err) + title, _, err := unstructured.NestedString(dryRun, "spec", "title") + require.NoError(t, err) + require.Equal(t, "Text options", title, "content should be updated to text-options dashboard") + + // Check it has the expected UID from text-options.json + name, _, err := unstructured.NestedString(dryRun, "metadata", "name") + require.NoError(t, err) + require.Equal(t, "WZ7AhQiVz", name, "should have the UID from text-options.json") + + // Verify source file no longer exists + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "moved", "simple-move.json") + require.Error(t, err, "source file should no longer exist") + + // Sync and verify the updated dashboard exists in Grafana + helper.SyncAndWait(t, repo, nil) + const textOptionsUID = "WZ7AhQiVz" // UID from text-options.json + updatedDashboard, err := helper.DashboardsV1.Resource.Get(ctx, textOptionsUID, metav1.GetOptions{}) + require.NoError(t, err, "updated dashboard should exist in Grafana") + + // Verify the original dashboard was deleted from Grafana + _, err = helper.DashboardsV1.Resource.Get(ctx, allPanelsUID, metav1.GetOptions{}) + require.Error(t, err, "original dashboard should be deleted from Grafana") + require.True(t, apierrors.IsNotFound(err)) + + // Verify the new dashboard has the updated content + updatedTitle, _, err := unstructured.NestedString(updatedDashboard.Object, "spec", "title") + require.NoError(t, err) + require.Equal(t, "Text options", updatedTitle) + }) + + t.Run("move directory", func(t *testing.T) { + // Create some files in a directory first using existing testdata files + helper.CopyToProvisioningPath(t, "testdata/timeline-demo.json", "source-dir/timeline-demo.json") + helper.CopyToProvisioningPath(t, "testdata/text-options.json", "source-dir/text-options.json") + + // Sync to ensure files are recognized + helper.SyncAndWait(t, repo, nil) + + const sourceDir = "source-dir/" + const targetDir = "moved-dir/" + + // Move directory using helper function + resp := helper.postFilesRequest(t, repo, filesPostOptions{ + targetPath: targetDir, + originalPath: sourceDir, + message: "move directory", + }) + // nolint:errcheck + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode, "directory move should succeed") + + // Verify source directory no longer exists + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "source-dir") + require.Error(t, err, "source directory should no longer exist") + + // Verify target directory and files exist + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "moved-dir", "timeline-demo.json") + require.NoError(t, err, "moved timeline-demo.json should exist") + _, err = helper.Repositories.Resource.Get(ctx, repo, metav1.GetOptions{}, "files", "moved-dir", "text-options.json") + require.NoError(t, err, "moved text-options.json should exist") + }) + + t.Run("error cases", func(t *testing.T) { + t.Run("missing originalPath parameter", func(t *testing.T) { + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "target.json"). + Body([]byte(`{"test": "content"}`)). + SetHeader("Content-Type", "application/json"). + Do(ctx) + require.Error(t, result.Error(), "should fail without originalPath") + }) + + t.Run("file to directory type mismatch", func(t *testing.T) { + // First create a simple test file without slashes in the path + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "simple-test.json"). + Body(helper.LoadFile("testdata/all-panels.json")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + require.NoError(t, result.Error(), "should create test file") + + // Now try to move this file to a directory path using helper function + resp := helper.postFilesRequest(t, repo, filesPostOptions{ + targetPath: "target-dir/", + originalPath: "simple-test.json", + message: "test move", + }) + // nolint:errcheck + defer resp.Body.Close() + // Read response body to check error message + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + require.NotEqual(t, http.StatusOK, resp.StatusCode, "should fail when moving file to directory") + require.Contains(t, string(body), "cannot move between file and directory types") + }) + + t.Run("non-existent source file", func(t *testing.T) { + result := helper.AdminREST.Post(). + Namespace("default"). + Resource("repositories"). + Name(repo). + SubResource("files", "target.json"). + Param("originalPath", "non-existent.json"). + Body([]byte("")). + SetHeader("Content-Type", "application/json"). + Do(ctx) + require.Error(t, result.Error(), "should fail when source file doesn't exist") + }) + }) +} diff --git a/public/app/api/clients/provisioning/v0alpha1/endpoints.gen.ts b/public/app/api/clients/provisioning/v0alpha1/endpoints.gen.ts index 0ecc569f2cb..ecd79c6c52e 100644 --- a/public/app/api/clients/provisioning/v0alpha1/endpoints.gen.ts +++ b/public/app/api/clients/provisioning/v0alpha1/endpoints.gen.ts @@ -161,6 +161,7 @@ const injectedRtkApi = api ref: queryArg.ref, message: queryArg.message, skipDryRun: queryArg.skipDryRun, + originalPath: queryArg.originalPath, }, }), invalidatesTags: ['Repository'], @@ -177,6 +178,7 @@ const injectedRtkApi = api ref: queryArg.ref, message: queryArg.message, skipDryRun: queryArg.skipDryRun, + originalPath: queryArg.originalPath, }, }), invalidatesTags: ['Repository'], @@ -192,6 +194,7 @@ const injectedRtkApi = api ref: queryArg.ref, message: queryArg.message, skipDryRun: queryArg.skipDryRun, + originalPath: queryArg.originalPath, }, }), invalidatesTags: ['Repository'], @@ -527,6 +530,8 @@ export type ReplaceRepositoryFilesWithPathApiArg = { message?: string; /** do not pro-actively verify the payload */ skipDryRun?: boolean; + /** path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -> 'c/d.json') or folder-to-folder (e.g., 'some/' -> 'new/') */ + originalPath?: string; body: { [key: string]: any; }; @@ -543,6 +548,8 @@ export type CreateRepositoryFilesWithPathApiArg = { message?: string; /** do not pro-actively verify the payload */ skipDryRun?: boolean; + /** path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -> 'c/d.json') or folder-to-folder (e.g., 'some/' -> 'new/') */ + originalPath?: string; body: { [key: string]: any; }; @@ -559,6 +566,8 @@ export type DeleteRepositoryFilesWithPathApiArg = { message?: string; /** do not pro-actively verify the payload */ skipDryRun?: boolean; + /** path of file to move (used with POST method for move operations). Must be same type as target path: file-to-file (e.g., 'some/a.json' -> 'c/d.json') or folder-to-folder (e.g., 'some/' -> 'new/') */ + originalPath?: string; }; export type GetRepositoryHistoryApiResponse = /** status 200 OK */ string; export type GetRepositoryHistoryApiArg = { @@ -1121,8 +1130,9 @@ export type ResourceObjects = { Possible enum values: - `"create"` - `"delete"` + - `"move"` - `"update"` */ - action?: 'create' | 'delete' | 'update'; + action?: 'create' | 'delete' | 'move' | 'update'; /** The value returned from a dryRun request */ dryRun?: Unstructured; /** The same value, currently saved in the grafana database */