From d6bd10aeb9901d29795b142b504cd8655c05731c Mon Sep 17 00:00:00 2001 From: Erik Seliger Date: Wed, 1 Sep 2021 12:36:19 +0200 Subject: [PATCH] Make steps resolution part of workspaces detection --- internal/batches/service/build_tasks.go | 45 +---- internal/batches/service/build_tasks_test.go | 138 --------------- internal/batches/service/workspaces.go | 69 ++++++++ internal/batches/service/workspaces_test.go | 173 +++++++++++++++---- 4 files changed, 219 insertions(+), 206 deletions(-) delete mode 100644 internal/batches/service/build_tasks_test.go diff --git a/internal/batches/service/build_tasks.go b/internal/batches/service/build_tasks.go index 61e6fcecac..832a7b6391 100644 --- a/internal/batches/service/build_tasks.go +++ b/internal/batches/service/build_tasks.go @@ -8,7 +8,6 @@ import ( "github.com/sourcegraph/sourcegraph/lib/batches/template" "github.com/sourcegraph/src-cli/internal/batches/executor" "github.com/sourcegraph/src-cli/internal/batches/graphql" - "github.com/sourcegraph/src-cli/internal/batches/util" ) // buildTasks returns tasks for all the workspaces determined for the given spec. @@ -24,56 +23,24 @@ func buildTasks(ctx context.Context, spec *batcheslib.BatchSpec, repos []*graphq if !ok { return nil, errors.New("invalid state, didn't find repo for workspace definition") } - t, ok, err := buildTask(spec, repo, ws.Path, ws.OnlyFetchWorkspace) + + t, err := buildTask(spec, repo, ws.Path, ws.OnlyFetchWorkspace, ws.Steps) if err != nil { return nil, err } - if ok { - tasks = append(tasks, t) - } + tasks = append(tasks, t) } return tasks, nil } -func buildTask(spec *batcheslib.BatchSpec, r *graphql.Repository, path string, onlyWorkspace bool) (*executor.Task, bool, error) { +func buildTask(spec *batcheslib.BatchSpec, r *graphql.Repository, path string, onlyWorkspace bool, steps []batcheslib.Step) (*executor.Task, error) { batchChange := template.BatchChangeAttributes{ Name: spec.Name, Description: spec.Description, } - taskSteps := []batcheslib.Step{} - for _, step := range spec.Steps { - // If no if condition is given, just go ahead and add the step to the list. - if step.IfCondition() == "" { - taskSteps = append(taskSteps, step) - continue - } - - stepCtx := &template.StepContext{ - Repository: util.GraphQLRepoToTemplatingRepo(r), - BatchChange: batchChange, - } - static, boolVal, err := template.IsStaticBool(step.IfCondition(), stepCtx) - if err != nil { - return nil, false, err - } - - // If we could evaluate the condition statically and the resulting - // boolean is false, we don't add that step. - if !static { - taskSteps = append(taskSteps, step) - } else if boolVal { - taskSteps = append(taskSteps, step) - } - } - - // If the task doesn't have any steps we don't need to execute it - if len(taskSteps) == 0 { - return nil, false, nil - } - // "." means the path is root, but in the executor we use "" to signify root if path == "." { path = "" @@ -82,11 +49,11 @@ func buildTask(spec *batcheslib.BatchSpec, r *graphql.Repository, path string, o return &executor.Task{ Repository: r, Path: path, - Steps: taskSteps, + Steps: steps, OnlyFetchWorkspace: onlyWorkspace, TransformChanges: spec.TransformChanges, Template: spec.ChangesetTemplate, BatchChangeAttributes: &batchChange, - }, true, nil + }, nil } diff --git a/internal/batches/service/build_tasks_test.go b/internal/batches/service/build_tasks_test.go deleted file mode 100644 index 73c3c8fe5e..0000000000 --- a/internal/batches/service/build_tasks_test.go +++ /dev/null @@ -1,138 +0,0 @@ -package service - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - batcheslib "github.com/sourcegraph/sourcegraph/lib/batches" -) - -func TestBuildTask_IfConditions(t *testing.T) { - tests := map[string]struct { - spec *batcheslib.BatchSpec - - wantSteps []batcheslib.Step - }{ - "no if": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1"}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1"}, - }, - }, - - "if has static true value": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: "true"}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1", If: "true"}, - }, - }, - - "one of many steps has if with static true value": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1"}, - {Run: "echo 2", If: "true"}, - {Run: "echo 3"}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1"}, - {Run: "echo 2", If: "true"}, - {Run: "echo 3"}, - }, - }, - - "if has static non-true value": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: "this is not true"}, - }, - }, - wantSteps: nil, - }, - - "one of many steps has if with static non-true value": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1"}, - {Run: "echo 2", If: "every type system needs generics"}, - {Run: "echo 3"}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1"}, - {Run: "echo 3"}, - }, - }, - - "if expression that can be partially evaluated to true": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ matches repository.name "github.com/sourcegraph/src*" }}`}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ matches repository.name "github.com/sourcegraph/src*" }}`}, - }, - }, - - "if expression that can be partially evaluated to false": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ matches repository.name "horse" }}`}, - }, - }, - wantSteps: nil, - }, - - "one of many steps has if expression that can be evaluated to true": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ matches repository.name "horse" }}`}, - }, - }, - wantSteps: nil, - }, - - "if expression that can NOT be partially evaluated": { - spec: &batcheslib.BatchSpec{ - Steps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ eq outputs.value "foobar" }}`}, - }, - }, - wantSteps: []batcheslib.Step{ - {Run: "echo 1", If: `${{ eq outputs.value "foobar" }}`}, - }, - }, - } - - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - task, ok, err := buildTask(tt.spec, testRepo1, "", false) - if err != nil { - t.Fatalf("unexpected err: %s", err) - } - - if !ok { - if tt.wantSteps != nil { - t.Fatalf("no task built, but steps expected") - } - return - } - - opts := cmpopts.IgnoreUnexported(batcheslib.Step{}) - if diff := cmp.Diff(tt.wantSteps, task.Steps, opts); diff != "" { - t.Errorf("mismatch (-want +got):\n%s", diff) - } - }) - } -} diff --git a/internal/batches/service/workspaces.go b/internal/batches/service/workspaces.go index 1c2e2b84e7..81b2b3efc1 100644 --- a/internal/batches/service/workspaces.go +++ b/internal/batches/service/workspaces.go @@ -3,16 +3,21 @@ package service import ( "context" "fmt" + "sort" "github.com/gobwas/glob" + "github.com/pkg/errors" batcheslib "github.com/sourcegraph/sourcegraph/lib/batches" + "github.com/sourcegraph/sourcegraph/lib/batches/template" "github.com/sourcegraph/src-cli/internal/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" + "github.com/sourcegraph/src-cli/internal/batches/util" ) type RepoWorkspace struct { RepoID string Path string + Steps []batcheslib.Step OnlyFetchWorkspace bool } @@ -31,6 +36,11 @@ func findWorkspaces( finder directoryFinder, repos []*graphql.Repository, ) ([]RepoWorkspace, error) { + repoByID := make(map[string]*graphql.Repository) + for _, repo := range repos { + repoByID[repo.ID] = repo + } + // Pre-compile all globs. workspaceMatchers := make(map[batcheslib.WorkspaceConfiguration]glob.Glob) for _, conf := range spec.Workspaces { @@ -117,12 +127,71 @@ func findWorkspaces( fetchWorkspace = false } + repo, ok := repoByID[workspace.RepoID] + if !ok { + return nil, errors.New("invalid state, repo not found") + } + + steps, err := stepsForRepo(spec, repo) + if err != nil { + return nil, err + } + + // If the workspace doesn't have any steps we don't need to include it. + if len(steps) == 0 { + continue + } + workspaces = append(workspaces, RepoWorkspace{ RepoID: workspace.RepoID, Path: path, + Steps: steps, OnlyFetchWorkspace: fetchWorkspace, }) } } + + // Stable sorting. + sort.Slice(workspaces, func(i, j int) bool { + if workspaces[i].RepoID == workspaces[j].RepoID { + return workspaces[i].Path < workspaces[j].Path + } + return workspaces[i].RepoID < workspaces[j].RepoID + }) + return workspaces, nil } + +// stepsForRepo calculates the steps required to run on the given repo. +func stepsForRepo(spec *batcheslib.BatchSpec, r *graphql.Repository) ([]batcheslib.Step, error) { + taskSteps := []batcheslib.Step{} + for _, step := range spec.Steps { + // If no if condition is given, just go ahead and add the step to the list. + if step.IfCondition() == "" { + taskSteps = append(taskSteps, step) + continue + } + + batchChange := template.BatchChangeAttributes{ + Name: spec.Name, + Description: spec.Description, + } + stepCtx := &template.StepContext{ + Repository: util.GraphQLRepoToTemplatingRepo(r), + BatchChange: batchChange, + } + static, boolVal, err := template.IsStaticBool(step.IfCondition(), stepCtx) + if err != nil { + return nil, err + } + + // If we could evaluate the condition statically and the resulting + // boolean is false, we don't add that step. + if !static { + taskSteps = append(taskSteps, step) + } else if boolVal { + taskSteps = append(taskSteps, step) + } + } + return taskSteps, nil +} diff --git a/internal/batches/service/workspaces_test.go b/internal/batches/service/workspaces_test.go index c47e79690a..e833818c7c 100644 --- a/internal/batches/service/workspaces_test.go +++ b/internal/batches/service/workspaces_test.go @@ -2,10 +2,10 @@ package service import ( "context" - "sort" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" batcheslib "github.com/sourcegraph/sourcegraph/lib/batches" "github.com/sourcegraph/src-cli/internal/batches/graphql" ) @@ -31,9 +31,9 @@ func TestFindWorkspaces(t *testing.T) { spec: &batcheslib.BatchSpec{Steps: steps}, finderResults: finderResults{}, wantWorkspaces: []RepoWorkspace{ - {RepoID: repos[0].ID, Path: ""}, - {RepoID: repos[1].ID, Path: ""}, - {RepoID: repos[2].ID, Path: ""}, + {RepoID: repos[0].ID, Steps: steps, Path: ""}, + {RepoID: repos[1].ID, Steps: steps, Path: ""}, + {RepoID: repos[2].ID, Steps: steps, Path: ""}, }, }, @@ -46,9 +46,9 @@ func TestFindWorkspaces(t *testing.T) { }, finderResults: finderResults{}, wantWorkspaces: []RepoWorkspace{ - {RepoID: repos[0].ID, Path: ""}, - {RepoID: repos[1].ID, Path: ""}, - {RepoID: repos[2].ID, Path: ""}, + {RepoID: repos[0].ID, Steps: steps, Path: ""}, + {RepoID: repos[1].ID, Steps: steps, Path: ""}, + {RepoID: repos[2].ID, Steps: steps, Path: ""}, }, }, @@ -64,7 +64,7 @@ func TestFindWorkspaces(t *testing.T) { repos[2]: []string{}, }, wantWorkspaces: []RepoWorkspace{ - {RepoID: repos[1].ID, Path: ""}, + {RepoID: repos[1].ID, Steps: steps, Path: ""}, }, }, @@ -80,13 +80,13 @@ func TestFindWorkspaces(t *testing.T) { repos[2]: {"a/b", "a/b/c", "d/e/f"}, }, wantWorkspaces: []RepoWorkspace{ - {RepoID: repos[0].ID, Path: "a/b"}, - {RepoID: repos[0].ID, Path: "a/b/c"}, - {RepoID: repos[0].ID, Path: "d/e/f"}, - {RepoID: repos[1].ID, Path: ""}, - {RepoID: repos[2].ID, Path: "a/b"}, - {RepoID: repos[2].ID, Path: "a/b/c"}, - {RepoID: repos[2].ID, Path: "d/e/f"}, + {RepoID: repos[0].ID, Steps: steps, Path: "a/b"}, + {RepoID: repos[0].ID, Steps: steps, Path: "a/b/c"}, + {RepoID: repos[0].ID, Steps: steps, Path: "d/e/f"}, + {RepoID: repos[1].ID, Steps: steps, Path: ""}, + {RepoID: repos[2].ID, Steps: steps, Path: "a/b"}, + {RepoID: repos[2].ID, Steps: steps, Path: "a/b/c"}, + {RepoID: repos[2].ID, Steps: steps, Path: "d/e/f"}, }, }, @@ -106,13 +106,13 @@ func TestFindWorkspaces(t *testing.T) { repos[2]: {"a/b", "a/b/c", "d/e/f"}, }, wantWorkspaces: []RepoWorkspace{ - {RepoID: repos[0].ID, Path: "a/b", OnlyFetchWorkspace: true}, - {RepoID: repos[0].ID, Path: "a/b/c", OnlyFetchWorkspace: true}, - {RepoID: repos[0].ID, Path: "d/e/f", OnlyFetchWorkspace: true}, - {RepoID: repos[1].ID, Path: ""}, - {RepoID: repos[2].ID, Path: "a/b", OnlyFetchWorkspace: true}, - {RepoID: repos[2].ID, Path: "a/b/c", OnlyFetchWorkspace: true}, - {RepoID: repos[2].ID, Path: "d/e/f", OnlyFetchWorkspace: true}, + {RepoID: repos[0].ID, Steps: steps, Path: "a/b", OnlyFetchWorkspace: true}, + {RepoID: repos[0].ID, Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true}, + {RepoID: repos[0].ID, Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true}, + {RepoID: repos[1].ID, Steps: steps, Path: ""}, + {RepoID: repos[2].ID, Steps: steps, Path: "a/b", OnlyFetchWorkspace: true}, + {RepoID: repos[2].ID, Steps: steps, Path: "a/b/c", OnlyFetchWorkspace: true}, + {RepoID: repos[2].ID, Steps: steps, Path: "d/e/f", OnlyFetchWorkspace: true}, }, }, } @@ -125,13 +125,6 @@ func TestFindWorkspaces(t *testing.T) { t.Fatalf("unexpected err: %s", err) } - sort.Slice(workspaces, func(i, j int) bool { - if workspaces[i].RepoID == workspaces[j].RepoID { - return workspaces[i].Path < workspaces[j].Path - } - return workspaces[i].RepoID < workspaces[j].RepoID - }) - if diff := cmp.Diff(tt.wantWorkspaces, workspaces); diff != "" { t.Errorf("mismatch (-want +got):\n%s", diff) } @@ -146,3 +139,125 @@ type mockDirectoryFinder struct { func (m *mockDirectoryFinder) FindDirectoriesInRepos(ctx context.Context, fileName string, repos ...*graphql.Repository) (map[*graphql.Repository][]string, error) { return m.results, nil } + +func TestStepsForRepo(t *testing.T) { + tests := map[string]struct { + spec *batcheslib.BatchSpec + + wantSteps []batcheslib.Step + }{ + "no if": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1"}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1"}, + }, + }, + + "if has static true value": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: "true"}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1", If: "true"}, + }, + }, + + "one of many steps has if with static true value": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1"}, + {Run: "echo 2", If: "true"}, + {Run: "echo 3"}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1"}, + {Run: "echo 2", If: "true"}, + {Run: "echo 3"}, + }, + }, + + "if has static non-true value": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: "this is not true"}, + }, + }, + wantSteps: []batcheslib.Step{}, + }, + + "one of many steps has if with static non-true value": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1"}, + {Run: "echo 2", If: "every type system needs generics"}, + {Run: "echo 3"}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1"}, + {Run: "echo 3"}, + }, + }, + + "if expression that can be partially evaluated to true": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ matches repository.name "github.com/sourcegraph/src*" }}`}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ matches repository.name "github.com/sourcegraph/src*" }}`}, + }, + }, + + "if expression that can be partially evaluated to false": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ matches repository.name "horse" }}`}, + }, + }, + wantSteps: []batcheslib.Step{}, + }, + + "one of many steps has if expression that can be evaluated to true": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ matches repository.name "horse" }}`}, + }, + }, + wantSteps: []batcheslib.Step{}, + }, + + "if expression that can NOT be partially evaluated": { + spec: &batcheslib.BatchSpec{ + Steps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ eq outputs.value "foobar" }}`}, + }, + }, + wantSteps: []batcheslib.Step{ + {Run: "echo 1", If: `${{ eq outputs.value "foobar" }}`}, + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + haveSteps, err := stepsForRepo(tt.spec, testRepo1) + if err != nil { + t.Fatalf("unexpected err: %s", err) + } + + opts := cmpopts.IgnoreUnexported(batcheslib.Step{}) + if diff := cmp.Diff(tt.wantSteps, haveSteps, opts); diff != "" { + t.Errorf("mismatch (-want +got):\n%s", diff) + } + }) + } +}