fix(cli): handle empty files #430
Conversation
Signed-off-by: Jose I. Paris <jiparis@gmail.com>
migmartri
changed the title
| } | ||
|
|
||
| if info.Size() == 0 { | ||
| a.Logger.Info().Msg("the file is empty, nothing to upload") |
There was a problem hiding this comment.
I have a context question, most likely for @migmartri, but anybody who knows the answer is welcome to reply ofc.
What is the context for these files?
Would someone find legit, in the generic scenario, to produce and upload an empty file?
This implementation is returning here, so client.UploadFile(...) is not being called at all.
There was a problem hiding this comment.
Interesting point.
I think you are right, it doesn't harm to upload empty files. In fact, artifact material types in attestations can be empty, but this code doesn't affect that (this is only for chainloop artifact upload cmd.)
The change in uploader.go is in fact what fixes the original panic #380, so I think it's ok if we remove
if info.Size() == 0 {
a.Logger.Info().Msg("the file is empty, nothing to upload")
...and just keep the change at internal/casclient/uploader.go.
What do you think?
Note: This implementation is based on my prescription offline, so any blame is on me :)
There was a problem hiding this comment.
Makes sense. I'll update the PR with your sugested changes. Thanks!
There was a problem hiding this comment.
Ok perfect. I'll request changes to remember not to merge this until it's fixed.
Thanks.
| } | ||
|
|
||
| if info.Size() == 0 { | ||
| a.Logger.Info().Msg("the file is empty, nothing to upload") |
There was a problem hiding this comment.
Interesting point.
I think you are right, it doesn't harm to upload empty files. In fact, artifact material types in attestations can be empty, but this code doesn't affect that (this is only for chainloop artifact upload cmd.)
The change in uploader.go is in fact what fixes the original panic #380, so I think it's ok if we remove
if info.Size() == 0 {
a.Logger.Info().Msg("the file is empty, nothing to upload")
...and just keep the change at internal/casclient/uploader.go.
What do you think?
Note: This implementation is based on my prescription offline, so any blame is on me :)
Signed-off-by: Jose I. Paris <jiparis@gmail.com>
@buccarel ptal, it seems that the fix is now up. |
3 participants
Gracefully handle empty files uploads.
Closes #380