address PR comments

pkg/registry/apis/provisioning/jobs/sync/changes.go

@@ -3,7 +3,6 @@ package sync
 import (
 	"context"
 	"fmt"
-	"sort"
 	"strings"
 
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
@@ -143,17 +142,7 @@ func Changes(source []repository.FileTreeEntry, target *provisioning.ResourceLis
 	}
 
 	// Deepest first (stable sort order)
-	sort.Slice(changes, func(i, j int) bool {
-		if safepath.Depth(changes[i].Path) > safepath.Depth(changes[j].Path) {
-			return true
-		}
-
-		if safepath.Depth(changes[i].Path) < safepath.Depth(changes[j].Path) {
-			return false
-		}
-
-		return changes[i].Path < changes[j].Path
-	})
+	safepath.SortByDepth(changes, func(c ResourceFileChange) string { return c.Path }, false)
 
 	return changes, nil
 }

pkg/registry/apis/provisioning/repository/local.go

@@ -251,8 +251,10 @@ func (r *localRepository) ReadTree(ctx context.Context, ref string) ([]FileTreeE
 			if err != nil {
 				return fmt.Errorf("read and calculate hash of path %s: %w", path, err)
 			}
+		} else if !strings.HasSuffix(entry.Path, "/") {
+			// ensure trailing slash for directories
+			entry.Path = entry.Path + "/"
 		}
-		// TODO: do folders have a trailing slash?
 		entries = append(entries, entry)
 		return err
 	})

pkg/registry/apis/provisioning/repository/local_test.go

