fix: filters skipped when more than one is specified (#521)

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

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
-}

pkg/services/scm_provider/utils_test.go

@@ -257,35 +257,36 @@ func TestNoFilters(t *testing.T) {
 	assert.Equal(t, "three", repos[2].Repository)
 }
 
-// tests the getApplicableFilters function, passing in all the filters, and an unset filter, plus an additional
+// tests the filters segmentation functions, passing in all the filters, and an unset filter, plus an additional
 // branch filter
 func TestApplicableFilterMap(t *testing.T) {
 	branchFilter := Filter{
 		BranchMatch: &regexp.Regexp{},
-		FilterType:  FilterTypeBranch,
 	}
 	repoFilter := Filter{
 		RepositoryMatch: &regexp.Regexp{},
-		FilterType:      FilterTypeRepo,
 	}
 	pathExistsFilter := Filter{
 		PathsExist: []string{"test"},
-		FilterType: FilterTypeBranch,
 	}
 	labelMatchFilter := Filter{
 		LabelMatch: &regexp.Regexp{},
-		FilterType: FilterTypeRepo,
 	}
 	unsetFilter := Filter{
 		LabelMatch: &regexp.Regexp{},
 	}
 	additionalBranchFilter := Filter{
 		BranchMatch: &regexp.Regexp{},
-		FilterType:  FilterTypeBranch,
 	}
-	filterMap := getApplicableFilters([]*Filter{&branchFilter, &repoFilter,
-		&pathExistsFilter, &labelMatchFilter, &unsetFilter, &additionalBranchFilter})
+	bothFilter := Filter{
+		RepositoryMatch: &regexp.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)
 }