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

Add script to clean up quay.io. #3772

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DailyDreaming
Copy link
Member

I just deleted a few years worth of CI/CD cruft with this. We shouldn't need to run it too often, so I'm advocating for keeping this manual (maybe only running it when releasing a new version).

Changelog Entry

To be copied to the draft changelog by merger:

  • Add script to clean up quay.io.

Reviewer Checklist

  • Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
    • If it is coming from an external repo, make sure to pull it in for CI with:
      contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
      
    • If there is no associated issue, create one.
  • Read through the code changes. Make sure that it doesn't have:
    • Addition of trailing whitespace.
    • New variable or member names in camelCase that want to be in snake_case.
    • New functions without type hints.
    • New functions or classes without informative docstrings.
    • Changes to semantics not reflected in the relevant docstrings.
    • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
    • New features without tests.
  • Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
  • Finish the review with an overall description of your opinion.

Merger Checklist

  • Make sure the PR passes tests.
  • Make sure the PR has been reviewed since its last modification. If not, review it.
  • Merge with the Github "Squash and merge" feature.
    • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
  • Copy its recommended changelog entry to the Draft Changelog.
  • Append the issue number in parentheses to the changelog entry.

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

This looks pretty good, although I'm not quite sure about the big blocklist of tags to never delete, and whether it does anything.

Also, do we care about whether we've told anyone to use a particular recent CI build to work around a problem with no released fix? As is, this script will delete even quite recent CI builds that people might be using. Maybe we shouldn't delete anything newer than the most recent real release?



# more of a comment; this list really doesn't need to be updated since we filter by length.
whitelist = [
Copy link
Member

Choose a reason for hiding this comment

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

This name could be a bit more descriptive, and I think the semantics are backward. These are the ones we always keep, and never delete, so we should call this ALWAYS_KEEP or IGNORED_TAGS or BLOCKLIST or something. The way it is now it looks a bit like these are the ones that the deleter is allowed to delete.

Comment on lines +122 to +123
non_release_minimum_tag_length = len('5.5.0a1-dce7a69af77da45c4ab1939e304ec92865ad9c57-py3.7')
if len(tag) >= non_release_minimum_tag_length and tag not in whitelist and dev_version:
Copy link
Member

Choose a reason for hiding this comment

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

Nothing in the list is as long as this minimum tag length; do we even need the list at all?


if user_reply == 'y':
for tag in tags:
response = requests.delete(f'https://quay.io/api/v1/repository/{repo}/tag/{tag}', headers=headers)
Copy link
Member

Choose a reason for hiding this comment

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

If the list is meant to be a second safety interlock, maybe we should check against it here instead?

tags.append(tag)
print(tag)

user_reply = input('\nDo you wish to delete these tags?\n\n')
Copy link
Member

Choose a reason for hiding this comment

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

This should have a (y/n), or someone will type yes or Y and be annoyed.

tags = []
for tag in response.json()['tags']:
tag = tag.strip()
dev_version = tag.split('-')[0].endswith('a1')
Copy link
Member

Choose a reason for hiding this comment

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

Are we ever going to actually release an a1 and get bumped to a2?

@adamnovak
Copy link
Member

Hey @DailyDreaming, it turns out that #3781 (comment) wasn't happening because @jpuerto-psc pulled 87293d63fa6c76f03bed3adf93414ffee67bf9a7 which is some weird commit.

It turns out that 87293d63fa6c76f03bed3adf93414ffee67bf9a7 is actually the tag releases/5.4.0, and that toil.applianceSelf() in the 5.4.0 release returns quay.io/ucsc_cgl/toil:5.4.0-87293d63fa6c76f03bed3adf93414ffee67bf9a7-py3.6. You can check this for that release and many others by doing:

virtualenv --python python3 testvenv
. testvenv/bin/activate
pip install toil==5.4.0
python -c 'import toil; print(toil.applianceSelf())'
# quay.io/ucsc_cgl/toil:5.4.0-87293d63fa6c76f03bed3adf93414ffee67bf9a7-py3.6

It looks like our releases have been referencing themselves more or less forever through the full tag including commit information, and not via the short tags like quay.io/ucsc_cgl/toil:5.4.0-py3.6. Unfortunately, we've now deleted all of those full tags, and @jpuerto-psc is the only person who has noticed.

So this PR should probably change Toil over to point to itself via the short tag, and we should find a way to restore the longer tags that point to release Toil versions and to skip over them when doing cleanup in the future.

@adamnovak adamnovak marked this pull request as draft January 31, 2024 16:06
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.

2 participants