No empty layers #293
No empty layers #293
Conversation
test/repack.bats
Outdated
| @@ -231,7 +231,7 @@ function teardown() { | |||
| bundle-verify "$BUNDLE" | |||
|
|
|||
| # Make some small change. | |||
| touch "$BUNDLE/a_small_change" | |||
| touch "$BUNDLE/rootfs/a_small_change" | |||
There was a problem hiding this comment.
Use $ROOTFS -- I thought I fixed all of these incorrect $BUNDLE usages...
test/repack.bats
Outdated
|
|
||
| layers0=$(cat "${IMAGE}/oci/blobs/sha256/$manifest0" | jq -r .layers) | ||
| layers1=$(cat "${IMAGE}/oci/blobs/sha256/$manifest1" | jq -r .layers) | ||
| [ "$layers0" == "$layers1" ] |
There was a problem hiding this comment.
Inconsistent tabs and spaces -- use tabs everywhere.
test/repack.bats
Outdated
|
|
||
| @test "umoci repack (empty diff)" { | ||
| # Unpack the original image | ||
| new_bundle_rootfs && BUNDLE_A="$BUNDLE" ROOTFS_A="$ROOTFS" |
There was a problem hiding this comment.
If you're not going to use BUNDLE_A and ROOTFS_A you can drop the && ... section of this line.
|
Huh. No idea why this one failed. Maybe a race? There's lots of goroutines in that code path... |
|
Yeah, I'm not sure. Every once in a while we see a flaky test (and this one was odd -- a return code was 0 when it shouldn't have been). At some point I should run some stress-like tests to see what falls out. Anyway, I re-ran it and it passes. I'll review this one tomorrow. The main thing that strikes me is that |
|
Ping, any word on this? |
1 similar comment
|
Ping, any word on this? |
|
Yup sorry, this looks okay. I might prefer having a separate LGTM. |
$BUNDLE is really the path to the top level thing, and we only detect changes to $BUNDLE/rootfs. So when we make changes, let's actually make them to the rootfs. The reason this worked before is that we always insert a layer, even if it's empty. With the next patch, we wouldn't do this any more, causing this test to fail. Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Let's not insert an empty gzip layer if there is no diff. It might make sense to not even insert a history entry, but it also seems reasonable to want to log that umoci ran at some point, so we do that, but just leave everything unchanged. Signed-off-by: Tycho Andersen <tycho@tycho.ws>
|
I think I've figured out the root cause of the failure. I think it's because the current trick of returning errors through |
|
LGTM. |
Here's some code to avoid inserting empty layers, and instead just creating a history entry to log that something happened.