From 926303ee8e0eaed11d2547f755b13fcf51e7209a Mon Sep 17 00:00:00 2001
From: Jake Coffman <jake@jakecoffman.com>
Date: Fri, 22 Dec 2023 10:08:50 -0600
Subject: [PATCH] return Updater error codes

---
 cmd/dependabot/internal/cmd/update.go |  2 +-
 internal/infra/proxy.go               |  2 +-
 internal/infra/run.go                 |  6 ++++-
 internal/infra/updater.go             | 34 +++++++++++++++++++++++----
 testdata/scripts/crash.txt            | 24 +++++++++++++++++++
 testdata/scripts/local.txt            |  2 +-
 6 files changed, 61 insertions(+), 9 deletions(-)
 create mode 100644 testdata/scripts/crash.txt

diff --git a/cmd/dependabot/internal/cmd/update.go b/cmd/dependabot/internal/cmd/update.go
index ed0200a..dad52ec 100644
--- a/cmd/dependabot/internal/cmd/update.go
+++ b/cmd/dependabot/internal/cmd/update.go
@@ -95,7 +95,7 @@ func NewUpdateCommand() *cobra.Command {
 				if errors.Is(err, context.DeadlineExceeded) {
 					log.Fatalf("update timed out after %s", flags.timeout)
 				}
-				log.Fatalf("failed to run updater: %v", err)
+				log.Fatalf("updater failure: %v", err)
 			}
 
 			return nil
diff --git a/internal/infra/proxy.go b/internal/infra/proxy.go
index f64a305..2b758a9 100644
--- a/internal/infra/proxy.go
+++ b/internal/infra/proxy.go
@@ -177,7 +177,7 @@ func (p *Proxy) Close() (err error) {
 	}()
 
 	// Check the error code if the container has already exited, so we can pass it along to the caller. If the proxy
-	//crashes we want the CLI to error out. Unlike the Updater it should never stop on its own.
+	//crashes we want the CLI to error out.
 	containerInfo, inspectErr := p.cli.ContainerInspect(context.Background(), p.containerID)
 	if inspectErr != nil {
 		return fmt.Errorf("failed to inspect proxy container: %w", inspectErr)
diff --git a/internal/infra/run.go b/internal/infra/run.go
index 30772cb..806eab2 100644
--- a/internal/infra/run.go
+++ b/internal/infra/run.go
@@ -389,7 +389,11 @@ func runContainers(ctx context.Context, params RunParams) (err error) {
 	if err != nil {
 		return err
 	}
-	defer updater.Close()
+	defer func() {
+		if updaterErr := updater.Close(); updaterErr != nil {
+			err = updaterErr
+		}
+	}()
 
 	// put the clone dir in the updater container to be used by during the update
 	if params.LocalDir != "" {
diff --git a/internal/infra/updater.go b/internal/infra/updater.go
index ca24130..5de3904 100644
--- a/internal/infra/updater.go
+++ b/internal/infra/updater.go
@@ -251,19 +251,29 @@ func (u *Updater) RunCmd(ctx context.Context, cmd, user string, env ...string) e
 		_, _ = io.Copy(os.Stderr, prefixer.New(r, "updater | "))
 	}()
 
-	// blocks until update is complete or ctl-c
 	ch := make(chan struct{})
 	go func() {
 		_, _ = stdcopy.StdCopy(w, w, execResp.Reader)
 		ch <- struct{}{}
 	}()
 
+	// blocks until update is complete or ctl-c
 	select {
 	case <-ctx.Done():
 		return ctx.Err()
 	case <-ch:
 	}
 
+	// check the exit code of the command
+	execInspect, err := u.cli.ContainerExecInspect(ctx, execCreate.ID)
+	if err != nil {
+		return fmt.Errorf("failed to inspect exec: %w", err)
+	}
+
+	if execInspect.ExitCode != 0 {
+		return fmt.Errorf("updater exited with code: %v", execInspect.ExitCode)
+	}
+
 	return nil
 }
 
@@ -282,10 +292,24 @@ func (u *Updater) Wait(ctx context.Context, condition container.WaitCondition) e
 }
 
 // Close kills and deletes the container and deletes updater mount paths related to the run.
-func (u *Updater) Close() error {
-	return u.cli.ContainerRemove(context.Background(), u.containerID, types.ContainerRemoveOptions{
-		Force: true,
-	})
+func (u *Updater) Close() (err error) {
+	defer func() {
+		removeErr := u.cli.ContainerRemove(context.Background(), u.containerID, types.ContainerRemoveOptions{Force: true})
+		if removeErr != nil {
+			err = fmt.Errorf("failed to remove proxy container: %w", removeErr)
+		}
+	}()
+
+	// Handle non-zero exit codes.
+	containerInfo, inspectErr := u.cli.ContainerInspect(context.Background(), u.containerID)
+	if inspectErr != nil {
+		return fmt.Errorf("failed to inspect proxy container: %w", inspectErr)
+	}
+	if containerInfo.State.ExitCode != 0 {
+		return fmt.Errorf("updater container exited with non-zero exit code: %d", containerInfo.State.ExitCode)
+	}
+
+	return
 }
 
 // JobFile  is the payload passed to file updater containers.
diff --git a/testdata/scripts/crash.txt b/testdata/scripts/crash.txt
new file mode 100644
index 0000000..3e586b0
--- /dev/null
+++ b/testdata/scripts/crash.txt
@@ -0,0 +1,24 @@
+exec docker build -t crashy-updater .
+
+! dependabot update go_modules dependabot/cli --updater-image crashy-updater
+stderr 'updater exited with code: 2'
+
+exec docker rmi -f crashy-updater
+
+-- Dockerfile --
+FROM ubuntu:22.04
+
+RUN useradd dependabot
+
+COPY --chown=dependabot --chmod=755 update-ca-certificates /usr/bin/update-ca-certificates
+COPY --chown=dependabot --chmod=755 run bin/run
+
+-- update-ca-certificates --
+#!/usr/bin/env bash
+
+echo "Updating CA certificates..."
+
+-- run --
+#!/usr/bin/env bash
+
+exit 2
diff --git a/testdata/scripts/local.txt b/testdata/scripts/local.txt
index 1e3fc0f..37ce8f3 100644
--- a/testdata/scripts/local.txt
+++ b/testdata/scripts/local.txt
@@ -1,7 +1,7 @@
 exec docker build -qt local-updater .
 
 # The ls command in run will fail since this isn't a real updater
-dependabot update go_modules dependabot-fixtures/go-modules-lib --updater-image local-updater
+! dependabot update go_modules dependabot-fixtures/go-modules-lib --updater-image local-updater
 stderr 'No such file or directory'
 
 # The local flag should create the repo directory and put my-repo in it