From f32ea102d42856e614b9a1880301d434df9d1a39 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 16 Dec 2020 17:39:48 -0800 Subject: [PATCH 01/52] Initial refactor of workspace concerns. --- internal/campaigns/archive_fetcher.go | 75 +++++++++++++++++++++++-- internal/campaigns/docker_workspace.go | 17 ++++++ internal/campaigns/executor.go | 2 +- internal/campaigns/git.go | 33 +++++++++++ internal/campaigns/run_steps.go | 77 ++++++-------------------- internal/campaigns/service.go | 6 +- internal/campaigns/workspace.go | 20 +++++++ 7 files changed, 160 insertions(+), 70 deletions(-) create mode 100644 internal/campaigns/docker_workspace.go create mode 100644 internal/campaigns/git.go create mode 100644 internal/campaigns/workspace.go diff --git a/internal/campaigns/archive_fetcher.go b/internal/campaigns/archive_fetcher.go index 69d4a61fb9..baca3e85f5 100644 --- a/internal/campaigns/archive_fetcher.go +++ b/internal/campaigns/archive_fetcher.go @@ -18,19 +18,21 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -type WorkspaceCreator struct { +type workspaceCreator struct { dir string client api.Client deleteZips bool } -func (wc *WorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (string, error) { +var _ WorkspaceCreator = &workspaceCreator{} + +func (wc *workspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { path := localRepositoryZipArchivePath(wc.dir, repo) exists, err := fileExists(path) if err != nil { - return "", err + return nil, err } if !exists { @@ -41,7 +43,7 @@ func (wc *WorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository // downloaded file. os.Remove(path) - return "", errors.Wrap(err, "fetching ZIP archive") + return nil, errors.Wrap(err, "fetching ZIP archive") } } @@ -52,10 +54,71 @@ func (wc *WorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository prefix := "workspace-" + repo.Slug() workspace, err := unzipToTempDir(ctx, path, wc.dir, prefix) if err != nil { - return "", errors.Wrap(err, "unzipping the ZIP archive") + return nil, errors.Wrap(err, "unzipping the ZIP archive") + } + + return &bindWorkspace{dir: workspace}, nil +} + +type bindWorkspace struct { + dir string +} + +var _ Workspace = &bindWorkspace{} + +func (w *bindWorkspace) Close(ctx context.Context) error { + return os.RemoveAll(w.dir) +} + +func (w *bindWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { + return []string{ + "--mount", + fmt.Sprintf("type=bind,source=%s,target=%s", w.dir, target), + }, nil +} + +func (w *bindWorkspace) Prepare(ctx context.Context) error { + if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { + return errors.Wrap(err, "git init failed") + } + + // --force because we want previously "gitignored" files in the repository + if _, err := runGitCmd(ctx, w.dir, "add", "--force", "--all"); err != nil { + return errors.Wrap(err, "git add failed") } + if _, err := runGitCmd(ctx, w.dir, "commit", "--quiet", "--all", "-m", "src-action-exec"); err != nil { + return errors.Wrap(err, "git commit failed") + } + + return nil +} + +func (w *bindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { + if _, err := runGitCmd(ctx, w.dir, "add", "--all"); err != nil { + return nil, errors.Wrap(err, "git add failed") + } + + statusOut, err := runGitCmd(ctx, w.dir, "status", "--porcelain") + if err != nil { + return nil, errors.Wrap(err, "git status failed") + } + + changes, err := parseGitStatus(statusOut) + if err != nil { + return nil, errors.Wrap(err, "parsing git status output") + } + + return &changes, nil +} - return workspace, nil +func (w *bindWorkspace) Diff(ctx context.Context) ([]byte, error) { + // As of Sourcegraph 3.14 we only support unified diff format. + // That means we need to strip away the `a/` and `/b` prefixes with `--no-prefix`. + // See: https://github.com/sourcegraph/sourcegraph/blob/82d5e7e1562fef6be5c0b17f18631040fd330835/enterprise/internal/campaigns/service.go#L324-L329 + // + // Also, we need to add --binary so binary file changes are inlined in the patch. + // + return runGitCmd(ctx, w.dir, "diff", "--cached", "--no-prefix", "--binary") } func fileExists(path string) (bool, error) { diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/docker_workspace.go new file mode 100644 index 0000000000..b30633b10c --- /dev/null +++ b/internal/campaigns/docker_workspace.go @@ -0,0 +1,17 @@ +package campaigns + +import ( + "context" + + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" +) + +type dockerWorkspaceCreator struct { + dir string +} + +var _ WorkspaceCreator = &dockerWorkspaceCreator{} + +func (w *dockerWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { + return nil, nil +} diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index a9824f956a..ea05995863 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -143,7 +143,7 @@ type executor struct { client api.Client features featureFlags logger *LogManager - creator *WorkspaceCreator + creator WorkspaceCreator tasks []*Task statuses map[*Task]*TaskStatus diff --git a/internal/campaigns/git.go b/internal/campaigns/git.go new file mode 100644 index 0000000000..5356e2cfcb --- /dev/null +++ b/internal/campaigns/git.go @@ -0,0 +1,33 @@ +package campaigns + +import ( + "context" + "os/exec" + "strings" + + "github.com/pkg/errors" +) + +func runGitCmd(ctx context.Context, dir string, args ...string) ([]byte, error) { + cmd := exec.CommandContext(ctx, "git", args...) + cmd.Env = []string{ + // Don't use the system wide git config. + "GIT_CONFIG_NOSYSTEM=1", + // And also not any other, because they can mess up output, change defaults, .. which can do unexpected things. + "GIT_CONFIG=/dev/null", + // Set user.name and user.email in the local repository. The user name and + // e-mail will eventually be ignored anyway, since we're just using the Git + // repository to generate diffs, but we don't want git to generate alarming + // looking warnings. + "GIT_AUTHOR_NAME=Sourcegraph", + "GIT_AUTHOR_EMAIL=campaigns@sourcegraph.com", + "GIT_COMMITTER_NAME=Sourcegraph", + "GIT_COMMITTER_EMAIL=campaigns@sourcegraph.com", + } + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err != nil { + return nil, errors.Wrapf(err, "'git %s' failed: %s", strings.Join(args, " "), out) + } + return out, nil +} diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index d400961675..68c9bac4e7 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -17,50 +17,18 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { +func runSteps(ctx context.Context, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { reportProgress("Downloading archive") - volumeDir, err := wc.Create(ctx, repo) + workspace, err := wc.Create(ctx, repo) if err != nil { return nil, errors.Wrap(err, "creating workspace") } - defer os.RemoveAll(volumeDir) - - runGitCmd := func(args ...string) ([]byte, error) { - cmd := exec.CommandContext(ctx, "git", args...) - cmd.Env = []string{ - // Don't use the system wide git config. - "GIT_CONFIG_NOSYSTEM=1", - // And also not any other, because they can mess up output, change defaults, .. which can do unexpected things. - "GIT_CONFIG=/dev/null", - // Set user.name and user.email in the local repository. The user name and - // e-mail will eventually be ignored anyway, since we're just using the Git - // repository to generate diffs, but we don't want git to generate alarming - // looking warnings. - "GIT_AUTHOR_NAME=Sourcegraph", - "GIT_AUTHOR_EMAIL=campaigns@sourcegraph.com", - "GIT_COMMITTER_NAME=Sourcegraph", - "GIT_COMMITTER_EMAIL=campaigns@sourcegraph.com", - } - cmd.Dir = volumeDir - out, err := cmd.CombinedOutput() - if err != nil { - return nil, errors.Wrapf(err, "'git %s' failed: %s", strings.Join(args, " "), out) - } - return out, nil - } + defer workspace.Close(ctx) reportProgress("Initializing workspace") - if _, err := runGitCmd("init"); err != nil { - return nil, errors.Wrap(err, "git init failed") - } - - // --force because we want previously "gitignored" files in the repository - if _, err := runGitCmd("add", "--force", "--all"); err != nil { - return nil, errors.Wrap(err, "git add failed") - } - if _, err := runGitCmd("commit", "--quiet", "--all", "-m", "src-action-exec"); err != nil { - return nil, errors.Wrap(err, "git commit failed") + if err := workspace.Prepare(ctx); err != nil { + return nil, errors.Wrap(err, "initializing workspace") } results := make([]StepResult, len(steps)) @@ -184,15 +152,18 @@ func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repositor reportProgress(runScript.String()) const workDir = "/work" - args := []string{ + workspaceOpts, err := workspace.DockerRunOpts(ctx, workDir) + if err != nil { + return nil, errors.Wrap(err, "getting Docker options for workspace") + } + args := append([]string{ "run", "--rm", "--init", "--cidfile", cidFile.Name(), "--workdir", workDir, - "--mount", fmt.Sprintf("type=bind,source=%s,target=%s", volumeDir, workDir), "--mount", fmt.Sprintf("type=bind,source=%s,target=%s,ro", runScriptFile.Name(), containerTemp), - } + }, workspaceOpts...) for target, source := range filesToMount { args = append(args, "--mount", fmt.Sprintf("type=bind,source=%s,target=%s,ro", source.Name(), target)) } @@ -205,7 +176,8 @@ func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repositor cmd := exec.CommandContext(ctx, "docker", args...) cmd.Args = append(cmd.Args, "--", step.image, containerTemp) - cmd.Dir = volumeDir + // XXX: should be moot now? + //cmd.Dir = volumeDir var stdoutBuffer, stderrBuffer bytes.Buffer cmd.Stdout = io.MultiWriter(&stdoutBuffer, logger.PrefixWriter("stdout")) @@ -233,31 +205,16 @@ func runSteps(ctx context.Context, wc *WorkspaceCreator, repo *graphql.Repositor logger.Logf("[Step %d] complete in %s", i+1, elapsed) - if _, err := runGitCmd("add", "--all"); err != nil { - return nil, errors.Wrap(err, "git add failed") - } - - statusOut, err := runGitCmd("status", "--porcelain") - if err != nil { - return nil, errors.Wrap(err, "git status failed") - } - - changes, err := parseGitStatus(statusOut) + changes, err := workspace.Changes(ctx) if err != nil { - return nil, errors.Wrap(err, "parsing git status output") + return nil, errors.Wrap(err, "getting changed files in step") } results[i] = StepResult{Files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} } reportProgress("Calculating diff") - // As of Sourcegraph 3.14 we only support unified diff format. - // That means we need to strip away the `a/` and `/b` prefixes with `--no-prefix`. - // See: https://github.com/sourcegraph/sourcegraph/blob/82d5e7e1562fef6be5c0b17f18631040fd330835/enterprise/internal/campaigns/service.go#L324-L329 - // - // Also, we need to add --binary so binary file changes are inlined in the patch. - // - diffOut, err := runGitCmd("diff", "--cached", "--no-prefix", "--binary") + diffOut, err := workspace.Diff(ctx) if err != nil { return nil, errors.Wrap(err, "git diff failed") } @@ -478,7 +435,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 *StepChanges // Stdout is the output produced by the step on standard out. Stdout *bytes.Buffer diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index d25dc9ec6a..72a433fa83 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -185,7 +185,7 @@ func (svc *Service) NewExecutionCache(dir string) ExecutionCache { type ExecutorOpts struct { Cache ExecutionCache - Creator *WorkspaceCreator + Creator WorkspaceCreator Parallelism int Timeout time.Duration @@ -199,8 +199,8 @@ func (svc *Service) NewExecutor(opts ExecutorOpts) Executor { return newExecutor(opts, svc.client, svc.features) } -func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) *WorkspaceCreator { - return &WorkspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} +func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) WorkspaceCreator { + return &workspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} } func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, progress func(i int)) error { diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go new file mode 100644 index 0000000000..b3216d4a1f --- /dev/null +++ b/internal/campaigns/workspace.go @@ -0,0 +1,20 @@ +package campaigns + +import ( + "context" + + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" +) + +type WorkspaceCreator interface { + Create(context.Context, *graphql.Repository) (Workspace, error) +} + +type Workspace interface { + Close(ctx context.Context) error + DockerRunOpts(ctx context.Context, target string) ([]string, error) + Prepare(ctx context.Context) error + + Changes(ctx context.Context) (*StepChanges, error) + Diff(ctx context.Context) ([]byte, error) +} From fe2798c1f6f2809ca1d07f3202e62213ce0a75db Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Fri, 18 Dec 2020 18:00:33 -0800 Subject: [PATCH 02/52] More WIP. --- campaign-container/Dockerfile | 5 + campaign-container/build.sh | 6 + cmd/src/campaigns_apply.go | 1 + cmd/src/campaigns_common.go | 5 +- cmd/src/campaigns_preview.go | 1 + internal/campaigns/docker_workspace.go | 191 ++++++++++++++++++++++++- internal/campaigns/service.go | 11 ++ 7 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 campaign-container/Dockerfile create mode 100755 campaign-container/build.sh diff --git a/campaign-container/Dockerfile b/campaign-container/Dockerfile new file mode 100644 index 0000000000..b740969ec4 --- /dev/null +++ b/campaign-container/Dockerfile @@ -0,0 +1,5 @@ +FROM alpine + +RUN apk add --update curl git unzip && \ + git config --global user.email campaigns@sourcegraph.com && \ + git config --global user.name 'Sourcegraph Campaigns' \ No newline at end of file diff --git a/campaign-container/build.sh b/campaign-container/build.sh new file mode 100755 index 0000000000..1b713642b5 --- /dev/null +++ b/campaign-container/build.sh @@ -0,0 +1,6 @@ +#!/bin/sh + +set -e +set -x + +exec docker build -t sourcegraph/src-campaign-workspace - <"$(dirname "$0")/Dockerfile" diff --git a/cmd/src/campaigns_apply.go b/cmd/src/campaigns_apply.go index 958550ed67..bbafd5260e 100644 --- a/cmd/src/campaigns_apply.go +++ b/cmd/src/campaigns_apply.go @@ -70,6 +70,7 @@ Examples: svc := campaigns.NewService(&campaigns.ServiceOpts{ AllowUnsupported: flags.allowUnsupported, Client: cfg.apiClient(flags.api, flagSet.Output()), + Workspace: flags.workspace, }) if err := svc.DetermineFeatureFlags(ctx); err != nil { diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 3c13e6c401..6d3c1d42af 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -40,6 +40,7 @@ type campaignsApplyFlags struct { namespace string parallelism int timeout time.Duration + workspace string cleanArchives bool skipErrors bool } @@ -99,6 +100,7 @@ func newCampaignsApplyFlags(flagSet *flag.FlagSet, cacheDir, tempDir string) *ca &caf.skipErrors, "skip-errors", false, "If true, errors encountered while executing steps in a repository won't stop the execution of the campaign spec but only cause that repository to be skipped.", ) + flagSet.StringVar(&caf.workspace, "workspace", "bind", "Workspace mode to use (bind or volume)") flagSet.BoolVar(verbose, "v", false, "print verbose output") @@ -188,6 +190,7 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se } else { opts.Parallelism = flags.parallelism } + out.VerboseLine(output.Linef("🚧", output.StyleSuccess, "Workspace creator: %T", opts.Creator)) executor := svc.NewExecutor(opts) if errs != nil { @@ -210,7 +213,7 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se imageProgress := out.Progress([]output.ProgressBar{{ Label: "Preparing container images", - Max: float64(len(campaignSpec.Steps)), + Max: float64(len(campaignSpec.Steps) + 1), }}, nil) err = svc.SetDockerImages(ctx, campaignSpec, func(step int) { imageProgress.SetValue(0, float64(step)) diff --git a/cmd/src/campaigns_preview.go b/cmd/src/campaigns_preview.go index 83ce4b98f3..fdd5ce7fd1 100644 --- a/cmd/src/campaigns_preview.go +++ b/cmd/src/campaigns_preview.go @@ -45,6 +45,7 @@ Examples: svc := campaigns.NewService(&campaigns.ServiceOpts{ AllowUnsupported: flags.allowUnsupported, Client: cfg.apiClient(flags.api, flagSet.Output()), + Workspace: flags.workspace, }) if err := svc.DetermineFeatureFlags(ctx); err != nil { diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/docker_workspace.go index b30633b10c..b0f986186d 100644 --- a/internal/campaigns/docker_workspace.go +++ b/internal/campaigns/docker_workspace.go @@ -1,17 +1,202 @@ package campaigns import ( + "bytes" "context" + "fmt" + "io" + "io/ioutil" + "net/http" + "os" + "os/exec" + "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) type dockerWorkspaceCreator struct { - dir string + client api.Client } var _ WorkspaceCreator = &dockerWorkspaceCreator{} -func (w *dockerWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { - return nil, nil +func (wc *dockerWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { + w := &dockerWorkspace{} + return w, w.init(ctx, wc.client, repo) +} + +// TODO: migrate to a real image on Docker Hub. +const baseImage = "sourcegraph/src-campaign-workspace" + +type dockerWorkspace struct { + volume string +} + +var _ Workspace = &dockerWorkspace{} + +func (w *dockerWorkspace) init(ctx context.Context, client api.Client, repo *graphql.Repository) error { + // Create a Docker volume. + out, err := exec.CommandContext(ctx, "docker", "volume", "create").CombinedOutput() + if err != nil { + return errors.Wrap(err, "creating Docker volume") + } + w.volume = string(bytes.TrimSpace(out)) + + // Download the ZIP archive. + req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) + if err != nil { + return err + } + req.Header.Set("Accept", "application/zip") + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) + } + + // Write the ZIP somewhere we can mount into a container. + f, err := ioutil.TempFile(os.TempDir(), "src-archive-*.zip") + if err != nil { + return errors.Wrap(err, "creating temporary archive") + } + hostZip := f.Name() + defer os.Remove(hostZip) + + _, err = io.Copy(f, resp.Body) + f.Close() + if err != nil { + return errors.Wrap(err, "writing temporary archive") + } + + // Now actually unzip it into the volume. + common, err := w.DockerRunOpts(ctx, "/work") + if err != nil { + return errors.Wrap(err, "generating run options") + } + + opts := append([]string{ + "run", + "--rm", + "--init", + "--workdir", "/work", + "--mount", "type=bind,source=" + hostZip + ",target=/tmp/zip,ro", + }, common...) + opts = append(opts, baseImage, "unzip", "/tmp/zip") + + out, err = exec.CommandContext(ctx, "docker", opts...).CombinedOutput() + if err != nil { + return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) + } + + return nil +} + +func (w *dockerWorkspace) Close(ctx context.Context) error { + return exec.CommandContext(ctx, "docker", "volume", "rm", w.volume).Run() +} + +func (w *dockerWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { + return []string{ + "--mount", "type=volume,source=" + w.volume + ",target=" + target, + }, nil +} + +func (w *dockerWorkspace) Prepare(ctx context.Context) error { + script := `#!/bin/sh + +set -e +set -x + +git init +# --force because we want previously "gitignored" files in the repository +git add --force --all +git commit --quiet --all -m src-action-exec +` + + if _, err := w.runScript(ctx, "/work", script); err != nil { + return errors.Wrap(err, "preparing workspace") + } + return nil +} + +func (w *dockerWorkspace) Changes(ctx context.Context) (*StepChanges, error) { + script := `#!/bin/sh + +set -e +# No set -x here, since we're going to parse the git status output. + +git add --all > /dev/null +exec git status --porcelain +` + + out, err := w.runScript(ctx, "/work", script) + if err != nil { + return nil, errors.Wrap(err, "running git status") + } + + changes, err := parseGitStatus(out) + if err != nil { + return nil, errors.Wrapf(err, "parsing git status output:\n\n%s", string(out)) + } + + return &changes, nil +} + +func (w *dockerWorkspace) Diff(ctx context.Context) ([]byte, error) { + // As of Sourcegraph 3.14 we only support unified diff format. + // That means we need to strip away the `a/` and `/b` prefixes with `--no-prefix`. + // See: https://github.com/sourcegraph/sourcegraph/blob/82d5e7e1562fef6be5c0b17f18631040fd330835/enterprise/internal/campaigns/service.go#L324-L329 + // + // Also, we need to add --binary so binary file changes are inlined in the patch. + script := `#!/bin/sh + +exec git diff --cached --no-prefix --binary +` + + out, err := w.runScript(ctx, "/work", script) + if err != nil { + return nil, errors.Wrapf(err, "git diff:\n\n%s", string(out)) + } + + return out, nil +} + +func (w *dockerWorkspace) runScript(ctx context.Context, target, script string) ([]byte, error) { + f, err := ioutil.TempFile(os.TempDir(), "src-run-*") + if err != nil { + return nil, errors.Wrap(err, "creating run script") + } + name := f.Name() + defer os.Remove(name) + + if _, err := f.WriteString(script); err != nil { + return nil, errors.Wrap(err, "writing run script") + } + f.Close() + + common, err := w.DockerRunOpts(ctx, target) + if err != nil { + return nil, errors.Wrap(err, "generating run options") + } + + opts := append([]string{ + "run", + "--rm", + "--init", + "--workdir", target, + "--mount", "type=bind,source=" + name + ",target=/run.sh,ro", + }, common...) + opts = append(opts, baseImage, "sh", "/run.sh") + + out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput() + if err != nil { + return out, errors.Wrapf(err, "Docker output:\n\n%s\n\n", string(out)) + } + + return out, nil } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 72a433fa83..a64d1f9c17 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -22,11 +22,13 @@ type Service struct { allowUnsupported bool client api.Client features featureFlags + workspace string } type ServiceOpts struct { AllowUnsupported bool Client api.Client + Workspace string } var ( @@ -37,6 +39,7 @@ func NewService(opts *ServiceOpts) *Service { return &Service{ allowUnsupported: opts.AllowUnsupported, client: opts.Client, + workspace: opts.Workspace, } } @@ -200,6 +203,9 @@ func (svc *Service) NewExecutor(opts ExecutorOpts) Executor { } func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) WorkspaceCreator { + if svc.workspace == "volume" { + return &dockerWorkspaceCreator{client: svc.client} + } return &workspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} } @@ -213,6 +219,11 @@ func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, pro progress(i + 1) } + if _, err := getDockerImageContentDigest(ctx, baseImage); err != nil { + return errors.Wrap(err, "downloading base image") + } + progress(len(spec.Steps) + 1) + return nil } From c989ec047b2325d435453921ba7317c74bc433c7 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 22 Dec 2020 19:06:19 -0800 Subject: [PATCH 03/52] More WIP. The tests don't pass, but this is getting closer. --- .../{archive_fetcher.go => bind_workspace.go} | 73 ++++----------- ..._test.go => bind_workspace_nonwin_test.go} | 0 ...fetcher_test.go => bind_workspace_test.go} | 18 ++-- ...test.go => bind_workspace_windows_test.go} | 0 internal/campaigns/docker_workspace.go | 88 ++++++++----------- internal/campaigns/executor_test.go | 2 +- internal/campaigns/run_steps_test.go | 2 +- internal/campaigns/service.go | 6 +- internal/campaigns/workspace.go | 69 ++++++++++++++- 9 files changed, 142 insertions(+), 116 deletions(-) rename internal/campaigns/{archive_fetcher.go => bind_workspace.go} (81%) rename internal/campaigns/{archive_fetcher_nonwin_test.go => bind_workspace_nonwin_test.go} (100%) rename internal/campaigns/{archive_fetcher_test.go => bind_workspace_test.go} (93%) rename internal/campaigns/{archive_fetcher_windows_test.go => bind_workspace_windows_test.go} (100%) diff --git a/internal/campaigns/archive_fetcher.go b/internal/campaigns/bind_workspace.go similarity index 81% rename from internal/campaigns/archive_fetcher.go rename to internal/campaigns/bind_workspace.go index baca3e85f5..3d8376bf20 100644 --- a/internal/campaigns/archive_fetcher.go +++ b/internal/campaigns/bind_workspace.go @@ -6,9 +6,7 @@ import ( "fmt" "io" "io/ioutil" - "net/http" "os" - "path" "path/filepath" "strings" @@ -18,16 +16,16 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -type workspaceCreator struct { +type dockerBindWorkspaceCreator struct { dir string client api.Client deleteZips bool } -var _ WorkspaceCreator = &workspaceCreator{} +var _ WorkspaceCreator = &dockerBindWorkspaceCreator{} -func (wc *workspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { +func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { path := localRepositoryZipArchivePath(wc.dir, repo) exists, err := fileExists(path) @@ -35,6 +33,14 @@ func (wc *workspaceCreator) Create(ctx context.Context, repo *graphql.Repository return nil, err } + // Unlike the mkdirAll() calls elsewhere in this file, this is only giving + // us a temporary place on the filesystem to keep the archive. Since it's + // never mounted into the containers being run, we can keep these + // directories 0700 without issue. + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + return nil, err + } + if !exists { err = fetchRepositoryArchive(ctx, wc.client, repo, path) if err != nil { @@ -57,27 +63,27 @@ func (wc *workspaceCreator) Create(ctx context.Context, repo *graphql.Repository return nil, errors.Wrap(err, "unzipping the ZIP archive") } - return &bindWorkspace{dir: workspace}, nil + return &dockerBindWorkspace{dir: workspace}, nil } -type bindWorkspace struct { +type dockerBindWorkspace struct { dir string } -var _ Workspace = &bindWorkspace{} +var _ Workspace = &dockerBindWorkspace{} -func (w *bindWorkspace) Close(ctx context.Context) error { +func (w *dockerBindWorkspace) Close(ctx context.Context) error { return os.RemoveAll(w.dir) } -func (w *bindWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { +func (w *dockerBindWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { return []string{ "--mount", fmt.Sprintf("type=bind,source=%s,target=%s", w.dir, target), }, nil } -func (w *bindWorkspace) Prepare(ctx context.Context) error { +func (w *dockerBindWorkspace) Prepare(ctx context.Context) error { if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { return errors.Wrap(err, "git init failed") } @@ -93,7 +99,7 @@ func (w *bindWorkspace) Prepare(ctx context.Context) error { return nil } -func (w *bindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { +func (w *dockerBindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { if _, err := runGitCmd(ctx, w.dir, "add", "--all"); err != nil { return nil, errors.Wrap(err, "git add failed") } @@ -111,7 +117,7 @@ func (w *bindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { return &changes, nil } -func (w *bindWorkspace) Diff(ctx context.Context) ([]byte, error) { +func (w *dockerBindWorkspace) Diff(ctx context.Context) ([]byte, error) { // As of Sourcegraph 3.14 we only support unified diff format. // That means we need to strip away the `a/` and `/b` prefixes with `--no-prefix`. // See: https://github.com/sourcegraph/sourcegraph/blob/82d5e7e1562fef6be5c0b17f18631040fd330835/enterprise/internal/campaigns/service.go#L324-L329 @@ -145,47 +151,6 @@ func unzipToTempDir(ctx context.Context, zipFile, tempDir, tempFilePrefix string return volumeDir, unzip(zipFile, volumeDir) } -func fetchRepositoryArchive(ctx context.Context, client api.Client, repo *graphql.Repository, dest string) error { - req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) - if err != nil { - return err - } - req.Header.Set("Accept", "application/zip") - resp, err := http.DefaultClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) - } - - // Unlike the mkdirAll() calls elsewhere in this file, this is only giving - // us a temporary place on the filesystem to keep the archive. Since it's - // never mounted into the containers being run, we can keep these - // directories 0700 without issue. - if err := os.MkdirAll(filepath.Dir(dest), 0700); err != nil { - return err - } - - f, err := os.Create(dest) - if err != nil { - return err - } - defer f.Close() - - if _, err := io.Copy(f, resp.Body); err != nil { - return err - } - - return nil -} - -func repositoryZipArchivePath(repo *graphql.Repository) string { - return path.Join("", repo.Name+"@"+repo.BaseRef(), "-", "raw") -} - func localRepositoryZipArchivePath(dir string, repo *graphql.Repository) string { return filepath.Join(dir, fmt.Sprintf("%s-%s.zip", repo.Slug(), repo.Rev())) } diff --git a/internal/campaigns/archive_fetcher_nonwin_test.go b/internal/campaigns/bind_workspace_nonwin_test.go similarity index 100% rename from internal/campaigns/archive_fetcher_nonwin_test.go rename to internal/campaigns/bind_workspace_nonwin_test.go diff --git a/internal/campaigns/archive_fetcher_test.go b/internal/campaigns/bind_workspace_test.go similarity index 93% rename from internal/campaigns/archive_fetcher_test.go rename to internal/campaigns/bind_workspace_test.go index 9e536173b7..ffeb0f55e7 100644 --- a/internal/campaigns/archive_fetcher_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) @@ -54,7 +55,7 @@ func TestWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) - creator := &WorkspaceCreator{dir: testTempDir, client: client} + creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} workspace, err := creator.Create(context.Background(), repo) if err != nil { t.Fatalf("unexpected error: %s", err) @@ -140,7 +141,7 @@ func TestWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) - creator := &WorkspaceCreator{dir: testTempDir, client: client} + creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} _, err := creator.Create(ctx, repo) if err == nil { @@ -178,7 +179,7 @@ func TestWorkspaceCreator_Create(t *testing.T) { testTempDir := workspaceTmpDir(t) - creator := &WorkspaceCreator{dir: testTempDir, client: client} + creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} _, err := creator.Create(context.Background(), repo) if err != nil { @@ -329,10 +330,15 @@ func isDir(t *testing.T, path string) bool { return st.IsDir() } -func readWorkspaceFiles(workspace string) (map[string]string, error) { +func readWorkspaceFiles(workspace Workspace) (map[string]string, error) { + w, ok := workspace.(*dockerBindWorkspace) + if !ok { + return nil, errors.Errorf("unknown workspace of type %T: %+v", workspace, workspace) + } + files := map[string]string{} - err := filepath.Walk(workspace, func(path string, info os.FileInfo, err error) error { + err := filepath.Walk(w.dir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -345,7 +351,7 @@ func readWorkspaceFiles(workspace string) (map[string]string, error) { return err } - rel, err := filepath.Rel(workspace, path) + rel, err := filepath.Rel(w.dir, path) if err != nil { return err } diff --git a/internal/campaigns/archive_fetcher_windows_test.go b/internal/campaigns/bind_workspace_windows_test.go similarity index 100% rename from internal/campaigns/archive_fetcher_windows_test.go rename to internal/campaigns/bind_workspace_windows_test.go diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/docker_workspace.go index b0f986186d..d5af3d6db2 100644 --- a/internal/campaigns/docker_workspace.go +++ b/internal/campaigns/docker_workspace.go @@ -3,10 +3,7 @@ package campaigns import ( "bytes" "context" - "fmt" - "io" "io/ioutil" - "net/http" "os" "os/exec" @@ -15,51 +12,30 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -type dockerWorkspaceCreator struct { +// dockerVolumeWorkspaceCreator creates dockerWorkspace instances. +type dockerVolumeWorkspaceCreator struct { client api.Client } -var _ WorkspaceCreator = &dockerWorkspaceCreator{} +var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} -func (wc *dockerWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { - w := &dockerWorkspace{} +func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { + w := &dockerVolumeWorkspace{} return w, w.init(ctx, wc.client, repo) } -// TODO: migrate to a real image on Docker Hub. -const baseImage = "sourcegraph/src-campaign-workspace" - -type dockerWorkspace struct { +// dockerVolumeWorkspace workspaces are placed on Docker volumes (surprise!), +// and are therefore transparent to the host filesystem. This has performance +// advantages if bind mounts are slow, such as on Docker for Mac, but could make +// debugging harder and is slower when it's time to actually retrieve the diff. +type dockerVolumeWorkspace struct { volume string } -var _ Workspace = &dockerWorkspace{} - -func (w *dockerWorkspace) init(ctx context.Context, client api.Client, repo *graphql.Repository) error { - // Create a Docker volume. - out, err := exec.CommandContext(ctx, "docker", "volume", "create").CombinedOutput() - if err != nil { - return errors.Wrap(err, "creating Docker volume") - } - w.volume = string(bytes.TrimSpace(out)) +var _ Workspace = &dockerVolumeWorkspace{} - // Download the ZIP archive. - req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) - if err != nil { - return err - } - req.Header.Set("Accept", "application/zip") - resp, err := http.DefaultClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) - } - - // Write the ZIP somewhere we can mount into a container. +func (w *dockerVolumeWorkspace) init(ctx context.Context, client api.Client, repo *graphql.Repository) error { + // Download the ZIP archive to a temporary file. f, err := ioutil.TempFile(os.TempDir(), "src-archive-*.zip") if err != nil { return errors.Wrap(err, "creating temporary archive") @@ -67,13 +43,19 @@ func (w *dockerWorkspace) init(ctx context.Context, client api.Client, repo *gra hostZip := f.Name() defer os.Remove(hostZip) - _, err = io.Copy(f, resp.Body) - f.Close() + if err := fetchRepositoryArchive(ctx, client, repo, hostZip); err != nil { + return errors.Wrap(err, "fetching repository archive") + } + + // Create a Docker volume. + out, err := exec.CommandContext(ctx, "docker", "volume", "create").CombinedOutput() if err != nil { - return errors.Wrap(err, "writing temporary archive") + return errors.Wrap(err, "creating Docker volume") } + w.volume = string(bytes.TrimSpace(out)) - // Now actually unzip it into the volume. + // Now mount that temporary file into a Docker container, and unzip it into + // the volume. common, err := w.DockerRunOpts(ctx, "/work") if err != nil { return errors.Wrap(err, "generating run options") @@ -86,7 +68,7 @@ func (w *dockerWorkspace) init(ctx context.Context, client api.Client, repo *gra "--workdir", "/work", "--mount", "type=bind,source=" + hostZip + ",target=/tmp/zip,ro", }, common...) - opts = append(opts, baseImage, "unzip", "/tmp/zip") + opts = append(opts, dockerWorkspaceImage, "unzip", "/tmp/zip") out, err = exec.CommandContext(ctx, "docker", opts...).CombinedOutput() if err != nil { @@ -96,17 +78,18 @@ func (w *dockerWorkspace) init(ctx context.Context, client api.Client, repo *gra return nil } -func (w *dockerWorkspace) Close(ctx context.Context) error { +func (w *dockerVolumeWorkspace) Close(ctx context.Context) error { + // Cleanup here is easy: we just get rid of the Docker volume. return exec.CommandContext(ctx, "docker", "volume", "rm", w.volume).Run() } -func (w *dockerWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { +func (w *dockerVolumeWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) { return []string{ "--mount", "type=volume,source=" + w.volume + ",target=" + target, }, nil } -func (w *dockerWorkspace) Prepare(ctx context.Context) error { +func (w *dockerVolumeWorkspace) Prepare(ctx context.Context) error { script := `#!/bin/sh set -e @@ -124,7 +107,7 @@ git commit --quiet --all -m src-action-exec return nil } -func (w *dockerWorkspace) Changes(ctx context.Context) (*StepChanges, error) { +func (w *dockerVolumeWorkspace) Changes(ctx context.Context) (*StepChanges, error) { script := `#!/bin/sh set -e @@ -147,7 +130,7 @@ exec git status --porcelain return &changes, nil } -func (w *dockerWorkspace) Diff(ctx context.Context) ([]byte, error) { +func (w *dockerVolumeWorkspace) Diff(ctx context.Context) ([]byte, error) { // As of Sourcegraph 3.14 we only support unified diff format. // That means we need to strip away the `a/` and `/b` prefixes with `--no-prefix`. // See: https://github.com/sourcegraph/sourcegraph/blob/82d5e7e1562fef6be5c0b17f18631040fd330835/enterprise/internal/campaigns/service.go#L324-L329 @@ -166,7 +149,14 @@ exec git diff --cached --no-prefix --binary return out, nil } -func (w *dockerWorkspace) runScript(ctx context.Context, target, script string) ([]byte, error) { +// dockerWorkspaceImage is the Docker image we'll run our unzip and git commands +// in. +const dockerWorkspaceImage = "sourcegraph/src-campaign-workspace" + +// runScript is a utility function to mount the given shell script into a Docker +// container started from the dockerWorkspaceImage, then run it and return the +// output. +func (w *dockerVolumeWorkspace) runScript(ctx context.Context, target, script string) ([]byte, error) { f, err := ioutil.TempFile(os.TempDir(), "src-run-*") if err != nil { return nil, errors.Wrap(err, "creating run script") @@ -191,7 +181,7 @@ func (w *dockerWorkspace) runScript(ctx context.Context, target, script string) "--workdir", target, "--mount", "type=bind,source=" + name + ",target=/run.sh,ro", }, common...) - opts = append(opts, baseImage, "sh", "/run.sh") + opts = append(opts, dockerWorkspaceImage, "sh", "/run.sh") out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput() if err != nil { diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index afe2617eea..25cc385df3 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -213,7 +213,7 @@ func TestExecutor_Integration(t *testing.T) { defer os.Remove(testTempDir) cache := newInMemoryExecutionCache() - creator := &WorkspaceCreator{dir: testTempDir, client: client} + creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} opts := ExecutorOpts{ Cache: cache, Creator: creator, diff --git a/internal/campaigns/run_steps_test.go b/internal/campaigns/run_steps_test.go index 8d4db8415a..52fc51db99 100644 --- a/internal/campaigns/run_steps_test.go +++ b/internal/campaigns/run_steps_test.go @@ -36,7 +36,7 @@ R README.md -> README.markdown func TestParsingAndRenderingTemplates(t *testing.T) { stepCtx := &StepContext{ PreviousStep: StepResult{ - Files: StepChanges{ + Files: &StepChanges{ Modified: []string{"go.mod"}, Added: []string{"main.go.swp"}, Deleted: []string{".DS_Store"}, diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index a64d1f9c17..8ee634eeb7 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -204,9 +204,9 @@ func (svc *Service) NewExecutor(opts ExecutorOpts) Executor { func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) WorkspaceCreator { if svc.workspace == "volume" { - return &dockerWorkspaceCreator{client: svc.client} + return &dockerVolumeWorkspaceCreator{client: svc.client} } - return &workspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} + return &dockerBindWorkspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} } func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, progress func(i int)) error { @@ -219,7 +219,7 @@ func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, pro progress(i + 1) } - if _, err := getDockerImageContentDigest(ctx, baseImage); err != nil { + if _, err := getDockerImageContentDigest(ctx, dockerWorkspaceImage); err != nil { return errors.Wrap(err, "downloading base image") } progress(len(spec.Steps) + 1) diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index b3216d4a1f..5e64fd44e2 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -2,19 +2,84 @@ package campaigns import ( "context" + "fmt" + "io" + "net/http" + "os" + "path" + "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) +// WorkspaceCreator implementations are used to create workspaces, which manage +// per-changeset persistent storage when executing campaign steps and are +// responsible for ultimately generating a diff. type WorkspaceCreator interface { + // Create should clone the given repository, and perform whatever other + // initial setup is required. Create(context.Context, *graphql.Repository) (Workspace, error) } +// Workspace implementations manage per-changeset storage when executing +// campaign step. type Workspace interface { - Close(ctx context.Context) error - DockerRunOpts(ctx context.Context, target string) ([]string, error) + // Prepare is called once, immediately before the first step is executed. + // Generally, this would perform whatever Git or other VCS setup is required + // to establish a base upon which to calculate changes later. Prepare(ctx context.Context) error + // DockerRunOpts provides the options that should be given to `docker run` + // in order to use this workspace. Generally, this will be a set of mount + // options. + DockerRunOpts(ctx context.Context, target string) ([]string, error) + + // Close is called once, after all steps have been executed and the diff has + // been calculated and stored outside the workspace. Implementations should + // delete the workspace when Close is called. + Close(ctx context.Context) error + + // 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) + + // 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) } + +// We'll put some useful utility functions below here that tend to be reused +// across workspace implementations. + +func fetchRepositoryArchive(ctx context.Context, client api.Client, repo *graphql.Repository, dest string) error { + req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) + if err != nil { + return err + } + req.Header.Set("Accept", "application/zip") + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) + } + + f, err := os.Create(dest) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, resp.Body); err != nil { + return err + } + + return nil +} + +func repositoryZipArchivePath(repo *graphql.Repository) string { + return path.Join("", repo.Name+"@"+repo.BaseRef(), "-", "raw") +} From fadf6f3ec6c92a90807fb3900e999146251cb788 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 13:12:55 -0800 Subject: [PATCH 04/52] Add workflow for building the Docker image. --- .github/workflows/docker.yml | 51 +++++++++++++++++++++ DEVELOPMENT.md | 16 +++++++ campaign-container/Dockerfile | 5 -- campaign-container/build.sh | 6 --- docker/campaign-volume-workspace/Dockerfile | 13 ++++++ internal/campaigns/docker_workspace.go | 4 +- 6 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 .github/workflows/docker.yml delete mode 100644 campaign-container/Dockerfile delete mode 100755 campaign-container/build.sh create mode 100644 docker/campaign-volume-workspace/Dockerfile diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml new file mode 100644 index 0000000000..553ef1b83b --- /dev/null +++ b/.github/workflows/docker.yml @@ -0,0 +1,51 @@ +# For more information, refer to the "Dependent Docker images" section of +# DEVELOPMENT.md. +name: Publish Docker image dependencies + +# We only want to build on releases; this condition is 100% stolen from the +# goreleaser action. +on: + push: + tags: + - "*" + - "!latest" + +jobs: + publish: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v2 + + - name: Publish to Docker Hub + uses: elgohr/Publish-Docker-Github-Action@master + with: + # This needs to match the dockerWorkspaceImage constant in + # docker_workspace.go. + name: sourcegraph/src-campaign-volume-workspace + username: ${{ secrets.DOCKER_USERNAME }} + password: ${{ secrets.DOCKER_PASSWORD }} + default_branch: main + dockerfile: ./docker/campaign-volume-workspace/Dockerfile + # This gets us idiomatic Docker versioning (ie tags 3, 3.23, and + # 3.23.2 for a 3.23.2 release), but at the cost of losing the latest + # tag. We'll deal with that momentarily. + tag_semver: true + + # We now have to run the publish action again to publish the latest tag, + # due to + # https://github.com/elgohr/Publish-Docker-Github-Action/issues/70: in + # short, tag_semver and tags are mutually exclusive. In practice, this + # image is small enough and quick enough to build that I'm not terribly + # concerned, and none of the more complicated Docker actions feel worth + # pulling in here. + - name: Publish to Docker Hub + uses: elgohr/Publish-Docker-Github-Action@master + with: + # This needs to match the dockerWorkspaceImage constant in + # docker_workspace.go. + name: sourcegraph/src-campaign-volume-workspace + username: ${{ secrets.DOCKER_USERNAME }} + password: ${{ secrets.DOCKER_PASSWORD }} + default_branch: main + dockerfile: ./docker/campaign-volume-workspace/Dockerfile diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 1d3f6a0178..5171aebd49 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -38,3 +38,19 @@ For example, suppose we have the the recommended versions. If a new feature is added to a new `3.91.6` release of src-cli and this change requires only features available in Sourcegraph `3.99`, then this feature should also be present in a new `3.85.8` release of src-cli. Because a Sourcegraph instance will automatically select the highest patch version, all non-breaking changes should increment only the patch version. Note that if instead the recommended src-cli version for Sourcegraph `3.99` was `3.90.4` in the example above, there is no additional step required, and the new patch version of src-cli will be available to both Sourcegraph versions. + +## Dependent Docker images + +`src campaign apply` and `src campaign preview` use a Docker image published as `sourcegraph/src-campaign-volume-workspace` for utility purposes when the volume workspace is selected, which is the default on macOS. This Docker image is updated by the GitHub Action workflow described in [`docker.yml`](.github/workflows/docker.yml). + +To build and develop this locally, you can build and tag the image with: + +```sh +docker build -t sourcegraph/src-campaign-volume-workspace - < docker/campaign-volume-workspace/Dockerfile +``` + +To remove it and then force the upstream image on Docker Hub to be used again: + +```sh +docker rmi sourcegraph/src-campaign-volume-workspace +``` diff --git a/campaign-container/Dockerfile b/campaign-container/Dockerfile deleted file mode 100644 index b740969ec4..0000000000 --- a/campaign-container/Dockerfile +++ /dev/null @@ -1,5 +0,0 @@ -FROM alpine - -RUN apk add --update curl git unzip && \ - git config --global user.email campaigns@sourcegraph.com && \ - git config --global user.name 'Sourcegraph Campaigns' \ No newline at end of file diff --git a/campaign-container/build.sh b/campaign-container/build.sh deleted file mode 100755 index 1b713642b5..0000000000 --- a/campaign-container/build.sh +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/sh - -set -e -set -x - -exec docker build -t sourcegraph/src-campaign-workspace - <"$(dirname "$0")/Dockerfile" diff --git a/docker/campaign-volume-workspace/Dockerfile b/docker/campaign-volume-workspace/Dockerfile new file mode 100644 index 0000000000..ce4cc19961 --- /dev/null +++ b/docker/campaign-volume-workspace/Dockerfile @@ -0,0 +1,13 @@ +# This Dockerfile builds the sourcegraph/src-campaign-volume-workspace image +# that we use to run curl, git, and unzip against a Docker volume when using +# the volume workspace. + +FROM alpine:3.12.3 + +# Note that we have to configure git's user.email and user.name settings to +# avoid issues when committing changes. These values are not used when creating +# changesets, since we only extract the diff from the container and not a full +# patch, but need to be set to avoid git erroring out. +RUN apk add --update curl git unzip && \ + git config --global user.email campaigns@sourcegraph.com && \ + git config --global user.name 'Sourcegraph Campaigns' diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/docker_workspace.go index d5af3d6db2..4c2dd425d0 100644 --- a/internal/campaigns/docker_workspace.go +++ b/internal/campaigns/docker_workspace.go @@ -150,8 +150,8 @@ exec git diff --cached --no-prefix --binary } // dockerWorkspaceImage is the Docker image we'll run our unzip and git commands -// in. -const dockerWorkspaceImage = "sourcegraph/src-campaign-workspace" +// in. This needs to match the name defined in .github/workflows/docker.yml. +const dockerWorkspaceImage = "sourcegraph/src-campaign-volume-workspace" // runScript is a utility function to mount the given shell script into a Docker // container started from the dockerWorkspaceImage, then run it and return the From dcde796354b7fe476928df9b77c424c0bad09760 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 13:15:23 -0800 Subject: [PATCH 05/52] First crack at a CHANGELOG. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 853023b86d..e61ac408a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to `src-cli` are documented in this file. ### Added +- Campaigns can now be executed using workspaces contained entirely on Docker volumes, rather than bind mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. `src campaign apply` and `src campaign preview` accept optional `-workspace` flags to override the default. []() + ### Changed - `src login` now defaults to validating against `SRC_ENDPOINT` if configured. From 29bf3f6ca7728c2ffc6475652de5683b61899f42 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 14:35:07 -0800 Subject: [PATCH 06/52] Fix test issue. --- internal/campaigns/bind_workspace.go | 2 ++ internal/campaigns/docker_workspace.go | 2 ++ internal/campaigns/run_steps.go | 5 +++-- internal/campaigns/workspace.go | 5 +++++ 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index 3d8376bf20..e46abace30 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -83,6 +83,8 @@ func (w *dockerBindWorkspace) DockerRunOpts(ctx context.Context, target string) }, nil } +func (w *dockerBindWorkspace) WorkDir() *string { return &w.dir } + func (w *dockerBindWorkspace) Prepare(ctx context.Context) error { if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { return errors.Wrap(err, "git init failed") diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/docker_workspace.go index 4c2dd425d0..9a7ef66b94 100644 --- a/internal/campaigns/docker_workspace.go +++ b/internal/campaigns/docker_workspace.go @@ -89,6 +89,8 @@ func (w *dockerVolumeWorkspace) DockerRunOpts(ctx context.Context, target string }, nil } +func (w *dockerVolumeWorkspace) WorkDir() *string { return nil } + func (w *dockerVolumeWorkspace) Prepare(ctx context.Context) error { script := `#!/bin/sh diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 68c9bac4e7..5670ccd0ab 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -176,8 +176,9 @@ func runSteps(ctx context.Context, wc WorkspaceCreator, repo *graphql.Repository cmd := exec.CommandContext(ctx, "docker", args...) cmd.Args = append(cmd.Args, "--", step.image, containerTemp) - // XXX: should be moot now? - //cmd.Dir = volumeDir + if dir := workspace.WorkDir(); dir != nil { + cmd.Dir = *dir + } var stdoutBuffer, stderrBuffer bytes.Buffer cmd.Stdout = io.MultiWriter(&stdoutBuffer, logger.PrefixWriter("stdout")) diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index 5e64fd44e2..c98c2b9ffc 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -34,6 +34,11 @@ type Workspace interface { // options. DockerRunOpts(ctx context.Context, target string) ([]string, error) + // WorkDir allows workspaces to specify the working directory that should be + // used when running Docker. If no specific working directory is needed, + // then the function should return nil. + WorkDir() *string + // Close is called once, after all steps have been executed and the diff has // been calculated and stored outside the workspace. Implementations should // delete the workspace when Close is called. From 6bad0e47e293b2b396b1444e685c712c8e745c75 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 14:41:19 -0800 Subject: [PATCH 07/52] Handle potential nil pointer in Files. --- internal/campaigns/run_steps.go | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 5670ccd0ab..26f905ccd8 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -453,16 +453,36 @@ type StepChanges struct { } // ModifiedFiles returns the files modified by a step. -func (r StepResult) ModifiedFiles() []string { return r.Files.Modified } +func (r StepResult) ModifiedFiles() []string { + if r.Files != nil { + return r.Files.Modified + } + return []string{} +} // AddedFiles returns the files added by a step. -func (r StepResult) AddedFiles() []string { return r.Files.Added } +func (r StepResult) AddedFiles() []string { + if r.Files != nil { + return r.Files.Added + } + return []string{} +} // DeletedFiles returns the files deleted by a step. -func (r StepResult) DeletedFiles() []string { return r.Files.Deleted } +func (r StepResult) DeletedFiles() []string { + if r.Files != nil { + return r.Files.Deleted + } + return []string{} +} // RenamedFiles returns the new name of files that have been renamed by a step. -func (r StepResult) RenamedFiles() []string { return r.Files.Renamed } +func (r StepResult) RenamedFiles() []string { + if r.Files != nil { + return r.Files.Renamed + } + return []string{} +} func parseGitStatus(out []byte) (StepChanges, error) { result := StepChanges{} From e8e127e0d2ed79e335ea2bd4c26289a0299fc144 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 17:52:09 -0800 Subject: [PATCH 08/52] Switch default on macOS. --- cmd/src/campaigns_common.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 6d3c1d42af..f4c6b02011 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -100,7 +100,17 @@ func newCampaignsApplyFlags(flagSet *flag.FlagSet, cacheDir, tempDir string) *ca &caf.skipErrors, "skip-errors", false, "If true, errors encountered while executing steps in a repository won't stop the execution of the campaign spec but only cause that repository to be skipped.", ) - flagSet.StringVar(&caf.workspace, "workspace", "bind", "Workspace mode to use (bind or volume)") + + var defaultWorkspace string + if runtime.GOOS == "darwin" { + defaultWorkspace = "volume" + } else { + defaultWorkspace = "bind" + } + flagSet.StringVar( + &caf.workspace, "workspace", defaultWorkspace, + `Workspace mode to use ("bind" or "volume")`, + ) flagSet.BoolVar(verbose, "v", false, "print verbose output") From 946d9bb54fba48ab59bfd5dcaecf053c03b80853 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 24 Dec 2020 17:52:16 -0800 Subject: [PATCH 09/52] Add initial workspace tests. --- go.mod | 1 + go.sum | 2 + ...ocker_workspace.go => volume_workspace.go} | 0 internal/campaigns/volume_workspace_test.go | 152 ++++++++++++++++++ 4 files changed, 155 insertions(+) rename internal/campaigns/{docker_workspace.go => volume_workspace.go} (100%) create mode 100644 internal/campaigns/volume_workspace_test.go diff --git a/go.mod b/go.mod index ad8ec67038..06471f4f71 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.13 require ( github.com/Masterminds/semver v1.5.0 + github.com/alessio/shellescape v1.4.1 github.com/dustin/go-humanize v1.0.0 github.com/efritz/pentimento v0.0.0-20190429011147-ade47d831101 github.com/google/go-cmp v0.5.2 diff --git a/go.sum b/go.sum index d569c1db34..2a96b140d6 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww= github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= +github.com/alessio/shellescape v1.4.1 h1:V7yhSDDn8LP4lc4jS8pFkt0zCnzVJlG5JXy9BVKJUX0= +github.com/alessio/shellescape v1.4.1/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPpyFjDCtHLS30= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/internal/campaigns/docker_workspace.go b/internal/campaigns/volume_workspace.go similarity index 100% rename from internal/campaigns/docker_workspace.go rename to internal/campaigns/volume_workspace.go diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go new file mode 100644 index 0000000000..6b1816365d --- /dev/null +++ b/internal/campaigns/volume_workspace_test.go @@ -0,0 +1,152 @@ +//+build !windows + +package campaigns + +import ( + "context" + "io/ioutil" + "os" + "path" + "strconv" + "testing" + + "github.com/alessio/shellescape" + "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" +) + +func TestVolumeWorkspace_Close(t *testing.T) { + ctx := context.Background() + + for name, tc := range map[string]struct { + w *dockerVolumeWorkspace + wantErr bool + }{ + "success": { + w: &dockerVolumeWorkspace{volume: "VOLUME"}, + wantErr: false, + }, + "failure": { + w: &dockerVolumeWorkspace{volume: "FOO"}, + wantErr: true, + }, + } { + t.Run(name, func(t *testing.T) { + var ( + success = 0 + failure = 1 + ) + if tc.wantErr { + success = 1 + failure = 0 + } + + ec, err := expectCommand(success, failure, "docker", "volume", "rm", tc.w.volume) + if err != nil { + t.Fatal(err) + } + defer ec.finalise(t) + + if err = tc.w.Close(ctx); tc.wantErr && err == nil { + t.Error("unexpected nil error") + } else if !tc.wantErr && err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + } +} + +func TestVolumeWorkspace_DockerRunOpts(t *testing.T) { + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: "VOLUME"} + + want := []string{ + "--mount", "type=volume,source=VOLUME,target=TARGET", + } + have, err := w.DockerRunOpts(ctx, "TARGET") + if err != nil { + t.Errorf("unexpected error: %+v", err) + } + if diff := cmp.Diff(have, want); diff != "" { + t.Errorf("unexpected options (-have +want):\n%s", diff) + } +} + +func TestVolumeWorkspace_WorkDir(t *testing.T) { + if have := (&dockerVolumeWorkspace{}).WorkDir(); have != nil { + t.Errorf("unexpected work dir: %q", *have) + } +} + +func expectCommand(success, failure int, command string, args ...string) (*expectedCommand, error) { + dir, err := ioutil.TempDir(os.TempDir(), "expect-*") + if err != nil { + return nil, errors.Wrap(err, "creating temporary directory to contain the expected command") + } + + called := path.Join(dir, command+".called") + script := `#!/bin/sh + +set -e +set -x + +retval=` + strconv.Itoa(success) + for i, arg := range args { + script += ` +if [ "$1" != ` + shellescape.Quote(arg) + ` ]; then + echo "Argument ` + strconv.Itoa(i+1) + ` is invalid: $1" + retval=` + strconv.Itoa(failure) + ` +fi +shift >/dev/null 2>&1 +` + } + script += ` +if [ "$retval" -eq ` + strconv.Itoa(success) + ` ]; then + touch ` + shellescape.Quote(called) + ` +fi + +exit "$retval"` + dest := path.Join(dir, command) + if err := ioutil.WriteFile(dest, []byte(script), 0755); err != nil { + return nil, errors.Wrapf(err, "writing script to %q", dest) + } + + oldPath := os.Getenv("PATH") + if oldPath != "" { + os.Setenv("PATH", dir+string(os.PathListSeparator)+oldPath) + } else { + os.Setenv("PATH", dir) + } + + return &expectedCommand{ + called: called, + dir: dir, + oldPath: oldPath, + }, nil +} + +type expectedCommand struct { + called string + dir string + oldPath string +} + +func (ec *expectedCommand) finalise(t *testing.T) { + t.Helper() + + called := true + if _, err := os.Stat(path.Join(ec.called)); os.IsNotExist(err) { + called = false + } else if err != nil { + t.Fatalf("error stating called file %q: %v", ec.called, err) + } + + os.Setenv("PATH", ec.oldPath) + if err := os.RemoveAll(ec.dir); err != nil { + t.Fatalf("error removing expected command directory %q: %v", ec.dir, err) + } + + if !called { + t.Error("expected command not called") + } +} From d937be1d3b353a855a0ac46b40b47ba3e13ea54a Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 29 Dec 2020 21:20:57 -0800 Subject: [PATCH 10/52] Flesh out volume workspace tests. --- go.mod | 1 + internal/api/mock/mock.go | 61 +++ internal/campaigns/main_test.go | 13 + internal/campaigns/volume_workspace.go | 3 +- internal/campaigns/volume_workspace_test.go | 444 +++++++++++++++----- internal/exec/exec.go | 31 ++ internal/exec/expect/expect.go | 197 +++++++++ internal/exec/middleware.go | 24 ++ 8 files changed, 671 insertions(+), 103 deletions(-) create mode 100644 internal/api/mock/mock.go create mode 100644 internal/campaigns/main_test.go create mode 100644 internal/exec/exec.go create mode 100644 internal/exec/expect/expect.go create mode 100644 internal/exec/middleware.go diff --git a/go.mod b/go.mod index 06471f4f71..5f9d3fa639 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/alessio/shellescape v1.4.1 github.com/dustin/go-humanize v1.0.0 github.com/efritz/pentimento v0.0.0-20190429011147-ade47d831101 + github.com/gobwas/glob v0.2.3 github.com/google/go-cmp v0.5.2 github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-multierror v1.1.0 diff --git a/internal/api/mock/mock.go b/internal/api/mock/mock.go new file mode 100644 index 0000000000..2047878770 --- /dev/null +++ b/internal/api/mock/mock.go @@ -0,0 +1,61 @@ +// Package mock provides mocking capabilities for api.Client instances. +package mock + +import ( + "bytes" + "net" + "net/http" + "testing" + + "github.com/pkg/errors" + + "github.com/sourcegraph/src-cli/internal/api" +) + +// ParrotClient creates a new API client that always receives the given response. +func ParrotClient(t *testing.T, resp []byte) (api.Client, error) { + // We're going to implement this by standing up a HTTP server on a random + // port that always returns the given response. + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return nil, errors.Wrap(err, "listening on random port") + } + + srv := &http.Server{ + Handler: &mockHandler{ + data: resp, + t: t, + }, + } + go srv.Serve(l) + + var buf bytes.Buffer + t.Cleanup(func() { + srv.Close() + t.Logf("output from API client: %s", buf.String()) + }) + + return api.NewClient(api.ClientOpts{ + Endpoint: "http://" + l.Addr().String(), + Out: &buf, + }), nil +} + +// mockHandler implements a HTTP handler that always returns the given data as its response, and always succeeds. +type mockHandler struct { + data []byte + t interface { + Logf(format string, args ...interface{}) + } +} + +var _ http.Handler = &mockHandler{} + +func (ms *mockHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if ms.t != nil { + ms.t.Logf("handling request: %+v", r) + } + + w.WriteHeader(200) + w.Write(ms.data) +} diff --git a/internal/campaigns/main_test.go b/internal/campaigns/main_test.go new file mode 100644 index 0000000000..103b728a97 --- /dev/null +++ b/internal/campaigns/main_test.go @@ -0,0 +1,13 @@ +package campaigns + +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/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 9a7ef66b94..584252fbd4 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -5,11 +5,12 @@ import ( "context" "io/ioutil" "os" - "os/exec" "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" + "github.com/sourcegraph/src-cli/internal/exec" ) // dockerVolumeWorkspaceCreator creates dockerWorkspace instances. diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 6b1816365d..29b72f6d6b 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -1,59 +1,132 @@ -//+build !windows - package campaigns import ( + "bytes" "context" "io/ioutil" - "os" - "path" - "strconv" + "strings" "testing" - "github.com/alessio/shellescape" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" + + "github.com/sourcegraph/src-cli/internal/api/mock" + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" + "github.com/sourcegraph/src-cli/internal/exec/expect" ) -func TestVolumeWorkspace_Close(t *testing.T) { +// We may as well use the same volume ID in all the subtests. +const volumeID = "VOLUME-ID" + +func TestVolumeWorkspaceCreator(t *testing.T) { ctx := context.Background() - for name, tc := range map[string]struct { - w *dockerVolumeWorkspace - wantErr bool - }{ - "success": { - w: &dockerVolumeWorkspace{volume: "VOLUME"}, - wantErr: false, - }, - "failure": { - w: &dockerVolumeWorkspace{volume: "FOO"}, - wantErr: true, - }, - } { - t.Run(name, func(t *testing.T) { - var ( - success = 0 - failure = 1 - ) - if tc.wantErr { - success = 1 - failure = 0 - } - - ec, err := expectCommand(success, failure, "docker", "volume", "rm", tc.w.volume) - if err != nil { - t.Fatal(err) - } - defer ec.finalise(t) - - if err = tc.w.Close(ctx); tc.wantErr && err == nil { - t.Error("unexpected nil error") - } else if !tc.wantErr && err != nil { - t.Errorf("unexpected error: %v", err) - } - }) + // It doesn't matter what content we return here, since the actual unzip + // command will never be executed anyway. + client, err := mock.ParrotClient(t, []byte("MOO")) + if err != nil { + t.Fatal(err) } + wc := &dockerVolumeWorkspaceCreator{client: client} + + // We'll set up a fake repository with just enough fields defined for init() + // and friends. + repo := &graphql.Repository{ + DefaultBranch: &graphql.Branch{Name: "main"}, + } + + t.Run("success", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "unzip", "/tmp/zip", + ), + ) + + if w, err := wc.Create(ctx, repo); err != nil { + t.Errorf("unexpected error: %v", err) + } else if have := w.(*dockerVolumeWorkspace).volume; have != volumeID { + t.Errorf("unexpected volume: have=%q want=%q", have, volumeID) + } + }) + + t.Run("docker volume create failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "volume", "create", + ), + ) + + if _, err := wc.Create(ctx, repo); err == nil { + t.Error("unexpected nil error") + } + }) + + t.Run("unzip failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "unzip", "/tmp/zip", + ), + ) + + if _, err := wc.Create(ctx, repo); err == nil { + t.Error("unexpected nil error") + } + }) +} + +func TestVolumeWorkspace_Close(t *testing.T) { + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: volumeID} + + t.Run("success", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{}, + "docker", "volume", "rm", volumeID, + ), + ) + + if err := w.Close(ctx); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + + t.Run("failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "volume", "rm", volumeID, + ), + ) + + if err := w.Close(ctx); err == nil { + t.Error("unexpected nil error") + } + }) } func TestVolumeWorkspace_DockerRunOpts(t *testing.T) { @@ -78,75 +151,242 @@ func TestVolumeWorkspace_WorkDir(t *testing.T) { } } -func expectCommand(success, failure int, command string, args ...string) (*expectedCommand, error) { - dir, err := ioutil.TempDir(os.TempDir(), "expect-*") - if err != nil { - return nil, errors.Wrap(err, "creating temporary directory to contain the expected command") - } +// For the below tests that essentially delegate to runScript, we're not going +// to test the content of the script file: we'll do that as a one off test at +// the bottom of runScript itself, rather than depending on script content that +// may drift over time. - called := path.Join(dir, command+".called") - script := `#!/bin/sh - -set -e -set -x - -retval=` + strconv.Itoa(success) - for i, arg := range args { - script += ` -if [ "$1" != ` + shellescape.Quote(arg) + ` ]; then - echo "Argument ` + strconv.Itoa(i+1) + ` is invalid: $1" - retval=` + strconv.Itoa(failure) + ` -fi -shift >/dev/null 2>&1 -` - } - script += ` -if [ "$retval" -eq ` + strconv.Itoa(success) + ` ]; then - touch ` + shellescape.Quote(called) + ` -fi - -exit "$retval"` - dest := path.Join(dir, command) - if err := ioutil.WriteFile(dest, []byte(script), 0755); err != nil { - return nil, errors.Wrapf(err, "writing script to %q", dest) - } +func TestVolumeWorkspace_Prepare(t *testing.T) { + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: volumeID} - oldPath := os.Getenv("PATH") - if oldPath != "" { - os.Setenv("PATH", dir+string(os.PathListSeparator)+oldPath) - } else { - os.Setenv("PATH", dir) - } + t.Run("success", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + if err := w.Prepare(ctx); err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + + t.Run("failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) - return &expectedCommand{ - called: called, - dir: dir, - oldPath: oldPath, - }, nil + if err := w.Prepare(ctx); err == nil { + t.Error("unexpected nil error") + } + }) } -type expectedCommand struct { - called string - dir string - oldPath string +func TestVolumeWorkspace_Changes(t *testing.T) { + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: volumeID} + + t.Run("success", func(t *testing.T) { + for name, tc := range map[string]struct { + stdout string + want *StepChanges + }{ + "empty": { + stdout: "", + want: &StepChanges{}, + }, + "valid": { + stdout: ` +M go.mod +M internal/campaigns/volume_workspace.go +M internal/campaigns/volume_workspace_test.go + `, + want: &StepChanges{Modified: []string{ + "go.mod", + "internal/campaigns/volume_workspace.go", + "internal/campaigns/volume_workspace_test.go", + }}, + }, + } { + t.Run(name, func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{Stdout: bytes.TrimSpace([]byte(tc.stdout))}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + have, err := w.Changes(ctx) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if diff := cmp.Diff(have, tc.want); diff != "" { + t.Errorf("unexpected changes (-have +want):\n\n%s", diff) + } + + }) + } + }) + + t.Run("failure", func(t *testing.T) { + for name, behaviour := range map[string]expect.Behaviour{ + "exit code": {ExitCode: 1}, + "malformed status": {Stdout: []byte("Z")}, + } { + t.Run(name, func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + behaviour, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + if _, err := w.Changes(ctx); err == nil { + t.Error("unexpected nil error") + } + }) + } + }) } -func (ec *expectedCommand) finalise(t *testing.T) { - t.Helper() +func TestVolumeWorkspace_Diff(t *testing.T) { + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: volumeID} - called := true - if _, err := os.Stat(path.Join(ec.called)); os.IsNotExist(err) { - called = false - } else if err != nil { - t.Fatalf("error stating called file %q: %v", ec.called, err) - } + t.Run("success", func(t *testing.T) { + for name, tc := range map[string]string{ + "empty": "", + "valid": ` +diff --git a/go.mod b/go.mod +index 06471f4..5f9d3fa 100644 +--- a/go.mod ++++ b/go.mod +@@ -7,6 +7,7 @@ require ( + github.com/alessio/shellescape v1.4.1 + github.com/dustin/go-humanize v1.0.0 + github.com/efritz/pentimento v0.0.0-20190429011147-ade47d831101 ++ github.com/gobwas/glob v0.2.3 + github.com/google/go-cmp v0.5.2 + github.com/hashicorp/errwrap v1.1.0 // indirect + github.com/hashicorp/go-multierror v1.1.0 + `, + } { + t.Run(name, func(t *testing.T) { + want := strings.TrimSpace(tc) - os.Setenv("PATH", ec.oldPath) - if err := os.RemoveAll(ec.dir); err != nil { - t.Fatalf("error removing expected command directory %q: %v", ec.dir, err) - } + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{Stdout: []byte(want)}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + have, err := w.Diff(ctx) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if diff := cmp.Diff(string(have), want); diff != "" { + t.Errorf("unexpected changes (-have +want):\n\n%s", diff) + } + + }) + } + }) + + t.Run("failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + if _, err := w.Diff(ctx); err == nil { + t.Error("unexpected nil error") + } + }) +} + +func TestVolumeWorkspace_runScript(t *testing.T) { + // Since the above tests have thoroughly tested our error handling, this + // test just fills in the one logical gap we have in our test coverage: is + // the temporary script file correct? + const script = "#!/bin/sh\n\necho FOO" + ctx := context.Background() + w := &dockerVolumeWorkspace{volume: volumeID} + + expect.Commands( + t, + &expect.Expectation{ + Validator: func(name string, arg ...string) error { + // Run normal glob validation of the command line. + glob := expect.NewGlobValidator( + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ) + if err := glob(name, arg...); err != nil { + return err + } + + // OK, we know that the temporary file the script lives in can + // be parsed out of the seventh parameter, which provides the + // mount options. Let's go get it! + values := strings.Split(arg[6], ",") + source := strings.SplitN(values[1], "=", 2) + have, err := ioutil.ReadFile(source[1]) + if err != nil { + return errors.Errorf("error reading temporary file %q: %v", source[1], err) + } + + if string(have) != script { + return errors.Errorf("unexpected script: have=%q want=%q", string(have), script) + } + return nil + }, + }, + ) - if !called { - t.Error("expected command not called") + if _, err := w.runScript(ctx, "/work", script); err != nil { + t.Fatal(err) } } diff --git a/internal/exec/exec.go b/internal/exec/exec.go new file mode 100644 index 0000000000..1da35175d0 --- /dev/null +++ b/internal/exec/exec.go @@ -0,0 +1,31 @@ +// Package exec provides wrapped implementations of os/exec's Command and +// CommandContext functions that allow for command creation to be overridden, +// thereby allowing commands to be mocked. +package exec + +import ( + "context" + goexec "os/exec" +) + +// CmdCreator instances are used to create commands. os/exec.CommandContext is a +// valid CmdCreator. +type CmdCreator func(context.Context, string, ...string) *goexec.Cmd + +// creator is the singleton used to create a new command. +var creator CmdCreator = goexec.CommandContext + +// Command wraps os/exec.Command, and implements the same behaviour. +func Command(name string, arg ...string) *goexec.Cmd { + return CommandContext(nil, name, arg...) +} + +// CommandContext wraps os/exec.CommandContext, and implements the same +// behaviour. +func CommandContext(ctx context.Context, name string, arg ...string) *goexec.Cmd { + // TODO: if we add global logging infrastructure to cmd/src, we could + // leverage it here to log all commands that are executed in an appropriate + // verbose mode. + + return creator(ctx, name, arg...) +} diff --git a/internal/exec/expect/expect.go b/internal/exec/expect/expect.go new file mode 100644 index 0000000000..a63a0ab8b7 --- /dev/null +++ b/internal/exec/expect/expect.go @@ -0,0 +1,197 @@ +// Package expect uses the middleware concept in internal/exec to mock external +// commands. +// +// Generally, you only want to use this package in testing code. +// +// At a high level, this package operates by overriding created commands to +// invoke the current executable with a specific environment variable, which +// points to a temporary file with metadata on the behaviour that the command +// should implement. (This approach is shamelessly stolen from Go's os/exec test +// suite, but the details are somewhat different.) +// +// This means that the main() function of the executable _must_ check the +// relevant environment variable as early as possible, and not perform its usual +// logic if it's found. An implementation of this is provided for TestMain +// functions in the Handle function, which is normally how this package is used. +package expect + +import ( + "context" + "encoding/json" + "fmt" + "io/ioutil" + "os" + goexec "os/exec" + "testing" + + "github.com/gobwas/glob" + "github.com/hashicorp/go-multierror" + "github.com/pkg/errors" + + "github.com/sourcegraph/src-cli/internal/exec" +) + +// envBehaviourFile is the environment variable used to define the behaviour +// file. +const envBehaviourFile = "GO_EXEC_TESTING_BEHAVIOUR_FILE" + +// Behaviour defines the behaviour of the mocked command. +type Behaviour struct { + // Stdout defines the data that will be returned on stdout. + Stdout []byte + + // Stderr defines the data that will be returned on stderr. + Stderr []byte + + // ExitCode is the exit code that the process will exit with. + ExitCode int +} + +// Commands defines a set of expected commands for the given test. Commands may +// be called from any number of nested subtests, but must only be called once +// from a single test function, as it uses (*testing.T).Cleanup to manage +// per-test state. +func Commands(t *testing.T, exp ...*Expectation) { + t.Helper() + + i := 0 + mc := exec.NewMiddleware(func(ctx context.Context, previous exec.CmdCreator, name string, arg ...string) *goexec.Cmd { + if i >= len(exp) { + t.Fatalf("one or more extra commands created in spite of expecting %d command(s); attempting to create %q with arguments %v", len(exp), name, arg) + } + + if err := exp[i].Validator(name, arg...); err != nil { + t.Fatalf("unexpected command: %v", err) + } + + // Create the command using the next level of middleware. (Which is + // probably eventually os/exec.CommandContext().) + cmd := previous(ctx, name, arg...) + if cmd == nil { + t.Fatalf("unexpected nil *Cmd for %q with arguments %v", name, arg) + } + + // Now we modify the command to re-invoke this executable instead. We'll + // also blank out the arguments, since this should be controlled + // entirely by the presence of the behaviour file environment variable. + cmd.Path = os.Args[0] + cmd.Args = []string{} + + // Actually create the behaviour file. + f, err := ioutil.TempFile(os.TempDir(), "behaviour") + if err != nil { + t.Fatalf("error creating behaviour file: %v", err) + } + defer f.Close() + file := f.Name() + t.Cleanup(func() { os.Remove(file) }) + + data, err := json.Marshal(&exp[i].Behaviour) + if err != nil { + t.Fatalf("error marshalling behaviour data: %v", err) + } + f.Write(data) + + // Set the relevant environment variable. + cmd.Env = append(cmd.Env, envBehaviourFile+"="+file) + + i++ + return cmd + }) + + t.Cleanup(func() { + mc.Remove() + + if i != len(exp) { + t.Fatalf("unexpected number of commands executed: have=%d want=%d", i, len(exp)) + } + }) +} + +// Handle should be called from TestMain. It intercepts expected commands and +// implements the expected behaviour. +// +// m is defined as an interface rather than *testing.M to make this usable from +// outside of a testing context. +func Handle(m interface{ Run() int }) int { + if file := os.Getenv(envBehaviourFile); file != "" { + panicErr := func(err error) { + fmt.Fprintf(os.Stderr, "panic with error: %s", err.Error()) + // This exit code is chosen at random: obviously, a test that + // expects a failure might just swallow this and be happy. We do the + // best we can. + os.Exit(255) + } + + // Load up the expected behaviour of this command. + data, err := ioutil.ReadFile(file) + if err != nil { + panicErr(err) + } + + var b Behaviour + if err := json.Unmarshal(data, &b); err != nil { + panicErr(err) + } + + // Do it! + os.Stderr.Write(b.Stderr) + os.Stdout.Write(b.Stdout) + os.Exit(b.ExitCode) + } + + return m.Run() +} + +// Expectation represents a single command invocation. +type Expectation struct { + Validator CommandValidator + Behaviour Behaviour +} + +// CommandValidator is used to validate the command line that is would be +// executed. +type CommandValidator func(name string, arg ...string) error + +// NewGlob is a convenience function that creates an Expectation that validates +// commands using a glob validator (as created by NewGlobValidator) and +// implements the given behaviour. +// +// You don't need to use this, but it tends to make Commands() calls more +// readable. +func NewGlob(behaviour Behaviour, wantName string, wantArg ...string) *Expectation { + return &Expectation{ + Behaviour: behaviour, + Validator: NewGlobValidator(wantName, wantArg...), + } +} + +// NewGlobValidator creates a validation function that will validate a command +// using glob syntax against the given name and arguments. +func NewGlobValidator(wantName string, wantArg ...string) CommandValidator { + wantNameGlob := glob.MustCompile(wantName) + wantArgGlobs := make([]glob.Glob, len(wantArg)) + for i, a := range wantArg { + wantArgGlobs[i] = glob.MustCompile(a) + } + + return func(haveName string, haveArg ...string) error { + var errs *multierror.Error + + if !wantNameGlob.Match(haveName) { + errs = multierror.Append(errs, errors.Errorf("name does not match: have=%q want=%q", haveName, wantName)) + } + + if len(haveArg) != len(wantArgGlobs) { + errs = multierror.Append(errs, errors.Errorf("unexpected number of arguments: have=%v want=%v", haveArg, wantArg)) + } else { + for i, g := range wantArgGlobs { + if !g.Match(haveArg[i]) { + errs = multierror.Append(errs, errors.Errorf("unexpected argument at position %d: have=%q want=%q", i, haveArg[i], wantArg[i])) + } + } + } + + return errs.ErrorOrNil() + } +} diff --git a/internal/exec/middleware.go b/internal/exec/middleware.go new file mode 100644 index 0000000000..3236047eb7 --- /dev/null +++ b/internal/exec/middleware.go @@ -0,0 +1,24 @@ +package exec + +import ( + "context" + goexec "os/exec" +) + +// CmdCreatorMiddleware creates *exec.Cmd instances that delegate command +// creation to a provided callback. +type CmdCreatorMiddleware struct{ previous CmdCreator } + +// NewMiddleware adds a middleware to the command creation stack. +func NewMiddleware(mock func(context.Context, CmdCreator, string, ...string) *goexec.Cmd) CmdCreatorMiddleware { + mc := CmdCreatorMiddleware{previous: creator} + creator = func(ctx context.Context, name string, arg ...string) *goexec.Cmd { + return mock(ctx, mc.previous, name, arg...) + } + return mc +} + +// Remove removes the command creation middleware from the stack. +func (mc CmdCreatorMiddleware) Remove() { + creator = mc.previous +} From 524e7e850b6215db0c3a005476c2cabda90de0e9 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 12:27:44 -0800 Subject: [PATCH 11/52] Fix comments. --- .github/workflows/docker.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 553ef1b83b..4b47d06c36 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,7 +21,7 @@ jobs: uses: elgohr/Publish-Docker-Github-Action@master with: # This needs to match the dockerWorkspaceImage constant in - # docker_workspace.go. + # volume_workspace.go. name: sourcegraph/src-campaign-volume-workspace username: ${{ secrets.DOCKER_USERNAME }} password: ${{ secrets.DOCKER_PASSWORD }} @@ -43,7 +43,7 @@ jobs: uses: elgohr/Publish-Docker-Github-Action@master with: # This needs to match the dockerWorkspaceImage constant in - # docker_workspace.go. + # volume_workspace.go. name: sourcegraph/src-campaign-volume-workspace username: ${{ secrets.DOCKER_USERNAME }} password: ${{ secrets.DOCKER_PASSWORD }} From c57bbeb9ae89499d8edcb03a9d4406682f585a4e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 12:27:58 -0800 Subject: [PATCH 12/52] Temporary enable build on all pushes for testing. --- .github/workflows/docker.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 4b47d06c36..ce87b1d5e6 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -5,10 +5,11 @@ name: Publish Docker image dependencies # We only want to build on releases; this condition is 100% stolen from the # goreleaser action. on: - push: - tags: - - "*" - - "!latest" + - push + # push: + # tags: + # - "*" + # - "!latest" jobs: publish: From 7ab154b8c18a9f0929658c87c72ea7c2f969b0fc Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 12:29:14 -0800 Subject: [PATCH 13/52] Fix username. --- .github/workflows/docker.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index ce87b1d5e6..bf863bbeb5 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -24,7 +24,7 @@ jobs: # This needs to match the dockerWorkspaceImage constant in # volume_workspace.go. name: sourcegraph/src-campaign-volume-workspace - username: ${{ secrets.DOCKER_USERNAME }} + username: sourcegraphci password: ${{ secrets.DOCKER_PASSWORD }} default_branch: main dockerfile: ./docker/campaign-volume-workspace/Dockerfile @@ -46,7 +46,7 @@ jobs: # This needs to match the dockerWorkspaceImage constant in # volume_workspace.go. name: sourcegraph/src-campaign-volume-workspace - username: ${{ secrets.DOCKER_USERNAME }} + username: sourcegraphci password: ${{ secrets.DOCKER_PASSWORD }} default_branch: main dockerfile: ./docker/campaign-volume-workspace/Dockerfile From e74781f4dfec0d87a0bc7fde8ba442b2884243d1 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 13:45:38 -0800 Subject: [PATCH 14/52] Replace the canned Action with a script. --- .github/workflows/docker.yml | 36 +-------- docker/campaign-volume-workspace/push.py | 95 ++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 32 deletions(-) create mode 100755 docker/campaign-volume-workspace/push.py diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index bf863bbeb5..d7938d2da0 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -18,35 +18,7 @@ jobs: - name: Checkout uses: actions/checkout@v2 - - name: Publish to Docker Hub - uses: elgohr/Publish-Docker-Github-Action@master - with: - # This needs to match the dockerWorkspaceImage constant in - # volume_workspace.go. - name: sourcegraph/src-campaign-volume-workspace - username: sourcegraphci - password: ${{ secrets.DOCKER_PASSWORD }} - default_branch: main - dockerfile: ./docker/campaign-volume-workspace/Dockerfile - # This gets us idiomatic Docker versioning (ie tags 3, 3.23, and - # 3.23.2 for a 3.23.2 release), but at the cost of losing the latest - # tag. We'll deal with that momentarily. - tag_semver: true - - # We now have to run the publish action again to publish the latest tag, - # due to - # https://github.com/elgohr/Publish-Docker-Github-Action/issues/70: in - # short, tag_semver and tags are mutually exclusive. In practice, this - # image is small enough and quick enough to build that I'm not terribly - # concerned, and none of the more complicated Docker actions feel worth - # pulling in here. - - name: Publish to Docker Hub - uses: elgohr/Publish-Docker-Github-Action@master - with: - # This needs to match the dockerWorkspaceImage constant in - # volume_workspace.go. - name: sourcegraph/src-campaign-volume-workspace - username: sourcegraphci - password: ${{ secrets.DOCKER_PASSWORD }} - default_branch: main - dockerfile: ./docker/campaign-volume-workspace/Dockerfile + - run: ./docker/campaign-volume-workspace/push.py -d ./docker/campaign-volume-workspace/Dockerfile -i sourcegraph/src-campaign-volume-workspace + env: + DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} + DOCKER_USERNAME: sourcegraphci diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py new file mode 100755 index 0000000000..de0331314f --- /dev/null +++ b/docker/campaign-volume-workspace/push.py @@ -0,0 +1,95 @@ +#!/usr/bin/env python3 + +import argparse +import itertools +import os +import subprocess + +from typing import BinaryIO, Sequence + + +def calculate_tags(ref: str) -> Sequence[str]: + # The tags always include latest. + tags = ["latest"] + + # If the ref is a tag ref, then we should parse the version out. + if ref.startswith("refs/tags/"): + tags.extend( + [ + ".".join(vc) + for vc in itertools.accumulate( + ref.split("/", 2)[2].split("."), + lambda vlist, v: vlist + [v], + initial=[], + ) + if len(vc) > 0 + ] + ) + + return tags + + +def docker_build(dockerfile: BinaryIO, image: str, tags: Sequence[str]): + run( + [ + "docker", + "build", + *itertools.chain(*[("-t", f"{image}:{tag}") for tag in tags]), + "-", + ], + stdin=dockerfile, + ) + + +def docker_login(username: str, password: str): + run( + ["docker", "login", f"-u={username}", "--password-stdin"], + input=password, + ) + + +def docker_push(image: str, tags: Sequence[str]): + for tag in tags: + subprocess.run(["docker", "push", f"{image}:{tag}"]) + + +def run(args: Sequence[str], /, **kwargs) -> subprocess.CompletedProcess: + print(f"+ {' '.join(args)}") + return subprocess.run(args, **kwargs) + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument( + "-d", "--dockerfile", required=True, help="the Dockerfile to build" + ) + parser.add_argument("-i", "--image", required=True, help="Docker image to push") + parser.add_argument( + "-r", + "--ref", + default=os.environ.get("GITHUB_REF"), + help="current ref in refs/heads/... or refs/tags/... form", + ) + args = parser.parse_args() + + tags = calculate_tags(args.ref) + print(f"will push tags: {', '.join(tags)}") + + print("building image") + docker_build(open(args.dockerfile, "rb"), args.image, tags) + + print("logging into Docker Hub") + try: + docker_login(os.environ["DOCKER_USERNAME"], os.environ["DOCKER_PASSWORD"]) + except KeyError as e: + print(f"error retrieving environment variables: {e}") + raise + + print("pushing images to Docker Hub") + docker_push(args.image, tags) + + print("success!") + + +if __name__ == "__main__": + main() From 59ee60b6760a609589a0ee9f56b591a2d04710ca Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 13:49:04 -0800 Subject: [PATCH 15/52] Run on Ubuntu 20.04. --- .github/workflows/docker.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index d7938d2da0..3532d3ac3e 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -13,7 +13,7 @@ on: jobs: publish: - runs-on: ubuntu-latest + runs-on: ubuntu-20.04 steps: - name: Checkout uses: actions/checkout@v2 From 201b120cf2641c5b8eab9bf89bfc401505dd8895 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 13:50:10 -0800 Subject: [PATCH 16/52] Use text mode when logging in. --- docker/campaign-volume-workspace/push.py | 1 + 1 file changed, 1 insertion(+) diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index de0331314f..19b1d46e2e 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -45,6 +45,7 @@ def docker_login(username: str, password: str): run( ["docker", "login", f"-u={username}", "--password-stdin"], input=password, + text=True, ) From 27796124283e3351309cd0c54e5be7f74eaf5571 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 13:53:20 -0800 Subject: [PATCH 17/52] Only build the Docker image on tag push. --- .github/workflows/docker.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 3532d3ac3e..7f9c2a6e9a 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -5,11 +5,10 @@ name: Publish Docker image dependencies # We only want to build on releases; this condition is 100% stolen from the # goreleaser action. on: - - push - # push: - # tags: - # - "*" - # - "!latest" + push: + tags: + - "*" + - "!latest" jobs: publish: From cd7d70f383c2d5bc416b13d89db5bea017f150d4 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:05:31 -0800 Subject: [PATCH 18/52] Multiplatform experimentation, part one. --- .github/workflows/docker.yml | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 7f9c2a6e9a..fa6c0a28ab 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -5,10 +5,11 @@ name: Publish Docker image dependencies # We only want to build on releases; this condition is 100% stolen from the # goreleaser action. on: - push: - tags: - - "*" - - "!latest" + - push + # push: + # tags: + # - "*" + # - "!latest" jobs: publish: @@ -17,6 +18,12 @@ jobs: - name: Checkout uses: actions/checkout@v2 + - name: Set up Docker buildx + uses: docker/setup-buildx-action@v1 + + - name: Available platforms + run: echo ${{ steps.buildx.outputs.platforms }} + - run: ./docker/campaign-volume-workspace/push.py -d ./docker/campaign-volume-workspace/Dockerfile -i sourcegraph/src-campaign-volume-workspace env: DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} From baf2e902ee1ef95b7520221f0ef9070b67602e08 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:07:45 -0800 Subject: [PATCH 19/52] Multiplatform experimentation, part _n_. --- .github/workflows/docker.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index fa6c0a28ab..8ff7b72454 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -21,8 +21,13 @@ jobs: - name: Set up Docker buildx uses: docker/setup-buildx-action@v1 + - name: Set up QEMU + uses: docker/setup-qemu-action@v1 + with: + platforms: arm64 + - name: Available platforms - run: echo ${{ steps.buildx.outputs.platforms }} + run: docker buildx ls - run: ./docker/campaign-volume-workspace/push.py -d ./docker/campaign-volume-workspace/Dockerfile -i sourcegraph/src-campaign-volume-workspace env: From c372daff4ee3a471c65f40be6af0adc9be8f2951 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:17:01 -0800 Subject: [PATCH 20/52] More multiarch. --- .github/workflows/docker.yml | 5 +-- docker/campaign-volume-workspace/push.py | 43 ++++++++++++------------ 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 8ff7b72454..755bd71bb8 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -26,10 +26,7 @@ jobs: with: platforms: arm64 - - name: Available platforms - run: docker buildx ls - - - run: ./docker/campaign-volume-workspace/push.py -d ./docker/campaign-volume-workspace/Dockerfile -i sourcegraph/src-campaign-volume-workspace + - run: ./docker/campaign-volume-workspace/push.py -d ./docker/campaign-volume-workspace/Dockerfile -i sourcegraph/src-campaign-volume-workspace -p linux/amd64,linux/arm64,linux/386 env: DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} DOCKER_USERNAME: sourcegraphci diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index 19b1d46e2e..f8e8483625 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -5,7 +5,7 @@ import os import subprocess -from typing import BinaryIO, Sequence +from typing import BinaryIO, Optional, Sequence def calculate_tags(ref: str) -> Sequence[str]: @@ -29,16 +29,20 @@ def calculate_tags(ref: str) -> Sequence[str]: return tags -def docker_build(dockerfile: BinaryIO, image: str, tags: Sequence[str]): - run( - [ - "docker", - "build", - *itertools.chain(*[("-t", f"{image}:{tag}") for tag in tags]), - "-", - ], - stdin=dockerfile, - ) +def docker_build( + dockerfile: BinaryIO, platform: Optional[str], image: str, tags: Sequence[str] +): + args = ["docker", "buildx", "build", "--push"] + + for tag in tags: + args.extend(["-t", tag]) + + if platform is not None: + args.extend(["--platform", platform]) + + args.append("-") + + run(args, stdin=dockerfile) def docker_login(username: str, password: str): @@ -49,11 +53,6 @@ def docker_login(username: str, password: str): ) -def docker_push(image: str, tags: Sequence[str]): - for tag in tags: - subprocess.run(["docker", "push", f"{image}:{tag}"]) - - def run(args: Sequence[str], /, **kwargs) -> subprocess.CompletedProcess: print(f"+ {' '.join(args)}") return subprocess.run(args, **kwargs) @@ -65,6 +64,11 @@ def main(): "-d", "--dockerfile", required=True, help="the Dockerfile to build" ) parser.add_argument("-i", "--image", required=True, help="Docker image to push") + parser.add_argument( + "-p", + "--platform", + help="platforms to build using docker buildx (if omitted, the default will be used)", + ) parser.add_argument( "-r", "--ref", @@ -76,9 +80,6 @@ def main(): tags = calculate_tags(args.ref) print(f"will push tags: {', '.join(tags)}") - print("building image") - docker_build(open(args.dockerfile, "rb"), args.image, tags) - print("logging into Docker Hub") try: docker_login(os.environ["DOCKER_USERNAME"], os.environ["DOCKER_PASSWORD"]) @@ -86,8 +87,8 @@ def main(): print(f"error retrieving environment variables: {e}") raise - print("pushing images to Docker Hub") - docker_push(args.image, tags) + print("building and pushing image") + docker_build(open(args.dockerfile, "rb"), args.platform, args.image, tags) print("success!") From 0baabff86504ac143797d8897c92fac4b2cadd76 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:20:10 -0800 Subject: [PATCH 21/52] More multiarch. --- docker/campaign-volume-workspace/push.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index f8e8483625..b46fdba6dd 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -32,7 +32,7 @@ def calculate_tags(ref: str) -> Sequence[str]: def docker_build( dockerfile: BinaryIO, platform: Optional[str], image: str, tags: Sequence[str] ): - args = ["docker", "buildx", "build", "--push"] + args = ["docker", "buildx", "build"] for tag in tags: args.extend(["-t", tag]) @@ -53,9 +53,14 @@ def docker_login(username: str, password: str): ) +def docker_push(image: str, tags: Sequence[str]): + for tag in tags: + subprocess.run(["docker", "push", f"{image}:{tag}"]) + + def run(args: Sequence[str], /, **kwargs) -> subprocess.CompletedProcess: print(f"+ {' '.join(args)}") - return subprocess.run(args, **kwargs) + return subprocess.run(args, check=True, **kwargs) def main(): @@ -80,6 +85,9 @@ def main(): tags = calculate_tags(args.ref) print(f"will push tags: {', '.join(tags)}") + print("building image") + docker_build(open(args.dockerfile, "rb"), args.platform, args.image, tags) + print("logging into Docker Hub") try: docker_login(os.environ["DOCKER_USERNAME"], os.environ["DOCKER_PASSWORD"]) @@ -87,8 +95,8 @@ def main(): print(f"error retrieving environment variables: {e}") raise - print("building and pushing image") - docker_build(open(args.dockerfile, "rb"), args.platform, args.image, tags) + print("pushing images to Docker Hub") + docker_push(args.image, tags) print("success!") From 0f3bf810127f32101f2b8103a27e4d445e0ecbf9 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:24:28 -0800 Subject: [PATCH 22/52] More multiarch. --- docker/campaign-volume-workspace/push.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index b46fdba6dd..09e421935f 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -55,7 +55,7 @@ def docker_login(username: str, password: str): def docker_push(image: str, tags: Sequence[str]): for tag in tags: - subprocess.run(["docker", "push", f"{image}:{tag}"]) + run(["docker", "push", f"{image}:{tag}"]) def run(args: Sequence[str], /, **kwargs) -> subprocess.CompletedProcess: From ef9ef4b280cad27c3d4f4715b15010d892d045d7 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:30:02 -0800 Subject: [PATCH 23/52] More multiarch. --- docker/campaign-volume-workspace/push.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index 09e421935f..00a4d1275f 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -32,7 +32,7 @@ def calculate_tags(ref: str) -> Sequence[str]: def docker_build( dockerfile: BinaryIO, platform: Optional[str], image: str, tags: Sequence[str] ): - args = ["docker", "buildx", "build"] + args = ["docker", "buildx", "build", "--push"] for tag in tags: args.extend(["-t", tag]) @@ -53,11 +53,6 @@ def docker_login(username: str, password: str): ) -def docker_push(image: str, tags: Sequence[str]): - for tag in tags: - run(["docker", "push", f"{image}:{tag}"]) - - def run(args: Sequence[str], /, **kwargs) -> subprocess.CompletedProcess: print(f"+ {' '.join(args)}") return subprocess.run(args, check=True, **kwargs) @@ -85,9 +80,6 @@ def main(): tags = calculate_tags(args.ref) print(f"will push tags: {', '.join(tags)}") - print("building image") - docker_build(open(args.dockerfile, "rb"), args.platform, args.image, tags) - print("logging into Docker Hub") try: docker_login(os.environ["DOCKER_USERNAME"], os.environ["DOCKER_PASSWORD"]) @@ -95,8 +87,8 @@ def main(): print(f"error retrieving environment variables: {e}") raise - print("pushing images to Docker Hub") - docker_push(args.image, tags) + print("building and pushing image") + docker_build(open(args.dockerfile, "rb"), args.platform, args.image, tags) print("success!") From 0451369cb4b2f70596c82a5aaf5026fdf1c7bdb6 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:31:49 -0800 Subject: [PATCH 24/52] More multiarch. --- docker/campaign-volume-workspace/push.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index 00a4d1275f..785dca0000 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -35,7 +35,7 @@ def docker_build( args = ["docker", "buildx", "build", "--push"] for tag in tags: - args.extend(["-t", tag]) + args.extend(["-t", f"{image}:{tag}"]) if platform is not None: args.extend(["--platform", platform]) From 1c64e5aa5a0f09b1155c4680736fc6e74c244ba5 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:59:08 -0800 Subject: [PATCH 25/52] Improve docs. --- DEVELOPMENT.md | 2 +- docker/campaign-volume-workspace/push.py | 41 +++++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 5171aebd49..5ad9b88909 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -41,7 +41,7 @@ Note that if instead the recommended src-cli version for Sourcegraph `3.99` was ## Dependent Docker images -`src campaign apply` and `src campaign preview` use a Docker image published as `sourcegraph/src-campaign-volume-workspace` for utility purposes when the volume workspace is selected, which is the default on macOS. This Docker image is updated by the GitHub Action workflow described in [`docker.yml`](.github/workflows/docker.yml). +`src campaign apply` and `src campaign preview` use a Docker image published as `sourcegraph/src-campaign-volume-workspace` for utility purposes when the volume workspace is selected, which is the default on macOS. This [Docker image](./docker/campaign-volume-workspace/Dockerfile) is built by [a Python script](./docker/campaign-volume-workspace/push.sh) invoked by the GitHub Action workflow described in [`docker.yml`](.github/workflows/docker.yml). To build and develop this locally, you can build and tag the image with: diff --git a/docker/campaign-volume-workspace/push.py b/docker/campaign-volume-workspace/push.py index 785dca0000..d230afb50c 100755 --- a/docker/campaign-volume-workspace/push.py +++ b/docker/campaign-volume-workspace/push.py @@ -1,5 +1,34 @@ #!/usr/bin/env python3 +# This is a very simple script to build and push the Docker image used by the +# Docker volume workspace driver. It's normally run from the "Publish Docker +# image dependencies" GitHub Action, but can be run locally if necessary. +# +# This script requires Python 3.8 or later, and Docker 19.03 (for buildx). You +# are strongly encouraged to use Black to format this file after modifying it. +# +# To run it locally, you'll need to be logged into Docker Hub, and create an +# image in a namespace that you have access to. For example, if your username is +# "alice", you could build and push the image as follows: +# +# $ ./push.py -d Dockerfile -i alice/src-campaign-volume-workspace +# +# By default, only the "latest" tag will be built and pushed. The script refers +# to the HEAD ref given to it, either via $GITHUB_REF or the -r argument. If +# this is in the form refs/tags/X.Y.Z, then we'll also push X, X.Y, and X.Y.Z +# tags. +# +# Finally, if you have your environment configured to allow multi-architecture +# builds with docker buildx, you can provide a --platform argument that will be +# passed through verbatim to docker buildx build. (This is how we build ARM64 +# builds in our CI.) For example, you could build ARM64 and AMD64 images with: +# +# $ ./push.py --platform linux/arm64,linux/amd64 ... +# +# For this to work, you will need a builder with the relevant platforms enabled. +# More instructions on this can be found at +# https://docs.docker.com/buildx/working-with-buildx/#build-multi-platform-images. + import argparse import itertools import os @@ -12,16 +41,23 @@ def calculate_tags(ref: str) -> Sequence[str]: # The tags always include latest. tags = ["latest"] - # If the ref is a tag ref, then we should parse the version out. + # If the ref is a tag ref, then we should parse the version out and add each + # component to our tag list: for example, for X.Y.Z, we want tags X, X.Y, + # and X.Y.Z. if ref.startswith("refs/tags/"): tags.extend( [ + # Join the version components back into a string. ".".join(vc) for vc in itertools.accumulate( + # Split the version string into its components. ref.split("/", 2)[2].split("."), + # Accumulate each component we've seen into a new list + # entry. lambda vlist, v: vlist + [v], initial=[], ) + # We also get the initial value, so we need to skip that. if len(vc) > 0 ] ) @@ -40,6 +76,9 @@ def docker_build( if platform is not None: args.extend(["--platform", platform]) + # Since we provide the Dockerfile via stdin, we don't need to provide it + # here. (Doing so means that we don't carry an unncessary context into the + # builder.) args.append("-") run(args, stdin=dockerfile) From 1cf0927e0f2e1b449763e6d39c8809d16975778d Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 14:59:30 -0800 Subject: [PATCH 26/52] Also push the README to Docker Hub. --- .github/workflows/docker.yml | 8 ++++++++ docker/campaign-volume-workspace/README.md | 7 +++++++ 2 files changed, 15 insertions(+) create mode 100644 docker/campaign-volume-workspace/README.md diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 755bd71bb8..f97cf0f10b 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -30,3 +30,11 @@ jobs: env: DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} DOCKER_USERNAME: sourcegraphci + + - name: Update Docker Hub description + uses: peter-evans/dockerhub-description@v2 + with: + username: sourcegraphci + password: ${{ secrets.DOCKER_PASSWORD }} + repository: sourcegraph/src-campaign-volume-workspace + readme-filepath: ./docker/campaign-volume-workspace/README.md diff --git a/docker/campaign-volume-workspace/README.md b/docker/campaign-volume-workspace/README.md new file mode 100644 index 0000000000..ee7c4238d8 --- /dev/null +++ b/docker/campaign-volume-workspace/README.md @@ -0,0 +1,7 @@ +# `src` volume workspace base image + +Sourcegraph [`src`] executes campaigns using either a bind or volume workspace. In the latter case (which is the default on macOS), this utility image is used to initialise the volume workspace within Docker, and then to extract the diff used when creating the changeset. + +This image is based on Alpine, and adds the tools we need: curl, git, and unzip. + +For more information, please refer to the [`src-cli` repository](https://github.com/sourcegraph/src-cli/tree/main/docker/campaign-volume-workspace). From e47c2339a4fbdf9ae7553007ad6bba8c7a05bd2e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 15:00:35 -0800 Subject: [PATCH 27/52] Fix README. --- docker/campaign-volume-workspace/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/campaign-volume-workspace/README.md b/docker/campaign-volume-workspace/README.md index ee7c4238d8..c4a4a9ed6a 100644 --- a/docker/campaign-volume-workspace/README.md +++ b/docker/campaign-volume-workspace/README.md @@ -1,6 +1,6 @@ # `src` volume workspace base image -Sourcegraph [`src`] executes campaigns using either a bind or volume workspace. In the latter case (which is the default on macOS), this utility image is used to initialise the volume workspace within Docker, and then to extract the diff used when creating the changeset. +Sourcegraph `src` executes campaigns using either a bind or volume workspace. In the latter case (which is the default on macOS), this utility image is used to initialise the volume workspace within Docker, and then to extract the diff used when creating the changeset. This image is based on Alpine, and adds the tools we need: curl, git, and unzip. From 72c8aa9dfcb8fd7d0c3c0fb59e569e997e50d0cf Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 15:00:52 -0800 Subject: [PATCH 28/52] Revert to only updating the image on tags. --- .github/workflows/docker.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index f97cf0f10b..b880e26361 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -5,11 +5,10 @@ name: Publish Docker image dependencies # We only want to build on releases; this condition is 100% stolen from the # goreleaser action. on: - - push - # push: - # tags: - # - "*" - # - "!latest" + push: + tags: + - "*" + - "!latest" jobs: publish: From f3ddce4be6c86e5706bb78f1f3637674b6a3ec5e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 15:02:49 -0800 Subject: [PATCH 29/52] Add more docs. --- .github/workflows/docker.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index b880e26361..3af0f7929f 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -17,9 +17,12 @@ jobs: - name: Checkout uses: actions/checkout@v2 + # We need buildx to be able to build a multi-architecture image. - name: Set up Docker buildx uses: docker/setup-buildx-action@v1 + # We also need QEMU, since this is running on an AMD64 host and we want to + # build ARM64 images. - name: Set up QEMU uses: docker/setup-qemu-action@v1 with: From 0de0d792f7b84908a0bceb085eece8b70629490a Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 15:57:24 -0800 Subject: [PATCH 30/52] Only enable volume workspace on x86-64 for now. --- cmd/src/campaigns_common.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index f4c6b02011..905a455b8b 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -101,8 +101,11 @@ func newCampaignsApplyFlags(flagSet *flag.FlagSet, cacheDir, tempDir string) *ca "If true, errors encountered while executing steps in a repository won't stop the execution of the campaign spec but only cause that repository to be skipped.", ) + // We default to bind workspaces on everything except AMD64 macOS at + // present. In the future, we'll likely want to switch the default for ARM64 + // macOS as well, but this requires access to the hardware for testing. var defaultWorkspace string - if runtime.GOOS == "darwin" { + if runtime.GOOS == "darwin" && runtime.GOARCH == "amd64" { defaultWorkspace = "volume" } else { defaultWorkspace = "bind" From f184d7c7bf09a1a07eb782c7f76df9b2ea7a3e6e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 31 Dec 2020 17:19:39 -0800 Subject: [PATCH 31/52] Improve workspace dependent Docker image handling. --- cmd/src/campaigns_common.go | 6 +-- internal/campaigns/bind_workspace.go | 2 + internal/campaigns/service.go | 59 ++++++++++++++++++++++---- internal/campaigns/volume_workspace.go | 4 ++ internal/campaigns/workspace.go | 4 ++ 5 files changed, 63 insertions(+), 12 deletions(-) diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 905a455b8b..e6051bd544 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -226,10 +226,10 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se imageProgress := out.Progress([]output.ProgressBar{{ Label: "Preparing container images", - Max: float64(len(campaignSpec.Steps) + 1), + Max: 1.0, }}, nil) - err = svc.SetDockerImages(ctx, campaignSpec, func(step int) { - imageProgress.SetValue(0, float64(step)) + err = svc.SetDockerImages(ctx, opts.Creator, campaignSpec, func(perc float64) { + imageProgress.SetValue(0, perc) }) if err != nil { return "", "", err diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index e46abace30..6dc2e31aa9 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -66,6 +66,8 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. return &dockerBindWorkspace{dir: workspace}, nil } +func (*dockerBindWorkspaceCreator) DockerImages() []string { return []string{} } + type dockerBindWorkspace struct { dir string } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 8ee634eeb7..064f5a32a1 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -209,20 +209,61 @@ func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) Workspac return &dockerBindWorkspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} } -func (svc *Service) SetDockerImages(ctx context.Context, spec *CampaignSpec, progress func(i int)) error { +// dockerImageSet represents a set of Docker images that need to be pulled. The +// keys are the Docker image names; the values are a slice of pointers to +// strings that should be set to the content digest of the image. +type dockerImageSet map[string][]*string + +func (dis dockerImageSet) add(image string, digestPtr *string) { + if digestPtr == nil { + // Since we don't have a digest pointer here, we just need to ensure + // that the image exists at all in the map. + if _, ok := dis[image]; !ok { + dis[image] = []*string{} + } + } else { + // Either append the digest pointer to an existing map entry, or add a + // new entry if required. + if digests, ok := dis[image]; ok { + dis[image] = append(digests, digestPtr) + } else { + dis[image] = []*string{digestPtr} + } + } +} + +// SetDockerImages updates the steps within the campaign spec to include the +// exact content digest to be used when running each step, and ensures that all +// Docker images are available, including any required by the workspace creator. +// +// 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, creator WorkspaceCreator, spec *CampaignSpec, progress func(perc float64)) error { + images := dockerImageSet{} for i, step := range spec.Steps { - image, err := getDockerImageContentDigest(ctx, step.Container) + images.add(step.Container, &spec.Steps[i].image) + } + + // The workspace creator may have its own dependencies. + for _, image := range creator.DockerImages() { + images.add(image, nil) + } + + progress(0) + i := 0 + for image, digests := range images { + digest, err := getDockerImageContentDigest(ctx, image) if err != nil { - return err + return errors.Wrapf(err, "getting content digest for image %q", image) + } + for _, digestPtr := range digests { + *digestPtr = digest } - spec.Steps[i].image = image - progress(i + 1) - } - if _, err := getDockerImageContentDigest(ctx, dockerWorkspaceImage); err != nil { - return errors.Wrap(err, "downloading base image") + progress(float64(i) / float64(len(images))) + i++ } - progress(len(spec.Steps) + 1) + progress(1) return nil } diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 584252fbd4..9b08a997ae 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -25,6 +25,10 @@ func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphq return w, w.init(ctx, wc.client, repo) } +func (*dockerVolumeWorkspaceCreator) DockerImages() []string { + return []string{dockerWorkspaceImage} +} + // dockerVolumeWorkspace workspaces are placed on Docker volumes (surprise!), // and are therefore transparent to the host filesystem. This has performance // advantages if bind mounts are slow, such as on Docker for Mac, but could make diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index c98c2b9ffc..52c442a549 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -19,6 +19,10 @@ type WorkspaceCreator interface { // Create should clone the given repository, and perform whatever other // initial setup is required. Create(context.Context, *graphql.Repository) (Workspace, error) + + // DockerImages returns any Docker images required to use workspaces created + // by this creator. + DockerImages() []string } // Workspace implementations manage per-changeset storage when executing From 7027d7d9e50536ea1df26795d62e43c56fce54c6 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:24:15 -0800 Subject: [PATCH 32/52] Update CHANGELOG.md Co-authored-by: Thorsten Ball --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61ac408a6..dea80b0594 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ All notable changes to `src-cli` are documented in this file. ### Added -- Campaigns can now be executed using workspaces contained entirely on Docker volumes, rather than bind mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. `src campaign apply` and `src campaign preview` accept optional `-workspace` flags to override the default. []() +- `src campaign [apply|preview] can now make use of Docker volumes, rather than bind-mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. The optional `-workspace` flag can be used to override the default. []() ### Changed From 0057a57927d34a6104698385245144525fb44b96 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:24:34 -0800 Subject: [PATCH 33/52] Update DEVELOPMENT.md Co-authored-by: Thorsten Ball --- DEVELOPMENT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 5ad9b88909..8fc2a1d6b1 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -41,7 +41,7 @@ Note that if instead the recommended src-cli version for Sourcegraph `3.99` was ## Dependent Docker images -`src campaign apply` and `src campaign preview` use a Docker image published as `sourcegraph/src-campaign-volume-workspace` for utility purposes when the volume workspace is selected, which is the default on macOS. This [Docker image](./docker/campaign-volume-workspace/Dockerfile) is built by [a Python script](./docker/campaign-volume-workspace/push.sh) invoked by the GitHub Action workflow described in [`docker.yml`](.github/workflows/docker.yml). +`src campaign apply` and `src campaign preview` use a Docker image published as `sourcegraph/src-campaign-volume-workspace` for utility purposes when the volume workspace is selected, which is the default on macOS. This [Docker image](./docker/campaign-volume-workspace/Dockerfile) is built by [a Python script](./docker/campaign-volume-workspace/push.py) invoked by the GitHub Action workflow described in [`docker.yml`](.github/workflows/docker.yml). To build and develop this locally, you can build and tag the image with: From 5d6ea2509affbdb764b360b78fc71354c96499cf Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:25:50 -0800 Subject: [PATCH 34/52] Update internal/api/mock/mock.go Co-authored-by: Thorsten Ball --- internal/api/mock/mock.go | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/internal/api/mock/mock.go b/internal/api/mock/mock.go index 2047878770..4e835cc0c2 100644 --- a/internal/api/mock/mock.go +++ b/internal/api/mock/mock.go @@ -21,12 +21,7 @@ func ParrotClient(t *testing.T, resp []byte) (api.Client, error) { return nil, errors.Wrap(err, "listening on random port") } - srv := &http.Server{ - Handler: &mockHandler{ - data: resp, - t: t, - }, - } + srv := &http.Server{Handler: mockHandler(t, resp)} go srv.Serve(l) var buf bytes.Buffer @@ -41,21 +36,11 @@ func ParrotClient(t *testing.T, resp []byte) (api.Client, error) { }), nil } -// mockHandler implements a HTTP handler that always returns the given data as its response, and always succeeds. -type mockHandler struct { - data []byte - t interface { - Logf(format string, args ...interface{}) - } -} - -var _ http.Handler = &mockHandler{} - -func (ms *mockHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if ms.t != nil { - ms.t.Logf("handling request: %+v", r) - } - - w.WriteHeader(200) - w.Write(ms.data) +// mockHandler returns a HTTP handler that always returns the given data as its response, and always succeeds. +func mockHandler(t *testing.T, data []byte) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Logf("handling request: %+v", r) + w.WriteHeader(200) + w.Write(data) + }) } From 21a684d64f1c17cad52e86926816776d381f788b Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:35:23 -0800 Subject: [PATCH 35/52] Move directory creation behind exists check. --- internal/campaigns/bind_workspace.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index 6dc2e31aa9..5434cb2ad7 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -33,15 +33,15 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. return nil, err } - // Unlike the mkdirAll() calls elsewhere in this file, this is only giving - // us a temporary place on the filesystem to keep the archive. Since it's - // never mounted into the containers being run, we can keep these - // directories 0700 without issue. - if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { - return nil, err - } - if !exists { + // Unlike the mkdirAll() calls elsewhere in this file, this is only + // giving us a temporary place on the filesystem to keep the archive. + // Since it's never mounted into the containers being run, we can keep + // these directories 0700 without issue. + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + return nil, err + } + err = fetchRepositoryArchive(ctx, wc.client, repo, path) if err != nil { // If the context got cancelled, or we ran out of disk space, or From f318d6cc50b0292409c818d2f5e09025db2c1f7b Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:35:54 -0800 Subject: [PATCH 36/52] Rename test function. --- internal/campaigns/bind_workspace_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/campaigns/bind_workspace_test.go b/internal/campaigns/bind_workspace_test.go index ffeb0f55e7..60df2598d3 100644 --- a/internal/campaigns/bind_workspace_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -17,7 +17,7 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -func TestWorkspaceCreator_Create(t *testing.T) { +func TestDockerBindWorkspaceCreator_Create(t *testing.T) { workspaceTmpDir := func(t *testing.T) string { testTempDir, err := ioutil.TempDir("", "executor-integration-test-*") if err != nil { From ab3468a5694565fc7525c65df12cd148523d0906 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:37:32 -0800 Subject: [PATCH 37/52] Simplify test. --- internal/campaigns/bind_workspace_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/internal/campaigns/bind_workspace_test.go b/internal/campaigns/bind_workspace_test.go index 60df2598d3..a9cf4a7893 100644 --- a/internal/campaigns/bind_workspace_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -12,7 +12,6 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) @@ -331,14 +330,9 @@ func isDir(t *testing.T, path string) bool { } func readWorkspaceFiles(workspace Workspace) (map[string]string, error) { - w, ok := workspace.(*dockerBindWorkspace) - if !ok { - return nil, errors.Errorf("unknown workspace of type %T: %+v", workspace, workspace) - } - files := map[string]string{} - - err := filepath.Walk(w.dir, func(path string, info os.FileInfo, err error) error { + wdir := workspace.WorkDir() + err := filepath.Walk(*wdir, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -351,7 +345,7 @@ func readWorkspaceFiles(workspace Workspace) (map[string]string, error) { return err } - rel, err := filepath.Rel(w.dir, path) + rel, err := filepath.Rel(*wdir, path) if err != nil { return err } From 9f830c7e8b1fa33f60df7b32c5b5b0383d2fa726 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 15:38:21 -0800 Subject: [PATCH 38/52] Make Files private. --- internal/campaigns/run_steps.go | 22 +++++++++++----------- internal/campaigns/run_steps_test.go | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 26f905ccd8..c9c803c6ef 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -211,7 +211,7 @@ func runSteps(ctx context.Context, wc WorkspaceCreator, repo *graphql.Repository return nil, errors.Wrap(err, "getting changed files in step") } - results[i] = StepResult{Files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} + results[i] = StepResult{files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} } reportProgress("Calculating diff") @@ -435,8 +435,8 @@ 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 are the changes made to files by the step. + files *StepChanges // Stdout is the output produced by the step on standard out. Stdout *bytes.Buffer @@ -454,32 +454,32 @@ type StepChanges struct { // ModifiedFiles returns the files modified by a step. func (r StepResult) ModifiedFiles() []string { - if r.Files != nil { - return r.Files.Modified + if r.files != nil { + return r.files.Modified } return []string{} } // AddedFiles returns the files added by a step. func (r StepResult) AddedFiles() []string { - if r.Files != nil { - return r.Files.Added + if r.files != nil { + return r.files.Added } return []string{} } // DeletedFiles returns the files deleted by a step. func (r StepResult) DeletedFiles() []string { - if r.Files != nil { - return r.Files.Deleted + if r.files != nil { + return r.files.Deleted } return []string{} } // RenamedFiles returns the new name of files that have been renamed by a step. func (r StepResult) RenamedFiles() []string { - if r.Files != nil { - return r.Files.Renamed + if r.files != nil { + return r.files.Renamed } return []string{} } diff --git a/internal/campaigns/run_steps_test.go b/internal/campaigns/run_steps_test.go index 52fc51db99..5beeb28d30 100644 --- a/internal/campaigns/run_steps_test.go +++ b/internal/campaigns/run_steps_test.go @@ -36,7 +36,7 @@ R README.md -> README.markdown func TestParsingAndRenderingTemplates(t *testing.T) { stepCtx := &StepContext{ PreviousStep: StepResult{ - Files: &StepChanges{ + files: &StepChanges{ Modified: []string{"go.mod"}, Added: []string{"main.go.swp"}, Deleted: []string{".DS_Store"}, From 71559c9fba6c83df13b8c1cba288d71e4506f4e2 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 16:12:04 -0800 Subject: [PATCH 39/52] Remove Prepare from Workspace interface. --- internal/campaigns/bind_workspace.go | 69 ++++++++----- internal/campaigns/bind_workspace_test.go | 5 + internal/campaigns/run_steps.go | 8 +- internal/campaigns/volume_workspace.go | 107 ++++++++++++-------- internal/campaigns/volume_workspace_test.go | 79 +++++++-------- internal/campaigns/workspace.go | 5 - 6 files changed, 151 insertions(+), 122 deletions(-) diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index 5434cb2ad7..c6a516212f 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -26,11 +26,30 @@ type dockerBindWorkspaceCreator struct { var _ WorkspaceCreator = &dockerBindWorkspaceCreator{} func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { + path, err := wc.downloadRepoZip(ctx, repo) + if err != nil { + return nil, errors.Wrap(err, "downloading repo zip file") + } + if wc.deleteZips { + defer os.Remove(path) + } + + w, err := wc.unzipToWorkspace(ctx, repo, path) + if err != nil { + return nil, errors.Wrap(err, "unzipping the repository") + } + + return w, errors.Wrap(wc.prepareGitRepo(ctx, w), "preparing local git repo") +} + +func (*dockerBindWorkspaceCreator) DockerImages() []string { return []string{} } + +func (wc *dockerBindWorkspaceCreator) downloadRepoZip(ctx context.Context, repo *graphql.Repository) (string, error) { path := localRepositoryZipArchivePath(wc.dir, repo) exists, err := fileExists(path) if err != nil { - return nil, err + return "", err } if !exists { @@ -39,26 +58,42 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. // Since it's never mounted into the containers being run, we can keep // these directories 0700 without issue. if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { - return nil, err + return "", err } err = fetchRepositoryArchive(ctx, wc.client, repo, path) if err != nil { - // If the context got cancelled, or we ran out of disk space, or - // ... while we were downloading the file, we remove the partially + // If the context got cancelled, or we ran out of disk space, or ... + // while we were downloading the file, we remove the partially // downloaded file. os.Remove(path) - return nil, errors.Wrap(err, "fetching ZIP archive") + return "", errors.Wrap(err, "fetching ZIP archive") } } - if wc.deleteZips { - defer os.Remove(path) + return path, nil +} + +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") + } + + // --force because we want previously "gitignored" files in the repository + if _, err := runGitCmd(ctx, w.dir, "add", "--force", "--all"); err != nil { + return errors.Wrap(err, "git add failed") + } + if _, err := runGitCmd(ctx, w.dir, "commit", "--quiet", "--all", "--allow-empty", "-m", "src-action-exec"); err != nil { + return errors.Wrap(err, "git commit failed") } + return nil +} + +func (wc *dockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo *graphql.Repository, zip string) (*dockerBindWorkspace, error) { prefix := "workspace-" + repo.Slug() - workspace, err := unzipToTempDir(ctx, path, wc.dir, prefix) + workspace, err := unzipToTempDir(ctx, zip, wc.dir, prefix) if err != nil { return nil, errors.Wrap(err, "unzipping the ZIP archive") } @@ -66,8 +101,6 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. return &dockerBindWorkspace{dir: workspace}, nil } -func (*dockerBindWorkspaceCreator) DockerImages() []string { return []string{} } - type dockerBindWorkspace struct { dir string } @@ -87,22 +120,6 @@ func (w *dockerBindWorkspace) DockerRunOpts(ctx context.Context, target string) func (w *dockerBindWorkspace) WorkDir() *string { return &w.dir } -func (w *dockerBindWorkspace) Prepare(ctx context.Context) error { - if _, err := runGitCmd(ctx, w.dir, "init"); err != nil { - return errors.Wrap(err, "git init failed") - } - - // --force because we want previously "gitignored" files in the repository - if _, err := runGitCmd(ctx, w.dir, "add", "--force", "--all"); err != nil { - return errors.Wrap(err, "git add failed") - } - if _, err := runGitCmd(ctx, w.dir, "commit", "--quiet", "--all", "-m", "src-action-exec"); err != nil { - return errors.Wrap(err, "git commit failed") - } - - return nil -} - func (w *dockerBindWorkspace) Changes(ctx context.Context) (*StepChanges, error) { if _, err := runGitCmd(ctx, w.dir, "add", "--all"); err != nil { return nil, errors.Wrap(err, "git add failed") diff --git a/internal/campaigns/bind_workspace_test.go b/internal/campaigns/bind_workspace_test.go index a9cf4a7893..ce9a777c1e 100644 --- a/internal/campaigns/bind_workspace_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -350,6 +351,10 @@ func readWorkspaceFiles(workspace Workspace) (map[string]string, error) { return err } + if strings.HasPrefix(rel, ".git") { + return nil + } + files[rel] = string(content) return nil }) diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index c9c803c6ef..44a0512ab6 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -18,19 +18,13 @@ import ( ) func runSteps(ctx context.Context, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { - reportProgress("Downloading archive") - + reportProgress("Downloading archive and initializing workspace") workspace, err := wc.Create(ctx, repo) if err != nil { return nil, errors.Wrap(err, "creating workspace") } defer workspace.Close(ctx) - reportProgress("Initializing workspace") - if err := workspace.Prepare(ctx); err != nil { - return nil, errors.Wrap(err, "initializing workspace") - } - results := make([]StepResult, len(steps)) for i, step := range steps { diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index 9b08a997ae..a20eca1306 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -22,45 +22,75 @@ var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { w := &dockerVolumeWorkspace{} - return w, w.init(ctx, wc.client, repo) + + zip, err := wc.downloadRepoZip(ctx, repo) + if err != nil { + return nil, errors.Wrap(err, "downloading repo zip file") + } + defer os.Remove(zip) + + w.volume, err = wc.createVolume(ctx) + if err != nil { + return nil, errors.Wrap(err, "creating Docker volume") + } + + if err := wc.unzipRepoIntoVolume(ctx, w, zip); err != nil { + return nil, errors.Wrap(err, "unzipping repo into workspace") + } + + return w, errors.Wrap(wc.prepareGitRepo(ctx, w), "preparing local git repo") } func (*dockerVolumeWorkspaceCreator) DockerImages() []string { return []string{dockerWorkspaceImage} } -// dockerVolumeWorkspace workspaces are placed on Docker volumes (surprise!), -// and are therefore transparent to the host filesystem. This has performance -// advantages if bind mounts are slow, such as on Docker for Mac, but could make -// debugging harder and is slower when it's time to actually retrieve the diff. -type dockerVolumeWorkspace struct { - volume string -} +func (*dockerVolumeWorkspaceCreator) createVolume(ctx context.Context) (string, error) { + out, err := exec.CommandContext(ctx, "docker", "volume", "create").CombinedOutput() + if err != nil { + return "", err + } -var _ Workspace = &dockerVolumeWorkspace{} + return string(bytes.TrimSpace(out)), nil +} -func (w *dockerVolumeWorkspace) init(ctx context.Context, client api.Client, repo *graphql.Repository) error { - // Download the ZIP archive to a temporary file. +func (wc *dockerVolumeWorkspaceCreator) downloadRepoZip(ctx context.Context, repo *graphql.Repository) (string, error) { f, err := ioutil.TempFile(os.TempDir(), "src-archive-*.zip") if err != nil { - return errors.Wrap(err, "creating temporary archive") + return "", errors.Wrap(err, "creating temporary archive") } hostZip := f.Name() - defer os.Remove(hostZip) + f.Close() - if err := fetchRepositoryArchive(ctx, client, repo, hostZip); err != nil { - return errors.Wrap(err, "fetching repository archive") + if err := fetchRepositoryArchive(ctx, wc.client, repo, hostZip); err != nil { + os.Remove(hostZip) + return "", errors.Wrap(err, "fetching repository archive") } - // Create a Docker volume. - out, err := exec.CommandContext(ctx, "docker", "volume", "create").CombinedOutput() - if err != nil { - return errors.Wrap(err, "creating Docker volume") + return hostZip, nil +} + +func (*dockerVolumeWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerVolumeWorkspace) error { + script := `#!/bin/sh + +set -e +set -x + +git init +# --force because we want previously "gitignored" files in the repository +git add --force --all +git commit --quiet --all --allow-empty -m src-action-exec +` + + if _, err := w.runScript(ctx, "/work", script); err != nil { + return errors.Wrap(err, "preparing workspace") } - w.volume = string(bytes.TrimSpace(out)) + return nil +} - // Now mount that temporary file into a Docker container, and unzip it into - // the volume. +func (*dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error { + // We want to mount that temporary file into a Docker container that has the + // workspace volume attached, and unzip it into the volume. common, err := w.DockerRunOpts(ctx, "/work") if err != nil { return errors.Wrap(err, "generating run options") @@ -71,18 +101,27 @@ func (w *dockerVolumeWorkspace) init(ctx context.Context, client api.Client, rep "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=" + hostZip + ",target=/tmp/zip,ro", + "--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro", }, common...) opts = append(opts, dockerWorkspaceImage, "unzip", "/tmp/zip") - out, err = exec.CommandContext(ctx, "docker", opts...).CombinedOutput() - if err != nil { + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) } return nil } +// dockerVolumeWorkspace workspaces are placed on Docker volumes (surprise!), +// and are therefore transparent to the host filesystem. This has performance +// advantages if bind mounts are slow, such as on Docker for Mac, but could make +// debugging harder and is slower when it's time to actually retrieve the diff. +type dockerVolumeWorkspace struct { + volume string +} + +var _ Workspace = &dockerVolumeWorkspace{} + func (w *dockerVolumeWorkspace) Close(ctx context.Context) error { // Cleanup here is easy: we just get rid of the Docker volume. return exec.CommandContext(ctx, "docker", "volume", "rm", w.volume).Run() @@ -96,24 +135,6 @@ func (w *dockerVolumeWorkspace) DockerRunOpts(ctx context.Context, target string func (w *dockerVolumeWorkspace) WorkDir() *string { return nil } -func (w *dockerVolumeWorkspace) Prepare(ctx context.Context) error { - script := `#!/bin/sh - -set -e -set -x - -git init -# --force because we want previously "gitignored" files in the repository -git add --force --all -git commit --quiet --all -m src-action-exec -` - - if _, err := w.runScript(ctx, "/work", script); err != nil { - return errors.Wrap(err, "preparing workspace") - } - return nil -} - func (w *dockerVolumeWorkspace) Changes(ctx context.Context) (*StepChanges, error) { script := `#!/bin/sh diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 29b72f6d6b..801e8756af 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -50,6 +50,14 @@ func TestVolumeWorkspaceCreator(t *testing.T) { dockerWorkspaceImage, "unzip", "/tmp/zip", ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), ) if w, err := wc.Create(ctx, repo); err != nil { @@ -94,6 +102,36 @@ func TestVolumeWorkspaceCreator(t *testing.T) { t.Error("unexpected nil error") } }) + + t.Run("git init failure", func(t *testing.T) { + expect.Commands( + t, + expect.NewGlob( + expect.Behaviour{Stdout: []byte(volumeID)}, + "docker", "volume", "create", + ), + expect.NewGlob( + expect.Behaviour{}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/zip,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "unzip", "/tmp/zip", + ), + expect.NewGlob( + expect.Behaviour{ExitCode: 1}, + "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/run.sh,ro", + "--mount", "type=volume,source="+volumeID+",target=/work", + dockerWorkspaceImage, + "sh", "/run.sh", + ), + ) + + if _, err := wc.Create(ctx, repo); err == nil { + t.Error("unexpected nil error") + } + }) } func TestVolumeWorkspace_Close(t *testing.T) { @@ -156,47 +194,6 @@ func TestVolumeWorkspace_WorkDir(t *testing.T) { // the bottom of runScript itself, rather than depending on script content that // may drift over time. -func TestVolumeWorkspace_Prepare(t *testing.T) { - ctx := context.Background() - w := &dockerVolumeWorkspace{volume: volumeID} - - t.Run("success", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/run.sh,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerWorkspaceImage, - "sh", "/run.sh", - ), - ) - - if err := w.Prepare(ctx); err != nil { - t.Errorf("unexpected error: %v", err) - } - }) - - t.Run("failure", func(t *testing.T) { - expect.Commands( - t, - expect.NewGlob( - expect.Behaviour{ExitCode: 1}, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--mount", "type=bind,source=*,target=/run.sh,ro", - "--mount", "type=volume,source="+volumeID+",target=/work", - dockerWorkspaceImage, - "sh", "/run.sh", - ), - ) - - if err := w.Prepare(ctx); err == nil { - t.Error("unexpected nil error") - } - }) -} - func TestVolumeWorkspace_Changes(t *testing.T) { ctx := context.Background() w := &dockerVolumeWorkspace{volume: volumeID} diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index 52c442a549..c7414a3f23 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -28,11 +28,6 @@ type WorkspaceCreator interface { // Workspace implementations manage per-changeset storage when executing // campaign step. type Workspace interface { - // Prepare is called once, immediately before the first step is executed. - // Generally, this would perform whatever Git or other VCS setup is required - // to establish a base upon which to calculate changes later. - Prepare(ctx context.Context) error - // DockerRunOpts provides the options that should be given to `docker run` // in order to use this workspace. Generally, this will be a set of mount // options. From 155e2130995420c726d619c69fb3eb470f310e3c Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 17:12:16 -0800 Subject: [PATCH 40/52] Unify repo fetching into a single type. --- cmd/src/campaigns_common.go | 13 +- internal/campaigns/bind_workspace.go | 49 +---- internal/campaigns/bind_workspace_test.go | 165 ++++------------ internal/campaigns/executor.go | 4 +- internal/campaigns/executor_test.go | 10 +- internal/campaigns/repo_fetcher.go | 80 ++++++++ internal/campaigns/repo_fetcher_test.go | 200 ++++++++++++++++++++ internal/campaigns/run_steps.go | 13 +- internal/campaigns/service.go | 15 +- internal/campaigns/volume_workspace.go | 36 +--- internal/campaigns/volume_workspace_test.go | 22 ++- internal/campaigns/workspace.go | 5 +- 12 files changed, 374 insertions(+), 238 deletions(-) create mode 100644 internal/campaigns/repo_fetcher.go create mode 100644 internal/campaigns/repo_fetcher_test.go diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index e6051bd544..131c8bbe39 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -191,12 +191,13 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se } opts := campaigns.ExecutorOpts{ - Cache: svc.NewExecutionCache(flags.cacheDir), - Creator: svc.NewWorkspaceCreator(flags.cacheDir, flags.cleanArchives), - ClearCache: flags.clearCache, - KeepLogs: flags.keepLogs, - Timeout: flags.timeout, - TempDir: flags.tempDir, + Cache: svc.NewExecutionCache(flags.cacheDir), + Creator: svc.NewWorkspaceCreator(flags.cacheDir), + RepoFetcher: svc.NewRepoFetcher(flags.cacheDir, flags.cleanArchives), + ClearCache: flags.clearCache, + KeepLogs: flags.keepLogs, + Timeout: flags.timeout, + TempDir: flags.tempDir, } if flags.parallelism <= 0 { opts.Parallelism = runtime.GOMAXPROCS(0) diff --git a/internal/campaigns/bind_workspace.go b/internal/campaigns/bind_workspace.go index c6a516212f..9e978037bf 100644 --- a/internal/campaigns/bind_workspace.go +++ b/internal/campaigns/bind_workspace.go @@ -12,29 +12,17 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" - "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) type dockerBindWorkspaceCreator struct { - dir string - client api.Client - - deleteZips bool + dir string } var _ WorkspaceCreator = &dockerBindWorkspaceCreator{} -func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { - path, err := wc.downloadRepoZip(ctx, repo) - if err != nil { - return nil, errors.Wrap(err, "downloading repo zip file") - } - if wc.deleteZips { - defer os.Remove(path) - } - - w, err := wc.unzipToWorkspace(ctx, repo, path) +func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) { + w, err := wc.unzipToWorkspace(ctx, repo, zip) if err != nil { return nil, errors.Wrap(err, "unzipping the repository") } @@ -44,37 +32,6 @@ func (wc *dockerBindWorkspaceCreator) Create(ctx context.Context, repo *graphql. func (*dockerBindWorkspaceCreator) DockerImages() []string { return []string{} } -func (wc *dockerBindWorkspaceCreator) downloadRepoZip(ctx context.Context, repo *graphql.Repository) (string, error) { - path := localRepositoryZipArchivePath(wc.dir, repo) - - exists, err := fileExists(path) - if err != nil { - return "", err - } - - if !exists { - // Unlike the mkdirAll() calls elsewhere in this file, this is only - // giving us a temporary place on the filesystem to keep the archive. - // Since it's never mounted into the containers being run, we can keep - // these directories 0700 without issue. - if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { - return "", err - } - - err = fetchRepositoryArchive(ctx, wc.client, repo, path) - if err != nil { - // If the context got cancelled, or we ran out of disk space, or ... - // while we were downloading the file, we remove the partially - // downloaded file. - os.Remove(path) - - return "", errors.Wrap(err, "fetching ZIP archive") - } - } - - return path, nil -} - 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") diff --git a/internal/campaigns/bind_workspace_test.go b/internal/campaigns/bind_workspace_test.go index ce9a777c1e..2ee90e7a38 100644 --- a/internal/campaigns/bind_workspace_test.go +++ b/internal/campaigns/bind_workspace_test.go @@ -1,19 +1,15 @@ package campaigns import ( - "bytes" + "archive/zip" "context" "io/ioutil" - "net/http" - "net/http/httptest" "os" "path/filepath" "strings" "testing" - "time" "github.com/google/go-cmp/cmp" - "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) @@ -41,35 +37,36 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { }, } - t.Run("success", func(t *testing.T) { - requestsReceived := 0 - callback := func(_ http.ResponseWriter, _ *http.Request) { - requestsReceived += 1 - } - - ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) - defer ts.Close() + // Create a zip file for all the other tests to use. + f, err := ioutil.TempFile(workspaceTmpDir(t), "repo-zip-*") + if err != nil { + t.Fatal(err) + } + archivePath := f.Name() + t.Cleanup(func() { os.Remove(archivePath) }) - var clientBuffer bytes.Buffer - client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + zw := zip.NewWriter(f) + for name, body := range archive.files { + f, err := zw.Create(name) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Fatal(err) + } + } + zw.Close() + f.Close() + t.Run("success", func(t *testing.T) { testTempDir := workspaceTmpDir(t) - creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} - workspace, err := creator.Create(context.Background(), repo) + creator := &dockerBindWorkspaceCreator{dir: testTempDir} + workspace, err := creator.Create(context.Background(), repo, archivePath) if err != nil { t.Fatalf("unexpected error: %s", err) } - wantZipFile := "github.com-sourcegraph-src-cli-d34db33f.zip" - ok, err := dirContains(creator.dir, wantZipFile) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatalf("temp dir doesnt contain zip file") - } - haveUnzippedFiles, err := readWorkspaceFiles(workspace) if err != nil { t.Fatalf("error walking workspace: %s", err) @@ -78,121 +75,23 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) { if !cmp.Equal(archive.files, haveUnzippedFiles) { t.Fatalf("wrong files in workspace:\n%s", cmp.Diff(archive.files, haveUnzippedFiles)) } - - // Create it a second time and make sure that the server wasn't called - _, err = creator.Create(context.Background(), repo) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - - if requestsReceived != 1 { - t.Fatalf("wrong number of requests received: %d", requestsReceived) - } - - // Third time, but this time with cleanup, _after_ unzipping - creator.deleteZips = true - _, err = creator.Create(context.Background(), repo) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - - if requestsReceived != 1 { - t.Fatalf("wrong number of requests received: %d", requestsReceived) - } - - ok, err = dirContains(creator.dir, wantZipFile) - if err != nil { - t.Fatal(err) - } - if ok { - t.Fatalf("temp dir contains zip file but should not") - } }) - t.Run("canceled", func(t *testing.T) { - // We create a context that is canceled after the server sent down the - // first file to simulate a slow download that's aborted by the user hitting Ctrl-C. - - firstFileWritten := make(chan struct{}) - callback := func(w http.ResponseWriter, r *http.Request) { - // We flush the headers and the first file - w.(http.Flusher).Flush() - - // Wait a bit for the client to start writing the file - time.Sleep(50 * time.Millisecond) - - // Cancel the context to simulate the Ctrl-C - firstFileWritten <- struct{}{} - - <-r.Context().Done() - } - - ctx, cancel := context.WithCancel(context.Background()) - go func() { - <-firstFileWritten - cancel() - }() - - ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) - defer ts.Close() - - var clientBuffer bytes.Buffer - client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) - + t.Run("failure", func(t *testing.T) { testTempDir := workspaceTmpDir(t) - creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} - - _, err := creator.Create(ctx, repo) - if err == nil { - t.Fatalf("error is nil") - } - - zipFile := "github.com-sourcegraph-src-cli-d34db33f.zip" - ok, err := dirContains(creator.dir, zipFile) + // Create an empty file (which is therefore a bad zip file). + badZip, err := ioutil.TempFile(testTempDir, "bad-zip-*") if err != nil { t.Fatal(err) } - if ok { - t.Fatalf("zip file in temp dir was not cleaned up") - } - }) - - t.Run("non-default branch", func(t *testing.T) { - otherBranchOID := "f00b4r" - repo := &graphql.Repository{ - ID: "src-cli-with-non-main-branch", - Name: "github.com/sourcegraph/src-cli", - DefaultBranch: &graphql.Branch{Name: "main", Target: graphql.Target{OID: "d34db33f"}}, - - Commit: graphql.Target{OID: otherBranchOID}, - Branch: graphql.Branch{Name: "other-branch", Target: graphql.Target{OID: otherBranchOID}}, - } - - archive := mockRepoArchive{repo: repo, files: map[string]string{}} - - ts := httptest.NewServer(newZipArchivesMux(t, nil, archive)) - defer ts.Close() - - var clientBuffer bytes.Buffer - client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) - - testTempDir := workspaceTmpDir(t) - - creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} - - _, err := creator.Create(context.Background(), repo) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } + badZipFile := badZip.Name() + t.Cleanup(func() { os.Remove(badZipFile) }) + badZip.Close() - wantZipFile := "github.com-sourcegraph-src-cli-" + otherBranchOID + ".zip" - ok, err := dirContains(creator.dir, wantZipFile) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatalf("temp dir doesnt contain zip file") + creator := &dockerBindWorkspaceCreator{dir: testTempDir} + if _, err := creator.Create(context.Background(), repo, badZipFile); err == nil { + t.Error("unexpected nil error") } }) } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index ea05995863..c41dec49a0 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -144,6 +144,7 @@ type executor struct { features featureFlags logger *LogManager creator WorkspaceCreator + fetcher RepoFetcher tasks []*Task statuses map[*Task]*TaskStatus @@ -165,6 +166,7 @@ func newExecutor(opts ExecutorOpts, client api.Client, features featureFlags) *e creator: opts.Creator, client: client, features: features, + fetcher: opts.RepoFetcher, doneEnqueuing: make(chan struct{}), logger: NewLogManager(opts.TempDir, opts.KeepLogs), tempDir: opts.TempDir, @@ -315,7 +317,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { defer cancel() // Actually execute the steps. - diff, err := runSteps(runCtx, x.creator, task.Repository, task.Steps, log, x.tempDir, func(currentlyExecuting string) { + diff, err := runSteps(runCtx, x.fetcher, x.creator, task.Repository, task.Steps, log, x.tempDir, func(currentlyExecuting string) { x.updateTaskStatus(task, func(status *TaskStatus) { status.CurrentlyExecuting = currentlyExecuting }) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 25cc385df3..e280cebfaf 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -213,10 +213,14 @@ func TestExecutor_Integration(t *testing.T) { defer os.Remove(testTempDir) cache := newInMemoryExecutionCache() - creator := &dockerBindWorkspaceCreator{dir: testTempDir, client: client} + creator := &dockerBindWorkspaceCreator{dir: testTempDir} opts := ExecutorOpts{ - Cache: cache, - Creator: creator, + Cache: cache, + Creator: creator, + RepoFetcher: &repoFetcher{ + client: client, + dir: testTempDir, + }, TempDir: testTempDir, Parallelism: runtime.GOMAXPROCS(0), Timeout: tc.executorTimeout, diff --git a/internal/campaigns/repo_fetcher.go b/internal/campaigns/repo_fetcher.go new file mode 100644 index 0000000000..c8c39093b8 --- /dev/null +++ b/internal/campaigns/repo_fetcher.go @@ -0,0 +1,80 @@ +package campaigns + +import ( + "context" + "os" + "path/filepath" + + "github.com/pkg/errors" + "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" +) + +type RepoFetcher interface { + Fetch(context.Context, *graphql.Repository) (RepoZip, error) +} + +type repoFetcher struct { + client api.Client + dir string + deleteZips bool +} + +var _ RepoFetcher = &repoFetcher{} + +type RepoZip interface { + Close() error + Path() string +} + +type repoZip struct { + path string + fetcher *repoFetcher +} + +var _ RepoZip = &repoZip{} + +func (rf *repoFetcher) Fetch(ctx context.Context, repo *graphql.Repository) (RepoZip, error) { + path := localRepositoryZipArchivePath(rf.dir, repo) + + exists, err := fileExists(path) + if err != nil { + return nil, err + } + + if !exists { + // Unlike the mkdirAll() calls elsewhere in this file, this is only + // giving us a temporary place on the filesystem to keep the archive. + // Since it's never mounted into the containers being run, we can keep + // these directories 0700 without issue. + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { + return nil, err + } + + err = fetchRepositoryArchive(ctx, rf.client, repo, path) + if err != nil { + // If the context got cancelled, or we ran out of disk space, or ... + // while we were downloading the file, we remove the partially + // downloaded file. + os.Remove(path) + + return nil, errors.Wrap(err, "fetching ZIP archive") + } + } + + return &repoZip{ + path: path, + fetcher: rf, + }, nil +} + +func (rz *repoZip) Close() error { + if rz.fetcher.deleteZips { + return os.Remove(rz.path) + } + return nil +} + +func (rz *repoZip) Path() string { + return rz.path +} diff --git a/internal/campaigns/repo_fetcher_test.go b/internal/campaigns/repo_fetcher_test.go new file mode 100644 index 0000000000..f3eb2aabf1 --- /dev/null +++ b/internal/campaigns/repo_fetcher_test.go @@ -0,0 +1,200 @@ +package campaigns + +import ( + "bytes" + "context" + "io/ioutil" + "net/http" + "net/http/httptest" + "os" + "path" + "testing" + "time" + + "github.com/sourcegraph/src-cli/internal/api" + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" +) + +func TestRepoFetcher_Fetch(t *testing.T) { + workspaceTmpDir := func(t *testing.T) string { + testTempDir, err := ioutil.TempDir("", "executor-integration-test-*") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Remove(testTempDir) }) + + return testTempDir + } + + repo := &graphql.Repository{ + ID: "src-cli", + Name: "github.com/sourcegraph/src-cli", + DefaultBranch: &graphql.Branch{Name: "main", Target: graphql.Target{OID: "d34db33f"}}, + } + + archive := mockRepoArchive{ + repo: repo, + files: map[string]string{ + "README.md": "# Welcome to the README\n", + }, + } + + t.Run("success", func(t *testing.T) { + requestsReceived := 0 + callback := func(_ http.ResponseWriter, _ *http.Request) { + requestsReceived++ + } + + ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) + defer ts.Close() + + var clientBuffer bytes.Buffer + client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + + rf := &repoFetcher{ + client: client, + dir: workspaceTmpDir(t), + deleteZips: false, + } + + rz, err := rf.Fetch(context.Background(), repo) + if err != nil { + t.Errorf("unexpected error: %s", err) + } + + wantZipFile := "github.com-sourcegraph-src-cli-d34db33f.zip" + ok, err := dirContains(rf.dir, wantZipFile) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatalf("temp dir doesnt contain zip file") + } + + if have, want := rz.Path(), path.Join(rf.dir, wantZipFile); want != have { + t.Errorf("unexpected path: have=%q want=%q", have, want) + } + rz.Close() + + // Create it a second time and make sure that the server wasn't called + rz, err = rf.Fetch(context.Background(), repo) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + rz.Close() + + if requestsReceived != 1 { + t.Fatalf("wrong number of requests received: %d", requestsReceived) + } + + // Third time, but this time with cleanup, _after_ unzipping + rf.deleteZips = true + _, err = rf.Fetch(context.Background(), repo) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if requestsReceived != 1 { + t.Fatalf("wrong number of requests received: %d", requestsReceived) + } + rz.Close() + + ok, err = dirContains(rf.dir, wantZipFile) + if err != nil { + t.Fatal(err) + } + if ok { + t.Fatalf("temp dir contains zip file but should not") + } + }) + + t.Run("cancelled", func(t *testing.T) { + // We create a context that is canceled after the server sent down the + // first file to simulate a slow download that's aborted by the user hitting Ctrl-C. + + firstFileWritten := make(chan struct{}) + callback := func(w http.ResponseWriter, r *http.Request) { + // We flush the headers and the first file + w.(http.Flusher).Flush() + + // Wait a bit for the client to start writing the file + time.Sleep(50 * time.Millisecond) + + // Cancel the context to simulate the Ctrl-C + firstFileWritten <- struct{}{} + + <-r.Context().Done() + } + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + <-firstFileWritten + cancel() + }() + + ts := httptest.NewServer(newZipArchivesMux(t, callback, archive)) + defer ts.Close() + + var clientBuffer bytes.Buffer + client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + + rf := &repoFetcher{ + client: client, + dir: workspaceTmpDir(t), + deleteZips: false, + } + + if _, err := rf.Fetch(ctx, repo); err == nil { + t.Error("error is nil") + } + + zipFile := "github.com-sourcegraph-src-cli-d34db33f.zip" + ok, err := dirContains(rf.dir, zipFile) + if err != nil { + t.Error(err) + } + if ok { + t.Errorf("zip file in temp dir was not cleaned up") + } + }) + + t.Run("non-default branch", func(t *testing.T) { + otherBranchOID := "f00b4r" + repo := &graphql.Repository{ + ID: "src-cli-with-non-main-branch", + Name: "github.com/sourcegraph/src-cli", + DefaultBranch: &graphql.Branch{Name: "main", Target: graphql.Target{OID: "d34db33f"}}, + + Commit: graphql.Target{OID: otherBranchOID}, + Branch: graphql.Branch{Name: "other-branch", Target: graphql.Target{OID: otherBranchOID}}, + } + + archive := mockRepoArchive{repo: repo, files: map[string]string{}} + + ts := httptest.NewServer(newZipArchivesMux(t, nil, archive)) + defer ts.Close() + + var clientBuffer bytes.Buffer + client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + + rf := &repoFetcher{ + client: client, + dir: workspaceTmpDir(t), + deleteZips: false, + } + + _, err := rf.Fetch(context.Background(), repo) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + wantZipFile := "github.com-sourcegraph-src-cli-" + otherBranchOID + ".zip" + ok, err := dirContains(rf.dir, wantZipFile) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatalf("temp dir doesnt contain zip file") + } + }) +} diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 44a0512ab6..4e18e84b09 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -17,9 +17,16 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -func runSteps(ctx context.Context, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { - reportProgress("Downloading archive and initializing workspace") - workspace, err := wc.Create(ctx, repo) +func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { + reportProgress("Downloading archive") + zip, err := rf.Fetch(ctx, repo) + if err != nil { + return nil, errors.Wrap(err, "fetching repo") + } + defer zip.Close() + + reportProgress("Initializing workspace") + workspace, err := wc.Create(ctx, repo, zip.Path()) if err != nil { return nil, errors.Wrap(err, "creating workspace") } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 064f5a32a1..0514f59f99 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -190,6 +190,7 @@ type ExecutorOpts struct { Cache ExecutionCache Creator WorkspaceCreator Parallelism int + RepoFetcher RepoFetcher Timeout time.Duration ClearCache bool @@ -202,11 +203,19 @@ func (svc *Service) NewExecutor(opts ExecutorOpts) Executor { return newExecutor(opts, svc.client, svc.features) } -func (svc *Service) NewWorkspaceCreator(dir string, cleanArchives bool) WorkspaceCreator { +func (svc *Service) NewRepoFetcher(dir string, cleanArchives bool) RepoFetcher { + return &repoFetcher{ + client: svc.client, + dir: dir, + deleteZips: cleanArchives, + } +} + +func (svc *Service) NewWorkspaceCreator(dir string) WorkspaceCreator { if svc.workspace == "volume" { - return &dockerVolumeWorkspaceCreator{client: svc.client} + return &dockerVolumeWorkspaceCreator{} } - return &dockerBindWorkspaceCreator{dir: dir, client: svc.client, deleteZips: cleanArchives} + return &dockerBindWorkspaceCreator{dir: dir} } // dockerImageSet represents a set of Docker images that need to be pulled. The diff --git a/internal/campaigns/volume_workspace.go b/internal/campaigns/volume_workspace.go index a20eca1306..70ef6fa518 100644 --- a/internal/campaigns/volume_workspace.go +++ b/internal/campaigns/volume_workspace.go @@ -8,32 +8,22 @@ import ( "github.com/pkg/errors" - "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" "github.com/sourcegraph/src-cli/internal/exec" ) -// dockerVolumeWorkspaceCreator creates dockerWorkspace instances. -type dockerVolumeWorkspaceCreator struct { - client api.Client -} +// dockerVolumeWorkspaceCreator creates dockerVolumeWorkspace instances. +type dockerVolumeWorkspaceCreator struct{} var _ WorkspaceCreator = &dockerVolumeWorkspaceCreator{} -func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository) (Workspace, error) { - w := &dockerVolumeWorkspace{} - - zip, err := wc.downloadRepoZip(ctx, repo) - if err != nil { - return nil, errors.Wrap(err, "downloading repo zip file") - } - defer os.Remove(zip) - - w.volume, err = wc.createVolume(ctx) +func (wc *dockerVolumeWorkspaceCreator) Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) { + volume, err := wc.createVolume(ctx) if err != nil { return nil, errors.Wrap(err, "creating Docker volume") } + w := &dockerVolumeWorkspace{volume: volume} if err := wc.unzipRepoIntoVolume(ctx, w, zip); err != nil { return nil, errors.Wrap(err, "unzipping repo into workspace") } @@ -54,22 +44,6 @@ func (*dockerVolumeWorkspaceCreator) createVolume(ctx context.Context) (string, return string(bytes.TrimSpace(out)), nil } -func (wc *dockerVolumeWorkspaceCreator) downloadRepoZip(ctx context.Context, repo *graphql.Repository) (string, error) { - f, err := ioutil.TempFile(os.TempDir(), "src-archive-*.zip") - if err != nil { - return "", errors.Wrap(err, "creating temporary archive") - } - hostZip := f.Name() - f.Close() - - if err := fetchRepositoryArchive(ctx, wc.client, repo, hostZip); err != nil { - os.Remove(hostZip) - return "", errors.Wrap(err, "fetching repository archive") - } - - return hostZip, nil -} - func (*dockerVolumeWorkspaceCreator) prepareGitRepo(ctx context.Context, w *dockerVolumeWorkspace) error { script := `#!/bin/sh diff --git a/internal/campaigns/volume_workspace_test.go b/internal/campaigns/volume_workspace_test.go index 801e8756af..3732cb6386 100644 --- a/internal/campaigns/volume_workspace_test.go +++ b/internal/campaigns/volume_workspace_test.go @@ -4,13 +4,13 @@ import ( "bytes" "context" "io/ioutil" + "os" "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" - "github.com/sourcegraph/src-cli/internal/api/mock" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" "github.com/sourcegraph/src-cli/internal/exec/expect" ) @@ -21,13 +21,17 @@ const volumeID = "VOLUME-ID" func TestVolumeWorkspaceCreator(t *testing.T) { ctx := context.Background() - // It doesn't matter what content we return here, since the actual unzip - // command will never be executed anyway. - client, err := mock.ParrotClient(t, []byte("MOO")) + // Create an empty file. It doesn't matter that it's an invalid zip, since + // we're mocking the unzip command anyway. + f, err := ioutil.TempFile(os.TempDir(), "volume-workspace-*") if err != nil { t.Fatal(err) } - wc := &dockerVolumeWorkspaceCreator{client: client} + zip := f.Name() + f.Close() + defer os.Remove(zip) + + wc := &dockerVolumeWorkspaceCreator{} // We'll set up a fake repository with just enough fields defined for init() // and friends. @@ -60,7 +64,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { ), ) - if w, err := wc.Create(ctx, repo); err != nil { + if w, err := wc.Create(ctx, repo, zip); err != nil { t.Errorf("unexpected error: %v", err) } else if have := w.(*dockerVolumeWorkspace).volume; have != volumeID { t.Errorf("unexpected volume: have=%q want=%q", have, volumeID) @@ -76,7 +80,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { ), ) - if _, err := wc.Create(ctx, repo); err == nil { + if _, err := wc.Create(ctx, repo, zip); err == nil { t.Error("unexpected nil error") } }) @@ -98,7 +102,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { ), ) - if _, err := wc.Create(ctx, repo); err == nil { + if _, err := wc.Create(ctx, repo, zip); err == nil { t.Error("unexpected nil error") } }) @@ -128,7 +132,7 @@ func TestVolumeWorkspaceCreator(t *testing.T) { ), ) - if _, err := wc.Create(ctx, repo); err == nil { + if _, err := wc.Create(ctx, repo, zip); err == nil { t.Error("unexpected nil error") } }) diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index c7414a3f23..44bb1e216f 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -16,9 +16,8 @@ import ( // per-changeset persistent storage when executing campaign steps and are // responsible for ultimately generating a diff. type WorkspaceCreator interface { - // Create should clone the given repository, and perform whatever other - // initial setup is required. - Create(context.Context, *graphql.Repository) (Workspace, error) + // Create creates a new workspace for the given repository and ZIP file. + Create(ctx context.Context, repo *graphql.Repository, zip string) (Workspace, error) // DockerImages returns any Docker images required to use workspaces created // by this creator. From 0c2fb66920d431e3256ce687cf7f0a0b8de83f6e Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 17:17:20 -0800 Subject: [PATCH 41/52] Remove unused API mock functionality. --- internal/api/mock/mock.go | 46 --------------------------------------- 1 file changed, 46 deletions(-) delete mode 100644 internal/api/mock/mock.go diff --git a/internal/api/mock/mock.go b/internal/api/mock/mock.go deleted file mode 100644 index 4e835cc0c2..0000000000 --- a/internal/api/mock/mock.go +++ /dev/null @@ -1,46 +0,0 @@ -// Package mock provides mocking capabilities for api.Client instances. -package mock - -import ( - "bytes" - "net" - "net/http" - "testing" - - "github.com/pkg/errors" - - "github.com/sourcegraph/src-cli/internal/api" -) - -// ParrotClient creates a new API client that always receives the given response. -func ParrotClient(t *testing.T, resp []byte) (api.Client, error) { - // We're going to implement this by standing up a HTTP server on a random - // port that always returns the given response. - l, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - return nil, errors.Wrap(err, "listening on random port") - } - - srv := &http.Server{Handler: mockHandler(t, resp)} - go srv.Serve(l) - - var buf bytes.Buffer - t.Cleanup(func() { - srv.Close() - t.Logf("output from API client: %s", buf.String()) - }) - - return api.NewClient(api.ClientOpts{ - Endpoint: "http://" + l.Addr().String(), - Out: &buf, - }), nil -} - -// mockHandler returns a HTTP handler that always returns the given data as its response, and always succeeds. -func mockHandler(t *testing.T, data []byte) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - t.Logf("handling request: %+v", r) - w.WriteHeader(200) - w.Write(data) - }) -} From f1a56669ca230a634346a6f319c4edd3b21870e5 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 17:26:38 -0800 Subject: [PATCH 42/52] Fix Windows compatibility. --- internal/campaigns/repo_fetcher_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/campaigns/repo_fetcher_test.go b/internal/campaigns/repo_fetcher_test.go index f3eb2aabf1..b4eba91b46 100644 --- a/internal/campaigns/repo_fetcher_test.go +++ b/internal/campaigns/repo_fetcher_test.go @@ -71,7 +71,10 @@ func TestRepoFetcher_Fetch(t *testing.T) { t.Fatalf("temp dir doesnt contain zip file") } - if have, want := rz.Path(), path.Join(rf.dir, wantZipFile); want != have { + // We can't use path.Join() here because it hardcodes / as the path + // separator because... well, honestly, I have no idea, but it's bad + // news bears on Windows. + if have, want := rz.Path(), path.Clean(rf.dir)+string(os.PathSeparator)+wantZipFile; want != have { t.Errorf("unexpected path: have=%q want=%q", have, want) } rz.Close() From 9e330c13854f39b887d795349805b959eb2cb104 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 17:57:43 -0800 Subject: [PATCH 43/52] temp ci hackery --- internal/campaigns/executor.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index c41dec49a0..f8c6c16847 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -388,12 +388,14 @@ func (e *errTimeoutReached) Error() string { } func reachedTimeout(cmdCtx context.Context, err error) bool { + fmt.Printf("err: %+v\nerrors.Cause: %T\nerrors.Cause: %+v\ncmdCtx.Err: %+v", err, errors.Cause(err), errors.Cause(err), cmdCtx.Err()) if ee, ok := errors.Cause(err).(*exec.ExitError); ok { if ee.String() == "signal: killed" && cmdCtx.Err() == context.DeadlineExceeded { return true } } + fmt.Printf("errors.Is: %v", errors.Is(err, context.DeadlineExceeded)) return errors.Is(err, context.DeadlineExceeded) } From 17f7c2c2d1976caf8ef3d06ba56109a2216498b6 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 18:02:49 -0800 Subject: [PATCH 44/52] more ci fuckery --- internal/campaigns/executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index f8c6c16847..5049f5c5fc 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -388,14 +388,14 @@ func (e *errTimeoutReached) Error() string { } func reachedTimeout(cmdCtx context.Context, err error) bool { - fmt.Printf("err: %+v\nerrors.Cause: %T\nerrors.Cause: %+v\ncmdCtx.Err: %+v", err, errors.Cause(err), errors.Cause(err), cmdCtx.Err()) + fmt.Printf("err: %+v\nerrors.Cause: %T\nerrors.Cause: %+v\ncmdCtx.Err: %+v\ncontext.DeadlineExceeded: %+v\n", err, errors.Cause(err), errors.Cause(err), cmdCtx.Err(), context.DeadlineExceeded) if ee, ok := errors.Cause(err).(*exec.ExitError); ok { if ee.String() == "signal: killed" && cmdCtx.Err() == context.DeadlineExceeded { return true } } - fmt.Printf("errors.Is: %v", errors.Is(err, context.DeadlineExceeded)) + fmt.Printf("errors.Is: %v\n", errors.Is(err, context.DeadlineExceeded)) return errors.Is(err, context.DeadlineExceeded) } From 4c81744b76c9f16b6246a9430436de86a2b6df8c Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 18:16:37 -0800 Subject: [PATCH 45/52] revert ci fuckery --- internal/campaigns/executor.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index 5049f5c5fc..c41dec49a0 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -388,14 +388,12 @@ func (e *errTimeoutReached) Error() string { } func reachedTimeout(cmdCtx context.Context, err error) bool { - fmt.Printf("err: %+v\nerrors.Cause: %T\nerrors.Cause: %+v\ncmdCtx.Err: %+v\ncontext.DeadlineExceeded: %+v\n", err, errors.Cause(err), errors.Cause(err), cmdCtx.Err(), context.DeadlineExceeded) if ee, ok := errors.Cause(err).(*exec.ExitError); ok { if ee.String() == "signal: killed" && cmdCtx.Err() == context.DeadlineExceeded { return true } } - fmt.Printf("errors.Is: %v\n", errors.Is(err, context.DeadlineExceeded)) return errors.Is(err, context.DeadlineExceeded) } From df126e18f37436247fb201cb507c9da80f58c9bc Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Tue, 5 Jan 2021 18:19:44 -0800 Subject: [PATCH 46/52] Add PR to CHANGELOG. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dea80b0594..dafc6aac1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ All notable changes to `src-cli` are documented in this file. ### Added -- `src campaign [apply|preview] can now make use of Docker volumes, rather than bind-mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. The optional `-workspace` flag can be used to override the default. []() +- `src campaign [apply|preview] can now make use of Docker volumes, rather than bind-mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. The optional `-workspace` flag can be used to override the default. [#412](https://github.com/sourcegraph/src-cli/pull/412) ### Changed From 605b0f1ccd58350d46a16dbb886ed6b25f1244a3 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Wed, 6 Jan 2021 17:38:49 -0800 Subject: [PATCH 47/52] Remove unused dependency. --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index 5f9d3fa639..ebe9209de1 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.13 require ( github.com/Masterminds/semver v1.5.0 - github.com/alessio/shellescape v1.4.1 github.com/dustin/go-humanize v1.0.0 github.com/efritz/pentimento v0.0.0-20190429011147-ade47d831101 github.com/gobwas/glob v0.2.3 diff --git a/go.sum b/go.sum index 2a96b140d6..d569c1db34 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,5 @@ github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww= github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= -github.com/alessio/shellescape v1.4.1 h1:V7yhSDDn8LP4lc4jS8pFkt0zCnzVJlG5JXy9BVKJUX0= -github.com/alessio/shellescape v1.4.1/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPpyFjDCtHLS30= github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= From 7479a24edb3be8b560ddbc4c5af92286826aaaa5 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 7 Jan 2021 13:29:22 -0800 Subject: [PATCH 48/52] Update CHANGELOG.md Co-authored-by: Thorsten Ball --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dafc6aac1f..a579b74e46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ All notable changes to `src-cli` are documented in this file. ### Added -- `src campaign [apply|preview] can now make use of Docker volumes, rather than bind-mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. The optional `-workspace` flag can be used to override the default. [#412](https://github.com/sourcegraph/src-cli/pull/412) +- `src campaign [apply|preview]` can now make use of Docker volumes, rather than bind-mounting the host filesystem. This is now the default on macOS, as volume mounts have generally better performance there. The optional `-workspace` flag can be used to override the default. [#412](https://github.com/sourcegraph/src-cli/pull/412) ### Changed From ffe31dfd1ed3ca445f340cb9b948d7a82be291df Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 7 Jan 2021 13:30:36 -0800 Subject: [PATCH 49/52] Update cmd/src/campaigns_common.go Co-authored-by: Thorsten Ball --- cmd/src/campaigns_common.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 131c8bbe39..64fd032d19 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -101,7 +101,7 @@ func newCampaignsApplyFlags(flagSet *flag.FlagSet, cacheDir, tempDir string) *ca "If true, errors encountered while executing steps in a repository won't stop the execution of the campaign spec but only cause that repository to be skipped.", ) - // We default to bind workspaces on everything except AMD64 macOS at + // We default to bind workspaces on everything except ARM64 macOS at // present. In the future, we'll likely want to switch the default for ARM64 // macOS as well, but this requires access to the hardware for testing. var defaultWorkspace string From b7d550526a5f68e234fe1252b850f00409aa2304 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 7 Jan 2021 13:32:09 -0800 Subject: [PATCH 50/52] Use filepath.Join. --- internal/campaigns/repo_fetcher_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/campaigns/repo_fetcher_test.go b/internal/campaigns/repo_fetcher_test.go index b4eba91b46..587b38913a 100644 --- a/internal/campaigns/repo_fetcher_test.go +++ b/internal/campaigns/repo_fetcher_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "os" "path" + "path/filepath" "testing" "time" @@ -71,10 +72,7 @@ func TestRepoFetcher_Fetch(t *testing.T) { t.Fatalf("temp dir doesnt contain zip file") } - // We can't use path.Join() here because it hardcodes / as the path - // separator because... well, honestly, I have no idea, but it's bad - // news bears on Windows. - if have, want := rz.Path(), path.Clean(rf.dir)+string(os.PathSeparator)+wantZipFile; want != have { + if have, want := rz.Path(), filepath.Join(path.Clean(rf.dir), wantZipFile); want != have { t.Errorf("unexpected path: have=%q want=%q", have, want) } rz.Close() From 203868dd9ce3ecf442638b1bfee906f3916c7472 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 7 Jan 2021 13:38:03 -0800 Subject: [PATCH 51/52] Add documentation. --- internal/campaigns/repo_fetcher.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/campaigns/repo_fetcher.go b/internal/campaigns/repo_fetcher.go index c8c39093b8..3f372739c3 100644 --- a/internal/campaigns/repo_fetcher.go +++ b/internal/campaigns/repo_fetcher.go @@ -10,10 +10,17 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) +// RepoFetcher abstracts the process of retrieving an archive for the given +// repository. type RepoFetcher interface { + // Fetch must retrieve the given repository and return it as a RepoZip. + // This will generally imply that the file should be written to a temporary + // location on the filesystem. Fetch(context.Context, *graphql.Repository) (RepoZip, error) } +// repoFetcher is the concrete implementation of the RepoFetcher interface used +// outside of tests. type repoFetcher struct { client api.Client dir string @@ -22,11 +29,18 @@ type repoFetcher struct { var _ RepoFetcher = &repoFetcher{} +// RepoZip implementations represent a downloaded repository archive. type RepoZip interface { + // Close must finalise the downloaded archive. If one or more temporary + // files were created, they should be deleted here. Close() error + + // Path must return the path to the archive on the filesystem. Path() string } +// repoZip is the concrete implementation of the RepoZip interface used outside +// of tests. type repoZip struct { path string fetcher *repoFetcher From 13b0eed01420244d7bf1b2a65ba4bd00b866d5b0 Mon Sep 17 00:00:00 2001 From: Adam Harvey Date: Thu, 7 Jan 2021 13:39:07 -0800 Subject: [PATCH 52/52] Migrate fetch utility functions to the right place. --- internal/campaigns/repo_fetcher.go | 37 ++++++++++++++++++++++++++ internal/campaigns/workspace.go | 42 ------------------------------ 2 files changed, 37 insertions(+), 42 deletions(-) diff --git a/internal/campaigns/repo_fetcher.go b/internal/campaigns/repo_fetcher.go index 3f372739c3..0427517e05 100644 --- a/internal/campaigns/repo_fetcher.go +++ b/internal/campaigns/repo_fetcher.go @@ -2,7 +2,11 @@ package campaigns import ( "context" + "fmt" + "io" + "net/http" "os" + "path" "path/filepath" "github.com/pkg/errors" @@ -92,3 +96,36 @@ func (rz *repoZip) Close() error { func (rz *repoZip) Path() string { return rz.path } + +func fetchRepositoryArchive(ctx context.Context, client api.Client, repo *graphql.Repository, dest string) error { + req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) + if err != nil { + return err + } + req.Header.Set("Accept", "application/zip") + resp, err := http.DefaultClient.Do(req) + if err != nil { + return err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) + } + + f, err := os.Create(dest) + if err != nil { + return err + } + defer f.Close() + + if _, err := io.Copy(f, resp.Body); err != nil { + return err + } + + return nil +} + +func repositoryZipArchivePath(repo *graphql.Repository) string { + return path.Join("", repo.Name+"@"+repo.BaseRef(), "-", "raw") +} diff --git a/internal/campaigns/workspace.go b/internal/campaigns/workspace.go index 44bb1e216f..45bd9797e1 100644 --- a/internal/campaigns/workspace.go +++ b/internal/campaigns/workspace.go @@ -2,13 +2,7 @@ package campaigns import ( "context" - "fmt" - "io" - "net/http" - "os" - "path" - "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) @@ -50,39 +44,3 @@ type Workspace interface { // multiple times in the life of a workspace. Diff(ctx context.Context) ([]byte, error) } - -// We'll put some useful utility functions below here that tend to be reused -// across workspace implementations. - -func fetchRepositoryArchive(ctx context.Context, client api.Client, repo *graphql.Repository, dest string) error { - req, err := client.NewHTTPRequest(ctx, "GET", repositoryZipArchivePath(repo), nil) - if err != nil { - return err - } - req.Header.Set("Accept", "application/zip") - resp, err := http.DefaultClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("unable to fetch archive (HTTP %d from %s)", resp.StatusCode, req.URL.String()) - } - - f, err := os.Create(dest) - if err != nil { - return err - } - defer f.Close() - - if _, err := io.Copy(f, resp.Body); err != nil { - return err - } - - return nil -} - -func repositoryZipArchivePath(repo *graphql.Repository) string { - return path.Join("", repo.Name+"@"+repo.BaseRef(), "-", "raw") -}