From 5a0527acecb8010873f753eb252104e972db9650 Mon Sep 17 00:00:00 2001 From: mmerrill3 Date: Thu, 20 Jan 2022 17:52:31 -0500 Subject: [PATCH 1/6] fix: Github rate limit easing, (#464) Signed-off-by: mmerrill3 --- pkg/services/scm_provider/github.go | 51 +++++++++++++-------- pkg/services/scm_provider/github_test.go | 4 +- pkg/services/scm_provider/gitlab.go | 50 +++++++++++++-------- pkg/services/scm_provider/gitlab_test.go | 4 +- pkg/services/scm_provider/mock.go | 35 ++++++++++++++- pkg/services/scm_provider/types.go | 2 + pkg/services/scm_provider/utils.go | 57 ++++++++++++++++++++++-- 7 files changed, 160 insertions(+), 43 deletions(-) diff --git a/pkg/services/scm_provider/github.go b/pkg/services/scm_provider/github.go index 60421e32..540c59b9 100644 --- a/pkg/services/scm_provider/github.go +++ b/pkg/services/scm_provider/github.go @@ -42,6 +42,27 @@ func NewGithubProvider(ctx context.Context, organization string, token string, u return &GithubProvider{client: client, organization: organization, allBranches: allBranches}, nil } +func (g *GithubProvider) GetBranches(ctx context.Context, repo *Repository) ([]*Repository, error) { + repos := []*Repository{} + branches, err := g.listBranches(ctx, repo) + if err != nil { + return nil, fmt.Errorf("error listing branches for %s/%s: %v", repo.Organization, repo.Repository, err) + } + + for _, branch := range branches { + repos = append(repos, &Repository{ + Organization: repo.Organization, + Repository: repo.Repository, + URL: repo.URL, + Branch: branch.GetName(), + SHA: branch.GetCommit().GetSHA(), + Labels: repo.Labels, + RepositoryId: repo.RepositoryId, + }) + } + return repos, nil +} + func (g *GithubProvider) ListRepos(ctx context.Context, cloneProtocol string) ([]*Repository, error) { opt := &github.RepositoryListByOrgOptions{ ListOptions: github.ListOptions{PerPage: 100}, @@ -63,22 +84,14 @@ func (g *GithubProvider) ListRepos(ctx context.Context, cloneProtocol string) ([ default: return nil, fmt.Errorf("unknown clone protocol for GitHub %v", cloneProtocol) } - - branches, err := g.listBranches(ctx, githubRepo) - if err != nil { - return nil, fmt.Errorf("error listing branches for %s/%s: %v", githubRepo.Owner.GetLogin(), githubRepo.GetName(), err) - } - - for _, branch := range branches { - repos = append(repos, &Repository{ - Organization: githubRepo.Owner.GetLogin(), - Repository: githubRepo.GetName(), - URL: url, - Branch: branch.GetName(), - SHA: branch.GetCommit().GetSHA(), - Labels: githubRepo.Topics, - }) - } + repos = append(repos, &Repository{ + Organization: githubRepo.Owner.GetLogin(), + Repository: githubRepo.GetName(), + Branch: githubRepo.GetDefaultBranch(), + URL: url, + Labels: githubRepo.Topics, + RepositoryId: githubRepo.ID, + }) } if resp.NextPage == 0 { break @@ -102,10 +115,10 @@ func (g *GithubProvider) RepoHasPath(ctx context.Context, repo *Repository, path return true, nil } -func (g *GithubProvider) listBranches(ctx context.Context, repo *github.Repository) ([]github.Branch, error) { +func (g *GithubProvider) listBranches(ctx context.Context, repo *Repository) ([]github.Branch, error) { // If we don't specifically want to query for all branches, just use the default branch and call it a day. if !g.allBranches { - defaultBranch, _, err := g.client.Repositories.GetBranch(ctx, repo.Owner.GetLogin(), repo.GetName(), repo.GetDefaultBranch()) + defaultBranch, _, err := g.client.Repositories.GetBranch(ctx, repo.Organization, repo.Repository, repo.Branch) if err != nil { return nil, err } @@ -117,7 +130,7 @@ func (g *GithubProvider) listBranches(ctx context.Context, repo *github.Reposito } branches := []github.Branch{} for { - githubBranches, resp, err := g.client.Repositories.ListBranches(ctx, repo.Owner.GetLogin(), repo.GetName(), opt) + githubBranches, resp, err := g.client.Repositories.ListBranches(ctx, repo.Organization, repo.Repository, opt) if err != nil { return nil, err } diff --git a/pkg/services/scm_provider/github_test.go b/pkg/services/scm_provider/github_test.go index 50ac25a8..c018248d 100644 --- a/pkg/services/scm_provider/github_test.go +++ b/pkg/services/scm_provider/github_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/argoproj/applicationset/api/v1alpha1" "github.com/stretchr/testify/assert" ) @@ -38,6 +39,7 @@ func TestGithubListRepos(t *testing.T) { name, proto, url string hasError, allBranches bool branches []string + filters []v1alpha1.SCMProviderGeneratorFilter }{ { name: "blank protocol", @@ -70,7 +72,7 @@ func TestGithubListRepos(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { provider, _ := NewGithubProvider(context.Background(), "argoproj", "", "", c.allBranches) - rawRepos, err := provider.ListRepos(context.Background(), c.proto) + rawRepos, err := ListRepos(context.Background(), provider, c.filters, c.proto) if c.hasError { assert.NotNil(t, err) } else { diff --git a/pkg/services/scm_provider/gitlab.go b/pkg/services/scm_provider/gitlab.go index e440cb63..d9b372a3 100644 --- a/pkg/services/scm_provider/gitlab.go +++ b/pkg/services/scm_provider/gitlab.go @@ -39,6 +39,27 @@ func NewGitlabProvider(ctx context.Context, organization string, token string, u return &GitlabProvider{client: client, organization: organization, allBranches: allBranches, includeSubgroups: includeSubgroups}, nil } +func (g *GitlabProvider) GetBranches(ctx context.Context, repo *Repository) ([]*Repository, error) { + repos := []*Repository{} + branches, err := g.listBranches(ctx, repo) + if err != nil { + return nil, fmt.Errorf("error listing branches for %s/%s: %v", repo.Organization, repo.Repository, err) + } + + for _, branch := range branches { + repos = append(repos, &Repository{ + Organization: repo.Organization, + Repository: repo.Repository, + URL: repo.URL, + Branch: branch.Name, + SHA: branch.Commit.ID, + Labels: repo.Labels, + RepositoryId: repo.RepositoryId, + }) + } + return repos, nil +} + func (g *GitlabProvider) ListRepos(ctx context.Context, cloneProtocol string) ([]*Repository, error) { opt := &gitlab.ListGroupProjectsOptions{ ListOptions: gitlab.ListOptions{PerPage: 100}, @@ -62,21 +83,14 @@ func (g *GitlabProvider) ListRepos(ctx context.Context, cloneProtocol string) ([ return nil, fmt.Errorf("unknown clone protocol for Gitlab %v", cloneProtocol) } - branches, err := g.listBranches(ctx, gitlabRepo) - if err != nil { - return nil, fmt.Errorf("error listing branches for %s/%s: %v", g.organization, gitlabRepo.Name, err) - } - - for _, branch := range branches { - repos = append(repos, &Repository{ - Organization: gitlabRepo.Namespace.FullPath, - Repository: gitlabRepo.Path, - URL: url, - Branch: branch.Name, - SHA: branch.Commit.ID, - Labels: gitlabRepo.TagList, - }) - } + repos = append(repos, &Repository{ + Organization: gitlabRepo.Namespace.FullPath, + Repository: gitlabRepo.Path, + URL: url, + Branch: gitlabRepo.DefaultBranch, + Labels: gitlabRepo.TagList, + RepositoryId: gitlabRepo.ID, + }) } if resp.CurrentPage >= resp.TotalPages { break @@ -104,11 +118,11 @@ func (g *GitlabProvider) RepoHasPath(_ context.Context, repo *Repository, path s return true, nil } -func (g *GitlabProvider) listBranches(_ context.Context, repo *gitlab.Project) ([]gitlab.Branch, error) { +func (g *GitlabProvider) listBranches(_ context.Context, repo *Repository) ([]gitlab.Branch, error) { branches := []gitlab.Branch{} // If we don't specifically want to query for all branches, just use the default branch and call it a day. if !g.allBranches { - gitlabBranch, _, err := g.client.Branches.GetBranch(repo.ID, repo.DefaultBranch, nil) + gitlabBranch, _, err := g.client.Branches.GetBranch(repo.RepositoryId, repo.Branch, nil) if err != nil { return nil, err } @@ -120,7 +134,7 @@ func (g *GitlabProvider) listBranches(_ context.Context, repo *gitlab.Project) ( ListOptions: gitlab.ListOptions{PerPage: 100}, } for { - gitlabBranches, resp, err := g.client.Branches.ListBranches(repo.ID, opt) + gitlabBranches, resp, err := g.client.Branches.ListBranches(repo.RepositoryId, opt) if err != nil { return nil, err } diff --git a/pkg/services/scm_provider/gitlab_test.go b/pkg/services/scm_provider/gitlab_test.go index abb447f8..d53ad1bd 100644 --- a/pkg/services/scm_provider/gitlab_test.go +++ b/pkg/services/scm_provider/gitlab_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/argoproj/applicationset/api/v1alpha1" "github.com/stretchr/testify/assert" ) @@ -12,6 +13,7 @@ func TestGitlabListRepos(t *testing.T) { name, proto, url string hasError, allBranches, includeSubgroups bool branches []string + filters []v1alpha1.SCMProviderGeneratorFilter }{ { name: "blank protocol", @@ -44,7 +46,7 @@ func TestGitlabListRepos(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { provider, _ := NewGitlabProvider(context.Background(), "test-argocd-proton", "", "", c.allBranches, c.includeSubgroups) - rawRepos, err := provider.ListRepos(context.Background(), c.proto) + rawRepos, err := ListRepos(context.Background(), provider, c.filters, c.proto) if c.hasError { assert.NotNil(t, err) } else { diff --git a/pkg/services/scm_provider/mock.go b/pkg/services/scm_provider/mock.go index 54b864a4..bf7e452c 100644 --- a/pkg/services/scm_provider/mock.go +++ b/pkg/services/scm_provider/mock.go @@ -9,9 +9,42 @@ type MockProvider struct { var _ SCMProviderService = &MockProvider{} func (m *MockProvider) ListRepos(_ context.Context, _ string) ([]*Repository, error) { - return m.Repos, nil + repos := []*Repository{} + for _, candidateRepo := range m.Repos { + found := false + for _, alreadySetRepo := range repos { + if alreadySetRepo.Repository == candidateRepo.Repository { + found = true + break + } + } + if !found { + repos = append(repos, candidateRepo) + } + } + return repos, nil } func (*MockProvider) RepoHasPath(_ context.Context, repo *Repository, path string) (bool, error) { return path == repo.Repository, nil } + +func (m *MockProvider) GetBranches(_ context.Context, repo *Repository) ([]*Repository, error) { + branchRepos := []*Repository{} + for _, candidateRepo := range m.Repos { + if candidateRepo.Repository == repo.Repository { + found := false + for _, alreadySetRepo := range branchRepos { + if alreadySetRepo.Branch == candidateRepo.Branch { + found = true + break + } + } + if !found { + branchRepos = append(branchRepos, candidateRepo) + } + } + + } + return branchRepos, nil +} diff --git a/pkg/services/scm_provider/types.go b/pkg/services/scm_provider/types.go index bc5aad8a..87baf7b4 100644 --- a/pkg/services/scm_provider/types.go +++ b/pkg/services/scm_provider/types.go @@ -13,11 +13,13 @@ type Repository struct { Branch string SHA string Labels []string + RepositoryId interface{} } type SCMProviderService interface { ListRepos(context.Context, string) ([]*Repository, error) RepoHasPath(context.Context, *Repository, string) (bool, error) + GetBranches(context.Context, *Repository) ([]*Repository, error) } // A compiled version of SCMProviderGeneratorFilter for performance. diff --git a/pkg/services/scm_provider/utils.go b/pkg/services/scm_provider/utils.go index 923b39a6..4250af3e 100644 --- a/pkg/services/scm_provider/utils.go +++ b/pkg/services/scm_provider/utils.go @@ -87,14 +87,52 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop return nil, err } - // Special case, if we have no filters, allow everything. - if len(compiledFilters) == 0 { + repoFilters := getApplicableFilters(compiledFilters, false) + if len(repoFilters) == 0 { + repos, err := getBranches(ctx, provider, repos, compiledFilters) + if err != nil { + return nil, err + } return repos, nil } filteredRepos := make([]*Repository, 0, len(repos)) for _, repo := range repos { - for _, filter := range compiledFilters { + for _, filter := range repoFilters { + matches, err := matchFilter(ctx, provider, repo, filter) + if err != nil { + return nil, err + } + if matches { + filteredRepos = append(filteredRepos, repo) + break + } + } + } + + repos, err = getBranches(ctx, provider, filteredRepos, compiledFilters) + if err != nil { + return nil, err + } + return repos, nil +} + +func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repository, compiledFilters []*Filter) ([]*Repository, error) { + reposWithBranches := []*Repository{} + for _, repo := range repos { + reposFilled, err := provider.GetBranches(ctx, repo) + if err != nil { + return nil, err + } + reposWithBranches = append(reposWithBranches, reposFilled...) + } + branchFilters := getApplicableFilters(compiledFilters, true) + if len(branchFilters) == 0 { + return reposWithBranches, nil + } + filteredRepos := make([]*Repository, 0, len(reposWithBranches)) + for _, repo := range reposWithBranches { + for _, filter := range branchFilters { matches, err := matchFilter(ctx, provider, repo, filter) if err != nil { return nil, err @@ -107,3 +145,16 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop } return filteredRepos, nil } + +// getApplicableFilters returns filters which either have a branch filter or do not have branch filters, depending on the hasBranchFilter argument. +func getApplicableFilters(filters []*Filter, branchFilter bool) []*Filter { + applicableFilters := []*Filter{} + for _, filter := range filters { + if branchFilter && (filter.BranchMatch != nil || filter.PathsExist != nil) { + applicableFilters = append(applicableFilters, filter) + } else if !branchFilter && filter.BranchMatch == nil && filter.PathsExist == nil { + applicableFilters = append(applicableFilters, filter) + } + } + return applicableFilters +} From 19803737fc64d946592a822375cd2dd69dd06e4f Mon Sep 17 00:00:00 2001 From: mmerrill3 Date: Fri, 11 Feb 2022 18:59:58 -0500 Subject: [PATCH 2/6] cleaning up code for readability with new types Signed-off-by: mmerrill3 --- pkg/services/scm_provider/types.go | 10 ++++++++++ pkg/services/scm_provider/utils.go | 25 +++++++++++++++---------- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/pkg/services/scm_provider/types.go b/pkg/services/scm_provider/types.go index 87baf7b4..eb7583d9 100644 --- a/pkg/services/scm_provider/types.go +++ b/pkg/services/scm_provider/types.go @@ -28,4 +28,14 @@ type Filter struct { PathsExist []string LabelMatch *regexp.Regexp BranchMatch *regexp.Regexp + FilterType FilterType } + +// A convenience type for indicating where to apply a filter +type FilterType int64 + +// The enum of filter types +const ( + FilterTypeBranch FilterType = iota + FilterTypeRepo +) diff --git a/pkg/services/scm_provider/utils.go b/pkg/services/scm_provider/utils.go index 4250af3e..e129884e 100644 --- a/pkg/services/scm_provider/utils.go +++ b/pkg/services/scm_provider/utils.go @@ -18,21 +18,25 @@ func compileFilters(filters []argoprojiov1alpha1.SCMProviderGeneratorFilter) ([] if err != nil { return nil, fmt.Errorf("error compiling RepositoryMatch regexp %q: %v", *filter.RepositoryMatch, err) } + outFilter.FilterType = FilterTypeRepo } if filter.LabelMatch != nil { outFilter.LabelMatch, err = regexp.Compile(*filter.LabelMatch) if err != nil { return nil, fmt.Errorf("error compiling LabelMatch regexp %q: %v", *filter.LabelMatch, err) } + outFilter.FilterType = FilterTypeRepo } if filter.PathsExist != nil { outFilter.PathsExist = filter.PathsExist + outFilter.FilterType = FilterTypeBranch } if filter.BranchMatch != nil { outFilter.BranchMatch, err = regexp.Compile(*filter.BranchMatch) if err != nil { return nil, fmt.Errorf("error compiling BranchMatch regexp %q: %v", *filter.LabelMatch, err) } + outFilter.FilterType = FilterTypeBranch } outFilters = append(outFilters, outFilter) } @@ -87,7 +91,7 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop return nil, err } - repoFilters := getApplicableFilters(compiledFilters, false) + _, repoFilters := getApplicableFilters(compiledFilters) if len(repoFilters) == 0 { repos, err := getBranches(ctx, provider, repos, compiledFilters) if err != nil { @@ -126,7 +130,7 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo } reposWithBranches = append(reposWithBranches, reposFilled...) } - branchFilters := getApplicableFilters(compiledFilters, true) + branchFilters, _ := getApplicableFilters(compiledFilters) if len(branchFilters) == 0 { return reposWithBranches, nil } @@ -146,15 +150,16 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo return filteredRepos, nil } -// getApplicableFilters returns filters which either have a branch filter or do not have branch filters, depending on the hasBranchFilter argument. -func getApplicableFilters(filters []*Filter, branchFilter bool) []*Filter { - applicableFilters := []*Filter{} +// getApplicableFilters returns branch filters and repo filters separately. +func getApplicableFilters(filters []*Filter) ([]*Filter, []*Filter) { + branchFilters := []*Filter{} + repoFilters := []*Filter{} for _, filter := range filters { - if branchFilter && (filter.BranchMatch != nil || filter.PathsExist != nil) { - applicableFilters = append(applicableFilters, filter) - } else if !branchFilter && filter.BranchMatch == nil && filter.PathsExist == nil { - applicableFilters = append(applicableFilters, filter) + if filter.FilterType == FilterTypeBranch { + branchFilters = append(branchFilters, filter) + } else if filter.FilterType == FilterTypeRepo { + repoFilters = append(repoFilters, filter) } } - return applicableFilters + return branchFilters, repoFilters } From fa5669269473864e06441c290fcab872168297b6 Mon Sep 17 00:00:00 2001 From: mmerrill3 Date: Sun, 13 Feb 2022 11:20:48 -0500 Subject: [PATCH 3/6] adding unit test, and returning map for clarity Signed-off-by: mmerrill3 --- pkg/services/scm_provider/utils.go | 20 ++++++++------- pkg/services/scm_provider/utils_test.go | 34 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/pkg/services/scm_provider/utils.go b/pkg/services/scm_provider/utils.go index e129884e..07f29a59 100644 --- a/pkg/services/scm_provider/utils.go +++ b/pkg/services/scm_provider/utils.go @@ -91,7 +91,7 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop return nil, err } - _, repoFilters := getApplicableFilters(compiledFilters) + repoFilters := getApplicableFilters(compiledFilters)[FilterTypeRepo] if len(repoFilters) == 0 { repos, err := getBranches(ctx, provider, repos, compiledFilters) if err != nil { @@ -130,7 +130,7 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo } reposWithBranches = append(reposWithBranches, reposFilled...) } - branchFilters, _ := getApplicableFilters(compiledFilters) + branchFilters := getApplicableFilters(compiledFilters)[FilterTypeBranch] if len(branchFilters) == 0 { return reposWithBranches, nil } @@ -150,16 +150,18 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo return filteredRepos, nil } -// getApplicableFilters returns branch filters and repo filters separately. -func getApplicableFilters(filters []*Filter) ([]*Filter, []*Filter) { - branchFilters := []*Filter{} - repoFilters := []*Filter{} +// getApplicableFilters returns a map of filters separated by type. +func getApplicableFilters(filters []*Filter) map[FilterType][]*Filter { + filterMap := map[FilterType][]*Filter{ + FilterTypeBranch: {}, + FilterTypeRepo: {}, + } for _, filter := range filters { if filter.FilterType == FilterTypeBranch { - branchFilters = append(branchFilters, filter) + filterMap[FilterTypeBranch] = append(filterMap[FilterTypeBranch], filter) } else if filter.FilterType == FilterTypeRepo { - repoFilters = append(repoFilters, filter) + filterMap[FilterTypeRepo] = append(filterMap[FilterTypeRepo], filter) } } - return branchFilters, repoFilters + return filterMap } diff --git a/pkg/services/scm_provider/utils_test.go b/pkg/services/scm_provider/utils_test.go index 4b017513..1bf68e96 100644 --- a/pkg/services/scm_provider/utils_test.go +++ b/pkg/services/scm_provider/utils_test.go @@ -2,6 +2,7 @@ package scm_provider import ( "context" + "regexp" "testing" argoprojiov1alpha1 "github.com/argoproj/applicationset/api/v1alpha1" @@ -255,3 +256,36 @@ func TestNoFilters(t *testing.T) { assert.Equal(t, "two", repos[1].Repository) assert.Equal(t, "three", repos[2].Repository) } + +// tests the getApplicableFilters function, passing in all the filters, and an unset filter, plus an additional +// branch filter +func TestApplicableFilterMap(t *testing.T) { + branchFilter := Filter{ + BranchMatch: ®exp.Regexp{}, + FilterType: FilterTypeBranch, + } + repoFilter := Filter{ + RepositoryMatch: ®exp.Regexp{}, + FilterType: FilterTypeRepo, + } + pathExistsFilter := Filter{ + PathsExist: []string{"test"}, + FilterType: FilterTypeBranch, + } + labelMatchFilter := Filter{ + LabelMatch: ®exp.Regexp{}, + FilterType: FilterTypeRepo, + } + unsetFilter := Filter{ + LabelMatch: ®exp.Regexp{}, + } + additionalBranchFilter := Filter{ + BranchMatch: ®exp.Regexp{}, + FilterType: FilterTypeBranch, + } + filterMap := getApplicableFilters([]*Filter{&branchFilter, &repoFilter, + &pathExistsFilter, &labelMatchFilter, &unsetFilter, &additionalBranchFilter}) + + assert.Len(t, filterMap[FilterTypeRepo], 2) + assert.Len(t, filterMap[FilterTypeBranch], 3) +} From fa66dd80bc7be8d94f3e8d38cc977e39d4fd463c Mon Sep 17 00:00:00 2001 From: mmerrill3 Date: Sun, 13 Feb 2022 11:49:20 -0500 Subject: [PATCH 4/6] adding undefined type Signed-off-by: mmerrill3 --- pkg/services/scm_provider/types.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/services/scm_provider/types.go b/pkg/services/scm_provider/types.go index eb7583d9..b7f90bd8 100644 --- a/pkg/services/scm_provider/types.go +++ b/pkg/services/scm_provider/types.go @@ -36,6 +36,7 @@ type FilterType int64 // The enum of filter types const ( - FilterTypeBranch FilterType = iota + FilterTypeUndefined FilterType = iota + FilterTypeBranch FilterTypeRepo ) From dee9884afdd9170083ac51453a64ce68103934fd Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Tue, 15 Feb 2022 10:22:51 -0500 Subject: [PATCH 5/6] chore: try alternative filter segmentation logic Signed-off-by: Michael Crenshaw --- api/v1alpha1/zz_generated.deepcopy.go | 1 + pkg/services/scm_provider/types.go | 34 +++++++++++++- pkg/services/scm_provider/utils.go | 60 ++++++++----------------- pkg/services/scm_provider/utils_test.go | 19 ++++---- 4 files changed, 62 insertions(+), 52 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index e1c19ddd..4248a934 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,3 +1,4 @@ +//go:build !ignore_autogenerated // +build !ignore_autogenerated /* diff --git a/pkg/services/scm_provider/types.go b/pkg/services/scm_provider/types.go index b7f90bd8..eb8cf847 100644 --- a/pkg/services/scm_provider/types.go +++ b/pkg/services/scm_provider/types.go @@ -28,7 +28,39 @@ type Filter struct { PathsExist []string LabelMatch *regexp.Regexp BranchMatch *regexp.Regexp - FilterType FilterType +} + +// HasRepoFilter returns true if a Filter includes a filter which can exclude a repo based on knowledge about the repo +// only (no need for knowledge of any branches). A Filter with a "repo filter" may also include branch-specific filters. +func (f *Filter) HasRepoFilter() bool { + return f.RepositoryMatch != nil || f.LabelMatch != nil +} + +// HasBranchFilter returns true if a Filter includes a filter which can exclude a branch. +func (f *Filter) HasBranchFilter() bool { + return f.BranchMatch != nil || f.PathsExist != nil +} + +type Filters []*Filter + +func (f Filters) GetRepoFilters() Filters { + var repoFilters Filters + for _, filter := range f { + if filter.HasRepoFilter() { + repoFilters = append(repoFilters, filter) + } + } + return repoFilters +} + +func (f Filters) GetBranchFilters() Filters { + var branchFilters Filters + for _, filter := range f { + if filter.HasBranchFilter() { + branchFilters = append(branchFilters, filter) + } + } + return branchFilters } // A convenience type for indicating where to apply a filter diff --git a/pkg/services/scm_provider/utils.go b/pkg/services/scm_provider/utils.go index 07f29a59..3a280c6a 100644 --- a/pkg/services/scm_provider/utils.go +++ b/pkg/services/scm_provider/utils.go @@ -8,8 +8,8 @@ import ( argoprojiov1alpha1 "github.com/argoproj/applicationset/api/v1alpha1" ) -func compileFilters(filters []argoprojiov1alpha1.SCMProviderGeneratorFilter) ([]*Filter, error) { - outFilters := make([]*Filter, 0, len(filters)) +func compileFilters(filters []argoprojiov1alpha1.SCMProviderGeneratorFilter) (Filters, error) { + outFilters := make(Filters, 0, len(filters)) for _, filter := range filters { outFilter := &Filter{} var err error @@ -18,25 +18,21 @@ func compileFilters(filters []argoprojiov1alpha1.SCMProviderGeneratorFilter) ([] if err != nil { return nil, fmt.Errorf("error compiling RepositoryMatch regexp %q: %v", *filter.RepositoryMatch, err) } - outFilter.FilterType = FilterTypeRepo } if filter.LabelMatch != nil { outFilter.LabelMatch, err = regexp.Compile(*filter.LabelMatch) if err != nil { return nil, fmt.Errorf("error compiling LabelMatch regexp %q: %v", *filter.LabelMatch, err) } - outFilter.FilterType = FilterTypeRepo } if filter.PathsExist != nil { outFilter.PathsExist = filter.PathsExist - outFilter.FilterType = FilterTypeBranch } if filter.BranchMatch != nil { outFilter.BranchMatch, err = regexp.Compile(*filter.BranchMatch) if err != nil { return nil, fmt.Errorf("error compiling BranchMatch regexp %q: %v", *filter.LabelMatch, err) } - outFilter.FilterType = FilterTypeBranch } outFilters = append(outFilters, outFilter) } @@ -91,25 +87,21 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop return nil, err } - repoFilters := getApplicableFilters(compiledFilters)[FilterTypeRepo] - if len(repoFilters) == 0 { - repos, err := getBranches(ctx, provider, repos, compiledFilters) - if err != nil { - return nil, err - } - return repos, nil - } - + repoFilters := compiledFilters.GetRepoFilters() filteredRepos := make([]*Repository, 0, len(repos)) - for _, repo := range repos { - for _, filter := range repoFilters { - matches, err := matchFilter(ctx, provider, repo, filter) - if err != nil { - return nil, err - } - if matches { - filteredRepos = append(filteredRepos, repo) - break + if len(repoFilters) == 0 { + filteredRepos = repos + } else { + for _, repo := range repos { + for _, filter := range repoFilters { + matches, err := matchFilter(ctx, provider, repo, filter) + if err != nil { + return nil, err + } + if matches { + filteredRepos = append(filteredRepos, repo) + break + } } } } @@ -121,7 +113,7 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop return repos, nil } -func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repository, compiledFilters []*Filter) ([]*Repository, error) { +func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repository, compiledFilters Filters) ([]*Repository, error) { reposWithBranches := []*Repository{} for _, repo := range repos { reposFilled, err := provider.GetBranches(ctx, repo) @@ -130,7 +122,7 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo } reposWithBranches = append(reposWithBranches, reposFilled...) } - branchFilters := getApplicableFilters(compiledFilters)[FilterTypeBranch] + branchFilters := compiledFilters.GetBranchFilters() if len(branchFilters) == 0 { return reposWithBranches, nil } @@ -149,19 +141,3 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo } return filteredRepos, nil } - -// getApplicableFilters returns a map of filters separated by type. -func getApplicableFilters(filters []*Filter) map[FilterType][]*Filter { - filterMap := map[FilterType][]*Filter{ - FilterTypeBranch: {}, - FilterTypeRepo: {}, - } - for _, filter := range filters { - if filter.FilterType == FilterTypeBranch { - filterMap[FilterTypeBranch] = append(filterMap[FilterTypeBranch], filter) - } else if filter.FilterType == FilterTypeRepo { - filterMap[FilterTypeRepo] = append(filterMap[FilterTypeRepo], filter) - } - } - return filterMap -} diff --git a/pkg/services/scm_provider/utils_test.go b/pkg/services/scm_provider/utils_test.go index 1bf68e96..24b6bfcd 100644 --- a/pkg/services/scm_provider/utils_test.go +++ b/pkg/services/scm_provider/utils_test.go @@ -262,30 +262,31 @@ func TestNoFilters(t *testing.T) { func TestApplicableFilterMap(t *testing.T) { branchFilter := Filter{ BranchMatch: ®exp.Regexp{}, - FilterType: FilterTypeBranch, } repoFilter := Filter{ RepositoryMatch: ®exp.Regexp{}, - FilterType: FilterTypeRepo, } pathExistsFilter := Filter{ PathsExist: []string{"test"}, - FilterType: FilterTypeBranch, } labelMatchFilter := Filter{ LabelMatch: ®exp.Regexp{}, - FilterType: FilterTypeRepo, } unsetFilter := Filter{ LabelMatch: ®exp.Regexp{}, } additionalBranchFilter := Filter{ BranchMatch: ®exp.Regexp{}, - FilterType: FilterTypeBranch, } - filterMap := getApplicableFilters([]*Filter{&branchFilter, &repoFilter, - &pathExistsFilter, &labelMatchFilter, &unsetFilter, &additionalBranchFilter}) + bothFilter := Filter{ + RepositoryMatch: ®exp.Regexp{}, + PathsExist: []string{"test"}, + } + filters := Filters{&branchFilter, &repoFilter, + &pathExistsFilter, &labelMatchFilter, &unsetFilter, &additionalBranchFilter, &bothFilter} + repoFilters := filters.GetRepoFilters() + branchFilters := filters.GetBranchFilters() - assert.Len(t, filterMap[FilterTypeRepo], 2) - assert.Len(t, filterMap[FilterTypeBranch], 3) + assert.Len(t, repoFilters, 4) + assert.Len(t, branchFilters, 4) } From ecc7386563f0b2e60b72b0b6789a7c53e08d5716 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw Date: Fri, 18 Feb 2022 14:07:21 -0500 Subject: [PATCH 6/6] chore: delete the flipping extra line that shouldn't exist Signed-off-by: Michael Crenshaw --- api/v1alpha1/zz_generated.deepcopy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4248a934..e1c19ddd 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1,4 +1,3 @@ -//go:build !ignore_autogenerated // +build !ignore_autogenerated /*