Add a dry-run flag #62
Conversation
…ng unintended images.
Codecov Report
@@ Coverage Diff @@
## main #62 +/- ##
==========================================
+ Coverage 93.15% 93.27% +0.12%
==========================================
Files 1 1
Lines 219 223 +4
Branches 53 54 +1
==========================================
+ Hits 204 208 +4
Misses 7 7
Partials 8 8
|
main.py
Outdated
| delete_image = False | ||
| image_name_with_tag = f'{image_name}:{version.id}' | ||
| print(f'Would delete image {image_name_with_tag}.') | ||
| deleted.append(image_name_with_tag) |
There was a problem hiding this comment.
What's the purpose of this append? Could we not just skip this?
There was a problem hiding this comment.
@sondrelg yep, happy to remove that. That was what I was commenting on in the initial PR comments with The image is added to the deleted list to follow what would happen as closely as possible, but I would be happy to remove that if desired since the image was not actually deleted.
I'll pull that out this evening.
There was a problem hiding this comment.
Sorry, trying to review between other tasks, so just didn't have the time to look at whether there was a benefit. If there is, I'm open to keeping it 👍
There was a problem hiding this comment.
Ah ok, I didn't manage to find the 2 seconds to pull it out last night either. The goal was to have the same data in place at the end of the run as what would have happened if the images really had been deleted.
That seems useful to me, but I can definitely see the reasoning to remove that line because the image wasn't really deleted and so to not have to maintain doing that in two places, though. I could also throw a quick comment before that line explaining why it's being done. Whatever your preference is, I'm happy to go with
There was a problem hiding this comment.
Since the list is used here to output which images were actually deleted, I suggest we remove it 👍 Could be misleading otherwise I think
|
I don't have any images hosted on ghcr anymore, so I can't really test this on any of my own projects. Would you mind doing a quick test run with: ..in your workflow? It's hopefully redundant, but seems worth the extra 5 minutes to save everyone else broken workflows. |
|
Happy to merge this after that 👍 |
No problem I'm also using my fork and branch for now for a project at work, but that is of course all private repos. |
|
Thanks @jmichalicek! |
This adds a simple dry-run flag to allow for testing to easily see what would be deleted. The check for a dry run and skip of actual removal could be done even later/deeper in, but this was a very easy and clear place to put it. The image is added to the
deletedlist to follow what would happen as closely as possible, but I would be happy to remove that if desired since the image was not actually deleted.