quality/os-autoinst-distri-fedora

Fork 2

Use image-builder for building ostree installers on F45+ (#542) #545

Merged

adamwill merged 1 commit from ostree-image-builder into main

2026-05-09 23:10:02 +00:00

adamwill commented

2026-05-09 22:29:30 +00:00

Owner

We recently switched to using image-builder for building ostree
installer images in Rawhide (F45):

releng/pungi-fedora#1641

The openQA tests are meant to check that the official builds will
work, so they have to shadow the official builds; thus we must
also switch our F45+ builds to image-builder. This is pretty
self-explanatory. I've reported the additional RAM requirements
and lack of .repo file support to @supakeen and will update the
test if those are resolved.

Signed-off-by: Adam Williamson awilliam@redhat.com

We recently switched to using image-builder for building ostree installer images in Rawhide (F45): https://forge.fedoraproject.org/releng/pungi-fedora/pulls/1641 The openQA tests are meant to check that the official builds will work, so they have to shadow the official builds; thus we must also switch our F45+ builds to image-builder. This is pretty self-explanatory. I've reported the additional RAM requirements and lack of .repo file support to @supakeen and will update the test if those are resolved. Signed-off-by: Adam Williamson <awilliam@redhat.com>

adamwill added 1 commit

2026-05-09 22:29:30 +00:00

Use image-builder for building ostree installers on F45+ (#542 )

CI via Tox and perl / tox (pull_request) Successful in 1m21s

Details

CI via Tox and perl / perl (pull_request) Failing after 3m9s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

AI Code Review / ai-review (pull_request_target) Successful in 40s

Details

dcf580ad3c

We recently switched to using image-builder for building ostree
installer images in Rawhide (F45):

releng/pungi-fedora#1641

The openQA tests are meant to check that the official builds will
work, so they have to shadow the official builds; thus we must
also switch our F45+ builds to image-builder. This is pretty
self-explanatory. I've reported the additional RAM requirements
and lack of .repo file support to @supakeen and will update the
test if those are resolved.

Signed-off-by: Adam Williamson <awilliam@redhat.com>

forgejo-actions commented

2026-05-09 22:35:17 +00:00

AI Code Review

Detailed Code Review

The migration to image-builder for F45+ is logically structured and appropriately segregated from the legacy lorax path. The openQA test runner successfully delegates to the correct build tooling based on the release number.

However, there are a few edge cases that could cause the test scripts to fail during runtime. In the Python parsing scripts, there are assumptions about string and dictionary formats that could lead to unhandled exceptions (like ValueError and KeyError). Additionally, there is a Perl scoping issue in the test runner where a variable is redeclared, which will trigger warnings and could cause strict test environments to fail. Implementing safer accessors and splits will ensure better reliability.

📂 File Reviews

📄 `data/updvercheck.py` - Adds a function to parse image-builder SPDX files and extract package versions.

Major [Bug]: Unpacking purl["version"].split("-", 1) directly into two variables will raise a ValueError: not enough values to unpack if a package version does not contain a hyphen. While RPMs typically have a release attached, third-party or irregularly formatted packages could crash the script.
- Suggestion: Split the string into a list and check the length before assigning.

        version_parts = purl["version"].split("-", 1)
        version = version_parts[0]
        release = version_parts[1] if len(version_parts) > 1 else "0"
        return f"unknown {purl['name']} {purl['qualifiers']['epoch']} {version} {release}"

📄 `tests/_ostree_build.pm` - Updates the test runner to use `image-builder` for F45+ and correctly passes the corresponding parameters and logs.

Minor [Bug]: The $cmd variable is declared with my twice within the same subroutine scope. This masks the earlier declaration and triggers a "my" variable $cmd masks earlier declaration in same scope warning in Perl. If the test environment treats warnings as fatal, this will fail the test.
- Suggestion: Remove the my keyword from the second assignment.

    my $logfn = $imagebuilder ? "${advortask}-${subv}-ostree-${arch}.image-anaconda-tree.spdx.json" : "pylorax.log";
    # Fixed: Removed 'my' to avoid redeclaration warning
    $cmd = "python3 ./updvercheck.py /mnt/updatepkgs.txt ${logfn}";

📄 `data/ostree-parse-pungi-ib.py` - New script to parse the pungi configuration and extract the image-builder blueprint.

Minor [Logic]: Deep dictionary access for forces = blueprint["customizations"]["installer"]["payload"]["flatpaks"]["force"] assumes the structure exists perfectly. If any of these keys are missing from the configuration, it will raise a KeyError.
- Suggestion: Wrap the access in a try/except block to provide a clear error message, or use .get() with default fallbacks if the force section is optional.

try:
    forces = blueprint["customizations"]["installer"]["payload"]["flatpaks"]["force"]
    for force in forces:
        force["references"] = [BASEPATT.sub(r"\g<1>" + BASEVER, ref).replace("$arch", arch) for ref in force["references"]]
except KeyError:
    pass # Or handle accordingly if it is strictly required

✅ Summary

Overall Assessment: The changes successfully implement the image-builder migration, but contain a few minor bugs involving unsafe dictionary/string accesses and a Perl variable redeclaration that should be fixed to ensure script stability.
Priority Issues:
- Fix ValueError risk in updvercheck.py when splitting PURL versions that might lack a hyphen
- Fix Perl scope warning in _ostree_build.pm by removing the duplicate my declaration for $cmd
Minor Suggestions:
- Add KeyError handling for deep dictionary accesses in ostree-parse-pungi-ib.py to prevent confusing stack traces if the config structure changes

🤖 AI Code Review | Generated with ai-code-review | Model: gemini-3.1-pro-preview

⚠️ AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.

## AI Code Review ### Detailed Code Review The migration to `image-builder` for F45+ is logically structured and appropriately segregated from the legacy `lorax` path. The openQA test runner successfully delegates to the correct build tooling based on the release number. However, there are a few edge cases that could cause the test scripts to fail during runtime. In the Python parsing scripts, there are assumptions about string and dictionary formats that could lead to unhandled exceptions (like `ValueError` and `KeyError`). Additionally, there is a Perl scoping issue in the test runner where a variable is redeclared, which will trigger warnings and could cause strict test environments to fail. Implementing safer accessors and splits will ensure better reliability. #### 📂 File Reviews <details> <summary>📄 `data/updvercheck.py` - Adds a function to parse image-builder SPDX files and extract package versions.</summary> - **Major** [Bug]: Unpacking `purl["version"].split("-", 1)` directly into two variables will raise a `ValueError: not enough values to unpack` if a package version does not contain a hyphen. While RPMs typically have a release attached, third-party or irregularly formatted packages could crash the script. - *Suggestion:* Split the string into a list and check the length before assigning. ```` version_parts = purl["version"].split("-", 1) version = version_parts[0] release = version_parts[1] if len(version_parts) > 1 else "0" return f"unknown {purl['name']} {purl['qualifiers']['epoch']} {version} {release}" ```` </details> <details> <summary>📄 `tests/_ostree_build.pm` - Updates the test runner to use `image-builder` for F45+ and correctly passes the corresponding parameters and logs.</summary> - **Minor** [Bug]: The `$cmd` variable is declared with `my` twice within the same subroutine scope. This masks the earlier declaration and triggers a `"my" variable $cmd masks earlier declaration in same scope` warning in Perl. If the test environment treats warnings as fatal, this will fail the test. - *Suggestion:* Remove the `my` keyword from the second assignment. ```` my $logfn = $imagebuilder ? "${advortask}-${subv}-ostree-${arch}.image-anaconda-tree.spdx.json" : "pylorax.log"; # Fixed: Removed 'my' to avoid redeclaration warning $cmd = "python3 ./updvercheck.py /mnt/updatepkgs.txt ${logfn}"; ```` </details> <details> <summary>📄 `data/ostree-parse-pungi-ib.py` - New script to parse the pungi configuration and extract the image-builder blueprint.</summary> - **Minor** [Logic]: Deep dictionary access for `forces = blueprint["customizations"]["installer"]["payload"]["flatpaks"]["force"]` assumes the structure exists perfectly. If any of these keys are missing from the configuration, it will raise a `KeyError`. - *Suggestion:* Wrap the access in a try/except block to provide a clear error message, or use `.get()` with default fallbacks if the `force` section is optional. ```` try: forces = blueprint["customizations"]["installer"]["payload"]["flatpaks"]["force"] for force in forces: force["references"] = [BASEPATT.sub(r"\g<1>" + BASEVER, ref).replace("$arch", arch) for ref in force["references"]] except KeyError: pass # Or handle accordingly if it is strictly required ```` </details> ### ✅ Summary - **Overall Assessment:** The changes successfully implement the image-builder migration, but contain a few minor bugs involving unsafe dictionary/string accesses and a Perl variable redeclaration that should be fixed to ensure script stability. - **Priority Issues:** - Fix `ValueError` risk in `updvercheck.py` when splitting PURL versions that might lack a hyphen - Fix Perl scope warning in `_ostree_build.pm` by removing the duplicate `my` declaration for `$cmd` - **Minor Suggestions:** - Add `KeyError` handling for deep dictionary accesses in `ostree-parse-pungi-ib.py` to prevent confusing stack traces if the config structure changes --- 🤖 **AI Code Review** | Generated with [ai-code-review](https://gitlab.com/redhat/edge/ci-cd/ai-code-review) | **Model:** `gemini-3.1-pro-preview` ⚠️ *AI-generated suggestions may be incorrect. Verify before applying. Not a replacement for human review.*

adamwill force-pushed ostree-image-builder from dcf580ad3c

CI via Tox and perl / tox (pull_request) Successful in 1m21s

Details

CI via Tox and perl / perl (pull_request) Failing after 3m9s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

AI Code Review / ai-review (pull_request_target) Successful in 40s

Details

to 6e4f9daa3e

CI via Tox and perl / tox (pull_request) Successful in 1m19s

Details

CI via Tox and perl / perl (pull_request) Successful in 3m8s

Details

CI via Tox and perl / checkwiki (pull_request) Successful in 33s

Details

2026-05-09 22:36:16 +00:00

Compare

adamwill commented

2026-05-09 22:40:27 +00:00

Author

Owner

I fixed the my $cmd thing but I'm fine with the other assumptions. I don't think we're going to have packages without releases, and if the SPDX file formats we need to know, crashing is fine.

I fixed the `my $cmd` thing but I'm fine with the other assumptions. I don't think we're going to have packages without releases, and if the SPDX file formats we need to know, crashing is fine.

adamwill commented

2026-05-09 23:08:28 +00:00

Author

Owner

Note: x86_64 builds succeed with "only" 8G of RAM, but aarch64 seem to need 9.