snok / container-retention-policy

GitHub action for pruning old GHCR container image versions.
MIT License
178 stars 29 forks source link

Unexpected behavior when filter-tags and dry-run are set #64

Closed chris-schra closed 1 year ago

chris-schra commented 1 year ago

First of all: thanks for the useful action. I'm currently integrating it in our chore repo and as we have different deployment environments (reflected in the image tags), my idea is, to filter-tags by environment name (from a matrix), like this:

    strategy:
      matrix:
        # use a matrix to keep 3 images per environment
        environment: ['develop', 'feature', 'prod']
    steps:
      - name: Delete all containers (per environment) older than a month
        uses: snok/container-retention-policy@v2
        with:
          image-names: 'my-cool-image'
          cut-off: One month ago UTC
          account-type: org
          org-name: my-cool-org
          dry-run: true
          filter-tags: '${{ matrix.environment }}-*'
          filter-include-untagged: false
          keep-at-least: 3
          token: ${{ secrets.PAT }}

It seems, the execution (in terms of filter-tags string building) works as expected: retention

I expected that the dry-run will only return a few results, but it seems filter-tags is not working and dry-run states it would delete all tags older than a month (can't tell if keep-at-least is honored). Is there something wrong with my logic or with the filter-tags arg?

sondrelg commented 1 year ago

The dry-run setting is pretty new and it seems it hasn't been implemented entirely correctly. See this section of code: https://github.com/snok/container-retention-policy/blob/main/main.py#L375:L391

I guess to behave correctly, it should be:

-if inputs.dry_run:
+if delete_image is True and inputs.dry_run:
    delete_image = False
    print(f'Would delete image {image_name}:{version.id}.')

A PR is welcome if you have time 👍

chris-schra commented 1 year ago

thanks for the hint, seems to work

Creating PR - with a disclaimer ;)

sondrelg commented 1 year ago

Thanks @chris-schra! If you haven't already, it'd be great if you tested the new version (which should be out in a few seconds). Thanks for the report and PR :clap:

chris-schra commented 1 year ago

@sondrelg thanks for the fast merge.. still in dry-run, but so far it looks pretty good. Thanks for the work!