to-disk: Fix cross-device link errors with AlmaLinux and similar images#131
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the cross-device link error by bind-mounting /var/tmp. The introduction of a parameterized integration test for multiple bootc images, including AlmaLinux, is a great addition to improve test coverage and prevent regressions. The refactoring of disk validation logic into the validate_disk_image helper function significantly improves code clarity and maintainability in the integration tests. I have a couple of minor suggestions to improve error handling in the new helper function, but overall this is a solid contribution.
9be4e0a to
a3708d0
Compare
Collaborator
Author
|
OK the problem here is yet again adding a new test started overloading the single runner. I did #133 so we can scale up our testing. |
24b147d to
733c621
Compare
gursewak1997
approved these changes
Nov 7, 2025
733c621 to
0286b35
Compare
The integration tests were creating separate disk images for tests that could share the same VM, causing unnecessary duplication of expensive `to-disk` operations. Changes: - Merged `test_base_disks_list_shows_timestamp` into `test_base_disk_creation_and_reuse` - the timestamp test no longer needs its own VM, it can verify the list output after the reuse test creates VMs - Created `test_libvirt_comprehensive_workflow` that consolidates multiple separate tests (`test_libvirt_run_list_json_ssh_metadata`, `test_libvirt_run_with_instancetype`, and `test_libvirt_run_label_functionality`) into a single test that creates one VM and verifies: - Instance type configuration - Label metadata and filtering - JSON output with SSH metadata - VM lifecycle This reduces the number of expensive disk image creations in CI, improving test performance while maintaining the same test coverage. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
Since having many to_disk run on the dinky default GHA runners can overwhelm them. Signed-off-by: Colin Walters <walters@verbum.org>
0286b35 to
0fd5cdc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #125
Switch to bind-mounting /var/tmp from the outer VM into the inner podman container. I think we fixed this in newer bootc, but this ensures compatibility with older versions.
Also add AlmaLinux to the test matrix and creates a parameterized integration test that validates to-disk works across all supported bootc images. Deduplicates disk validation logic into a shared helper function.
Assisted-by: Claude Code (Sonnet 4.5)