-
Notifications
You must be signed in to change notification settings - Fork 20
feat(just): add overlay command for sysext testing #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tulip Blossom <tulilirockz@outlook.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new overlay command in the Justfile to facilitate system extension testing by building an EROFS image. This is a great addition for testing purposes. My review focuses on improving the robustness and correctness of the new bash script. I've pointed out two high-severity issues: the script should be configured to exit on any error to prevent generating faulty images, and a directory move operation is flawed, potentially leading to an incorrect filesystem structure in the generated image. Addressing these points will make the new command more reliable.
|
|
||
| overlay $BLUEFIN_MERGE="1": | ||
| #!/usr/bin/env bash | ||
| ROOTFS_DIR="$(mktemp -d --tmpdir="${ROOTFS_BASE:-/tmp}")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script should be configured to exit immediately if any command fails. Without this, an error in one of the steps (e.g., crane export failing) would be ignored, and the script would continue executing, potentially resulting in a corrupt or incomplete image. Adding set -euo pipefail is a best practice for writing robust shell scripts.
set -e: Exits the script if any command fails.set -u: Treats unset variables as an error.set -o pipefail: Ensures that a pipeline's exit code is the exit code of the last command to exit with a non-zero status, which is crucial for thecrane | tarcommand on line 42.
set -euo pipefail
ROOTFS_DIR="$(mktemp -d --tmpdir="${ROOTFS_BASE:-/tmp}")"
| if [ -d "${ROOTFS_DIR}/etc" ] ; then | ||
| mv --no-clobber "${ROOTFS_DIR}/etc" "${ROOTFS_DIR}/usr/etc" | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mv command used here is incorrect if ${ROOTFS_DIR}/usr/etc already exists. It would move the etc directory inside usr/etc, creating ${ROOTFS_DIR}/usr/etc/etc, which is not the intended behavior.
To correctly merge the contents of ${ROOTFS_DIR}/etc into ${ROOTFS_DIR}/usr/etc, it's better to use cp to copy the contents and then rm to delete the source directory. This ensures the directory structure is correct.
if [ -d "${ROOTFS_DIR}/etc" ] ; then
install -d "${ROOTFS_DIR}/usr/etc"
cp -a "${ROOTFS_DIR}/etc/." "${ROOTFS_DIR}/usr/etc/"
rm -rf "${ROOTFS_DIR}/etc"
fi
c160c98 to
ee9d52d
Compare
Signed-off-by: Tulip Blossom <tulilirockz@outlook.com>
ee9d52d to
4fc1a23
Compare
This should make it slightly easier to test stuff. It just builds an image that you can mount on
/var/lib/extensionsif you want to