containers
Refactor NewImageSource to add a manifest type abstraction
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Currently, NewImageSource creates a Docker schema2 manifest and an OCI manifest at the same time. This precludes functionality that isn't supported by both manifest types, for example zstd compression. Refactoring this to create only the desired manifest type solves this and also cleans up the code by separating manifest-type-specific code into distinct implementations of a "manifest builder".
How to verify it
Existing tests should suffice since this does not change any semantics, it only cleans up the implementation.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
See discussion in https://github.com/containers/buildah/pull/5452.
Does this PR introduce a user-facing change?
None
@rhatdan @mtrmac: Would it be possible to get some eyes on this when you have a moment? I was asked to do this to unblock #5452, so I would really appreciate if we could avoid letting it sit too long.
Any possibility of moving this forward? It's a bit frustrating that I was asked to come back and do this refactor, but the PR is not getting reviewed.
@aaronlehmann sorry for the delay. Many of the maintainers are Red Hat based, and we've all been under a big crunch the past few weeks.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@TomSweeneyRedHat @mtrmac @Honny1: Would any of you be able to look at this? I've been trying to get this through for a very long time.
@aaronlehmann Could you please rebase this first, just to be sure if every thing is good.
@flouthoc: Hoping I can get this merged before it needs yet another rebase :smile:
@nalind @flouthoc: Can we please move ahead with merging this? Thanks!
@nalind: Sorry to be so persistent here, but I'd really like to get this merged, since it has been dragging on for so long, and it's hard to keep coming back to this after long pauses.
@nalind @flouthoc: I'm grateful for your help moving this forward, but I have to say this has been a very frustrating experience. I'm making one last request for a final review so I can get this merged.
I’m afraid I can’t spend much time on this now, and this is probably locally not helpful: I happened to notice parallel https://github.com/containers/buildah/pull/6159 .
@aaronlehmann Apologies for the delay. The PR LGTM, I am rebasing this and getting this merged. This already has 2 LGTM.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: aaronlehmann, flouthoc, nalind
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [nalind]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
![openshift-ci[bot] avatar](https://avatars.githubusercontent.com/in/91280?v=4 "Published by a pub.dev verified publisher"){kind=small}
Ephemeral COPR build failed. @containers/packit-build please check.
![packit-as-a-service[bot] avatar](https://avatars.githubusercontent.com/in/29076?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks so much @flouthoc, I really appreciate it. This refactor was motivated by #5452, which now becomes a trivial change to the OCI manifest code path. I've rebased that PR - if you could take a look that would be awesome!