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

Add archive package to imgutil, to make it a utility library #73

Closed
wants to merge 2 commits into from

Conversation

dfreilich
Copy link
Member

Signed-off-by: David Freilich [email protected]

Summary

There was a request (buildpacks/pack#814) to extract the archive package from https://github.com/buildpacks/pack:

The archive package contains a zip to tar conversion utility that is useful outside of the pack CLI itself. We have a use case where we would like to convert user-submitted zip files containing app source code into .tar files so that we can construct single-layer OCI images.
The pack CLI is battle-tested and includes logic for handling a wide-variety of zip files (e.g. those created on Windows machines, FAT filesystems, files with strange permissions, etc.) so we would like to build upon this rather than reinvent the wheel. 🎡 🌝

A discussion was apparently had to move the archive package to imgutil (as a general util for the project), rather than having it be an external library in pack.

Specifics

This PR is built upon buildpacks/pack#943, which added significant test coverage for the package, and in the process resolved a small bug. It also adds in a number of test helper files/funcs from pack (which we may be able to remove from pack as a result).

* Add necessary testhelpers from pack

Signed-off-by: David Freilich <[email protected]>
@dfreilich dfreilich requested a review from a team as a code owner November 11, 2020 10:17
@dfreilich
Copy link
Member Author

cc @tcdowney @piyalibanerjee

Signed-off-by: David Freilich <[email protected]>
@jromero
Copy link
Member

jromero commented Nov 19, 2020

@ekcasey any thoughts on merging this in?

@ekcasey
Copy link
Member

ekcasey commented Nov 19, 2020

@jromero @dfreilich Sorry I meant to respond to this sooner.

Let me take you on a journey through my thought process...

  • Thought 1: If we merged this into imgutil we would naturally want to update the lifecycle to use the common archive helpers.
  • Thought 2: Alack! The lifecycle has its own archive package and the pack interface doesn't provide all of the functionality the lifecycle needs and vice versa.
  • Thought 3: But I think it would be a good thing to standardize on the same archive package across the project. Although we should acknowledge that it will raise the barrier to changing this interface.
  • Thought 4: 🚨 Bias alert! 🚨 I like the lifecycle archive package interface better than the pack one. If we are going to maintain a public library I want the usage to look more like the lifecycle version. And I think it would be a bad thing to encourage folks to import this and then change it abruptly/dramatically.
  • Thought 5: However the lifecycle package doesn't handle the ZIP -> TAR conversation that motivated this request in the first place.
  • Thought 6: Short term compromise? Would it be possible to start by creating a an archive package in imgutil that simply exposes the ReadXAsTar functions?
  • Thought 7: Long term strategy? We can have a spirited debate about the ideal shared interface and migrate additional functionality incrementally.

What do you think?

@dfreilich
Copy link
Member Author

Interesting, I didn't realize the lifecycle also had a similar package, though that makes sense. I do agree that it would be nice to do your start (Though 6), at least, though I'd also want the necessary code behind it to also live in imgutil (minimizing transitive imports for people who use it, and probably forestalling any import problems for pack)

@natalieparellano
Copy link
Member

Since this PR has been open awhile, I fear that it may now be quite out of date.

It sounds like we aligned on Thought #6

Thought 6: Short term compromise? Would it be possible to start by creating a an archive package in imgutil that simply exposes the ReadXAsTar functions?

Would it make sense to close this PR, but open a new issue that references it for doing that unit of work?

@natalieparellano
Copy link
Member

Created issue #133 to capture next steps from this PR.

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

Successfully merging this pull request may close these issues.

4 participants