executor UI: extract StepsExecutionUI from TaskExecutionUI and implement it in TUI and JSON (#606)

This is a follow-up to #597 and splits up the existing interface into two.

That allows us to remove all formatting from `runSteps` and `executeSingleStep` and also provide more information for the JSON interface (e.g. when a step is finished).

**Note**: the `OPERATION` and `STATUS` fields in the `logEvents` don't make a lot of sense. Happy to change everything there.

Author: mrnugget

internal/batches/executor/coordinator.go

@@ -2,13 +2,13 @@ package executor
 
 import (
 	"context"
-	"io"
 	"reflect"
 	"strconv"
 	"time"
 
 	"github.com/cockroachdb/errors"
 	"github.com/hashicorp/go-multierror"
+
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 
 	"github.com/sourcegraph/src-cli/internal/api"
@@ -207,22 +207,6 @@ func (c *Coordinator) cacheAndBuildSpec(ctx context.Context, taskResult taskResu
 	return specs, nil
 }
 
-type TaskExecutionUI interface {
-	Start([]*Task)
-	Success()
-
-	TaskStarted(*Task)
-	TaskFinished(*Task, error)
-
-	TaskChangesetSpecsBuilt(*Task, []*batcheslib.ChangesetSpec)
-
-	// TODO: This should be split up into methods that are more specific.
-	TaskCurrentlyExecuting(*Task, string)
-
-	StepStdoutWriter(context.Context, *Task, int) io.WriteCloser
-	StepStderrWriter(context.Context, *Task, int) io.WriteCloser
-}
-
 // Execute executes the given Tasks and the importChangeset statements in the
 // given spec. It regularly calls the executionProgressPrinter with the
 // current TaskStatuses.

internal/batches/executor/coordinator_test.go

@@ -3,14 +3,14 @@ package executor
 import (
 	"context"
 	"fmt"
-	"io"
 	"strings"
 	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/sourcegraph/batch-change-utils/overridable"
+
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/git"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
@@ -545,18 +545,8 @@ func (d *dummyTaskExecutionUI) TaskChangesetSpecsBuilt(t *Task, specs []*batches
 	d.specs[t] = specs
 }
 
-type discardCloser struct {
-	io.Writer
-}
-
-func (discardCloser) Close() error { return nil }
-
-func (d *dummyTaskExecutionUI) TaskCurrentlyExecuting(*Task, string) {}
-func (d *dummyTaskExecutionUI) StepStdoutWriter(ctx context.Context, task *Task, step int) io.WriteCloser {
-	return discardCloser{io.Discard}
-}
-func (d *dummyTaskExecutionUI) StepStderrWriter(ctx context.Context, task *Task, step int) io.WriteCloser {
-	return discardCloser{io.Discard}
+func (d *dummyTaskExecutionUI) StepsExecutionUI(t *Task) StepsExecutionUI {
+	return NoopStepsExecUI{}
 }
 
 var _ taskExecutor = &dummyExecutor{}

internal/batches/executor/executor.go

@@ -174,11 +174,8 @@ func (x *executor) do(ctx context.Context, task *Task, ui TaskExecutionUI) (err
 		wc:          x.opts.Creator,
 		ensureImage: x.opts.EnsureImage,
 		tempDir:     x.opts.TempDir,
-		reportProgress: func(currentlyExecuting string) {
-			ui.TaskCurrentlyExecuting(task, currentlyExecuting)
-		},
-		newUiStdoutWriter: ui.StepStdoutWriter,
-		newUiStderrWriter: ui.StepStderrWriter,
+
+		ui: ui.StepsExecutionUI(task),
 	}
 
 	result, stepResults, err := runSteps(runCtx, opts)

internal/batches/executor/run_steps.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/cockroachdb/errors"
 	"github.com/hashicorp/go-multierror"
+
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/git"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
@@ -64,27 +65,27 @@ type executionOpts struct {
 
 	tempDir string
 
-	logger         log.TaskLogger
-	reportProgress func(string)
+	logger log.TaskLogger
 
-	newUiStdoutWriter func(context.Context, *Task, int) io.WriteCloser
-	newUiStderrWriter func(context.Context, *Task, int) io.WriteCloser
+	ui StepsExecutionUI
 }
 
 func runSteps(ctx context.Context, opts *executionOpts) (result executionResult, stepResults []stepExecutionResult, err error) {
-	opts.reportProgress("Downloading archive")
+	opts.ui.ArchiveDownloadStarted()
 	err = opts.task.Archive.Ensure(ctx)
 	if err != nil {
 		return executionResult{}, nil, errors.Wrap(err, "fetching repo")
 	}
 	defer opts.task.Archive.Close()
+	opts.ui.ArchiveDownloadFinished()
 
-	opts.reportProgress("Initializing workspace")
+	opts.ui.WorkspaceInitializationStarted()
 	workspace, err := opts.wc.Create(ctx, opts.task.Repository, opts.task.Steps, opts.task.Archive)
 	if err != nil {
 		return executionResult{}, nil, errors.Wrap(err, "creating workspace")
 	}
 	defer workspace.Close(ctx)
+	opts.ui.WorkspaceInitializationFinished()
 
 	var (
 		execResult = executionResult{
@@ -118,12 +119,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult,
 			return execResult, stepResults, nil
 		}
 
-		switch startStep {
-		case 1:
-			opts.reportProgress("Skipping step 1. Found cached result.")
-		default:
-			opts.reportProgress(fmt.Sprintf("Skipping steps 1 to %d. Found cached results.", startStep))
-		}
+		opts.ui.SkippingStepsUpto(startStep)
 	}
 
 	for i := startStep; i < len(opts.task.Steps); i++ {
@@ -157,9 +153,10 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult,
 			return execResult, nil, errors.Wrap(err, "evaluating step condition")
 		}
 		if !cond {
-			opts.reportProgress(fmt.Sprintf("Skipping step %d", i+1))
+			opts.ui.StepSkipped(i + 1)
 			continue
 		}
+
 		// We need to grab the digest for the exact image we're using.
 		img, err := opts.ensureImage(ctx, step.Container)
 		if err != nil {
@@ -204,14 +201,18 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult,
 		}
 		stepResults = append(stepResults, stepResult)
 		previousStepResult = result
+
+		opts.ui.StepFinished(i+1, stepResult.Diff, result.Files, stepResult.Outputs)
 	}
 
-	opts.reportProgress("Calculating diff")
+	opts.ui.CalculatingDiffStarted()
 	diffOut, err := workspace.Diff(ctx)
 	if err != nil {
 		return execResult, nil, errors.Wrap(err, "git diff failed")
 	}
 
+	opts.ui.CalculatingDiffFinished()
+
 	execResult.Diff = string(diffOut)
 	execResult.ChangedFiles = previousStepResult.Files
 
@@ -232,7 +233,7 @@ func executeSingleStep(
 	// ----------
 	// PREPARATION
 	// ----------
-	opts.reportProgress(fmt.Sprintf("Preparing step %d", i+1))
+	opts.ui.StepPreparing(i + 1)
 
 	cidFile, cleanup, err := createCidFile(ctx, opts.tempDir, util.SlugForRepo(opts.task.Repository.Name, opts.task.Repository.Rev()))
 	if err != nil {
@@ -274,7 +275,7 @@ func executeSingleStep(
 	// ----------
 	// EXECUTION
 	// ----------
-	opts.reportProgress(runScript)
+	opts.ui.StepStarted(i+1, runScript)
 
 	workspaceOpts, err := workspace.DockerRunOpts(ctx, workDir)
 	if err != nil {
@@ -316,8 +317,8 @@ func executeSingleStep(
 
 	writerCtx, writerCancel := context.WithCancel(ctx)
 	defer writerCancel()
-	uiStdoutWriter := opts.newUiStdoutWriter(writerCtx, opts.task, i)
-	uiStderrWriter := opts.newUiStderrWriter(writerCtx, opts.task, i)
+	uiStdoutWriter := opts.ui.StepStdoutWriter(writerCtx, opts.task, i)
+	uiStderrWriter := opts.ui.StepStderrWriter(writerCtx, opts.task, i)
 	defer func() {
 		uiStdoutWriter.Close()
 		uiStderrWriter.Close()

internal/batches/executor/ui.go

@@ -0,0 +1,72 @@
+package executor
+
+import (
+	"context"
+	"io"
+
+	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
+	"github.com/sourcegraph/sourcegraph/lib/batches/git"
+)
+
+type TaskExecutionUI interface {
+	Start([]*Task)
+	Success()
+
+	TaskStarted(*Task)
+	TaskFinished(*Task, error)
+
+	TaskChangesetSpecsBuilt(*Task, []*batcheslib.ChangesetSpec)
+
+	StepsExecutionUI(*Task) StepsExecutionUI
+}
+
+type StepsExecutionUI interface {
+	ArchiveDownloadStarted()
+	ArchiveDownloadFinished()
+
+	WorkspaceInitializationStarted()
+	WorkspaceInitializationFinished()
+
+	SkippingStepsUpto(int)
+
+	StepSkipped(int)
+
+	StepPreparing(int)
+	StepStarted(int, string)
+
+	StepStdoutWriter(context.Context, *Task, int) io.WriteCloser
+	StepStderrWriter(context.Context, *Task, int) io.WriteCloser
+
+	CalculatingDiffStarted()
+	CalculatingDiffFinished()
+
+	StepFinished(idx int, diff []byte, changes *git.Changes, outputs map[string]interface{})
+}
+
+// NoopStepsExecUI is an implementation of StepsExecutionUI that does nothing.
+type NoopStepsExecUI struct{}
+
+func (noop NoopStepsExecUI) ArchiveDownloadStarted()                {}
+func (noop NoopStepsExecUI) ArchiveDownloadFinished()               {}
+func (noop NoopStepsExecUI) WorkspaceInitializationStarted()        {}
+func (noop NoopStepsExecUI) WorkspaceInitializationFinished()       {}
+func (noop NoopStepsExecUI) SkippingStepsUpto(startStep int)        {}
+func (noop NoopStepsExecUI) StepSkipped(step int)                   {}
+func (noop NoopStepsExecUI) StepPreparing(step int)                 {}
+func (noop NoopStepsExecUI) StepStarted(step int, runScript string) {}
+func (noop NoopStepsExecUI) StepStdoutWriter(ctx context.Context, task *Task, step int) io.WriteCloser {
+	return discardCloser{io.Discard}
+}
+func (noop NoopStepsExecUI) StepStderrWriter(ctx context.Context, task *Task, step int) io.WriteCloser {
+	return discardCloser{io.Discard}
+}
+func (noop NoopStepsExecUI) CalculatingDiffStarted()  {}
+func (noop NoopStepsExecUI) CalculatingDiffFinished() {}
+func (noop NoopStepsExecUI) StepFinished(idx int, diff []byte, changes *git.Changes, outputs map[string]interface{}) {
+}
+
+type discardCloser struct {
+	io.Writer
+}
+
+func (discardCloser) Close() error { return nil }