-
Notifications
You must be signed in to change notification settings - Fork 542
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
Add v1util.EstargzReadCloser utility. #870
Conversation
Codecov Report
@@ Coverage Diff @@
## master #870 +/- ##
==========================================
+ Coverage 74.28% 74.34% +0.05%
==========================================
Files 106 107 +1
Lines 4476 4486 +10
==========================================
+ Hits 3325 3335 +10
Misses 656 656
Partials 495 495
Continue to review full report at Codecov.
|
bcc3fa1
to
dcf13b4
Compare
Now with 100% coverage 😛 |
Commented on Slack, but for visibility:
|
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.
I am really tempted to fork their package and remove github.com/hashicorp/errwrap
and github.com/hashicorp/go-multierror
, but I understand that's asking a lot 😄
pkg/v1/v1util/zip.go
Outdated
// returns an io.ReadCloser from which compressed data may be read. | ||
// Refer to estargz for the options: | ||
// https://pkg.go.dev/github.com/containerd/[email protected]/estargz#Option | ||
func EstargzReadCloser(r io.ReadCloser, opts ...estargz.Option) (io.ReadCloser, 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.
Can we put this under pkg/internal/estargz
while we're experimenting and expose it later? I'm not sure that I want to commit to this interface quite yet, and v1util is depended on by basically every package in this module, so it can't get pruned if it's in here.
Also estargz.NewRead[Close?]er
reads nicely 😛
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 should be done, and I blew up the rest of v1util in: #872 for consistency.
pkg/v1/v1util/zip.go
Outdated
func EstargzReadCloser(r io.ReadCloser, opts ...estargz.Option) (io.ReadCloser, v1.Hash, error) { | ||
defer r.Close() | ||
|
||
bs, err := ioutil.ReadAll(r) |
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.
Assuming this is just for prototyping and we won't always buffer the whole thing for a final implementation? We should be teeing r
into a hasher, right?
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.
Yeah, I don't love this implementation either, but I think the estargz stuff wants to be able to seek around to build the index and reorder things for the final estargz file. The v1.Hash
is not the overall hash, but the hash of the table of contents. I owe a better function comment than this has, so let me take a crack at that now.
Are you me?? |
Why not patch upstream? |
Very finite amounts of time. |
So you want to maintain a fork of something... right... 🤣 |
This adds the first utility function we will need to start producing estargz layers.
6beb862
to
3983a88
Compare
2648c64
to
ed4e70f
Compare
This adds the first utility function we will need to start producing estargz layers.
Related: #866