Build changeset specs server side (#661)

This PR changes src exec's behavior to not upload changeset specs anymore. They aren't required anymore. 

See main PR for more details: https://github.com/sourcegraph/sourcegraph/pull/28158

Author: eseliger

cmd/src/batch_common.go

@@ -250,9 +250,9 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 
 	var workspaceCreator workspace.Creator
 
-	if svc.HasDockerImages(batchSpec) {
+	if len(batchSpec.Steps) > 0 {
 		ui.PreparingContainerImages()
-		images, err := svc.EnsureDockerImages(ctx, batchSpec, ui.PreparingContainerImagesProgress)
+		images, err := svc.EnsureDockerImages(ctx, batchSpec.Steps, ui.PreparingContainerImagesProgress)
 		if err != nil {
 			return err
 		}

cmd/src/batch_exec.go

@@ -8,7 +8,6 @@ import (
 	"io"
 
 	"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"
@@ -109,27 +108,11 @@ func executeBatchSpecInWorkspaces(ctx context.Context, ui *ui.JSONLines, opts ex
 	// we can convert it to a RepoWorkspace and build a task only for that one.
 	repoWorkspace := convertWorkspace(input.Workspace)
 
-	// Parse the raw batch spec contained in the input
-	ui.ParsingBatchSpec()
-	batchSpec, err := svc.ParseBatchSpec([]byte(input.RawSpec))
-	if err != nil {
-		var multiErr *multierror.Error
-		if errors.As(err, &multiErr) {
-			ui.ParsingBatchSpecFailure(multiErr)
-			return cmderrors.ExitCode(2, nil)
-		} else {
-			// This shouldn't happen; let's just punt and let the normal
-			// rendering occur.
-			return err
-		}
-	}
-	ui.ParsingBatchSpecSuccess()
-
 	var workspaceCreator workspace.Creator
 
-	if svc.HasDockerImages(batchSpec) {
+	if len(input.Workspace.Steps) > 0 {
 		ui.PreparingContainerImages()
-		images, err := svc.EnsureDockerImages(ctx, batchSpec, ui.PreparingContainerImagesProgress)
+		images, err := svc.EnsureDockerImages(ctx, input.Workspace.Steps, ui.PreparingContainerImagesProgress)
 		if err != nil {
 			return err
 		}
@@ -162,13 +145,13 @@ func executeBatchSpecInWorkspaces(ctx context.Context, ui *ui.JSONLines, opts ex
 	// `src batch exec` uses server-side caching for changeset specs, so we
 	// only need to call `CheckStepResultsCache` to make sure that per-step cache entries
 	// are loaded and set on the tasks.
-	tasks := svc.BuildTasks(ctx, batchSpec, []service.RepoWorkspace{repoWorkspace})
+	tasks := svc.BuildTasks(ctx, input.Spec, []service.RepoWorkspace{repoWorkspace})
 	if err := coord.CheckStepResultsCache(ctx, tasks); err != nil {
 		return err
 	}
 
 	taskExecUI := ui.ExecutingTasks(*verbose, opts.flags.parallelism)
-	specs, _, err := coord.Execute(ctx, tasks, batchSpec, taskExecUI)
+	_, _, err = coord.Execute(ctx, tasks, input.Spec, taskExecUI)
 	if err == nil || opts.flags.skipErrors {
 		if err == nil {
 			taskExecUI.Success()
@@ -181,18 +164,6 @@ func executeBatchSpecInWorkspaces(ctx context.Context, ui *ui.JSONLines, opts ex
 			return err
 		}
 	}
-	ids := make([]graphql.ChangesetSpecID, len(specs))
-
-	ui.UploadingChangesetSpecs(len(specs))
-	for i, spec := range specs {
-		id, err := svc.CreateChangesetSpec(ctx, spec)
-		if err != nil {
-			return err
-		}
-		ids[i] = id
-		ui.UploadingChangesetSpecsProgress(i+1, len(specs))
-	}
-	ui.UploadingChangesetSpecsSuccess(ids)
 
 	return nil
 }

go.mod

@@ -20,7 +20,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-20211126224101-2ada4911722e
+	github.com/sourcegraph/sourcegraph/lib v0.0.0-20211129161320-80f1543dd742
 	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-20210805182204-aaa1db679c0d
@@ -30,7 +30,7 @@ require (
 
 require (
 	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
-	github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f // indirect
+	github.com/cockroachdb/logtags v0.0.0-20211118104740-dabe8e521a4f // indirect
 	github.com/cockroachdb/sentry-go v0.6.1-cockroachdb.2 // indirect
 	github.com/ghodss/yaml v1.0.0 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect

go.sum

@@ -25,8 +25,9 @@ github.com/cockroachdb/datadriven v1.0.0/go.mod h1:5Ib8Meh+jk1RlHIXej6Pzevx/NLlN
 github.com/cockroachdb/errors v1.6.1/go.mod h1:tm6FTP5G81vwJ5lC0SizQo374JNCOPrHyXGitRJoDqM=
 github.com/cockroachdb/errors v1.8.6 h1:Am9evxl/po3RzpokemQvq7S7Cd0mxv24xy0B/trlQF4=
 github.com/cockroachdb/errors v1.8.6/go.mod h1:hOm5fabihW+xEyY1kuypGwqT+Vt7rafg04ytBtIpeIQ=
-github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f h1:o/kfcElHqOiXqcou5a3rIlMc7oJbMQkeLk0VQJ7zgqY=
 github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI=
+github.com/cockroachdb/logtags v0.0.0-20211118104740-dabe8e521a4f h1:6jduT9Hfc0njg5jJ1DdKCFPdMBrp/mdZfCpa5h+WM74=
+github.com/cockroachdb/logtags v0.0.0-20211118104740-dabe8e521a4f/go.mod h1:Vz9DsVWQQhf3vs21MhPMZpMGSht7O/2vFW2xusFUVOs=
 github.com/cockroachdb/redact v1.1.1/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
 github.com/cockroachdb/redact v1.1.3 h1:AKZds10rFSIj7qADf0g46UixK8NNLwWTNdCIGS5wfSQ=
 github.com/cockroachdb/redact v1.1.3/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
@@ -258,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-20211126224101-2ada4911722e h1:BZpmejyF9KR5h7U9U/lfg4PfcPQZc29fuNGn0c1kT+c=
-github.com/sourcegraph/sourcegraph/lib v0.0.0-20211126224101-2ada4911722e/go.mod h1:4WdX5odo9YL0GD0Wg9OZJJtIJIn1jIsh+UWrQ4MEcaw=
+github.com/sourcegraph/sourcegraph/lib v0.0.0-20211129161320-80f1543dd742 h1:8IegMj6lV20zSCt4f+fjOKRD7ef3/hONH9MmZ9OMYp4=
+github.com/sourcegraph/sourcegraph/lib v0.0.0-20211129161320-80f1543dd742/go.mod h1:4WdX5odo9YL0GD0Wg9OZJJtIJIn1jIsh+UWrQ4MEcaw=
 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.go

@@ -174,9 +174,8 @@ func (c *Coordinator) checkCacheForTask(ctx context.Context, task *Task) (specs
 
 func (c Coordinator) buildChangesetSpecs(task *Task, result execution.Result) ([]*batcheslib.ChangesetSpec, error) {
 	input := &batcheslib.ChangesetSpecInput{
-		BaseRepositoryID: task.Repository.ID,
-		HeadRepositoryID: task.Repository.ID,
-		Repository: batcheslib.ChangesetSpecRepository{
+		Repository: batcheslib.Repository{
+			ID:          task.Repository.ID,
 			Name:        task.Repository.Name,
 			FileMatches: task.Repository.SortedFileMatches(),
 			BaseRef:     task.Repository.BaseRef(),

internal/batches/executor/coordinator_test.go

@@ -376,9 +376,9 @@ func TestCoordinator_Execute_StepCaching(t *testing.T) {
 			Path:         "",
 		},
 		stepResults: []execution.AfterStepResult{
-			{StepIndex: 0, Diff: []byte(`step-0-diff`)},
-			{StepIndex: 1, Diff: []byte(`step-1-diff`)},
-			{StepIndex: 2, Diff: []byte(`step-2-diff`)},
+			{StepIndex: 0, Diff: `step-0-diff`},
+			{StepIndex: 1, Diff: `step-1-diff`},
+			{StepIndex: 2, Diff: `step-2-diff`},
 		},
 	}}
 

internal/batches/executor/executor_test.go

@@ -480,14 +480,14 @@ func TestExecutor_CachedStepResults(t *testing.T) {
 			},
 		}
 
-		cachedDiff := []byte(`diff --git README.md README.md
+		cachedDiff := `diff --git README.md README.md
 index 02a19af..c9644dd 100644
 --- README.md
 +++ README.md
 @@ -1 +1,2 @@
  # Welcome to the README
 +foobar
-`)
+`
 
 		task := &Task{
 			BatchChangeAttributes: &template.BatchChangeAttributes{},
@@ -516,7 +516,7 @@ index 02a19af..c9644dd 100644
 		// We want the diff to be the same as the cached one, since we only had to
 		// execute a single step
 		executionResult := results[0].result
-		if diff := cmp.Diff(executionResult.Diff, string(cachedDiff)); diff != "" {
+		if diff := cmp.Diff(executionResult.Diff, cachedDiff); diff != "" {
 			t.Fatalf("wrong diff: %s", diff)
 		}
 
@@ -543,7 +543,7 @@ This repository is used to test opening and closing pull request with Automation
 			},
 		}
 
-		cachedDiff := []byte(`diff --git README.md README.md
+		cachedDiff := `diff --git README.md README.md
 index 1914491..cd2ccbf 100644
 --- README.md
 +++ README.md
@@ -562,7 +562,7 @@ index 0000000..888e1ec
 +++ README.txt
 @@ -0,0 +1 @@
 +this is step 1
-`)
+`
 
 		wantFinalDiff := `diff --git README.md README.md
 index 1914491..d6782d3 100644

internal/batches/executor/run_steps.go

@@ -78,12 +78,12 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
 		// If we have cached results and don't need to execute any more steps,
 		// we can quit
 		if lastStep == len(opts.task.Steps)-1 {
-			changes, err := git.ChangesInDiff(opts.task.CachedResult.Diff)
+			changes, err := git.ChangesInDiff([]byte(opts.task.CachedResult.Diff))
 			if err != nil {
 				return execResult, nil, errors.Wrap(err, "parsing cached step diff")
 			}
 
-			execResult.Diff = string(opts.task.CachedResult.Diff)
+			execResult.Diff = opts.task.CachedResult.Diff
 			execResult.ChangedFiles = &changes
 			stepResults = append(stepResults, opts.task.CachedResult)
 
@@ -119,7 +119,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
 			stepContext.Steps.Changes = previousStepResult.Files
 			stepContext.Outputs = opts.task.CachedResult.Outputs
 
-			if err := workspace.ApplyDiff(ctx, opts.task.CachedResult.Diff); err != nil {
+			if err := workspace.ApplyDiff(ctx, []byte(opts.task.CachedResult.Diff)); err != nil {
 				return execResult, nil, errors.Wrap(err, "getting changed files in step")
 			}
 		}
@@ -178,7 +178,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
 		}
 		stepResult := execution.AfterStepResult{
 			StepIndex:          i,
-			Diff:               stepDiff,
+			Diff:               string(stepDiff),
 			Outputs:            make(map[string]interface{}),
 			PreviousStepResult: stepContext.PreviousStep,
 		}

internal/batches/executor/ui.go

@@ -48,7 +48,7 @@ type StepsExecutionUI interface {
 	CalculatingDiffStarted()
 	CalculatingDiffFinished()
 
-	StepFinished(idx int, diff []byte, changes *git.Changes, outputs map[string]interface{})
+	StepFinished(idx int, diff string, changes *git.Changes, outputs map[string]interface{})
 	StepFailed(idx int, err error, exitCode int)
 }
 
@@ -70,7 +70,7 @@ func (noop NoopStepsExecUI) StepOutputWriter(ctx context.Context, task *Task, st
 }
 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) StepFinished(idx int, diff string, changes *git.Changes, outputs map[string]interface{}) {
 }
 func (noop NoopStepsExecUI) StepFailed(idx int, err error, exitCode int) {
 }

internal/batches/service/service.go

@@ -158,30 +158,26 @@ func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *batcheslib.Ch
 //
 // 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) EnsureDockerImages(ctx context.Context, spec *batcheslib.BatchSpec, progress func(done, total int)) (map[string]docker.Image, error) {
-	total := len(spec.Steps)
+func (svc *Service) EnsureDockerImages(ctx context.Context, steps []batcheslib.Step, progress func(done, total int)) (map[string]docker.Image, error) {
+	total := len(steps)
 	progress(0, total)
 
 	// TODO: this _really_ should be parallelised, since the image cache takes
 	// care to only pull the same image once.
 	images := make(map[string]docker.Image)
-	for i := range spec.Steps {
-		img, err := svc.EnsureImage(ctx, spec.Steps[i].Container)
+	for i := range steps {
+		img, err := svc.EnsureImage(ctx, steps[i].Container)
 		if err != nil {
 			return nil, err
 		}
-		images[spec.Steps[i].Container] = img
+		images[steps[i].Container] = img
 
 		progress(i+1, total)
 	}
 
 	return images, nil
 }
 
-func (svc *Service) HasDockerImages(spec *batcheslib.BatchSpec) bool {
-	return len(spec.Steps) > 0
-}
-
 func (svc *Service) EnsureImage(ctx context.Context, name string) (docker.Image, error) {
 	img := svc.imageCache.Get(name)