-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ubuntu and Fedora #16
Conversation
WalkthroughThe project has introduced a matrix strategy for CI workflows to support multiple operating systems, Ubuntu and Fedora, enhancing the build process's flexibility. The Dockerfiles and scripts have been updated to accommodate these changes, including dynamic package installation and configuration adjustments based on the distribution. Additionally, the project now emphasizes creating fast-booting MicroVMs for self-hosted GitHub runners, supporting multi-platform OCI images, and refining the base image to optimize runner execution. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- .github/workflows/ci.yml (2 hunks)
- .github/workflows/dev.yml (2 hunks)
- .github/workflows/test.yml (1 hunks)
- Dockerfile (2 hunks)
- Dockerfile.base.ubuntu (1 hunks)
- README.md (4 hunks)
- base/base.sh (3 hunks)
- base/root/etc/containers/storage.conf (1 hunks)
- runner.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/test.yml
Additional comments: 20
Dockerfile.base.ubuntu (4)
- 2-2: The default
ARG UBUNTU_VERSION=23.10
sets the Ubuntu version to 23.10. This is a good practice as it allows for easy updates and customization. However, consider verifying if this is the most stable and compatible version for the project's needs.- 5-5: The
ARG INSTALL_NAMESPACE=/opt/gh-runner-krunvm
defines a custom installation namespace. This is a good practice for organizing project-related files and avoiding conflicts with system directories.- 7-10: Copying scripts and setting executable permissions before running the base installation script is a standard procedure in Dockerfiles. However, ensure that the scripts in
base/*.sh
andlib/*.sh
are properly sanitized and do not contain any sensitive information or security vulnerabilities.- 11-11: Copying the
base/root/
directory to the root of the filesystem can potentially overwrite system files or configurations. Ensure that this operation is safe and does not introduce security risks or unwanted changes to the base image.Dockerfile (2)
- 3-3: Setting
ARG DISTRO=fedora
by default aligns with the PR's objective to support both Ubuntu and Fedora. However, since the PR aims to make Ubuntu the default, consider if this default value should be changed to Ubuntu or if the choice of default is determined elsewhere in the project.- 13-13: The command
"${INSTALL_NAMESPACE}/bin/install.sh" -v
runs the installation script with verbosity. Ensure that theinstall.sh
script is robust against failures and correctly handles errors, especially since it's being run in a Docker build process where failure handling is crucial..github/workflows/ci.yml (2)
- 11-16: Introducing a matrix strategy for
os: [ubuntu, fedora]
in thebuild-base
job is a great way to support building images for both operating systems. This approach enhances the project's flexibility and aligns with the PR objectives. Ensure that theimage
andfile
values dynamically adjust based on the operating system.- 25-33: Similarly, the matrix strategy in the
build-main
job supports building images for both Ubuntu and Fedora. AddingDISTRO=${{ matrix.os }}
to thebuild-args
is a good practice for passing the operating system context to the Docker build process. Ensure that the downstream Dockerfiles and scripts correctly utilize this argument..github/workflows/dev.yml (2)
- 11-16: The matrix strategy for
os: [ubuntu, fedora]
in thebuild-base
job within thedev.yml
workflow is consistent with the changes in theci.yml
file. This consistency is crucial for maintaining a uniform build process across different environments. Ensure that theplatforms: linux/amd64
specification aligns with the project's platform support objectives.- 26-35: The matrix strategy in the
build-main
job, including theDISTRO=${{ matrix.os }}
build argument, is correctly applied here as well. This ensures that development builds also benefit from the multi-platform support. Verify that theVERSION=${{ needs.build-base.outputs.version }}
correctly captures the version from thebuild-base
job output.base/base.sh (6)
- 75-84: The introduction of the
apt_install
function for Ubuntu package installation is a good practice for code reuse and maintainability. Ensure thatapt-get update
andapt-get install -y "$@"
are correctly handling errors and that the script exits if these commands fail.- 87-161: The
install_base
function dynamically handles package installation based on the distribution type (fedora
orubuntu
). This is a significant improvement in code organization and flexibility. Ensure that the package lists for both distributions are up-to-date and aligned with the project's requirements.- 72-183: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [163-191]
The
install_docker
function correctly handles Docker installation for both Fedora and Ubuntu, including setting up repositories and installing Docker packages. The replacement of the real Docker binary with a wrapper for network configuration is a clever workaround. Ensure that this approach does not introduce security risks or compatibility issues.
- 193-211: The
install_gh
function for installing the GitHub CLI is well-implemented for both Fedora and Ubuntu. This ensures that the GitHub CLI is available in the runner environment, enhancing automation capabilities. Verify that the repository configurations and package installations are secure and up-to-date.- 213-217: The
install_yq
function downloads and installsyq
directly from GitHub releases. While this approach is straightforward, ensure that the download is secure (using HTTPS) and consider verifying the integrity of the downloaded binary.- 219-246: The
create_user
function dynamically handles user and group creation based on the distribution, including adding the user to the appropriatesudo
group. This is a critical step for runner configuration. Ensure that user and group IDs do not conflict with existing system users and that theNOPASSWD
sudo configuration does not introduce security risks.README.md (2)
- 8-9: The update to the README.md file to mention support for multi-platform OCI images for Ubuntu (default) and Fedora aligns with the PR objectives. This clarification enhances the project's documentation and helps users understand the available options.
- 120-124: The explanation of the base images installing a minimal set of binaries and packages is an important detail that helps set expectations for users. It's good practice to highlight the differences between these base images and regular GitHub runners. Ensure that the list of installed packages is documented somewhere accessible for users to review.
runner.sh (1)
- 43-43: The change to use
ghcr.io/efrecon/runner-krunvm-ubuntu:main
as the default OCI image aligns with the PR's objective to make Ubuntu the default OS for the project's Docker images. This is a straightforward update and correctly implements the desired functionality.base/root/etc/containers/storage.conf (1)
- 1-247: The modifications in
storage.conf
appear to be comprehensive and well-documented, providing clear instructions and defaults for container storage management. This includes the setup for the default storage driver, paths for container storage, and various options for managing storage layers, performance optimizations, and security settings. The inclusion offuse-overlayfs
as themount_program
and enablingmetacopy=on
formountopt
are particularly noteworthy for improving performance and efficiency in container storage operations. These changes are consistent with best practices for container storage configuration and should contribute positively to the project's infrastructure.
Also create Ubuntu images and make them the default. The set of packages is almost the same in both OSes.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chore