Skip to content
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

Simplify TarStreamBuilder#addEntry() to deal with bytes #630

Closed
briandealwis opened this issue Jul 17, 2018 · 2 comments
Closed

Simplify TarStreamBuilder#addEntry() to deal with bytes #630

briandealwis opened this issue Jul 17, 2018 · 2 comments

Comments

@briandealwis
Copy link
Member

From #627 (review)

TarStreamBuilder#addEntry(String content, String fileLocation) seems to be called by ImageToTarballTranslator#toTarballBlob() to add container configuration and manifest files. Oddly the content of these files is passed as a String such that the TarStreamBuilder is responsible for performing an text to byte encoding. Rather than provide two variants of TarStreamBuilder#addEntry(), why don't we just pass the file contents as byte[] and let toTarballBlob() deal with the encoding?

@elharo
Copy link
Contributor

elharo commented Jul 17, 2018

Agreed. Something looks very off here. Output streams should work on bytes, not strings. Passing file content as a string is a huge red flag.

@coollog
Copy link
Contributor

coollog commented Jul 21, 2018

I think the original point of using String as a parameter type is to prevent callers from accidentally using the addEntry(String content, String fileLocation) signature for files (as opposed to addEntry(TarArchiveEntry entry)), where they accidentally read the file into a byte[] and then call this method. For files, the addEntry(TarArchiveEntry entry) signature should be used so that files are never read into memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants