common: Sync the bootc-ubuntu-setup action here #38
Conversation
We're going to use it in most of our repos, so stop copying it manually. (At some point we may publish it, but this is a useful intermediate step) Signed-off-by: Colin Walters <walters@verbum.org>
Summary of ChangesHello @cgwalters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to avoid manual copying of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new reusable GitHub action, bootc-ubuntu-setup, to standardize the setup of Ubuntu runners. This is a great initiative for centralizing CI logic. My review has identified a critical issue in the disk cleanup logic where a background task is unintentionally run synchronously, an inefficiency in package removal, and several medium-severity suggestions to improve the action's maintainability, flexibility, and security. Please see the detailed comments below.
| for x in ${unwanted_pkgs[@]}; do | ||
| /bin/time -f '%E %C' sudo apt-get remove -y $x | ||
| done |
There was a problem hiding this comment.
Calling apt-get remove inside a for loop is inefficient because it starts a new process for each package. You can significantly speed up this step by passing all packages to a single apt-get remove command. I also recommend adding the --purge flag to also remove configuration files, which will free up more disk space.
/bin/time -f '%E %C' sudo apt-get remove --purge -y "${unwanted_pkgs[@]}"| IDV=$(. /usr/lib/os-release && echo ${ID}-${VERSION_ID}) | ||
| test "${IDV}" = "ubuntu-24.04" |
There was a problem hiding this comment.
The action hardcodes a check for ubuntu-24.04 and will fail if run on any other version. This limits the action's reusability and will require manual updates for future Ubuntu releases. Consider making this more flexible, for instance by supporting a range of versions, or at least clearly documenting this strict version requirement in the action's main description.
| shell: bash | ||
| run: | | ||
| set -xeuo pipefail | ||
| echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules |
There was a problem hiding this comment.
The udev rule sets permissions for /dev/kvm to 0666, making it world-writable. While this is a convenient approach for CI environments and is the default on some distributions like Fedora, it is less secure than the default on Debian/Ubuntu. The more secure alternative would be to use MODE="0660" and add the runner user to the kvm group. While the current implementation may be an acceptable trade-off for CI, it's important to be aware of this security consideration.
| shell: bash | ||
| run: | | ||
| set -xeuo pipefail | ||
| export BCVK_VERSION=0.5.3 |
There was a problem hiding this comment.
The BCVK_VERSION is hardcoded, which makes updating it cumbersome as it requires modifying the action's source. To improve flexibility and maintainability, this should be exposed as an input parameter with a default value.
You can add this to the inputs section of the action:
bcvk-version:
description: 'Version of bcvk to install'
required: false
default: '0.5.3'Then, you can use the input value in this step.
export BCVK_VERSION=${{ inputs.bcvk-version }}There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new GitHub Actions composite action for setting up Bootc development environments on Ubuntu runners. The action provides a standardized and optimized setup for bootc-dev organization projects.
- Creates a reusable composite action for Ubuntu runner setup with disk cleanup, package updates, and virtualization support
- Implements background cleanup operations for disk space management on GitHub runners
- Adds optional libvirt/virtualization stack installation with bcvk tool setup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| done | ||
| # Apt removals in foreground, as we can't parallelize these | ||
| for x in ${unwanted_pkgs[@]}; do | ||
| /bin/time -f '%E %C' sudo apt-get remove -y $x |
There was a problem hiding this comment.
The variable $x should be quoted to prevent word splitting and glob expansion. Change to \"$x\" to handle package names with special characters safely.
| /bin/time -f '%E %C' sudo apt-get remove -y $x | |
| /bin/time -f '%E %C' sudo apt-get remove -y "$x" |
| # Something in the stack is overriding this, but we want session right now for bcvk | ||
| echo LIBVIRT_DEFAULT_URI=qemu:///session >> $GITHUB_ENV | ||
| td=$(mktemp -d) | ||
| cd $td |
There was a problem hiding this comment.
The variable $td should be quoted to prevent word splitting issues if the path contains spaces. Change to cd \"$td\".
| cd $td | |
| cd "$td" |
| IDV=$(. /usr/lib/os-release && echo ${ID}-${VERSION_ID}) | ||
| test "${IDV}" = "ubuntu-24.04" | ||
| # plucky is the next release | ||
| echo 'deb http://azure.archive.ubuntu.com/ubuntu plucky universe main' | sudo tee /etc/apt/sources.list.d/plucky.list |
There was a problem hiding this comment.
The APT repository URL uses HTTP instead of HTTPS, which makes the connection susceptible to man-in-the-middle attacks. Consider using https://azure.archive.ubuntu.com/ubuntu instead.
| echo 'deb http://azure.archive.ubuntu.com/ubuntu plucky universe main' | sudo tee /etc/apt/sources.list.d/plucky.list | |
| echo 'deb https://azure.archive.ubuntu.com/ubuntu plucky universe main' | sudo tee /etc/apt/sources.list.d/plucky.list |
| cd $td | ||
| # Install bcvk | ||
| target=bcvk-$(arch)-unknown-linux-gnu | ||
| /bin/time -f '%E %C' curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz |
There was a problem hiding this comment.
Downloading and installing binaries without checksum verification poses a security risk. Consider adding checksum validation after downloading the tarball to ensure integrity and authenticity of the bcvk binary.
| /bin/time -f '%E %C' curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz | |
| /bin/time -f '%E %C' curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz | |
| /bin/time -f '%E %C' curl -LO https://github.com/bootc-dev/bcvk/releases/download/v${BCVK_VERSION}/${target}.tar.gz.sha256 | |
| sha256sum -c ${target}.tar.gz.sha256 |
There was a problem hiding this comment.
No, fetching the digest over the same TLS connection as the main binary doesn't add anything. We'd have to hardcode the digest here instead or use asymmetric crypto keys.
| - name: Set architecture variable | ||
| id: set_arch | ||
| shell: bash | ||
| run: echo "ARCH=$(arch)" >> $GITHUB_ENV |
There was a problem hiding this comment.
The step sets an id but the architecture value is only exported to GITHUB_ENV, not as an output. Either remove the unused id: set_arch or add a step output using $GITHUB_OUTPUT if the value needs to be referenced by other steps.
| run: echo "ARCH=$(arch)" >> $GITHUB_ENV | |
| run: | | |
| echo "ARCH=$(arch)" >> $GITHUB_ENV | |
| echo "ARCH=$(arch)" >> $GITHUB_OUTPUT |
|
Yeah, various things to improve here, but should come secondary to the sync of the existing file. |
2 participants
We're going to use it in most of our repos, so stop copying it manually.
(At some point we may publish it, but this is a useful
intermediate step)