opencontainers / umoci

umoci modifies Open Container images
https://umo.ci
Apache License 2.0
744 stars 98 forks source link

generateinsertlayer: ensure that we close the tarwriter #534

Closed mikemccracken closed 9 months ago

mikemccracken commented 9 months ago

This fixes #436.

I added a test that shows the issue - the commit with the fix has the full explanation, repeated here:

generatelayer closes the tarwriter, but generateinsertlayer forgets to.

Closing the tarwriter writes the required footer of 1k of zeros.

This results in tar files that are complete but invalid, and different reading tools will behave differently:

codecov-commenter commented 9 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (909d140) 73.44% compared to head (7bb2940) 73.34%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/opencontainers/umoci/pull/534/graphs/tree.svg?width=650&height=150&src=pr&token=f3YupAtrEk&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opencontainers)](https://app.codecov.io/gh/opencontainers/umoci/pull/534?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opencontainers) ```diff @@ Coverage Diff @@ ## main #534 +/- ## ========================================== - Coverage 73.44% 73.34% -0.11% ========================================== Files 60 60 Lines 4891 4895 +4 ========================================== - Hits 3592 3590 -2 - Misses 940 944 +4 - Partials 359 361 +2 ``` | [Files](https://app.codecov.io/gh/opencontainers/umoci/pull/534?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opencontainers) | Coverage Δ | | |---|---|---| | [oci/layer/generate.go](https://app.codecov.io/gh/opencontainers/umoci/pull/534?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opencontainers#diff-b2NpL2xheWVyL2dlbmVyYXRlLmdv) | `64.48% <70.00%> (-1.54%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/opencontainers/umoci/pull/534/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opencontainers)
tych0 commented 9 months ago

Everything looks good to me, except you have,

(cherry picked from commit 3f53b10037f03d01412b5b0eb9e50a0053b7a156)

in your commit message, which is a mystery hash?

tych0 commented 9 months ago

I just cleaned it up, will merge once that all passes CI

mikemccracken commented 9 months ago

Everything looks good to me, except you have,

(cherry picked from commit 3f53b10037f03d01412b5b0eb9e50a0053b7a156)

in your commit message, which is a mystery hash?

d'oh- thanks for taking care of this, that hash was a commit on top of the stacker fork of umoci, I should've cleaned up the message.

Thanks for the quick review guys!