From 3033c7b4f05f430dcffa35a9c8577dc5cc91a093 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 14 Apr 2021 12:59:09 +0200 Subject: [PATCH 1/6] Create executor, git, service packages --- cmd/src/batch_apply.go | 6 +- cmd/src/batch_common.go | 12 +- cmd/src/batch_preview.go | 4 +- cmd/src/batch_progress_printer.go | 18 +- cmd/src/batch_progress_printer_test.go | 5 +- cmd/src/batch_repositories.go | 4 +- cmd/src/batch_validate.go | 4 +- internal/batches/batch_spec.go | 33 +- internal/batches/batch_spec_test.go | 6 +- internal/batches/bind_workspace.go | 22 +- internal/batches/bind_workspace_test.go | 6 +- internal/batches/errors.go | 15 +- .../batches/{ => executor}/execution_cache.go | 10 +- .../{ => executor}/execution_cache_test.go | 16 +- internal/batches/{ => executor}/executor.go | 61 ++-- .../batches/{ => executor}/executor_test.go | 310 +++++++----------- internal/batches/{ => executor}/run_steps.go | 22 +- internal/batches/{ => executor}/templating.go | 49 +-- .../batches/{ => executor}/templating_test.go | 36 +- .../testdata/dummydocker/docker | 0 internal/batches/features.go | 30 +- internal/batches/features_test.go | 8 - internal/batches/git/changes.go | 47 +++ internal/batches/git/changes_test.go | 32 ++ internal/batches/log.go | 10 +- internal/batches/mock/image.go | 29 ++ internal/batches/mock/repo_archive.go | 86 +++++ internal/batches/repo_fetcher.go | 10 + internal/batches/repo_fetcher_test.go | 37 ++- internal/batches/{ => service}/service.go | 124 ++++--- .../batches/{ => service}/service_test.go | 61 ++-- internal/batches/step_changes.go | 1 + internal/batches/volume_workspace.go | 19 +- internal/batches/volume_workspace_test.go | 92 ++---- internal/batches/workspace.go | 39 ++- internal/batches/workspace_test.go | 31 +- 36 files changed, 690 insertions(+), 605 deletions(-) rename internal/batches/{ => executor}/execution_cache.go (96%) rename internal/batches/{ => executor}/execution_cache_test.go (95%) rename internal/batches/{ => executor}/executor.go (90%) rename internal/batches/{ => executor}/executor_test.go (76%) rename internal/batches/{ => executor}/run_steps.go (96%) rename internal/batches/{ => executor}/templating.go (87%) rename internal/batches/{ => executor}/templating_test.go (91%) rename internal/batches/{ => executor}/testdata/dummydocker/docker (100%) create mode 100644 internal/batches/git/changes.go create mode 100644 internal/batches/git/changes_test.go create mode 100644 internal/batches/mock/image.go create mode 100644 internal/batches/mock/repo_archive.go rename internal/batches/{ => service}/service.go (87%) rename internal/batches/{ => service}/service_test.go (94%) create mode 100644 internal/batches/step_changes.go diff --git a/cmd/src/batch_apply.go b/cmd/src/batch_apply.go index 4e58c6dee8..1d62c22cdb 100644 --- a/cmd/src/batch_apply.go +++ b/cmd/src/batch_apply.go @@ -6,7 +6,7 @@ import ( "flag" "fmt" - "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/service" "github.com/sourcegraph/src-cli/internal/output" ) @@ -30,7 +30,7 @@ Examples: flagSet := flag.NewFlagSet("apply", flag.ExitOnError) flags := newBatchApplyFlags(flagSet, batchDefaultCacheDir(), batchDefaultTempDirPrefix()) - doApply := func(ctx context.Context, out *output.Output, svc *batches.Service, flags *batchApplyFlags) error { + doApply := func(ctx context.Context, out *output.Output, svc *service.Service, flags *batchApplyFlags) error { id, _, err := batchExecute(ctx, out, svc, flags) if err != nil { return err @@ -67,7 +67,7 @@ Examples: ctx, cancel := contextCancelOnInterrupt(context.Background()) defer cancel() - svc := batches.NewService(&batches.ServiceOpts{ + svc := service.NewService(&service.ServiceOpts{ AllowUnsupported: flags.allowUnsupported, AllowIgnored: flags.allowIgnored, Client: cfg.apiClient(flags.api, flagSet.Output()), diff --git a/cmd/src/batch_common.go b/cmd/src/batch_common.go index de096b7c44..f02d4b44eb 100644 --- a/cmd/src/batch_common.go +++ b/cmd/src/batch_common.go @@ -19,7 +19,9 @@ import ( "github.com/sourcegraph/go-diff/diff" "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/executor" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/service" "github.com/sourcegraph/src-cli/internal/output" ) @@ -189,7 +191,7 @@ func batchOpenFileFlag(flag *string) (io.ReadCloser, error) { // batchExecute performs all the steps required to upload the campaign spec // to Sourcegraph, including execution as needed. The return values are the // spec ID, spec URL, and error. -func batchExecute(ctx context.Context, out *output.Output, svc *batches.Service, flags *batchApplyFlags) (graphql.BatchSpecID, string, error) { +func batchExecute(ctx context.Context, out *output.Output, svc *service.Service, flags *batchApplyFlags) (graphql.BatchSpecID, string, error) { if err := checkExecutable("git", "version"); err != nil { return "", "", err } @@ -275,7 +277,7 @@ func batchExecute(ctx context.Context, out *output.Output, svc *batches.Service, task.Archive = fetcher.Checkout(task.Repository, task.ArchivePathToFetch()) } - opts := batches.ExecutorOpts{ + opts := executor.ExecutorOpts{ Cache: svc.NewExecutionCache(flags.cacheDir), Creator: workspaceCreator, ClearCache: flags.clearCache, @@ -354,7 +356,7 @@ func batchExecute(ctx context.Context, out *output.Output, svc *batches.Service, // batchParseSpec parses and validates the given batch spec. If the spec has // validation errors, the errors are output in a human readable form and an // exitCodeError is returned. -func batchParseSpec(out *output.Output, svc *batches.Service, input io.ReadCloser) (*batches.BatchSpec, string, error) { +func batchParseSpec(out *output.Output, svc *service.Service, input io.ReadCloser) (*batches.BatchSpec, string, error) { spec, raw, err := svc.ParseBatchSpec(input) if err != nil { if merr, ok := err.(*multierror.Error); ok { @@ -400,7 +402,7 @@ func printExecutionError(out *output.Output, err error) { } for _, e := range errs { - if taskErr, ok := e.(batches.TaskExecutionErr); ok { + if taskErr, ok := e.(executor.TaskExecutionErr); ok { block.Write(formatTaskExecutionErr(taskErr)) } else { if err == context.Canceled { @@ -455,7 +457,7 @@ func flattenErrs(err error) (result []error) { return result } -func formatTaskExecutionErr(err batches.TaskExecutionErr) string { +func formatTaskExecutionErr(err executor.TaskExecutionErr) string { if ee, ok := errors.Cause(err).(*exec.ExitError); ok && ee.String() == "signal: killed" { return fmt.Sprintf( "%s%s%s: killed by interrupt signal", diff --git a/cmd/src/batch_preview.go b/cmd/src/batch_preview.go index b8c645d2b9..312fcd5c47 100644 --- a/cmd/src/batch_preview.go +++ b/cmd/src/batch_preview.go @@ -6,7 +6,7 @@ import ( "flag" "fmt" - "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/service" "github.com/sourcegraph/src-cli/internal/output" ) @@ -42,7 +42,7 @@ Examples: ctx, cancel := contextCancelOnInterrupt(context.Background()) defer cancel() - svc := batches.NewService(&batches.ServiceOpts{ + svc := service.NewService(&service.ServiceOpts{ AllowUnsupported: flags.allowUnsupported, AllowIgnored: flags.allowIgnored, Client: cfg.apiClient(flags.api, flagSet.Output()), diff --git a/cmd/src/batch_progress_printer.go b/cmd/src/batch_progress_printer.go index e27316871e..69e15dbbb4 100644 --- a/cmd/src/batch_progress_printer.go +++ b/cmd/src/batch_progress_printer.go @@ -6,7 +6,7 @@ import ( "strings" "github.com/sourcegraph/go-diff/diff" - "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/executor" "github.com/sourcegraph/src-cli/internal/output" "golang.org/x/sync/semaphore" ) @@ -22,7 +22,7 @@ func newBatchProgressPrinter(out *output.Output, verbose bool, numParallelism in numParallelism: numParallelism, completedTasks: map[string]bool{}, - runningTasks: map[string]*batches.TaskStatus{}, + runningTasks: map[string]*executor.TaskStatus{}, repoStatusBar: map[string]int{}, statusBarRepo: map[int]string{}, @@ -46,13 +46,13 @@ type batchProgressPrinter struct { numParallelism int completedTasks map[string]bool - runningTasks map[string]*batches.TaskStatus + runningTasks map[string]*executor.TaskStatus repoStatusBar map[string]int statusBarRepo map[int]string } -func (p *batchProgressPrinter) initProgressBar(statuses []*batches.TaskStatus) int { +func (p *batchProgressPrinter) initProgressBar(statuses []*executor.TaskStatus) int { numStatusBars := p.numParallelism if len(statuses) < numStatusBars { numStatusBars = len(statuses) @@ -93,7 +93,7 @@ func (p *batchProgressPrinter) updateProgressBar(completed, errored, total int) p.progress.SetLabelAndRecalc(0, label) } -func (p *batchProgressPrinter) PrintStatuses(statuses []*batches.TaskStatus) { +func (p *batchProgressPrinter) PrintStatuses(statuses []*executor.TaskStatus) { if len(statuses) == 0 { return } @@ -109,8 +109,8 @@ func (p *batchProgressPrinter) PrintStatuses(statuses []*batches.TaskStatus) { p.numStatusBars = p.initProgressBar(statuses) } - newlyCompleted := []*batches.TaskStatus{} - currentlyRunning := []*batches.TaskStatus{} + newlyCompleted := []*executor.TaskStatus{} + currentlyRunning := []*executor.TaskStatus{} errored := 0 for _, ts := range statuses { @@ -145,7 +145,7 @@ func (p *batchProgressPrinter) PrintStatuses(statuses []*batches.TaskStatus) { p.updateProgressBar(len(p.completedTasks), errored, len(statuses)) - newlyStarted := map[string]*batches.TaskStatus{} + newlyStarted := map[string]*executor.TaskStatus{} statusBarIndex := 0 for _, ts := range currentlyRunning { if _, ok := p.runningTasks[ts.DisplayName()]; ok { @@ -252,7 +252,7 @@ type statusTexter interface { StatusText() string } -func taskStatusBarText(ts *batches.TaskStatus) (string, error) { +func taskStatusBarText(ts *executor.TaskStatus) (string, error) { var statusText string if ts.IsCompleted() { diff --git a/cmd/src/batch_progress_printer_test.go b/cmd/src/batch_progress_printer_test.go index 5671453db5..09ee75709d 100644 --- a/cmd/src/batch_progress_printer_test.go +++ b/cmd/src/batch_progress_printer_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/executor" "github.com/sourcegraph/src-cli/internal/output" ) @@ -52,7 +53,7 @@ func TestBatchProgressPrinterIntegration(t *testing.T) { }) now := time.Now() - statuses := []*batches.TaskStatus{ + statuses := []*executor.TaskStatus{ { RepoName: "github.com/sourcegraph/sourcegraph", StartedAt: now, @@ -89,7 +90,7 @@ func TestBatchProgressPrinterIntegration(t *testing.T) { } // Now mark the last task as completed - statuses[len(statuses)-1] = &batches.TaskStatus{ + statuses[len(statuses)-1] = &executor.TaskStatus{ RepoName: "github.com/sourcegraph/automation-testing", StartedAt: now.Add(time.Duration(-5) * time.Second), FinishedAt: now.Add(time.Duration(5) * time.Second), diff --git a/cmd/src/batch_repositories.go b/cmd/src/batch_repositories.go index 1674f069b2..d3f97f21b8 100644 --- a/cmd/src/batch_repositories.go +++ b/cmd/src/batch_repositories.go @@ -7,8 +7,8 @@ import ( "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/api" - "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/service" "github.com/sourcegraph/src-cli/internal/output" ) @@ -48,7 +48,7 @@ Examples: ctx := context.Background() client := cfg.apiClient(apiFlags, flagSet.Output()) - svc := batches.NewService(&batches.ServiceOpts{Client: client}) + svc := service.NewService(&service.ServiceOpts{Client: client}) if err := svc.DetermineFeatureFlags(ctx); err != nil { return err diff --git a/cmd/src/batch_validate.go b/cmd/src/batch_validate.go index bf1f582955..3c638594b7 100644 --- a/cmd/src/batch_validate.go +++ b/cmd/src/batch_validate.go @@ -5,7 +5,7 @@ import ( "flag" "fmt" - "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/service" "github.com/sourcegraph/src-cli/internal/output" ) @@ -41,7 +41,7 @@ Examples: } defer specFile.Close() - svc := batches.NewService(&batches.ServiceOpts{}) + svc := service.NewService(&service.ServiceOpts{}) out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose}) if _, _, err := batchParseSpec(out, svc, specFile); err != nil { diff --git a/internal/batches/batch_spec.go b/internal/batches/batch_spec.go index a87cdff248..a98ddab881 100644 --- a/internal/batches/batch_spec.go +++ b/internal/batches/batch_spec.go @@ -1,6 +1,7 @@ package batches import ( + "context" "fmt" "strings" @@ -72,6 +73,14 @@ type WorkspaceConfiguration struct { glob glob.Glob } +func (wc *WorkspaceConfiguration) SetGlob(g glob.Glob) { + wc.glob = g +} + +func (wc *WorkspaceConfiguration) Matches(repoName string) bool { + return wc.glob.Match(repoName) +} + type OnQueryOrRepository struct { RepositoriesMatchingQuery string `json:"repositoriesMatchingQuery,omitempty" yaml:"repositoriesMatchingQuery"` Repository string `json:"repository,omitempty" yaml:"repository"` @@ -88,6 +97,22 @@ type Step struct { image docker.Image } +func (s *Step) SetImage(image docker.Image) { + s.image = image +} + +func (s Step) ImageDigest(ctx context.Context) (string, error) { + return s.image.Digest(ctx) +} + +func (s Step) DockerImage() docker.Image { + return s.image +} + +func (s Step) EnsureImage(ctx context.Context) error { + return s.image.Ensure(ctx) +} + type Outputs map[string]Output type Output struct { @@ -105,7 +130,7 @@ type Group struct { Repository string `json:"repository,omitempty" yaml:"repository"` } -func ParseBatchSpec(data []byte, features featureFlags) (*BatchSpec, error) { +func ParseBatchSpec(data []byte, features FeatureFlags) (*BatchSpec, error) { var spec BatchSpec if err := yaml.UnmarshalValidate(schema.BatchSpecJSON, data, &spec); err != nil { if multiErr, ok := err.(*multierror.Error); ok { @@ -128,7 +153,7 @@ func ParseBatchSpec(data []byte, features featureFlags) (*BatchSpec, error) { var errs *multierror.Error - if !features.allowArrayEnvironments { + if !features.AllowArrayEnvironments { for i, step := range spec.Steps { if !step.Env.IsStatic() { errs = multierror.Append(errs, errors.Errorf("step %d includes one or more dynamic environment variables, which are unsupported in this Sourcegraph version", i+1)) @@ -140,11 +165,11 @@ func ParseBatchSpec(data []byte, features featureFlags) (*BatchSpec, error) { errs = multierror.Append(errs, errors.New("batch spec includes steps but no changesetTemplate")) } - if spec.TransformChanges != nil && !features.allowtransformChanges { + if spec.TransformChanges != nil && !features.AllowTransformChanges { errs = multierror.Append(errs, errors.New("batch spec includes transformChanges, which is not supported in this Sourcegraph version")) } - if len(spec.Workspaces) != 0 && !features.allowtransformChanges { + if len(spec.Workspaces) != 0 && !features.AllowTransformChanges { errs = multierror.Append(errs, errors.New("batch spec includes workspaces, which is not supported in this Sourcegraph version")) } diff --git a/internal/batches/batch_spec_test.go b/internal/batches/batch_spec_test.go index 7f91c1efb3..a13502ac4f 100644 --- a/internal/batches/batch_spec_test.go +++ b/internal/batches/batch_spec_test.go @@ -21,7 +21,7 @@ changesetTemplate: published: false ` - _, err := ParseBatchSpec([]byte(spec), featureFlags{}) + _, err := ParseBatchSpec([]byte(spec), FeatureFlags{}) if err != nil { t.Fatalf("parsing valid spec returned error: %s", err) } @@ -38,7 +38,7 @@ steps: container: alpine:3 ` - _, err := ParseBatchSpec([]byte(spec), featureFlags{}) + _, err := ParseBatchSpec([]byte(spec), FeatureFlags{}) if err == nil { t.Fatal("no error returned") } @@ -71,7 +71,7 @@ changesetTemplate: published: false ` - _, err := ParseBatchSpec([]byte(spec), featureFlags{}) + _, err := ParseBatchSpec([]byte(spec), FeatureFlags{}) if err == nil { t.Fatal("no error returned") } diff --git a/internal/batches/bind_workspace.go b/internal/batches/bind_workspace.go index 6fc6164cbd..ae8688440a 100644 --- a/internal/batches/bind_workspace.go +++ b/internal/batches/bind_workspace.go @@ -13,16 +13,18 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) -type dockerBindWorkspaceCreator struct { - dir string +// TODO(mrnugget): unexport this +type DockerBindWorkspaceCreator struct { + Dir string } -var _ WorkspaceCreator = &dockerBindWorkspaceCreator{} +var _ WorkspaceCreator = &DockerBindWorkspaceCreator{} -func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, archive RepoZip) (Workspace, error) { +func (wc *DockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, archive RepoZip) (Workspace, error) { w, err := wc.unzipToWorkspace(ctx, repo, archive.Path()) if err != nil { return nil, errors.Wrap(err, "unzipping the repository") @@ -35,7 +37,7 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. return w, errors.Wrap(wc.prepareGitRepo(ctx, w), "preparing local git repo") } -func (*dockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerBindWorkspace) error { +func (*DockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerBindWorkspace) error { if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { return errors.Wrap(err, "git init failed") } @@ -51,9 +53,9 @@ func (*dockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *docker return nil } -func (wc *dockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo *graphql.Repository, zip string) (*dockerBindWorkspace, error) { +func (wc *DockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo *graphql.Repository, zip string) (*dockerBindWorkspace, error) { prefix := "workspace-" + repo.Slug() - workspace, err := unzipToTempDir(ctx, zip, wc.dir, prefix) + workspace, err := unzipToTempDir(ctx, zip, wc.Dir, prefix) if err != nil { return nil, errors.Wrap(err, "unzipping the ZIP archive") } @@ -61,7 +63,7 @@ func (wc *dockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo return &dockerBindWorkspace{dir: workspace}, nil } -func (wc *dockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *dockerBindWorkspace, files map[string]string) error { +func (wc *DockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *dockerBindWorkspace, files map[string]string) error { for name, src := range files { srcStat, err := os.Stat(src) if err != nil { @@ -118,7 +120,7 @@ func (w *dockerBindWorkspace) DockerRunOpts(ctx context.Context, target string) func (w *dockerBindWorkspace) WorkDir() *string { return &w.dir } -func (w *dockerBindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { +func (w *dockerBindWorkspace) Changes(ctx context.Context) (*git.Changes, error) { if _, err := runGitCmd(ctx, w.dir, "add", "--all"); err != nil { return nil, errors.Wrap(err, "git add failed") } @@ -128,7 +130,7 @@ func (w *dockerBindWorkspace) Changes(ctx context.Context) (*StepChanges, error) return nil, errors.Wrap(err, "git status failed") } - changes, err := parseGitStatus(statusOut) + changes, err := git.ParseGitStatus(statusOut) if err != nil { return nil, errors.Wrap(err, "parsing git status output") } diff --git a/internal/batches/bind_workspace_test.go b/internal/batches/bind_workspace_test.go index 7cc9d3dc40..4319b25852 100644 --- a/internal/batches/bind_workspace_test.go +++ b/internal/batches/bind_workspace_test.go @@ -83,7 +83,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) archive := &fakeRepoArchive{mockPath: archivePath} - creator := &dockerBindWorkspaceCreator{dir: testTempDir} + creator := &DockerBindWorkspaceCreator{Dir: testTempDir} workspace, err := creator.Create(context.Background(), repo, nil, archive) if err != nil { t.Fatalf("unexpected error: %s", err) @@ -113,7 +113,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { badArchive := &fakeRepoArchive{mockPath: badZipFile} - creator := &dockerBindWorkspaceCreator{dir: testTempDir} + creator := &DockerBindWorkspaceCreator{Dir: testTempDir} if _, err := creator.Create(context.Background(), repo, nil, badArchive); err == nil { t.Error("unexpected nil error") } @@ -127,7 +127,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { mockAdditionalFilePaths: additionalFilePaths, } - creator := &dockerBindWorkspaceCreator{dir: testTempDir} + creator := &DockerBindWorkspaceCreator{Dir: testTempDir} workspace, err := creator.Create(context.Background(), repo, nil, archive) if err != nil { t.Fatalf("unexpected error: %s", err) diff --git a/internal/batches/errors.go b/internal/batches/errors.go index 67a0d9790e..13f8e4794a 100644 --- a/internal/batches/errors.go +++ b/internal/batches/errors.go @@ -7,12 +7,15 @@ import ( "github.com/sourcegraph/src-cli/internal/batches/graphql" ) +// TODO(mrnugget): Merge these two types (give them an "errorfmt" function, +// rename "Has*" methods to "NotEmpty" or something) + // UnsupportedRepoSet provides a set to manage repositories that are on // unsupported code hosts. This type implements error to allow it to be // returned directly as an error value if needed. type UnsupportedRepoSet map[*graphql.Repository]struct{} -func (e UnsupportedRepoSet) includes(r *graphql.Repository) bool { +func (e UnsupportedRepoSet) Includes(r *graphql.Repository) bool { _, ok := e[r] return ok } @@ -37,11 +40,11 @@ func (e UnsupportedRepoSet) Error() string { ) } -func (e UnsupportedRepoSet) appendRepo(repo *graphql.Repository) { +func (e UnsupportedRepoSet) Append(repo *graphql.Repository) { e[repo] = struct{}{} } -func (e UnsupportedRepoSet) hasUnsupported() bool { +func (e UnsupportedRepoSet) HasUnsupported() bool { return len(e) > 0 } @@ -50,7 +53,7 @@ func (e UnsupportedRepoSet) hasUnsupported() bool { // returned directly as an error value if needed. type IgnoredRepoSet map[*graphql.Repository]struct{} -func (e IgnoredRepoSet) includes(r *graphql.Repository) bool { +func (e IgnoredRepoSet) Includes(r *graphql.Repository) bool { _, ok := e[r] return ok } @@ -67,10 +70,10 @@ func (e IgnoredRepoSet) Error() string { ) } -func (e IgnoredRepoSet) appendRepo(repo *graphql.Repository) { +func (e IgnoredRepoSet) Append(repo *graphql.Repository) { e[repo] = struct{}{} } -func (e IgnoredRepoSet) hasIgnored() bool { +func (e IgnoredRepoSet) HasIgnored() bool { return len(e) > 0 } diff --git a/internal/batches/execution_cache.go b/internal/batches/executor/execution_cache.go similarity index 96% rename from internal/batches/execution_cache.go rename to internal/batches/executor/execution_cache.go index 3eb0d1b03b..3f71dd3fe7 100644 --- a/internal/batches/execution_cache.go +++ b/internal/batches/executor/execution_cache.go @@ -1,4 +1,4 @@ -package batches +package executor import ( "context" @@ -67,6 +67,14 @@ type ExecutionCache interface { Clear(ctx context.Context, key ExecutionCacheKey) error } +func NewCache(dir string) ExecutionCache { + if dir == "" { + return &ExecutionNoOpCache{} + } + + return &ExecutionDiskCache{dir} +} + type ExecutionDiskCache struct { Dir string } diff --git a/internal/batches/execution_cache_test.go b/internal/batches/executor/execution_cache_test.go similarity index 95% rename from internal/batches/execution_cache_test.go rename to internal/batches/executor/execution_cache_test.go index f43770c8d3..34d03ae528 100644 --- a/internal/batches/execution_cache_test.go +++ b/internal/batches/executor/execution_cache_test.go @@ -1,4 +1,4 @@ -package batches +package executor import ( "context" @@ -9,6 +9,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" "gopkg.in/yaml.v3" ) @@ -18,7 +20,7 @@ const testExecutionCacheKeyEnv = "TEST_EXECUTION_CACHE_KEY_ENV" func TestExecutionCacheKey(t *testing.T) { // Let's set up an array of steps that we can test with. One step will // depend on an environment variable outside the spec. - var steps []Step + var steps []batches.Step if err := yaml.Unmarshal([]byte(` - run: foo env: @@ -119,21 +121,21 @@ func TestExecutionDiskCache(t *testing.T) { cacheKey1 := ExecutionCacheKey{Task: &Task{ Repository: &graphql.Repository{Name: "src-cli"}, - Steps: []Step{ + Steps: []batches.Step{ {Run: "echo 'Hello World'", Container: "alpine:3"}, }, }} cacheKey2 := ExecutionCacheKey{Task: &Task{ Repository: &graphql.Repository{Name: "documentation"}, - Steps: []Step{ + Steps: []batches.Step{ {Run: "echo 'Hello World'", Container: "alpine:3"}, }, }} value := executionResult{ Diff: testDiff, - ChangedFiles: &StepChanges{ + ChangedFiles: &git.Changes{ Added: []string{"README.md"}, }, Outputs: map[string]interface{}{}, @@ -218,8 +220,8 @@ func writeV1CacheFile(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey, d path = filepath.Join(c.Dir, hashedKey+".json") // v1 contained a fully serialized ChangesetSpec - spec := ChangesetSpec{CreatedChangeset: &CreatedChangeset{ - Commits: []GitCommitDescription{ + spec := batches.ChangesetSpec{CreatedChangeset: &batches.CreatedChangeset{ + Commits: []batches.GitCommitDescription{ {Diff: testDiff}, }, }} diff --git a/internal/batches/executor.go b/internal/batches/executor/executor.go similarity index 90% rename from internal/batches/executor.go rename to internal/batches/executor/executor.go index a3ab9d8614..caa70e15ee 100644 --- a/internal/batches/executor.go +++ b/internal/batches/executor/executor.go @@ -1,4 +1,4 @@ -package batches +package executor import ( "context" @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" "github.com/sourcegraph/go-diff/diff" "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -45,7 +46,7 @@ type Executor interface { AddTask(*Task) LogFiles() []string Start(ctx context.Context) - Wait(ctx context.Context) ([]*ChangesetSpec, error) + Wait(ctx context.Context) ([]*batches.ChangesetSpec, error) // LockedTaskStatuses calls the given function with the current state of // the task statuses. Before calling the function, the statuses are locked @@ -66,14 +67,14 @@ type Task struct { // If Path is "" then this setting has no effect. OnlyFetchWorkspace bool - Steps []Step + Steps []batches.Step Outputs map[string]interface{} - BatchChangeAttributes *BatchChangeAttributes `json:"-"` - Template *ChangesetTemplate `json:"-"` - TransformChanges *TransformChanges `json:"-"` + BatchChangeAttributes *BatchChangeAttributes `json:"-"` + Template *batches.ChangesetTemplate `json:"-"` + TransformChanges *batches.TransformChanges `json:"-"` - Archive RepoZip `json:"-"` + Archive batches.RepoZip `json:"-"` } func (t *Task) ArchivePathToFetch() string { @@ -104,7 +105,7 @@ type TaskStatus struct { // ChangesetSpecs are the specs produced by executing the Task in a // repository. With the introduction of `transformChanges` to the batch // spec, one Task can produce multiple ChangesetSpecs. - ChangesetSpecs []*ChangesetSpec + ChangesetSpecs []*batches.ChangesetSpec // Err is set if executing the Task lead to an error. Err error @@ -162,7 +163,7 @@ func (ts *TaskStatus) FileDiffs() ([]*diff.FileDiff, bool, error) { type ExecutorOpts struct { Cache ExecutionCache - Creator WorkspaceCreator + Creator batches.WorkspaceCreator Parallelism int Timeout time.Duration @@ -176,9 +177,9 @@ type executor struct { cache ExecutionCache client api.Client - features featureFlags - logger *LogManager - creator WorkspaceCreator + features batches.FeatureFlags + logger *batches.LogManager + creator batches.WorkspaceCreator tasks []*Task statuses map[*Task]*TaskStatus @@ -189,11 +190,11 @@ type executor struct { par *parallel.Run doneEnqueuing chan struct{} - specs []*ChangesetSpec + specs []*batches.ChangesetSpec specsMu sync.Mutex } -func newExecutor(opts ExecutorOpts, client api.Client, features featureFlags) *executor { +func New(opts ExecutorOpts, client api.Client, features batches.FeatureFlags) *executor { return &executor{ ExecutorOpts: opts, cache: opts.Cache, @@ -201,7 +202,7 @@ func newExecutor(opts ExecutorOpts, client api.Client, features featureFlags) *e client: client, features: features, doneEnqueuing: make(chan struct{}), - logger: NewLogManager(opts.TempDir, opts.KeepLogs), + logger: batches.NewLogManager(opts.TempDir, opts.KeepLogs), tempDir: opts.TempDir, par: parallel.NewRun(opts.Parallelism), tasks: []*Task{}, @@ -249,7 +250,7 @@ func (x *executor) Start(ctx context.Context) { } } -func (x *executor) Wait(ctx context.Context) ([]*ChangesetSpec, error) { +func (x *executor) Wait(ctx context.Context) ([]*batches.ChangesetSpec, error) { <-x.doneEnqueuing result := make(chan error, 1) @@ -318,7 +319,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { return } - var specs []*ChangesetSpec + var specs []*batches.ChangesetSpec specs, err = createChangesetSpecs(task, result, x.features) if err != nil { return err @@ -341,7 +342,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { // It isn't, so let's get ready to run the task. First, let's set up our // logging. - log, err := x.logger.AddTask(task) + log, err := x.logger.AddTask(task.Repository.SlugForPath(task.Path)) if err != nil { err = errors.Wrap(err, "creating log file") return @@ -425,7 +426,7 @@ func (x *executor) updateTaskStatus(task *Task, update func(status *TaskStatus)) } } -func (x *executor) addCompletedSpecs(repository *graphql.Repository, specs []*ChangesetSpec) error { +func (x *executor) addCompletedSpecs(repository *graphql.Repository, specs []*batches.ChangesetSpec) error { x.specsMu.Lock() defer x.specsMu.Unlock() @@ -461,7 +462,7 @@ func reachedTimeout(cmdCtx context.Context, err error) bool { return errors.Is(errors.Cause(err), context.DeadlineExceeded) } -func createChangesetSpecs(task *Task, result executionResult, features featureFlags) ([]*ChangesetSpec, error) { +func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batches.ChangesetSpec, error) { repo := task.Repository.Name tmplCtx := &ChangesetTemplateContext{ @@ -478,7 +479,7 @@ func createChangesetSpecs(task *Task, result executionResult, features featureFl var authorEmail string if task.Template.Commit.Author == nil { - if features.includeAutoAuthorDetails { + if features.IncludeAutoAuthorDetails { // user did not provide author info, so use defaults authorName = "Sourcegraph" authorEmail = "batch-changes@sourcegraph.com" @@ -518,17 +519,17 @@ func createChangesetSpecs(task *Task, result executionResult, features featureFl return nil, err } - newSpec := func(branch, diff string) *ChangesetSpec { - return &ChangesetSpec{ + newSpec := func(branch, diff string) *batches.ChangesetSpec { + return &batches.ChangesetSpec{ BaseRepository: task.Repository.ID, - CreatedChangeset: &CreatedChangeset{ + CreatedChangeset: &batches.CreatedChangeset{ BaseRef: task.Repository.BaseRef(), BaseRev: task.Repository.Rev(), HeadRepository: task.Repository.ID, HeadRef: "refs/heads/" + branch, Title: title, Body: body, - Commits: []GitCommitDescription{ + Commits: []batches.GitCommitDescription{ { Message: message, AuthorName: authorName, @@ -541,7 +542,7 @@ func createChangesetSpecs(task *Task, result executionResult, features featureFl } } - var specs []*ChangesetSpec + var specs []*batches.ChangesetSpec groups := groupsForRepository(task.Repository.Name, task.TransformChanges) if len(groups) != 0 { @@ -566,8 +567,8 @@ func createChangesetSpecs(task *Task, result executionResult, features featureFl return specs, nil } -func groupsForRepository(repo string, transform *TransformChanges) []Group { - var groups []Group +func groupsForRepository(repo string, transform *batches.TransformChanges) []batches.Group { + var groups []batches.Group if transform == nil { return groups @@ -586,7 +587,7 @@ func groupsForRepository(repo string, transform *TransformChanges) []Group { return groups } -func validateGroups(repo, defaultBranch string, groups []Group) error { +func validateGroups(repo, defaultBranch string, groups []batches.Group) error { uniqueBranches := make(map[string]struct{}, len(groups)) for _, g := range groups { @@ -604,7 +605,7 @@ func validateGroups(repo, defaultBranch string, groups []Group) error { return nil } -func groupFileDiffs(completeDiff, defaultBranch string, groups []Group) (map[string]string, error) { +func groupFileDiffs(completeDiff, defaultBranch string, groups []batches.Group) (map[string]string, error) { fileDiffs, err := diff.ParseMultiFileDiff([]byte(completeDiff)) if err != nil { return nil, err diff --git a/internal/batches/executor_test.go b/internal/batches/executor/executor_test.go similarity index 76% rename from internal/batches/executor_test.go rename to internal/batches/executor/executor_test.go index afc9acf92d..4497a02811 100644 --- a/internal/batches/executor_test.go +++ b/internal/batches/executor/executor_test.go @@ -1,14 +1,11 @@ -package batches +package executor import ( - "archive/zip" "bytes" "context" "encoding/json" "fmt" "io/ioutil" - "log" - "mime" "net/http" "net/http/httptest" "os" @@ -23,7 +20,10 @@ import ( "github.com/sourcegraph/batch-change-utils/overridable" "github.com/sourcegraph/go-diff/diff" "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/mock" ) func TestExecutor_Integration(t *testing.T) { @@ -49,7 +49,7 @@ func TestExecutor_Integration(t *testing.T) { } changesetTemplateBranch := "my-branch" - defaultTemplate := &ChangesetTemplate{Branch: changesetTemplateBranch} + defaultTemplate := &batches.ChangesetTemplate{Branch: changesetTemplateBranch} defaultBatchChangeAttributes := &BatchChangeAttributes{ Name: "integration-test-batch-change", Description: "this is an integration test", @@ -61,13 +61,13 @@ func TestExecutor_Integration(t *testing.T) { tests := []struct { name string - archives []mockRepoArchive - additionalFiles []mockRepoAdditionalFiles + archives []mock.RepoArchive + additionalFiles []mock.MockRepoAdditionalFiles // We define the steps only once per test case so there's less duplication - steps []Step + steps []batches.Step // Same goes for transformChanges - transform *TransformChanges + transform *batches.TransformChanges tasks []*Task @@ -84,18 +84,18 @@ func TestExecutor_Integration(t *testing.T) { }{ { name: "success", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", }}, - {repo: sourcegraphRepo, files: map[string]string{ + {Repo: sourcegraphRepo, Files: map[string]string{ "README.md": "# Sourcegraph README\n", }}, }, - steps: []Step{ - {Run: `echo -e "foobar\n" >> README.md`, Container: "alpine:13"}, - {Run: `[[ -f "main.go" ]] && go fmt main.go || exit 0`, Container: "doesntmatter:13"}, + steps: []batches.Step{ + {Run: `echo -e "foobar\n" >> README.md`}, + {Run: `[[ -f "main.go" ]] && go fmt main.go || exit 0`}, }, tasks: []*Task{ {Repository: srcCLIRepo}, @@ -112,16 +112,16 @@ func TestExecutor_Integration(t *testing.T) { }, { name: "timeout", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{"README.md": "line 1"}}, + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{"README.md": "line 1"}}, }, - steps: []Step{ + steps: []batches.Step{ // This needs to be a loop, because when a process goes to sleep // it's not interruptible, meaning that while it will receive SIGKILL // it won't exit until it had its full night of sleep. // So. // Instead we take short powernaps. - {Run: `while true; do echo "zZzzZ" && sleep 0.05; done`, Container: "alpine:13"}, + {Run: `while true; do echo "zZzzZ" && sleep 0.05; done`}, }, tasks: []*Task{ {Repository: srcCLIRepo}, @@ -131,16 +131,16 @@ func TestExecutor_Integration(t *testing.T) { }, { name: "templated", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", }}, }, - steps: []Step{ - {Run: `go fmt main.go`, Container: "doesntmatter:13"}, - {Run: `touch modified-${{ join previous_step.modified_files " " }}.md`, Container: "alpine:13"}, - {Run: `touch added-${{ join previous_step.added_files " " }}`, Container: "alpine:13"}, + steps: []batches.Step{ + {Run: `go fmt main.go`}, + {Run: `touch modified-${{ join previous_step.modified_files " " }}.md`}, + {Run: `touch added-${{ join previous_step.added_files " " }}`}, }, tasks: []*Task{ @@ -154,14 +154,14 @@ func TestExecutor_Integration(t *testing.T) { }, { name: "empty", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", }}, }, - steps: []Step{ - {Run: `true`, Container: "doesntmatter:13"}, + steps: []batches.Step{ + {Run: "true"}, }, tasks: []*Task{ @@ -172,14 +172,14 @@ func TestExecutor_Integration(t *testing.T) { }, { name: "transform group", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "a/a.go": "package a", "a/b/b.go": "package b", "a/b/c/c.go": "package c", }}, - {repo: sourcegraphRepo, files: map[string]string{ + {Repo: sourcegraphRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "a/a.go": "package a", "a/b/b.go": "package b", @@ -191,13 +191,13 @@ func TestExecutor_Integration(t *testing.T) { {Repository: srcCLIRepo}, {Repository: sourcegraphRepo}, }, - steps: []Step{ - {Run: `echo 'var a = 1' >> a/a.go`, Container: "doesntmatter:13"}, - {Run: `echo 'var b = 2' >> a/b/b.go`, Container: "doesntmatter:13"}, - {Run: `echo 'var c = 3' >> a/b/c/c.go`, Container: "doesntmatter:13"}, + steps: []batches.Step{ + {Run: `echo 'var a = 1' >> a/a.go`}, + {Run: `echo 'var b = 2' >> a/b/b.go`}, + {Run: `echo 'var c = 3' >> a/b/c/c.go`}, }, - transform: &TransformChanges{ - Group: []Group{ + transform: &batches.TransformChanges{ + Group: []batches.Group{ {Directory: "a/b/c", Branch: "in-directory-c"}, {Directory: "a/b", Branch: "in-directory-b", Repository: sourcegraphRepo.Name}, }, @@ -226,18 +226,17 @@ func TestExecutor_Integration(t *testing.T) { }, { name: "templated changesetTemplate", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Files: map[string]string{ "README.md": "# Welcome to the README\n", "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", }}, }, - steps: []Step{ + steps: []batches.Step{ { - Run: `go fmt main.go`, - Container: "doesntmatter:13", - Outputs: Outputs{ - "myOutputName1": Output{ + Run: `go fmt main.go`, + Outputs: batches.Outputs{ + "myOutputName1": batches.Output{ Value: "${{ index step.modified_files 0 }}", }, }, @@ -245,15 +244,15 @@ func TestExecutor_Integration(t *testing.T) { { Run: `echo -n "Hello World!"`, Container: "alpine:13", - Outputs: Outputs{ - "myOutputName2": Output{ + Outputs: batches.Outputs{ + "myOutputName2": batches.Output{ Value: `thisStepStdout: "${{ step.stdout }}"`, Format: "yaml", }, - "myOutputName3": Output{ + "myOutputName3": batches.Output{ Value: "cool-suffix", }, - "myOutputName4": Output{ + "myOutputName4": batches.Output{ Value: "${{ batch_change.name }}", }, }, @@ -262,7 +261,7 @@ func TestExecutor_Integration(t *testing.T) { tasks: []*Task{ { Repository: srcCLIRepo, - Template: &ChangesetTemplate{ + Template: &batches.ChangesetTemplate{ Title: "myOutputName1=${{ outputs.myOutputName1}}", Body: `myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }} modified_files=${{ steps.modified_files }} @@ -275,9 +274,9 @@ batch_change_description=${{ batch_change.description }} output4=${{ outputs.myOutputName4 }} `, Branch: "templated-branch-${{ outputs.myOutputName3 }}", - Commit: ExpandedGitCommitDescription{ + Commit: batches.ExpandedGitCommitDescription{ Message: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", - Author: &GitCommitAuthor{ + Author: &batches.GitCommitAuthor{ Name: "myOutputName1=${{ outputs.myOutputName1}}", Email: "myOutputName1=${{ outputs.myOutputName1}}", }, @@ -308,74 +307,61 @@ output4=integration-test-batch-change`, { name: "workspaces", - archives: []mockRepoArchive{ - {repo: srcCLIRepo, path: "", files: map[string]string{ + archives: []mock.RepoArchive{ + {Repo: srcCLIRepo, Path: "", Files: map[string]string{ ".gitignore": "node_modules", "message.txt": "root-dir", "a/message.txt": "a-dir", "a/.gitignore": "node_modules-in-a", "a/b/message.txt": "b-dir", }}, - {repo: srcCLIRepo, path: "a", files: map[string]string{ + {Repo: srcCLIRepo, Path: "a", Files: map[string]string{ "a/message.txt": "a-dir", "a/.gitignore": "node_modules-in-a", "a/b/message.txt": "b-dir", }}, - {repo: srcCLIRepo, path: "a/b", files: map[string]string{ + {Repo: srcCLIRepo, Path: "a/b", Files: map[string]string{ "a/b/message.txt": "b-dir", }}, }, - additionalFiles: []mockRepoAdditionalFiles{ - {repo: srcCLIRepo, additionalFiles: map[string]string{ + additionalFiles: []mock.MockRepoAdditionalFiles{ + {Repo: srcCLIRepo, AdditionalFiles: map[string]string{ ".gitignore": "node_modules", "a/.gitignore": "node_modules-in-a", }}, }, - steps: []Step{ + steps: []batches.Step{ { - Run: "cat message.txt && echo 'Hello' > hello.txt", - Container: "doesntmatter:13", - Outputs: Outputs{ - "message": Output{ + Run: "cat message.txt && echo 'Hello' > hello.txt", + Outputs: batches.Outputs{ + "message": batches.Output{ Value: "${{ step.stdout }}", }, }, }, - { - Run: `if [[ -f ".gitignore" ]]; then echo "yes" >> gitignore-exists; fi`, - Container: "doesntmatter:13", - }, - { - Run: `if [[ $(basename $(pwd)) == "a" && -f "../.gitignore" ]]; then echo "yes" >> gitignore-exists; fi`, - Container: "doesntmatter:13", - }, + {Run: `if [[ -f ".gitignore" ]]; then echo "yes" >> gitignore-exists; fi`}, + {Run: `if [[ $(basename $(pwd)) == "a" && -f "../.gitignore" ]]; then echo "yes" >> gitignore-exists; fi`}, // In `a/b` we want the `.gitignore` file in the root folder and in `a` to be fetched: - { - Run: `if [[ $(basename $(pwd)) == "b" && -f "../../.gitignore" ]]; then echo "yes" >> gitignore-exists; fi`, - Container: "doesntmatter:13", - }, - { - Run: `if [[ $(basename $(pwd)) == "b" && -f "../.gitignore" ]]; then echo "yes" >> gitignore-exists-in-a; fi`, - Container: "doesntmatter:13", - }, + {Run: `if [[ $(basename $(pwd)) == "b" && -f "../../.gitignore" ]]; then echo "yes" >> gitignore-exists; fi`}, + {Run: `if [[ $(basename $(pwd)) == "b" && -f "../.gitignore" ]]; then echo "yes" >> gitignore-exists-in-a; fi`}, }, tasks: []*Task{ { Repository: srcCLIRepo, Path: "", - Template: &ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, + Template: &batches.ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, }, { Repository: srcCLIRepo, Path: "a", - Template: &ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, + Template: &batches.ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, }, { Repository: srcCLIRepo, Path: "a/b", - Template: &ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, + Template: &batches.ChangesetTemplate{Branch: "workspace-${{ outputs.message }}"}, }, }, @@ -391,7 +377,7 @@ output4=integration-test-batch-change`, for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - mux := newZipArchivesMux(t, nil, tc.archives...) + mux := mock.NewZipArchivesMux(t, nil, tc.archives...) middle := func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -399,7 +385,7 @@ output4=integration-test-batch-change`, }) } for _, additionalFiles := range tc.additionalFiles { - handleAdditionalFiles(mux, additionalFiles, middle) + mock.HandleAdditionalFiles(mux, additionalFiles, middle) } ts := httptest.NewServer(mux) defer ts.Close() @@ -414,7 +400,7 @@ output4=integration-test-batch-change`, defer os.Remove(testTempDir) cache := newInMemoryExecutionCache() - creator := &dockerBindWorkspaceCreator{dir: testTempDir} + creator := &batches.DockerBindWorkspaceCreator{Dir: testTempDir} opts := ExecutorOpts{ Cache: cache, Creator: creator, @@ -427,20 +413,17 @@ output4=integration-test-batch-change`, opts.Timeout = 30 * time.Second } - repoFetcher := &repoFetcher{ - client: client, - dir: testTempDir, - } + repoFetcher := batches.NewRepoFetcher(client, testTempDir) // execute contains the actual logic running the tasks on an // executor. We'll run this multiple times to cover both the cache // and non-cache code paths. execute := func(t *testing.T) { - executor := newExecutor(opts, client, featuresAllEnabled()) + executor := New(opts, client, featuresAllEnabled()) for i := range tc.steps { - tc.steps[i].image = &mockImage{ - digest: tc.steps[i].Container, - } + tc.steps[i].SetImage(&mock.Image{ + RawDigest: tc.steps[i].Container, + }) } for _, task := range tc.tasks { if task.Template == nil { @@ -592,25 +575,25 @@ func TestValidateGroups(t *testing.T) { tests := []struct { defaultBranch string - groups []Group + groups []batches.Group wantErr string }{ { - groups: []Group{ + groups: []batches.Group{ {Directory: "a", Branch: "my-batch-change-a"}, {Directory: "b", Branch: "my-batch-change-b"}, }, wantErr: "", }, { - groups: []Group{ + groups: []batches.Group{ {Directory: "a", Branch: "my-batch-change-SAME"}, {Directory: "b", Branch: "my-batch-change-SAME"}, }, wantErr: "transformChanges would lead to multiple changesets in repository github.com/sourcegraph/src-cli to have the same branch \"my-batch-change-SAME\"", }, { - groups: []Group{ + groups: []batches.Group{ {Directory: "a", Branch: "my-batch-change-SAME"}, {Directory: "b", Branch: defaultBranch}, }, @@ -663,12 +646,12 @@ index 0000000..1bd79fb tests := []struct { diff string defaultBranch string - groups []Group + groups []batches.Group want map[string]string }{ { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ {Directory: "1/2/3", Branch: "everything-in-3"}, }, want: map[string]string{ @@ -678,7 +661,7 @@ index 0000000..1bd79fb }, { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ {Directory: "1/2", Branch: "everything-in-2-and-3"}, }, want: map[string]string{ @@ -688,7 +671,7 @@ index 0000000..1bd79fb }, { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ {Directory: "1", Branch: "everything-in-1-and-2-and-3"}, }, want: map[string]string{ @@ -698,7 +681,7 @@ index 0000000..1bd79fb }, { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ // Each diff is matched against each directory, last match wins {Directory: "1", Branch: "only-in-1"}, {Directory: "1/2", Branch: "only-in-2"}, @@ -713,7 +696,7 @@ index 0000000..1bd79fb }, { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ // Last one wins here, because it matches every diff {Directory: "1/2/3", Branch: "only-in-3"}, {Directory: "1/2", Branch: "only-in-2"}, @@ -726,7 +709,7 @@ index 0000000..1bd79fb }, { diff: allDiffs, - groups: []Group{ + groups: []batches.Group{ {Directory: "", Branch: "everything"}, }, want: map[string]string{ @@ -747,30 +730,22 @@ index 0000000..1bd79fb } func TestCreateChangesetSpecs(t *testing.T) { - allFeatures := featureFlags{ - allowArrayEnvironments: true, - includeAutoAuthorDetails: true, - useGzipCompression: true, - allowtransformChanges: true, - allowWorkspaces: true, - } - srcCLI := &graphql.Repository{ ID: "src-cli", Name: "github.com/sourcegraph/src-cli", DefaultBranch: &graphql.Branch{Name: "main", Target: graphql.Target{OID: "d34db33f"}}, } - defaultChangesetSpec := &ChangesetSpec{ + defaultChangesetSpec := &batches.ChangesetSpec{ BaseRepository: srcCLI.ID, - CreatedChangeset: &CreatedChangeset{ + CreatedChangeset: &batches.CreatedChangeset{ BaseRef: srcCLI.DefaultBranch.Name, BaseRev: srcCLI.DefaultBranch.Target.OID, HeadRepository: srcCLI.ID, HeadRef: "refs/heads/my-branch", Title: "The title", Body: "The body", - Commits: []GitCommitDescription{ + Commits: []batches.GitCommitDescription{ { Message: "git commit message", Diff: "cool diff", @@ -782,7 +757,7 @@ func TestCreateChangesetSpecs(t *testing.T) { }, } - specWith := func(s *ChangesetSpec, f func(s *ChangesetSpec)) *ChangesetSpec { + specWith := func(s *batches.ChangesetSpec, f func(s *batches.ChangesetSpec)) *batches.ChangesetSpec { f(s) return s } @@ -792,11 +767,11 @@ func TestCreateChangesetSpecs(t *testing.T) { Name: "the name", Description: "The description", }, - Template: &ChangesetTemplate{ + Template: &batches.ChangesetTemplate{ Title: "The title", Body: "The body", Branch: "my-branch", - Commit: ExpandedGitCommitDescription{ + Commit: batches.ExpandedGitCommitDescription{ Message: "git commit message", }, Published: parsePublishedFieldString(t, "false"), @@ -811,7 +786,7 @@ func TestCreateChangesetSpecs(t *testing.T) { defaultResult := executionResult{ Diff: "cool diff", - ChangedFiles: &StepChanges{ + ChangedFiles: &git.Changes{ Modified: []string{"README.md"}, }, Outputs: map[string]interface{}{}, @@ -822,14 +797,14 @@ func TestCreateChangesetSpecs(t *testing.T) { task *Task result executionResult - want []*ChangesetSpec + want []*batches.ChangesetSpec wantErr string }{ { name: "success", task: defaultTask, result: defaultResult, - want: []*ChangesetSpec{ + want: []*batches.ChangesetSpec{ defaultChangesetSpec, }, wantErr: "", @@ -841,8 +816,8 @@ func TestCreateChangesetSpecs(t *testing.T) { task.Template.Published = parsePublishedFieldString(t, published) }), result: defaultResult, - want: []*ChangesetSpec{ - specWith(defaultChangesetSpec, func(s *ChangesetSpec) { + want: []*batches.ChangesetSpec{ + specWith(defaultChangesetSpec, func(s *batches.ChangesetSpec) { s.Published = true }), }, @@ -855,8 +830,8 @@ func TestCreateChangesetSpecs(t *testing.T) { task.Template.Published = parsePublishedFieldString(t, published) }), result: defaultResult, - want: []*ChangesetSpec{ - specWith(defaultChangesetSpec, func(s *ChangesetSpec) { + want: []*batches.ChangesetSpec{ + specWith(defaultChangesetSpec, func(s *batches.ChangesetSpec) { s.Published = false }), }, @@ -866,7 +841,7 @@ func TestCreateChangesetSpecs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - have, err := createChangesetSpecs(tt.task, tt.result, allFeatures) + have, err := createChangesetSpecs(tt.task, tt.result, featuresAllEnabled()) if err != nil { if tt.wantErr != "" { if err.Error() != tt.wantErr { @@ -905,79 +880,6 @@ func addToPath(t *testing.T, relPath string) { os.Setenv("PATH", fmt.Sprintf("%s%c%s", dummyDockerPath, os.PathListSeparator, os.Getenv("PATH"))) } -type mockRepoArchive struct { - repo *graphql.Repository - path string - files map[string]string -} - -func newZipArchivesMux(t *testing.T, callback http.HandlerFunc, archives ...mockRepoArchive) *http.ServeMux { - mux := http.NewServeMux() - - for _, archive := range archives { - files := archive.files - path := fmt.Sprintf("/%s@%s/-/raw", archive.repo.Name, archive.repo.BaseRef()) - if archive.path != "" { - path = path + "/" + archive.path - } - - downloadName := filepath.Base(archive.repo.Name) - mediaType := mime.FormatMediaType("Attachment", map[string]string{ - "filename": downloadName, - }) - - mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("Content-Type", "application/zip") - w.Header().Set("Content-Disposition", mediaType) - - zipWriter := zip.NewWriter(w) - for name, body := range files { - f, err := zipWriter.Create(name) - if err != nil { - log.Fatal(err) - } - if _, err := f.Write([]byte(body)); err != nil { - t.Errorf("failed to write body for %s to zip: %s", name, err) - } - - if callback != nil { - callback(w, r) - } - } - if err := zipWriter.Close(); err != nil { - t.Fatalf("closing zipWriter failed: %s", err) - } - }) - } - - return mux -} - -type middleware func(http.Handler) http.Handler - -type mockRepoAdditionalFiles struct { - repo *graphql.Repository - additionalFiles map[string]string -} - -func handleAdditionalFiles(mux *http.ServeMux, files mockRepoAdditionalFiles, middle middleware) { - for name, content := range files.additionalFiles { - path := fmt.Sprintf("/%s@%s/-/raw/%s", files.repo.Name, files.repo.BaseRef(), name) - handler := func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "text/plain") - - w.Write([]byte(content)) - } - - if middle != nil { - mux.Handle(path, middle(http.HandlerFunc(handler))) - } else { - mux.HandleFunc(path, handler) - } - } -} - // inMemoryExecutionCache provides an in-memory cache for testing purposes. type inMemoryExecutionCache struct { cache map[string]executionResult @@ -1037,3 +939,13 @@ func (c *inMemoryExecutionCache) Clear(ctx context.Context, key ExecutionCacheKe delete(c.cache, k) return nil } + +func featuresAllEnabled() batches.FeatureFlags { + return batches.FeatureFlags{ + AllowArrayEnvironments: true, + IncludeAutoAuthorDetails: true, + UseGzipCompression: true, + AllowTransformChanges: true, + AllowWorkspaces: true, + } +} diff --git a/internal/batches/run_steps.go b/internal/batches/executor/run_steps.go similarity index 96% rename from internal/batches/run_steps.go rename to internal/batches/executor/run_steps.go index 4cba100650..d857bc8e20 100644 --- a/internal/batches/run_steps.go +++ b/internal/batches/executor/run_steps.go @@ -1,4 +1,4 @@ -package batches +package executor import ( "bytes" @@ -14,6 +14,8 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" yamlv3 "gopkg.in/yaml.v3" @@ -24,7 +26,7 @@ type executionResult struct { Diff string `json:"diff"` // ChangedFiles are files that have been changed by all steps. - ChangedFiles *StepChanges `json:"changedFiles"` + ChangedFiles *git.Changes `json:"changedFiles"` // Outputs are the outputs produced by all steps. Outputs map[string]interface{} `json:"outputs"` @@ -36,18 +38,18 @@ type executionResult struct { } type executionOpts struct { - archive RepoZip + archive batches.RepoZip - wc WorkspaceCreator + wc batches.WorkspaceCreator path string batchChangeAttributes *BatchChangeAttributes repo *graphql.Repository - steps []Step + steps []batches.Step tempDir string - logger *TaskLogger + logger *batches.TaskLogger reportProgress func(string) } @@ -109,15 +111,15 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult, }() // We need to grab the digest for the exact image we're using. - digest, err := step.image.Digest(ctx) + digest, err := step.ImageDigest(ctx) if err != nil { - return execResult, errors.Wrapf(err, "getting digest for %v", step.image) + return execResult, errors.Wrapf(err, "getting digest for %v", step.DockerImage()) } // For now, we only support shell scripts provided via the Run field. shell, containerTemp, err := probeImageForShell(ctx, digest) if err != nil { - return execResult, errors.Wrapf(err, "probing image %q for shell", step.image) + return execResult, errors.Wrapf(err, "probing image %q for shell", step.DockerImage()) } // Set up a temporary file on the host filesystem to contain the @@ -284,7 +286,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result executionResult, return execResult, err } -func setOutputs(stepOutputs Outputs, global map[string]interface{}, stepCtx *StepContext) error { +func setOutputs(stepOutputs batches.Outputs, global map[string]interface{}, stepCtx *StepContext) error { for name, output := range stepOutputs { var value bytes.Buffer diff --git a/internal/batches/templating.go b/internal/batches/executor/templating.go similarity index 87% rename from internal/batches/templating.go rename to internal/batches/executor/templating.go index c537c4682a..f0e9322f1b 100644 --- a/internal/batches/templating.go +++ b/internal/batches/executor/templating.go @@ -1,13 +1,13 @@ -package batches +package executor import ( "bytes" - "fmt" "io" "strings" "text/template" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -40,6 +40,7 @@ func renderStepMap(m map[string]string, stepCtx *StepContext) (map[string]string return rendered, nil } +// TODO(mrnugget): This is bad and should be (a) removed or (b) moved to batches package type BatchChangeAttributes struct { Name string Description string @@ -134,7 +135,7 @@ func (stepCtx *StepContext) ToFuncMap() template.FuncMap { // StepResult represents the result of a previously executed step. type StepResult struct { // files are the changes made to files by the step. - files *StepChanges + files *git.Changes // Stdout is the output produced by the step on standard out. Stdout *bytes.Buffer @@ -142,14 +143,6 @@ type StepResult struct { Stderr *bytes.Buffer } -// StepChanges are the changes made to files by a previous step in a repository. -type StepChanges struct { - Modified []string `json:"modified"` - Added []string `json:"added"` - Deleted []string `json:"deleted"` - Renamed []string `json:"renamed"` -} - // ModifiedFiles returns the files modified by a step. func (r StepResult) ModifiedFiles() []string { if r.files != nil { @@ -184,7 +177,7 @@ func (r StepResult) RenamedFiles() []string { type StepsContext struct { // Changes that have been made by executing all steps. - Changes *StepChanges + Changes *git.Changes // Path is the relative-to-root directory in which the steps have been // executed. Default is "". No leading "/". Path string @@ -268,35 +261,3 @@ func renderChangesetTemplateField(name, tmpl string, tmplCtx *ChangesetTemplateC return strings.TrimSpace(out.String()), nil } - -func parseGitStatus(out []byte) (StepChanges, error) { - result := StepChanges{} - - stripped := strings.TrimSpace(string(out)) - if stripped == "" { - return result, nil - } - - for _, line := range strings.Split(stripped, "\n") { - if len(line) < 4 { - return result, fmt.Errorf("git status line has unrecognized format: %q", line) - } - - file := line[3:] - - switch line[0] { - case 'M': - result.Modified = append(result.Modified, file) - case 'A': - result.Added = append(result.Added, file) - case 'D': - result.Deleted = append(result.Deleted, file) - case 'R': - files := strings.Split(file, " -> ") - newFile := files[len(files)-1] - result.Renamed = append(result.Renamed, newFile) - } - } - - return result, nil -} diff --git a/internal/batches/templating_test.go b/internal/batches/executor/templating_test.go similarity index 91% rename from internal/batches/templating_test.go rename to internal/batches/executor/templating_test.go index 7e95ee5364..dcb9839de8 100644 --- a/internal/batches/templating_test.go +++ b/internal/batches/executor/templating_test.go @@ -1,39 +1,15 @@ -package batches +package executor import ( "bytes" "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" "gopkg.in/yaml.v3" ) -func TestParseGitStatus(t *testing.T) { - const input = `M README.md -M another_file.go -A new_file.txt -A barfoo/new_file.txt -D to_be_deleted.txt -R README.md -> README.markdown -` - parsed, err := parseGitStatus([]byte(input)) - if err != nil { - t.Fatal(err) - } - - want := StepChanges{ - Modified: []string{"README.md", "another_file.go"}, - Added: []string{"new_file.txt", "barfoo/new_file.txt"}, - Deleted: []string{"to_be_deleted.txt"}, - Renamed: []string{"README.markdown"}, - } - - if !cmp.Equal(want, parsed) { - t.Fatalf("wrong output:\n%s", cmp.Diff(want, parsed)) - } -} - const rawYaml = `dist: release env: - GO111MODULE=on @@ -60,7 +36,7 @@ func TestRenderStepTemplate(t *testing.T) { Description: "This batch change is just an experiment", }, PreviousStep: StepResult{ - files: &StepChanges{ + files: &git.Changes{ Modified: []string{"go.mod"}, Added: []string{"main.go.swp"}, Deleted: []string{".DS_Store"}, @@ -74,7 +50,7 @@ func TestRenderStepTemplate(t *testing.T) { "project": parsedYaml, }, Step: StepResult{ - files: &StepChanges{ + files: &git.Changes{ Modified: []string{"step-go.mod"}, Added: []string{"step-main.go.swp"}, Deleted: []string{"step-.DS_Store"}, @@ -199,7 +175,7 @@ ${{ step.stderr}} func TestRenderStepMap(t *testing.T) { stepCtx := &StepContext{ PreviousStep: StepResult{ - files: &StepChanges{ + files: &git.Changes{ Modified: []string{"go.mod"}, Added: []string{"main.go.swp"}, Deleted: []string{".DS_Store"}, @@ -266,7 +242,7 @@ func TestRenderChangesetTemplateField(t *testing.T) { }, }, Steps: StepsContext{ - Changes: &StepChanges{ + Changes: &git.Changes{ Modified: []string{"modified-file.txt"}, Added: []string{"added-file.txt"}, Deleted: []string{"deleted-file.txt"}, diff --git a/internal/batches/testdata/dummydocker/docker b/internal/batches/executor/testdata/dummydocker/docker similarity index 100% rename from internal/batches/testdata/dummydocker/docker rename to internal/batches/executor/testdata/dummydocker/docker diff --git a/internal/batches/features.go b/internal/batches/features.go index 7f6d7e614a..3ca075cb41 100644 --- a/internal/batches/features.go +++ b/internal/batches/features.go @@ -5,29 +5,29 @@ import ( "github.com/sourcegraph/src-cli/internal/api" ) -// featureFlags represent features that are only available on certain +// FeatureFlags represent features that are only available on certain // Sourcegraph versions and we therefore have to detect at runtime. -type featureFlags struct { - allowArrayEnvironments bool - includeAutoAuthorDetails bool - useGzipCompression bool - allowtransformChanges bool - allowWorkspaces bool - batchChanges bool +type FeatureFlags struct { + AllowArrayEnvironments bool + IncludeAutoAuthorDetails bool + UseGzipCompression bool + AllowTransformChanges bool + AllowWorkspaces bool + BatchChanges bool } -func (ff *featureFlags) setFromVersion(version string) error { +func (ff *FeatureFlags) SetFromVersion(version string) error { for _, feature := range []struct { flag *bool constraint string minDate string }{ - {&ff.allowArrayEnvironments, ">= 3.23.0", "2020-11-24"}, - {&ff.includeAutoAuthorDetails, ">= 3.20.0", "2020-09-10"}, - {&ff.useGzipCompression, ">= 3.21.0", "2020-10-12"}, - {&ff.allowtransformChanges, ">= 3.23.0", "2020-12-11"}, - {&ff.allowWorkspaces, ">= 3.25.0", "2021-01-29"}, - {&ff.batchChanges, ">= 3.26.0", "2021-03-07"}, + {&ff.AllowArrayEnvironments, ">= 3.23.0", "2020-11-24"}, + {&ff.IncludeAutoAuthorDetails, ">= 3.20.0", "2020-09-10"}, + {&ff.UseGzipCompression, ">= 3.21.0", "2020-10-12"}, + {&ff.AllowTransformChanges, ">= 3.23.0", "2020-12-11"}, + {&ff.AllowWorkspaces, ">= 3.25.0", "2021-01-29"}, + {&ff.BatchChanges, ">= 3.26.0", "2021-03-07"}, } { value, err := api.CheckSourcegraphVersion(version, feature.constraint, feature.minDate) if err != nil { diff --git a/internal/batches/features_test.go b/internal/batches/features_test.go index 85ec59ff2f..0c23368752 100644 --- a/internal/batches/features_test.go +++ b/internal/batches/features_test.go @@ -1,9 +1 @@ package batches - -func featuresAllEnabled() featureFlags { - return featureFlags{ - allowArrayEnvironments: true, - includeAutoAuthorDetails: true, - useGzipCompression: true, - } -} diff --git a/internal/batches/git/changes.go b/internal/batches/git/changes.go new file mode 100644 index 0000000000..8957fa7b85 --- /dev/null +++ b/internal/batches/git/changes.go @@ -0,0 +1,47 @@ +package git + +import ( + "fmt" + "strings" +) + +// Changes are the changes made to files in a repository. +type Changes struct { + Modified []string `json:"modified"` + Added []string `json:"added"` + Deleted []string `json:"deleted"` + Renamed []string `json:"renamed"` +} + +// ParseStatus parses the output of `git status` and turns it into Changes. +func ParseGitStatus(out []byte) (Changes, error) { + result := Changes{} + + stripped := strings.TrimSpace(string(out)) + if stripped == "" { + return result, nil + } + + for _, line := range strings.Split(stripped, "\n") { + if len(line) < 4 { + return result, fmt.Errorf("git status line has unrecognized format: %q", line) + } + + file := line[3:] + + switch line[0] { + case 'M': + result.Modified = append(result.Modified, file) + case 'A': + result.Added = append(result.Added, file) + case 'D': + result.Deleted = append(result.Deleted, file) + case 'R': + files := strings.Split(file, " -> ") + newFile := files[len(files)-1] + result.Renamed = append(result.Renamed, newFile) + } + } + + return result, nil +} diff --git a/internal/batches/git/changes_test.go b/internal/batches/git/changes_test.go new file mode 100644 index 0000000000..7b6b58597c --- /dev/null +++ b/internal/batches/git/changes_test.go @@ -0,0 +1,32 @@ +package git + +import ( + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestParseGitStatus(t *testing.T) { + const input = `M README.md +M another_file.go +A new_file.txt +A barfoo/new_file.txt +D to_be_deleted.txt +R README.md -> README.markdown +` + parsed, err := ParseGitStatus([]byte(input)) + if err != nil { + t.Fatal(err) + } + + want := Changes{ + Modified: []string{"README.md", "another_file.go"}, + Added: []string{"new_file.txt", "barfoo/new_file.txt"}, + Deleted: []string{"to_be_deleted.txt"}, + Renamed: []string{"README.markdown"}, + } + + if !cmp.Equal(want, parsed) { + t.Fatalf("wrong output:\n%s", cmp.Diff(want, parsed)) + } +} diff --git a/internal/batches/log.go b/internal/batches/log.go index 6e321d02cd..3cca5ee277 100644 --- a/internal/batches/log.go +++ b/internal/batches/log.go @@ -24,13 +24,13 @@ func NewLogManager(dir string, keepLogs bool) *LogManager { return &LogManager{dir: dir, keepLogs: keepLogs} } -func (lm *LogManager) AddTask(task *Task) (*TaskLogger, error) { - tl, err := newTaskLogger(task, lm.keepLogs, lm.dir) +func (lm *LogManager) AddTask(slug string) (*TaskLogger, error) { + tl, err := newTaskLogger(slug, lm.keepLogs, lm.dir) if err != nil { return nil, err } - lm.tasks.Store(task, tl) + lm.tasks.Store(slug, tl) return tl, nil } @@ -68,8 +68,8 @@ type TaskLogger struct { keep bool } -func newTaskLogger(task *Task, keep bool, dir string) (*TaskLogger, error) { - prefix := "changeset-" + task.Repository.Slug() +func newTaskLogger(slug string, keep bool, dir string) (*TaskLogger, error) { + prefix := "changeset-" + slug f, err := ioutil.TempFile(dir, prefix+".*.log") if err != nil { diff --git a/internal/batches/mock/image.go b/internal/batches/mock/image.go new file mode 100644 index 0000000000..dad71075e1 --- /dev/null +++ b/internal/batches/mock/image.go @@ -0,0 +1,29 @@ +package mock + +import ( + "context" + + "github.com/sourcegraph/src-cli/internal/batches/docker" +) + +type Image struct { + RawDigest string + DigestErr error + EnsureErr error + UidGid docker.UIDGID + UidGidErr error +} + +var _ docker.Image = &Image{} + +func (image *Image) Digest(ctx context.Context) (string, error) { + return image.RawDigest, image.DigestErr +} + +func (image *Image) Ensure(ctx context.Context) error { + return image.EnsureErr +} + +func (image *Image) UIDGID(ctx context.Context) (docker.UIDGID, error) { + return image.UidGid, image.UidGidErr +} diff --git a/internal/batches/mock/repo_archive.go b/internal/batches/mock/repo_archive.go new file mode 100644 index 0000000000..b753234ab3 --- /dev/null +++ b/internal/batches/mock/repo_archive.go @@ -0,0 +1,86 @@ +package mock + +import ( + "archive/zip" + "fmt" + "log" + "mime" + "net/http" + "path/filepath" + "testing" + + "github.com/sourcegraph/src-cli/internal/batches/graphql" +) + +type RepoArchive struct { + Repo *graphql.Repository + Path string + Files map[string]string +} + +func NewZipArchivesMux(t *testing.T, callback http.HandlerFunc, archives ...RepoArchive) *http.ServeMux { + mux := http.NewServeMux() + + for _, archive := range archives { + files := archive.Files + path := fmt.Sprintf("/%s@%s/-/raw", archive.Repo.Name, archive.Repo.BaseRef()) + if archive.Path != "" { + path = path + "/" + archive.Path + } + + downloadName := filepath.Base(archive.Repo.Name) + mediaType := mime.FormatMediaType("Attachment", map[string]string{ + "filename": downloadName, + }) + + mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Content-Type", "application/zip") + w.Header().Set("Content-Disposition", mediaType) + + zipWriter := zip.NewWriter(w) + for name, body := range files { + f, err := zipWriter.Create(name) + if err != nil { + log.Fatal(err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Errorf("failed to write body for %s to zip: %s", name, err) + } + + if callback != nil { + callback(w, r) + } + } + if err := zipWriter.Close(); err != nil { + t.Fatalf("closing zipWriter failed: %s", err) + } + }) + } + + return mux +} + +type middleware func(http.Handler) http.Handler + +type MockRepoAdditionalFiles struct { + Repo *graphql.Repository + AdditionalFiles map[string]string +} + +func HandleAdditionalFiles(mux *http.ServeMux, files MockRepoAdditionalFiles, middle middleware) { + for name, content := range files.AdditionalFiles { + path := fmt.Sprintf("/%s@%s/-/raw/%s", files.Repo.Name, files.Repo.BaseRef(), name) + handler := func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/plain") + + w.Write([]byte(content)) + } + + if middle != nil { + mux.Handle(path, middle(http.HandlerFunc(handler))) + } else { + mux.HandleFunc(path, handler) + } + } +} diff --git a/internal/batches/repo_fetcher.go b/internal/batches/repo_fetcher.go index 336133472e..b58cd0871d 100644 --- a/internal/batches/repo_fetcher.go +++ b/internal/batches/repo_fetcher.go @@ -26,6 +26,16 @@ type RepoFetcher interface { Checkout(repo *graphql.Repository, path string) RepoZip } +// TODO(mrnugget): just added this for executor tests +func NewRepoFetcher(client api.Client, dir string) RepoFetcher { + return &repoFetcher{client: client, dir: dir} +} + +// TODO(mrnugget): just added this for executor tests +func NewRepoFetcher2(client api.Client, dir string, deleteZips bool) RepoFetcher { + return &repoFetcher{client: client, dir: dir, deleteZips: deleteZips} +} + // repoFetcher is the concrete implementation of the RepoFetcher interface used // outside of tests. type repoFetcher struct { diff --git a/internal/batches/repo_fetcher_test.go b/internal/batches/repo_fetcher_test.go index ff25964c75..e87356e70f 100644 --- a/internal/batches/repo_fetcher_test.go +++ b/internal/batches/repo_fetcher_test.go @@ -17,6 +17,7 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/mock" ) func TestRepoFetcher_Fetch(t *testing.T) { @@ -36,9 +37,9 @@ func TestRepoFetcher_Fetch(t *testing.T) { DefaultBranch: &graphql.Branch{Name: "main", Target: graphql.Target{OID: "d34db33f"}}, } - archive := mockRepoArchive{ - repo: repo, - files: map[string]string{ + archive := mock.RepoArchive{ + Repo: repo, + Files: map[string]string{ "README.md": "# Welcome to the README\n", }, } @@ -49,7 +50,7 @@ func TestRepoFetcher_Fetch(t *testing.T) { requestsReceived++ } - ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) + ts := httptest.NewServer(mock.NewZipArchivesMux(t, callback, archive)) defer ts.Close() var clientBuffer bytes.Buffer @@ -94,7 +95,7 @@ func TestRepoFetcher_Fetch(t *testing.T) { }) t.Run("delete on close", func(t *testing.T) { - ts := httptest.NewServer(newZipArchivesMux(t, nil, archive)) + ts := httptest.NewServer(mock.NewZipArchivesMux(t, nil, archive)) defer ts.Close() var clientBuffer bytes.Buffer @@ -158,7 +159,7 @@ func TestRepoFetcher_Fetch(t *testing.T) { cancel() }() - ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) + ts := httptest.NewServer(mock.NewZipArchivesMux(t, callback, archive)) defer ts.Close() var clientBuffer bytes.Buffer @@ -196,9 +197,9 @@ func TestRepoFetcher_Fetch(t *testing.T) { Branch: graphql.Branch{Name: "other-branch", Target: graphql.Target{OID: otherBranchOID}}, } - archive := mockRepoArchive{repo: repo, files: map[string]string{}} + archive := mock.RepoArchive{Repo: repo, Files: map[string]string{}} - ts := httptest.NewServer(newZipArchivesMux(t, nil, archive)) + ts := httptest.NewServer(mock.NewZipArchivesMux(t, nil, archive)) defer ts.Close() var clientBuffer bytes.Buffer @@ -227,9 +228,9 @@ func TestRepoFetcher_Fetch(t *testing.T) { }) t.Run("path in repository", func(t *testing.T) { - additionalFiles := mockRepoAdditionalFiles{ - repo: repo, - additionalFiles: map[string]string{ + additionalFiles := mock.MockRepoAdditionalFiles{ + Repo: repo, + AdditionalFiles: map[string]string{ ".gitignore": "node_modules", ".gitattributes": "* -text", "a/.gitignore": "node_modules-in-a", @@ -237,10 +238,10 @@ func TestRepoFetcher_Fetch(t *testing.T) { } path := "a/b" - archive := mockRepoArchive{ - repo: repo, - path: path, - files: map[string]string{ + archive := mock.RepoArchive{ + Repo: repo, + Path: path, + Files: map[string]string{ "a/b/1.txt": "this is 1", "a/b/2.txt": "this is 1", }, @@ -262,8 +263,8 @@ func TestRepoFetcher_Fetch(t *testing.T) { }) } - mux := newZipArchivesMux(t, callback, archive) - handleAdditionalFiles(mux, additionalFiles, middle) + mux := mock.NewZipArchivesMux(t, callback, archive) + mock.HandleAdditionalFiles(mux, additionalFiles, middle) ts := httptest.NewServer(mux) defer ts.Close() @@ -302,3 +303,5 @@ func TestRepoFetcher_Fetch(t *testing.T) { } }) } + +func sortStrings(a, b string) bool { return a < b } diff --git a/internal/batches/service.go b/internal/batches/service/service.go similarity index 87% rename from internal/batches/service.go rename to internal/batches/service/service.go index ac5b733cd1..2bf2d24fcb 100644 --- a/internal/batches/service.go +++ b/internal/batches/service/service.go @@ -1,4 +1,4 @@ -package batches +package service import ( "context" @@ -17,7 +17,9 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/docker" + "github.com/sourcegraph/src-cli/internal/batches/executor" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -25,7 +27,7 @@ type Service struct { allowUnsupported bool allowIgnored bool client api.Client - features featureFlags + features batches.FeatureFlags imageCache *docker.ImageCache workspace string } @@ -84,20 +86,20 @@ func (svc *Service) DetermineFeatureFlags(ctx context.Context) error { return errors.Wrap(err, "failed to query Sourcegraph version to check for available features") } - return svc.features.setFromVersion(version) + return svc.features.SetFromVersion(version) } // TODO(campaigns-deprecation): this shim can be removed in Sourcegraph 4.0. func (svc *Service) newOperations() graphql.Operations { return graphql.NewOperations( svc.client, - svc.features.batchChanges, - svc.features.useGzipCompression, + svc.features.BatchChanges, + svc.features.UseGzipCompression, ) } func (svc *Service) newRequest(query string, vars map[string]interface{}) api.Request { - if svc.features.useGzipCompression { + if svc.features.UseGzipCompression { return svc.client.NewGzippedRequest(query, vars) } return svc.client.NewRequest(query, vars) @@ -129,7 +131,7 @@ mutation CreateChangesetSpec($spec: String!) { } ` -func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *ChangesetSpec) (graphql.ChangesetSpecID, error) { +func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *batches.ChangesetSpec) (graphql.ChangesetSpecID, error) { raw, err := json.Marshal(spec) if err != nil { return "", errors.Wrap(err, "marshalling changeset spec JSON") @@ -149,37 +151,23 @@ func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *ChangesetSpec return graphql.ChangesetSpecID(result.CreateChangesetSpec.ID), nil } -func (svc *Service) NewExecutionCache(dir string) ExecutionCache { - if dir == "" { - return &ExecutionNoOpCache{} - } - - return &ExecutionDiskCache{dir} -} - -func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) RepoFetcher { - return &repoFetcher{ - client: svc.client, - dir: dir, - deleteZips: cleanArchives, - } +// TODO(mrnugget): This can be removed +func (svc *Service) NewExecutionCache(dir string) executor.ExecutionCache { + return executor.NewCache(dir) } -func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []Step) WorkspaceCreator { - if svc.workspaceCreatorType(ctx, steps) == workspaceCreatorVolume { - return &dockerVolumeWorkspaceCreator{tempDir: tempDir} - } - return &dockerBindWorkspaceCreator{dir: cacheDir} +// TODO(mrnugget): This can be removed +func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) batches.RepoFetcher { + return batches.NewRepoFetcher2( + svc.client, + dir, + cleanArchives, + ) } -func (svc *Service) workspaceCreatorType(ctx context.Context, steps []Step) workspaceCreatorType { - if svc.workspace == "volume" { - return workspaceCreatorVolume - } else if svc.workspace == "bind" { - return workspaceCreatorBind - } - - return bestWorkspaceCreator(ctx, steps) +// TODO(mrnugget): This can be removed +func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []batches.Step) batches.WorkspaceCreator { + return batches.NewWorkspaceCreator(ctx, svc.workspace, cacheDir, tempDir, steps) } // SetDockerImages updates the steps within the batch spec to include the exact @@ -188,15 +176,15 @@ func (svc *Service) workspaceCreatorType(ctx context.Context, steps []Step) work // // 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 *BatchSpec, 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].image = svc.imageCache.Get(spec.Steps[i].Container) - if err := spec.Steps[i].image.Ensure(ctx); err != nil { + 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) } progress(float64(i) / float64(total)) @@ -204,24 +192,26 @@ func (svc *Service) SetDockerImages(ctx context.Context, spec *BatchSpec, progre // We also need to ensure we have our own utility images available, if // necessary. - if svc.workspaceCreatorType(ctx, spec.Steps) == workspaceCreatorVolume { - if err := svc.imageCache.Get(dockerVolumeWorkspaceImage).Ensure(ctx); err != nil { - return errors.Wrapf(err, "pulling image %q", dockerVolumeWorkspaceImage) - } + + // TODO(mrnugget): figure out how to check for this, but load it always now + // if svc.workspaceCreatorType(ctx, spec.Steps) == workspaceCreatorVolume { + if err := svc.imageCache.Get(batches.DockerVolumeWorkspaceImage).Ensure(ctx); err != nil { + return errors.Wrapf(err, "pulling image %q", batches.DockerVolumeWorkspaceImage) } + // } progress(1) return nil } -func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, spec *BatchSpec) ([]*Task, error) { - workspaceConfigs := []WorkspaceConfiguration{} +func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, spec *batches.BatchSpec) ([]*executor.Task, error) { + workspaceConfigs := []batches.WorkspaceConfiguration{} for _, conf := range spec.Workspaces { g, err := glob.Compile(conf.In) if err != nil { return nil, err } - conf.glob = g + conf.SetGlob(g) workspaceConfigs = append(workspaceConfigs, conf) } @@ -236,7 +226,7 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, matched := false for i, conf := range workspaceConfigs { - if !conf.glob.Match(repo.Name) { + if !conf.Matches(repo.Name) { continue } @@ -257,9 +247,9 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, } } - var tasks []*Task + var tasks []*executor.Task - attr := &BatchChangeAttributes{Name: spec.Name, Description: spec.Description} + attr := &executor.BatchChangeAttributes{Name: spec.Name, Description: spec.Description} for configIndex, repos := range reposByWorkspaceConfig { workspaceConfig := workspaceConfigs[configIndex] @@ -280,7 +270,7 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, } } - tasks = append(tasks, &Task{ + tasks = append(tasks, &executor.Task{ Repository: repo, Path: d, Steps: spec.Steps, @@ -294,7 +284,7 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, } for r := range rootWorkspace { - tasks = append(tasks, &Task{ + tasks = append(tasks, &executor.Task{ Repository: r, Path: "", Steps: spec.Steps, @@ -307,8 +297,8 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, return tasks, nil } -func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts ExecutorOpts, tasks []*Task, spec *BatchSpec, progress func([]*TaskStatus), skipErrors bool) ([]*ChangesetSpec, []string, error) { - x := newExecutor(opts, svc.client, svc.features) +func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts executor.ExecutorOpts, tasks []*executor.Task, spec *batches.BatchSpec, progress func([]*executor.TaskStatus), skipErrors bool) ([]*batches.ChangesetSpec, []string, error) { + x := executor.New(opts, svc.client, svc.features) for _, t := range tasks { x.AddTask(t) } @@ -380,9 +370,9 @@ func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts ExecutorOpts, tas return nil, nil, errors.Errorf("cannot convert value of type %T into a valid external ID: expected string or int", id) } - specs = append(specs, &ChangesetSpec{ + specs = append(specs, &batches.ChangesetSpec{ BaseRepository: repo.ID, - ExternalChangeset: &ExternalChangeset{sid}, + ExternalChangeset: &batches.ExternalChangeset{ExternalID: sid}, }) } } @@ -390,13 +380,13 @@ func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts ExecutorOpts, tas return specs, x.LogFiles(), errs.ErrorOrNil() } -func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs []*ChangesetSpec) error { +func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs []*batches.ChangesetSpec) error { repoByID := make(map[string]*graphql.Repository, len(repos)) for _, repo := range repos { repoByID[repo.ID] = repo } - byRepoAndBranch := make(map[string]map[string][]*ChangesetSpec) + byRepoAndBranch := make(map[string]map[string][]*batches.ChangesetSpec) for _, spec := range specs { // We don't need to validate imported changesets, as they can // never have a critical branch name overlap. @@ -404,7 +394,7 @@ func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs [] continue } if _, ok := byRepoAndBranch[spec.HeadRepository]; !ok { - byRepoAndBranch[spec.HeadRepository] = make(map[string][]*ChangesetSpec) + byRepoAndBranch[spec.HeadRepository] = make(map[string][]*batches.ChangesetSpec) } byRepoAndBranch[spec.HeadRepository][spec.HeadRef] = append(byRepoAndBranch[spec.HeadRepository][spec.HeadRef], spec) @@ -454,13 +444,13 @@ func (e *duplicateBranchesErr) Error() string { return out.String() } -func (svc *Service) ParseBatchSpec(in io.Reader) (*BatchSpec, string, error) { +func (svc *Service) ParseBatchSpec(in io.Reader) (*batches.BatchSpec, string, error) { data, err := ioutil.ReadAll(in) if err != nil { return nil, "", errors.Wrap(err, "reading batch spec") } - spec, err := ParseBatchSpec(data, svc.features) + spec, err := batches.ParseBatchSpec(data, svc.features) if err != nil { return nil, "", errors.Wrap(err, "parsing batch spec") } @@ -529,10 +519,10 @@ func (svc *Service) ResolveNamespace(ctx context.Context, namespace string) (str return "", fmt.Errorf("failed to resolve namespace %q: no user or organization found", namespace) } -func (svc *Service) ResolveRepositories(ctx context.Context, spec *BatchSpec) ([]*graphql.Repository, error) { +func (svc *Service) ResolveRepositories(ctx context.Context, spec *batches.BatchSpec) ([]*graphql.Repository, error) { seen := map[string]*graphql.Repository{} - unsupported := UnsupportedRepoSet{} - ignored := IgnoredRepoSet{} + unsupported := batches.UnsupportedRepoSet{} + ignored := batches.IgnoredRepoSet{} // TODO: this could be trivially parallelised in the future. for _, on := range spec.On { @@ -561,13 +551,13 @@ func (svc *Service) ResolveRepositories(ctx context.Context, spec *BatchSpec) ([ case "github", "gitlab", "bitbucketserver": default: if !svc.allowUnsupported { - unsupported.appendRepo(repo) + unsupported.Append(repo) } } if !svc.allowIgnored { if locations, ok := repoBatchIgnores[repo]; ok && len(locations) > 0 { - ignored.appendRepo(repo) + ignored.Append(repo) } } } else { @@ -581,23 +571,23 @@ func (svc *Service) ResolveRepositories(ctx context.Context, spec *BatchSpec) ([ final := make([]*graphql.Repository, 0, len(seen)) for _, repo := range seen { - if !unsupported.includes(repo) && !ignored.includes(repo) { + if !unsupported.Includes(repo) && !ignored.Includes(repo) { final = append(final, repo) } } - if unsupported.hasUnsupported() { + if unsupported.HasUnsupported() { return final, unsupported } - if ignored.hasIgnored() { + if ignored.HasIgnored() { return final, ignored } return final, nil } -func (svc *Service) ResolveRepositoriesOn(ctx context.Context, on *OnQueryOrRepository) ([]*graphql.Repository, error) { +func (svc *Service) ResolveRepositoriesOn(ctx context.Context, on *batches.OnQueryOrRepository) ([]*graphql.Repository, error) { if on.RepositoriesMatchingQuery != "" { return svc.resolveRepositorySearch(ctx, on.RepositoriesMatchingQuery) } else if on.Repository != "" && on.Branch != "" { diff --git a/internal/batches/service_test.go b/internal/batches/service/service_test.go similarity index 94% rename from internal/batches/service_test.go rename to internal/batches/service/service_test.go index d264d5da4a..704f0eb06d 100644 --- a/internal/batches/service_test.go +++ b/internal/batches/service/service_test.go @@ -1,4 +1,4 @@ -package batches +package service import ( "bytes" @@ -14,6 +14,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -144,8 +145,8 @@ func TestResolveRepositories_Unsupported(t *testing.T) { client, done := mockGraphQLClient(testResolveRepositoriesUnsupported) defer done() - spec := &BatchSpec{ - On: []OnQueryOrRepository{ + spec := &batches.BatchSpec{ + On: []batches.OnQueryOrRepository{ {RepositoriesMatchingQuery: "testquery"}, }, } @@ -166,7 +167,7 @@ func TestResolveRepositories_Unsupported(t *testing.T) { svc := &Service{client: client, allowUnsupported: false, allowIgnored: true} repos, err := svc.ResolveRepositories(context.Background(), spec) - repoSet, ok := err.(UnsupportedRepoSet) + repoSet, ok := err.(batches.UnsupportedRepoSet) if !ok { t.Fatalf("err is not UnsupportedRepoSet") } @@ -224,8 +225,8 @@ const testResolveRepositoriesUnsupported = `{ ` func TestResolveRepositories_Ignored(t *testing.T) { - spec := &BatchSpec{ - On: []OnQueryOrRepository{ + spec := &batches.BatchSpec{ + On: []batches.OnQueryOrRepository{ {RepositoriesMatchingQuery: "testquery"}, }, } @@ -252,7 +253,7 @@ func TestResolveRepositories_Ignored(t *testing.T) { svc := &Service{client: client, allowIgnored: false} repos, err := svc.ResolveRepositories(context.Background(), spec) - ignored, ok := err.(IgnoredRepoSet) + ignored, ok := err.(batches.IgnoredRepoSet) if !ok { t.Fatalf("err is not IgnoredRepoSet: %s", err) } @@ -420,7 +421,7 @@ func TestService_BuildTasks(t *testing.T) { } tests := map[string]struct { - spec *BatchSpec + spec *batches.BatchSpec repos []*graphql.Repository searchResults filesInRepos @@ -431,7 +432,7 @@ func TestService_BuildTasks(t *testing.T) { wantTasks map[string][]wantTask }{ "no workspace configuration": { - spec: &BatchSpec{}, + spec: &batches.BatchSpec{}, repos: repos, searchResults: filesInRepos{}, wantNumTasks: len(repos), @@ -443,8 +444,8 @@ func TestService_BuildTasks(t *testing.T) { }, "workspace configuration matching no repos": { - spec: &BatchSpec{ - Workspaces: []WorkspaceConfiguration{ + spec: &batches.BatchSpec{ + Workspaces: []batches.WorkspaceConfiguration{ {In: "this-does-not-match", RootAtLocationOf: "package.json"}, }, }, @@ -459,8 +460,8 @@ func TestService_BuildTasks(t *testing.T) { }, "workspace configuration matching 2 repos with no results": { - spec: &BatchSpec{ - Workspaces: []WorkspaceConfiguration{ + spec: &batches.BatchSpec{ + Workspaces: []batches.WorkspaceConfiguration{ {In: "*automation-testing", RootAtLocationOf: "package.json"}, }, }, @@ -476,8 +477,8 @@ func TestService_BuildTasks(t *testing.T) { }, "workspace configuration matching 2 repos with 3 results each": { - spec: &BatchSpec{ - Workspaces: []WorkspaceConfiguration{ + spec: &batches.BatchSpec{ + Workspaces: []batches.WorkspaceConfiguration{ {In: "*automation-testing", RootAtLocationOf: "package.json"}, }, }, @@ -503,8 +504,8 @@ func TestService_BuildTasks(t *testing.T) { }, "workspace configuration matches repo with OnlyFetchWorkspace": { - spec: &BatchSpec{ - Workspaces: []WorkspaceConfiguration{ + spec: &batches.BatchSpec{ + Workspaces: []batches.WorkspaceConfiguration{ { OnlyFetchWorkspace: true, In: "*automation-testing", @@ -620,23 +621,23 @@ func TestService_ValidateChangesetSpecs(t *testing.T) { tests := map[string]struct { repos []*graphql.Repository - specs []*ChangesetSpec + specs []*batches.ChangesetSpec wantErrInclude string }{ "no errors": { repos: []*graphql.Repository{repo1, repo2}, - specs: []*ChangesetSpec{ - {CreatedChangeset: &CreatedChangeset{ + specs: []*batches.ChangesetSpec{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-2"}, }, }, @@ -644,8 +645,8 @@ func TestService_ValidateChangesetSpecs(t *testing.T) { "imported changeset": { repos: []*graphql.Repository{repo1}, - specs: []*ChangesetSpec{ - {ExternalChangeset: &ExternalChangeset{ + specs: []*batches.ChangesetSpec{ + {ExternalChangeset: &batches.ExternalChangeset{ ExternalID: "123", }}, }, @@ -654,17 +655,17 @@ func TestService_ValidateChangesetSpecs(t *testing.T) { "duplicate branches": { repos: []*graphql.Repository{repo1, repo2}, - specs: []*ChangesetSpec{ - {CreatedChangeset: &CreatedChangeset{ + specs: []*batches.ChangesetSpec{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, }, - {CreatedChangeset: &CreatedChangeset{ + {CreatedChangeset: &batches.CreatedChangeset{ HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, }, }, diff --git a/internal/batches/step_changes.go b/internal/batches/step_changes.go new file mode 100644 index 0000000000..0c23368752 --- /dev/null +++ b/internal/batches/step_changes.go @@ -0,0 +1 @@ +package batches diff --git a/internal/batches/volume_workspace.go b/internal/batches/volume_workspace.go index 16738a220e..0e38336b01 100644 --- a/internal/batches/volume_workspace.go +++ b/internal/batches/volume_workspace.go @@ -14,6 +14,7 @@ import ( "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/batches/docker" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" "github.com/sourcegraph/src-cli/internal/exec" "github.com/sourcegraph/src-cli/internal/version" @@ -121,7 +122,7 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, }, w.dockerRunOptsWithUser(docker.Root, "/work")...) opts = append( opts, - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", fmt.Sprintf("touch /work/%s; chown -R %s /work", dummy, w.uidGid.String()), ) @@ -140,7 +141,7 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, }, w.dockerRunOptsWithUser(w.uidGid, "/work")...) opts = append( opts, - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", fmt.Sprintf("unzip /tmp/zip; rm /work/%s", dummy), ) @@ -183,7 +184,7 @@ func (wc *dockerVolumeWorkspaceCreator) copyFilesIntoVolumes(ctx context.Context opts = append( opts, - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", strings.Join(copyCmds, " && ")+";", ) @@ -217,7 +218,7 @@ func (w *dockerVolumeWorkspace) DockerRunOpts(ctx context.Context, target string func (w *dockerVolumeWorkspace) WorkDir() *string { return nil } -func (w *dockerVolumeWorkspace) Changes(ctx context.Context) (*StepChanges, error) { +func (w *dockerVolumeWorkspace) Changes(ctx context.Context) (*git.Changes, error) { script := `#!/bin/sh set -e @@ -232,7 +233,7 @@ exec git status --porcelain return nil, errors.Wrap(err, "running git status") } - changes, err := parseGitStatus(out) + changes, err := git.ParseGitStatus(out) if err != nil { return nil, errors.Wrapf(err, "parsing git status output:\n\n%s", string(out)) } @@ -259,10 +260,10 @@ exec git diff --cached --no-prefix --binary return out, nil } -// dockerVolumeWorkspaceImage is the Docker image we'll run our unzip and git +// DockerVolumeWorkspaceImage is the Docker image we'll run our unzip and git // commands in. This needs to match the name defined in // .github/workflows/docker.yml. -var dockerVolumeWorkspaceImage = "sourcegraph/src-batch-change-volume-workspace" +var DockerVolumeWorkspaceImage = "sourcegraph/src-batch-change-volume-workspace" func init() { dockerTag := version.BuildTag @@ -270,7 +271,7 @@ func init() { dockerTag = "latest" } - dockerVolumeWorkspaceImage = dockerVolumeWorkspaceImage + ":" + dockerTag + DockerVolumeWorkspaceImage = DockerVolumeWorkspaceImage + ":" + dockerTag } // runScript is a utility function to mount the given shell script into a Docker @@ -305,7 +306,7 @@ func (w *dockerVolumeWorkspace) runScript(ctx context.Context, target, script st "--workdir", target, "--mount", "type=bind,source=" + name + ",target=/run.sh,ro", }, common...) - opts = append(opts, dockerVolumeWorkspaceImage, "sh", "/run.sh") + opts = append(opts, DockerVolumeWorkspaceImage, "sh", "/run.sh") out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput() if err != nil { diff --git a/internal/batches/volume_workspace_test.go b/internal/batches/volume_workspace_test.go index 98c3ea4c3c..8ec53c57e3 100644 --- a/internal/batches/volume_workspace_test.go +++ b/internal/batches/volume_workspace_test.go @@ -13,7 +13,9 @@ import ( "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/batches/docker" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/mock" "github.com/sourcegraph/src-cli/internal/exec/expect" ) @@ -72,7 +74,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( @@ -82,7 +84,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( @@ -91,7 +93,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, @@ -108,7 +110,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( @@ -118,7 +120,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( @@ -127,12 +129,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, }, "one user:user step": { @@ -146,7 +148,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 1:2 /work", ), expect.NewGlob( @@ -156,7 +158,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "1:2", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( @@ -165,12 +167,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "1:2", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.UIDGID{UID: 1, GID: 2}}}, + {image: &mock.Image{UidGid: docker.UIDGID{UID: 1, GID: 2}}}, }, }, "docker volume create failure": { @@ -181,7 +183,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, wantErr: true, }, @@ -196,12 +198,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, wantErr: true, }, @@ -216,7 +218,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( @@ -226,7 +228,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( @@ -235,12 +237,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, wantErr: true, }, @@ -255,7 +257,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( @@ -265,12 +267,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, wantErr: true, }, @@ -286,7 +288,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "run", "--rm", "--init", "--workdir", "/work", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), expect.NewGlob( @@ -296,7 +298,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/tmp/zip,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "unzip /tmp/zip; rm /work/*", ), expect.NewGlob( @@ -307,7 +309,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=volume,source="+volumeID+",target=/work", "--mount", "type=bind,source="+archiveWithAdditionalFiles.mockAdditionalFilePaths[".gitignore"]+",target=/tmp/.gitignore,ro", "--mount", "type=bind,source="+archiveWithAdditionalFiles.mockAdditionalFilePaths["another-file"]+",target=/tmp/another-file,ro", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "-c", "cp /tmp/.gitignore /work/.gitignore && cp /tmp/another-file /work/another-file;", ), expect.NewGlob( @@ -316,12 +318,12 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), }, steps: []Step{ - {image: &mockImage{uidGid: docker.Root}}, + {image: &mock.Image{UidGid: docker.Root}}, }, }, } { @@ -419,11 +421,11 @@ func TestVolumeWorkspace_Changes(t *testing.T) { t.Run("success", func(t *testing.T) { for name, tc := range map[string]struct { stdout string - want *StepChanges + want *git.Changes }{ "empty": { stdout: "", - want: &StepChanges{}, + want: &git.Changes{}, }, "valid": { stdout: ` @@ -431,7 +433,7 @@ M go.mod M internal/campaigns/volume_workspace.go M internal/campaigns/volume_workspace_test.go `, - want: &StepChanges{Modified: []string{ + want: &git.Changes{Modified: []string{ "go.mod", "internal/campaigns/volume_workspace.go", "internal/campaigns/volume_workspace_test.go", @@ -447,7 +449,7 @@ M internal/campaigns/volume_workspace_test.go "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), ) @@ -479,7 +481,7 @@ M internal/campaigns/volume_workspace_test.go "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), ) @@ -525,7 +527,7 @@ index 06471f4..5f9d3fa 100644 "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), ) @@ -552,7 +554,7 @@ index 06471f4..5f9d3fa 100644 "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ), ) @@ -581,7 +583,7 @@ func TestVolumeWorkspace_runScript(t *testing.T) { "--mount", "type=bind,source=*,target=/run.sh,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - dockerVolumeWorkspaceImage, + DockerVolumeWorkspaceImage, "sh", "/run.sh", ) if err := glob(name, arg...); err != nil { @@ -610,25 +612,3 @@ func TestVolumeWorkspace_runScript(t *testing.T) { t.Fatal(err) } } - -type mockImage struct { - digest string - digestErr error - ensureErr error - uidGid docker.UIDGID - uidGidErr error -} - -var _ docker.Image = &mockImage{} - -func (image *mockImage) Digest(ctx context.Context) (string, error) { - return image.digest, image.digestErr -} - -func (image *mockImage) Ensure(ctx context.Context) error { - return image.ensureErr -} - -func (image *mockImage) UIDGID(ctx context.Context) (docker.UIDGID, error) { - return image.uidGid, image.uidGidErr -} diff --git a/internal/batches/workspace.go b/internal/batches/workspace.go index 3e25a51239..76e0242799 100644 --- a/internal/batches/workspace.go +++ b/internal/batches/workspace.go @@ -4,6 +4,7 @@ import ( "context" "runtime" + "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -35,23 +36,39 @@ type Workspace interface { // Changes is called after each step is executed, and should return the // cumulative file changes that have occurred since Prepare was called. - Changes(ctx context.Context) (*StepChanges, error) + Changes(ctx context.Context) (*git.Changes, error) // Diff should return the total diff for the workspace. This may be called // multiple times in the life of a workspace. Diff(ctx context.Context) ([]byte, error) } -type workspaceCreatorType int +type WorkspaceCreatorType int const ( - workspaceCreatorBind workspaceCreatorType = iota - workspaceCreatorVolume + WorkspaceCreatorBind WorkspaceCreatorType = iota + WorkspaceCreatorVolume ) -// bestWorkspaceCreator determines the correct workspace creator to use based on +func NewWorkspaceCreator(ctx context.Context, preference, cacheDir, tempDir string, steps []Step) WorkspaceCreator { + var workspaceType WorkspaceCreatorType + if preference == "volume" { + workspaceType = WorkspaceCreatorVolume + } else if preference == "bind" { + workspaceType = WorkspaceCreatorBind + } else { + workspaceType = BestWorkspaceCreator(ctx, steps) + } + + if workspaceType == WorkspaceCreatorVolume { + return &dockerVolumeWorkspaceCreator{tempDir: tempDir} + } + return &DockerBindWorkspaceCreator{Dir: cacheDir} +} + +// BestWorkspaceCreator determines the correct workspace creator to use based on // the environment and batch change to be executed. -func bestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { +func BestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCreatorType { // The basic theory here is that we have two options: bind and volume. Bind // is battle tested and always safe, but can be slow on non-Linux platforms // because bind mounts are slow. Volume is faster on those platforms, but @@ -63,13 +80,13 @@ func bestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorTyp // For the time being, we're only going to consider volume mode on Intel // macOS. if runtime.GOOS != "darwin" || runtime.GOARCH != "amd64" { - return workspaceCreatorBind + return WorkspaceCreatorBind } return detectBestWorkspaceCreator(ctx, steps) } -func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCreatorType { +func detectBestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCreatorType { // OK, so we're interested in volume mode, but we need to take its // shortcomings around mixed user environments into account. // @@ -94,15 +111,15 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) workspaceCrea // An error here likely indicates that `id` isn't available on the // path. That's OK: let's not make any assumptions at this point // about the image, and we'll default to the always safe option. - return workspaceCreatorBind + return WorkspaceCreatorBind } if uid == nil { uid = &ug.UID } else if *uid != ug.UID { - return workspaceCreatorBind + return WorkspaceCreatorBind } } - return workspaceCreatorVolume + return WorkspaceCreatorVolume } diff --git a/internal/batches/workspace_test.go b/internal/batches/workspace_test.go index 845584e06a..8436d314f9 100644 --- a/internal/batches/workspace_test.go +++ b/internal/batches/workspace_test.go @@ -7,6 +7,7 @@ import ( "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/batches/docker" + "github.com/sourcegraph/src-cli/internal/batches/mock" "github.com/sourcegraph/src-cli/internal/exec/expect" ) @@ -24,41 +25,41 @@ func TestBestWorkspaceCreator(t *testing.T) { } for name, tc := range map[string]struct { images []docker.Image - want workspaceCreatorType + want WorkspaceCreatorType }{ "nil steps": { images: nil, - want: workspaceCreatorVolume, + want: WorkspaceCreatorVolume, }, "no steps": { images: []docker.Image{}, - want: workspaceCreatorVolume, + want: WorkspaceCreatorVolume, }, "root": { images: []docker.Image{ - &mockImage{uidGid: uidGid(0, 0)}, + &mock.Image{UidGid: uidGid(0, 0)}, }, - want: workspaceCreatorVolume, + want: WorkspaceCreatorVolume, }, "same user": { images: []docker.Image{ - &mockImage{uidGid: uidGid(1000, 1000)}, - &mockImage{uidGid: uidGid(1000, 1000)}, + &mock.Image{UidGid: uidGid(1000, 1000)}, + &mock.Image{UidGid: uidGid(1000, 1000)}, }, - want: workspaceCreatorVolume, + want: WorkspaceCreatorVolume, }, "different user": { images: []docker.Image{ - &mockImage{uidGid: uidGid(1000, 1000)}, - &mockImage{uidGid: uidGid(0, 0)}, + &mock.Image{UidGid: uidGid(1000, 1000)}, + &mock.Image{UidGid: uidGid(0, 0)}, }, - want: workspaceCreatorBind, + want: WorkspaceCreatorBind, }, "id error": { images: []docker.Image{ - &mockImage{uidGidErr: errors.New("foo")}, + &mock.Image{UidGidErr: errors.New("foo")}, }, - want: workspaceCreatorBind, + want: WorkspaceCreatorBind, }, } { t.Run(name, func(t *testing.T) { @@ -73,11 +74,11 @@ func TestBestWorkspaceCreator(t *testing.T) { if isOverridden { // This is an overridden platform, so the workspace type will // always be bind from bestWorkspaceCreator(). - if have, want := bestWorkspaceCreator(ctx, steps), workspaceCreatorBind; have != want { + if have, want := BestWorkspaceCreator(ctx, steps), WorkspaceCreatorBind; have != want { t.Errorf("unexpected creator type on overridden platform: have=%d want=%d", have, want) } } else { - if have := bestWorkspaceCreator(ctx, steps); have != tc.want { + if have := BestWorkspaceCreator(ctx, steps); have != tc.want { t.Errorf("unexpected creator type on non-overridden platform: have=%d want=%d", have, tc.want) } } From 3d0b22a7796b195edce37f4114ae38c2654cb35e Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 14 Apr 2021 13:16:31 +0200 Subject: [PATCH 2/6] Rename Opts in executor --- cmd/src/batch_common.go | 5 +- internal/batches/executor/executor.go | 53 +++++++++++++--------- internal/batches/executor/executor_test.go | 4 +- internal/batches/service/service.go | 7 +-- 4 files changed, 37 insertions(+), 32 deletions(-) diff --git a/cmd/src/batch_common.go b/cmd/src/batch_common.go index f02d4b44eb..5dbfccdf87 100644 --- a/cmd/src/batch_common.go +++ b/cmd/src/batch_common.go @@ -277,13 +277,12 @@ func batchExecute(ctx context.Context, out *output.Output, svc *service.Service, task.Archive = fetcher.Checkout(task.Repository, task.ArchivePathToFetch()) } - opts := executor.ExecutorOpts{ - Cache: svc.NewExecutionCache(flags.cacheDir), + opts := executor.Opts{ + TempDir: flags.tempDir, Creator: workspaceCreator, ClearCache: flags.clearCache, KeepLogs: flags.keepLogs, Timeout: flags.timeout, - TempDir: flags.tempDir, Parallelism: flags.parallelism, } diff --git a/internal/batches/executor/executor.go b/internal/batches/executor/executor.go index caa70e15ee..4f6df3a809 100644 --- a/internal/batches/executor/executor.go +++ b/internal/batches/executor/executor.go @@ -70,6 +70,8 @@ type Task struct { Steps []batches.Step Outputs map[string]interface{} + // TODO(mrnugget): this should just be a single BatchSpec field instead, if + // we can make it work with caching BatchChangeAttributes *BatchChangeAttributes `json:"-"` Template *batches.ChangesetTemplate `json:"-"` TransformChanges *batches.TransformChanges `json:"-"` @@ -161,31 +163,34 @@ func (ts *TaskStatus) FileDiffs() ([]*diff.FileDiff, bool, error) { return ts.fileDiffs, len(ts.fileDiffs) != 0, ts.fileDiffsErr } -type ExecutorOpts struct { - Cache ExecutionCache +type Opts struct { + CacheDir string + ClearCache bool + Creator batches.WorkspaceCreator Parallelism int Timeout time.Duration - ClearCache bool - KeepLogs bool - TempDir string + KeepLogs bool + TempDir string } type executor struct { - ExecutorOpts + cache ExecutionCache + clearCache bool - cache ExecutionCache - client api.Client features batches.FeatureFlags - logger *batches.LogManager - creator batches.WorkspaceCreator + + client api.Client + logger *batches.LogManager + creator batches.WorkspaceCreator tasks []*Task statuses map[*Task]*TaskStatus statusesMu sync.RWMutex tempDir string + timeout time.Duration par *parallel.Run doneEnqueuing chan struct{} @@ -194,16 +199,22 @@ type executor struct { specsMu sync.Mutex } -func New(opts ExecutorOpts, client api.Client, features batches.FeatureFlags) *executor { +// TODO(mrnugget): Why are client and features not part of Opts? +func New(opts Opts, client api.Client, features batches.FeatureFlags) *executor { return &executor{ - ExecutorOpts: opts, - cache: opts.Cache, - creator: opts.Creator, - client: client, - features: features, + cache: NewCache(opts.CacheDir), + clearCache: opts.ClearCache, + + logger: batches.NewLogManager(opts.TempDir, opts.KeepLogs), + creator: opts.Creator, + + client: client, + features: features, + + tempDir: opts.TempDir, + timeout: opts.Timeout, + doneEnqueuing: make(chan struct{}), - logger: batches.NewLogManager(opts.TempDir, opts.KeepLogs), - tempDir: opts.TempDir, par: parallel.NewRun(opts.Parallelism), tasks: []*Task{}, statuses: map[*Task]*TaskStatus{}, @@ -289,7 +300,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { // Check if the task is cached. cacheKey := task.cacheKey() - if x.ClearCache { + if x.clearCache { if err = x.cache.Clear(ctx, cacheKey); err != nil { err = errors.Wrapf(err, "clearing cache for %q", task.Repository.Name) return @@ -360,7 +371,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { }() // Set up our timeout. - runCtx, cancel := context.WithTimeout(ctx, x.Timeout) + runCtx, cancel := context.WithTimeout(ctx, x.timeout) defer cancel() // Actually execute the steps. @@ -382,7 +393,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { result, err := runSteps(runCtx, opts) if err != nil { if reachedTimeout(runCtx, err) { - err = &errTimeoutReached{timeout: x.Timeout} + err = &errTimeoutReached{timeout: x.timeout} } return } diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go index 4497a02811..a5fc48e1e1 100644 --- a/internal/batches/executor/executor_test.go +++ b/internal/batches/executor/executor_test.go @@ -401,8 +401,7 @@ output4=integration-test-batch-change`, cache := newInMemoryExecutionCache() creator := &batches.DockerBindWorkspaceCreator{Dir: testTempDir} - opts := ExecutorOpts{ - Cache: cache, + opts := Opts{ Creator: creator, TempDir: testTempDir, Parallelism: runtime.GOMAXPROCS(0), @@ -419,6 +418,7 @@ output4=integration-test-batch-change`, // and non-cache code paths. execute := func(t *testing.T) { executor := New(opts, client, featuresAllEnabled()) + executor.cache = cache for i := range tc.steps { tc.steps[i].SetImage(&mock.Image{ diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go index 2bf2d24fcb..e843a0221f 100644 --- a/internal/batches/service/service.go +++ b/internal/batches/service/service.go @@ -151,11 +151,6 @@ func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *batches.Chang return graphql.ChangesetSpecID(result.CreateChangesetSpec.ID), nil } -// TODO(mrnugget): This can be removed -func (svc *Service) NewExecutionCache(dir string) executor.ExecutionCache { - return executor.NewCache(dir) -} - // TODO(mrnugget): This can be removed func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) batches.RepoFetcher { return batches.NewRepoFetcher2( @@ -297,7 +292,7 @@ func (svc *Service) BuildTasks(ctx context.Context, repos []*graphql.Repository, return tasks, nil } -func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts executor.ExecutorOpts, tasks []*executor.Task, spec *batches.BatchSpec, progress func([]*executor.TaskStatus), skipErrors bool) ([]*batches.ChangesetSpec, []string, error) { +func (svc *Service) ExecuteBatchSpec(ctx context.Context, opts executor.Opts, tasks []*executor.Task, spec *batches.BatchSpec, progress func([]*executor.TaskStatus), skipErrors bool) ([]*batches.ChangesetSpec, []string, error) { x := executor.New(opts, svc.client, svc.features) for _, t := range tasks { x.AddTask(t) From 10b24aab721e10366de8209314798668c06cf2bc Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 14 Apr 2021 16:15:58 +0200 Subject: [PATCH 3/6] Rename ServiceOpts --- cmd/src/batch_apply.go | 2 +- cmd/src/batch_preview.go | 2 +- cmd/src/batch_repositories.go | 2 +- cmd/src/batch_validate.go | 2 +- internal/batches/executor/executor_test.go | 2 +- internal/batches/repo_fetcher.go | 8 +------- internal/batches/service/service.go | 6 +++--- 7 files changed, 9 insertions(+), 15 deletions(-) diff --git a/cmd/src/batch_apply.go b/cmd/src/batch_apply.go index 1d62c22cdb..5c9ef32997 100644 --- a/cmd/src/batch_apply.go +++ b/cmd/src/batch_apply.go @@ -67,7 +67,7 @@ Examples: ctx, cancel := contextCancelOnInterrupt(context.Background()) defer cancel() - svc := service.NewService(&service.ServiceOpts{ + svc := service.New(&service.Opts{ AllowUnsupported: flags.allowUnsupported, AllowIgnored: flags.allowIgnored, Client: cfg.apiClient(flags.api, flagSet.Output()), diff --git a/cmd/src/batch_preview.go b/cmd/src/batch_preview.go index 312fcd5c47..8331c574a2 100644 --- a/cmd/src/batch_preview.go +++ b/cmd/src/batch_preview.go @@ -42,7 +42,7 @@ Examples: ctx, cancel := contextCancelOnInterrupt(context.Background()) defer cancel() - svc := service.NewService(&service.ServiceOpts{ + svc := service.New(&service.Opts{ AllowUnsupported: flags.allowUnsupported, AllowIgnored: flags.allowIgnored, Client: cfg.apiClient(flags.api, flagSet.Output()), diff --git a/cmd/src/batch_repositories.go b/cmd/src/batch_repositories.go index d3f97f21b8..9be8d2c4c1 100644 --- a/cmd/src/batch_repositories.go +++ b/cmd/src/batch_repositories.go @@ -48,7 +48,7 @@ Examples: ctx := context.Background() client := cfg.apiClient(apiFlags, flagSet.Output()) - svc := service.NewService(&service.ServiceOpts{Client: client}) + svc := service.New(&service.Opts{Client: client}) if err := svc.DetermineFeatureFlags(ctx); err != nil { return err diff --git a/cmd/src/batch_validate.go b/cmd/src/batch_validate.go index 3c638594b7..d0a3235fb4 100644 --- a/cmd/src/batch_validate.go +++ b/cmd/src/batch_validate.go @@ -41,7 +41,7 @@ Examples: } defer specFile.Close() - svc := service.NewService(&service.ServiceOpts{}) + svc := service.New(&service.Opts{}) out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose}) if _, _, err := batchParseSpec(out, svc, specFile); err != nil { diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go index a5fc48e1e1..a9b0b3bc1b 100644 --- a/internal/batches/executor/executor_test.go +++ b/internal/batches/executor/executor_test.go @@ -412,7 +412,7 @@ output4=integration-test-batch-change`, opts.Timeout = 30 * time.Second } - repoFetcher := batches.NewRepoFetcher(client, testTempDir) + repoFetcher := batches.NewRepoFetcher(client, testTempDir, false) // execute contains the actual logic running the tasks on an // executor. We'll run this multiple times to cover both the cache // and non-cache code paths. diff --git a/internal/batches/repo_fetcher.go b/internal/batches/repo_fetcher.go index b58cd0871d..a463c90713 100644 --- a/internal/batches/repo_fetcher.go +++ b/internal/batches/repo_fetcher.go @@ -26,13 +26,7 @@ type RepoFetcher interface { Checkout(repo *graphql.Repository, path string) RepoZip } -// TODO(mrnugget): just added this for executor tests -func NewRepoFetcher(client api.Client, dir string) RepoFetcher { - return &repoFetcher{client: client, dir: dir} -} - -// TODO(mrnugget): just added this for executor tests -func NewRepoFetcher2(client api.Client, dir string, deleteZips bool) RepoFetcher { +func NewRepoFetcher(client api.Client, dir string, deleteZips bool) RepoFetcher { return &repoFetcher{client: client, dir: dir, deleteZips: deleteZips} } diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go index e843a0221f..29c9757f08 100644 --- a/internal/batches/service/service.go +++ b/internal/batches/service/service.go @@ -32,7 +32,7 @@ type Service struct { workspace string } -type ServiceOpts struct { +type Opts struct { AllowUnsupported bool AllowIgnored bool Client api.Client @@ -43,7 +43,7 @@ var ( ErrMalformedOnQueryOrRepository = errors.New("malformed 'on' field; missing either a repository name or a query") ) -func NewService(opts *ServiceOpts) *Service { +func New(opts *Opts) *Service { return &Service{ allowUnsupported: opts.AllowUnsupported, allowIgnored: opts.AllowIgnored, @@ -153,7 +153,7 @@ func (svc *Service) CreateChangesetSpec(ctx context.Context, spec *batches.Chang // TODO(mrnugget): This can be removed func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) batches.RepoFetcher { - return batches.NewRepoFetcher2( + return batches.NewRepoFetcher( svc.client, dir, cleanArchives, From e89fa8bd299626ba5604403d85d718c5fab317d2 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 14 Apr 2021 16:21:15 +0200 Subject: [PATCH 4/6] Move LogManager to log package --- internal/batches/executor/executor.go | 5 +- internal/batches/executor/run_steps.go | 3 +- internal/batches/log/manager.go | 55 +++++++++++++++++++ .../batches/{log.go => log/task_logger.go} | 52 +----------------- 4 files changed, 61 insertions(+), 54 deletions(-) create mode 100644 internal/batches/log/manager.go rename internal/batches/{log.go => log/task_logger.go} (62%) diff --git a/internal/batches/executor/executor.go b/internal/batches/executor/executor.go index 4f6df3a809..f10ee7c7c2 100644 --- a/internal/batches/executor/executor.go +++ b/internal/batches/executor/executor.go @@ -14,6 +14,7 @@ import ( "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/log" ) type TaskExecutionErr struct { @@ -182,7 +183,7 @@ type executor struct { features batches.FeatureFlags client api.Client - logger *batches.LogManager + logger *log.Manager creator batches.WorkspaceCreator tasks []*Task @@ -205,7 +206,7 @@ func New(opts Opts, client api.Client, features batches.FeatureFlags) *executor cache: NewCache(opts.CacheDir), clearCache: opts.ClearCache, - logger: batches.NewLogManager(opts.TempDir, opts.KeepLogs), + logger: log.NewManager(opts.TempDir, opts.KeepLogs), creator: opts.Creator, client: client, diff --git a/internal/batches/executor/run_steps.go b/internal/batches/executor/run_steps.go index d857bc8e20..b217b115ef 100644 --- a/internal/batches/executor/run_steps.go +++ b/internal/batches/executor/run_steps.go @@ -17,6 +17,7 @@ import ( "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/log" yamlv3 "gopkg.in/yaml.v3" ) @@ -49,7 +50,7 @@ type executionOpts struct { tempDir string - logger *batches.TaskLogger + logger *log.TaskLogger reportProgress func(string) } diff --git a/internal/batches/log/manager.go b/internal/batches/log/manager.go new file mode 100644 index 0000000000..525ee7f90f --- /dev/null +++ b/internal/batches/log/manager.go @@ -0,0 +1,55 @@ +package log + +import ( + "sync" + + "github.com/hashicorp/go-multierror" +) + +type Manager struct { + dir string + keepLogs bool + + tasks sync.Map +} + +func NewManager(dir string, keepLogs bool) *Manager { + return &Manager{dir: dir, keepLogs: keepLogs} +} + +func (lm *Manager) AddTask(slug string) (*TaskLogger, error) { + tl, err := newTaskLogger(slug, lm.keepLogs, lm.dir) + if err != nil { + return nil, err + } + + lm.tasks.Store(slug, tl) + return tl, nil +} + +func (lm *Manager) Close() error { + var errs *multierror.Error + + lm.tasks.Range(func(_, v interface{}) bool { + logger := v.(*TaskLogger) + + if err := logger.Close(); err != nil { + errs = multierror.Append(errs, err) + } + + return true + }) + + return errs +} + +func (lm *Manager) LogFiles() []string { + var files []string + + lm.tasks.Range(func(_, v interface{}) bool { + files = append(files, v.(*TaskLogger).Path()) + return true + }) + + return files +} diff --git a/internal/batches/log.go b/internal/batches/log/task_logger.go similarity index 62% rename from internal/batches/log.go rename to internal/batches/log/task_logger.go index 3cca5ee277..4bddcdcb04 100644 --- a/internal/batches/log.go +++ b/internal/batches/log/task_logger.go @@ -1,4 +1,4 @@ -package batches +package log import ( "bytes" @@ -6,61 +6,11 @@ import ( "io" "io/ioutil" "os" - "sync" "time" - "github.com/hashicorp/go-multierror" "github.com/pkg/errors" ) -type LogManager struct { - dir string - keepLogs bool - - tasks sync.Map -} - -func NewLogManager(dir string, keepLogs bool) *LogManager { - return &LogManager{dir: dir, keepLogs: keepLogs} -} - -func (lm *LogManager) AddTask(slug string) (*TaskLogger, error) { - tl, err := newTaskLogger(slug, lm.keepLogs, lm.dir) - if err != nil { - return nil, err - } - - lm.tasks.Store(slug, tl) - return tl, nil -} - -func (lm *LogManager) Close() error { - var errs *multierror.Error - - lm.tasks.Range(func(_, v interface{}) bool { - logger := v.(*TaskLogger) - - if err := logger.Close(); err != nil { - errs = multierror.Append(errs, err) - } - - return true - }) - - return errs -} - -func (lm *LogManager) LogFiles() []string { - var files []string - - lm.tasks.Range(func(_, v interface{}) bool { - files = append(files, v.(*TaskLogger).Path()) - return true - }) - - return files -} - type TaskLogger struct { f *os.File From b27da2ebe5b1556b32fa1b658bf3132b0ac16173 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 14 Apr 2021 17:16:38 +0200 Subject: [PATCH 5/6] workspace pkg, unexport types and rename WorkspaceCreator to Creator --- internal/batches/batch_spec.go | 5 ++ internal/batches/executor/executor.go | 5 +- internal/batches/executor/executor_test.go | 3 +- internal/batches/executor/run_steps.go | 3 +- internal/batches/repo_fetcher.go | 11 +++++ internal/batches/repo_fetcher_test.go | 15 ++++++ internal/batches/service/service.go | 9 ++-- .../batches/{ => workspace}/bind_workspace.go | 16 +++--- .../bind_workspace_nonwin_test.go | 2 +- .../{ => workspace}/bind_workspace_test.go | 26 +++------- .../bind_workspace_windows_test.go | 2 +- internal/batches/{ => workspace}/git.go | 2 +- internal/batches/workspace/main_test.go | 13 +++++ .../{ => workspace}/volume_workspace.go | 9 ++-- .../{ => workspace}/volume_workspace_test.go | 41 +++++++++------- internal/batches/{ => workspace}/workspace.go | 49 ++++++++++--------- .../batches/{ => workspace}/workspace_test.go | 29 +++++------ 17 files changed, 142 insertions(+), 98 deletions(-) rename internal/batches/{ => workspace}/bind_workspace.go (94%) rename internal/batches/{ => workspace}/bind_workspace_nonwin_test.go (98%) rename internal/batches/{ => workspace}/bind_workspace_test.go (94%) rename internal/batches/{ => workspace}/bind_workspace_windows_test.go (97%) rename internal/batches/{ => workspace}/git.go (98%) create mode 100644 internal/batches/workspace/main_test.go rename internal/batches/{ => workspace}/volume_workspace.go (97%) rename internal/batches/{ => workspace}/volume_workspace_test.go (95%) rename internal/batches/{ => workspace}/workspace.go (74%) rename internal/batches/{ => workspace}/workspace_test.go (78%) diff --git a/internal/batches/batch_spec.go b/internal/batches/batch_spec.go index a98ddab881..22fe53d2c1 100644 --- a/internal/batches/batch_spec.go +++ b/internal/batches/batch_spec.go @@ -101,6 +101,7 @@ func (s *Step) SetImage(image docker.Image) { s.image = image } +// TODO(mrnugget): All of these wrappers are not good func (s Step) ImageDigest(ctx context.Context) (string, error) { return s.image.Digest(ctx) } @@ -113,6 +114,10 @@ func (s Step) EnsureImage(ctx context.Context) error { return s.image.Ensure(ctx) } +func (s Step) ImageUIDGID(ctx context.Context) (docker.UIDGID, error) { + return s.image.UIDGID(ctx) +} + type Outputs map[string]Output type Output struct { diff --git a/internal/batches/executor/executor.go b/internal/batches/executor/executor.go index f10ee7c7c2..9061fec867 100644 --- a/internal/batches/executor/executor.go +++ b/internal/batches/executor/executor.go @@ -15,6 +15,7 @@ import ( "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" "github.com/sourcegraph/src-cli/internal/batches/log" + "github.com/sourcegraph/src-cli/internal/batches/workspace" ) type TaskExecutionErr struct { @@ -168,7 +169,7 @@ type Opts struct { CacheDir string ClearCache bool - Creator batches.WorkspaceCreator + Creator workspace.Creator Parallelism int Timeout time.Duration @@ -184,7 +185,7 @@ type executor struct { client api.Client logger *log.Manager - creator batches.WorkspaceCreator + creator workspace.Creator tasks []*Task statuses map[*Task]*TaskStatus diff --git a/internal/batches/executor/executor_test.go b/internal/batches/executor/executor_test.go index a9b0b3bc1b..a62d83df86 100644 --- a/internal/batches/executor/executor_test.go +++ b/internal/batches/executor/executor_test.go @@ -24,6 +24,7 @@ import ( "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" "github.com/sourcegraph/src-cli/internal/batches/mock" + "github.com/sourcegraph/src-cli/internal/batches/workspace" ) func TestExecutor_Integration(t *testing.T) { @@ -400,7 +401,7 @@ output4=integration-test-batch-change`, defer os.Remove(testTempDir) cache := newInMemoryExecutionCache() - creator := &batches.DockerBindWorkspaceCreator{Dir: testTempDir} + creator := workspace.NewCreator(context.Background(), "bind", testTempDir, testTempDir, []batches.Step{}) opts := Opts{ Creator: creator, TempDir: testTempDir, diff --git a/internal/batches/executor/run_steps.go b/internal/batches/executor/run_steps.go index b217b115ef..fed8f908c5 100644 --- a/internal/batches/executor/run_steps.go +++ b/internal/batches/executor/run_steps.go @@ -18,6 +18,7 @@ import ( "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" "github.com/sourcegraph/src-cli/internal/batches/log" + "github.com/sourcegraph/src-cli/internal/batches/workspace" yamlv3 "gopkg.in/yaml.v3" ) @@ -41,7 +42,7 @@ type executionResult struct { type executionOpts struct { archive batches.RepoZip - wc batches.WorkspaceCreator + wc workspace.Creator path string batchChangeAttributes *BatchChangeAttributes diff --git a/internal/batches/repo_fetcher.go b/internal/batches/repo_fetcher.go index a463c90713..d7f5732ce1 100644 --- a/internal/batches/repo_fetcher.go +++ b/internal/batches/repo_fetcher.go @@ -340,3 +340,14 @@ func repositoryRawFileEndpoint(repo *graphql.Repository, pathInRepo string) stri } return p } + +func fileExists(path string) (bool, error) { + _, err := os.Stat(path) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + return true, nil +} diff --git a/internal/batches/repo_fetcher_test.go b/internal/batches/repo_fetcher_test.go index e87356e70f..0bec61de45 100644 --- a/internal/batches/repo_fetcher_test.go +++ b/internal/batches/repo_fetcher_test.go @@ -305,3 +305,18 @@ func TestRepoFetcher_Fetch(t *testing.T) { } func sortStrings(a, b string) bool { return a < b } + +func dirContains(dir, filename string) (bool, error) { + files, err := ioutil.ReadDir(dir) + if err != nil { + return false, err + } + + for _, f := range files { + if f.Name() == filename { + return true, nil + } + } + + return false, nil +} diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go index 29c9757f08..ed79dcdbfd 100644 --- a/internal/batches/service/service.go +++ b/internal/batches/service/service.go @@ -21,6 +21,7 @@ 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 { @@ -161,8 +162,8 @@ func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) batches.RepoF } // TODO(mrnugget): This can be removed -func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []batches.Step) batches.WorkspaceCreator { - return batches.NewWorkspaceCreator(ctx, svc.workspace, cacheDir, tempDir, steps) +func (svc *Service) NewWorkspaceCreator(ctx context.Context, cacheDir, tempDir string, steps []batches.Step) workspace.Creator { + return workspace.NewCreator(ctx, svc.workspace, cacheDir, tempDir, steps) } // SetDockerImages updates the steps within the batch spec to include the exact @@ -190,8 +191,8 @@ func (svc *Service) SetDockerImages(ctx context.Context, spec *batches.BatchSpec // TODO(mrnugget): figure out how to check for this, but load it always now // if svc.workspaceCreatorType(ctx, spec.Steps) == workspaceCreatorVolume { - if err := svc.imageCache.Get(batches.DockerVolumeWorkspaceImage).Ensure(ctx); err != nil { - return errors.Wrapf(err, "pulling image %q", batches.DockerVolumeWorkspaceImage) + if err := svc.imageCache.Get(workspace.DockerVolumeWorkspaceImage).Ensure(ctx); err != nil { + return errors.Wrapf(err, "pulling image %q", workspace.DockerVolumeWorkspaceImage) } // } diff --git a/internal/batches/bind_workspace.go b/internal/batches/workspace/bind_workspace.go similarity index 94% rename from internal/batches/bind_workspace.go rename to internal/batches/workspace/bind_workspace.go index ae8688440a..db86106e3d 100644 --- a/internal/batches/bind_workspace.go +++ b/internal/batches/workspace/bind_workspace.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "archive/zip" @@ -13,18 +13,18 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) -// TODO(mrnugget): unexport this -type DockerBindWorkspaceCreator struct { +type dockerBindWorkspaceCreator struct { Dir string } -var _ WorkspaceCreator = &DockerBindWorkspaceCreator{} +var _ Creator = &dockerBindWorkspaceCreator{} -func (wc *DockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, archive RepoZip) (Workspace, error) { +func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []batches.Step, archive batches.RepoZip) (Workspace, error) { w, err := wc.unzipToWorkspace(ctx, repo, archive.Path()) if err != nil { return nil, errors.Wrap(err, "unzipping the repository") @@ -37,7 +37,7 @@ func (wc *DockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. return w, errors.Wrap(wc.prepareGitRepo(ctx, w), "preparing local git repo") } -func (*DockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerBindWorkspace) error { +func (*dockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerBindWorkspace) error { if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { return errors.Wrap(err, "git init failed") } @@ -53,7 +53,7 @@ func (*DockerBindWorkspaceCreator) prepareGitRepo(ctx context.Context, w *docker return nil } -func (wc *DockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo *graphql.Repository, zip string) (*dockerBindWorkspace, error) { +func (wc *dockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo *graphql.Repository, zip string) (*dockerBindWorkspace, error) { prefix := "workspace-" + repo.Slug() workspace, err := unzipToTempDir(ctx, zip, wc.Dir, prefix) if err != nil { @@ -63,7 +63,7 @@ func (wc *DockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo return &dockerBindWorkspace{dir: workspace}, nil } -func (wc *DockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *dockerBindWorkspace, files map[string]string) error { +func (wc *dockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *dockerBindWorkspace, files map[string]string) error { for name, src := range files { srcStat, err := os.Stat(src) if err != nil { diff --git a/internal/batches/bind_workspace_nonwin_test.go b/internal/batches/workspace/bind_workspace_nonwin_test.go similarity index 98% rename from internal/batches/bind_workspace_nonwin_test.go rename to internal/batches/workspace/bind_workspace_nonwin_test.go index e1e7260b41..a756d236b2 100644 --- a/internal/batches/bind_workspace_nonwin_test.go +++ b/internal/batches/workspace/bind_workspace_nonwin_test.go @@ -1,6 +1,6 @@ // +build !windows -package batches +package workspace import ( "os" diff --git a/internal/batches/bind_workspace_test.go b/internal/batches/workspace/bind_workspace_test.go similarity index 94% rename from internal/batches/bind_workspace_test.go rename to internal/batches/workspace/bind_workspace_test.go index 4319b25852..6d89d43a0b 100644 --- a/internal/batches/bind_workspace_test.go +++ b/internal/batches/workspace/bind_workspace_test.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "archive/zip" @@ -10,6 +10,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -83,7 +84,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) archive := &fakeRepoArchive{mockPath: archivePath} - creator := &DockerBindWorkspaceCreator{Dir: testTempDir} + creator := &dockerBindWorkspaceCreator{Dir: testTempDir} workspace, err := creator.Create(context.Background(), repo, nil, archive) if err != nil { t.Fatalf("unexpected error: %s", err) @@ -113,7 +114,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { badArchive := &fakeRepoArchive{mockPath: badZipFile} - creator := &DockerBindWorkspaceCreator{Dir: testTempDir} + creator := &dockerBindWorkspaceCreator{Dir: testTempDir} if _, err := creator.Create(context.Background(), repo, nil, badArchive); err == nil { t.Error("unexpected nil error") } @@ -127,7 +128,7 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { mockAdditionalFilePaths: additionalFilePaths, } - creator := &DockerBindWorkspaceCreator{Dir: testTempDir} + creator := &dockerBindWorkspaceCreator{Dir: testTempDir} workspace, err := creator.Create(context.Background(), repo, nil, archive) if err != nil { t.Fatalf("unexpected error: %s", err) @@ -316,22 +317,7 @@ func readWorkspaceFiles(workspace Workspace) (map[string]string, error) { return files, err } -func dirContains(dir, filename string) (bool, error) { - files, err := ioutil.ReadDir(dir) - if err != nil { - return false, err - } - - for _, f := range files { - if f.Name() == filename { - return true, nil - } - } - - return false, nil -} - -var _ RepoZip = &fakeRepoArchive{} +var _ batches.RepoZip = &fakeRepoArchive{} type fakeRepoArchive struct { mockPath string diff --git a/internal/batches/bind_workspace_windows_test.go b/internal/batches/workspace/bind_workspace_windows_test.go similarity index 97% rename from internal/batches/bind_workspace_windows_test.go rename to internal/batches/workspace/bind_workspace_windows_test.go index 8eb986505c..8c22d20565 100644 --- a/internal/batches/bind_workspace_windows_test.go +++ b/internal/batches/workspace/bind_workspace_windows_test.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "os" diff --git a/internal/batches/git.go b/internal/batches/workspace/git.go similarity index 98% rename from internal/batches/git.go rename to internal/batches/workspace/git.go index dbc38376de..02e82ffe0e 100644 --- a/internal/batches/git.go +++ b/internal/batches/workspace/git.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "context" diff --git a/internal/batches/workspace/main_test.go b/internal/batches/workspace/main_test.go new file mode 100644 index 0000000000..3c67da93c5 --- /dev/null +++ b/internal/batches/workspace/main_test.go @@ -0,0 +1,13 @@ +package workspace + +import ( + "os" + "testing" + + "github.com/sourcegraph/src-cli/internal/exec/expect" +) + +func TestMain(m *testing.M) { + code := expect.Handle(m) + os.Exit(code) +} diff --git a/internal/batches/volume_workspace.go b/internal/batches/workspace/volume_workspace.go similarity index 97% rename from internal/batches/volume_workspace.go rename to internal/batches/workspace/volume_workspace.go index 0e38336b01..281b94e460 100644 --- a/internal/batches/volume_workspace.go +++ b/internal/batches/workspace/volume_workspace.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "bytes" @@ -13,6 +13,7 @@ import ( "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/docker" "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" @@ -22,9 +23,9 @@ import ( type dockerVolumeWorkspaceCreator struct{ tempDir string } -var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} +var _ Creator = &dockerVolumeWorkspaceCreator{} -func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []Step, archive RepoZip) (Workspace, error) { +func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, steps []batches.Step, archive batches.RepoZip) (Workspace, error) { volume, err := wc.createVolume(ctx) if err != nil { return nil, errors.Wrap(err, "creating Docker volume") @@ -34,7 +35,7 @@ func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphq ug := docker.UIDGID{} if len(steps) > 0 { var err error - if ug, err = steps[0].image.UIDGID(ctx); err != nil { + if ug, err = steps[0].ImageUIDGID(ctx); err != nil { return nil, errors.Wrap(err, "getting container UID and GID") } } diff --git a/internal/batches/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go similarity index 95% rename from internal/batches/volume_workspace_test.go rename to internal/batches/workspace/volume_workspace_test.go index 8ec53c57e3..eaae95675a 100644 --- a/internal/batches/volume_workspace_test.go +++ b/internal/batches/workspace/volume_workspace_test.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "bytes" @@ -12,6 +12,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/docker" "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" @@ -57,10 +58,16 @@ func TestVolumeWorkspaceCreator(t *testing.T) { DefaultBranch: &graphql.Branch{Name: "main"}, } + stepWithImage := func(image docker.Image) batches.Step { + s := batches.Step{} + s.SetImage(image) + return s + } + for name, tc := range map[string]struct { archive *fakeRepoArchive expectations []*expect.Expectation - steps []Step + steps []batches.Step wantErr bool }{ "no steps": { @@ -97,7 +104,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "/run.sh", ), }, - steps: []Step{}, + steps: []batches.Step{}, }, "one root:root step": { expectations: []*expect.Expectation{ @@ -133,8 +140,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "/run.sh", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, }, "one user:user step": { @@ -171,8 +178,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "/run.sh", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.UIDGID{UID: 1, GID: 2}}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.UIDGID{UID: 1, GID: 2}}), }, }, "docker volume create failure": { @@ -182,8 +189,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "docker", "volume", "create", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, wantErr: true, }, @@ -202,8 +209,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "touch /work/*; chown -R 0:0 /work", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, wantErr: true, }, @@ -241,8 +248,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "/run.sh", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, wantErr: true, }, @@ -271,8 +278,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "-c", "unzip /tmp/zip; rm /work/*", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, wantErr: true, }, @@ -322,8 +329,8 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "sh", "/run.sh", ), }, - steps: []Step{ - {image: &mock.Image{UidGid: docker.Root}}, + steps: []batches.Step{ + stepWithImage(&mock.Image{UidGid: docker.Root}), }, }, } { diff --git a/internal/batches/workspace.go b/internal/batches/workspace/workspace.go similarity index 74% rename from internal/batches/workspace.go rename to internal/batches/workspace/workspace.go index 76e0242799..e9498541a1 100644 --- a/internal/batches/workspace.go +++ b/internal/batches/workspace/workspace.go @@ -1,19 +1,20 @@ -package batches +package workspace import ( "context" "runtime" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/git" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) -// WorkspaceCreator implementations are used to create workspaces, which manage +// Creator implementations are used to create workspaces, which manage // per-changeset persistent storage when executing batch change steps and are // responsible for ultimately generating a diff. -type WorkspaceCreator interface { +type Creator interface { // Create creates a new workspace for the given repository and archive file. - Create(ctx context.Context, repo *graphql.Repository, steps []Step, archive RepoZip) (Workspace, error) + Create(ctx context.Context, repo *graphql.Repository, steps []batches.Step, archive batches.RepoZip) (Workspace, error) } // Workspace implementations manage per-changeset storage when executing batch @@ -43,32 +44,32 @@ type Workspace interface { Diff(ctx context.Context) ([]byte, error) } -type WorkspaceCreatorType int +type CreatorType int const ( - WorkspaceCreatorBind WorkspaceCreatorType = iota - WorkspaceCreatorVolume + CreatorBind CreatorType = iota + CreatorVolume ) -func NewWorkspaceCreator(ctx context.Context, preference, cacheDir, tempDir string, steps []Step) WorkspaceCreator { - var workspaceType WorkspaceCreatorType +func NewCreator(ctx context.Context, preference, cacheDir, tempDir string, steps []batches.Step) Creator { + var workspaceType CreatorType if preference == "volume" { - workspaceType = WorkspaceCreatorVolume + workspaceType = CreatorVolume } else if preference == "bind" { - workspaceType = WorkspaceCreatorBind + workspaceType = CreatorBind } else { - workspaceType = BestWorkspaceCreator(ctx, steps) + workspaceType = BestCreatorType(ctx, steps) } - if workspaceType == WorkspaceCreatorVolume { + if workspaceType == CreatorVolume { return &dockerVolumeWorkspaceCreator{tempDir: tempDir} } - return &DockerBindWorkspaceCreator{Dir: cacheDir} + return &dockerBindWorkspaceCreator{Dir: cacheDir} } -// BestWorkspaceCreator determines the correct workspace creator to use based on -// the environment and batch change to be executed. -func BestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCreatorType { +// BestCreatorType determines the correct workspace creator type to use based +// on the environment and batch change to be executed. +func BestCreatorType(ctx context.Context, steps []batches.Step) CreatorType { // The basic theory here is that we have two options: bind and volume. Bind // is battle tested and always safe, but can be slow on non-Linux platforms // because bind mounts are slow. Volume is faster on those platforms, but @@ -80,13 +81,13 @@ func BestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCreatorTyp // For the time being, we're only going to consider volume mode on Intel // macOS. if runtime.GOOS != "darwin" || runtime.GOARCH != "amd64" { - return WorkspaceCreatorBind + return CreatorBind } - return detectBestWorkspaceCreator(ctx, steps) + return detectBestCreatorType(ctx, steps) } -func detectBestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCreatorType { +func detectBestCreatorType(ctx context.Context, steps []batches.Step) CreatorType { // OK, so we're interested in volume mode, but we need to take its // shortcomings around mixed user environments into account. // @@ -106,20 +107,20 @@ func detectBestWorkspaceCreator(ctx context.Context, steps []Step) WorkspaceCrea var uid *int for _, step := range steps { - ug, err := step.image.UIDGID(ctx) + ug, err := step.ImageUIDGID(ctx) if err != nil { // An error here likely indicates that `id` isn't available on the // path. That's OK: let's not make any assumptions at this point // about the image, and we'll default to the always safe option. - return WorkspaceCreatorBind + return CreatorBind } if uid == nil { uid = &ug.UID } else if *uid != ug.UID { - return WorkspaceCreatorBind + return CreatorBind } } - return WorkspaceCreatorVolume + return CreatorVolume } diff --git a/internal/batches/workspace_test.go b/internal/batches/workspace/workspace_test.go similarity index 78% rename from internal/batches/workspace_test.go rename to internal/batches/workspace/workspace_test.go index 8436d314f9..2b4c39368d 100644 --- a/internal/batches/workspace_test.go +++ b/internal/batches/workspace/workspace_test.go @@ -1,4 +1,4 @@ -package batches +package workspace import ( "context" @@ -6,6 +6,7 @@ import ( "testing" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/docker" "github.com/sourcegraph/src-cli/internal/batches/mock" "github.com/sourcegraph/src-cli/internal/exec/expect" @@ -25,60 +26,60 @@ func TestBestWorkspaceCreator(t *testing.T) { } for name, tc := range map[string]struct { images []docker.Image - want WorkspaceCreatorType + want CreatorType }{ "nil steps": { images: nil, - want: WorkspaceCreatorVolume, + want: CreatorVolume, }, "no steps": { images: []docker.Image{}, - want: WorkspaceCreatorVolume, + want: CreatorVolume, }, "root": { images: []docker.Image{ &mock.Image{UidGid: uidGid(0, 0)}, }, - want: WorkspaceCreatorVolume, + want: CreatorVolume, }, "same user": { images: []docker.Image{ &mock.Image{UidGid: uidGid(1000, 1000)}, &mock.Image{UidGid: uidGid(1000, 1000)}, }, - want: WorkspaceCreatorVolume, + want: CreatorVolume, }, "different user": { images: []docker.Image{ &mock.Image{UidGid: uidGid(1000, 1000)}, &mock.Image{UidGid: uidGid(0, 0)}, }, - want: WorkspaceCreatorBind, + want: CreatorBind, }, "id error": { images: []docker.Image{ &mock.Image{UidGidErr: errors.New("foo")}, }, - want: WorkspaceCreatorBind, + want: CreatorBind, }, } { t.Run(name, func(t *testing.T) { - var steps []Step + var steps []batches.Step if tc.images != nil { - steps = make([]Step, len(tc.images)) + steps = make([]batches.Step, len(tc.images)) for i, image := range tc.images { - steps[i].image = image + steps[i].SetImage(image) } } if isOverridden { // This is an overridden platform, so the workspace type will // always be bind from bestWorkspaceCreator(). - if have, want := BestWorkspaceCreator(ctx, steps), WorkspaceCreatorBind; have != want { + if have, want := BestCreatorType(ctx, steps), CreatorBind; have != want { t.Errorf("unexpected creator type on overridden platform: have=%d want=%d", have, want) } } else { - if have := BestWorkspaceCreator(ctx, steps); have != tc.want { + if have := BestCreatorType(ctx, steps); have != tc.want { t.Errorf("unexpected creator type on non-overridden platform: have=%d want=%d", have, tc.want) } } @@ -86,7 +87,7 @@ func TestBestWorkspaceCreator(t *testing.T) { // Regardless of what bestWorkspaceCreator() would have done, let's // test that the right thing happens regardless if detection were to // actually occur. - have := detectBestWorkspaceCreator(ctx, steps) + have := detectBestCreatorType(ctx, steps) if have != tc.want { t.Errorf("unexpected creator type: have=%d want=%d", have, tc.want) } From 39c144c2ae5e8b9d5c952412d49f88f00f72ce6e Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 15 Apr 2021 13:58:40 +0200 Subject: [PATCH 6/6] Remove unused file --- internal/batches/step_changes.go | 1 - 1 file changed, 1 deletion(-) delete mode 100644 internal/batches/step_changes.go diff --git a/internal/batches/step_changes.go b/internal/batches/step_changes.go deleted file mode 100644 index 0c23368752..0000000000 --- a/internal/batches/step_changes.go +++ /dev/null @@ -1 +0,0 @@ -package batches