image: accept arbitrary file streams #136
Conversation
s-urbaniak
left a comment
s-urbaniak
left a comment
There was a problem hiding this comment.
LGTM with a minor question/suggestion.
I think it is unfortunate that the walker interface received new methods via reader in the first place effectively also gaining a public method.
This looks better now by making get private only (as it was intended in the first place) for the walker interface.
image/image.go
Outdated
| // ValidateReader walks through the stream and validates the manifest | ||
| // pointed to by the given refs or returns an error if the validation failed. | ||
| // The stream must be a valid .tar file. | ||
| func ValidateReader(r io.ReadSeeker, refs []string, out *log.Logger) error { |
There was a problem hiding this comment.
I'd even call this Validate as it is more general than the filename based one.
There was a problem hiding this comment.
If we can't break the API, then I'd call this ValidateReadSeeker as it is more honest.
image/image.go
Outdated
| // Validate walks through the given .tar file and validates the manifest | ||
| // pointed to by the given refs or returns an error if the validation failed. | ||
| // Validate opens the tar file given by the filename, then calls ValidateReader | ||
| func Validate(tarFile string, refs []string, out *log.Logger) error { |
There was a problem hiding this comment.
I'd call this ValidateFile(name string, refs []string, out *log.Logger....
There was a problem hiding this comment.
I didn't want to break the public API - though I can if you like.
image/image.go
Outdated
| // UnpackReader walks through the tar stream and, using the layers specified in | ||
| // the manifest pointed to by the given ref, unpacks all layers in the given | ||
| // destination directory or returns an error if the unpacking failed. | ||
| func UnpackReader(r io.ReadSeeker, dest, refName string) error { |
There was a problem hiding this comment.
same comments apply as for the Validate* methods.
|
At the moment, the public API is going to have to be reworked anyway. Ultimately I don't really care if we break it because: a. We are not a 1.0 project. |
|
agreed with aleksa - this looks reasonable for now. Needs signed-off-by at the least though. |
squeed
force-pushed
the
tar-streams
branch
2 times, most recently
from
f2f8899 to
7785596
Compare
|
Updated the PR to rename methods according to @s-urbaniak's suggestions; I added a quick optimization while I was at it. |
|
Fixed Not sure why "Signed-off-by" isn't being detected. |
|
also, |
|
Whups, fixed, thanks. |
|
Not sure about the @pullapprove failure on recognizing the DCO. It passed the |
|
@vbatts is "Signed-off-by: Casey Callendrello [email protected]" an email associated with @squeed's github account? |
This allows the image library to handle any ReadSeeker, enabling on-the-fly uncompression. Also, refactor the validate methods to avoid unnecessary copies. Signed-off-by: Casey Callendrello <[email protected]>
|
Ah, probably just the case difference - I fixed the commit. |
|
@squeed looks like that was it (sigh) |
|
OHMAN |
| // or returns an error if the unpacking failed. | ||
| func CreateRuntimeBundle(tarFile, dest, ref, root string) error { | ||
| // CreateRuntimeBundleFile opens the file pointed by tarFile and calls | ||
| // CreateRuntimeBundle. |
There was a problem hiding this comment.
s/CreateRuntimeBundle/createRuntimeBundle ?
And, CreateRuntimeBundle which seems to be unused should be removed?
There was a problem hiding this comment.
The purpose of this PR is to allow for arbitrary tar streams, making this more useful as a library.
While CreateRuntimeBundle isn't used by the cli tool, it's a good library function.
|
👍 |
7 participants
This allows the image library to handle any ReadSeeker, enabling on-the-fly uncompression.
Also, refactor the validate methods to avoid unnecessary copies.
Signed-off-by: Casey Callendrello [email protected]