-
Notifications
You must be signed in to change notification settings - Fork 770
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
use c/image/pkg/blobcache #3795
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Any reason to not wait until it's in an actually-tagged release of the image library? Please add a type alias and |
My memory is still fresh on it, and I fear we'd forget about it.
Note that the BlobCache in c/image is a struct, not an interface. It should compile just fine but strictly speaking it's an API break. Still cool with it? |
Dependabot will catch it. Does the struct not implement the interface? |
func cacheLookupReferenceFunc(directory string, compress types.LayerCompression) libimage.LookupReferenceFunc { | ||
// NOTE: this prevents us from moving BlobCache around and generalizes | ||
// the libimage API. | ||
return func(ref types.ImageReference) (types.ImageReference, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return ref
if directory
is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependabot will catch it.
I am more afraid of forgetting about the manual work to update the code to use the new package.
Does the struct not implement the interface?
The ClearCache
was made private but I think we can make it public again. @mtrmac are you cool with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return
ref
ifdirectory
is empty.
BTW that could simplify the callers.
func cacheLookupReferenceFunc(directory string, compress types.LayerCompression) libimage.LookupReferenceFunc { | ||
// NOTE: this prevents us from moving BlobCache around and generalizes | ||
// the libimage API. | ||
return func(ref types.ImageReference) (types.ImageReference, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return
ref
ifdirectory
is empty.
BTW that could simplify the callers.
(Thinking about the Should the existing Buildah callers use Buildah’s wrapper, or call the c/image version directly?
Yes; I can’t see a strong reason not to expose that. |
To allow for callers of Buildah's pkg/blobcache to continue building [1]. [1] containers/buildah#3795 (comment) Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
We're a library, so it's supposed to be a principle that we compromise on only rarely and with care. Historically, not everyone agrees with that, and I make mistakes, too. |
Makes sense, thanks. |
Repushed with a type alias and forwarding of NewBlobCache to c/image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation mechanism LGTM. I understood the discussion to mean that Buildah’s type should remain an interface — up to @nalind .
Let's do that. |
LGTM |
Implementation looks fine, but again, I'd rather we wait until this is in an actual release of the image library. |
Let’s do that: containers/image#1482 . |
@vrothberg c/image 5.20.0 has been released. |
To be frank, I don't think it's a good policy to only merge c/image if there's a release. We don't do it for c/storage either and it's slowing down development. |
Thanks for taking care! |
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
Buildah's pkg/blobcache has been moved into containers/image [1] to consolidate implementations of interfaces such as image destinations and image sources. Since there are no functional changes: [NO NEW TESTS NEEDED] [1] containers/image#1459 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
/lgtm |
Buildah's pkg/blobcache has been moved into containers/image to
consolidate implementations of interfaces such as image destinations
and image sources.
Create a type alias in pkg/buildcache and redirect NewBlobCache to the
containers/image function. While it is still an API break as it changes
an interface to a struct, known callers will continue building since
they are not implementing the interface.
Since there are no functional changes: [NO NEW TESTS NEEDED]
[1] containers/image#1459
Signed-off-by: Valentin Rothberg vrothberg@redhat.com