-
Notifications
You must be signed in to change notification settings - Fork 97
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
fix: update error returned when oci.NewFromTar
is used with a non-tar file
#793
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Adnan Gulegulzar <[email protected]>
a7514ae
to
995e97f
Compare
internal/fs/tarfs/tarfs.go
Outdated
|
||
extension := filepath.Ext(pathAbs) | ||
|
||
if extension != ".tar" { |
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.
Are compressed formats supported here like tgz
?
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.
No, I believe they will not be.
Is there a list of extensions we need to support?
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 can add them to the check and add corresponding tests for them too.
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 imagine only tar
, but I'm not sure.
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.
The current tests work with the files hello-world.tar
and test.tar
. From my understanding, .tar
s are the only ones supported as of now.
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.
What if the target file/directory name ends with .tar
, but it is actually not a tar
type? Or what if the file is a tar file but it does not have a .tar
suffix in its name?
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.
In tfs.indexEntries()
, maybe we can return a friendly error if tr.Next()
fails?
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 think tfs.indexEntries
would indeed be a better place for this check. We can check if EOF
is reached before any entries are added to tfs
. This would infer it is an empty file. While a non-tar file will give us an io.ErrUnexpectedEOF
while reading it.
We can then give the user our custom exported error.
internal/fs/tarfs/tarfs.go
Outdated
|
||
extension := filepath.Ext(pathAbs) | ||
|
||
if extension != ".tar" { |
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.
What if the target file/directory name ends with .tar
, but it is actually not a tar
type? Or what if the file is a tar file but it does not have a .tar
suffix in its name?
internal/fs/tarfs/tarfs.go
Outdated
|
||
extension := filepath.Ext(pathAbs) | ||
|
||
if extension != ".tar" { |
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.
In tfs.indexEntries()
, maybe we can return a friendly error if tr.Next()
fails?
Hello @ADorigi , thank you for the PR! I just reviewed it and left some comments. |
Signed-off-by: Adnan Gulegulzar <[email protected]>
Hi @Wwwsylvia |
oci.NewFromTar
is used with a non-tar fileoci.NewFromTar
is used with a non-tar file
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
==========================================
+ Coverage 77.92% 78.03% +0.10%
==========================================
Files 63 63
Lines 4694 4698 +4
==========================================
+ Hits 3658 3666 +8
+ Misses 656 654 -2
+ Partials 380 378 -2 ☔ View full report in Codecov by Sentry. |
Hi @ADorigi , the updated PR looks good overall. I left a few nit comments. |
fix: updated error text Signed-off-by: Adnan Gulegulzar <[email protected]>
Hi @Wwwsylvia |
Signed-off-by: Adnan Gulegulzar <[email protected]>
Signed-off-by: Adnan Gulegulzar <[email protected]>
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.
LGTM with some thoughts. @shizhMSFT Please also take a look at this PR when you get a chance.
if len(tfs.entries) == 0 { | ||
// indicates the given file is empty | ||
return fmt.Errorf("%s: file is empty: %w", tfs.path, errdef.ErrInvalidTarFile) |
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.
Just thinking out loud: From the tarfs
point of view, the tar file can be empty. But since tarfs
is an internal package and is only used by ReadOnlyStorage
and ReadOnlyOCI
, which requires oci-layout
file in it, I think it makes sense to return an error for an empty tar file. 🤔
oci.NewFromTar
is used with a non-tar fileoci.NewFromTar
is used with a non-tar file
cc @qweeah who opened the original issue |
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.
Set to Request changes
to block merge as the original issue might be invalid.
if len(tfs.entries) == 0 { | ||
// indicates the given file is empty | ||
return fmt.Errorf("%s: file is empty: %w", tfs.path, errdef.ErrInvalidTarFile) | ||
} |
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 don't think we should return error on empty valid tar files since it breaks the encapsulation principal.
If we really want to check for empty tar file, it should be checked in the caller.
if errors.Is(err, io.ErrUnexpectedEOF) { | ||
// indicates that its either not a tarfile or it is a corrupted one | ||
return fmt.Errorf("%s: %w", tfs.path, errdef.ErrInvalidTarFile) | ||
} |
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.
The io.ErrUnexpectedEOF
error itself indicates the tar file is corrupted. There is no need to wrap it to errdef.ErrInvalidTarFile
.
This PR is stale because it has been open for 45 days with no activity. Remove the stale label or comment to prevent it from being closed in 30 days. |
fixes #640
This pull request includes the following changes:
ErrNotTarFile
.tar
file