From ba43aae655660b7a64cfc9a28b8569c1f91037da Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Mon, 1 Feb 2021 13:51:36 +0100 Subject: [PATCH 1/2] Check for branch duplicates after creating changeset specs This is a follow-up to #442 and ensures that changeset specs are not getting silently lost by validating that multiple changeset specs in the same repository have different branches. I decided to make this a separate step _after_ the execution of the steps so that users can leverage the cache. That allows them to change the campaign spec and then rerun the command after they get this error, vs. the execution being aborted after running into this error (if we'd do the check inside executor). --- cmd/src/campaigns_common.go | 7 ++- internal/campaigns/executor.go | 22 ++++++--- internal/campaigns/executor_test.go | 19 +++++--- internal/campaigns/service.go | 64 ++++++++++++++++++++++++++ internal/campaigns/service_test.go | 70 +++++++++++++++++++++++++++++ 5 files changed, 167 insertions(+), 15 deletions(-) diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 2064946ffc..49a424c663 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -268,6 +268,11 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se }() } + err = svc.ValidateChangesetSpecs(repos, specs) + if err != nil { + return "", "", err + } + ids := make([]campaigns.ChangesetSpecID, len(specs)) if len(specs) > 0 { @@ -362,7 +367,7 @@ func printExecutionError(out *output.Output, err error) { if err == context.Canceled { block.Writef("%sAborting", output.StyleBold) } else { - block.Writef("%s%s=%+v)", output.StyleBold, e.Error()) + block.Writef("%s%s", output.StyleBold, e.Error()) } } } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index c71101df21..d9bcfb2bab 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -316,9 +316,9 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { }) // Add the spec to the executor's list of completed specs. - x.specsMu.Lock() - x.specs = append(x.specs, specs...) - x.specsMu.Unlock() + if err := x.addCompletedSpecs(task.Repository, specs); err != nil { + return err + } return } @@ -392,10 +392,10 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { status.ChangesetSpecs = specs }) - // Add the spec to the executor's list of completed specs. - x.specsMu.Lock() - x.specs = append(x.specs, specs...) - x.specsMu.Unlock() + if err := x.addCompletedSpecs(task.Repository, specs); err != nil { + return err + } + return } @@ -409,6 +409,14 @@ func (x *executor) updateTaskStatus(task *Task, update func(status *TaskStatus)) } } +func (x *executor) addCompletedSpecs(repository *graphql.Repository, specs []*ChangesetSpec) error { + x.specsMu.Lock() + defer x.specsMu.Unlock() + + x.specs = append(x.specs, specs...) + return nil +} + func (x *executor) LockedTaskStatuses(callback func([]*TaskStatus)) { x.statusesMu.RLock() defer x.statusesMu.RUnlock() diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index a8a8fea9ae..d7075253aa 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -400,13 +400,18 @@ repository_name=github.com/sourcegraph/src-cli`, executor.Start(context.Background()) specs, err := executor.Wait(context.Background()) - if tc.wantErrInclude == "" && err != nil { - t.Fatalf("execution failed: %s", err) - } - if err != nil && !strings.Contains(err.Error(), tc.wantErrInclude) { - t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude) - } - if tc.wantErrInclude != "" { + if tc.wantErrInclude == "" { + if err != nil { + t.Fatalf("execution failed: %s", err) + } + } else { + if err == nil { + t.Fatalf("expected error to include %q, but got no error", tc.wantErrInclude) + } else { + if !strings.Contains(err.Error(), tc.wantErrInclude) { + t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude) + } + } return } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 6f97714fe1..85006637b6 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -415,6 +415,70 @@ func (svc *Service) ExecuteCampaignSpec(ctx context.Context, opts ExecutorOpts, return specs, x.LogFiles(), errs.ErrorOrNil() } +func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs []*ChangesetSpec) error { + repoByID := make(map[string]*graphql.Repository, len(repos)) + for _, repo := range repos { + repoByID[repo.ID] = repo + } + + byRepoAndBranch := make(map[string]map[string][]*ChangesetSpec) + for _, spec := range specs { + _, ok := byRepoAndBranch[spec.HeadRepository] + if !ok { + byRepoAndBranch[spec.HeadRepository] = make(map[string][]*ChangesetSpec) + } + + byRepoAndBranch[spec.HeadRepository][spec.HeadRef] = append(byRepoAndBranch[spec.HeadRepository][spec.HeadRef], spec) + } + + duplicates := make(map[*graphql.Repository]map[string]int) + for repoID, specsByBranch := range byRepoAndBranch { + for branch, specs := range specsByBranch { + if len(specs) < 2 { + continue + } + + r := repoByID[repoID] + if _, ok := duplicates[r]; !ok { + duplicates[r] = make(map[string]int) + } + + duplicates[r][branch] = len(specs) + } + } + + if len(duplicates) > 0 { + return &duplicateBranchErr{fooduplicates: duplicates} + } + + return nil +} + +type duplicateBranchErr struct { + repository *graphql.Repository + headRef string + duplicates int + + fooduplicates map[*graphql.Repository]map[string]int +} + +func (e *duplicateBranchErr) Error() string { + var out strings.Builder + + fmt.Fprintf(&out, "Multiple changeset specs have the same branch:\n\n") + + for repo, branches := range e.fooduplicates { + for branch, duplicates := range branches { + branch = strings.TrimPrefix(branch, "refs/heads/") + fmt.Fprintf(&out, "\t* %s: %d changeset specs have the branch %q\n", repo.Name, duplicates, branch) + } + } + + fmt.Fprint(&out, "\nMake sure that the changesetTemplate.branch field in the campaign spec produces unique values for each changeset in a single repository and rerun this command.") + + return out.String() +} + func (svc *Service) ParseCampaignSpec(in io.Reader) (*CampaignSpec, string, error) { data, err := ioutil.ReadAll(in) if err != nil { diff --git a/internal/campaigns/service_test.go b/internal/campaigns/service_test.go index b86e9a320e..bb3b42cda2 100644 --- a/internal/campaigns/service_test.go +++ b/internal/campaigns/service_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -455,3 +456,72 @@ func mockDirectoriesInReposResults(t *testing.T, filesPerRepo filesInRepos) (cli return mockGraphQLClient(string(rawRes)) } + +func TestService_ValidateChangesetSpecs(t *testing.T) { + repo1 := &graphql.Repository{ID: "repo-graphql-id-1", Name: "github.com/sourcegraph/src-cli"} + repo2 := &graphql.Repository{ID: "repo-graphql-id-2", Name: "github.com/sourcegraph/sourcegraph"} + + tests := map[string]struct { + repos []*graphql.Repository + specs []*ChangesetSpec + + wantErrInclude string + }{ + "no errors": { + repos: []*graphql.Repository{repo1, repo2}, + specs: []*ChangesetSpec{ + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-2"}, + }, + }, + }, + + "duplicate branches": { + repos: []*graphql.Repository{repo1, repo2}, + specs: []*ChangesetSpec{ + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + }, + wantErrInclude: `github.com/sourcegraph/sourcegraph: 2 changeset specs have the branch "branch-1"`, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + svc := &Service{} + haveErr := svc.ValidateChangesetSpecs(tt.repos, tt.specs) + if tt.wantErrInclude != "" { + if haveErr == nil { + t.Fatalf("expected %q to be included in error, but got none", tt.wantErrInclude) + } else { + if !strings.Contains(haveErr.Error(), tt.wantErrInclude) { + t.Fatalf("expected %q to be included in error, but was not. error=%q", tt.wantErrInclude, haveErr.Error()) + } + } + } else { + if haveErr != nil { + t.Fatalf("unexpected error: %s", haveErr) + } + } + }) + } +} From 6a6a39ec13d44598a1e87414ab6d8a29a04f0cdc Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Tue, 2 Feb 2021 09:27:43 +0100 Subject: [PATCH 2/2] Fix naming in duplicateBranchesErr --- internal/campaigns/service.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 85006637b6..c2128a2202 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -448,26 +448,22 @@ func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs [] } if len(duplicates) > 0 { - return &duplicateBranchErr{fooduplicates: duplicates} + return &duplicateBranchesErr{duplicates: duplicates} } return nil } -type duplicateBranchErr struct { - repository *graphql.Repository - headRef string - duplicates int - - fooduplicates map[*graphql.Repository]map[string]int +type duplicateBranchesErr struct { + duplicates map[*graphql.Repository]map[string]int } -func (e *duplicateBranchErr) Error() string { +func (e *duplicateBranchesErr) Error() string { var out strings.Builder fmt.Fprintf(&out, "Multiple changeset specs have the same branch:\n\n") - for repo, branches := range e.fooduplicates { + for repo, branches := range e.duplicates { for branch, duplicates := range branches { branch = strings.TrimPrefix(branch, "refs/heads/") fmt.Fprintf(&out, "\t* %s: %d changeset specs have the branch %q\n", repo.Name, duplicates, branch)