Fix macOS panic by pulling images before determining workspace creator (#522)

I introduced this when re-organizing the code in `batch_common.go`, but
I didn't run into this because I've only developed on Linux since.

On macOS, though, `src` would panic when the
`DockerVolumeWorkspaceCreator` would check the `ImageUIDGUID` on each
step, because `step.image` was nil.

This fixes the issue by making sure that all images are pulled before
inspecting them.

As a requirement to the change it also pulls out the pulling of the
workspace creator image, which I think is actually an improvement
because it makes the code less decoupled.

Guess I should switch between macOS and Linux more often.

Author: mrnugget

cmd/src/batch_common.go

@@ -234,24 +234,30 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) error {
 	}
 	batchCompletePending(pending, "Resolving namespace")
 
-	pending = batchCreatePending(opts.out, "Determining workspace type")
-	workspaceCreator := workspace.NewCreator(ctx, opts.flags.workspace, opts.flags.cacheDir, opts.flags.tempDir, batchSpec.Steps)
-	pending.VerboseLine(output.Linef("🚧", output.StyleSuccess, "Workspace creator: %T", workspaceCreator))
-	batchCompletePending(pending, "Set workspace type")
-
-	loadWorkspaceImage := workspaceCreator.Type() == workspace.CreatorTypeVolume
 	imageProgress := opts.out.Progress([]output.ProgressBar{{
 		Label: "Preparing container images",
 		Max:   1.0,
 	}}, nil)
-	err = svc.SetDockerImages(ctx, batchSpec, loadWorkspaceImage, func(perc float64) {
+	err = svc.SetDockerImages(ctx, batchSpec, func(perc float64) {
 		imageProgress.SetValue(0, perc)
 	})
 	if err != nil {
 		return err
 	}
 	imageProgress.Complete()
 
+	pending = batchCreatePending(opts.out, "Determining workspace type")
+	workspaceCreator := workspace.NewCreator(ctx, opts.flags.workspace, opts.flags.cacheDir, opts.flags.tempDir, batchSpec.Steps)
+	if workspaceCreator.Type() == workspace.CreatorTypeVolume {
+		_, err = svc.EnsureImage(ctx, workspace.DockerVolumeWorkspaceImage)
+		if err != nil {
+			return err
+		}
+	}
+
+	pending.VerboseLine(output.Linef("🚧", output.StyleSuccess, "Workspace creator: %T", workspaceCreator))
+	batchCompletePending(pending, "Set workspace type")
+
 	pending = batchCreatePending(opts.out, "Resolving repositories")
 	repos, err := svc.ResolveRepositories(ctx, batchSpec)
 	if err != nil {

internal/batches/service/service.go

@@ -21,7 +21,6 @@ import (
 	"github.com/sourcegraph/src-cli/internal/batches/docker"
 	"github.com/sourcegraph/src-cli/internal/batches/executor"
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
-	"github.com/sourcegraph/src-cli/internal/batches/workspace"
 )
 
 type Service struct {
@@ -155,32 +154,36 @@ func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *batches.Chang
 //
 // Progress information is reported back to the given progress function: perc
 // will be a value between 0.0 and 1.0, inclusive.
-func (svc *Service) SetDockerImages(ctx context.Context, spec *batches.BatchSpec, loadWorkspaceImage bool, progress func(perc float64)) error {
+func (svc *Service) SetDockerImages(ctx context.Context, spec *batches.BatchSpec, progress func(perc float64)) error {
 	total := len(spec.Steps) + 1
 	progress(0)
 
 	// TODO: this _really_ should be parallelised, since the image cache takes
 	// care to only pull the same image once.
 	for i := range spec.Steps {
-		spec.Steps[i].SetImage(svc.imageCache.Get(spec.Steps[i].Container))
-		if err := spec.Steps[i].EnsureImage(ctx); err != nil {
-			return errors.Wrapf(err, "pulling image %q", spec.Steps[i].Container)
+		img, err := svc.EnsureImage(ctx, spec.Steps[i].Container)
+		if err != nil {
+			return err
 		}
-		progress(float64(i) / float64(total))
-	}
+		spec.Steps[i].SetImage(img)
 
-	// We also need to ensure we have our own utility images available, if
-	// necessary.
-	if loadWorkspaceImage {
-		if err := svc.imageCache.Get(workspace.DockerVolumeWorkspaceImage).Ensure(ctx); err != nil {
-			return errors.Wrapf(err, "pulling image %q", workspace.DockerVolumeWorkspaceImage)
-		}
+		progress(float64(i) / float64(total))
 	}
 
 	progress(1)
 	return nil
 }
 
+func (svc *Service) EnsureImage(ctx context.Context, name string) (docker.Image, error) {
+	img := svc.imageCache.Get(name)
+
+	if err := img.Ensure(ctx); err != nil {
+		return nil, errors.Wrapf(err, "pulling image %q", name)
+	}
+
+	return img, nil
+}
+
 func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, spec *batches.BatchSpec) ([]*executor.Task, error) {
 	workspaceConfigs := []batches.WorkspaceConfiguration{}
 	for _, conf := range spec.Workspaces {