test/container: Extend staged update version check#5499
Conversation
There was a problem hiding this comment.
Code Review
This pull request extends the container update check test to verify that staged updates and subsequent updates are handled correctly. The added test cases are valuable for ensuring the robustness of the update mechanism. My review focuses on improving the maintainability of the test script by addressing code duplication. I've suggested refactoring two repeated blocks of code into reusable shell functions.
| td=$(mktemp -d) | ||
| cd ${td} | ||
| cat > Containerfile << EOF | ||
| FROM localhost/fcos | ||
| RUN touch /usr/foo | ||
| LABEL org.opencontainers.image.version 3 | ||
| EOF | ||
|
|
||
| touched_resolv_conf=0 | ||
| if test '!' -f /etc/resolv.conf; then | ||
| podmanv=$(podman --version) | ||
| case "${podmanv#podman version }" in | ||
| 3.*) touched_resolv_conf=1; touch /etc/resolv.conf;; | ||
| esac | ||
| fi | ||
| podman build --net=host -t localhost/fcos-derived --squash . | ||
| if test "${touched_resolv_conf}" -eq 1; then | ||
| rm -vf /etc/resolv.conf | ||
| fi |
There was a problem hiding this comment.
This block of code for building a derived container image is duplicated multiple times in this file (see lines 52-69 and 89-107). To improve maintainability and reduce redundancy, consider extracting this logic into a reusable shell function. The function could take the Containerfile contents as an argument.
For example:
build_derived_image() {
local containerfile_content=$1
# It's safer to run this in a subshell to avoid `cd` affecting
# the rest of the script, and to handle cleanup of the temp dir.
(
td=$(mktemp -d)
trap 'rm -rf -- "$td"' EXIT
cd "$td"
cat > Containerfile << EOF
${containerfile_content}
EOF
local touched_resolv_conf=0
if test '!' -f /etc/resolv.conf; then
podmanv=$(podman --version)
case "${podmanv#podman version }" in
3.*) touched_resolv_conf=1; touch /etc/resolv.conf;;
esac
fi
podman build --net=host -t localhost/fcos-derived --squash .
if test "${touched_resolv_conf}" -eq 1; then
rm -vf /etc/resolv.conf
fi
)
}Then you could call it like this:
build_derived_image "FROM localhost/fcos
RUN touch /usr/foo
LABEL org.opencontainers.image.version 3"| assert_file_has_content_literal out.txt '"n-added":' | ||
| assert_file_has_content_literal out.txt '"n-removed":' | ||
| assert_file_has_content_literal out.txt '"removed-size":' | ||
| assert_file_has_content_literal out.txt '"total-size":' | ||
| assert_file_has_content_literal out.txt '"total":' | ||
| assert_file_has_content_literal out.txt '"added-size":' | ||
| assert_file_has_content_literal out.txt '"version": "3"' | ||
| assert_file_has_content_literal out.txt '"origin": "ostree-unverified-image:containers-storage:localhost/fcos-derived"' |
There was a problem hiding this comment.
This set of assertions is very similar to the one in lines 131-138. The only difference is the version number being checked. To avoid code duplication and improve readability, you could extract these assertions into a helper function that takes the version number as a parameter.
For example:
assert_cached_update_status() {
local version=$1
assert_file_has_content_literal out.txt '"n-added":'
assert_file_has_content_literal out.txt '"n-removed":'
assert_file_has_content_literal out.txt '"removed-size":'
assert_file_has_content_literal out.txt '"total-size":'
assert_file_has_content_literal out.txt '"total":'
assert_file_has_content_literal out.txt '"added-size":'
assert_file_has_content_literal out.txt "'"version"': '"${version}"'"
assert_file_has_content_literal out.txt '"origin": "ostree-unverified-image:containers-storage:localhost/fcos-derived"'
}Then you could call it with assert_cached_update_status 3.
|
/retest |
Looks like we need to make space on the GitHub runners. |
020d99e to
c8a64a0
Compare
This test makes sure that when we cache a first version update and then another update comes in, the second update is properly cached. Split from: coreos#4951
This test failed on low disk space. Clean up things we don't use from the runner.
c8a64a0 to
1a8c009
Compare
|
@travier: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This test makes sure that when we cache a first version update and then another update comes in, the second update is properly cached.
Split from: #4951