Builder.untarPath(): always evaluate b.ContentDigester.Hash() #1955
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM, TYVM @nalind, much more elegant than my solution, per usual. |
Make sure we evaluate b.ContentDigester.Hash() every time the callback returned by Builder.untarPath() is called, since it may be for a different digest object than the previous time it was called, or different even from when the callback was first retrieved. Signed-off-by: Nalin Dahyabhai <[email protected]>
Add Tom's test for cache checking from containers#1929. Signed-off-by: TomSweeneyRedHat <[email protected]> Signed-off-by: Nalin Dahyabhai <[email protected]>
|
📌 Commit 5b77381 has been approved by |
edsantiago
reviewed
Oct 30, 2019
Comment on lines
+1815
to
+1817
|
|
||
| date>bud/${target}/test; tar cJf - bud/${target}/test > bud/${target}/test.tar.xz | ||
| run_buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t ${target} ${TESTSDIR}/bud/${target} |
There was a problem hiding this comment.
This looks copied from #1929 and I have my same comment: why are these two lines necessary? ISTM that they can be safely removed, no? (In which case one of 1814 or 1821 could also be removed)
| expect_output --substring "COMMIT copy-archive" | ||
|
|
||
| # Now test that we do NOT use cache if the tar file changes | ||
| date>bud/${target}/test; tar cJf - bud/${target}/test > bud/${target}/test.tar.xz |
There was a problem hiding this comment.
Another holdover comment from #1929: I kind of cringe at using date > in this situation: it is not impossible for lines 1803-1812 to run in less than one second, in which case the test file mtime might change but its content would not. I would like to see a clearer, more explicit change: maybe something like
echo This is new content >> bud/${target}/test; tar ....
edsantiago
reviewed
Oct 30, 2019
| @test "bud containerfile with tar archive in copy" { | ||
| # First check to verify cache is uses if the tar file does not change | ||
| target=copy-archive | ||
| date>bud/${target}/test; tar cJf - bud/${target}/test > bud/${target}/test.tar.xz |
There was a problem hiding this comment.
Oh oops: this needs to be ${TESTSDIR}/bud/...
There was a problem hiding this comment.
...and the tar probably needs -C, and other instances of implicit cwd
|
Fixed a few other problems (missing # Fixes #1906: buildah was not detecting changed tarfile
@test "bud containerfile with tar archive in copy" {
# First check to verify cache is used if the tar file does not change
target=copy-archive
date > ${TESTSDIR}/bud/${target}/test
tar -C $TESTSDIR -cJf ${TESTSDIR}/bud/${target}/test.tar.xz bud/${target}/test
run_buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t ${target} ${TESTSDIR}/bud/${target}
expect_output --substring "COMMIT copy-archive"
run_buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t ${target} ${TESTSDIR}/bud/${target}
expect_output --substring " Using cache"
expect_output --substring "COMMIT copy-archive"
# Now test that we do NOT use cache if the tar file changes
echo This is a change >> ${TESTSDIR}/bud/${target}/test
tar -C $TESTSDIR -cJf ${TESTSDIR}/bud/${target}/test.tar.xz bud/${target}/test
run_buildah bud --signature-policy ${TESTSDIR}/policy.json --layers -t ${target} ${TESTSDIR}/bud/${target}
if [[ "$output" =~ " Using cache" ]]; then
expect_output "[no instance of 'Using cache']" "no cache used"
fi
expect_output --substring "COMMIT copy-archive"
rm -f ${TESTSDIR}/bud/${target}/test*
} |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 31, 2019
Add Tom's test for cache checking from #1929. Signed-off-by: TomSweeneyRedHat <[email protected]> Signed-off-by: Nalin Dahyabhai <[email protected]> Closes: #1955 Approved by: rhatdan
|
☀️ Test successful - status-papr, status-travis |
|
@edsantiago thanks! I'll spin up a PR to replace that test with your variant. |
TomSweeneyRedHat
added a commit
to TomSweeneyRedHat/buildah
that referenced
this pull request
Oct 31, 2019
After containers#1955 merged, @edsantiago had suggestions for tweaks to the test that was part of that merge. This PR addresses those comments. Signed-off-by: TomSweeneyRedHat <[email protected]>
This was referenced Oct 31, 2019
rh-atomic-bot
pushed a commit
that referenced
this pull request
Oct 31, 2019
After #1955 merged, @edsantiago had suggestions for tweaks to the test that was part of that merge. This PR addresses those comments. Signed-off-by: TomSweeneyRedHat <[email protected]> Closes: #1958 Approved by: vrothberg
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
Make sure we evaluate b.ContentDigester.Hash() every time the callback returned by Builder.untarPath() is called, since it may be for a different digest object than the previous time it was called, or different even from when the callback was first retrieved. Signed-off-by: Nalin Dahyabhai <[email protected]> Closes: containers#1955 Approved by: rhatdan
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
Add Tom's test for cache checking from containers#1929. Signed-off-by: TomSweeneyRedHat <[email protected]> Signed-off-by: Nalin Dahyabhai <[email protected]> Closes: containers#1955 Approved by: rhatdan
caiges
pushed a commit
to caiges/buildah
that referenced
this pull request
Nov 12, 2019
After containers#1955 merged, @edsantiago had suggestions for tweaks to the test that was part of that merge. This PR addresses those comments. Signed-off-by: TomSweeneyRedHat <[email protected]>
edsantiago
added a commit
to edsantiago/libpod
that referenced
this pull request
Dec 5, 2019
See containers/buildah#1955 I've confirmed that this test fails under podman-1.6.2-2.fc30 and passes under current master. Signed-off-by: Ed Santiago <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure we evaluate
b.ContentDigester.Hash()every time the callback returned byBuilder.untarPath()is called, since it may be for a different digest object than the previous time it was called, or different even from when the callback was first retrieved.Adds the test from #1929, and aims to fix #1906.