Merge pull request #3068 from buildkite/pipe-358-shell-refactor

Shell package cleanup

agent/job_runner.go

@@ -14,8 +14,9 @@ import (
 
 	"github.com/buildkite/agent/v3/api"
 	"github.com/buildkite/agent/v3/core"
+	"github.com/buildkite/agent/v3/env"
 	"github.com/buildkite/agent/v3/internal/experiments"
-	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/shell"
 	"github.com/buildkite/agent/v3/kubernetes"
 	"github.com/buildkite/agent/v3/logger"
 	"github.com/buildkite/agent/v3/metrics"
@@ -638,7 +639,9 @@ func (w LogWriter) Write(bytes []byte) (int, error) {
 func (r *JobRunner) executePreBootstrapHook(ctx context.Context, hook string) (bool, error) {
 	r.agentLogger.Info("Running pre-bootstrap hook %q", hook)
 
-	sh, err := shell.New()
+	sh, err := shell.New(
+		shell.WithStdout(LogWriter{l: r.agentLogger}),
+	)
 	if err != nil {
 		return false, err
 	}
@@ -648,25 +651,26 @@ func (r *JobRunner) executePreBootstrapHook(ctx context.Context, hook string) (b
 	// - Env files are designed to be validated by the pre-bootstrap hook
 	// - The pre-bootstrap hook may want to create annotations, so it can also
 	//   have a few necessary and global args as env vars.
-	sh.Env.Set("BUILDKITE_ENV_FILE", r.envShellFile.Name())
-	sh.Env.Set("BUILDKITE_ENV_JSON_FILE", r.envJSONFile.Name())
+	environ := env.New()
+	environ.Set("BUILDKITE_ENV_FILE", r.envShellFile.Name())
+	environ.Set("BUILDKITE_ENV_JSON_FILE", r.envJSONFile.Name())
+	environ.Set("BUILDKITE_JOB_ID", r.conf.Job.ID)
 	apiConfig := r.apiClient.Config()
-	sh.Env.Set("BUILDKITE_JOB_ID", r.conf.Job.ID)
-	sh.Env.Set("BUILDKITE_AGENT_ACCESS_TOKEN", apiConfig.Token)
-	sh.Env.Set("BUILDKITE_AGENT_ENDPOINT", apiConfig.Endpoint)
-	sh.Env.Set("BUILDKITE_NO_HTTP2", fmt.Sprint(apiConfig.DisableHTTP2))
-	sh.Env.Set("BUILDKITE_AGENT_DEBUG", fmt.Sprint(r.conf.Debug))
-	sh.Env.Set("BUILDKITE_AGENT_DEBUG_HTTP", fmt.Sprint(r.conf.DebugHTTP))
+	environ.Set("BUILDKITE_AGENT_ACCESS_TOKEN", apiConfig.Token)
+	environ.Set("BUILDKITE_AGENT_ENDPOINT", apiConfig.Endpoint)
+	environ.Set("BUILDKITE_NO_HTTP2", fmt.Sprint(apiConfig.DisableHTTP2))
+	environ.Set("BUILDKITE_AGENT_DEBUG", fmt.Sprint(r.conf.Debug))
+	environ.Set("BUILDKITE_AGENT_DEBUG_HTTP", fmt.Sprint(r.conf.DebugHTTP))
 
-	sh.Writer = LogWriter{
-		l: r.agentLogger,
+	script, err := sh.Script(hook)
+	if err != nil {
+		r.agentLogger.Error("Finished pre-bootstrap hook %q: script not runnable: %v", hook, err)
+		return false, err
 	}
-
-	if err := sh.RunScript(ctx, hook, nil); err != nil {
-		r.agentLogger.Error("Finished pre-bootstrap hook %q: job rejected: %s", hook, err)
+	if err := script.Run(ctx, shell.ShowPrompt(false), shell.WithExtraEnv(environ)); err != nil {
+		r.agentLogger.Error("Finished pre-bootstrap hook %q: job rejected: %v", hook, err)
 		return false, err
 	}
-
 	r.agentLogger.Info("Finished pre-bootstrap hook %q: job accepted", hook)
 	return true, nil
 }

clicommand/agent_start.go

@@ -29,8 +29,8 @@ import (
 	awssigner "github.com/buildkite/agent/v3/internal/cryptosigner/aws"
 	"github.com/buildkite/agent/v3/internal/experiments"
 	"github.com/buildkite/agent/v3/internal/job/hook"
-	"github.com/buildkite/agent/v3/internal/job/shell"
 	"github.com/buildkite/agent/v3/internal/osutil"
+	"github.com/buildkite/agent/v3/internal/shell"
 	"github.com/buildkite/agent/v3/logger"
 	"github.com/buildkite/agent/v3/metrics"
 	"github.com/buildkite/agent/v3/process"
@@ -1351,16 +1351,18 @@ func agentLifecycleHook(hookName string, log logger.Logger, cfg AgentStartConfig
 		}
 		return nil
 	}
-	sh, err := shell.New()
+
+	// pipe from hook output to logger
+	r, w := io.Pipe()
+	sh, err := shell.New(
+		shell.WithStdout(w),
+		shell.WithLogger(shell.NewWriterLogger(w, !cfg.NoColor, nil)), // for Promptf
+	)
 	if err != nil {
 		log.Error("creating shell for %q hook: %v", hookName, err)
 		return err
 	}
 
-	// pipe from hook output to logger
-	r, w := io.Pipe()
-	sh.Logger = shell.NewWriterLogger(w, !cfg.NoColor, nil) // for Promptf
-	sh.Writer = w                                           // for stdout+stderr
 	var wg sync.WaitGroup
 	wg.Add(1)
 	go func() {
@@ -1373,8 +1375,14 @@ func agentLifecycleHook(hookName string, log logger.Logger, cfg AgentStartConfig
 	}()
 
 	// run hook
+	script, err := sh.Script(p)
+	if err != nil {
+		log.Error("%q hook: %v", hookName, err)
+		return err
+	}
+	// For these hooks, hide the interpreter from the "prompt".
 	sh.Promptf("%s", p)
-	if err = sh.RunScript(context.Background(), p, nil); err != nil {
+	if err := script.Run(context.TODO(), shell.ShowPrompt(false)); err != nil {
 		log.Error("%q hook: %v", hookName, err)
 		return err
 	}

internal/file/is_opened.go

@@ -5,7 +5,7 @@ import (
 	"os"
 	"strconv"
 
-	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/shell"
 )
 
 // IsOpened returns true if the file at the given path is opened by the current process.

internal/file/opened_by.go

@@ -8,7 +8,7 @@ import (
 	"regexp"
 	"strconv"
 
-	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/shell"
 )
 
 const stderrFd = 2

internal/job/artifacts.go

@@ -69,7 +69,7 @@ func (e *Executor) uploadArtifacts(ctx context.Context) error {
 		args = append(args, e.ArtifactUploadDestination)
 	}
 
-	if err = e.shell.Run(ctx, "buildkite-agent", args...); err != nil {
+	if err = e.shell.Command("buildkite-agent", args...).Run(ctx); err != nil {
 		return err
 	}
 

internal/job/checkout.go

@@ -10,8 +10,8 @@ import (
 	"time"
 
 	"github.com/buildkite/agent/v3/internal/experiments"
-	"github.com/buildkite/agent/v3/internal/job/shell"
 	"github.com/buildkite/agent/v3/internal/osutil"
+	"github.com/buildkite/agent/v3/internal/shell"
 	"github.com/buildkite/agent/v3/tracetools"
 	"github.com/buildkite/roko"
 )
@@ -26,7 +26,7 @@ func (e *Executor) configureGitCredentialHelper(ctx context.Context) error {
 	// This is important for the case where a user clones multiple repos in a step - ie, if we always crammed
 	// os.Getenv("BUILDKITE_REPO") into credential helper, we'd only ever get a token for the repo that the step is running
 	// in, and not for any other repos that the step might clone.
-	err := e.shell.RunWithoutPrompt(ctx, "git", "config", "--global", "credential.useHttpPath", "true")
+	err := e.shell.Command("git", "config", "--global", "credential.useHttpPath", "true").Run(ctx, shell.ShowPrompt(false))
 	if err != nil {
 		return fmt.Errorf("enabling git credential.useHttpPath: %w", err)
 	}
@@ -37,7 +37,7 @@ func (e *Executor) configureGitCredentialHelper(ctx context.Context) error {
 	}
 
 	helper := fmt.Sprintf(`%s git-credentials-helper`, buildkiteAgent)
-	err = e.shell.RunWithoutPrompt(ctx, "git", "config", "--global", "credential.helper", helper)
+	err = e.shell.Command("git", "config", "--global", "credential.helper", helper).Run(ctx, shell.ShowPrompt(false))
 	if err != nil {
 		return fmt.Errorf("configuring git credential.helper: %w", err)
 	}
@@ -48,9 +48,9 @@ func (e *Executor) configureGitCredentialHelper(ctx context.Context) error {
 // Disables SSH keyscan and configures git to use HTTPS instead of SSH for github.
 // We may later expand this for other SCMs.
 func (e *Executor) configureHTTPSInsteadOfSSH(ctx context.Context) error {
-	return e.shell.RunWithoutPrompt(ctx,
+	return e.shell.Command(
 		"git", "config", "--global", "url.https://github.com/.insteadOf", "git@github.com:",
-	)
+	).Run(ctx, shell.ShowPrompt(false))
 }
 
 func (e *Executor) removeCheckoutDir() error {
@@ -167,7 +167,7 @@ func (e *Executor) CheckoutPhase(ctx context.Context) error {
 
 			var errLockTimeout ErrTimedOutAcquiringLock
 			switch {
-			case shell.IsExitError(err) && shell.GetExitCode(err) == -1:
+			case shell.IsExitError(err) && shell.ExitCode(err) == -1:
 				e.shell.Warningf("Checkout was interrupted by a signal")
 				r.Break()
 
@@ -270,7 +270,7 @@ func hasGitSubmodules(sh *shell.Shell) bool {
 
 func hasGitCommit(ctx context.Context, sh *shell.Shell, gitDir string, commit string) bool {
 	// Resolve commit to an actual commit object
-	output, err := sh.RunAndCapture(ctx, "git", "--git-dir", gitDir, "rev-parse", commit+"^{commit}")
+	output, err := sh.Command("git", "--git-dir", gitDir, "rev-parse", commit+"^{commit}").RunAndCaptureStdout(ctx)
 	if err != nil {
 		return false
 	}
@@ -382,12 +382,14 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri
 			e.shell.Commentf("Fetch and mirror pull request head from GitHub")
 			refspec := fmt.Sprintf("refs/pull/%s/head", e.PullRequest)
 			// Fetch the PR head from the upstream repository into the mirror.
-			if err := e.shell.Run(ctx, "git", "--git-dir", mirrorDir, "fetch", "origin", refspec); err != nil {
+			cmd := e.shell.Command("git", "--git-dir", mirrorDir, "fetch", "origin", refspec)
+			if err := cmd.Run(ctx); err != nil {
 				return "", err
 			}
 		} else {
 			// Fetch the build branch from the upstream repository into the mirror.
-			if err := e.shell.Run(ctx, "git", "--git-dir", mirrorDir, "fetch", "origin", e.Branch); err != nil {
+			cmd := e.shell.Command("git", "--git-dir", mirrorDir, "fetch", "origin", e.Branch)
+			if err := cmd.Run(ctx); err != nil {
 				return "", err
 			}
 		}
@@ -400,7 +402,8 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri
 		// a clean host or with a clean checkout.)
 		// TODO: Investigate getting the ref from the main repo and passing
 		// that in here.
-		if err := e.shell.Run(ctx, "git", "--git-dir", mirrorDir, "fetch", "origin"); err != nil {
+		cmd := e.shell.Command("git", "--git-dir", mirrorDir, "fetch", "origin")
+		if err := cmd.Run(ctx); err != nil {
 			return "", err
 		}
 	}
@@ -409,10 +412,10 @@ func (e *Executor) updateGitMirror(ctx context.Context, repository string) (stri
 		// Let's opportunistically fsck and gc.
 		// 1. In case of remote URL confusion (bug introduced in #1959), and
 		// 2. There's possibly some object churn when remotes are renamed.
-		if err := e.shell.Run(ctx, "git", "--git-dir", mirrorDir, "fsck"); err != nil {
+		if err := e.shell.Command("git", "--git-dir", mirrorDir, "fsck").Run(ctx); err != nil {
 			e.shell.Logger.Warningf("Couldn't run git fsck: %v", err)
 		}
-		if err := e.shell.Run(ctx, "git", "--git-dir", mirrorDir, "gc"); err != nil {
+		if err := e.shell.Command("git", "--git-dir", mirrorDir, "gc").Run(ctx); err != nil {
 			e.shell.Logger.Warningf("Couldn't run git gc: %v", err)
 		}
 	}
@@ -445,7 +448,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 	if gitDir != "" {
 		args = append([]string{"--git-dir", gitDir}, args...)
 	}
-	gotURL, err := e.shell.RunAndCapture(ctx, "git", args...)
+	gotURL, err := e.shell.Command("git", args...).RunAndCaptureStdout(ctx)
 	if err != nil {
 		return false, err
 	}
@@ -468,7 +471,7 @@ func (e *Executor) updateRemoteURL(ctx context.Context, gitDir, repository strin
 	if gitDir != "" {
 		args = append([]string{"--git-dir", gitDir}, args...)
 	}
-	return true, e.shell.Run(ctx, "git", args...)
+	return true, e.shell.Command("git", args...).Run(ctx)
 }
 
 func (e *Executor) getOrUpdateMirrorDir(ctx context.Context, repository string) (string, error) {
@@ -577,7 +580,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
 			return fmt.Errorf("fetching PR refspec %q: %w", refspec, err)
 		}
 
-		gitFetchHead, _ := e.shell.RunAndCapture(ctx, "git", "rev-parse", "FETCH_HEAD")
+		gitFetchHead, _ := e.shell.Command("git", "rev-parse", "FETCH_HEAD").RunAndCaptureStdout(ctx)
 		e.shell.Commentf("FETCH_HEAD is now `%s`", gitFetchHead)
 
 		if e.Commit != "HEAD" {
@@ -631,8 +634,8 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
 		// is only available in git version 1.8.1, so
 		// if the call fails, continue the job
 		// script, and show an informative error.
-		if err := e.shell.Run(ctx, "git", "submodule", "sync", "--recursive"); err != nil {
-			gitVersionOutput, _ := e.shell.RunAndCapture(ctx, "git", "--version")
+		if err := e.shell.Command("git", "submodule", "sync", "--recursive").Run(ctx); err != nil {
+			gitVersionOutput, _ := e.shell.Command("git", "--version").RunAndCaptureStdout(ctx)
 			e.shell.Warningf("Failed to recursively sync git submodules. This is most likely because you have an older version of git installed (" + gitVersionOutput + ") and you need version 1.8.1 and above. If you're using submodules, it's highly recommended you upgrade if you can.")
 		}
 
@@ -684,19 +687,20 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
 					submoduleArgs = append(submoduleArgs, "submodule", "update", "--init", "--recursive", "--force")
 				}
 
-				if err := e.shell.Run(ctx, "git", submoduleArgs...); err != nil {
+				if err := e.shell.Command("git", submoduleArgs...).Run(ctx); err != nil {
 					return fmt.Errorf("updating submodules: %w", err)
 				}
 			}
 
 			if !mirrorSubmodules {
 				args = append(args, "submodule", "update", "--init", "--recursive", "--force")
-				if err := e.shell.Run(ctx, "git", args...); err != nil {
+				if err := e.shell.Command("git", args...).Run(ctx); err != nil {
 					return fmt.Errorf("updating submodules: %w", err)
 				}
 			}
 
-			if err := e.shell.Run(ctx, "git", "submodule", "foreach", "--recursive", "git reset --hard"); err != nil {
+			cmd := e.shell.Command("git", "submodule", "foreach", "--recursive", "git reset --hard")
+			if err := cmd.Run(ctx); err != nil {
 				return fmt.Errorf("resetting submodules: %w", err)
 			}
 		}
@@ -754,7 +758,7 @@ func gitFetchCommitWithFallback(ctx context.Context, shell *shell.Shell, gitFetc
 	// reachable from a fetches branch. git 1.9.0+ changed `--tags` to
 	// fetch all tags in addition to the default refspec, but pre 1.9.0 it
 	// excludes the default refspec.
-	gitFetchRefspec, err := shell.RunAndCapture(ctx, "git", "config", "remote.origin.fetch")
+	gitFetchRefspec, err := shell.Command("git", "config", "remote.origin.fetch").RunAndCaptureStdout(ctx)
 	if err != nil {
 		return fmt.Errorf("getting remote.origin.fetch: %w", err)
 	}
@@ -777,7 +781,8 @@ const CommitMetadataKey = "buildkite:git:commit"
 // note that we bail early if the key already exists, as we don't want to overwrite it
 func (e *Executor) sendCommitToBuildkite(ctx context.Context) error {
 	e.shell.Commentf("Checking to see if git commit information needs to be sent to Buildkite...")
-	if err := e.shell.Run(ctx, "buildkite-agent", "meta-data", "exists", CommitMetadataKey); err == nil {
+	cmd := e.shell.Command("buildkite-agent", "meta-data", "exists", CommitMetadataKey)
+	if err := cmd.Run(ctx); err == nil {
 		// Command exited 0, ie the key exists, so we don't need to send it again
 		e.shell.Commentf("Git commit information has already been sent to Buildkite")
 		return nil
@@ -803,13 +808,14 @@ func (e *Executor) sendCommitToBuildkite(ctx context.Context) error {
 		"--no-color",
 		"--format=commit %H%nabbrev-commit %h%nAuthor: %an <%ae>%n%n%w(0,4,4)%B",
 	}
-	out, err := e.shell.RunAndCapture(ctx, "git", gitArgs...)
+	out, err := e.shell.Command("git", gitArgs...).RunAndCaptureStdout(ctx)
 	if err != nil {
 		return fmt.Errorf("getting git commit information: %w", err)
 	}
 
 	stdin := strings.NewReader(out)
-	if err := e.shell.WithStdin(stdin).Run(ctx, "buildkite-agent", "meta-data", "set", CommitMetadataKey); err != nil {
+	cmd = e.shell.CloneWithStdin(stdin).Command("buildkite-agent", "meta-data", "set", CommitMetadataKey)
+	if err := cmd.Run(ctx); err != nil {
 		return fmt.Errorf("sending git commit information to Buildkite: %w", err)
 	}
 
@@ -822,7 +828,7 @@ func (e *Executor) resolveCommit(ctx context.Context) {
 		e.shell.Warningf("BUILDKITE_COMMIT was empty")
 		return
 	}
-	cmdOut, err := e.shell.RunAndCapture(ctx, "git", "rev-parse", commitRef)
+	cmdOut, err := e.shell.Command("git", "rev-parse", commitRef).RunAndCaptureStdout(ctx)
 	if err != nil {
 		e.shell.Warningf("Error running git rev-parse %q: %v", commitRef, err)
 		return

internal/job/docker.go

@@ -6,7 +6,7 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/buildkite/agent/v3/internal/job/shell"
+	"github.com/buildkite/agent/v3/internal/shell"
 )
 
 var dockerEnv = []string{
@@ -61,7 +61,7 @@ func tearDownDeprecatedDockerIntegration(ctx context.Context, sh *shell.Shell) e
 	if container, ok := sh.Env.Get("DOCKER_CONTAINER"); ok {
 		sh.Printf("~~~ Cleaning up Docker containers")
 
-		if err := sh.Run(ctx, "docker", "rm", "-f", "-v", container); err != nil {
+		if err := sh.Command("docker", "rm", "-f", "-v", container).Run(ctx); err != nil {
 			return err
 		}
 	} else if projectName, ok := sh.Env.Get("COMPOSE_PROJ_NAME"); ok {
@@ -98,12 +98,14 @@ func runDockerCommand(ctx context.Context, sh *shell.Shell, cmd []string) error
 	sh.Env.Set("DOCKER_IMAGE", dockerImage)
 
 	sh.Printf("~~~ :docker: Building Docker image %s", dockerImage)
-	if err := sh.Run(ctx, "docker", "build", "-f", dockerFile, "-t", dockerImage, "."); err != nil {
+	shCmd := sh.Command("docker", "build", "-f", dockerFile, "-t", dockerImage, ".")
+	if err := shCmd.Run(ctx); err != nil {
 		return err
 	}
 
 	sh.Headerf(":docker: Running command (in Docker container)")
-	if err := sh.Run(ctx, "docker", append([]string{"run", "--name", dockerContainer, dockerImage}, cmd...)...); err != nil {
+	shCmd = sh.Command("docker", append([]string{"run", "--name", dockerContainer, dockerImage}, cmd...)...)
+	if err := shCmd.Run(ctx); err != nil {
 		return err
 	}
 
@@ -159,5 +161,5 @@ func runDockerCompose(ctx context.Context, sh *shell.Shell, projectName string,
 	}
 
 	args = append(args, commandArgs...)
-	return sh.Run(ctx, "docker-compose", args...)
+	return sh.Command("docker-compose", args...).Run(ctx)
 }