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

vacation: handle compressed non-tar archives #258

Merged

Conversation

mboldt
Copy link
Contributor

@mboldt mboldt commented Dec 1, 2021

Summary

Add support for compressed files that are not tar archives.
In particular, my buildpack has a dependency that is a gzipped executable.

Use Cases

My use case is the Elm compiler, which is distributed as a gzipped executable.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement. (I believe I have signed the CFF CLA before.)
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs) (No issues to link)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

Support use case where the archive is a gzipped executable.
Signed-off-by: Mikey Boldt <mboldt@vmware.com>
Rename the compression archive types to not imply tar contents.
Write executable file to the bin/ directory with the given name.

Signed-off-by: Mikey Boldt <mboldt@vmware.com>
@mboldt mboldt requested a review from a team as a code owner December 1, 2021 14:51
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 1, 2021

CLA Signed

The committers are authorized under a signed CLA.

@ForestEckhardt
Copy link
Contributor

Hey there @mboldt, this change looks pretty good and I like the overall idea. Before moving forward however, the PR does not currently pass the unit test for this repo. If you could get those going green that would be fantastic. You can run the test locally with go test ./....

Rename the compression archive types to not imply tar contents.
Write executable file to the bin/ directory with the given name.

Signed-off-by: Mikey Boldt <mboldt@vmware.com>
@mboldt
Copy link
Contributor Author

mboldt commented Dec 1, 2021

Unit tests fixed; apologies for missing that before.

@ForestEckhardt
Copy link
Contributor

@mboldt It looks like there are still a couple of stale name in the example_test.go if you could just give those a quick update that would be much appreciated.

@mboldt
Copy link
Contributor Author

mboldt commented Dec 1, 2021

Yes, just noticed and fixed that, and fixed one more thing the linter caught. Thank you.

@ForestEckhardt ForestEckhardt added the semver:major A change requiring a major version bump label Dec 1, 2021
@ForestEckhardt
Copy link
Contributor

@paketo-buildpacks/tooling-maintainers Seeing how this seems like a major version change I would like to get one other person to sign off on this.

@sophiewigmore
Copy link
Member

sophiewigmore commented Dec 1, 2021

@ForestEckhardt / @paketo-buildpacks/tooling-maintainers These changes make sense to me, although I am wondering if there is a way we could introduce these changes in a non-breaking way? I understand this is the best implementation to reduce complexity/code duplication by making the types non-specific to tar. I haven't personally contributed much to vacation so I'll defer to you on that Forest

@mboldt
Copy link
Contributor Author

mboldt commented Dec 1, 2021

Some thoughts on various approaches:

  1. Do not handle non-tar archives.
    • Pros: No code change ;-)
    • Cons: Supports fewer use cases.
  2. Keep the current PR implementation.
    • Pros: Implementation makes sense.
    • Cons: breaking change. (I don't know how wide the impact would be...do consumers use the specific archive types?)
  3. Just leave the type names alone (i.e., keep Tar) even though they don't assume tar archives.
    • Pros: doesn't break.
    • Cons: it is misleading. (IMHO that's enough to discount this.)
  4. Add the new non-tar types, and leave the existing assume-tar types alone. Internally, use the non-tar types exclusively (e.g., when decompressing a generic archive), effectively deprecating the assume-tar types.
    • Pros: Keeps the internal usage clean; doesn't force breaking change.
    • Cons: Keeps the (internally) unused code around. Code duplication.
    • This seems worthy of discussion if the breaking changes are a concern.
    • Note: (Just for completeness) could also try to use both the non-tar and assume-tar types, but that seems like useless complexity...don't see any upside.

Hopefully that helps us think through some of these options. I am happy to rework the implementation based on discussion/feedback.

@sophiewigmore
Copy link
Member

sophiewigmore commented Dec 1, 2021

@mboldt Thank you for laying these out! It's probably worth pointing out that breaking changes are not the end of the world for us. This would extend out to all of our buildpacks that use this package, which is quite a few, but it's still not the end of the world by any means. I brought it up just in case there was a clever way we could get around it, but it's definitely not a blocker to getting this merged in. I'll think on the options you've laid out

Edit: This actually won't be a huge issue at all, we are already going to be cutting a major release with some other changes, and these changes will be used within other packit packages. All good with these changes as is.

@ryanmoran ryanmoran added this to the v2.0.0 milestone Dec 2, 2021
@ForestEckhardt ForestEckhardt merged commit 8f9764f into paketo-buildpacks:main Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:major A change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants