Add `--keep` argument to prevent deleting images #151

keelerm84 · 2021-06-17T16:39:04Z

--keep is a new argument that allows users to specify one or more
regular expressions which, when matched against an image name, will
prevent that image from being deleted.

This is particularly useful for images that may require long re-build
times and so should not be cleaned up as part of normal system
maintenance.

Status: Ready

Fixes: #129

keelerm84 · 2021-06-17T16:43:47Z

src/main.rs

+                .value_name("KEEP")
+                .short("k")
+                .long(KEEP_OPTION)
+                .multiple(true)


Not sure if you'll like this structure, but I went with a multiple argument (with one value each) so the usage is:

--keep regex1 --keep regex2

I thought this might be preferable to having a single very convoluted regex for more complex setups.

Seems reasonable.

stepchowfun · 2021-06-18T20:44:26Z

Thank you for this contribution! I will take a look as soon as I can.

keelerm84 · 2021-06-19T03:21:43Z

Thank you for this contribution! I will take a look as soon as I can.

My pleasure. This is my first open source contribution to the rust community so please don't hesitate to let me know if you'd rather I do something differently. And thank you for this project; it has been quite helpful.

stepchowfun · 2021-07-12T20:07:33Z

Hi @keelerm84, sorry for the long delay on this. Would you mind rebasing onto the latest commit?

`--keep` is a new argument that allows users to specify one or more regular expressions which, when matched against an image name, will prevent that image from being deleted. This is particularly useful for images that may require long re-build times and so should not be cleaned up as part of normal system maintenance.

keelerm84 · 2021-07-12T22:09:03Z

Hey @stepchowfun, no problem. I have updated the commit as requested. Thanks for following up on this!

stepchowfun

Overall seems reasonable. I left a few suggestions.

But one thing I'm wondering is: why does --keep filter on the repository name? Why not repository:tag, so that users could specify that they want to keep only a specific image, not all images from the repository?

stepchowfun · 2021-07-12T23:59:24Z

src/main.rs

@@ -102,6 +105,16 @@ fn settings() -> io::Result<Settings> {
                    DEFAULT_THRESHOLD.code_str(),
                )),
        )
