From 569e37da1e069b041de86721c37848dfe1d81097 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 12 Jan 2021 14:21:07 +0100 Subject: [PATCH 01/19] Add templating to changesetTemplate and steps.outputs --- internal/campaigns/campaign_spec.go | 8 ++ internal/campaigns/executor.go | 56 ++++++++-- internal/campaigns/executor_test.go | 81 +++++++++++++- internal/campaigns/run_steps.go | 82 +++++++++----- internal/campaigns/templating.go | 120 ++++++++++++++------ internal/campaigns/templating_test.go | 153 ++++++++++++++------------ schema/campaign_spec.schema.json | 24 ++++ schema/campaign_spec_stringdata.go | 63 +++++++++++ 8 files changed, 446 insertions(+), 141 deletions(-) diff --git a/internal/campaigns/campaign_spec.go b/internal/campaigns/campaign_spec.go index 8d6695cd9b..f5b94ed7ce 100644 --- a/internal/campaigns/campaign_spec.go +++ b/internal/campaigns/campaign_spec.go @@ -71,10 +71,18 @@ type Step struct { Container string `json:"container,omitempty" yaml:"container"` Env env.Environment `json:"env,omitempty" yaml:"env"` Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"` + Outputs Outputs `json:"output,omitempty" yaml:"output,omitempty"` image string } +type Outputs map[string]Output + +type Output struct { + Value string `json:"value,omitempty" yaml:"value,omitempty"` + Format string `json:"format,omitempty" yaml:"format,omitempty"` +} + type TransformChanges struct { Group []Group `json:"group,omitempty" yaml:"group"` } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index c41dec49a0..14d40cbccd 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -177,7 +177,12 @@ func newExecutor(opts ExecutorOpts, client api.Client, features featureFlags) *e } func (x *executor) AddTask(repo *graphql.Repository, steps []Step, transform *TransformChanges, template *ChangesetTemplate) { - task := &Task{repo, steps, template, transform} + task := &Task{ + Repository: repo, + Steps: steps, + Template: template, + TransformChanges: transform, + } x.tasks = append(x.tasks, task) x.statusesMu.Lock() @@ -272,8 +277,10 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { return } + // TODO: We need to cache the vars here too if we want use to render changesetTemplate + vars := map[string]interface{}{} var specs []*ChangesetSpec - specs, err = createChangesetSpecs(task, diff, x.features) + specs, err = createChangesetSpecs(task, diff, vars, x.features) if err != nil { return err } @@ -317,7 +324,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { defer cancel() // Actually execute the steps. - diff, err := runSteps(runCtx, x.fetcher, x.creator, task.Repository, task.Steps, log, x.tempDir, func(currentlyExecuting string) { + diff, outputs, 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 }) @@ -331,7 +338,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } // Build the changeset specs. - specs, err := createChangesetSpecs(task, string(diff), x.features) + specs, err := createChangesetSpecs(task, string(diff), outputs, x.features) if err != nil { return err } @@ -397,7 +404,7 @@ func reachedTimeout(cmdCtx context.Context, err error) bool { return errors.Is(err, context.DeadlineExceeded) } -func createChangesetSpecs(task *Task, completeDiff string, features featureFlags) ([]*ChangesetSpec, error) { +func createChangesetSpecs(task *Task, completeDiff string, outputs map[string]interface{}, features featureFlags) ([]*ChangesetSpec, error) { repo := task.Repository.Name var authorName string @@ -414,6 +421,34 @@ func createChangesetSpecs(task *Task, completeDiff string, features featureFlags authorEmail = task.Template.Commit.Author.Email } + tmplCtx := &ChangesetTemplateContext{ + Outputs: outputs, + Repository: *task.Repository, + } + + title, err := renderChangesetTemplateField("title", task.Template.Title, tmplCtx) + if err != nil { + return nil, err + } + + body, err := renderChangesetTemplateField("body", task.Template.Body, tmplCtx) + if err != nil { + return nil, err + } + + message, err := renderChangesetTemplateField("message", task.Template.Commit.Message, tmplCtx) + if err != nil { + return nil, err + } + + // TODO: As a next step, we should extend the ChangesetTemplateContext to also include + // TransformChanges.Group and then change validateGroups and groupFileDiffs to, for each group, + // render the branch name *before* grouping the diffs. + defaultBranch, err := renderChangesetTemplateField("branch", task.Template.Branch, tmplCtx) + if err != nil { + return nil, err + } + newSpec := func(branch, diff string) *ChangesetSpec { return &ChangesetSpec{ BaseRepository: task.Repository.ID, @@ -422,11 +457,11 @@ func createChangesetSpecs(task *Task, completeDiff string, features featureFlags BaseRev: task.Repository.Rev(), HeadRepository: task.Repository.ID, HeadRef: "refs/heads/" + branch, - Title: task.Template.Title, - Body: task.Template.Body, + Title: title, + Body: body, Commits: []GitCommitDescription{ { - Message: task.Template.Commit.Message, + Message: message, AuthorName: authorName, AuthorEmail: authorEmail, Diff: diff, @@ -446,7 +481,8 @@ func createChangesetSpecs(task *Task, completeDiff string, features featureFlags return specs, err } - diffsByBranch, err := groupFileDiffs(completeDiff, task.Template.Branch, groups) + // TODO: Regarding 'defaultBranch', see comment above + diffsByBranch, err := groupFileDiffs(completeDiff, defaultBranch, groups) if err != nil { return specs, errors.Wrap(err, "grouping diffs failed") } @@ -455,7 +491,7 @@ func createChangesetSpecs(task *Task, completeDiff string, features featureFlags specs = append(specs, newSpec(branch, diff)) } } else { - specs = append(specs, newSpec(task.Template.Branch, string(completeDiff))) + specs = append(specs, newSpec(defaultBranch, string(completeDiff))) } return specs, nil diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index e280cebfaf..8a64b4f9f3 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -61,13 +61,18 @@ func TestExecutor_Integration(t *testing.T) { repos []*graphql.Repository archives []mockRepoArchive + template *ChangesetTemplate steps []Step transform *TransformChanges executorTimeout time.Duration - wantFilesChanged filesByRepository - wantErrInclude string + wantFilesChanged filesByRepository + wantTitle string + wantBody string + wantCommitMessage string + + wantErrInclude string }{ { name: "success", @@ -196,6 +201,57 @@ func TestExecutor_Integration(t *testing.T) { }, }, }, + { + name: "templated changesetTemplate", + repos: []*graphql.Repository{srcCLIRepo}, + archives: []mockRepoArchive{ + {repo: srcCLIRepo, files: map[string]string{ + "README.md": "# Welcome to the README\n", + "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", + }}, + }, + steps: []Step{ + { + Run: `go fmt main.go`, + Container: "doesntmatter:13", + Outputs: Outputs{ + "myOutputName1": Output{ + Value: "${{ index step.modified_files 0 }}", + }, + }, + }, + { + Run: `echo -n "Hello World!"`, + Container: "alpine:13", + Outputs: Outputs{ + "myOutputName2": Output{ + Value: `thisStepStdout: "${{ step.stdout }}"`, + Format: "yaml", + }, + "myOutputName3": Output{ + Value: "cool-suffix", + }, + }, + }, + }, + template: &ChangesetTemplate{ + Title: "myOutputName1=${{ outputs.myOutputName1}}", + Body: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", + Branch: "templated-branch-${{ outputs.myOutputName3 }}", + Commit: ExpandedGitCommitDescription{ + Message: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", + }, + }, + + wantFilesChanged: filesByRepository{ + srcCLIRepo.ID: filesByBranch{ + "templated-branch-cool-suffix": []string{"main.go"}, + }, + }, + wantTitle: "myOutputName1=main.go", + wantBody: "myOutputName1=main.go,myOutputName2=Hello World!", + wantCommitMessage: "myOutputName1=main.go,myOutputName2=Hello World!", + }, } for _, tc := range tests { @@ -236,6 +292,9 @@ func TestExecutor_Integration(t *testing.T) { executor := newExecutor(opts, client, featuresAllEnabled()) template := &ChangesetTemplate{Branch: changesetTemplateBranch} + if tc.template != nil { + template = tc.template + } for _, r := range tc.repos { executor.AddTask(r, tc.steps, tc.transform, template) @@ -261,11 +320,25 @@ func TestExecutor_Integration(t *testing.T) { t.Fatalf("wrong number of changeset specs. want=%d, have=%d", want, have) } + for i, spec := range specs { + t.Logf("specs[%d]=%+v\n", i, spec.CreatedChangeset) + } for _, spec := range specs { + if tc.wantTitle != "" && spec.Title != tc.wantTitle { + t.Errorf("wrong title. want=%q, have=%q", tc.wantTitle, spec.Title) + } + if tc.wantBody != "" && spec.Body != tc.wantBody { + t.Errorf("wrong body. want=%q, have=%q", tc.wantBody, spec.Body) + } + if have, want := len(spec.Commits), 1; have != want { t.Fatalf("wrong number of commits. want=%d, have=%d", want, have) } + if tc.wantCommitMessage != "" && spec.Commits[0].Message != tc.wantCommitMessage { + t.Errorf("wrong commitmessage. want=%q, have=%q", tc.wantCommitMessage, spec.Commits[0].Message) + } + wantFiles, ok := tc.wantFilesChanged[spec.BaseRepository] if !ok { t.Fatalf("unexpected file changes in repo %s", spec.BaseRepository) @@ -329,6 +402,10 @@ func TestExecutor_Integration(t *testing.T) { // Run with a warm cache. t.Run("warm cache", func(t *testing.T) { + if tc.name == "templated changesetTemplate" { + // TODO: Make caching work with steps outputs + t.Skip() + } execute() verifyCache() }) diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 9350dc2efc..7694f92e2d 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -3,6 +3,7 @@ package campaigns import ( "bytes" "context" + "encoding/json" "fmt" "io" "io/ioutil" @@ -14,29 +15,31 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" + "gopkg.in/yaml.v2" ) -func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) ([]byte, error) { +func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) (diff []byte, outputs map[string]interface{}, err error) { reportProgress("Downloading archive") zip, err := rf.Fetch(ctx, repo) if err != nil { - return nil, errors.Wrap(err, "fetching repo") + return nil, 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") + return nil, nil, errors.Wrap(err, "creating workspace") } defer workspace.Close(ctx) results := make([]StepResult, len(steps)) + outputs = make(map[string]interface{}) for i, step := range steps { reportProgress(fmt.Sprintf("Preparing step %d", i+1)) - stepContext := StepContext{Repository: *repo} + stepContext := StepContext{Repository: *repo, Outputs: outputs} if i > 0 { stepContext.PreviousStep = results[i-1] } @@ -46,7 +49,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // container on a successful run, rather than leaving it dangling. cidFile, err := ioutil.TempFile(tempDir, repo.Slug()+"-container-id") if err != nil { - return nil, errors.Wrap(err, "Creating a CID file failed") + return nil, nil, errors.Wrap(err, "Creating a CID file failed") } // However, Docker will fail if the cidfile actually exists, so we need @@ -54,7 +57,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // close it, even though that's unnecessary elsewhere. cidFile.Close() if err = os.Remove(cidFile.Name()); err != nil { - return nil, errors.Wrap(err, "removing cidfile") + return nil, nil, errors.Wrap(err, "removing cidfile") } // Since we went to all that effort, we can now defer a function that @@ -72,14 +75,14 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // For now, we only support shell scripts provided via the Run field. shell, containerTemp, err := probeImageForShell(ctx, step.image) if err != nil { - return nil, errors.Wrapf(err, "probing image %q for shell", step.image) + return nil, nil, errors.Wrapf(err, "probing image %q for shell", step.image) } // Set up a temporary file on the host filesystem to contain the // script. runScriptFile, err := ioutil.TempFile(tempDir, "") if err != nil { - return nil, errors.Wrap(err, "creating temporary file") + return nil, nil, errors.Wrap(err, "creating temporary file") } defer os.Remove(runScriptFile.Name()) @@ -87,12 +90,12 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // temp file we just created. var runScript bytes.Buffer out := io.MultiWriter(&runScript, runScriptFile) - if err := renderTemplate("step-run", step.Run, out, &stepContext); err != nil { - return nil, errors.Wrap(err, "parsing step run") + if err := renderStepTemplate("step-run", step.Run, out, &stepContext); err != nil { + return nil, nil, errors.Wrap(err, "parsing step run") } if err := runScriptFile.Close(); err != nil { - return nil, errors.Wrap(err, "closing temporary file") + return nil, nil, errors.Wrap(err, "closing temporary file") } // This file needs to be readable within the container regardless of the @@ -105,13 +108,13 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // conditionally compiled files here, instead we'll just wait until the // file is closed to twiddle the permission bits. Which is now! if err := os.Chmod(runScriptFile.Name(), 0644); err != nil { - return nil, errors.Wrap(err, "setting permissions on the temporary file") + return nil, nil, errors.Wrap(err, "setting permissions on the temporary file") } // Parse and render the step.Files. - files, err := renderMap(step.Files, &stepContext) + files, err := renderStepMap(step.Files, &stepContext) if err != nil { - return nil, errors.Wrap(err, "parsing step files") + return nil, nil, errors.Wrap(err, "parsing step files") } // Create temp files with the rendered content of step.Files so that we @@ -120,16 +123,16 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr for name, content := range files { fp, err := ioutil.TempFile(tempDir, "") if err != nil { - return nil, errors.Wrap(err, "creating temporary file") + return nil, nil, errors.Wrap(err, "creating temporary file") } defer os.Remove(fp.Name()) if _, err := fp.WriteString(content); err != nil { - return nil, errors.Wrap(err, "writing to temporary file") + return nil, nil, errors.Wrap(err, "writing to temporary file") } if err := fp.Close(); err != nil { - return nil, errors.Wrap(err, "closing temporary file") + return nil, nil, errors.Wrap(err, "closing temporary file") } filesToMount[name] = fp @@ -138,20 +141,20 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // Resolve step.Env given the current environment. stepEnv, err := step.Env.Resolve(os.Environ()) if err != nil { - return nil, errors.Wrap(err, "resolving step environment") + return nil, nil, errors.Wrap(err, "resolving step environment") } // Render the step.Env variables as templates. - env, err := renderMap(stepEnv, &stepContext) + env, err := renderStepMap(stepEnv, &stepContext) if err != nil { - return nil, errors.Wrap(err, "parsing step environment") + return nil, nil, errors.Wrap(err, "parsing step environment") } reportProgress(runScript.String()) const workDir = "/work" workspaceOpts, err := workspace.DockerRunOpts(ctx, workDir) if err != nil { - return nil, errors.Wrap(err, "getting Docker options for workspace") + return nil, nil, errors.Wrap(err, "getting Docker options for workspace") } args := append([]string{ "run", @@ -190,7 +193,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr if err != nil { logger.Logf("[Step %d] took %s; error running Docker container: %+v", i+1, elapsed, err) - return nil, stepFailedErr{ + return nil, nil, stepFailedErr{ Err: err, Args: cmd.Args, Run: runScript.String(), @@ -205,19 +208,46 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr changes, err := workspace.Changes(ctx) if err != nil { - return nil, errors.Wrap(err, "getting changed files in step") + return nil, nil, errors.Wrap(err, "getting changed files in step") } - results[i] = StepResult{files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} + result := StepResult{files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} + stepContext.Step = result + results[i] = result + + for name, output := range step.Outputs { + var value bytes.Buffer + + if err := renderStepTemplate("outputs-"+name, output.Value, &value, &stepContext); err != nil { + return nil, nil, errors.Wrap(err, "parsing step run") + } + + switch output.Format { + case "yaml": + var out interface{} + if err := yaml.NewDecoder(&value).Decode(&out); err != nil { + return nil, nil, err + } + outputs[name] = out + case "json": + var out interface{} + if err := json.NewDecoder(&value).Decode(&out); err != nil { + return nil, nil, err + } + outputs[name] = out + default: + outputs[name] = value.String() + } + } } reportProgress("Calculating diff") diffOut, err := workspace.Diff(ctx) if err != nil { - return nil, errors.Wrap(err, "git diff failed") + return nil, nil, errors.Wrap(err, "git diff failed") } - return diffOut, err + return diffOut, outputs, err } func probeImageForShell(ctx context.Context, image string) (shell, tempfile string, err error) { diff --git a/internal/campaigns/templating.go b/internal/campaigns/templating.go index f7c0620da9..218fc9cccb 100644 --- a/internal/campaigns/templating.go +++ b/internal/campaigns/templating.go @@ -11,7 +11,48 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -func renderTemplate(name, tmpl string, out io.Writer, stepCtx *StepContext) error { +// ChangesetTemplateContext represents the contextual information available +// when rendering a field of the ChangesetTemplate as a template. +type ChangesetTemplateContext struct { + // TODO: This should also have `Steps` + Outputs map[string]interface{} + Repository graphql.Repository +} + +// ToFuncMap returns a template.FuncMap to access fields on the StepContext in a +// text/template. +func (tmplCtx *ChangesetTemplateContext) ToFuncMap() template.FuncMap { + return template.FuncMap{ + "join": strings.Join, + "split": strings.Split, + "repository": func() map[string]interface{} { + return map[string]interface{}{ + "search_result_paths": tmplCtx.Repository.SearchResultPaths(), + "name": tmplCtx.Repository.Name, + } + }, + "outputs": func() map[string]interface{} { + return tmplCtx.Outputs + }, + } +} + +func renderChangesetTemplateField(name, tmpl string, tmplCtx *ChangesetTemplateContext) (string, error) { + var out bytes.Buffer + + t, err := template.New(name).Delims("${{", "}}").Funcs(tmplCtx.ToFuncMap()).Parse(tmpl) + if err != nil { + return "", err + } + + if err := t.Execute(&out, tmplCtx); err != nil { + return "", err + } + + return out.String(), nil +} + +func renderStepTemplate(name, tmpl string, out io.Writer, stepCtx *StepContext) error { t, err := parseAsTemplate(name, tmpl, stepCtx) if err != nil { return errors.Wrap(err, "parsing step run") @@ -24,13 +65,13 @@ func parseAsTemplate(name, input string, stepCtx *StepContext) (*template.Templa return template.New(name).Delims("${{", "}}").Funcs(stepCtx.ToFuncMap()).Parse(input) } -func renderMap(m map[string]string, stepCtx *StepContext) (map[string]string, error) { +func renderStepMap(m map[string]string, stepCtx *StepContext) (map[string]string, error) { rendered := make(map[string]string, len(m)) for k, v := range rendered { var out bytes.Buffer - if err := renderTemplate(k, v, &out, stepCtx); err != nil { + if err := renderStepTemplate(k, v, &out, stepCtx); err != nil { return rendered, err } @@ -40,44 +81,61 @@ func renderMap(m map[string]string, stepCtx *StepContext) (map[string]string, er return rendered, nil } -// StepContext represents the contextual information available when executing a -// step that's defined in a campaign spec. +// StepContext represents the contextual information available when render a +// step's fields, such as "run" or "outputs", as templates. type StepContext struct { + // Outputs are the outputs set by the current and all previous steps. + Outputs map[string]interface{} + // Step is the result of the current step. Empty when evaluating the "run" field + // but filled when evaluating the "outputs" field. + Step StepResult + // PreviousStep is the result of the previous step. Empty when there is no + // previous step. PreviousStep StepResult - Repository graphql.Repository + // Repository is the Sourcegraph repository in which the steps are executed. + Repository graphql.Repository } // ToFuncMap returns a template.FuncMap to access fields on the StepContext in a // text/template. func (stepCtx *StepContext) ToFuncMap() template.FuncMap { - return template.FuncMap{ - "join": func(list []string, sep string) string { - return strings.Join(list, sep) - }, - "split": func(s string, sep string) []string { - return strings.Split(s, sep) - }, - "previous_step": func() map[string]interface{} { - result := map[string]interface{}{ - "modified_files": stepCtx.PreviousStep.ModifiedFiles(), - "added_files": stepCtx.PreviousStep.AddedFiles(), - "deleted_files": stepCtx.PreviousStep.DeletedFiles(), - "renamed_files": stepCtx.PreviousStep.RenamedFiles(), - } + newStepResult := func(res *StepResult) map[string]interface{} { + m := map[string]interface{}{ + "modified_files": "", + "added_files": "", + "deleted_files": "", + "renamed_files": "", + "stdout": "", + "stderr": "", + } - if stepCtx.PreviousStep.Stdout != nil { - result["stdout"] = stepCtx.PreviousStep.Stdout.String() - } else { - result["stdout"] = "" - } + m["modified_files"] = res.ModifiedFiles() + m["added_files"] = res.AddedFiles() + m["deleted_files"] = res.DeletedFiles() + m["renamed_files"] = res.RenamedFiles() - if stepCtx.PreviousStep.Stderr != nil { - result["stderr"] = stepCtx.PreviousStep.Stderr.String() - } else { - result["stderr"] = "" - } + if res.Stdout != nil { + m["stdout"] = res.Stdout.String() + } + + if res.Stderr != nil { + m["stderr"] = res.Stderr.String() + } + + return m + } - return result + return template.FuncMap{ + "join": strings.Join, + "split": strings.Split, + "previous_step": func() map[string]interface{} { + return newStepResult(&stepCtx.PreviousStep) + }, + "step": func() map[string]interface{} { + return newStepResult(&stepCtx.Step) + }, + "outputs": func() map[string]interface{} { + return stepCtx.Outputs }, "repository": func() map[string]interface{} { return map[string]interface{}{ diff --git a/internal/campaigns/templating_test.go b/internal/campaigns/templating_test.go index 5beeb28d30..e8be60b165 100644 --- a/internal/campaigns/templating_test.go +++ b/internal/campaigns/templating_test.go @@ -6,6 +6,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" + "gopkg.in/yaml.v2" ) func TestParseGitStatus(t *testing.T) { @@ -33,7 +34,26 @@ R README.md -> README.markdown } } +const rawYaml = `dist: release +env: + - GO111MODULE=on + - CGO_ENABLED=0 +before: + hooks: + - go mod download + - go mod tidy + - go generate ./schema +` + func TestParsingAndRenderingTemplates(t *testing.T) { + // To avoid bugs due to differences between test setup and actual code, we + // do the actual parsing of YAML here to get an interface{} which we'll put + // in the StepContext. + var parsedYaml interface{} + if err := yaml.Unmarshal([]byte(rawYaml), &parsedYaml); err != nil { + t.Fatalf("failed to parse YAML: %s", err) + } + stepCtx := &StepContext{ PreviousStep: StepResult{ files: &StepChanges{ @@ -42,8 +62,22 @@ func TestParsingAndRenderingTemplates(t *testing.T) { Deleted: []string{".DS_Store"}, Renamed: []string{"new-filename.txt"}, }, - Stdout: bytes.NewBufferString("this is stdout"), - Stderr: bytes.NewBufferString("this is stderr"), + Stdout: bytes.NewBufferString("this is previous step's stdout"), + Stderr: bytes.NewBufferString("this is previous step's stderr"), + }, + Outputs: map[string]interface{}{ + "lastLine": "lastLine is this", + "project": parsedYaml, + }, + Step: StepResult{ + files: &StepChanges{ + Modified: []string{"step-go.mod"}, + Added: []string{"step-main.go.swp"}, + Deleted: []string{"step-.DS_Store"}, + Renamed: []string{"step-new-filename.txt"}, + }, + Stdout: bytes.NewBufferString("this is current step's stdout"), + Stderr: bytes.NewBufferString("this is current step's stderr"), }, Repository: graphql.Repository{ Name: "github.com/sourcegraph/src-cli", @@ -60,84 +94,46 @@ func TestParsingAndRenderingTemplates(t *testing.T) { run string want string }{ - { - name: "previous step file changes", - stepCtx: stepCtx, - run: `${{ .PreviousStep.ModifiedFiles }} -${{ .PreviousStep.AddedFiles }} -${{ .PreviousStep.DeletedFiles }} -${{ .PreviousStep.RenamedFiles }} -`, - want: `[go.mod] -[main.go.swp] -[.DS_Store] -[new-filename.txt] -`, - }, - { - name: "previous step output", - stepCtx: stepCtx, - run: `${{ .PreviousStep.Stdout }} ${{ .PreviousStep.Stderr }}`, - want: `this is stdout this is stderr`, - }, - { - name: "repository name", - stepCtx: stepCtx, - run: `${{ .Repository.Name }}`, - want: `github.com/sourcegraph/src-cli`, - }, - { - name: "search result paths", - stepCtx: stepCtx, - run: `${{ .Repository.SearchResultPaths }}`, - want: `README.md main.go`, - }, { name: "lower-case aliases", stepCtx: stepCtx, run: `${{ repository.search_result_paths }} - ${{ repository.name }} - ${{ previous_step.modified_files }} - ${{ previous_step.added_files }} - ${{ previous_step.deleted_files }} - ${{ previous_step.renamed_files }} - ${{ previous_step.stdout }} - ${{ previous_step.stderr}} - `, - want: `README.md main.go - github.com/sourcegraph/src-cli - [go.mod] - [main.go.swp] - [.DS_Store] - [new-filename.txt] - this is stdout - this is stderr - `, - }, - { - name: "empty context", - stepCtx: &StepContext{}, - run: `${{ .Repository.SearchResultPaths }} -${{ .Repository.Name }} +${{ repository.name }} ${{ previous_step.modified_files }} ${{ previous_step.added_files }} ${{ previous_step.deleted_files }} ${{ previous_step.renamed_files }} ${{ previous_step.stdout }} ${{ previous_step.stderr}} +${{ outputs.lastLine }} +${{ index outputs.project.env 1 }} +${{ step.modified_files }} +${{ step.added_files }} +${{ step.deleted_files }} +${{ step.renamed_files }} +${{ step.stdout}} +${{ step.stderr}} `, - want: ` - -[] -[] -[] -[] - - + want: `README.md main.go +github.com/sourcegraph/src-cli +[go.mod] +[main.go.swp] +[.DS_Store] +[new-filename.txt] +this is previous step's stdout +this is previous step's stderr +lastLine is this +CGO_ENABLED=0 +[step-go.mod] +[step-main.go.swp] +[step-.DS_Store] +[step-new-filename.txt] +this is current step's stdout +this is current step's stderr `, }, { - name: "empty context and aliases", + name: "empty context", stepCtx: &StepContext{}, run: `${{ repository.search_result_paths }} ${{ repository.name }} @@ -147,6 +143,14 @@ ${{ previous_step.deleted_files }} ${{ previous_step.renamed_files }} ${{ previous_step.stdout }} ${{ previous_step.stderr}} +${{ outputs.lastLine }} +${{ outputs.project }} +${{ step.modified_files }} +${{ step.added_files }} +${{ step.deleted_files }} +${{ step.renamed_files }} +${{ step.stdout}} +${{ step.stderr}} `, want: ` @@ -156,22 +160,27 @@ ${{ previous_step.stderr}} [] + + +[] +[] +[] +[] + + `, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - parsed, err := parseAsTemplate("testing", tc.run, tc.stepCtx) + var out bytes.Buffer + + err := renderStepTemplate("testing", tc.run, &out, tc.stepCtx) if err != nil { t.Fatal(err) } - var out bytes.Buffer - if err := parsed.Execute(&out, tc.stepCtx); err != nil { - t.Fatalf("executing template failed: %s", err) - } - if out.String() != tc.want { t.Fatalf("wrong output:\n%s", cmp.Diff(tc.want, out.String())) } diff --git a/schema/campaign_spec.schema.json b/schema/campaign_spec.schema.json index b57dc52bcd..5eb49e2e92 100644 --- a/schema/campaign_spec.schema.json +++ b/schema/campaign_spec.schema.json @@ -75,6 +75,26 @@ "description": "The Docker image used to launch the Docker container in which the shell command is run.", "examples": ["alpine:3"] }, + "outputs": { + "type": "object", + "description": "Output variables of this step that can be referenced in the changesetTemplate or other steps via outputs.", + "additionalProperties": { + "type": "object", + "required": ["value"], + "properties": { + "value": { + "type": "string", + "description": "The value of the output, which can be a template string.", + "examples": ["${{ step.stdout }}", "${{ repository.name }}" ] + }, + "format": { + "type": "string", + "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var.", + "enum": ["json", "yaml", "text"] + } + } + } + }, "env": { "description": "Environment variables to set in the step environment.", "oneOf": [ @@ -109,6 +129,10 @@ "type": "object", "description": "Files that should be mounted into or be created inside the Docker container.", "additionalProperties": {"type": "string"} + }, + "outputVariable": { + "type": "string", + "description": "The name of the variable in which to save the output of the step. The variable has to have been declared in `vars`." } } } diff --git a/schema/campaign_spec_stringdata.go b/schema/campaign_spec_stringdata.go index 131ed382c9..9590f2e2f2 100644 --- a/schema/campaign_spec_stringdata.go +++ b/schema/campaign_spec_stringdata.go @@ -61,6 +61,29 @@ const CampaignSpecJSON = `{ ] } }, + "vars": { + "type": "array", + "description": "Optional list of variables that can be used with templating-syntax to interpolate values into ` + "`" + `steps` + "`" + ` and ` + "`" + `changesetTemplate` + "`" + `", + "items": { + "title": "Var", + "type": "object", + "description": "A variable that is available in the ` + "`" + `steps` + "`" + ` or ` + "`" + `changesetTemplate` + "`" + ` as a template variable.", + "additionalProperties": false, + "required": ["name", "default"], + "properties": { + "name": { + "type": "string", + "description": "The name of the variable.", + "pattern": "^[\\w.-_]+$" + }, + "default": { + "type": "string", + "description": "The default value of the variable.", + "examples": ["alpine:3"] + } + } + } + }, "steps": { "type": "array", "description": "The sequence of commands to run (for each repository branch matched in the ` + "`" + `on` + "`" + ` property) to produce the campaign's changes.", @@ -80,6 +103,42 @@ const CampaignSpecJSON = `{ "description": "The Docker image used to launch the Docker container in which the shell command is run.", "examples": ["alpine:3"] }, + "output": { + "type": "object", + "description": "Specification of whether and how to capture the output produced by the command", + "required": ["var"], + "properties": { + "var": { + "type": "string", + "description": "The name of the variable in which to store the output, after optionally converting it if 'format' is given." + }, + "format": { + "type": "string", + "description": "The expected format of the output. If given the output is being parsed in that format before being stored in the var.", + "enum": ["json", "yaml", "text"] + } + } + }, + "outputs": { + "type": "object", + "description": "Output variables of this step that can be referenced in the changesetTemplate or other steps via outputs.", + "additionalProperties": { + "type": "object", + "required": ["value"], + "properties": { + "value": { + "type": "string", + "description": "The value of the output, which can be a template string.", + "examples": ["${{ step.stdout }}", "${{ repository.name }}" ] + }, + "format": { + "type": "string", + "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var.", + "enum": ["json", "yaml", "text"] + } + } + } + }, "env": { "description": "Environment variables to set in the step environment.", "oneOf": [ @@ -114,6 +173,10 @@ const CampaignSpecJSON = `{ "type": "object", "description": "Files that should be mounted into or be created inside the Docker container.", "additionalProperties": {"type": "string"} + }, + "outputVariable": { + "type": "string", + "description": "The name of the variable in which to save the output of the step. The variable has to have been declared in ` + "`" + `vars` + "`" + `." } } } From 18c666a4f97b365b9eecbe5c0315d311320d3b30 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 13 Jan 2021 14:55:16 +0100 Subject: [PATCH 02/19] Fix parsing of outputs --- internal/campaigns/campaign_spec.go | 2 +- internal/campaigns/executor.go | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/campaigns/campaign_spec.go b/internal/campaigns/campaign_spec.go index f5b94ed7ce..0aefd0cb52 100644 --- a/internal/campaigns/campaign_spec.go +++ b/internal/campaigns/campaign_spec.go @@ -71,7 +71,7 @@ type Step struct { Container string `json:"container,omitempty" yaml:"container"` Env env.Environment `json:"env,omitempty" yaml:"env"` Files map[string]string `json:"files,omitempty" yaml:"files,omitempty"` - Outputs Outputs `json:"output,omitempty" yaml:"output,omitempty"` + Outputs Outputs `json:"outputs,omitempty" yaml:"outputs,omitempty"` image string } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index 14d40cbccd..9641b8193a 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -183,6 +183,7 @@ func (x *executor) AddTask(repo *graphql.Repository, steps []Step, transform *Tr Template: template, TransformChanges: transform, } + x.tasks = append(x.tasks, task) x.statusesMu.Lock() @@ -277,10 +278,10 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { return } - // TODO: We need to cache the vars here too if we want use to render changesetTemplate - vars := map[string]interface{}{} + // TODO: We need to cache the outputs here too if we want use to render changesetTemplate + outputs := map[string]interface{}{} var specs []*ChangesetSpec - specs, err = createChangesetSpecs(task, diff, vars, x.features) + specs, err = createChangesetSpecs(task, diff, outputs, x.features) if err != nil { return err } From d4972e660990a13f3ceea9c638b659bb80927e83 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 13 Jan 2021 15:16:33 +0100 Subject: [PATCH 03/19] Remove leftovers in campaign spec schema --- schema/campaign_spec.schema.json | 4 --- schema/campaign_spec_stringdata.go | 43 ------------------------------ 2 files changed, 47 deletions(-) diff --git a/schema/campaign_spec.schema.json b/schema/campaign_spec.schema.json index 5eb49e2e92..8c8bffb2db 100644 --- a/schema/campaign_spec.schema.json +++ b/schema/campaign_spec.schema.json @@ -129,10 +129,6 @@ "type": "object", "description": "Files that should be mounted into or be created inside the Docker container.", "additionalProperties": {"type": "string"} - }, - "outputVariable": { - "type": "string", - "description": "The name of the variable in which to save the output of the step. The variable has to have been declared in `vars`." } } } diff --git a/schema/campaign_spec_stringdata.go b/schema/campaign_spec_stringdata.go index 9590f2e2f2..c4d6bb2732 100644 --- a/schema/campaign_spec_stringdata.go +++ b/schema/campaign_spec_stringdata.go @@ -61,29 +61,6 @@ const CampaignSpecJSON = `{ ] } }, - "vars": { - "type": "array", - "description": "Optional list of variables that can be used with templating-syntax to interpolate values into ` + "`" + `steps` + "`" + ` and ` + "`" + `changesetTemplate` + "`" + `", - "items": { - "title": "Var", - "type": "object", - "description": "A variable that is available in the ` + "`" + `steps` + "`" + ` or ` + "`" + `changesetTemplate` + "`" + ` as a template variable.", - "additionalProperties": false, - "required": ["name", "default"], - "properties": { - "name": { - "type": "string", - "description": "The name of the variable.", - "pattern": "^[\\w.-_]+$" - }, - "default": { - "type": "string", - "description": "The default value of the variable.", - "examples": ["alpine:3"] - } - } - } - }, "steps": { "type": "array", "description": "The sequence of commands to run (for each repository branch matched in the ` + "`" + `on` + "`" + ` property) to produce the campaign's changes.", @@ -103,22 +80,6 @@ const CampaignSpecJSON = `{ "description": "The Docker image used to launch the Docker container in which the shell command is run.", "examples": ["alpine:3"] }, - "output": { - "type": "object", - "description": "Specification of whether and how to capture the output produced by the command", - "required": ["var"], - "properties": { - "var": { - "type": "string", - "description": "The name of the variable in which to store the output, after optionally converting it if 'format' is given." - }, - "format": { - "type": "string", - "description": "The expected format of the output. If given the output is being parsed in that format before being stored in the var.", - "enum": ["json", "yaml", "text"] - } - } - }, "outputs": { "type": "object", "description": "Output variables of this step that can be referenced in the changesetTemplate or other steps via outputs.", @@ -173,10 +134,6 @@ const CampaignSpecJSON = `{ "type": "object", "description": "Files that should be mounted into or be created inside the Docker container.", "additionalProperties": {"type": "string"} - }, - "outputVariable": { - "type": "string", - "description": "The name of the variable in which to save the output of the step. The variable has to have been declared in ` + "`" + `vars` + "`" + `." } } } From 971d81dc82ab40bc789f1a41d11eceef3c67802f Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 13 Jan 2021 16:55:53 +0100 Subject: [PATCH 04/19] Remove debug output --- internal/campaigns/executor_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 8a64b4f9f3..2aab283907 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -320,9 +320,6 @@ func TestExecutor_Integration(t *testing.T) { t.Fatalf("wrong number of changeset specs. want=%d, have=%d", want, have) } - for i, spec := range specs { - t.Logf("specs[%d]=%+v\n", i, spec.CreatedChangeset) - } for _, spec := range specs { if tc.wantTitle != "" && spec.Title != tc.wantTitle { t.Errorf("wrong title. want=%q, have=%q", tc.wantTitle, spec.Title) From be95d6f7569664aa712ac9e1948885e6df6c698c Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 13 Jan 2021 16:56:29 +0100 Subject: [PATCH 05/19] Rename test function --- internal/campaigns/templating_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/campaigns/templating_test.go b/internal/campaigns/templating_test.go index e8be60b165..ae7f403031 100644 --- a/internal/campaigns/templating_test.go +++ b/internal/campaigns/templating_test.go @@ -45,7 +45,7 @@ before: - go generate ./schema ` -func TestParsingAndRenderingTemplates(t *testing.T) { +func TestRenderStepTemplate(t *testing.T) { // To avoid bugs due to differences between test setup and actual code, we // do the actual parsing of YAML here to get an interface{} which we'll put // in the StepContext. From 580a3be36519a28a516e982c06f4a8550be86ee4 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Wed, 13 Jan 2021 17:01:01 +0100 Subject: [PATCH 06/19] Add a test for renderChangesetTemplateField --- internal/campaigns/templating_test.go | 73 +++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/internal/campaigns/templating_test.go b/internal/campaigns/templating_test.go index ae7f403031..5236ad43c4 100644 --- a/internal/campaigns/templating_test.go +++ b/internal/campaigns/templating_test.go @@ -187,3 +187,76 @@ ${{ step.stderr}} }) } } + +func TestRenderChangesetTemplateField(t *testing.T) { + // To avoid bugs due to differences between test setup and actual code, we + // do the actual parsing of YAML here to get an interface{} which we'll put + // in the StepContext. + var parsedYaml interface{} + if err := yaml.Unmarshal([]byte(rawYaml), &parsedYaml); err != nil { + t.Fatalf("failed to parse YAML: %s", err) + } + + tmplCtx := &ChangesetTemplateContext{ + Outputs: map[string]interface{}{ + "lastLine": "lastLine is this", + "project": parsedYaml, + }, + Repository: graphql.Repository{ + Name: "github.com/sourcegraph/src-cli", + FileMatches: map[string]bool{ + "README.md": true, + "main.go": true, + }, + }, + } + + tests := []struct { + name string + tmplCtx *ChangesetTemplateContext + run string + want string + }{ + { + name: "lower-case aliases", + tmplCtx: tmplCtx, + run: `${{ repository.search_result_paths }} +${{ repository.name }} +${{ outputs.lastLine }} +${{ index outputs.project.env 1 }} +`, + want: `README.md main.go +github.com/sourcegraph/src-cli +lastLine is this +CGO_ENABLED=0 +`, + }, + { + name: "empty context", + tmplCtx: &ChangesetTemplateContext{}, + run: `${{ repository.search_result_paths }} +${{ repository.name }} +${{ outputs.lastLine }} +${{ outputs.project }} +`, + want: ` + + + +`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + out, err := renderChangesetTemplateField("testing", tc.run, tc.tmplCtx) + if err != nil { + t.Fatal(err) + } + + if out != tc.want { + t.Fatalf("wrong output:\n%s", cmp.Diff(tc.want, out)) + } + }) + } +} From a8a696db205cdef31db6ae6d8d5306a8be6c1874 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 14 Jan 2021 10:28:29 +0100 Subject: [PATCH 07/19] Update docs/examples in campaign spec schema --- schema/campaign_spec.schema.json | 4 ++-- schema/campaign_spec_stringdata.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/schema/campaign_spec.schema.json b/schema/campaign_spec.schema.json index 8c8bffb2db..bd2bfbb828 100644 --- a/schema/campaign_spec.schema.json +++ b/schema/campaign_spec.schema.json @@ -85,11 +85,11 @@ "value": { "type": "string", "description": "The value of the output, which can be a template string.", - "examples": ["${{ step.stdout }}", "${{ repository.name }}" ] + "examples": ["hello world", "${{ step.stdout }}", "${{ repository.name }}" ] }, "format": { "type": "string", - "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var.", + "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var. If not set, 'text' is assumed to the format.", "enum": ["json", "yaml", "text"] } } diff --git a/schema/campaign_spec_stringdata.go b/schema/campaign_spec_stringdata.go index c4d6bb2732..6d850af9ea 100644 --- a/schema/campaign_spec_stringdata.go +++ b/schema/campaign_spec_stringdata.go @@ -90,11 +90,11 @@ const CampaignSpecJSON = `{ "value": { "type": "string", "description": "The value of the output, which can be a template string.", - "examples": ["${{ step.stdout }}", "${{ repository.name }}" ] + "examples": ["hello world", "${{ step.stdout }}", "${{ repository.name }}" ] }, "format": { "type": "string", - "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var.", + "description": "The expected format of the output. If set, the output is being parsed in that format before being stored in the var. If not set, 'text' is assumed to the format.", "enum": ["json", "yaml", "text"] } } From 3cbd3623e39b604a6f4a65e95260134236728e2b Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 14 Jan 2021 14:03:24 +0100 Subject: [PATCH 08/19] Get dynamic changeset templates working with cache --- go.mod | 1 + internal/campaigns/execution_cache.go | 79 +++++++++++++++++++-------- internal/campaigns/executor.go | 12 ++-- internal/campaigns/executor_test.go | 22 +++----- internal/campaigns/run_steps.go | 61 ++++++++++++--------- 5 files changed, 108 insertions(+), 67 deletions(-) diff --git a/go.mod b/go.mod index ebe9209de1..e9f3a5a216 100644 --- a/go.mod +++ b/go.mod @@ -30,6 +30,7 @@ require ( golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4 gopkg.in/yaml.v2 v2.3.0 + gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 jaytaylor.com/html2text v0.0.0-20200412013138-3577fbdbcff7 ) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index 62017a8430..9317c5b7c1 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -62,8 +62,8 @@ func (key ExecutionCacheKey) Key() (string, error) { } type ExecutionCache interface { - Get(ctx context.Context, key ExecutionCacheKey) (diff string, found bool, err error) - Set(ctx context.Context, key ExecutionCacheKey, diff string) error + Get(ctx context.Context, key ExecutionCacheKey) (diff string, outputs map[string]interface{}, found bool, err error) + Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error Clear(ctx context.Context, key ExecutionCacheKey) error } @@ -77,13 +77,20 @@ func (c ExecutionDiskCache) cacheFilePath(key ExecutionCacheKey) (string, error) return "", errors.Wrap(err, "calculating execution cache key") } - return filepath.Join(c.Dir, keyString+".diff"), nil + return filepath.Join(c.Dir, keyString+".v3.json"), nil } -func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (string, bool, error) { +type executionResult struct { + Diff string `json:"diff"` + Outputs map[string]interface{} `json:"outputs"` +} + +func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (string, map[string]interface{}, bool, error) { + outputs := map[string]interface{}{} + path, err := c.cacheFilePath(key) if err != nil { - return "", false, err + return "", outputs, false, err } data, err := ioutil.ReadFile(path) @@ -91,44 +98,70 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (str if os.IsNotExist(err) { err = nil // treat as not-found } - return "", false, err + return "", outputs, false, err } - // We previously cached complete ChangesetSpecs instead of just the diffs. - // To be backwards compatible, we keep reading these: - if strings.HasSuffix(path, ".json") { + // There are now three different cache versions out in the wild and to be + // backwards compatible we read all of them. + // 2-step plan for taming this: + // 1) February 2021: deprecate old caches by deleting the files when + // detected and reporting as cache-miss. + // 2) May 2021 (two releases later): remove handling of these files from + // this function + switch { + case strings.HasSuffix(path, ".v3.json"): + // v3 of the cache: we cache the diff and the outputs produced by the step. + var result executionResult + if err := json.Unmarshal(data, &result); err != nil { + // Delete the invalid data to avoid causing an error for next time. + if err := os.Remove(path); err != nil { + return "", outputs, false, errors.Wrap(err, "while deleting cache file with invalid JSON") + } + return "", outputs, false, errors.Wrapf(err, "reading cache file %s", path) + } + return result.Diff, result.Outputs, true, nil + + case strings.HasSuffix(path, ".diff"): + // v2 of the cache: we only cached the diff, since that's the + // only bit of data we were interested in. + return string(data), outputs, true, nil + + case strings.HasSuffix(path, ".json"): + // v1 of the cache: we cached the complete ChangesetSpec instead of just the diffs. var result ChangesetSpec if err := json.Unmarshal(data, &result); err != nil { // Delete the invalid data to avoid causing an error for next time. if err := os.Remove(path); err != nil { - return "", false, errors.Wrap(err, "while deleting cache file with invalid JSON") + return "", outputs, false, errors.Wrap(err, "while deleting cache file with invalid JSON") } - return "", false, errors.Wrapf(err, "reading cache file %s", path) + return "", outputs, false, errors.Wrapf(err, "reading cache file %s", path) } if len(result.Commits) != 1 { - return "", false, errors.New("cached result has no commits") + return "", outputs, false, errors.New("cached result has no commits") } - return result.Commits[0].Diff, true, nil - } - - if strings.HasSuffix(path, ".diff") { - return string(data), true, nil + return result.Commits[0].Diff, outputs, true, nil } - return "", false, fmt.Errorf("unknown file format for cache file %q", path) + return "", outputs, false, fmt.Errorf("unknown file format for cache file %q", path) } -func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, diff string) error { +func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { path, err := c.cacheFilePath(key) if err != nil { return err } + res := &executionResult{Diff: diff, Outputs: outputs} + raw, err := json.Marshal(&res) + if err != nil { + return errors.Wrap(err, "serializing execution result to JSON") + } + if err := os.MkdirAll(filepath.Dir(path), 0700); err != nil { return err } - return ioutil.WriteFile(path, []byte(diff), 0600) + return ioutil.WriteFile(path, raw, 0600) } func (c ExecutionDiskCache) Clear(ctx context.Context, key ExecutionCacheKey) error { @@ -148,11 +181,11 @@ func (c ExecutionDiskCache) Clear(ctx context.Context, key ExecutionCacheKey) er // retrieve cache entries. type ExecutionNoOpCache struct{} -func (ExecutionNoOpCache) Get(ctx context.Context, key ExecutionCacheKey) (diff string, found bool, err error) { - return "", false, nil +func (ExecutionNoOpCache) Get(ctx context.Context, key ExecutionCacheKey) (diff string, outputs map[string]interface{}, found bool, err error) { + return "", map[string]interface{}{}, false, nil } -func (ExecutionNoOpCache) Set(ctx context.Context, key ExecutionCacheKey, diff string) error { +func (ExecutionNoOpCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { return nil } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index 9641b8193a..2fc15ee7c5 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -57,6 +57,7 @@ type Executor interface { type Task struct { Repository *graphql.Repository Steps []Step + Outputs map[string]interface{} Template *ChangesetTemplate `json:"-"` TransformChanges *TransformChanges `json:"-"` @@ -255,11 +256,12 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } } else { var ( - diff string - found bool + diff string + outputs map[string]interface{} + found bool ) - diff, found, err = x.cache.Get(ctx, cacheKey) + diff, outputs, found, err = x.cache.Get(ctx, cacheKey) if err != nil { err = errors.Wrapf(err, "checking cache for %q", task.Repository.Name) return @@ -278,8 +280,6 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { return } - // TODO: We need to cache the outputs here too if we want use to render changesetTemplate - outputs := map[string]interface{}{} var specs []*ChangesetSpec specs, err = createChangesetSpecs(task, diff, outputs, x.features) if err != nil { @@ -346,7 +346,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { // Add to the cache. We don't use runCtx here because we want to write to // the cache even if we've now reached the timeout. - if err = x.cache.Set(ctx, cacheKey, string(diff)); err != nil { + if err = x.cache.Set(ctx, cacheKey, string(diff), outputs); err != nil { err = errors.Wrapf(err, "caching result for %q", task.Repository.Name) } diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 2aab283907..73109a3410 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -399,10 +399,6 @@ func TestExecutor_Integration(t *testing.T) { // Run with a warm cache. t.Run("warm cache", func(t *testing.T) { - if tc.name == "templated changesetTemplate" { - // TODO: Make caching work with steps outputs - t.Skip() - } execute() verifyCache() }) @@ -622,13 +618,13 @@ func newZipArchivesMux(t *testing.T, callback http.HandlerFunc, archives ...mock // inMemoryExecutionCache provides an in-memory cache for testing purposes. type inMemoryExecutionCache struct { - cache map[string]string + cache map[string]executionResult mu sync.RWMutex } func newInMemoryExecutionCache() *inMemoryExecutionCache { return &inMemoryExecutionCache{ - cache: make(map[string]string), + cache: make(map[string]executionResult), } } @@ -639,22 +635,22 @@ func (c *inMemoryExecutionCache) size() int { return len(c.cache) } -func (c *inMemoryExecutionCache) Get(ctx context.Context, key ExecutionCacheKey) (string, bool, error) { +func (c *inMemoryExecutionCache) Get(ctx context.Context, key ExecutionCacheKey) (string, map[string]interface{}, bool, error) { k, err := key.Key() if err != nil { - return "", false, err + return "", nil, false, err } c.mu.RLock() defer c.mu.RUnlock() - if diff, ok := c.cache[k]; ok { - return diff, true, nil + if res, ok := c.cache[k]; ok { + return res.Diff, res.Outputs, true, nil } - return "", false, nil + return "", nil, false, nil } -func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, diff string) error { +func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { k, err := key.Key() if err != nil { return err @@ -663,7 +659,7 @@ func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, c.mu.Lock() defer c.mu.Unlock() - c.cache[k] = diff + c.cache[k] = executionResult{Diff: diff, Outputs: outputs} return nil } diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index 7694f92e2d..a0c9e560c0 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -15,7 +15,8 @@ import ( "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" - "gopkg.in/yaml.v2" + + yamlv3 "gopkg.in/yaml.v3" ) func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) (diff []byte, outputs map[string]interface{}, err error) { @@ -215,30 +216,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr stepContext.Step = result results[i] = result - for name, output := range step.Outputs { - var value bytes.Buffer - - if err := renderStepTemplate("outputs-"+name, output.Value, &value, &stepContext); err != nil { - return nil, nil, errors.Wrap(err, "parsing step run") - } - - switch output.Format { - case "yaml": - var out interface{} - if err := yaml.NewDecoder(&value).Decode(&out); err != nil { - return nil, nil, err - } - outputs[name] = out - case "json": - var out interface{} - if err := json.NewDecoder(&value).Decode(&out); err != nil { - return nil, nil, err - } - outputs[name] = out - default: - outputs[name] = value.String() - } - } + setOutputs(step.Outputs, outputs, &stepContext) } reportProgress("Calculating diff") @@ -250,6 +228,39 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr return diffOut, outputs, err } +func setOutputs(stepOutputs Outputs, global map[string]interface{}, stepCtx *StepContext) error { + for name, output := range stepOutputs { + var value bytes.Buffer + + if err := renderStepTemplate("outputs-"+name, output.Value, &value, stepCtx); err != nil { + return errors.Wrap(err, "parsing step run") + } + + switch output.Format { + case "yaml": + var out interface{} + // We use yamlv3 here, because it unmarshals YAML into + // map[string]interface{} which we need to serialize it back to + // JSON when we cache the results. + // See https://github.com/go-yaml/yaml/issues/139 for context + if err := yamlv3.NewDecoder(&value).Decode(&out); err != nil { + return err + } + global[name] = out + case "json": + var out interface{} + if err := json.NewDecoder(&value).Decode(&out); err != nil { + return err + } + global[name] = out + default: + global[name] = value.String() + } + } + + return nil +} + func probeImageForShell(ctx context.Context, image string) (shell, tempfile string, err error) { // We need to know two things to be able to run a shell script: // From 3ffa54efe1ef2f4e744ce445dfbc4fbbf81db998 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 14 Jan 2021 14:11:44 +0100 Subject: [PATCH 09/19] Simplify ExecutionCache by using ExecutionResult --- internal/campaigns/execution_cache.go | 57 +++++++++++++++------------ internal/campaigns/executor.go | 14 +++---- internal/campaigns/executor_test.go | 16 ++++---- 3 files changed, 46 insertions(+), 41 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index 9317c5b7c1..de82f02c52 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -62,8 +62,8 @@ func (key ExecutionCacheKey) Key() (string, error) { } type ExecutionCache interface { - Get(ctx context.Context, key ExecutionCacheKey) (diff string, outputs map[string]interface{}, found bool, err error) - Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error + Get(ctx context.Context, key ExecutionCacheKey) (result ExecutionResult, found bool, err error) + Set(ctx context.Context, key ExecutionCacheKey, result ExecutionResult) error Clear(ctx context.Context, key ExecutionCacheKey) error } @@ -80,17 +80,17 @@ func (c ExecutionDiskCache) cacheFilePath(key ExecutionCacheKey) (string, error) return filepath.Join(c.Dir, keyString+".v3.json"), nil } -type executionResult struct { +type ExecutionResult struct { Diff string `json:"diff"` Outputs map[string]interface{} `json:"outputs"` } -func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (string, map[string]interface{}, bool, error) { - outputs := map[string]interface{}{} +func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (ExecutionResult, bool, error) { + var result ExecutionResult path, err := c.cacheFilePath(key) if err != nil { - return "", outputs, false, err + return result, false, err } data, err := ioutil.ReadFile(path) @@ -98,7 +98,7 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (str if os.IsNotExist(err) { err = nil // treat as not-found } - return "", outputs, false, err + return result, false, err } // There are now three different cache versions out in the wild and to be @@ -111,48 +111,53 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (str switch { case strings.HasSuffix(path, ".v3.json"): // v3 of the cache: we cache the diff and the outputs produced by the step. - var result executionResult if err := json.Unmarshal(data, &result); err != nil { // Delete the invalid data to avoid causing an error for next time. if err := os.Remove(path); err != nil { - return "", outputs, false, errors.Wrap(err, "while deleting cache file with invalid JSON") + return result, false, errors.Wrap(err, "while deleting cache file with invalid JSON") } - return "", outputs, false, errors.Wrapf(err, "reading cache file %s", path) + return result, false, errors.Wrapf(err, "reading cache file %s", path) } - return result.Diff, result.Outputs, true, nil + return result, true, nil case strings.HasSuffix(path, ".diff"): // v2 of the cache: we only cached the diff, since that's the // only bit of data we were interested in. - return string(data), outputs, true, nil + result.Diff = string(data) + result.Outputs = map[string]interface{}{} + + return result, true, nil case strings.HasSuffix(path, ".json"): // v1 of the cache: we cached the complete ChangesetSpec instead of just the diffs. - var result ChangesetSpec - if err := json.Unmarshal(data, &result); err != nil { + var spec ChangesetSpec + if err := json.Unmarshal(data, &spec); err != nil { // Delete the invalid data to avoid causing an error for next time. if err := os.Remove(path); err != nil { - return "", outputs, false, errors.Wrap(err, "while deleting cache file with invalid JSON") + return result, false, errors.Wrap(err, "while deleting cache file with invalid JSON") } - return "", outputs, false, errors.Wrapf(err, "reading cache file %s", path) + return result, false, errors.Wrapf(err, "reading cache file %s", path) } - if len(result.Commits) != 1 { - return "", outputs, false, errors.New("cached result has no commits") + if len(spec.Commits) != 1 { + return result, false, errors.New("cached result has no commits") } - return result.Commits[0].Diff, outputs, true, nil + + result.Diff = spec.Commits[0].Diff + result.Outputs = map[string]interface{}{} + + return result, true, nil } - return "", outputs, false, fmt.Errorf("unknown file format for cache file %q", path) + return result, false, fmt.Errorf("unknown file format for cache file %q", path) } -func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { +func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, result ExecutionResult) error { path, err := c.cacheFilePath(key) if err != nil { return err } - res := &executionResult{Diff: diff, Outputs: outputs} - raw, err := json.Marshal(&res) + raw, err := json.Marshal(&result) if err != nil { return errors.Wrap(err, "serializing execution result to JSON") } @@ -181,11 +186,11 @@ func (c ExecutionDiskCache) Clear(ctx context.Context, key ExecutionCacheKey) er // retrieve cache entries. type ExecutionNoOpCache struct{} -func (ExecutionNoOpCache) Get(ctx context.Context, key ExecutionCacheKey) (diff string, outputs map[string]interface{}, found bool, err error) { - return "", map[string]interface{}{}, false, nil +func (ExecutionNoOpCache) Get(ctx context.Context, key ExecutionCacheKey) (result ExecutionResult, found bool, err error) { + return ExecutionResult{}, false, nil } -func (ExecutionNoOpCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { +func (ExecutionNoOpCache) Set(ctx context.Context, key ExecutionCacheKey, result ExecutionResult) error { return nil } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index 2fc15ee7c5..c4f6ae6ffc 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -256,12 +256,11 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } } else { var ( - diff string - outputs map[string]interface{} - found bool + result ExecutionResult + found bool ) - diff, outputs, found, err = x.cache.Get(ctx, cacheKey) + result, found, err = x.cache.Get(ctx, cacheKey) if err != nil { err = errors.Wrapf(err, "checking cache for %q", task.Repository.Name) return @@ -271,7 +270,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { // add it to the list of specs that are displayed to the user and // send to the server. Instead, we can just report that the task is // complete and move on. - if len(diff) == 0 { + if len(result.Diff) == 0 { x.updateTaskStatus(task, func(status *TaskStatus) { status.Cached = true status.FinishedAt = time.Now() @@ -281,7 +280,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } var specs []*ChangesetSpec - specs, err = createChangesetSpecs(task, diff, outputs, x.features) + specs, err = createChangesetSpecs(task, result.Diff, result.Outputs, x.features) if err != nil { return err } @@ -346,7 +345,8 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { // Add to the cache. We don't use runCtx here because we want to write to // the cache even if we've now reached the timeout. - if err = x.cache.Set(ctx, cacheKey, string(diff), outputs); err != nil { + result := ExecutionResult{Diff: string(diff), Outputs: outputs} + if err = x.cache.Set(ctx, cacheKey, result); err != nil { err = errors.Wrapf(err, "caching result for %q", task.Repository.Name) } diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 73109a3410..0a72002830 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -618,13 +618,13 @@ func newZipArchivesMux(t *testing.T, callback http.HandlerFunc, archives ...mock // inMemoryExecutionCache provides an in-memory cache for testing purposes. type inMemoryExecutionCache struct { - cache map[string]executionResult + cache map[string]ExecutionResult mu sync.RWMutex } func newInMemoryExecutionCache() *inMemoryExecutionCache { return &inMemoryExecutionCache{ - cache: make(map[string]executionResult), + cache: make(map[string]ExecutionResult), } } @@ -635,22 +635,22 @@ func (c *inMemoryExecutionCache) size() int { return len(c.cache) } -func (c *inMemoryExecutionCache) Get(ctx context.Context, key ExecutionCacheKey) (string, map[string]interface{}, bool, error) { +func (c *inMemoryExecutionCache) Get(ctx context.Context, key ExecutionCacheKey) (ExecutionResult, bool, error) { k, err := key.Key() if err != nil { - return "", nil, false, err + return ExecutionResult{}, false, err } c.mu.RLock() defer c.mu.RUnlock() if res, ok := c.cache[k]; ok { - return res.Diff, res.Outputs, true, nil + return res, true, nil } - return "", nil, false, nil + return ExecutionResult{}, false, nil } -func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, diff string, outputs map[string]interface{}) error { +func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, result ExecutionResult) error { k, err := key.Key() if err != nil { return err @@ -659,7 +659,7 @@ func (c *inMemoryExecutionCache) Set(ctx context.Context, key ExecutionCacheKey, c.mu.Lock() defer c.mu.Unlock() - c.cache[k] = executionResult{Diff: diff, Outputs: outputs} + c.cache[k] = result return nil } From a0af0a09bfdb4d2bcf6983911f2ce8a47eb30d82 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 14 Jan 2021 14:19:17 +0100 Subject: [PATCH 10/19] Change interface of runSteps to return ExecutionResult --- internal/campaigns/execution_cache.go | 5 --- internal/campaigns/executor.go | 7 ++-- internal/campaigns/run_steps.go | 57 +++++++++++++++------------ 3 files changed, 35 insertions(+), 34 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index de82f02c52..1dc03d6aa1 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -80,11 +80,6 @@ func (c ExecutionDiskCache) cacheFilePath(key ExecutionCacheKey) (string, error) return filepath.Join(c.Dir, keyString+".v3.json"), nil } -type ExecutionResult struct { - Diff string `json:"diff"` - Outputs map[string]interface{} `json:"outputs"` -} - func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (ExecutionResult, bool, error) { var result ExecutionResult diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index c4f6ae6ffc..e4e2ea18a5 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -324,7 +324,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { defer cancel() // Actually execute the steps. - diff, outputs, err := runSteps(runCtx, x.fetcher, x.creator, task.Repository, task.Steps, log, x.tempDir, func(currentlyExecuting string) { + result, 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 }) @@ -338,21 +338,20 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } // Build the changeset specs. - specs, err := createChangesetSpecs(task, string(diff), outputs, x.features) + specs, err := createChangesetSpecs(task, result.Diff, result.Outputs, x.features) if err != nil { return err } // Add to the cache. We don't use runCtx here because we want to write to // the cache even if we've now reached the timeout. - result := ExecutionResult{Diff: string(diff), Outputs: outputs} if err = x.cache.Set(ctx, cacheKey, result); err != nil { err = errors.Wrapf(err, "caching result for %q", task.Repository.Name) } // If the steps didn't result in any diff, we don't need to add it to the // list of specs that are displayed to the user and send to the server. - if len(diff) == 0 { + if len(result.Diff) == 0 { return } diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index a0c9e560c0..ff11731179 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -19,28 +19,35 @@ import ( yamlv3 "gopkg.in/yaml.v3" ) -func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) (diff []byte, outputs map[string]interface{}, err error) { +type ExecutionResult struct { + Diff string `json:"diff"` + Outputs map[string]interface{} `json:"outputs"` +} + +func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *graphql.Repository, steps []Step, logger *TaskLogger, tempDir string, reportProgress func(string)) (result ExecutionResult, err error) { reportProgress("Downloading archive") zip, err := rf.Fetch(ctx, repo) if err != nil { - return nil, nil, errors.Wrap(err, "fetching repo") + return ExecutionResult{}, errors.Wrap(err, "fetching repo") } defer zip.Close() reportProgress("Initializing workspace") workspace, err := wc.Create(ctx, repo, zip.Path()) if err != nil { - return nil, nil, errors.Wrap(err, "creating workspace") + return ExecutionResult{}, errors.Wrap(err, "creating workspace") } defer workspace.Close(ctx) + execResult := ExecutionResult{ + Outputs: make(map[string]interface{}), + } results := make([]StepResult, len(steps)) - outputs = make(map[string]interface{}) for i, step := range steps { reportProgress(fmt.Sprintf("Preparing step %d", i+1)) - stepContext := StepContext{Repository: *repo, Outputs: outputs} + stepContext := StepContext{Repository: *repo, Outputs: execResult.Outputs} if i > 0 { stepContext.PreviousStep = results[i-1] } @@ -50,7 +57,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // container on a successful run, rather than leaving it dangling. cidFile, err := ioutil.TempFile(tempDir, repo.Slug()+"-container-id") if err != nil { - return nil, nil, errors.Wrap(err, "Creating a CID file failed") + return execResult, errors.Wrap(err, "Creating a CID file failed") } // However, Docker will fail if the cidfile actually exists, so we need @@ -58,7 +65,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // close it, even though that's unnecessary elsewhere. cidFile.Close() if err = os.Remove(cidFile.Name()); err != nil { - return nil, nil, errors.Wrap(err, "removing cidfile") + return execResult, errors.Wrap(err, "removing cidfile") } // Since we went to all that effort, we can now defer a function that @@ -76,14 +83,14 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // For now, we only support shell scripts provided via the Run field. shell, containerTemp, err := probeImageForShell(ctx, step.image) if err != nil { - return nil, nil, errors.Wrapf(err, "probing image %q for shell", step.image) + return execResult, errors.Wrapf(err, "probing image %q for shell", step.image) } // Set up a temporary file on the host filesystem to contain the // script. runScriptFile, err := ioutil.TempFile(tempDir, "") if err != nil { - return nil, nil, errors.Wrap(err, "creating temporary file") + return execResult, errors.Wrap(err, "creating temporary file") } defer os.Remove(runScriptFile.Name()) @@ -92,11 +99,11 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr var runScript bytes.Buffer out := io.MultiWriter(&runScript, runScriptFile) if err := renderStepTemplate("step-run", step.Run, out, &stepContext); err != nil { - return nil, nil, errors.Wrap(err, "parsing step run") + return execResult, errors.Wrap(err, "parsing step run") } if err := runScriptFile.Close(); err != nil { - return nil, nil, errors.Wrap(err, "closing temporary file") + return execResult, errors.Wrap(err, "closing temporary file") } // This file needs to be readable within the container regardless of the @@ -109,13 +116,13 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // conditionally compiled files here, instead we'll just wait until the // file is closed to twiddle the permission bits. Which is now! if err := os.Chmod(runScriptFile.Name(), 0644); err != nil { - return nil, nil, errors.Wrap(err, "setting permissions on the temporary file") + return execResult, errors.Wrap(err, "setting permissions on the temporary file") } // Parse and render the step.Files. files, err := renderStepMap(step.Files, &stepContext) if err != nil { - return nil, nil, errors.Wrap(err, "parsing step files") + return execResult, errors.Wrap(err, "parsing step files") } // Create temp files with the rendered content of step.Files so that we @@ -124,16 +131,16 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr for name, content := range files { fp, err := ioutil.TempFile(tempDir, "") if err != nil { - return nil, nil, errors.Wrap(err, "creating temporary file") + return execResult, errors.Wrap(err, "creating temporary file") } defer os.Remove(fp.Name()) if _, err := fp.WriteString(content); err != nil { - return nil, nil, errors.Wrap(err, "writing to temporary file") + return execResult, errors.Wrap(err, "writing to temporary file") } if err := fp.Close(); err != nil { - return nil, nil, errors.Wrap(err, "closing temporary file") + return execResult, errors.Wrap(err, "closing temporary file") } filesToMount[name] = fp @@ -142,20 +149,20 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr // Resolve step.Env given the current environment. stepEnv, err := step.Env.Resolve(os.Environ()) if err != nil { - return nil, nil, errors.Wrap(err, "resolving step environment") + return execResult, errors.Wrap(err, "resolving step environment") } // Render the step.Env variables as templates. env, err := renderStepMap(stepEnv, &stepContext) if err != nil { - return nil, nil, errors.Wrap(err, "parsing step environment") + return execResult, errors.Wrap(err, "parsing step environment") } reportProgress(runScript.String()) const workDir = "/work" workspaceOpts, err := workspace.DockerRunOpts(ctx, workDir) if err != nil { - return nil, nil, errors.Wrap(err, "getting Docker options for workspace") + return execResult, errors.Wrap(err, "getting Docker options for workspace") } args := append([]string{ "run", @@ -194,7 +201,7 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr if err != nil { logger.Logf("[Step %d] took %s; error running Docker container: %+v", i+1, elapsed, err) - return nil, nil, stepFailedErr{ + return execResult, stepFailedErr{ Err: err, Args: cmd.Args, Run: runScript.String(), @@ -209,23 +216,23 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr changes, err := workspace.Changes(ctx) if err != nil { - return nil, nil, errors.Wrap(err, "getting changed files in step") + return execResult, errors.Wrap(err, "getting changed files in step") } result := StepResult{files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer} stepContext.Step = result results[i] = result - setOutputs(step.Outputs, outputs, &stepContext) + setOutputs(step.Outputs, execResult.Outputs, &stepContext) } reportProgress("Calculating diff") diffOut, err := workspace.Diff(ctx) if err != nil { - return nil, nil, errors.Wrap(err, "git diff failed") + return execResult, errors.Wrap(err, "git diff failed") } - - return diffOut, outputs, err + execResult.Diff = string(diffOut) + return execResult, err } func setOutputs(stepOutputs Outputs, global map[string]interface{}, stepCtx *StepContext) error { From 8df2650cae32be623f164c2c2c7b13abe5cc483a Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 14 Jan 2021 15:37:04 +0100 Subject: [PATCH 11/19] Add 'steps' to changesetTemplate template variables --- internal/campaigns/executor.go | 13 +-- internal/campaigns/executor_test.go | 20 +++-- internal/campaigns/run_steps.go | 13 ++- internal/campaigns/templating.go | 110 +++++++++++++++----------- internal/campaigns/templating_test.go | 22 ++++++ 5 files changed, 121 insertions(+), 57 deletions(-) diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index e4e2ea18a5..fc7f15b466 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -280,7 +280,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } var specs []*ChangesetSpec - specs, err = createChangesetSpecs(task, result.Diff, result.Outputs, x.features) + specs, err = createChangesetSpecs(task, result, x.features) if err != nil { return err } @@ -338,7 +338,7 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { } // Build the changeset specs. - specs, err := createChangesetSpecs(task, result.Diff, result.Outputs, x.features) + specs, err := createChangesetSpecs(task, result, x.features) if err != nil { return err } @@ -404,7 +404,7 @@ func reachedTimeout(cmdCtx context.Context, err error) bool { return errors.Is(err, context.DeadlineExceeded) } -func createChangesetSpecs(task *Task, completeDiff string, outputs map[string]interface{}, features featureFlags) ([]*ChangesetSpec, error) { +func createChangesetSpecs(task *Task, result ExecutionResult, features featureFlags) ([]*ChangesetSpec, error) { repo := task.Repository.Name var authorName string @@ -422,7 +422,8 @@ func createChangesetSpecs(task *Task, completeDiff string, outputs map[string]in } tmplCtx := &ChangesetTemplateContext{ - Outputs: outputs, + Steps: result.ChangedFiles, + Outputs: result.Outputs, Repository: *task.Repository, } @@ -482,7 +483,7 @@ func createChangesetSpecs(task *Task, completeDiff string, outputs map[string]in } // TODO: Regarding 'defaultBranch', see comment above - diffsByBranch, err := groupFileDiffs(completeDiff, defaultBranch, groups) + diffsByBranch, err := groupFileDiffs(result.Diff, defaultBranch, groups) if err != nil { return specs, errors.Wrap(err, "grouping diffs failed") } @@ -491,7 +492,7 @@ func createChangesetSpecs(task *Task, completeDiff string, outputs map[string]in specs = append(specs, newSpec(branch, diff)) } } else { - specs = append(specs, newSpec(defaultBranch, string(completeDiff))) + specs = append(specs, newSpec(defaultBranch, result.Diff)) } return specs, nil diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 0a72002830..5df9d6c6f2 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -235,8 +235,13 @@ func TestExecutor_Integration(t *testing.T) { }, }, template: &ChangesetTemplate{ - Title: "myOutputName1=${{ outputs.myOutputName1}}", - Body: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", + Title: "myOutputName1=${{ outputs.myOutputName1}}", + Body: `myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }} +modified_files=${{ steps.modified_files }} +added_files=${{ steps.added_files }} +deleted_files=${{ steps.deleted_files }} +renamed_files=${{ steps.renamed_files }} +repository_name=${{ repository.name }}`, Branch: "templated-branch-${{ outputs.myOutputName3 }}", Commit: ExpandedGitCommitDescription{ Message: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", @@ -248,8 +253,13 @@ func TestExecutor_Integration(t *testing.T) { "templated-branch-cool-suffix": []string{"main.go"}, }, }, - wantTitle: "myOutputName1=main.go", - wantBody: "myOutputName1=main.go,myOutputName2=Hello World!", + wantTitle: "myOutputName1=main.go", + wantBody: `myOutputName1=main.go,myOutputName2=Hello World! +modified_files=[main.go] +added_files=[] +deleted_files=[] +renamed_files=[] +repository_name=github.com/sourcegraph/src-cli`, wantCommitMessage: "myOutputName1=main.go,myOutputName2=Hello World!", }, } @@ -325,7 +335,7 @@ func TestExecutor_Integration(t *testing.T) { t.Errorf("wrong title. want=%q, have=%q", tc.wantTitle, spec.Title) } if tc.wantBody != "" && spec.Body != tc.wantBody { - t.Errorf("wrong body. want=%q, have=%q", tc.wantBody, spec.Body) + t.Errorf("wrong body.\nwant=%q\nhave=%q\n", tc.wantBody, spec.Body) } if have, want := len(spec.Commits), 1; have != want { diff --git a/internal/campaigns/run_steps.go b/internal/campaigns/run_steps.go index ff11731179..d30b0a1129 100644 --- a/internal/campaigns/run_steps.go +++ b/internal/campaigns/run_steps.go @@ -20,7 +20,13 @@ import ( ) type ExecutionResult struct { - Diff string `json:"diff"` + // Diff is the produced by executing all steps. + Diff string `json:"diff"` + + // ChangedFiles are files that have been changed by all steps. + ChangedFiles *StepChanges `json:"changedFiles"` + + // Outputs are the outputs produced by all steps. Outputs map[string]interface{} `json:"outputs"` } @@ -231,7 +237,12 @@ func runSteps(ctx context.Context, rf RepoFetcher, wc WorkspaceCreator, repo *gr if err != nil { return execResult, errors.Wrap(err, "git diff failed") } + execResult.Diff = string(diffOut) + if len(results) > 0 && results[len(results)-1].files != nil { + execResult.ChangedFiles = results[len(results)-1].files + } + return execResult, err } diff --git a/internal/campaigns/templating.go b/internal/campaigns/templating.go index 218fc9cccb..d54bc72132 100644 --- a/internal/campaigns/templating.go +++ b/internal/campaigns/templating.go @@ -11,47 +11,6 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) -// ChangesetTemplateContext represents the contextual information available -// when rendering a field of the ChangesetTemplate as a template. -type ChangesetTemplateContext struct { - // TODO: This should also have `Steps` - Outputs map[string]interface{} - Repository graphql.Repository -} - -// ToFuncMap returns a template.FuncMap to access fields on the StepContext in a -// text/template. -func (tmplCtx *ChangesetTemplateContext) ToFuncMap() template.FuncMap { - return template.FuncMap{ - "join": strings.Join, - "split": strings.Split, - "repository": func() map[string]interface{} { - return map[string]interface{}{ - "search_result_paths": tmplCtx.Repository.SearchResultPaths(), - "name": tmplCtx.Repository.Name, - } - }, - "outputs": func() map[string]interface{} { - return tmplCtx.Outputs - }, - } -} - -func renderChangesetTemplateField(name, tmpl string, tmplCtx *ChangesetTemplateContext) (string, error) { - var out bytes.Buffer - - t, err := template.New(name).Delims("${{", "}}").Funcs(tmplCtx.ToFuncMap()).Parse(tmpl) - if err != nil { - return "", err - } - - if err := t.Execute(&out, tmplCtx); err != nil { - return "", err - } - - return out.String(), nil -} - func renderStepTemplate(name, tmpl string, out io.Writer, stepCtx *StepContext) error { t, err := parseAsTemplate(name, tmpl, stepCtx) if err != nil { @@ -108,6 +67,9 @@ func (stepCtx *StepContext) ToFuncMap() template.FuncMap { "stdout": "", "stderr": "", } + if res == nil { + return m + } m["modified_files"] = res.ModifiedFiles() m["added_files"] = res.AddedFiles() @@ -159,10 +121,10 @@ type StepResult struct { // StepChanges are the changes made to files by a previous step in a repository. type StepChanges struct { - Modified []string - Added []string - Deleted []string - Renamed []string + Modified []string `json:"modified"` + Added []string `json:"added"` + Deleted []string `json:"deleted"` + Renamed []string `json:"renamed"` } // ModifiedFiles returns the files modified by a step. @@ -197,6 +159,64 @@ func (r StepResult) RenamedFiles() []string { return []string{} } +// ChangesetTemplateContext represents the contextual information available +// when rendering a field of the ChangesetTemplate as a template. +type ChangesetTemplateContext struct { + // Steps are the changes made by all steps that were executed. + Steps *StepChanges + + // Outputs are the outputs defined and initialized by the steps. + Outputs map[string]interface{} + + // Repository is the repository in which the steps were executed. + Repository graphql.Repository +} + +// ToFuncMap returns a template.FuncMap to access fields on the StepContext in a +// text/template. +func (tmplCtx *ChangesetTemplateContext) ToFuncMap() template.FuncMap { + return template.FuncMap{ + "join": strings.Join, + "split": strings.Split, + "repository": func() map[string]interface{} { + return map[string]interface{}{ + "search_result_paths": tmplCtx.Repository.SearchResultPaths(), + "name": tmplCtx.Repository.Name, + } + }, + "outputs": func() map[string]interface{} { + return tmplCtx.Outputs + }, + "steps": func() map[string]interface{} { + // Wrap the *StepChanges in a StepResult so we can use nil-safe + // methods. + res := StepResult{files: tmplCtx.Steps} + + return map[string]interface{}{ + "modified_files": res.ModifiedFiles(), + "added_files": res.AddedFiles(), + "deleted_files": res.DeletedFiles(), + "renamed_files": res.RenamedFiles(), + } + }, + } +} + +func renderChangesetTemplateField(name, tmpl string, tmplCtx *ChangesetTemplateContext) (string, error) { + var out bytes.Buffer + + t, err := template.New(name).Delims("${{", "}}").Funcs(tmplCtx.ToFuncMap()).Parse(tmpl) + if err != nil { + return "", err + } + + if err := t.Execute(&out, tmplCtx); err != nil { + return "", err + } + + return out.String(), nil +} + func parseGitStatus(out []byte) (StepChanges, error) { result := StepChanges{} diff --git a/internal/campaigns/templating_test.go b/internal/campaigns/templating_test.go index 5236ad43c4..9961ee28e7 100644 --- a/internal/campaigns/templating_test.go +++ b/internal/campaigns/templating_test.go @@ -209,6 +209,12 @@ func TestRenderChangesetTemplateField(t *testing.T) { "main.go": true, }, }, + Steps: &StepChanges{ + Modified: []string{"modified-file.txt"}, + Added: []string{"added-file.txt"}, + Deleted: []string{"deleted-file.txt"}, + Renamed: []string{"renamed-file.txt"}, + }, } tests := []struct { @@ -224,11 +230,19 @@ func TestRenderChangesetTemplateField(t *testing.T) { ${{ repository.name }} ${{ outputs.lastLine }} ${{ index outputs.project.env 1 }} +${{ steps.modified_files }} +${{ steps.added_files }} +${{ steps.deleted_files }} +${{ steps.renamed_files }} `, want: `README.md main.go github.com/sourcegraph/src-cli lastLine is this CGO_ENABLED=0 +[modified-file.txt] +[added-file.txt] +[deleted-file.txt] +[renamed-file.txt] `, }, { @@ -238,11 +252,19 @@ CGO_ENABLED=0 ${{ repository.name }} ${{ outputs.lastLine }} ${{ outputs.project }} +${{ steps.modified_files }} +${{ steps.added_files }} +${{ steps.deleted_files }} +${{ steps.renamed_files }} `, want: ` +[] +[] +[] +[] `, }, } From 585abbd5114d6f1eed1ff2c92764c17beb4c6637 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 10:29:11 +0100 Subject: [PATCH 12/19] Support templating in changesetTemplate.author fields --- internal/campaigns/executor.go | 23 +++++++++++++++-------- internal/campaigns/executor_test.go | 28 +++++++++++++++++++--------- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index fc7f15b466..9897b4385e 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -407,6 +407,12 @@ func reachedTimeout(cmdCtx context.Context, err error) bool { func createChangesetSpecs(task *Task, result ExecutionResult, features featureFlags) ([]*ChangesetSpec, error) { repo := task.Repository.Name + tmplCtx := &ChangesetTemplateContext{ + Steps: result.ChangedFiles, + Outputs: result.Outputs, + Repository: *task.Repository, + } + var authorName string var authorEmail string @@ -417,14 +423,15 @@ func createChangesetSpecs(task *Task, result ExecutionResult, features featureFl authorEmail = "campaigns@sourcegraph.com" } } else { - authorName = task.Template.Commit.Author.Name - authorEmail = task.Template.Commit.Author.Email - } - - tmplCtx := &ChangesetTemplateContext{ - Steps: result.ChangedFiles, - Outputs: result.Outputs, - Repository: *task.Repository, + var err error + authorName, err = renderChangesetTemplateField("authorName", task.Template.Commit.Author.Name, tmplCtx) + if err != nil { + return nil, err + } + authorEmail, err = renderChangesetTemplateField("authorEmail", task.Template.Commit.Author.Email, tmplCtx) + if err != nil { + return nil, err + } } title, err := renderChangesetTemplateField("title", task.Template.Title, tmplCtx) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 5df9d6c6f2..f0c714cd23 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -71,6 +71,8 @@ func TestExecutor_Integration(t *testing.T) { wantTitle string wantBody string wantCommitMessage string + wantAuthorName string + wantAuthorEmail string wantErrInclude string }{ @@ -245,6 +247,10 @@ repository_name=${{ repository.name }}`, Branch: "templated-branch-${{ outputs.myOutputName3 }}", Commit: ExpandedGitCommitDescription{ Message: "myOutputName1=${{ outputs.myOutputName1}},myOutputName2=${{ outputs.myOutputName2.thisStepStdout }}", + Author: &GitCommitAuthor{ + Name: "myOutputName1=${{ outputs.myOutputName1}}", + Email: "myOutputName1=${{ outputs.myOutputName1}}", + }, }, }, @@ -261,6 +267,8 @@ deleted_files=[] renamed_files=[] repository_name=github.com/sourcegraph/src-cli`, wantCommitMessage: "myOutputName1=main.go,myOutputName2=Hello World!", + wantAuthorName: "myOutputName1=main.go", + wantAuthorEmail: "myOutputName1=main.go", }, } @@ -331,19 +339,21 @@ repository_name=github.com/sourcegraph/src-cli`, } for _, spec := range specs { - if tc.wantTitle != "" && spec.Title != tc.wantTitle { - t.Errorf("wrong title. want=%q, have=%q", tc.wantTitle, spec.Title) - } - if tc.wantBody != "" && spec.Body != tc.wantBody { - t.Errorf("wrong body.\nwant=%q\nhave=%q\n", tc.wantBody, spec.Body) - } - if have, want := len(spec.Commits), 1; have != want { t.Fatalf("wrong number of commits. want=%d, have=%d", want, have) } - if tc.wantCommitMessage != "" && spec.Commits[0].Message != tc.wantCommitMessage { - t.Errorf("wrong commitmessage. want=%q, have=%q", tc.wantCommitMessage, spec.Commits[0].Message) + attrs := []struct{ name, want, have string }{ + {name: "title", want: tc.wantTitle, have: spec.Title}, + {name: "body", want: tc.wantBody, have: spec.Body}, + {name: "commit.Message", want: tc.wantCommitMessage, have: spec.Commits[0].Message}, + {name: "commit.AuthorEmail", want: tc.wantAuthorEmail, have: spec.Commits[0].AuthorEmail}, + {name: "commit.AuthorName", want: tc.wantAuthorName, have: spec.Commits[0].AuthorName}, + } + for _, attr := range attrs { + if attr.want != "" && attr.have != attr.want { + t.Errorf("wrong %q attribute. want=%q, have=%q", attr.name, attr.want, attr.have) + } } wantFiles, ok := tc.wantFilesChanged[spec.BaseRepository] From 5febcce0f41392b6d7b37fd82c9f7950b45603f2 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 10:38:41 +0100 Subject: [PATCH 13/19] Fix doc comment --- internal/campaigns/templating.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/campaigns/templating.go b/internal/campaigns/templating.go index d54bc72132..417b5a8799 100644 --- a/internal/campaigns/templating.go +++ b/internal/campaigns/templating.go @@ -40,7 +40,7 @@ func renderStepMap(m map[string]string, stepCtx *StepContext) (map[string]string return rendered, nil } -// StepContext represents the contextual information available when render a +// StepContext represents the contextual information available when rendering a // step's fields, such as "run" or "outputs", as templates. type StepContext struct { // Outputs are the outputs set by the current and all previous steps. From c4fc282859af048b02b0844f83b1ed9c240f6f99 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 10:38:51 +0100 Subject: [PATCH 14/19] Only use yaml.v3 in internal/campaigns --- internal/campaigns/execution_cache_test.go | 2 +- internal/campaigns/templating_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go index 8ab99fa7a6..5857d60303 100644 --- a/internal/campaigns/execution_cache_test.go +++ b/internal/campaigns/execution_cache_test.go @@ -4,7 +4,7 @@ import ( "os" "testing" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" ) const testExecutionCacheKeyEnv = "TEST_EXECUTION_CACHE_KEY_ENV" diff --git a/internal/campaigns/templating_test.go b/internal/campaigns/templating_test.go index 9961ee28e7..ace3ff2997 100644 --- a/internal/campaigns/templating_test.go +++ b/internal/campaigns/templating_test.go @@ -6,7 +6,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/src-cli/internal/campaigns/graphql" - "gopkg.in/yaml.v2" + "gopkg.in/yaml.v3" ) func TestParseGitStatus(t *testing.T) { From 219579b92ba00749e66cfbc270a2853bf803df6e Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 11:13:33 +0100 Subject: [PATCH 15/19] Add tests for ExecutionCacheTest --- internal/campaigns/execution_cache_test.go | 104 +++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go index 5857d60303..32d88024fb 100644 --- a/internal/campaigns/execution_cache_test.go +++ b/internal/campaigns/execution_cache_test.go @@ -1,9 +1,13 @@ package campaigns import ( + "context" + "io/ioutil" "os" "testing" + "github.com/google/go-cmp/cmp" + "github.com/sourcegraph/src-cli/internal/campaigns/graphql" "gopkg.in/yaml.v3" ) @@ -86,3 +90,103 @@ func TestExecutionCacheKey(t *testing.T) { t.Errorf("unexpected change in key: initial=%q have=%q", initial, have) } } + +const testDiff = `diff --git a/README.md b/README.md +new file mode 100644 +index 0000000..3363c39 +--- /dev/null ++++ b/README.md +@@ -0,0 +1,3 @@ ++# README ++ ++This is the readme +` + +func TestExecutionDiskCache_Get(t *testing.T) { + ctx := context.Background() + + cacheTmpDir := func(t *testing.T) string { + testTempDir, err := ioutil.TempDir("", "execution-disk-cache-test-*") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Remove(testTempDir) }) + + return testTempDir + } + + cacheKey1 := ExecutionCacheKey{Task: &Task{ + Repository: &graphql.Repository{Name: "src-cli"}, + Steps: []Step{ + {Run: "echo 'Hello World'", Container: "alpine:3"}, + }, + }} + + cacheKey2 := ExecutionCacheKey{Task: &Task{ + Repository: &graphql.Repository{Name: "documentation"}, + Steps: []Step{ + {Run: "echo 'Hello World'", Container: "alpine:3"}, + }, + }} + + value := ExecutionResult{ + Diff: testDiff, + ChangedFiles: &StepChanges{ + Added: []string{"README.md"}, + }, + Outputs: map[string]interface{}{}, + } + + t.Run("cache contains v1 cache file", func(t *testing.T) { + cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} + + // Empty cache, no hits + assertCacheMiss(t, cache, cacheKey1) + assertCacheMiss(t, cache, cacheKey2) + + // Set the cache + if err := cache.Set(ctx, cacheKey1, value); err != nil { + t.Fatalf("cache.Set returned unexpected error: %s", err) + } + + // Cache hit + assertCacheHit(t, cache, cacheKey1, value) + + // Cache miss due to different key + assertCacheMiss(t, cache, cacheKey2) + + // Cache miss due to cleared cache + if err := cache.Clear(ctx, cacheKey1); err != nil { + t.Fatalf("cache.Get returned unexpected error: %s", err) + } + assertCacheMiss(t, cache, cacheKey1) + }) +} + +func assertCacheHit(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey, want ExecutionResult) { + t.Helper() + + have, found, err := c.Get(context.Background(), k) + if err != nil { + t.Fatalf("cache.Get returned unexpected error: %s", err) + } + if !found { + t.Fatalf("cache miss when hit was expected") + } + + if diff := cmp.Diff(have, want); diff != "" { + t.Errorf("wrong cached result (-have +want):\n\n%s", diff) + } +} + +func assertCacheMiss(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey) { + t.Helper() + + _, found, err := c.Get(context.Background(), k) + if err != nil { + t.Fatalf("cache.Get returned unexpected error: %s", err) + } + if found { + t.Fatalf("cache hit when miss was expected") + } +} From 00111370db2f8c810e3f5c74ea2b49c0d148130f Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 12:52:11 +0100 Subject: [PATCH 16/19] Add proper backwards-compatibility to ExecutionDiskCache --- internal/campaigns/execution_cache.go | 115 +++++++++++--- internal/campaigns/execution_cache_test.go | 169 ++++++++++++++++++++- 2 files changed, 263 insertions(+), 21 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index 1dc03d6aa1..c5900e221d 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -9,6 +9,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sort" "strings" "github.com/pkg/errors" @@ -71,13 +72,15 @@ type ExecutionDiskCache struct { Dir string } +const cacheFileExt = ".v3.json" + func (c ExecutionDiskCache) cacheFilePath(key ExecutionCacheKey) (string, error) { keyString, err := key.Key() if err != nil { return "", errors.Wrap(err, "calculating execution cache key") } - return filepath.Join(c.Dir, keyString+".v3.json"), nil + return filepath.Join(c.Dir, keyString+cacheFileExt), nil } func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (ExecutionResult, bool, error) { @@ -88,32 +91,106 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (Exe return result, false, err } + // We try to be backwards compatible and see if we also find older cache + // files. + // + // There are three different cache versions out in the wild and to be + // backwards compatible we read all of them. + // + // In Sourcegraph/src-cli 3.26 we can remove the code here and simply read + // the cache from `path`, since all the old cache files should be deleted + // until then. + globPattern := strings.TrimSuffix(path, cacheFileExt) + ".*" + matches, err := filepath.Glob(globPattern) + if err != nil { + return result, false, err + } + + switch len(matches) { + case 0: + // Nothing found + return result, false, nil + case 1: + // One cache file found + if err := c.readCacheFile(matches[0], &result); err != nil { + return result, false, err + } + + // If it's an old cache file, we rewrite the cache and delete the old file + if isOldCacheFile(matches[0]) { + if err := c.Set(ctx, key, result); err != nil { + return result, false, errors.Wrap(err, "failed to rewrite cache in new format") + } + if err := os.Remove(matches[0]); err != nil { + return result, false, errors.Wrap(err, "failed to remove old cache file") + } + } + + return result, true, err + default: + // More than one cache file found. + // Sort them so that we'll can possibly read from the one with the most + // current version. + sortCacheFiles(matches) + + newest := matches[0] + toDelete := matches[1:] + + // Read from newest + if err := c.readCacheFile(newest, &result); err != nil { + return result, false, err + } + + // If the newest was also an older version, we write a new version... + if isOldCacheFile(newest) { + if err := c.Set(ctx, key, result); err != nil { + return result, false, errors.Wrap(err, "failed to rewrite cache in new format") + } + // ... and mark the file also as to-be-deleted + toDelete = append(toDelete, newest) + } + + // Now we clean up the old ones + for _, path := range toDelete { + if err := os.Remove(path); err != nil { + return result, false, errors.Wrap(err, "failed to remove old cache file") + } + } + + return result, true, nil + } +} + +// sortCacheFiles sorts cache file paths by their "version", so that files +// ending in `cacheFileExt` are first. +func sortCacheFiles(paths []string) { + sort.Slice(paths, func(i, j int) bool { + return !isOldCacheFile(paths[i]) && isOldCacheFile(paths[j]) + }) +} + +func isOldCacheFile(path string) bool { return !strings.HasSuffix(path, cacheFileExt) } + +func (c ExecutionDiskCache) readCacheFile(path string, result *ExecutionResult) error { data, err := ioutil.ReadFile(path) if err != nil { if os.IsNotExist(err) { err = nil // treat as not-found } - return result, false, err + return err } - // There are now three different cache versions out in the wild and to be - // backwards compatible we read all of them. - // 2-step plan for taming this: - // 1) February 2021: deprecate old caches by deleting the files when - // detected and reporting as cache-miss. - // 2) May 2021 (two releases later): remove handling of these files from - // this function switch { case strings.HasSuffix(path, ".v3.json"): // v3 of the cache: we cache the diff and the outputs produced by the step. - if err := json.Unmarshal(data, &result); err != nil { + if err := json.Unmarshal(data, result); err != nil { // Delete the invalid data to avoid causing an error for next time. if err := os.Remove(path); err != nil { - return result, false, errors.Wrap(err, "while deleting cache file with invalid JSON") + return errors.Wrap(err, "while deleting cache file with invalid JSON") } - return result, false, errors.Wrapf(err, "reading cache file %s", path) + return errors.Wrapf(err, "reading cache file %s", path) } - return result, true, nil + return nil case strings.HasSuffix(path, ".diff"): // v2 of the cache: we only cached the diff, since that's the @@ -121,7 +198,7 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (Exe result.Diff = string(data) result.Outputs = map[string]interface{}{} - return result, true, nil + return nil case strings.HasSuffix(path, ".json"): // v1 of the cache: we cached the complete ChangesetSpec instead of just the diffs. @@ -129,21 +206,21 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (Exe if err := json.Unmarshal(data, &spec); err != nil { // Delete the invalid data to avoid causing an error for next time. if err := os.Remove(path); err != nil { - return result, false, errors.Wrap(err, "while deleting cache file with invalid JSON") + return errors.Wrap(err, "while deleting cache file with invalid JSON") } - return result, false, errors.Wrapf(err, "reading cache file %s", path) + return errors.Wrapf(err, "reading cache file %s", path) } if len(spec.Commits) != 1 { - return result, false, errors.New("cached result has no commits") + return errors.New("cached result has no commits") } result.Diff = spec.Commits[0].Diff result.Outputs = map[string]interface{}{} - return result, true, nil + return nil } - return result, false, fmt.Errorf("unknown file format for cache file %q", path) + return fmt.Errorf("unknown file format for cache file %q", path) } func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, result ExecutionResult) error { diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go index 32d88024fb..7cd0154ce3 100644 --- a/internal/campaigns/execution_cache_test.go +++ b/internal/campaigns/execution_cache_test.go @@ -2,8 +2,10 @@ package campaigns import ( "context" + "encoding/json" "io/ioutil" "os" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" @@ -102,7 +104,7 @@ index 0000000..3363c39 +This is the readme ` -func TestExecutionDiskCache_Get(t *testing.T) { +func TestExecutionDiskCache(t *testing.T) { ctx := context.Background() cacheTmpDir := func(t *testing.T) string { @@ -137,7 +139,7 @@ func TestExecutionDiskCache_Get(t *testing.T) { Outputs: map[string]interface{}{}, } - t.Run("cache contains v1 cache file", func(t *testing.T) { + t.Run("cache contains v3 cache file", func(t *testing.T) { cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} // Empty cache, no hits @@ -161,6 +163,169 @@ func TestExecutionDiskCache_Get(t *testing.T) { } assertCacheMiss(t, cache, cacheKey1) }) + + t.Run("cache contains v1 cache file", func(t *testing.T) { + cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} + + // Empty cache, no hit + assertCacheMiss(t, cache, cacheKey1) + + // Simulate old cache file lying around in cache + oldFilePath := writeV1CacheFile(t, cache, cacheKey1, testDiff) + + // Cache hit, but only for the diff + onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} + assertCacheHit(t, cache, cacheKey1, onlyDiff) + + // And the old file should be deleted + assertFileDeleted(t, oldFilePath) + // .. but we should still get a cache hit, because we rewrote the cache + assertCacheHit(t, cache, cacheKey1, onlyDiff) + }) + + t.Run("cache contains v2 cache file", func(t *testing.T) { + cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} + + // Empty cache, no hit + assertCacheMiss(t, cache, cacheKey1) + + // Simulate old cache file lying around in cache + oldFilePath := writeV2CacheFile(t, cache, cacheKey1, testDiff) + + // Now we get a cache hit, but only for the diff ( + onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} + assertCacheHit(t, cache, cacheKey1, onlyDiff) + + // And the old file should be deleted + assertFileDeleted(t, oldFilePath) + // .. but we should still get a cache hit, because we rewrote the cache + assertCacheHit(t, cache, cacheKey1, onlyDiff) + }) + + t.Run("cache contains one old and one v3 cache file", func(t *testing.T) { + cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} + + // Simulate v2 and v3 files in cache + oldFilePath := writeV1CacheFile(t, cache, cacheKey1, testDiff) + + if err := cache.Set(ctx, cacheKey1, value); err != nil { + t.Fatalf("cache.Set returned unexpected error: %s", err) + } + + // Cache hit + assertCacheHit(t, cache, cacheKey1, value) + + // And the old file should be deleted + assertFileDeleted(t, oldFilePath) + }) + + t.Run("cache contains multiple old cache files", func(t *testing.T) { + cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} + + // Simulate v1 and v2 files in cache + oldFilePath1 := writeV1CacheFile(t, cache, cacheKey1, testDiff) + oldFilePath2 := writeV1CacheFile(t, cache, cacheKey1, testDiff) + + // Now we get a cache hit, but only for the diff ( + onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} + assertCacheHit(t, cache, cacheKey1, onlyDiff) + + // And the old files should be deleted + assertFileDeleted(t, oldFilePath1) + assertFileDeleted(t, oldFilePath2) + // .. but we should still get a cache hit, because we rewrote the cache + assertCacheHit(t, cache, cacheKey1, onlyDiff) + }) +} + +func TestSortCacheFiles(t *testing.T) { + tests := []struct { + paths []string + want []string + }{ + { + paths: []string{"file.v3.json", "file.diff", "file.json"}, + want: []string{"file.v3.json", "file.diff", "file.json"}, + }, + { + paths: []string{"file.json", "file.diff", "file.v3.json"}, + want: []string{"file.v3.json", "file.json", "file.diff"}, + }, + { + paths: []string{"file.diff", "file.v3.json"}, + want: []string{"file.v3.json", "file.diff"}, + }, + { + paths: []string{"file1.v3.json", "file2.v3.json"}, + want: []string{"file1.v3.json", "file2.v3.json"}, + }, + } + + for _, tt := range tests { + sortCacheFiles(tt.paths) + if diff := cmp.Diff(tt.paths, tt.want); diff != "" { + t.Errorf("wrong cached result (-have +want):\n\n%s", diff) + } + } +} + +func assertFileDeleted(t *testing.T, path string) { + t.Helper() + if _, err := os.Stat(path); err == nil { + t.Fatalf("file exists: %s", path) + } else if os.IsNotExist(err) { + // Seems to be deleted, all good + } else { + t.Fatalf("could not determine whether file exists: %s", err) + } +} + +func writeV1CacheFile(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey, diff string) (path string) { + t.Helper() + + hashedKey, err := k.Key() + if err != nil { + t.Fatalf("failed to hash cacheKey: %s", err) + } + // The v1 file format ended in .json + path = filepath.Join(c.Dir, hashedKey+".json") + + // v1 contained a fully serialized ChangesetSpec + spec := ChangesetSpec{CreatedChangeset: &CreatedChangeset{ + Commits: []GitCommitDescription{ + {Diff: testDiff}, + }, + }} + + raw, err := json.Marshal(&spec) + if err != nil { + t.Fatal(err) + } + + if err := ioutil.WriteFile(path, raw, 0600); err != nil { + t.Fatalf("writing the cache file failed: %s", err) + } + + return path +} + +func writeV2CacheFile(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey, diff string) (path string) { + t.Helper() + + hashedKey, err := k.Key() + if err != nil { + t.Fatalf("failed to hash cacheKey: %s", err) + } + + // The v2 file format ended in .json + path = filepath.Join(c.Dir, hashedKey+".diff") + + // v2 contained only a diff + if err := ioutil.WriteFile(path, []byte(diff), 0600); err != nil { + t.Fatalf("writing the cache file failed: %s", err) + } + + return path } func assertCacheHit(t *testing.T, c ExecutionDiskCache, k ExecutionCacheKey, want ExecutionResult) { From e56c34a72e7f8f82377a1c7622c3cc1facaa1e03 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 12:53:23 +0100 Subject: [PATCH 17/19] Remove unneeded code --- internal/campaigns/execution_cache.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index c5900e221d..d2b7dd6105 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -174,9 +174,6 @@ func isOldCacheFile(path string) bool { return !strings.HasSuffix(path, cacheFil func (c ExecutionDiskCache) readCacheFile(path string, result *ExecutionResult) error { data, err := ioutil.ReadFile(path) if err != nil { - if os.IsNotExist(err) { - err = nil // treat as not-found - } return err } From 897c427e8200c32a7405d9f3d1993ce720d87f44 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 12:59:57 +0100 Subject: [PATCH 18/19] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83e0219bad..bacfd97053 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ All notable changes to `src-cli` are documented in this file. ### Added +- `steps` in campaign specs can now have [`outputs`](https://docs.sourcegraph.com/campaigns/references/campaign_spec_yaml_reference#steps-outputs) that support [templating](https://docs.sourcegraph.com/campaigns/references/campaign_spec_templating). [#424](https://github.com/sourcegraph/src-cli/pull/424) +- `changesetTemplate` fields in campaign specs now also support [templating](https://docs.sourcegraph.com/campaigns/references/campaign_spec_templating). [#424](https://github.com/sourcegraph/src-cli/pull/424) + ### Changed ### Fixed From 90575dae77df20a0ea0c96d8139ebaa37f03dfcf Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 15 Jan 2021 15:15:28 +0100 Subject: [PATCH 19/19] Add comment about lossiness of cache conversion --- internal/campaigns/execution_cache.go | 4 ++++ internal/campaigns/execution_cache_test.go | 13 ++++++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/campaigns/execution_cache.go b/internal/campaigns/execution_cache.go index d2b7dd6105..5a5be4a7b5 100644 --- a/internal/campaigns/execution_cache.go +++ b/internal/campaigns/execution_cache.go @@ -127,6 +127,7 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (Exe } return result, true, err + default: // More than one cache file found. // Sort them so that we'll can possibly read from the one with the most @@ -194,6 +195,8 @@ func (c ExecutionDiskCache) readCacheFile(path string, result *ExecutionResult) // only bit of data we were interested in. result.Diff = string(data) result.Outputs = map[string]interface{}{} + // Conversion is lossy, though: we don't populate result.StepChanges. + result.ChangedFiles = &StepChanges{} return nil @@ -213,6 +216,7 @@ func (c ExecutionDiskCache) readCacheFile(path string, result *ExecutionResult) result.Diff = spec.Commits[0].Diff result.Outputs = map[string]interface{}{} + result.ChangedFiles = &StepChanges{} return nil } diff --git a/internal/campaigns/execution_cache_test.go b/internal/campaigns/execution_cache_test.go index 7cd0154ce3..95c4581c6a 100644 --- a/internal/campaigns/execution_cache_test.go +++ b/internal/campaigns/execution_cache_test.go @@ -139,6 +139,12 @@ func TestExecutionDiskCache(t *testing.T) { Outputs: map[string]interface{}{}, } + onlyDiff := ExecutionResult{ + Diff: testDiff, + ChangedFiles: &StepChanges{}, + Outputs: map[string]interface{}{}, + } + t.Run("cache contains v3 cache file", func(t *testing.T) { cache := ExecutionDiskCache{Dir: cacheTmpDir(t)} @@ -174,7 +180,6 @@ func TestExecutionDiskCache(t *testing.T) { oldFilePath := writeV1CacheFile(t, cache, cacheKey1, testDiff) // Cache hit, but only for the diff - onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} assertCacheHit(t, cache, cacheKey1, onlyDiff) // And the old file should be deleted @@ -192,8 +197,7 @@ func TestExecutionDiskCache(t *testing.T) { // Simulate old cache file lying around in cache oldFilePath := writeV2CacheFile(t, cache, cacheKey1, testDiff) - // Now we get a cache hit, but only for the diff ( - onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} + // Now we get a cache hit, but only for the diff assertCacheHit(t, cache, cacheKey1, onlyDiff) // And the old file should be deleted @@ -226,8 +230,7 @@ func TestExecutionDiskCache(t *testing.T) { oldFilePath1 := writeV1CacheFile(t, cache, cacheKey1, testDiff) oldFilePath2 := writeV1CacheFile(t, cache, cacheKey1, testDiff) - // Now we get a cache hit, but only for the diff ( - onlyDiff := ExecutionResult{Diff: testDiff, Outputs: map[string]interface{}{}} + // Now we get a cache hit, but only for the diff assertCacheHit(t, cache, cacheKey1, onlyDiff) // And the old files should be deleted