refactor(images): remove VM base image - #18776
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new Python tests violate enforced Ruff S101 rules, and the README still documents the removed test directory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Removes the redundant vm-base image while retaining its shared package profile and replacing it with 1p-vm-base-gen2.
Changes:
- Moves Gen2 UEFI configuration into the 1P hierarchy.
- Registers the replacement image and transfers its test suites.
- Adds replacement-specific kernel and partition checks.
File summaries
| File | Description |
|---|---|
base/images/tests/README.md |
Updates VM test examples. |
base/images/tests/cases/static/1p-vm-base-gen2/test_partitions.py |
Adds partition checks. |
base/images/tests/cases/static/1p-vm-base-gen2/test_kernel.py |
Adds kernel checks. |
base/images/teams/cloud/vm-base.xml |
Retains only VmBaseCore. |
base/images/teams/cloud/onep.xml |
Defines the Gen2 UEFI profile. |
base/images/images.toml |
Replaces the image registration. |
Review details
- Files reviewed: 4/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
80baf57 to
9d54a10
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The replacement preserves the shared profile hierarchy and correctly redirects image registration and static validation.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7bd3adef-a234-420d-b07f-e7301af0b050
As part of consolidating images to 1P and 3P variants for both Gen1 and Gen2 image types, drop the standalone vm-base image. vm-base was effectively the Gen2 1P x86_64 image and was the first image built in this repo, predating the current 1P/3P hierarchy. Now that the hierarchical image definitions and the rest of the 1P/3P Gen1/Gen2 images are in place, vm-base is redundant: 1p-vm-base-gen2 is a strict superset of vm-base, carrying 3 extra packages (rsyslog, libestr, libfastjson) on top of an otherwise identical package set. Retain the shared VmBaseCore profile for 1P images and move the UEFI static checks to the 1P Gen2 replacement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3188afea-d864-49af-acba-88a493e5b703
9d54a10 to
90f164f
Compare
Tobias Brick (tobiasb-ms)
left a comment
Tobias Brick (tobiasb-ms)
left a comment
There was a problem hiding this comment.
Image changes themselves look good but I'm confused about the changes away from assert in the tests.
| if not modules_dir.exists(): | ||
| modules_dir = rootfs / "lib" / "modules" | ||
| assert modules_dir.exists(), "No kernel modules directory found" | ||
| if not modules_dir.exists(): |
There was a problem hiding this comment.
issue(blocking): Why are you changing these from assert to if with fail? Are you changing behavior in any way? What's the purpose of the changes?
| @@ -13,21 +13,26 @@ def test_kernel_modules_present(rootfs: Path) -> None: | |||
| modules_dir = rootfs / "usr" / "lib" / "modules" | |||
There was a problem hiding this comment.
question(non-blocking): should these tests be marked as applicable to all VM images? they were presumably associated with vm-base when that was our only VM image.
| modules_dir = rootfs / "lib" / "modules" | ||
| assert modules_dir.exists(), "No kernel modules directory found" | ||
| if not modules_dir.exists(): | ||
| pytest.fail("No kernel modules directory found") |
There was a problem hiding this comment.
question(blocking): Why were these assertions changed to pytest.fail() calls? Ditto for below.
| <requires profile="LegacyBoot" /> | ||
| <requires profile="OnePBase" /> | ||
| </profile> | ||
| <profile name="1p-vm-base-gen2" description="1P VM Base Gen2 Image (UEFI)" import="false"> |
There was a problem hiding this comment.
question(blocking): Why are there XML changes? Did we not already have the right composition? I didn't see the explanation in the PR summary.
Summary
vm-baseimageVmBaseCoreprofile for 1P images1p-vm-base-gen2replacementRationale
vm-basepredates the current 1P/3P Gen1/Gen2 hierarchy and is superseded by1p-vm-base-gen2, which otherwise has the same package set plusrsyslog,libestr, andlibfastjson.