[image] Filter unsupported images at manifest parsing#4702
[image] Filter unsupported images at manifest parsing#4702deepakshirkem wants to merge 1 commit intocanonical:mainfrom
Conversation
ricab
left a comment
There was a problem hiding this comment.
Hi @deepakshirkem, thank you for your interest. I am afraid this approach is too superficial. We'd be giving up an essential benefit in dropping unsupported images: not having to maintain any code to dealt with them at all. That would mean filtering them out at the root (when parsing simplestreams) and removing any code that dealt with them specifically. There would be no more --show-unsupported in find or UnsupportedImageException. Saying multipass launch bionic would effectively be handled just like multipass launch unknown.
|
Hi @ricab, I should have though this through more carefully. |
|
Hi @deepakshirkem, yes, that sounds reasonable. Note that the code is already aware of what images are considered unsupported. It should now skip them altogether. Be sure to have a look at our contribution guidelines at https://github.com/canonical/multipass/blob/main/CONTRIBUTING.md and good luck! |
d7ff252 to
07dc880
Compare
|
Hi @ricab, I have implemented requested changes please review. |
|
Hi @ricab, I just wanted to follow up on this PR when you get a chance. I understand you have a busy schedule. Just wanted to make sure this didn't get lost! Thank You ::)) |
|
Hi @deepakshirkem, sure, this is not lost, but things with higher priority keep getting in front and I haven't found the time yet. Hope to get to it in the next couple of weeks. |
There was a problem hiding this comment.
Pull request overview
This PR removes “unsupported (EOL) image” handling from Multipass by filtering such images out during simplestreams manifest parsing, so they never enter the image catalog and are treated like unknown images if requested by name.
Changes:
- Filter out unsupported products while parsing simplestreams manifests.
- Remove the
allow_unsupportedplumbing (Query, RPC/daemon usage, andVMImageHost::all_images_forparameter) and deleteUnsupportedImageException. - Remove CLI support/tests for
multipass find --show-unsupportedand related unit tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_ubuntu_image_host.cpp | Removes unsupported-image exception tests (but remaining tests need updating for new API/behavior). |
| tests/unit/test_image_vault.cpp | Removes warning-on-unsupported test now that unsupported images are filtered earlier. |
| tests/unit/test_cli_client.cpp | Removes CLI test for --show-unsupported. |
| tests/unit/stub_image_host.h | Updates stub host to match new all_images_for(remote_name) signature. |
| src/simplestreams/simple_streams_manifest.cpp | Filters unsupported products at manifest parse time. |
| src/image_host/ubuntu_image_host.cpp | Removes unsupported checks/exception usage; updates all_images_for signature. |
| src/image_host/custom_image_host.cpp | Updates all_images_for signature. |
| src/daemon/default_vm_image_vault.cpp | Removes catch/logging for UnsupportedImageException. |
| src/daemon/daemon.cpp | Removes allow_unsupported usage and calls updated host API. |
| src/client/cli/cmd/find.cpp | Removes --show-unsupported option and request flag setting. |
| include/multipass/query.h | Removes allow_unsupported from Query. |
| include/multipass/image_host/vm_image_host.h | Updates all_images_for interface to remove allow_unsupported. |
| include/multipass/image_host/ubuntu_image_host.h | Updates host interface implementation signature. |
| include/multipass/image_host/custom_image_host.h | Updates host interface implementation signature. |
| include/multipass/exceptions/unsupported_image_exception.h | Removes the exception type entirely. |
Comments suppressed due to low confidence (1)
tests/unit/test_ubuntu_image_host.cpp:416
- After removing the unsupported-image path, this test file still contains
all_images_for(..., bool)calls and expectations that depended on unsupported images being present (e.g. expecting 5 images when including unsupported). Those remaining tests need to be updated to the newall_images_for(remote_name)signature and to the new counts (unsupported images should be absent).
TEST_F(UbuntuImageHost, develRequestWithNoRemoteReturnsExpectedInfo)
{
mp::UbuntuVMImageHost host{all_remote_specs, &url_downloader};
host.update_manifests(false);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| virtual std::vector<std::pair<std::string, VMImageInfo>> all_info_for(const Query& query) = 0; | ||
| virtual VMImageInfo info_for_full_hash(const std::string& full_hash) = 0; | ||
| virtual std::vector<VMImageInfo> all_images_for(const std::string& remote_name, | ||
| const bool allow_unsupported) = 0; | ||
| virtual std::vector<VMImageInfo> all_images_for(const std::string& remote_name) = 0; | ||
| virtual void for_each_entry_do(const Action& action) = 0; |
There was a problem hiding this comment.
VMImageHost::all_images_for signature was changed to take only remote_name, but several unit tests/mocks still use the old (remote_name, allow_unsupported) overload (e.g. tests/unit/mock_image_host.h, tests/unit/test_custom_image_host.cpp, tests/unit/test_daemon_find.cpp, tests/unit/test_ubuntu_image_host.cpp). This will break the build until those call sites and mock declarations are updated to match the new interface and expectations adjusted (unsupported images should no longer be counted/listed).
| std::vector<mp::VMImageInfo> mp::UbuntuVMImageHost::all_images_for(const std::string& remote_name) | ||
| { | ||
| std::vector<mp::VMImageInfo> images; | ||
| auto manifest = manifest_from(remote_name); | ||
|
|
||
| for (const auto& entry : manifest->products) | ||
| { | ||
| if (entry.supported || allow_unsupported) | ||
| { | ||
| images.push_back(entry); | ||
| } | ||
| images.push_back(entry); | ||
| } |
There was a problem hiding this comment.
There is trailing whitespace on the function declaration line and inside the loop body here (after remote_name) and after images.push_back(entry);). Please remove it to keep the file clean and avoid style/lint issues.
|
|
||
| if (!supported) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
These newly added blank lines contain trailing whitespace. Please remove the extra spaces to avoid unnecessary diff noise and potential whitespace-lint failures.
| if (!supported) | |
| continue; | |
| if (!supported) | |
| continue; |
| const auto supported = | ||
| product["supported"].toBool() || product_aliases.contains("devel") || | ||
| (product["os"] == "ubuntu-core" && product["image_type"] == "stable"); | ||
|
|
||
|
|
||
| if (!supported) | ||
| continue; | ||
|
|
There was a problem hiding this comment.
Filtering out unsupported products at manifest parse time is a behavior change that should be covered by a unit test. There is already tests/unit/test_simple_streams_manifest.cpp with fixtures and test data containing "supported": false; please add/adjust a test to assert that those products (and their aliases) are not present in products/image_records after parsing.
| } | ||
|
|
||
| request.set_allow_unsupported(parser->isSet(unsupportedOption)); | ||
|
|
There was a problem hiding this comment.
This blank line contains trailing whitespace after removing --show-unsupported handling. Please delete the extra spaces to keep the diff clean.
ricab
left a comment
There was a problem hiding this comment.
Hi @deepakshirkem, it looks like you did not fully build the project?? There are compilation errors.
Can you please address copilot's complaints and rebase on main? Thank you.
| const auto supported = | ||
| product["supported"].toBool() || product_aliases.contains("devel") || | ||
| (product["os"] == "ubuntu-core" && product["image_type"] == "stable"); | ||
|
|
||
|
|
||
| if (!supported) | ||
| continue; | ||
|
|
||
| const auto versions = product["versions"].toObject(); | ||
| if (versions.isEmpty()) | ||
| continue; |
There was a problem hiding this comment.
It's a detail, but if we are going to skip, better move the check as early as possible in the loop. For consistency, you can move the check on versions too, and pack the two checks together with the arch check above. You'll still need product_aliases though.
There was a problem hiding this comment.
Hi @ricab, I have moved the supported check earlier in the loop and packed it together with the versions check Both checks now happen as early as possible after the arch check, keeping product_aliases available for the supported calculation.
07dc880 to
47b346c
Compare
|
Hi @ricab , Apologies for the compilation errors in the previous commit. I had built locally without tests enabled. I have now enabled tests and fixed all the issues. Thank You ::)) |
| } | ||
| } | ||
|
|
||
| request.set_allow_unsupported(parser->isSet(unsupportedOption)); | ||
| } | ||
| request.set_force_manifest_network_download(parser->isSet(force_manifest_network_download)); | ||
|
|
There was a problem hiding this comment.
This still has trailing whitespace. In fact, there is no reason to change these lines at all.
|
|
||
| return images; | ||
| } |
47b346c to
5e884f9
Compare
5e884f9 to
82a652d
Compare
|
Hi @ricab, I address all suggestion and rebase with main. Please review once you get chance. |
ricab
left a comment
There was a problem hiding this comment.
Not quite there yet. If we're going to skip unsupported images entirely, we should not need to mark images as un/supported.
| if (!supported) | ||
| continue; |
There was a problem hiding this comment.
Now if we're going to skip these, no point having the supported flag in VMImageInfo, right?
| info = manifest->image_records.find("16.04"); | ||
| EXPECT_EQ(info, manifest->image_records.end()); | ||
| } | ||
| } // namespace No newline at end of file |
Description
What does this PR do?
Filters out unsupported (EOL) images at the simplestreams manifest parsing level, so they never enter the system at all.
Attempting to launch an unsupported image by name is handled just like launching an unknown image. URL-based launches remain available.
Why is this change needed?
Previously, Multipass was aware of unsupported images and had special code to deal with them (UnsupportedImageException, --show-unsupported flag). This change removes that complexity entirely by filtering at the root.
Related Issue(s)
Closes #3934
Testing
multipass launch xenial— returns unknown image errormultipass launch 22.04— launches successfullymultipass find— should only show supported imagesScreenshots (if applicable)
Checklist
Additional Notes
NA