Cleanup code after refactoring of workspaces/steps

cmd/src/batch_common.go

@@ -279,11 +279,7 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 	if err != nil {
 		return err
 	}
-	tasks, err := svc.BuildTasks(ctx, repos, batchSpec, workspaces)
-	if err != nil {
-		return err
-	}
-	opts.ui.DeterminingWorkspacesSuccess(len(tasks))
+	opts.ui.DeterminingWorkspacesSuccess(len(workspaces))
 
 	// EXECUTION OF TASKS
 	coord := svc.NewCoordinator(executor.NewCoordinatorOpts{
@@ -299,6 +295,7 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error
 	})
 
 	opts.ui.CheckingCache()
+	tasks := svc.BuildTasks(ctx, batchSpec, workspaces)
 	uncachedTasks, cachedSpecs, err := coord.CheckCache(ctx, tasks)
 	if err != nil {
 		return err

internal/batches/service/build_tasks.go

@@ -3,57 +3,42 @@ package service
 import (
 	"context"
 
-	"github.com/pkg/errors"
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
 	"github.com/sourcegraph/src-cli/internal/batches/executor"
-	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 )
 
-// buildTasks returns tasks for all the workspaces determined for the given spec.
-func buildTasks(ctx context.Context, spec *batcheslib.BatchSpec, repos []*graphql.Repository, workspaces []RepoWorkspace) ([]*executor.Task, error) {
-	repoByID := make(map[string]*graphql.Repository)
-	for _, repo := range repos {
-		repoByID[repo.ID] = repo
-	}
+// buildTasks returns *executor.Tasks for all the workspaces determined for the given spec.
+func buildTasks(ctx context.Context, spec *batcheslib.BatchSpec, workspaces []RepoWorkspace) []*executor.Task {
+	tasks := make([]*executor.Task, 0, len(workspaces))
 
-	tasks := []*executor.Task{}
 	for _, ws := range workspaces {
-		repo, ok := repoByID[ws.RepoID]
-		if !ok {
-			return nil, errors.New("invalid state, didn't find repo for workspace definition")
-		}
-
-		t, err := buildTask(spec, repo, ws.Path, ws.OnlyFetchWorkspace, ws.Steps)
-		if err != nil {
-			return nil, err
-		}
-
-		tasks = append(tasks, t)
+		tasks = append(tasks, buildTask(spec, ws))
 	}
 
-	return tasks, nil
+	return tasks
 }
 
-func buildTask(spec *batcheslib.BatchSpec, r *graphql.Repository, path string, onlyWorkspace bool, steps []batcheslib.Step) (*executor.Task, error) {
+func buildTask(spec *batcheslib.BatchSpec, workspace RepoWorkspace) *executor.Task {
 	batchChange := template.BatchChangeAttributes{
 		Name:        spec.Name,
 		Description: spec.Description,
 	}
 
-	// "." means the path is root, but in the executor we use "" to signify root
+	// "." means the path is root, but in the executor we use "" to signify root.
+	path := workspace.Path
 	if path == "." {
 		path = ""
 	}
 
 	return &executor.Task{
-		Repository:         r,
+		Repository:         workspace.Repo,
 		Path:               path,
-		Steps:              steps,
-		OnlyFetchWorkspace: onlyWorkspace,
+		Steps:              workspace.Steps,
+		OnlyFetchWorkspace: workspace.OnlyFetchWorkspace,
 
 		TransformChanges:      spec.TransformChanges,
 		Template:              spec.ChangesetTemplate,
 		BatchChangeAttributes: &batchChange,
-	}, nil
+	}
 }

internal/batches/service/service.go

@@ -186,8 +186,8 @@ func (svc *Service) DetermineWorkspaces(ctx context.Context, repos []*graphql.Re
 	return findWorkspaces(ctx, spec, svc, repos)
 }
 
-func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, spec *batcheslib.BatchSpec, workspaces []RepoWorkspace) ([]*executor.Task, error) {
-	return buildTasks(ctx, spec, repos, workspaces)
+func (svc *Service) BuildTasks(ctx context.Context, spec *batcheslib.BatchSpec, workspaces []RepoWorkspace) []*executor.Task {
+	return buildTasks(ctx, spec, workspaces)
 }
 
 func (svc *Service) NewCoordinator(opts executor.NewCoordinatorOpts) *executor.Coordinator {

internal/batches/service/workspaces.go

@@ -6,7 +6,6 @@ import (
 	"sort"
 
 	"github.com/gobwas/glob"
-	"github.com/pkg/errors"
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
 	"github.com/sourcegraph/src-cli/internal/batches"
@@ -15,7 +14,7 @@ import (
 )
 
 type RepoWorkspace struct {
-	RepoID             string
+	Repo               *graphql.Repository
 	Path               string
 	Steps              []batcheslib.Step
 	OnlyFetchWorkspace bool
@@ -80,7 +79,7 @@ func findWorkspaces(
 	}
 
 	type repoWorkspaces struct {
-		RepoID             string
+		Repo               *graphql.Repository
 		Paths              []string
 		OnlyFetchWorkspace bool
 	}
@@ -98,7 +97,7 @@ func findWorkspaces(
 				continue
 			}
 			workspacesByID[repo.ID] = repoWorkspaces{
-				RepoID:             repo.ID,
+				Repo:               repo,
 				Paths:              dirs,
 				OnlyFetchWorkspace: conf.OnlyFetchWorkspace,
 			}
@@ -110,7 +109,7 @@ func findWorkspaces(
 		conf, ok := workspacesByID[repo.ID]
 		if !ok {
 			workspacesByID[repo.ID] = repoWorkspaces{
-				RepoID:             repo.ID,
+				Repo:               repo,
 				Paths:              []string{""},
 				OnlyFetchWorkspace: false,
 			}
@@ -127,12 +126,7 @@ func findWorkspaces(
 				fetchWorkspace = false
 			}
 
-			repo, ok := repoByID[workspace.RepoID]
-			if !ok {
-				return nil, errors.New("invalid state, repo not found")
-			}
-
-			steps, err := stepsForRepo(spec, repo)
+			steps, err := stepsForRepo(spec, workspace.Repo)
 			if err != nil {
 				return nil, err
 			}
@@ -143,7 +137,7 @@ func findWorkspaces(
 			}
 
 			workspaces = append(workspaces, RepoWorkspace{
-				RepoID:             workspace.RepoID,
+				Repo:               workspace.Repo,
 				Path:               path,
 				Steps:              steps,
 				OnlyFetchWorkspace: fetchWorkspace,
@@ -153,10 +147,10 @@ func findWorkspaces(
 
 	// Stable sorting.
 	sort.Slice(workspaces, func(i, j int) bool {
-		if workspaces[i].RepoID == workspaces[j].RepoID {
+		if workspaces[i].Repo.Name == workspaces[j].Repo.Name {
 			return workspaces[i].Path < workspaces[j].Path
 		}
-		return workspaces[i].RepoID < workspaces[j].RepoID
+		return workspaces[i].Repo.Name < workspaces[j].Repo.Name
 	})
 
 	return workspaces, nil

internal/batches/service/workspaces_test.go

@@ -2,6 +2,7 @@ package service
 
 import (
 	"context"
+	"sort"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -31,9 +32,9 @@ func TestFindWorkspaces(t *testing.T) {
 			spec:          &batcheslib.BatchSpec{Steps: steps},
 			finderResults: finderResults{},
 			wantWorkspaces: []RepoWorkspace{
-				{RepoID: repos[0].ID, Steps: steps, Path: ""},
-				{RepoID: repos[1].ID, Steps: steps, Path: ""},
-				{RepoID: repos[2].ID, Steps: steps, Path: ""},
+				{Repo: repos[0], Steps: steps, Path: ""},
+				{Repo: repos[1], Steps: steps, Path: ""},
+				{Repo: repos[2], Steps: steps, Path: ""},
 			},
 		},
 
@@ -46,9 +47,9 @@ func TestFindWorkspaces(t *testing.T) {
 			},
 			finderResults: finderResults{},
 			wantWorkspaces: []RepoWorkspace{
-				{RepoID: repos[0].ID, Steps: steps, Path: ""},
-				{RepoID: repos[1].ID, Steps: steps, Path: ""},
-				{RepoID: repos[2].ID, Steps: steps, Path: ""},
+				{Repo: repos[0], Steps: steps, Path: ""},
+				{Repo: repos[1], Steps: steps, Path: ""},
+				{Repo: repos[2], Steps: steps, Path: ""},
 			},
 		},
 
@@ -64,7 +65,7 @@ func TestFindWorkspaces(t *testing.T) {
 				repos[2]: []string{},
 			},
 			wantWorkspaces: []RepoWorkspace{
-				{RepoID: repos[1].ID, Steps: steps, Path: ""},
+				{Repo: repos[1], Steps: steps, Path: ""},
 			},
 		},
 
@@ -80,13 +81,13 @@ func TestFindWorkspaces(t *testing.T) {
 				repos[2]: {"a/b", "a/b/c", "d/e/f"},
 			},
 			wantWorkspaces: []RepoWorkspace{
-				{RepoID: repos[0].ID, Steps: steps, Path: "a/b"},
-				{RepoID: repos[0].ID, Steps: steps, Path: "a/b/c"},
-				{RepoID: repos[0].ID, Steps: steps, Path: "d/e/f"},
-				{RepoID: repos[1].ID, Steps: steps, Path: ""},
-				{RepoID: repos[2].ID, Steps: steps, Path: "a/b"},
-				{RepoID: repos[2].ID, Steps: steps, Path: "a/b/c"},
-				{RepoID: repos[2].ID, Steps: steps, Path: "d/e/f"},
+				{Repo: repos[0], Steps: steps, Path: "a/b"},
+				{Repo: repos[0], Steps: steps, Path: "a/b/c"},
+				{Repo: repos[0], Steps: steps, Path: "d/e/f"},
+				{Repo: repos[1], Steps: steps, Path: ""},
+				{Repo: repos[2], Steps: steps, Path: "a/b"},
+				{Repo: repos[2], Steps: steps, Path: "a/b/c"},
+				{Repo: repos[2], Steps: steps, Path: "d/e/f"},
 			},
 		},
 
@@ -106,13 +107,13 @@ func TestFindWorkspaces(t *testing.T) {
 				repos[2]: {"a/b", "a/b/c", "d/e/f"},
 			},
 			wantWorkspaces: []RepoWorkspace{
-				{RepoID: repos[0].ID, Steps: steps, Path: "a/b", OnlyFetchWorkspace: true},
-				{RepoID: repos[0].ID, Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true},
-				{RepoID: repos[0].ID, Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true},
-				{RepoID: repos[1].ID, Steps: steps, Path: ""},
-				{RepoID: repos[2].ID, Steps: steps, Path: "a/b", OnlyFetchWorkspace: true},
-				{RepoID: repos[2].ID, Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true},
-				{RepoID: repos[2].ID, Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true},
+				{Repo: repos[0], Steps: steps, Path: "a/b", OnlyFetchWorkspace: true},
+				{Repo: repos[0], Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true},
+				{Repo: repos[0], Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true},
+				{Repo: repos[1], Steps: steps, Path: ""},
+				{Repo: repos[2], Steps: steps, Path: "a/b", OnlyFetchWorkspace: true},
+				{Repo: repos[2], Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true},
+				{Repo: repos[2], Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true},
 			},
 		},
 	}
@@ -125,6 +126,14 @@ func TestFindWorkspaces(t *testing.T) {
 				t.Fatalf("unexpected err: %s", err)
 			}
 
+			// Sort by ID, easier than by name for tests.
+			sort.Slice(workspaces, func(i, j int) bool {
+				if workspaces[i].Repo.ID == workspaces[j].Repo.ID {
+					return workspaces[i].Path < workspaces[j].Path
+				}
+				return workspaces[i].Repo.ID < workspaces[j].Repo.ID
+			})
+
 			if diff := cmp.Diff(tt.wantWorkspaces, workspaces); diff != "" {
 				t.Errorf("mismatch (-want +got):\n%s", diff)
 			}