Merge pull request #2341 from buildkite/pdp-1020-glob-better

Provide new glob library with experiment

EXPERIMENTS.md

@@ -38,6 +38,7 @@ After repository checkout, resolve `BUILDKITE_COMMIT` to a commit hash. This mak
 **Status**: broadly useful, we'd like this to be the standard behaviour in 4.0. 👍👍
 
 ### `kubernetes-exec`
+
 Modifies `start` and `bootstrap` in such a way that they can run in separate Kubernetes containers in the same pod.
 
 Currently, this experiment is being used by [agent-stack-k8s](https://github.com/buildkite/agent-stack-k8s).
@@ -99,6 +100,20 @@ https://github.com/buildkite/agent/blob/40b8a5f3794b04bd64da6e2527857add849a35bd
 **Status:** Since the default behaviour is buggy, we will be promoting this to non-experiment soon™️.
 
 ### `isolated-plugin-checkout`
+
 Checks out each plugin to an directory that that is namespaced by the agent name. Thus each agent worker will have an isolated copy of the plugin. This removes the need to lock the plugin checkout directories in a single agent process with spawned workers. However, if your plugin directory is shared between multiple agent processes *with the same agent name* , you may run into race conditions. This is just one reason we recommend you ensure agents have unique names.
 
-***Status:*** Likely to be the default behaviour in the future.
+**Status:** Likely to be the default behaviour in the future.
+
+### `use-zzglob`
+
+Uses a different library for resolving glob expressions used for `artifact upload`.
+The new glob library should resolve a few issues experienced with the old library:
+
+- Because `**` is used to mean "zero or more path segments", `/**/` should match `/`.
+- Directories that cannot match the glob pattern shouldn't be walked while resolving the pattern. Failure to do this makes `artifact upload` difficult to use when run in a directory containing a mix of subdirectories with different permissions.
+- Failures to walk potential file paths should be reported individually.
+
+The new library should handle all syntax supported by the old library, but because of the chance of incompatibilities and bugs, we're providing it via experiment only for now.
+
+**Status:** Since using the old library causes problems, we hope to promote this to be the default soon™️.

agent/artifact_uploader.go

@@ -7,13 +7,15 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/DrJosh9000/zzglob"
 	"github.com/buildkite/agent/v3/api"
 	"github.com/buildkite/agent/v3/internal/artifact"
 	"github.com/buildkite/agent/v3/internal/experiments"
@@ -22,7 +24,7 @@ import (
 	"github.com/buildkite/agent/v3/pool"
 	"github.com/buildkite/roko"
 	"github.com/dustin/go-humanize"
-	zglob "github.com/mattn/go-zglob"
+	"github.com/mattn/go-zglob"
 )
 
 const (
@@ -125,19 +127,46 @@ func (a *ArtifactUploader) Collect(ctx context.Context) (artifacts []*api.Artifa
 
 		a.logger.Debug("Searching for %s", globPath)
 
-		// Resolve the globs (with * and ** in them), if it's a non-globbed path and doesn't exists
-		// then we will get the ErrNotExist that is handled below
-		globfunc := zglob.Glob
-		if a.conf.GlobResolveFollowSymlinks {
-			// Follow symbolic links for files & directories while expanding globs
-			globfunc = zglob.GlobFollowSymlinks
-		}
-		files, err := globfunc(globPath)
-		if errors.Is(err, os.ErrNotExist) {
-			a.logger.Info("File not found: %s", globPath)
-			continue
-		} else if err != nil {
-			return nil, fmt.Errorf("resolving glob: %w", err)
+		// Resolve the globs (with * and ** in them)
+		var files []string
+		if experiments.IsEnabled(ctx, experiments.UseZZGlob) {
+			// New zzglob library.
+			pattern, err := zzglob.Parse(globPath)
+			if err != nil {
+				return nil, fmt.Errorf("invalid glob pattern: %w", err)
+			}
+
+			walkDirFunc := func(path string, d fs.DirEntry, err error) error {
+				if err != nil {
+					a.logger.Warn("Couldn't walk path %s", path)
+					return nil
+				}
+				if d != nil && d.IsDir() {
+					a.logger.Warn("Glob pattern %s matched a directory %s", globPath, path)
+					return nil
+				}
+				files = append(files, path)
+				return nil
+			}
+			err = pattern.Glob(walkDirFunc, zzglob.TraverseSymlinks(a.conf.GlobResolveFollowSymlinks))
+			if err != nil {
+				return nil, fmt.Errorf("globbing pattern: %w", err)
+			}
+		} else {
+			// Old go-zglob library.
+			globfunc := zglob.Glob
+			if a.conf.GlobResolveFollowSymlinks {
+				// Follow symbolic links for files & directories while expanding globs
+				globfunc = zglob.GlobFollowSymlinks
+			}
+			files, err = globfunc(globPath)
+			if errors.Is(err, os.ErrNotExist) {
+				a.logger.Info("File not found: %s", globPath)
+				continue
+			}
+			if err != nil {
+				return nil, fmt.Errorf("resolving glob: %w", err)
+			}
 		}
 
 		// Process each glob match into an api.Artifact