@@ -91,14 +91,14 @@ func TestLocalResolver(t *testing.T) {
 
 	// Verify all directories and files are present
 	expectedPaths := []string{
-		"another",
-		"another/path",
+		"another/",
+		"another/path/",
 		"another/path/file.txt",
-		"level1",
+		"level1/",
 		"level1/file1.txt",
-		"level1/level2",
+		"level1/level2/",
 		"level1/level2/file2.txt",
-		"level1/level2/level3",
+		"level1/level2/level3/",
 		"level1/level2/level3/file3.txt",
 		"root.txt",
 	}
@@ -1382,7 +1382,7 @@ func TestLocalRepository_ReadTree(t *testing.T) {
 			expected: []FileTreeEntry{
 				{Path: "file1.txt", Blob: true, Size: 8},
 				{Path: "file2.txt", Blob: true, Size: 8},
-				{Path: "subdir", Blob: false},
+				{Path: "subdir/", Blob: false},
 				{Path: "subdir/file3.txt", Blob: true, Size: 8},
 			},
 			expectedErr: nil,

pkg/registry/apis/provisioning/resources/dualwriter.go

@@ -3,8 +3,6 @@ package resources
 import (
 	"context"
 	"fmt"
-	"sort"
-	"strings"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -133,24 +131,6 @@ func (r *DualReadWriter) Delete(ctx context.Context, opts DualWriteOptions) (*Pa
 	return parsed, err
 }
 
-func (r *DualReadWriter) getConfiguredBranch() string {
-	cfg := r.repo.Config()
-	switch cfg.Spec.Type {
-	case provisioning.GitHubRepositoryType:
-		if cfg.Spec.GitHub != nil {
-			return cfg.Spec.GitHub.Branch
-		}
-	case provisioning.GitRepositoryType:
-		if cfg.Spec.Git != nil {
-			return cfg.Spec.Git.Branch
-		}
-	case provisioning.LocalRepositoryType:
-		// branches are not supported for local repositories
-		return ""
-	}
-	return ""
-}
-
 // CreateFolder creates a new folder in the repository
 // FIXME: fix signature to return ParsedResource
 func (r *DualReadWriter) CreateFolder(ctx context.Context, opts DualWriteOptions) (*provisioning.ResourceWrapper, error) {
@@ -186,6 +166,12 @@ func (r *DualReadWriter) CreateFolder(ctx context.Context, opts DualWriteOptions
 		},
 	}
 
+	urls, err := getFolderURLs(ctx, opts.Path, opts.Ref, r.repo)
+	if err != nil {
+		return nil, err
+	}
+	wrap.URLs = urls
+
 	if opts.Ref == "" {
 		folderName, err := r.folders.EnsureFolderPathExist(ctx, opts.Path)
 		if err != nil {
@@ -339,15 +325,15 @@ func (r *DualReadWriter) authorizeCreateFolder(ctx context.Context, _ string) er
 }
 
 func (r *DualReadWriter) deleteFolder(ctx context.Context, opts DualWriteOptions) (*ParsedResource, error) {
-	// if the ref is not the active branch, just delete the files from the branch
-	// do not delete the items from grafana itself
-	if opts.Ref != "" && opts.Ref != r.getConfiguredBranch() {
+	// if the ref is set, it is not the active branch, so just delete the files from the branch
+	// and do not delete the items from grafana itself
+	if opts.Ref != "" {
 		err := r.repo.Delete(ctx, opts.Path, opts.Ref, opts.Message)
 		if err != nil {
 			return nil, fmt.Errorf("error deleting folder from repository: %w", err)
 		}
 
-		return folderDeleteResponse(opts.Path, opts.Ref, r.repo.Config()), nil
+		return folderDeleteResponse(ctx, opts.Path, opts.Ref, r.repo)
 	}
 
 	// before deleting from the repo, first get all children resources to delete from grafana afterwards
@@ -376,11 +362,27 @@ func (r *DualReadWriter) deleteFolder(ctx context.Context, opts DualWriteOptions
 		return nil, fmt.Errorf("delete folder from grafana: %w", err)
 	}
 
-	return folderDeleteResponse(opts.Path, opts.Ref, r.repo.Config()), nil
+	return folderDeleteResponse(ctx, opts.Path, opts.Ref, r.repo)
 }
 
-func folderDeleteResponse(path, ref string, cfg *provisioning.Repository) *ParsedResource {
-	return &ParsedResource{
+func getFolderURLs(ctx context.Context, path, ref string, repo repository.Repository) (*provisioning.ResourceURLs, error) {
+	if urlRepo, ok := repo.(repository.RepositoryWithURLs); ok && ref != "" {
+		urls, err := urlRepo.ResourceURLs(ctx, &repository.FileInfo{Path: path, Ref: ref})
+		if err != nil {
+			return nil, err
+		}
+		return urls, nil
+	}
+	return nil, nil
+}
+
+func folderDeleteResponse(ctx context.Context, path, ref string, repo repository.Repository) (*ParsedResource, error) {
+	urls, err := getFolderURLs(ctx, path, ref, repo)
+	if err != nil {
+		return nil, err
+	}
+
+	parsed := &ParsedResource{
 		Action: provisioning.ResourceActionDelete,
 		Info: &repository.FileInfo{
 			Path: path,
@@ -393,20 +395,23 @@ func folderDeleteResponse(path, ref string, cfg *provisioning.Repository) *Parse
 		},
 		GVR: FolderResource,
 		Repo: provisioning.ResourceRepositoryInfo{
-			Type:      cfg.Spec.Type,
-			Namespace: cfg.Namespace,
-			Name:      cfg.Name,
-			Title:     cfg.Spec.Title,
+			Type:      repo.Config().Spec.Type,
+			Namespace: repo.Config().Namespace,
+			Name:      repo.Config().Name,
+			Title:     repo.Config().Spec.Title,
 		},
+		URLs: urls,
 	}
+
+	return parsed, nil
 }
 
 func (r *DualReadWriter) getChildren(ctx context.Context, folderPath string, treeEntries []repository.FileTreeEntry) ([]*ParsedResource, []Folder, error) {
 	var resourcesInFolder []repository.FileTreeEntry
 	var foldersInFolder []Folder
 	for _, entry := range treeEntries {
-		// the folder itself should be included in this, to do that, trim the suffix of the folder path and see if it matches exactly
-		if !strings.HasPrefix(entry.Path, folderPath) && entry.Path != strings.TrimSuffix(folderPath, "/") {
+		// make sure the path is supported (i.e. not ignored by git sync) and that the path is the folder itself or a child of the folder
+		if IsPathSupported(entry.Path) != nil || !safepath.InDir(entry.Path, folderPath) {
 			continue
 		}
 		// folders cannot be parsed as resources, so handle them separately
@@ -445,12 +450,8 @@ func (r *DualReadWriter) deleteChildren(ctx context.Context, childrenResources [
 	}
 
 	// we need to delete the folders furthest down in the tree first, as folder deletion will fail if there is anything inside of it
-	sort.Slice(folders, func(i, j int) bool {
-		depthI := strings.Count(folders[i].Path, "/")
-		depthJ := strings.Count(folders[j].Path, "/")
+	safepath.SortByDepth(folders, func(f Folder) string { return f.Path }, false)
 
-		return depthI > depthJ
-	})
 	for _, f := range folders {
 		err := r.folders.Client().Delete(ctx, f.ID, metav1.DeleteOptions{})
 		if err != nil {

pkg/registry/apis/provisioning/resources/tree.go

@@ -3,7 +3,6 @@ package resources
 import (
 	"context"
 	"fmt"
-	"sort"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
@@ -95,10 +94,8 @@ func (t *folderTree) Walk(ctx context.Context, fn WalkFunc) error {
 		toWalk = append(toWalk, folder)
 	}
 
-	// sort by depth of the paths
-	sort.Slice(toWalk, func(i, j int) bool {
-		return safepath.Depth(toWalk[i].Path) < safepath.Depth(toWalk[j].Path)
-	})
+	// sort by depth (shallowest first)
+	safepath.SortByDepth(toWalk, func(f Folder) string { return f.Path }, true)
 
 	for _, folder := range toWalk {
 		if err := fn(ctx, folder, t.tree[folder.ID]); err != nil {

pkg/registry/apis/provisioning/safepath/walk.go

@@ -3,6 +3,7 @@ package safepath
 import (
 	"context"
 	"path"
+	"sort"
 	"strings"
 )
 
@@ -43,3 +44,22 @@ func Split(p string) []string {
 	}
 	return strings.Split(trimmed, "/")
 }
+
+// SortByDepth will sort any resource, by its path depth. You must pass in
+// a way to get said path. Ties are alphabetical by default.
+func SortByDepth[T any](items []T, pathExtractor func(T) string, asc bool) {
+	sort.Slice(items, func(i, j int) bool {
+		pathI, pathJ := pathExtractor(items[i]), pathExtractor(items[j])
+		depthI, depthJ := Depth(pathI), Depth(pathJ)
+
+		if depthI == depthJ {
+			// alphabetical by default if depth is the same
+			return pathI < pathJ
+		}
+
+		if asc {
+			return depthI < depthJ
+		}
+		return depthI > depthJ
+	})
+}

pkg/registry/apis/provisioning/safepath/walk_test.go

@@ -176,3 +176,56 @@ func TestWalkError(t *testing.T) {
 
 	require.ErrorIs(t, err, expectedErr)
 }
+
+func TestSortByDepth(t *testing.T) {
+	tests := []struct {
+		name     string
+		asc      bool
+		paths    []string
+		expected []string
+	}{
+		{
+			name:     "ascending sort (shallowest first)",
+			paths:    []string{"a/b/c", "a", "a/b", "d/e/f/g"},
+			asc:      true,
+			expected: []string{"a", "a/b", "a/b/c", "d/e/f/g"},
+		},
+		{
+			name:     "descending sort with alphabetical tie-break",
+			paths:    []string{"a/b/c", "a", "a/b", "d/e/f/g", "x/y/z"},
+			asc:      false,
+			expected: []string{"d/e/f/g", "a/b/c", "x/y/z", "a/b", "a"},
+		},
+		{
+			name:     "paths with empty string",
+			paths:    []string{"a/b/c", "", "a", "a/b"},
+			asc:      true,
+			expected: []string{"", "a", "a/b", "a/b/c"},
+		},
+		{
+			name:     "paths with trailing slashes",
+			paths:    []string{"a/b/", "a/b/c", "b/", "a/", "a"},
+			asc:      true,
+			expected: []string{"a", "a/", "b/", "a/b/", "a/b/c"},
+		},
+		{
+			name:     "single path",
+			paths:    []string{"a/b/c"},
+			expected: []string{"a/b/c"},
+		},
+		{
+			name:     "empty paths",
+			paths:    []string{},
+			expected: []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			paths := make([]string, len(tt.paths))
+			copy(paths, tt.paths)
+			SortByDepth(paths, func(s string) string { return s }, tt.asc)
+			assert.Equal(t, tt.expected, paths)
+		})
+	}
+}

pkg/tests/apis/provisioning/provisioning_test.go

@@ -707,6 +707,9 @@ func TestIntegrationProvisioning_DeleteResources(t *testing.T) {
 		Do(ctx)
 	require.NoError(t, result.Error())
 
+	// make sure we don't fail when there is a .keep file in a folder
+	helper.CopyToProvisioningPath(t, "testdata/.keep", "folder/nested/.keep")
+
 	dashboards, err := helper.DashboardsV1.Resource.List(ctx, metav1.ListOptions{})
 	require.NoError(t, err)
 	require.Equal(t, 3, len(dashboards.Items))

pkg/tests/apis/provisioning/testdata/.keep

@@ -0,0 +1 @@
+# This file ensures the folder/nested directory is tracked in version control