-
Notifications
You must be signed in to change notification settings - Fork 42
Skip qemu installation on macos-13 runners #392
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
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The approach/PR looks good to me, but it’s CI which will tell us whether it’s good to go in or not :) |
|
The CI seems to be taking forever with this change 😞 |
|
It still tried to build some brew formulas from source instead of using pre-built binaries/libraries |
WalkthroughAdds conditional execution to CI workflow and tests to skip qemu-img operations on macos-13 due to unavailability. The workflow conditionally installs and verifies qemu-img, while a test skips execution when the environment indicates macos-13. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/config_test.go (1)
52-54: Consider checking qemu-img availability directly instead of hardcoded OS check.While this skip aligns with the workflow changes, using a hardcoded string comparison with
"macos-13"is brittle. If the matrix value format changes (e.g., to"macOS-13"or"macos_13"), the check will silently fail.Consider checking for qemu-img availability directly:
- if os.Getenv("RUNNER_OS_VERSION") == "macos-13" { - t.Skip("Skipping test on macos-13 due to qemu-img not being available") - } + if _, err := exec.LookPath("qemu-img"); err != nil { + t.Skip("Skipping test: qemu-img not available") + }This approach:
- Works regardless of OS or runner version
- Self-documents the actual dependency
- Fails gracefully in any environment without qemu-img
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/compile.yml(1 hunks)pkg/config/config_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-13)
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-15)
- GitHub Check: lint
🔇 Additional comments (2)
.github/workflows/compile.yml (2)
61-62: Good: Environment variable propagation enables OS-aware test skipping.This correctly passes the runner OS version to unit tests, enabling the test skip logic in
pkg/config/config_test.go.
52-59: Remove review comment—implementation is correct and internally consistent.The code doesn't attempt to install qemu-img on macos-13; instead, it intentionally skips installation and the dependent tests properly handle this. The test in
pkg/config/config_test.go(lines 52-54) explicitly skips on macos-13 using theRUNNER_OS_VERSIONenvironment variable that your workflow propagates. Integration tests will run on macos-13 but skip the qemu-img-dependent tests—they won't fail.However, if the PR title/description states qemu will be installed on macos-13, update them to clarify the actual behavior: qemu-img is not installed on macos-13 and dependent tests are skipped.
|
There's only one test that uses qemu-img. The latest commit skips the test so the CI can pass. |
|
This should no longer be needed with #376 |
qemu-imgstep fails on all workflows on macos-13.Summary by CodeRabbit