set an error exit code when the Updater exits with an error (#214)

* return Updater error codes

* too noisy

* this test no longer needed

* exit code errors the update subcommand only

* test for failure

.github/workflows/ci.yml

@@ -23,7 +23,7 @@ jobs:
       - name: Test
         # -count=2 ensures that test fixtures cleanup after themselves
         # because any leftover state will generally cause the second run to fail.
-        run: go test -shuffle=on -count=2 -race -cover -v -timeout=5m ./...
+        run: go test -shuffle=on -count=2 -race -cover -timeout=5m ./...
 
   lint:
     runs-on: ubuntu-latest

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

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)

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 != "" {
@@ -407,6 +411,10 @@ func runContainers(ctx context.Context, params RunParams) (err error) {
 		if err := updater.RunCmd(ctx, cmd, dependabot, userEnv(prox.url, params.ApiUrl)...); err != nil {
 			return err
 		}
+		// If the exit code is non-zero, error when using the `update` subcommand, but not the `test` subcommand.
+		if params.Expected == nil && *updater.ExitCode != 0 {
+			return fmt.Errorf("updater exited with code %d", *updater.ExitCode)
+		}
 	}
 
 	return nil

internal/infra/run_test.go

@@ -1,10 +1,7 @@
 package infra
 
 import (
-	"archive/tar"
-	"bytes"
 	"context"
-	"io"
 	"net/http"
 	"os"
 	"reflect"
@@ -13,11 +10,7 @@ import (
 
 	"github.com/dependabot/cli/internal/server"
 
-	"gopkg.in/yaml.v3"
-
 	"github.com/dependabot/cli/internal/model"
-	"github.com/docker/docker/api/types"
-	"github.com/moby/moby/client"
 )
 
 func Test_checkCredAccess(t *testing.T) {
@@ -166,169 +159,3 @@ func Test_generateIgnoreConditions(t *testing.T) {
 		}
 	})
 }
-
-func TestRun(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping integration test")
-	}
-
-	cli, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	ctx := context.Background()
-
-	var buildContext bytes.Buffer
-	tw := tar.NewWriter(&buildContext)
-	_ = addFileToArchive(tw, "/Dockerfile", 0644, dockerFile)
-	_ = addFileToArchive(tw, "/test_main.go", 0644, testMain)
-	_ = tw.Close()
-
-	UpdaterImageName := "test-updater"
-	resp, err := cli.ImageBuild(ctx, &buildContext, types.ImageBuildOptions{Tags: []string{UpdaterImageName}})
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	_, _ = io.Copy(io.Discard, resp.Body)
-	_ = resp.Body.Close()
-
-	defer func() {
-		_, _ = cli.ImageRemove(ctx, UpdaterImageName, types.ImageRemoveOptions{})
-	}()
-
-	cred := model.Credential{
-		"type":     "git_source",
-		"host":     "github.com",
-		"username": "x-access-token",
-		"password": "$LOCAL_GITHUB_ACCESS_TOKEN",
-	}
-
-	os.Setenv("LOCAL_GITHUB_ACCESS_TOKEN", "test-token")
-	err = Run(RunParams{
-		PullImages: true,
-		Job: &model.Job{
-			PackageManager: "ecosystem",
-			Source: model.Source{
-				Repo: "org/name",
-			},
-		},
-		Creds:        []model.Credential{cred},
-		UpdaterImage: UpdaterImageName,
-		Output:       "out.yaml",
-	})
-	if err != nil {
-		t.Error(err)
-	}
-
-	f, err := os.Open("out.yaml")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer f.Close()
-
-	var output model.Scenario
-	if err = yaml.NewDecoder(f).Decode(&output); err != nil {
-		t.Fatal(err)
-	}
-
-	if !reflect.DeepEqual(output.Input.Credentials, []model.Credential{cred}) {
-		t.Error("unexpected credentials", output.Input.Credentials)
-	}
-	if output.Input.Credentials[0]["password"] != "$LOCAL_GITHUB_ACCESS_TOKEN" {
-		t.Error("expected password to be masked")
-	}
-}
-
-const dockerFile = `
-FROM golang:1.21
-
-# needed to run update-ca-certificates
-RUN apt-get update && apt-get install -y ca-certificates
-# cli will try to start as dependabot
-RUN useradd -ms /bin/bash dependabot
-
-# need to be the user for permissions to work
-USER dependabot
-WORKDIR /home/dependabot
-COPY *.go .
-# cli runs bin/run (twice) to do an update, so put exe there
-RUN go mod init cli_test && go mod tidy && go build -o bin/run
-`
-
-const testMain = `package main
-
-import (
-	"bytes"
-	"context"
-	"encoding/xml"
-	"log"
-	"net"
-	"net/http"
-	"os"
-	"os/exec"
-	"strings"
-	"sync"
-	"time"
-)
-
-func main() {
-	if os.Args[1] == "update_files" {
-		return
-	}
-	var wg sync.WaitGroup
-
-	// print the line that fails
-	log.SetFlags(log.Lshortfile)
-
-	go connectivityCheck(&wg)
-	checkIfRoot()
-	proxyCheck()
-
-	wg.Wait()
-}
-
-func checkIfRoot() {
-	buf := &bytes.Buffer{}
-	cmd := exec.Command("id", "-u")
-	cmd.Stdout = buf
-	if err := cmd.Run(); err != nil {
-		log.Fatalln(err)
-	}
-	userID := strings.TrimSpace(buf.String())
-	if userID == "0" {
-		log.Fatalln("User is root")
-	}
-}
-
-func connectivityCheck(wg *sync.WaitGroup) {
-	wg.Add(1)
-
-	var d net.Dialer
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
-	defer cancel()
-
-	_, err := d.DialContext(ctx, "tcp", "1.1.1.1:22")
-	if err != nil && err.Error() != "dial tcp 1.1.1.1:22: i/o timeout" {
-		log.Fatalln(err)
-	}
-	if err == nil {
-		log.Fatalln("a connection shouldn't be possible")
-	}
-
-	wg.Done()
-}
-
-func proxyCheck() {
-	res, err := http.Get("https://example.com")
-	if err != nil {
-		log.Fatalln(err)
-	}
-	defer res.Body.Close()
-	var v any
-	if err = xml.NewDecoder(res.Body).Decode(&v); err != nil {
-		log.Fatalln(err)
-	}
-}
-`

internal/infra/updater.go

@@ -38,6 +38,9 @@ const (
 type Updater struct {
 	cli         *client.Client
 	containerID string
+
+	// ExitCode is set once an Updater command has completed.
+	ExitCode *int
 }
 
 const (
@@ -251,19 +254,27 @@ 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)
+	}
+
+	u.ExitCode = &execInspect.ExitCode
+
 	return nil
 }
 
@@ -282,10 +293,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.

testdata/scripts/fail.txt

@@ -0,0 +1,37 @@
+exec docker build -t fail-updater .
+
+! dependabot update go_modules dependabot/cli --updater-image fail-updater
+stderr 'updater failure: updater exited with code 2'
+
+# Assert that the test command doesn't fail if the updater fails
+dependabot test -f input.yml --updater-image fail-updater
+
+exec docker rmi -f fail-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
+
+-- input.yml --
+input:
+    job:
+        package-manager: go_modules
+        source:
+            repo: dependabot/cli
+            directory: /
+output:
+    -

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