keep-at-least parameter accounts only versions marked for deletion, not all versions

[yma-het]

In my opinion, this is not that behavior what user expects to see.

1. We are creating tasks: https://github.com/snok/container-retention-policy/blob/main/main.py#L431C19-L443
2. Remove versions from list marked for deletion: https://github.com/snok/container-retention-policy/blob/main/main.py#L446-L449

[sondrelg]

You're saying we're keeping too many images, when some tags are filtered out?

[yma-het]

When i'm reading the name of param "keep-at-least", i expect that all of images will be accounted in this number, not only marked for deletion. So yes, correct, too many images. May be it is better to rename this param or make effect of this parameter more predictable.

[sondrelg]

Would you like to contribute a fix? Sounds like we could change the counting a little, and maybe have two iterations over the set of images, and it should be possible to make this work as you assumed.

[yma-het]

#68
Here is it

[lovromazgon]

The change in #68 has created a regression, it caused us to delete 25 images that would otherwise not be deleted. Here's the CI action run the day after the release of v2.1.3.

Our configuration is:

image-names: conduit
cut-off: 1 week ago UTC
account-type: org
org-name: ConduitIO
keep-at-least: 5
token: ${{ secrets.NIGHTLY_BUILD_GH_TOKEN }}
filter-tags: '*-nightly*'

Given filter-tags I would expect only nightly tags to be touched, but that was not the case.

On that note, it would be nice to have a failsafe option where you can configure the highest amount of deleted images. If that amount is exceeded I'd expect the action to fail, which would cause a maintainer to be notified about an anomaly. It would certainly make the action safer since a bug can have devastating results.

[sondrelg]

Both a fix and a failsafe would be welcome. I'm unavailable for the next few days, but if anyone is able to put together either, I'll make sure they're reviewed and merged asap 👍

[sondrelg]

Hi everyone,

I'm closing in on a fix for most of the open issues, and after reviewing all of our settings I'm not really seeing the benefit of a keep-at-least argument for this action (after a rewrite). Could you help me understand what the benefit of this setting is to you? 🙏

A bit of necessary context:

• A container registry can only assign a tag to one package version. Once you upload a new latest, the old loses ownership of that tag.
• The keep-at-least setting does not protect against untagged package versions being deleted.

Reasons keep-at-least no longer seems useful

• If you want package versions attached to docker tags to not be deleted, there is an argument called untagged-only (will be renamed to tag-selection) which will allow you to keep all package versions with tags (if you never, e.g., want latest to be deleted).
• In the rewrite we will have better support for filtering on which package names and/or image tags to keep and which to delete.

After the rewrite, you will be able to specify something like:

    --account snok \
    --tag-selection both \  # untagged + tagged
    --image-names "container-retention-policy* !foo"  \
    --image-tags "!latest !test-1*" \
    --cut-off 1w \

Where specifically the latest tag and any test-1-* (where * is a wildcard) tag is protected.

So keep-at-least seems like an imprecise (and redundant) way of going about keeping your tagged images. Do you agree with this, or do you have a use-case that would not be satisfied by this new API. All feedback is appreciated 🙇

[sondrelg]

@lovromazgon, your problem will be addressed in the new release 👍 Sorry about that.

[lovromazgon]

I'll describe our use case in Conduit. We cut nightly releases every night, that include creating a docker image among other assets. Nightlies are only relevant for a short period of time, if we want to try out or showcase a feature we're working on, maybe testing regressions, performance improvements etc. We don't want to keep them around for too long, so we use this action to clean up older nightlies.

Now, in our case we have a regular nightly release schedule, so we know exactly how many releases happened in a timespan. Specifying cut-off or keep-at-least is essentially the same in our case. That said, I imagine there are projects that don't trigger releases based on a schedule but rather cut a release based on other irregular events (e.g. pushes to main). In that case, you might not know how many releases happened in a certain timespan, but you might want to keep at least the last X releases, even if they are older. To illustrate a more specific example, when people go on vacation and no releases are cut, you might still want to keep the last X releases. Would this still be possible?

Once again, to be clear, I personally don't care about this functionality, as cut-off already scratches our itch, I'm just speculating that keep-at-least might still have a place.

[sondrelg]

Hmm that's a good point @lovromazgon. I think one valid use-case that resembles your example would be if you want to keep the n latest releases, so that you're able to roll back n releases in kubernetes.

[sondrelg]

This issue has been fixed in the v3 release 👍 Sorry for the delay!

The migration guide for v3 is included in the release post 👍 If you run into any issues, please share them in the issue opened for tracking the v3 release ☺️