+        .arg(
+            Arg::with_name(KEEP_OPTION)
+                .value_name("KEEP")


I think the value name should be REGEX (or similar).

stepchowfun · 2021-07-13T00:01:57Z

src/main.rs

+                .multiple(true)
+                .number_of_values(1)
+                .required(false)
+                .help("Regular expression of repository names to keep despite space constraints"),


Can we use the same verb tense/usage as the other flags? So maybe something like:

Prevents Docuum from deleting images from repositories matching <REGEX>

stepchowfun · 2021-07-13T00:02:28Z

src/main.rs

+                .value_name("KEEP")
+                .short("k")
+                .long(KEEP_OPTION)
+                .multiple(true)


Seems reasonable.

stepchowfun · 2021-07-13T00:04:25Z

src/main.rs

+                .short("k")
+                .long(KEEP_OPTION)
+                .multiple(true)
+                .number_of_values(1)


Is this not the default when value_name is specified? What happens if this is omitted?

If you remove this, you can actually specify multiple with the same flag. So with my latest change you can now do:

--keep pattern1 pattern2 pattern3 --keep pattern4 pattern5

Oh I see—thanks for explaining it. Actually, would you mind changing it back? I'm worried about how this will interact with other flags that might be added in the future. Having one pattern per flag seems like a safer choice.

stepchowfun · 2021-07-13T00:04:53Z

src/main.rs

+                .long(KEEP_OPTION)
+                .multiple(true)
+                .number_of_values(1)
+                .required(false)


Wondering the same thing about this: isn't this the default?

stepchowfun · 2021-07-13T00:08:21Z

src/main.rs

+            Ok(set) => Some(set),
+            Err(_) => {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidInput,


While we're here, would you mind changing line 127 to io::ErrorKind::InvalidInput as well? I think it's a better choice than what I had there (io::ErrorKind::Other).

stepchowfun · 2021-07-13T00:12:31Z

src/main.rs

+            Err(_) => {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidInput,
+                    "Invalid regex format provided",


How ugly would it be if we just used the error message from RegexSet::new? I think it would be good to include the original error message so the user knows what they messed up, but not if it's too ugly.

I made this change. The format isn't very ugly and it is way more helpful. Example:

[2021-07-13 21:17:11 -04:00 ERROR] regex parse error: (\bkong\b| ^ error: unclosed group

Hah, I think it's kind of ugly since the message isn't capitalized and isn't a sentence, unlike all the error messages we emit. But I agree it's more helpful!

stepchowfun · 2021-07-13T00:14:01Z

src/run.rs

+                } else {
+                    return Err(io::Error::new(
+                        io::ErrorKind::Other,
+                        "Failed to split image ID and date.",


Can we update this error message? Maybe just Failed to parse image list output from Docker. or something like that.

stepchowfun · 2021-07-13T00:21:19Z

src/run.rs

+                        "Skipping image {} which matches one or more of the provided regexs",
+                        image_node.image_info.repository
+                    );
+                    return false;


If you don't mind, I have a slight preference for formatting the lambda like this (instead of the imperative-style return):

if regex_set.is_match(&image_node.image_info.repository) { { info!(...); false } else { true }

stepchowfun · 2021-07-13T00:26:45Z

src/run.rs

+                if regex_set.is_match(&image_node.image_info.repository) {
+                    info!(
+                        "Skipping image {} which matches one or more of the provided regexs",
+                        image_node.image_info.repository


(The failing lint task wants you to put a trailing comma at the end of this line.)

keelerm84 · 2021-07-14T01:14:53Z

Thank you for the thorough and detailed feedback. I believe I have addressed each bit as requested. Happy to change anything else you'd like.

stepchowfun · 2021-07-14T16:55:56Z

src/run.rs

+                    info!(
+                        "Skipping image {} which matches one or more of the provided regexs",
+                        image_node.image_info.repository_tag
+                    );


How about:

info!( "Ignored image {} due to the {} flag.", image_node.image_info.repository_tag.code_str(), "--keep".code_str(), );

(There was missing punctuation and a missing trailing comma, and I think it is good to reference the flag that caused the image to be skipped. Also "regexs" seemed like a strange pluralization to me.)

stepchowfun · 2021-07-14T17:05:27Z

Looking great to me. I added a couple more comments, but otherwise I think this is pretty much there.

keelerm84 · 2021-07-15T02:14:43Z

I appreciate your continued patience while I work through these issues for you. Thank you, and please let me know if there is anything else you'd like me to address!

stepchowfun · 2021-07-15T02:38:42Z

src/main.rs

+                .long(KEEP_OPTION)
+                .multiple(true)
+                .number_of_values(1)
+                .help("Prevents Docuum from deleting repository:tag images that match the provided <REGEX>"),


Seems like the linter is complaining that this line is too long. Can we break up the string somewhere?

stepchowfun · 2021-07-15T02:41:47Z

I think you're the patient one here. Thanks for being receptive to all the feedback.

BTW, if you want to run all the linters and stuff locally (instead of waiting for me to approve the CI jobs), I recommend installing Toast. Then you can just run toast lint in the repository.

I'll merge this PR once it passes the CI checks.

stepchowfun · 2021-07-15T02:44:59Z

src/run.rs

@@ -543,6 +550,26 @@ fn vacuum(state: &mut State, threshold: &Byte) -> io::Result<()> {
            .then(y.ancestors.cmp(&x.ancestors))
    });

+    // If the user provided the keep argument, we need to filter out those images which match the


Tiny nit: could you change it to this:

// If the user provided the `--keep` argument, ...

(Add the dashes and backticks.)

keelerm84 · 2021-07-15T14:23:13Z

Installed and linted locally without complaint so 🤞

stepchowfun · 2021-07-15T14:38:55Z

src/run.rs

-                    parent_id: None, // This will be populated below.
-                    created_since_epoch: parse_docker_date(&date_str)?,
-                });
+                let image_parts = trimmed_line.split('\t').take(3).collect::<Vec<_>>();


I'm thinking it might be safer to remove the .take(3), so that the pattern match on the following line will fail (as it should) if there are more than 3 parts in the line.

stepchowfun · 2021-07-15T19:16:51Z

Seems like there is something we overlooked: the same image can have multiple tags. So unfortunately what we have here is not quite right (I think): an image might still be deleted even if one of its tags matches the regex(es), because another one of its tags might not.

I am going to hold off on releasing a new version until I can fix this. If there isn't an easy fix, I might have to revert this PR—but hopefully I can fix the problem instead of doing that.

stepchowfun · 2021-08-02T10:29:44Z

I've fixed the aforementioned issue in #168 and released this new feature in v0.20.0. Unfortunately the fix was fairly involved, which is why it took me a few weeks to do. But I think now the code in better shape thanks to the the events set in motion by this PR. So overall I'm glad we went on this journey.

keelerm84 · 2021-08-04T13:34:29Z

Thank you for addressing the issue and having worked with me as much as you did.

As I mentioned before, this was my first open source contribution to a rust project and I want to thank you again for being as welcoming and accommodating as you were.

keelerm84 commented Jun 17, 2021

View reviewed changes

keelerm84 force-pushed the feature/add-keep-flag branch from c460ed5 to ce93af6 Compare July 12, 2021 22:08

stepchowfun requested changes Jul 13, 2021

View reviewed changes

stepchowfun reviewed Jul 13, 2021

View reviewed changes

keelerm84 added 4 commits July 13, 2021 21:13

Simplify arg flags

3454e5c

Tweak error reporting

33ce274

Match on repository:tag combinations

402914d

Merge branch 'main' into feature/add-keep-flag

c009598

stepchowfun reviewed Jul 14, 2021

View reviewed changes

keelerm84 added 3 commits July 14, 2021 22:08

Revert change so keep is one pattern per flag

3ffc266

Change image skipped message

7b57e53

Merge branch 'main' into feature/add-keep-flag

5dbf951

keelerm84 requested a review from stepchowfun July 15, 2021 02:13

stepchowfun reviewed Jul 15, 2021

View reviewed changes

Short help message

ae486e3

stepchowfun reviewed Jul 15, 2021

View reviewed changes

stepchowfun merged commit b6dc704 into stepchowfun:main Jul 15, 2021

keelerm84 deleted the feature/add-keep-flag branch August 4, 2021 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--keep` argument to prevent deleting images #151

Add `--keep` argument to prevent deleting images #151

keelerm84 commented Jun 17, 2021 •

edited

Loading

keelerm84 Jun 17, 2021

stepchowfun Jul 13, 2021

stepchowfun commented Jun 18, 2021

keelerm84 commented Jun 19, 2021

stepchowfun commented Jul 12, 2021

keelerm84 commented Jul 12, 2021

stepchowfun left a comment

stepchowfun Jul 12, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

keelerm84 Jul 14, 2021

stepchowfun Jul 14, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

keelerm84 Jul 14, 2021

stepchowfun Jul 14, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

stepchowfun Jul 13, 2021

keelerm84 commented Jul 14, 2021

stepchowfun Jul 14, 2021

stepchowfun commented Jul 14, 2021

keelerm84 commented Jul 15, 2021

stepchowfun Jul 15, 2021

stepchowfun commented Jul 15, 2021

stepchowfun Jul 15, 2021

keelerm84 commented Jul 15, 2021

stepchowfun Jul 15, 2021

stepchowfun commented Jul 15, 2021

stepchowfun commented Aug 2, 2021

keelerm84 commented Aug 4, 2021

Add --keep argument to prevent deleting images #151

Add --keep argument to prevent deleting images #151

Conversation

keelerm84 commented Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stepchowfun commented Jun 18, 2021

keelerm84 commented Jun 19, 2021

stepchowfun commented Jul 12, 2021

keelerm84 commented Jul 12, 2021

stepchowfun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keelerm84 commented Jul 14, 2021

Choose a reason for hiding this comment

stepchowfun commented Jul 14, 2021

keelerm84 commented Jul 15, 2021

Choose a reason for hiding this comment

stepchowfun commented Jul 15, 2021

Choose a reason for hiding this comment

keelerm84 commented Jul 15, 2021

Choose a reason for hiding this comment

stepchowfun commented Jul 15, 2021

stepchowfun commented Aug 2, 2021

keelerm84 commented Aug 4, 2021

Add `--keep` argument to prevent deleting images #151

Add `--keep` argument to prevent deleting images #151

keelerm84 commented Jun 17, 2021 •

edited

Loading