Conversation
|
Build succeeded.
|
|
@AkihiroSuda can you help me dig up some images using this compression? You proposed having it adding to the image-spec, but looks like it was rushed through without even showing examples of where it is being used. |
Codecov Report
@@ Coverage Diff @@
## master #3649 +/- ##
==========================================
+ Coverage 42.39% 42.39% +<.01%
==========================================
Files 127 127
Lines 14048 14065 +17
==========================================
+ Hits 5955 5963 +8
- Misses 7193 7200 +7
- Partials 900 902 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3649 +/- ##
======================================
Coverage 42% 42%
======================================
Files 129 129
Lines 14275 14275
======================================
Hits 5996 5996
Misses 7379 7379
Partials 900 900
Continue to review full report at Codecov.
|
|
Support for zstd layers are implemented by Red Hat Their work seems mostly motivated for CRFS integration containers/fuse-overlayfs#79 |
|
I agree the merge was too much rushed, and the media type string may change before the next formal release of OCI Image Spec opencontainers/image-spec#791 |
741a99e to
2aa8f4a
Compare
|
Build succeeded.
|
diff/walking/differ.go
Outdated
There was a problem hiding this comment.
Wouldn't it be better to return compression.XYZ directly from DiffCompression to avoid extra switch? compression package expects compression.Compression type anyway.
There was a problem hiding this comment.
If we were to return that, it would be better to move it out of the images package to limit its dependency graph. Right now they are intended to be different though as there may be more compression types from the media types than supported in the compression package. Likewise handling the "unknown" logic is kind of weird and would prefer to handle it outside the compression package. What might make more sense though is to have a package just for handling the media types.
If you think I should split these up into separate PRs, I can do that. I think the generic part is easier to accept. Even the test repo that I saw used by RH in their PR comments didn't seem to still be using |
Yes, it seems better for 1.3, and I think we can revisit support for zstd in 1.4. If somebody wants to decompress zstd layers with 1.3, they should be able to use stream processors? |
2aa8f4a to
30354f5
Compare
|
Build succeeded.
|
|
Removed
I like getting support for stuff like this in as early as possible to reduce the support matrix in the future. Moving it out of this release is fine though, I just want to make sure it at least works through the stream processors. |
30354f5 to
c1b12ae
Compare
|
Build succeeded.
|
|
LGTM |
|
need to rebase. |
Avoid directly handling media types with "+" attributes, instead handling the base and passing through the full media type to the appropriate stream processor or decompression. Signed-off-by: Derek McGowan <derek@mcgstyle.net>
c1b12ae to
6f31417
Compare
|
Build succeeded.
|
Updates layer support to generically handle the OCI layer types, removing the need to continue to add to a long list of possible values. For example, encrypted containers are using "+enc", however this is not yet standardized but support is desirable. Additionally, OCI has added "+zstd" to define zstd compression. This change ensures "+zstd" defines the correct compression algorithm and adds it to the compression library.