fix(appset): scmProvider generator filters not applying properly (#14420)

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

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
 }

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,31 +228,46 @@ 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-.*$"),
 		},
 	}
 	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: &regexp.Regexp{},
-		FilterType:  FilterTypeBranch,
-	}
-	repoFilter := Filter{
-		RepositoryMatch: &regexp.Regexp{},
-		FilterType:      FilterTypeRepo,
-	}
-	pathExistsFilter := Filter{
-		PathsExist: []string{"test"},
-		FilterType: FilterTypeBranch,
-	}
-	pathDoesntExistsFilter := Filter{
-		PathsDoNotExist: []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, &pathDoesntExistsFilter})
-
-	assert.Len(t, filterMap[FilterTypeRepo], 2)
-	assert.Len(t, filterMap[FilterTypeBranch], 4)
-}