Plugins: Follow root level dist and siblings when walking file system (#78042)

* simplify

* add comments

* tidy up

* remove line

* update comment

* fix pfs test

* fix err import

* delete code

pkg/plugins/manager/loader/finder/local_test.go

@@ -336,6 +336,45 @@ func TestFinder_getAbsPluginJSONPaths(t *testing.T) {
 	})
 }
 
+func TestFinder_getAbsPluginJSONPaths_PluginClass(t *testing.T) {
+	t.Run("When a dist folder exists as a direct child of the plugins path, it will always be resolved", func(t *testing.T) {
+		dir, err := filepath.Abs("../../testdata/pluginRootWithDist")
+		require.NoError(t, err)
+
+		tcs := []struct {
+			name       string
+			followDist bool
+			expected   []string
+		}{
+			{
+				name:       "When followDistFolder is enabled, a nested dist folder will also be resolved",
+				followDist: true,
+				expected: []string{
+					filepath.Join(dir, "datasource/plugin.json"),
+					filepath.Join(dir, "dist/plugin.json"),
+					filepath.Join(dir, "panel/dist/plugin.json"),
+				},
+			},
+			{
+				name:       "When followDistFolder is disabled, a nested dist folder will not be resolved",
+				followDist: false,
+				expected: []string{
+					filepath.Join(dir, "datasource/plugin.json"),
+					filepath.Join(dir, "dist/plugin.json"),
+					filepath.Join(dir, "panel/src/plugin.json"),
+				},
+			},
+		}
+		for _, tc := range tcs {
+			pluginBundles, err := NewLocalFinder(false).getAbsPluginJSONPaths(dir, tc.followDist)
+			require.NoError(t, err)
+
+			sort.Strings(pluginBundles)
+			require.Equal(t, tc.expected, pluginBundles)
+		}
+	})
+}
+
 var fsComparer = cmp.Comparer(func(fs1 plugins.FS, fs2 plugins.FS) bool {
 	fs1Files, err := fs1.Files()
 	if err != nil {

pkg/plugins/pfs/pfs_test.go

@@ -121,6 +121,10 @@ func TestParsePluginTestdata(t *testing.T) {
 			err:  ErrInvalidLineage,
 			skip: "TODO implement BindOption in thema, SatisfiesJoinSchema, then use it here",
 		},
+		"pluginRootWithDist": {
+			err:  ErrNoRootFile,
+			skip: "This folder is used to test multiple plugins in the same folder",
+		},
 		"name-mismatch-panel": {
 			err: ErrInvalidGrafanaPluginInstance,
 		},

pkg/util/filepath.go

@@ -14,6 +14,15 @@ var ErrWalkSkipDir = errors.New("skip this directory")
 // If resolvedPath != "", then we are following symbolic links.
 type WalkFunc func(resolvedPath string, info os.FileInfo, err error) error
 
+type walker struct {
+	rootDir string
+}
+
+// newWalker creates a new walker
+func newWalker(rootDir string) *walker {
+	return &walker{rootDir: rootDir}
+}
+
 // Walk walks a path, optionally following symbolic links, and for each path,
 // it calls the walkFn passed.
 //
@@ -34,7 +43,8 @@ func Walk(path string, followSymlinks bool, detectSymlinkInfiniteLoop bool, foll
 			symlinkPathsFollowed = make(map[string]bool, 8)
 		}
 	}
-	return walk(path, info, resolvedPath, symlinkPathsFollowed, followDistFolder, walkFn)
+
+	return newWalker(path).walk(path, info, resolvedPath, symlinkPathsFollowed, followDistFolder, walkFn)
 }
 
 // walk walks the path. It is a helper/sibling function to Walk.
@@ -43,7 +53,7 @@ func Walk(path string, followSymlinks bool, detectSymlinkInfiniteLoop bool, foll
 //
 // If resolvedPath is "", then we are not following symbolic links.
 // If symlinkPathsFollowed is not nil, then we need to detect infinite loop.
-func walk(path string, info os.FileInfo, resolvedPath string, symlinkPathsFollowed map[string]bool, followDistFolder bool, walkFn WalkFunc) error {
+func (w *walker) walk(path string, info os.FileInfo, resolvedPath string, symlinkPathsFollowed map[string]bool, followDistFolder bool, walkFn WalkFunc) error {
 	if info == nil {
 		return errors.New("walk: Nil FileInfo passed")
 	}
@@ -81,7 +91,7 @@ func walk(path string, info os.FileInfo, resolvedPath string, symlinkPathsFollow
 		if err != nil {
 			return err
 		}
-		return walk(path, info2, path2, symlinkPathsFollowed, followDistFolder, walkFn)
+		return w.walk(path, info2, path2, symlinkPathsFollowed, followDistFolder, walkFn)
 	} else if info.IsDir() {
 		list, err := os.ReadDir(path)
 		if err != nil {
@@ -102,50 +112,52 @@ func walk(path string, info os.FileInfo, resolvedPath string, symlinkPathsFollow
 			subFiles = append(subFiles, subFile{path: path2, resolvedPath: resolvedPath2, fileInfo: fileInfo})
 		}
 
-		if containsDistFolder(subFiles) && followDistFolder {
-			err := walk(
-				filepath.Join(path, "dist"),
-				info,
-				filepath.Join(resolvedPath, "dist"),
-				symlinkPathsFollowed,
-				followDistFolder,
-				walkFn)
-
-			if err != nil {
-				return err
-			}
+		// If we have found a dist directory in a subdirectory (IE not at root path), and followDistFolder is true,
+		// then we want to follow only the dist directory and ignore all other subdirectories.
+		atRootDir := w.rootDir == path
+		if followDistFolder && w.containsDistFolder(subFiles) && !atRootDir {
+			return w.walk(filepath.Join(path, "dist"), info, filepath.Join(resolvedPath, "dist"), symlinkPathsFollowed,
+				followDistFolder, walkFn)
 		} else {
+			// Follow all subdirectories, with special handling for dist directories.
 			for _, p := range subFiles {
-				err = walk(p.path, p.fileInfo, p.resolvedPath, symlinkPathsFollowed, followDistFolder, walkFn)
+				// We only want to skip a dist directory if it is not in the root directory, and followDistFolder is false.
+				if p.isDistDir() && !atRootDir && !followDistFolder {
+					continue
+				}
 
+				err = w.walk(p.path, p.fileInfo, p.resolvedPath, symlinkPathsFollowed, followDistFolder, walkFn)
 				if err != nil {
 					return err
 				}
 			}
 		}
-
 		return nil
 	}
 	return nil
 }
 
+// containsDistFolder returns true if the provided subFiles is a folder named "dist".
+func (w *walker) containsDistFolder(subFiles []subFile) bool {
+	for _, p := range subFiles {
+		if p.fileInfo.IsDir() && p.fileInfo.Name() == "dist" {
+			return true
+		}
+	}
+	return false
+}
+
 type subFile struct {
 	path, resolvedPath string
 	fileInfo           os.FileInfo
 }
 
-func containsDistFolder(subFiles []subFile) bool {
-	for _, p := range subFiles {
-		if p.fileInfo.IsDir() && p.fileInfo.Name() == "dist" {
-			return true
-		}
-	}
-
-	return false
+func (s subFile) isDistDir() bool {
+	return s.fileInfo.IsDir() && s.fileInfo.Name() == "dist"
 }
 
 // CleanRelativePath returns the shortest path name equivalent to path
-// by purely lexical processing. It make sure the provided path is rooted
+// by purely lexical processing. It makes sure the provided path is rooted
 // and then uses filepath.Clean and filepath.Rel to make sure the path
 // doesn't include any separators or elements that shouldn't be there
 // like ., .., //.