From 4733c4d5852bd34eaed475076147bccd1dcfa7ed Mon Sep 17 00:00:00 2001 From: Jedrzej Kotkowski Date: Tue, 11 Jul 2023 21:51:57 +0200 Subject: [PATCH] fix scmProvider generator filters Signed-off-by: Jedrzej Kotkowski --- applicationset/services/scm_provider/types.go | 15 +-- applicationset/services/scm_provider/utils.go | 118 ++++++++---------- .../services/scm_provider/utils_test.go | 60 +++------ 3 files changed, 76 insertions(+), 117 deletions(-) diff --git a/applicationset/services/scm_provider/types.go b/applicationset/services/scm_provider/types.go index dde6db03c7c27..837bd95827d67 100644 --- a/applicationset/services/scm_provider/types.go +++ b/applicationset/services/scm_provider/types.go @@ -29,15 +29,8 @@ type Filter struct { PathsDoNotExist []string LabelMatch *regexp.Regexp BranchMatch *regexp.Regexp - FilterType FilterType + // FilterTypeRepo are filters that apply to the repo itself (name, labels) + FilterTypeRepo bool + // FilterTypeBranch are filters that apply to the repo content (path, branch) + FilterTypeBranch bool } - -// A convenience type for indicating where to apply a filter -type FilterType int64 - -// The enum of filter types -const ( - FilterTypeUndefined FilterType = iota - FilterTypeBranch - FilterTypeRepo -) diff --git a/applicationset/services/scm_provider/utils.go b/applicationset/services/scm_provider/utils.go index e92923f52707b..fad3b05106f9f 100644 --- a/applicationset/services/scm_provider/utils.go +++ b/applicationset/services/scm_provider/utils.go @@ -19,42 +19,38 @@ func compileFilters(filters []argoprojiov1alpha1.SCMProviderGeneratorFilter) ([] if err != nil { return nil, fmt.Errorf("error compiling RepositoryMatch regexp %q: %v", *filter.RepositoryMatch, err) } - outFilter.FilterType = FilterTypeRepo + outFilter.FilterTypeRepo = true } 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 + outFilter.FilterTypeRepo = true } if filter.PathsExist != nil { outFilter.PathsExist = filter.PathsExist - outFilter.FilterType = FilterTypeBranch + outFilter.FilterTypeBranch = true } if filter.PathsDoNotExist != nil { outFilter.PathsDoNotExist = filter.PathsDoNotExist - outFilter.FilterType = FilterTypeBranch + outFilter.FilterTypeBranch = true } 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.BranchMatch, err) } - outFilter.FilterType = FilterTypeBranch + outFilter.FilterTypeBranch = true } outFilters = append(outFilters, outFilter) } return outFilters, nil } -func matchFilter(ctx context.Context, provider SCMProviderService, repo *Repository, filter *Filter) (bool, error) { +func matchRepoFilter(ctx context.Context, provider SCMProviderService, repo *Repository, filter *Filter) bool { if filter.RepositoryMatch != nil && !filter.RepositoryMatch.MatchString(repo.Repository) { - return false, nil - } - - if filter.BranchMatch != nil && !filter.BranchMatch.MatchString(repo.Branch) { - return false, nil + return false } if filter.LabelMatch != nil { @@ -66,10 +62,18 @@ func matchFilter(ctx context.Context, provider SCMProviderService, repo *Reposit } } if !found { - return false, nil + return false } } + return true +} + +func matchBranchFilter(ctx context.Context, provider SCMProviderService, repo *Repository, filter *Filter) (bool, error) { + if filter.BranchMatch != nil && !filter.BranchMatch.MatchString(repo.Branch) { + return false, nil + } + if len(filter.PathsExist) != 0 { for _, path := range filter.PathsExist { path = strings.TrimRight(path, "/") @@ -107,76 +111,60 @@ func ListRepos(ctx context.Context, provider SCMProviderService, filters []argop if err != nil { 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 - } - filteredRepos := make([]*Repository, 0, len(repos)) + + filledRepos := make([]*Repository, 0, len(repos)) for _, repo := range repos { - for _, filter := range repoFilters { - matches, err := matchFilter(ctx, provider, repo, filter) + + if len(compiledFilters) > 0 { + for _, filter := range compiledFilters { + if filter.FilterTypeRepo { + if matches := matchRepoFilter(ctx, provider, repo, filter); !matches { + continue + } + } + + filteredRepoWithBranches, err := getBranches(ctx, provider, repo, filter) + if err != nil { + return nil, err + } + + filledRepos = append(filledRepos, filteredRepoWithBranches...) + } + } else { + repoWithBranches, err := getBranches(ctx, provider, repo, nil) if err != nil { return nil, err } - if matches { - filteredRepos = append(filteredRepos, repo) - break - } + + filledRepos = append(filledRepos, repoWithBranches...) } } - repos, err = getBranches(ctx, provider, filteredRepos, compiledFilters) + return filledRepos, nil +} + +func getBranches(ctx context.Context, provider SCMProviderService, repo *Repository, filter *Filter) ([]*Repository, error) { + repoWithBranches := []*Repository{} + + repoFilled, err := provider.GetBranches(ctx, repo) if err != nil { return nil, err } - return repos, nil -} + repoWithBranches = append(repoWithBranches, repoFilled...) -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)[FilterTypeBranch] - 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) + filteredRepoWithBranches := make([]*Repository, 0, len(repoWithBranches)) + if filter != nil && filter.FilterTypeBranch { + for _, repoBranch := range repoWithBranches { + matches, err := matchBranchFilter(ctx, provider, repoBranch, filter) if err != nil { return nil, err } if matches { - filteredRepos = append(filteredRepos, repo) - break + filteredRepoWithBranches = append(filteredRepoWithBranches, repoBranch) } } + return filteredRepoWithBranches, nil } - 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 + return repoWithBranches, nil } diff --git a/applicationset/services/scm_provider/utils_test.go b/applicationset/services/scm_provider/utils_test.go index 5ef6d582f8d34..e9b0611fce958 100644 --- a/applicationset/services/scm_provider/utils_test.go +++ b/applicationset/services/scm_provider/utils_test.go @@ -2,7 +2,6 @@ package scm_provider import ( "context" - "regexp" "testing" "github.com/stretchr/testify/assert" @@ -213,6 +212,7 @@ func TestMultiFilterAnd(t *testing.T) { filters := []argoprojiov1alpha1.SCMProviderGeneratorFilter{ { RepositoryMatch: strp("w"), + PathsExist: []string{"two"}, LabelMatch: strp("^prod-.*$"), }, } @@ -228,20 +228,34 @@ func TestMultiFilterOr(t *testing.T) { { Repository: "one", Labels: []string{"prod-one", "prod-two", "staging"}, + Branch: "devel", }, { Repository: "two", - Labels: []string{"prod-two"}, + Labels: []string{"prod-one", "prod-two", "staging"}, + Branch: "feature/ABC", + }, + { + Repository: "two", + Labels: []string{"prod-one", "prod-two", "staging"}, + Branch: "main", }, { Repository: "three", Labels: []string{"staging"}, + Branch: "feature/XYZ", + }, + { + Repository: "four", + Labels: []string{"dev"}, + Branch: "feature/IDK", }, }, } filters := []argoprojiov1alpha1.SCMProviderGeneratorFilter{ { RepositoryMatch: strp("e"), + BranchMatch: strp("feature/.*"), }, { LabelMatch: strp("^prod-.*$"), @@ -249,10 +263,11 @@ func TestMultiFilterOr(t *testing.T) { } repos, err := ListRepos(context.Background(), provider, filters, "") assert.Nil(t, err) - assert.Len(t, repos, 3) + assert.Len(t, repos, 4) assert.Equal(t, "one", repos[0].Repository) assert.Equal(t, "two", repos[1].Repository) - assert.Equal(t, "three", repos[2].Repository) + assert.Equal(t, "two", repos[2].Repository) + assert.Equal(t, "three", repos[3].Repository) } func TestNoFilters(t *testing.T) { @@ -280,40 +295,3 @@ 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, - } - pathDoesntExistsFilter := Filter{ - PathsDoNotExist: []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, &pathDoesntExistsFilter}) - - assert.Len(t, filterMap[FilterTypeRepo], 2) - assert.Len(t, filterMap[FilterTypeBranch], 4) -}