Skip to content

Feature: added exclude-filter argument to keep certain image tags#60

Closed
angrox wants to merge 8 commits intoAzure:masterfrom
angrox:feature-add-exclude-filters
Closed

Feature: added exclude-filter argument to keep certain image tags#60
angrox wants to merge 8 commits intoAzure:masterfrom
angrox:feature-add-exclude-filters

Conversation

@angrox
Copy link

@angrox angrox commented Nov 13, 2019

Purpose of the PR

Golang does not support the full capability of perl regex to filter image tags, e.g. to negate a regex. A new flag is added to exclude tags found by the --filter argument. This is useful when you want to keep certain images, e.g. all image tags which are currently in use by a K8s cluster.

Fixes #

@msftclas
Copy link

msftclas commented Nov 13, 2019

CLA assistant check
All CLA requirements met.

| Untag tags that include hello-world in their name | --filter `"<repository>:hello-world"` |
| Untag all tags that are older than the duration | --filter `"<repository>:.*"` |

| Exclude all tags from untagging that end with -prod | --exclude-filter `".*-prod$"` |
Copy link
Member

Choose a reason for hiding this comment

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

nit: -prod


##### Exclude filter tag

To exclude tags from untagging one or more --exclude-filter arguments can be added
Copy link
Member

Choose a reason for hiding this comment

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

nit: --exclude-filter for better representation.

}
isExcluded := false
for _, exFilter := range excludeFilter {
exFilterRegex, exFilterErr := regexp.Compile(exFilter)
Copy link
Member

Choose a reason for hiding this comment

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

exFilter is complied unnecessarily multiple times. You should pre-compile it like filter.

acrClient api.AcrCLIClientInterface,
repoName string,
filter *regexp.Regexp,
excludeFilter []string,
Copy link
Member

Choose a reason for hiding this comment

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

You should pass a *regexp.Regexp instead of []string for excludeFilter. The reason is explained in the next comment.

continue
}
}
if isExcluded {
Copy link
Member

Choose a reason for hiding this comment

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

Once the excludeFilter is a *regexp.Regexp, the variable isExcluded can be simplified.

mockClient := &mocks.AcrCLIClientInterface{}
mockClient.On("GetAcrTags", testCtx, testRepo, "", "").Return(notFoundTagResponse, errors.New("testRepo not found")).Once()
deletedTags, err := purgeTags(testCtx, mockClient, testLoginURL, testRepo, "1d", "[\\s\\S]*")
deletedTags, err := purgeTags(testCtx, mockClient, testLoginURL, testRepo, "1d", "[\\s\\S]*", nil)
Copy link
Member

Choose a reason for hiding this comment

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

You need real test cases instead of simply passing nil to it.

@fiv21
Copy link

fiv21 commented Aug 20, 2021

How can we help to get this implemented in the short term? 🤔

@sashokbg
Copy link

Hello @northtyphoon , why was this MR closed ? Can we finish the work (I can try if needed) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants