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

mutate: allow for custom compression #348

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

tych0
Copy link
Member

@tych0 tych0 commented Sep 28, 2020

at least via the API for now :)

Signed-off-by: Tycho Andersen [email protected]

@tych0 tych0 requested a review from cyphar September 28, 2020 16:01
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

Seems reasonable, just a few API questions and cleanups that I reckon would be a good idea.

mutate/compress.go Show resolved Hide resolved
Comment on lines 17 to 27
type withClose struct {
r io.Reader
}

func (wc withClose) Read(p []byte) (n int, err error) {
return wc.r.Read(p)
}

func (wc withClose) Close() error {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

You can switch to ioutil.NopCloser and remove this wrapper.

mutate/compress.go Outdated Show resolved Hide resolved
mutate/mutate.go Outdated Show resolved Hide resolved
@cyphar
Copy link
Member

cyphar commented Sep 29, 2020

And yeah, I can cook up the CLI UX for it. Oh and I like the new email btw. :P

Instead of hard coding this media type, allow people to specify it in
Add().

This lets us drop AddNonDistributable(), which was just copypasta of Add()
with the media type changed.

Signed-off-by: Tycho Andersen <[email protected]>
@tych0 tych0 force-pushed the custom-compression branch from a705649 to 5d0bf68 Compare September 29, 2020 16:12
@tych0
Copy link
Member Author

tych0 commented Sep 29, 2020

408 while uploading code coverage seems like a transient failure... I re-ran travis.

at least via the API for now :)

Signed-off-by: Tycho Andersen <[email protected]>
@tych0 tych0 force-pushed the custom-compression branch from 5d0bf68 to d82a276 Compare September 29, 2020 20:22
@codecov-commenter
Copy link

Codecov Report

Merging #348 into master will increase coverage by 0.04%.
The diff coverage is 74.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   74.45%   74.50%   +0.04%     
==========================================
  Files          57       58       +1     
  Lines        3054     3051       -3     
==========================================
- Hits         2274     2273       -1     
+ Misses        455      454       -1     
+ Partials      325      324       -1     
Impacted Files Coverage Δ
cmd/umoci/raw-add-layer.go 55.55% <0.00%> (ø)
mutate/compress.go 73.33% <73.33%> (ø)
mutate/mutate.go 70.45% <80.00%> (+0.45%) ⬆️
cmd/umoci/insert.go 81.33% <100.00%> (ø)


// AddNonDistributable is the same as Add, except it adds a non-distributable
// layer to the image.
func (m *Mutator) AddNonDistributable(ctx context.Context, r io.Reader, history *ispec.History) (ispec.Descriptor, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Huh, I didn't consider that we could drop AddNonDistributable entirely. Nice. 😸

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM. The number of arguments to mutator.Add is getting a bit long but we can always deal with that some other time.

@tych0 tych0 merged commit 05c3036 into opencontainers:master Sep 30, 2020
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.

3 participants