-
Notifications
You must be signed in to change notification settings - Fork 169
tests/sysexts: Rework to build sysext locally #3966
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: testing-devel
Are you sure you want to change the base?
Conversation
Build the sysext from Fedora RPMs and from scratch as part of the test to make this work on all releases and independently of the unofficial sysext project. Only test on x86_64 & aarch64 for now.
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 reworks the systemd sysext test to build the extension from RPMs locally, rather than downloading pre-built ones. This is a great improvement for test reliability and independence. My review includes several suggestions to enhance the robustness, security, and efficiency of the new build script. Key areas for improvement include proper temporary directory management, safer package repository handling, correct version sorting, and more robust file operations.
| dnf download --resolve --arch=noarch --arch="$(arch)" "${rpm}" | ||
|
|
||
| # Figure out version to use | ||
| pkg="$(ls ${rpm}-*.rpm | sort -h | head -1)" |
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 sort -h command is for sorting human-readable numbers (e.g., 2K, 1G), not version strings. For correct version sorting, you should use sort -V. To get the latest version, you should select the last item after sorting, not the first. Using ls -1 is also recommended in scripts to ensure one file per line.
| pkg="$(ls ${rpm}-*.rpm | sort -h | head -1)" | |
| pkg="$(ls -1 ${rpm}-*.rpm | sort -V | tail -n 1)" |
| tmpdir="/tmp/sysext-${rpm}" | ||
| mkdir "${tmpdir}" |
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.
| rm -rf /etc/yum.repos.d/fedora-cisco-openh264.repo | ||
| dnf download --resolve --arch=noarch --arch="$(arch)" "${rpm}" |
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.
Removing the repository file is a destructive action that modifies system state. A safer approach is to temporarily disable the repository for the dnf command using the --disablerepo flag.
| rm -rf /etc/yum.repos.d/fedora-cisco-openh264.repo | |
| dnf download --resolve --arch=noarch --arch="$(arch)" "${rpm}" | |
| dnf download --resolve --arch=noarch --arch="$(arch)" --disablerepo=fedora-cisco-openh264 "${rpm}" |
| mkfs.erofs -zlz4 "${name}" rootfs | ||
| mv "${name}" /tmp | ||
|
|
||
| popd > /dev/null |
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.
| mv "/tmp/${name}"*".raw" "/var/lib/extensions.d" | ||
| ln -snf "/var/lib/extensions.d/${name}"*".raw" "/var/lib/extensions/${name}.raw" |
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.
Using globs (*) with mv and ln can be risky. If the glob expands to zero, or more than one file, these commands can fail or have unintended consequences. It's safer to verify that the glob matches exactly one file before proceeding.
For example:
local files=(/path/to/"${name}"*.raw)
if [ "${#files[@]}" -ne 1 ]; then
fatal "Expected 1 file, found ${#files[@]}"
fi
# Now use "${files[0]}"
safelyPlease apply this pattern to both the mv and ln commands.
Build the sysext from Fedora RPMs and from scratch as part of the test to make this work on all releases and independently of the unofficial sysext project.
Only test on x86_64 & aarch64 for now.
Fixes: coreos/fedora-coreos-tracker#1940