-
Notifications
You must be signed in to change notification settings - Fork 527
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
Support a mode to emit estargz automagically. #871
Conversation
3839d9e
to
22cd44c
Compare
Codecov Report
@@ Coverage Diff @@
## master #871 +/- ##
==========================================
- Coverage 74.34% 74.33% -0.02%
==========================================
Files 107 107
Lines 4486 4523 +37
==========================================
+ Hits 3335 3362 +27
- Misses 656 661 +5
- Partials 495 500 +5
Continue to review full report at Codecov.
|
fcf725e
to
b3c2a25
Compare
b3c2a25
to
2047c65
Compare
@@ -31,7 +34,7 @@ import ( | |||
// | |||
// Refer to estargz for the options: | |||
// https://pkg.go.dev/github.com/containerd/stargz-snapshotter@v0.2.0/estargz#Option | |||
func ReadCloser(r io.ReadCloser, opts ...estargz.Option) (io.ReadCloser, v1.Hash, error) { | |||
func ReadCloser(r io.ReadCloser, opts ...estargz.Option) (*estargz.Blob, v1.Hash, 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.
Hmm... so all we're doing here is actually implementing func(io.ReadCloser) (io.SectionReader, error)
...
Do we really need this? If we want to keep it, can we update the return signature to match estargz.Build
? I'd honestly rather they change their API and take advantage of any implemented interfaces, and then we just drop this entirely.
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.
This is a projection to our idioms, e.g. taking io.ReadCloser
and returning v1.Hash
(they cannot adopt the latter without a dependency cycle, but we could switch to using go-digest
everywhere).
As #876 evolves, perhaps the necessity of this will diminish, but I think it's a useful place for us to mitigate things in the interim, and since it's internal there's effectively zero risk that we can't remove it. 🤷
aa8fc48
to
e8e061c
Compare
Fixes: #866