Fix garbage-collect --delete-untagged to handle schema 2 manifest list and OCI image index #4285
Conversation
|
This is failing the linter and needs fixing |
|
@milosgajdos the issues reported by the linter should be fixed |
|
How is this pull request coming along? If some help is needed I'd be willing to contribute to this issue - just let me know. |
I don't understand what else is needed? all the checks have passed. Do I need to rebase onto origin/main ? |
The question was more directed at @milosgajdos since he seems to be the one assigned with this issue.
I think a rebase onto origin/main can't do any harm since it's necessary to rebase before merging anyways. Sorry if my previous comment sounded like I was trying to stress you out. I really appreciate your work on this issue! |
No, I am NOT assigned this issue. This is all done in everyone's free unpaid time. I merely mentioned the CI failure in my comment. |
|
@milosgajdos @WhySoBad so can anyone point me to two reviewers ? |
|
@thewolt We have no authority to merge but we reviewed and used your patch against 3.0.0-alpha and rolled it out to prod because it looked good, had tests that passed, worked in our additional testing and at that point any risk that posed was far less than the constant problems posed by a destructive garbage collection mechanism. It looks like milosgajdos can merge but I've looked through the merged PRs and he's the only one who has merged since February 29th when wy65701436 merged #4286. So sadly it seems he's been going it alone recently. There's a MAINTAINERS File and a GOVERNANCE document if you want to dig further or solicit more help somehow. |
I apologize for my faulty assumption. Based on your activity in this project and your github profile it seemed as you were employed by docker. |
I used to be employed by Docker inc. but they never paid me to work on this project. Assuming someone who works for Docker automatically qualifies as a person who works on this project is a false assumption. This project was donated to CNCF by Docker a few years ago. All the work I've done on this project has been in my free and unpaid time. No hurt feelings, I just need to clarify all the wrong assumptions made here for the future generations 😄 |
Personal fork for now while the upstream PR distribution/distribution#4285 is open.
|
There's some bottleneck in the CNCF process around review and merge. The sad irony is I DO pay people to work on CNCF projects ... most of our issues and PRs have gone ignored and closed by bots. I guess I should figure out how to get more involved in the CNCF but I've assumed since we don't work for some unicorn we'll be ignored. Especially given recent supply chain hysteria. But I digress, this isn't the venue for this discussion. |
There was a problem hiding this comment.
On a first look it LGTM. I need to give it another shot at some point this week if time permits. @deleteriousEffect @joaodrp mind having a look at this?
|
Can someone double check this PR and merge it finally? I feel this fix is somehow underrated unfortunately. There are plenty of people waiting for it but I guess none of them are allowed to click the merge button. So frustrating. |
|
PTAL @thaJeztah @squizzi |
|
Yeah, this bug caused a huge issue for us after we started building multiplatform containers. First garbage-collect after that was catastrophic. Please merge. |
|
@squizzi Thanks for the review, i've updated the test case commit with your suggestions |
|
@thewolt can you squash, please. |
Signed-off-by: Anthony Ramahay <thewolt@gmail.com>
@milosgajdos Squash done 👍 |
|
@milosgajdos when is it gonna be released? |
Short answer: there is no concrete plan Long answer: this project is run by unpaid volunteers who have their own full-time jobs, so it all depends on their time availability TBD |
| // fetch all tags from repository | ||
| // all of these tags could contain manifest in history | ||
| // which means that we need check (and delete) those references when deleting manifest | ||
| allTags, err := repository.Tags(ctx).All(ctx) | ||
| if err != nil { | ||
| if _, ok := err.(distribution.ErrManifestUnknownRevision); !ok { |
There was a problem hiding this comment.
There was a problem hiding this comment.
@microyahoo Sorry but I've never used Go before this PR.
When I brought the test cases from readerx, I found that adding this condition fixed the test and I looked no further.
Using the debugger, it's clear that the error i wanted to catch was indeed ErrRepositoryUnknown and the way i've put the condition is useless.
So yes, your PR is correct and provides a more useful message.
There was a problem hiding this comment.
Thanks for your clarification, I will update it.
resolves #3178