Cleanup and complete json logs a bit (#623)

Required for new SSBC step resolvers to have all data needed.

Author: eseliger

cmd/src/batch_common.go

@@ -312,9 +312,17 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 	if err != nil && !opts.flags.skipErrors {
 		return err
 	}
-	taskExecUI.Success()
-	if err != nil && opts.flags.skipErrors {
-		opts.ui.ExecutingTasksSkippingErrors(err)
+	if err == nil || opts.flags.skipErrors {
+		if err == nil {
+			taskExecUI.Success()
+		} else {
+			opts.ui.ExecutingTasksSkippingErrors(err)
+		}
+	} else {
+		if err != nil {
+			taskExecUI.Failed(err)
+			return err
+		}
 	}
 
 	if len(logFiles) > 0 && opts.flags.keepLogs {
@@ -349,22 +357,23 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 
 	opts.ui.CreatingBatchSpec()
 	id, url, err := svc.CreateBatchSpec(ctx, namespace, rawSpec, ids)
-	opts.ui.CreatingBatchSpecSuccess()
 	if err != nil {
 		return opts.ui.CreatingBatchSpecError(err)
 	}
+	previewURL := cfg.Endpoint + url
+	opts.ui.CreatingBatchSpecSuccess(previewURL)
 
-	if opts.applyBatchSpec {
-		opts.ui.ApplyingBatchSpec()
-		batch, err := svc.ApplyBatchChange(ctx, id)
-		if err != nil {
-			return err
-		}
-		opts.ui.ApplyingBatchSpecSuccess(cfg.Endpoint + batch.URL)
+	if !opts.applyBatchSpec {
+		opts.ui.PreviewBatchSpec(previewURL)
+		return
+	}
 
-	} else {
-		opts.ui.PreviewBatchSpec(cfg.Endpoint + url)
+	opts.ui.ApplyingBatchSpec()
+	batch, err := svc.ApplyBatchChange(ctx, id)
+	if err != nil {
+		return err
 	}
+	opts.ui.ApplyingBatchSpecSuccess(cfg.Endpoint + batch.URL)
 
 	return nil
 }

cmd/src/batch_exec.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/cockroachdb/errors"
 	"github.com/hashicorp/go-multierror"
+
 	"github.com/sourcegraph/src-cli/internal/batches/executor"
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 	"github.com/sourcegraph/src-cli/internal/batches/service"
@@ -164,12 +165,17 @@ func executeBatchSpecInWorkspaces(ctx context.Context, opts executeBatchSpecOpts
 
 	taskExecUI := opts.ui.ExecutingTasks(*verbose, opts.flags.parallelism)
 	freshSpecs, _, err := coord.Execute(ctx, uncachedTasks, batchSpec, taskExecUI)
-	if err != nil && !opts.flags.skipErrors {
-		return err
-	}
-	taskExecUI.Success()
-	if err != nil && opts.flags.skipErrors {
-		opts.ui.ExecutingTasksSkippingErrors(err)
+	if err == nil || opts.flags.skipErrors {
+		if err == nil {
+			taskExecUI.Success()
+		} else {
+			opts.ui.ExecutingTasksSkippingErrors(err)
+		}
+	} else {
+		if err != nil {
+			taskExecUI.Failed(err)
+			return err
+		}
 	}
 
 	specs := append(cachedSpecs, freshSpecs...)

go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/pkg/browser v0.0.0-20180916011732-0a3d74bf9ce4
 	github.com/sourcegraph/go-diff v0.6.1
 	github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf
-	github.com/sourcegraph/sourcegraph/lib v0.0.0-20210920114500-3eabeb8208ef
+	github.com/sourcegraph/sourcegraph/lib v0.0.0-20210929121718-d5efc9330247
 	github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf // indirect
 	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
 	golang.org/x/net v0.0.0-20210614182718-04defd469f4e

go.sum

@@ -259,8 +259,8 @@ github.com/sourcegraph/go-diff v0.6.1 h1:hmA1LzxW0n1c3Q4YbrFgg4P99GSnebYa3x8gr0H
 github.com/sourcegraph/go-diff v0.6.1/go.mod h1:iBszgVvyxdc8SFZ7gm69go2KDdt3ag071iBaWPF6cjs=
 github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf h1:oAdWFqhStsWiiMP/vkkHiMXqFXzl1XfUNOdxKJbd6bI=
 github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf/go.mod h1:ppFaPm6kpcHnZGqQTFhUIAQRIEhdQDWP1PCv4/ON354=
-github.com/sourcegraph/sourcegraph/lib v0.0.0-20210920114500-3eabeb8208ef h1:NVbGTt8WfgPflixd3d1AFKZt+UgOevTE3rUz+8HEM6A=
-github.com/sourcegraph/sourcegraph/lib v0.0.0-20210920114500-3eabeb8208ef/go.mod h1:9vLg5fQL9Q3iktfqu96OrswbOq7yqMesMJER9DCy8vk=
+github.com/sourcegraph/sourcegraph/lib v0.0.0-20210929121718-d5efc9330247 h1:U1qXWB+6i/YeQ73kirisIEsxCBgNXw4td4QYdYlBeKM=
+github.com/sourcegraph/sourcegraph/lib v0.0.0-20210929121718-d5efc9330247/go.mod h1:9vLg5fQL9Q3iktfqu96OrswbOq7yqMesMJER9DCy8vk=
 github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152 h1:z/MpntplPaW6QW95pzcAR/72Z5TWDyDnSo0EOcyij9o=
 github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152/go.mod h1:GIjDIg/heH5DOkXY3YJ/wNhfHsQHoXGjl8G8amsYQ1I=
 github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=

internal/batches/executor/coordinator_test.go

@@ -519,8 +519,9 @@ type dummyTaskExecutionUI struct {
 	specs           map[*Task][]*batcheslib.ChangesetSpec
 }
 
-func (d *dummyTaskExecutionUI) Start([]*Task) {}
-func (d *dummyTaskExecutionUI) Success()      {}
+func (d *dummyTaskExecutionUI) Start([]*Task)    {}
+func (d *dummyTaskExecutionUI) Success()         {}
+func (d *dummyTaskExecutionUI) Failed(err error) {}
 func (d *dummyTaskExecutionUI) TaskStarted(t *Task) {
 	d.mu.Lock()
 	defer d.mu.Unlock()

internal/batches/executor/run_steps.go

@@ -73,11 +73,11 @@ type executionOpts struct {
 func runSteps(ctx context.Context, opts *executionOpts) (result executionResult, stepResults []stepExecutionResult, err error) {
 	opts.ui.ArchiveDownloadStarted()
 	err = opts.task.Archive.Ensure(ctx)
+	opts.ui.ArchiveDownloadFinished(err)
 	if err != nil {
 		return executionResult{}, nil, errors.Wrap(err, "fetching repo")
 	}
 	defer opts.task.Archive.Close()
-	opts.ui.ArchiveDownloadFinished()
 
 	opts.ui.WorkspaceInitializationStarted()
 	workspace, err := opts.wc.Create(ctx, opts.task.Repository, opts.task.Steps, opts.task.Archive)
@@ -167,6 +167,16 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult,
 			return execResult, nil, err
 		}
 		stdoutBuffer, stderrBuffer, err := executeSingleStep(ctx, opts, workspace, i, step, digest, &stepContext)
+		defer func() {
+			if err != nil {
+				exitCode := -1
+				sfe := &stepFailedErr{}
+				if errors.As(err, sfe) {
+					exitCode = sfe.ExitCode
+				}
+				opts.ui.StepFailed(i+1, err, exitCode)
+			}
+		}()
 		if err != nil {
 			return execResult, nil, err
 		}
@@ -233,44 +243,55 @@ func executeSingleStep(
 	// ----------
 	// PREPARATION
 	// ----------
-	opts.ui.StepPreparing(i + 1)
+	opts.ui.StepPreparingStart(i + 1)
 
 	cidFile, cleanup, err := createCidFile(ctx, opts.tempDir, util.SlugForRepo(opts.task.Repository.Name, opts.task.Repository.Rev()))
 	if err != nil {
+		opts.ui.StepPreparingFailed(i+1, err)
 		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 	defer cleanup()
 
 	// For now, we only support shell scripts provided via the Run field.
 	shell, containerTemp, err := probeImageForShell(ctx, imageDigest)
 	if err != nil {
-		return bytes.Buffer{}, bytes.Buffer{}, errors.Wrapf(err, "probing image %q for shell", step.Container)
+		err = errors.Wrapf(err, "probing image %q for shell", step.Container)
+		opts.ui.StepPreparingFailed(i+1, err)
+		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 
 	runScriptFile, runScript, cleanup, err := createRunScriptFile(ctx, opts.tempDir, step.Run, stepContext)
 	if err != nil {
+		opts.ui.StepPreparingFailed(i+1, err)
 		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 	defer cleanup()
 
 	// Parse and render the step.Files.
 	filesToMount, cleanup, err := createFilesToMount(opts.tempDir, step, stepContext)
 	if err != nil {
+		opts.ui.StepPreparingFailed(i+1, err)
 		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 	defer cleanup()
 
 	// Resolve step.Env given the current environment.
 	stepEnv, err := step.Env.Resolve(os.Environ())
 	if err != nil {
-		return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "resolving step environment")
+		err = errors.Wrap(err, "resolving step environment")
+		opts.ui.StepPreparingFailed(i+1, err)
+		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 	// Render the step.Env variables as templates.
 	env, err := template.RenderStepMap(stepEnv, stepContext)
 	if err != nil {
-		return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "parsing step environment")
+		err = errors.Wrap(err, "parsing step environment")
+		opts.ui.StepPreparingFailed(i+1, err)
+		return bytes.Buffer{}, bytes.Buffer{}, err
 	}
 
+	opts.ui.StepPreparingSuccess(i + 1)
+
 	// ----------
 	// EXECUTION
 	// ----------
@@ -330,8 +351,14 @@ func executeSingleStep(
 	}
 
 	newStepFailedErr := func(wrappedErr error) stepFailedErr {
+		exitCode := -1
+		exitErr := &exec.ExitError{}
+		if errors.As(wrappedErr, &exitErr) {
+			exitCode = exitErr.ExitCode()
+		}
 		return stepFailedErr{
 			Err:         wrappedErr,
+			ExitCode:    exitCode,
 			Args:        cmd.Args,
 			Run:         runScript,
 			Container:   step.Container,
@@ -570,7 +597,9 @@ type stepFailedErr struct {
 	Stdout string
 	Stderr string
 
-	Err error
+	// ExitCode of the command, or -1 if a non-command error occured.
+	ExitCode int
+	Err      error
 }
 
 func (e stepFailedErr) Cause() error { return e.Err }
@@ -607,8 +636,8 @@ func (e stepFailedErr) Error() string {
 		printOutput(e.Stderr)
 	}
 
-	if exitErr, ok := e.Err.(*exec.ExitError); ok {
-		out.WriteString(fmt.Sprintf("\nCommand failed with exit code %d.", exitErr.ExitCode()))
+	if e.ExitCode != -1 {
+		out.WriteString(fmt.Sprintf("\nCommand failed with exit code %d.", e.ExitCode))
 	} else {
 		out.WriteString(fmt.Sprintf("\nCommand failed: %s", e.Err))
 	}

internal/batches/executor/ui.go

@@ -11,6 +11,7 @@ import (
 type TaskExecutionUI interface {
 	Start([]*Task)
 	Success()
+	Failed(err error)
 
 	TaskStarted(*Task)
 	TaskFinished(*Task, error)
@@ -28,7 +29,7 @@ type StepOutputWriter interface {
 
 type StepsExecutionUI interface {
 	ArchiveDownloadStarted()
-	ArchiveDownloadFinished()
+	ArchiveDownloadFinished(error)
 
 	WorkspaceInitializationStarted()
 	WorkspaceInitializationFinished()
@@ -37,7 +38,9 @@ type StepsExecutionUI interface {
 
 	StepSkipped(int)
 
-	StepPreparing(int)
+	StepPreparingStart(int)
+	StepPreparingSuccess(int)
+	StepPreparingFailed(int, error)
 	StepStarted(stepIdx int, runScript string, env map[string]string)
 
 	StepOutputWriter(context.Context, *Task, int) StepOutputWriter
@@ -46,18 +49,21 @@ type StepsExecutionUI interface {
 	CalculatingDiffFinished()
 
 	StepFinished(idx int, diff []byte, changes *git.Changes, outputs map[string]interface{})
+	StepFailed(idx int, err error, exitCode int)
 }
 
 // NoopStepsExecUI is an implementation of StepsExecutionUI that does nothing.
 type NoopStepsExecUI struct{}
 
 func (noop NoopStepsExecUI) ArchiveDownloadStarted()                                       {}
-func (noop NoopStepsExecUI) ArchiveDownloadFinished()                                      {}
+func (noop NoopStepsExecUI) ArchiveDownloadFinished(error)                                 {}
 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) StepPreparingStart(step int)                                   {}
+func (noop NoopStepsExecUI) StepPreparingSuccess(step int)                                 {}
+func (noop NoopStepsExecUI) StepPreparingFailed(step int, err error)                       {}
 func (noop NoopStepsExecUI) StepStarted(step int, runScript string, env map[string]string) {}
 func (noop NoopStepsExecUI) StepOutputWriter(ctx context.Context, task *Task, step int) StepOutputWriter {
 	return NoopStepOutputWriter{}
@@ -66,6 +72,8 @@ func (noop NoopStepsExecUI) CalculatingDiffStarted()  {}
 func (noop NoopStepsExecUI) CalculatingDiffFinished() {}
 func (noop NoopStepsExecUI) StepFinished(idx int, diff []byte, changes *git.Changes, outputs map[string]interface{}) {
 }
+func (noop NoopStepsExecUI) StepFailed(idx int, err error, exitCode int) {
+}
 
 type NoopStepOutputWriter struct{}
 

internal/batches/ui/exec_ui.go

@@ -42,7 +42,7 @@ type ExecUI interface {
 	UploadingChangesetSpecsSuccess(ids []graphql.ChangesetSpecID)
 
 	CreatingBatchSpec()
-	CreatingBatchSpecSuccess()
+	CreatingBatchSpecSuccess(previewURL string)
 	CreatingBatchSpecError(err error) error
 
 	PreviewBatchSpec(previewURL string)