Implement registry and registry cache cleaning#253
Conversation
Test Results797 tests 797 ✅ 8m 59s ⏱️ Results for commit fdf2add. ♻️ This comment has been updated with latest results. |
bschwedler
left a comment
There was a problem hiding this comment.
This all makes sense to me.
| clean-caches: | ||
| name: Clean Caches | ||
| permissions: |
There was a problem hiding this comment.
Do you anticipate consuming workflows will invoke this as a separate job as is done here?
There was a problem hiding this comment.
Could be a separate job or a separate workflow? Not sure. I guess I'd lean for it to be a job that runs at the end of builds to clean up. Did you have a lean?
There was a problem hiding this comment.
For repos that trigger ci at least daily, running it there makes a lot of sense.
This job has no needs, so it will continue to clean up old images even if the builds fail. Yay!
| untagged_versions = package_versions.filter_untagged() | ||
| untagged_old_versions = untagged_versions.filter_older_than(remove_untagged_older_than) |
There was a problem hiding this comment.
Would it make sense to chain the methods here?
Example uses the method names without filter_
| untagged_versions = package_versions.filter_untagged() | |
| untagged_old_versions = untagged_versions.filter_older_than(remove_untagged_older_than) | |
| untagged_old_versions = package_versions.untagged().older_than(remove_untagged_older_than) |
There was a problem hiding this comment.
That would select items that are untagged and older than the given time. Don't we want to always remove images that are older than that date regardless of their tagging?
There was a problem hiding this comment.
Good point. I was thinking about this in terms of a subfilter.
For the cache, it makes sense to clean up anything older than a certain age.
For production repos we probably want to support this type of subselection. The logic of the larger function should help accomplish this though.
bschwedler
left a comment
There was a problem hiding this comment.
This looks good to me. A few small thoughts, but take 'em or leave 'em!
| clean-caches: | ||
| name: Clean Caches | ||
| permissions: |
There was a problem hiding this comment.
For repos that trigger ci at least daily, running it there makes a lot of sense.
This job has no needs, so it will continue to clean up old images even if the builds fail. Yay!
| def get_repositories(self, namespace: str = None) -> list[dict]: | ||
| if namespace is None: | ||
| namespace = self.identifier | ||
| target = urljoin(self.BASE_URL, self.ENDPOINTS["repositories"].format(namespace=namespace)) |
There was a problem hiding this comment.
What do you think about making this a class method?
| target = urljoin(self.BASE_URL, self.ENDPOINTS["repositories"].format(namespace=namespace)) | |
| target = urljoin(self.BASE_URL, self.endpoints("repositories", namespace=namespace)) |
| """Get details on a package.""" | ||
| target_url = self.ENDPOINTS["package"].format(organization=organization, package=quote(package, safe="")) | ||
| log.debug(f"GET {target_url}") | ||
| headers, response = self.client.requester.requestJsonAndCheck( |
There was a problem hiding this comment.
nit: unused variable
| headers, response = self.client.requester.requestJsonAndCheck( | |
| _, response = self.client.requester.requestJsonAndCheck( |
| organization=organization, package=quote(package, safe="") | ||
| ) | ||
| log.debug(f"GET {target_url} (page {page}/{page_count})") | ||
| headers, response = self.client.requester.requestJsonAndCheck( |
There was a problem hiding this comment.
nit:
| headers, response = self.client.requester.requestJsonAndCheck( | |
| _, response = self.client.requester.requestJsonAndCheck( |
| untagged_versions = package_versions.filter_untagged() | ||
| untagged_old_versions = untagged_versions.filter_older_than(remove_untagged_older_than) |
There was a problem hiding this comment.
Good point. I was thinking about this in terms of a subfilter.
For the cache, it makes sense to clean up anything older than a certain age.
For production repos we probably want to support this type of subselection. The logic of the larger function should help accomplish this though.
| "tag": "/namespaces/{namespace}/repositories/{repository}/tags/{tag}", | ||
| } | ||
|
|
||
| def __init__(self, identifier: str = None, secret: str = None): |
There was a problem hiding this comment.
There are several places in this PR where we should use the union type for nullalble params.
| def __init__(self, identifier: str = None, secret: str = None): | |
| def __init__(self, identifier: str | None = None, secret: str | None = None): |
This reverts commit 36f2745.
Co-authored-by: Benjamin R. J. Schwedler <ben@posit.co>
14c71a6 to
5ce8332
Compare
Closes #247