From 5319b41baa6700f1d0430c619fa1cf2a13e28427 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 11 Dec 2020 13:00:03 +0100 Subject: [PATCH 1/2] Actually skip unsupported repos if -allow-unsupported is not set I think multiple changes to this method lead to a regression where unsupported repositories were correctly being reported as unsupported but were still added to the final list of repositories, regardless of whether `-allow-unsupported` was set or not. --- CHANGELOG.md | 1 + internal/campaigns/errors.go | 5 ++ internal/campaigns/service.go | 10 +-- internal/campaigns/service_test.go | 111 ++++++++++++++++++++++++++--- 4 files changed, 112 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fadb37dbf0..662056b089 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ All notable changes to `src-cli` are documented in this file. ### Fixed - Two race conditions in the terminal UI of `src campaign [apply|preview]` have been fixed. [#399](https://github.com/sourcegraph/src-cli/pull/399) +- A regression caused repositories on unsupported code host to not be skipped by `src campaign [apply|preview]`, regardless of whether `-allow-unsupported` was set or not. ### Removed diff --git a/internal/campaigns/errors.go b/internal/campaigns/errors.go index 49e9affb41..ee050576d8 100644 --- a/internal/campaigns/errors.go +++ b/internal/campaigns/errors.go @@ -12,6 +12,11 @@ import ( // returned directly as an error value if needed. type UnsupportedRepoSet map[*graphql.Repository]struct{} +func (e UnsupportedRepoSet) includes(r *graphql.Repository) bool { + _, ok := e[r] + return ok +} + func (e UnsupportedRepoSet) Error() string { repos := []string{} typeSet := map[string]struct{}{} diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 4ab7d909a5..d25dc9ec6a 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -391,12 +391,12 @@ func (svc *Service) ResolveRepositories(ctx context.Context, spec *CampaignSpec) if other, ok := seen[repo.ID]; !ok { seen[repo.ID] = repo + switch st := strings.ToLower(repo.ExternalRepository.ServiceType); st { case "github", "gitlab", "bitbucketserver": default: - unsupported.appendRepo(repo) if !svc.allowUnsupported { - continue + unsupported.appendRepo(repo) } } } else { @@ -410,10 +410,12 @@ func (svc *Service) ResolveRepositories(ctx context.Context, spec *CampaignSpec) final := make([]*graphql.Repository, 0, len(seen)) for _, repo := range seen { - final = append(final, repo) + if !unsupported.includes(repo) { + final = append(final, repo) + } } - if unsupported.hasUnsupported() && !svc.allowUnsupported { + if unsupported.hasUnsupported() { return final, unsupported } diff --git a/internal/campaigns/service_test.go b/internal/campaigns/service_test.go index d7970d3e70..094d0d682b 100644 --- a/internal/campaigns/service_test.go +++ b/internal/campaigns/service_test.go @@ -30,17 +30,8 @@ func TestSetDefaultQueryCount(t *testing.T) { } func TestResolveRepositorySearch(t *testing.T) { - mux := http.NewServeMux() - mux.HandleFunc("/.api/graphql", func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(testResolveRepositorySearchResult)) - }) - - ts := httptest.NewServer(mux) - defer ts.Close() - - var clientBuffer bytes.Buffer - client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + client, done := mockGraphQLClient(testResolveRepositorySearchResult) + defer done() svc := &Service{client: client} @@ -119,3 +110,101 @@ const testResolveRepositorySearchResult = `{ } } ` + +func mockGraphQLClient(response string) (client api.Client, done func()) { + mux := http.NewServeMux() + mux.HandleFunc("/.api/graphql", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(response)) + }) + + ts := httptest.NewServer(mux) + + var clientBuffer bytes.Buffer + client = api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) + + return client, ts.Close +} + +func TestResolveRepositories_Unsupported(t *testing.T) { + client, done := mockGraphQLClient(testResolveRepositoriesUnsupported) + defer done() + + spec := &CampaignSpec{ + On: []OnQueryOrRepository{ + {RepositoriesMatchingQuery: "testquery"}, + }, + } + + t.Run("allowUnsupported:true", func(t *testing.T) { + svc := &Service{client: client, allowUnsupported: true} + + repos, err := svc.ResolveRepositories(context.Background(), spec) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if len(repos) != 4 { + t.Fatalf("wrong number of repos. want=%d, have=%d", 4, len(repos)) + } + }) + + t.Run("allowUnsupported:false", func(t *testing.T) { + svc := &Service{client: client, allowUnsupported: false} + + repos, err := svc.ResolveRepositories(context.Background(), spec) + repoSet, ok := err.(UnsupportedRepoSet) + if !ok { + t.Fatalf("err is not UnsupportedRepoSet") + } + if len(repoSet) != 1 { + t.Fatalf("wrong number of repos. want=%d, have=%d", 1, len(repoSet)) + } + if len(repos) != 3 { + t.Fatalf("wrong number of repos. want=%d, have=%d", 4, len(repos)) + } + }) +} + +const testResolveRepositoriesUnsupported = `{ + "data": { + "search": { + "results": { + "results": [ + { + "__typename": "Repository", + "id": "UmVwb3NpdG9yeToxMw==", + "name": "bitbucket.sgdev.org/SOUR/automation-testing", + "url": "/bitbucket.sgdev.org/SOUR/automation-testing", + "externalRepository": { "serviceType": "bitbucketserver" }, + "defaultBranch": { "name": "refs/heads/master", "target": { "oid": "b978d56de5578a935ca0bf07b56528acc99d5a61" } } + }, + { + "__typename": "Repository", + "id": "UmVwb3NpdG9yeTo0", + "name": "github.com/sourcegraph/automation-testing", + "url": "/github.com/sourcegraph/automation-testing", + "externalRepository": { "serviceType": "github" }, + "defaultBranch": { "name": "refs/heads/master", "target": { "oid": "6ac8a32ecaf6c4dc5ce050b9af51bce3db8efd63" } } + }, + { + "__typename": "Repository", + "id": "UmVwb3NpdG9yeTo2MQ==", + "name": "gitlab.sgdev.org/sourcegraph/automation-testing", + "url": "/gitlab.sgdev.org/sourcegraph/automation-testing", + "externalRepository": { "serviceType": "gitlab" }, + "defaultBranch": { "name": "refs/heads/master", "target": { "oid": "3b79a5d479d2af9cfe91e0aad4e9dddca7278150" } } + }, + { + "__typename": "Repository", + "id": "UmVwb3NpdG9yeToxNDg=", + "name": "git-codecommit.us-est-1.amazonaws.com/stripe-go", + "url": "/git-codecommit.us-est-1.amazonaws.com/stripe-go", + "externalRepository": { "serviceType": "awscodecommit" }, + "defaultBranch": { "name": "refs/heads/master", "target": { "oid": "3b79a5d479d2af9cfe91e0aad4e9dddca7278150" } } + } + ] + } + } + } +} +` From b8c64fb73d3c83ee8becfc6b3bb0a9335a664581 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Fri, 11 Dec 2020 13:02:30 +0100 Subject: [PATCH 2/2] Add PR URL to changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 662056b089..b48142b06b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ All notable changes to `src-cli` are documented in this file. ### Fixed - Two race conditions in the terminal UI of `src campaign [apply|preview]` have been fixed. [#399](https://github.com/sourcegraph/src-cli/pull/399) -- A regression caused repositories on unsupported code host to not be skipped by `src campaign [apply|preview]`, regardless of whether `-allow-unsupported` was set or not. +- A regression caused repositories on unsupported code host to not be skipped by `src campaign [apply|preview]`, regardless of whether `-allow-unsupported` was set or not. [#403](https://github.com/sourcegraph/src-cli/pull/403) ### Removed