Skip to content

Do not list repositories if the repository filters are not regexes #354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Do not list repositories if the repository filters are not regexes #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions cmd/acr/purge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,11 +734,21 @@ func TestCollectTagFilters(t *testing.T) {
mockClient.AssertExpectations(t)
})

t.Run("SingleRepo", func(t *testing.T) {
t.Run("SingleRepoWithRegex", func(t *testing.T) {
assert := assert.New(t)
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
filters, err := common.CollectTagFilters(testCtx, []string{testRepo + ".?:.*"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(1, len(filters), "Number of found should be one")
assert.Equal(".*", filters[testRepo], "Filter for test repo should be .*")
assert.Equal(nil, err, "Error should be nil")
mockClient.AssertExpectations(t)
})

t.Run("SingleRepoWithoutRegex", func(t *testing.T) {
assert := assert.New(t)
mockClient := &mocks.BaseClientAPI{}
filters, err := common.CollectTagFilters(testCtx, []string{testRepo + ":.*"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(1, len(filters), "Number of found should be one")
assert.Equal(".*", filters[testRepo], "Filter for test repo should be .*")
Expand All @@ -764,7 +774,7 @@ func TestCollectTagFilters(t *testing.T) {
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
filters, err := common.CollectTagFilters(testCtx, []string{"ba:.*"}, mockClient, 60, defaultRepoPageSize)
filters, err := common.CollectTagFilters(testCtx, []string{"ba?:.*"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(0, len(filters), "Number of found repos should be zero")
assert.Equal(nil, err, "Error should be nil")
mockClient.AssertExpectations(t)
Expand All @@ -775,7 +785,7 @@ func TestCollectTagFilters(t *testing.T) {
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
filters, err := common.CollectTagFilters(testCtx, []string{"foo/bar:.*"}, mockClient, 60, defaultRepoPageSize)
filters, err := common.CollectTagFilters(testCtx, []string{"foo/bar?:.*"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(1, len(filters), "Number of found repos should be one")
assert.Equal(nil, err, "Error should be nil")
mockClient.AssertExpectations(t)
Expand All @@ -786,7 +796,7 @@ func TestCollectTagFilters(t *testing.T) {
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
filters, err := common.CollectTagFilters(testCtx, []string{"foo/bar:(?:.*)"}, mockClient, 60, defaultRepoPageSize)
filters, err := common.CollectTagFilters(testCtx, []string{"foo/bar?:(?:.*)"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(1, len(filters), "Number of found repos should be one")
assert.Equal(nil, err, "Error should be nil")
mockClient.AssertExpectations(t)
Expand Down Expand Up @@ -840,7 +850,7 @@ func TestCollectTagFilters(t *testing.T) {
assert := assert.New(t)
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(NoRepositoriesResult, nil).Once()
filters, err := common.CollectTagFilters(testCtx, []string{testRepo + ":.*"}, mockClient, 60, defaultRepoPageSize)
filters, err := common.CollectTagFilters(testCtx, []string{testRepo + "?:.*"}, mockClient, 60, defaultRepoPageSize)
assert.Equal(0, len(filters), "Number of found repos should be zero")
assert.Equal(nil, err, "Error should be nil")
mockClient.AssertExpectations(t)
Expand All @@ -849,8 +859,6 @@ func TestCollectTagFilters(t *testing.T) {
t.Run("EmptyRepoRegex", func(t *testing.T) {
assert := assert.New(t)
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
_, err := common.CollectTagFilters(testCtx, []string{":.*"}, mockClient, 60, defaultRepoPageSize)
assert.NotEqual(nil, err, "Error should not be nil")
mockClient.AssertExpectations(t)
Expand All @@ -859,8 +867,6 @@ func TestCollectTagFilters(t *testing.T) {
t.Run("EmptyTagRegex", func(t *testing.T) {
assert := assert.New(t)
mockClient := &mocks.BaseClientAPI{}
mockClient.On("GetRepositories", mock.Anything, "", mock.Anything).Return(ManyRepositoriesResult, nil).Once()
mockClient.On("GetRepositories", mock.Anything, mock.Anything, mock.Anything).Return(NoRepositoriesResult, nil).Once()
_, err := common.CollectTagFilters(testCtx, []string{testRepo + ".*:"}, mockClient, 60, defaultRepoPageSize)
assert.NotEqual(nil, err, "Error should not be nil")
mockClient.AssertExpectations(t)
Expand Down
31 changes: 28 additions & 3 deletions cmd/common/image_functions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,34 @@ func GetRepositoryAndTagRegex(filter string) (string, string, error) {

// CollectTagFilters collects all matching repos and collects the associated tag filters
func CollectTagFilters(ctx context.Context, rawFilters []string, client acrapi.BaseClientAPI, regexMatchTimeout int64, repoPageSize int32) (map[string]string, error) {
allRepoNames, err := GetAllRepositoryNames(ctx, client, repoPageSize)
if err != nil {
return nil, err
var allRepoNames []string

// Bypass listing the repositories if there are no regex in the repository name filters
notRegexRegex := regexp2.MustCompile(`^[a-z0-9-\/]+$`, defaultRegexpOptions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Regular characters are still considered a valid regex, however in this case a search is not necessary. In light of that and the fact that using notRegexRegex is a bit redundant, maybe check for if this is a single repo name? like isSingleRepoNameRegex?

listRepositories := false
for _, filter := range rawFilters {
repoRegex, _, err := GetRepositoryAndTagRegex(filter)
if err != nil {
return nil, err
}

notRegex, err := notRegexRegex.MatchString(repoRegex)
if err != nil {
return nil, err
}
if !notRegex {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates an odd double negative. Following what I noted above, maybe use isSingleRepoName?

listRepositories = true
break
}

allRepoNames = append(allRepoNames, repoRegex)
}

if listRepositories {
var err error
if allRepoNames, err = GetAllRepositoryNames(ctx, client, repoPageSize); err != nil {
return nil, err
}
}

tagFilters := map[string]string{}
Expand Down