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 compression option to tarball.Layer and stream.Layer #574

Merged

Conversation

jromero
Copy link
Contributor

@jromero jromero commented Oct 16, 2019

Resolves #572

Signed-off-by: Javier Romero jromero@pivotal.io

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Collaborator

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but I'll wait for @jonjohnsonjr's review to merge

@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #574 into master will increase coverage by 0.05%.
The diff coverage is 93.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #574      +/-   ##
========================================
+ Coverage   72.95%    73%   +0.05%     
========================================
  Files          99     99              
  Lines        4411   4420       +9     
========================================
+ Hits         3218   3227       +9     
  Misses        786    786              
  Partials      407    407
Impacted Files Coverage Δ
pkg/v1/stream/layer.go 81.92% <100%> (+2.19%) ⬆️
pkg/v1/tarball/layer.go 77.77% <88.88%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad4d343...f2b0120. Read the comment docs.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, a couple small nits.

This only fixes tarball.Layer and not stream.Layer -- I'd update the PR description to avoid closing this until we address both (or just fix stream in this PR as well?).

@jromero jromero force-pushed the feature/layer-compression-option branch 2 times, most recently from dbee5a0 to 626937d Compare October 16, 2019 21:56
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jromero jromero changed the title Add compression option to tarball.Layer Add compression option to tarball.Layer and stream.Layer Oct 16, 2019
@jromero jromero force-pushed the feature/layer-compression-option branch from 626937d to 30d2ead Compare October 16, 2019 22:01
@jromero
Copy link
Contributor Author

jromero commented Oct 16, 2019

Alright, think I've got everything squared away. I've implemented the change to stream.Layer as well to truly resolve the issue mentioned.

I did notice (and followed) the slightly different testing convention so the tests will look different between them.

Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, think I've got everything squared away. I've implemented the change to stream.Layer as well to truly resolve the issue mentioned.

I have one small nit on the comments (sorry!), otherwise lgtm. I think we'll eventually want to deprecate this and move to a more general way to augment layer content and media type (see my comment here), but I don't want to block this on trying to design around that :)

I did notice (and followed) the slightly different testing convention so the tests will look different between them.

Yeah it's a bit of a mess.

pkg/v1/tarball/layer.go Outdated Show resolved Hide resolved
pkg/v1/stream/layer.go Outdated Show resolved Hide resolved
Signed-off-by: Javier Romero <jromero@pivotal.io>
@jromero
Copy link
Contributor Author

jromero commented Oct 17, 2019

@jonjohnsonjr I can totally see the need and use case of different compression algorithms but that goes a bit beyond our scope at the moment. I'd be happy to take a swing at it via a different PR.

@jonjohnsonjr jonjohnsonjr merged commit 561fd28 into google:master Oct 17, 2019
@jonjohnsonjr
Copy link
Collaborator

Thanks!

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.

Layer digests don't match pushed digests using docker
5 participants