Skip to content
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.

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

feat: regex container name filtering #1241

Merged
merged 2 commits into from
Aug 14, 2022
Merged

feat: regex container name filtering #1241

merged 2 commits into from
Aug 14, 2022

Conversation

mateuszdrab
Copy link
Contributor

This small change allows the use of re2 regex in the container name filter parameter

I'm currently running this in my environment with the following cmdline
--cleanup --interval 21600 -d \/[^k][^8][^s][^_][\S]+

This prevents containers belonging to K8s from being auto updated as there is no way to enforce labels correctly in K8s.

I've been running this since end of January and had no problems so far - thought it would be nice to PR this.

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@mateuszdrab
Copy link
Contributor Author

Should we merge it or leave it?

@simskij
Copy link
Member

simskij commented Jun 21, 2022

Sorry Mateusz, I've been preoccupied with other things for a while. Will have a look as soon as time allows. Thank you for your contribution. 🙏🏼

@simskij simskij requested review from simskij and piksel June 21, 2022 12:40
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #1241 (c14d6df) into main (70ca4c2) will increase coverage by 1.84%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1241      +/-   ##
==========================================
+ Coverage   62.70%   64.55%   +1.84%     
==========================================
  Files          23       23              
  Lines        1488     1560      +72     
==========================================
+ Hits          933     1007      +74     
+ Misses        471      464       -7     
- Partials       84       89       +5     
Impacted Files Coverage Δ
pkg/filters/filters.go 88.88% <100.00%> (+7.63%) ⬆️
internal/actions/update.go 63.07% <0.00%> (-6.03%) ⬇️
pkg/notifications/slack.go 95.12% <0.00%> (+0.25%) ⬆️
pkg/notifications/shoutrrr.go 74.69% <0.00%> (+0.30%) ⬆️
internal/flags/flags.go 89.60% <0.00%> (+0.95%) ⬆️
pkg/container/client.go 30.80% <0.00%> (+2.88%) ⬆️
pkg/notifications/notifier.go 84.93% <0.00%> (+7.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@piksel
Copy link
Member

piksel commented Aug 1, 2022

Soo, to make this change backwards-compatible, we need to ensure that the whole name matches the regex, otherwise we would have containers which included the name as a substring match.
I added some tests to confirm this, and replaced the regexp.MatchStrings with *Regex.FindStringIndex and verifying the match indices.
Since the initial / is optional, the match needs to start at 0 or 1.

Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

I have handled the scenarios that I was worried about. @simskij ?

@piksel piksel changed the title Allow container name regex filtering feat: regex container name filtering Aug 14, 2022
@piksel piksel merged commit a429c37 into containrrr:main Aug 14, 2022
@mateuszdrab mateuszdrab deleted the enable-container-name-regex branch August 14, 2022 10:53
@mateuszdrab
Copy link
Contributor Author

mateuszdrab commented Aug 30, 2022

@piksel

Thanks for merging

Quick question, will there be a release any time soon?

If not, I'm happy to use the dev builds in the meantime; however, those are only built for amd64 - any chance to have cross platform dev builds?

Thanks

@piksel
Copy link
Member

piksel commented Sep 2, 2022

@mateuszdrab What platform do you need? arm64? If it's not giving me too much trouble it would of course be better to just do a multi-platform image, but otherwise I could just add a platform specific tag.

I have been piling up work for some time, so I can't give you a release date unfortunately.

@mateuszdrab
Copy link
Contributor Author

mateuszdrab commented Sep 2, 2022

@mateuszdrab What platform do you need? arm64? If it's not giving me too much trouble it would of course be better to just do a multi-platform image, but otherwise I could just add a platform specific tag.

I have been piling up work for some time, so I can't give you a release date unfortunately.

@piksel
armv7 actually, haven't switched the Raspberry Pis to 64 bit yet since its a mixture of 2s, 3s and a 4.

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.

None yet

3 participants