-
Notifications
You must be signed in to change notification settings - Fork 99
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
fix(compress): allow passing in compressor options #509
base: main
Are you sure you want to change the base?
Conversation
An alternative approach to: #508 |
|
Goals of this PR: 1. Allow passing in options to individual compressor 2. Do not change default behavior Signed-off-by: Ramkumar Chinchani <[email protected]>
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 73.48% 73.45% -0.04%
==========================================
Files 60 60
Lines 4884 4893 +9
==========================================
+ Hits 3589 3594 +5
- Misses 935 939 +4
Partials 360 360
|
Looks nice |
@cyphar thoughts on this PR? Hoping to avoid a umoci fork. |
A fork definitely won't be necessary, I'll review this on Monday. Sorry, I was a bit busy last week. |
@@ -76,6 +91,15 @@ func (gz gzipCompressor) MediaTypeSuffix() string { | |||
return "gzip" | |||
} | |||
|
|||
func (gz gzipCompressor) WithOpt(opt CompressorOpt) Compressor { | |||
switch val := opt.(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like that we ignore invalid arguments here. If we make this act on *gzipCompressor
we could make it return a bool (or error) and then the caller can decide if they care about whether WithOpt
actually did anything.
Alternatively, it feels like it should be possible to reimplement all of this compressor stuff with a closure that is passed by the caller (that creates the writer and configures it). But that would require more work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add a CustomCompressor
just for cases where you need to configure the compression? Idk...
Another place of relevance in umoci code wrt compressor options :( https://github.com/opencontainers/umoci/blob/main/pkg/system/copy.go#L32 |
fyi, filed this against image-spec opencontainers/image-spec#1145 |
Goals of this PR: