Skip to content

Conversation

@cgwalters
Copy link
Collaborator

Our parser here doesn't handle complex cases that may specify c/storage options, as used by bootc-image-builder for example.

Closes: #1308

Our parser here doesn't handle complex cases that may specify
c/storage options, as used by bootc-image-builder for example.

Closes: bootc-dev#1308
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @cgwalters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request.

This pull request, authored by cgwalters, addresses an issue where the code was attempting to canonicalize containers-storage image references using a parser designed for standard registry references (based on the OCI distribution spec). As noted in the description and linked issue (#1308), this parser isn't equipped to handle the potentially more complex formats or options that can be used with containers-storage, particularly in scenarios like those involving bootc-image-builder. To fix this, the PR modifies the image reference handling logic to explicitly skip canonicalization for containers-storage references, treating them as already in their desired form. This resolves the parsing issue for these types of references.

Highlights

  • Skip containers-storage canonicalization: The core change is to prevent the code from attempting to parse and canonicalize image references that use the containers-storage transport. The existing OCI distribution spec parser is not suitable for this transport's format.
  • Updated test cases: The test suite in lib/src/spec.rs has been updated to reflect this change. A test case that previously expected canonicalization for a containers-storage reference has been removed, and a new test case has been added to explicitly assert that containers-storage references are not canonicalized.
  • Fixes issue #1308: This change directly addresses and closes issue #1308, which reported problems parsing complex containers-storage references.

Changelog

  • lib/src/spec.rs
    • Modified the ImageReference::canonicalize method to remove Transport::ContainerStorage from the match arm that performs canonicalization using oci_spec::distribution::Reference::parse (Diff 1, lines 116-116).
    • Removed a test case in the canonicalize test suite that expected a containers-storage reference to be canonicalized (Diff 2, lines 353-357).
    • Added a new test case to the canonicalize test suite confirming that containers-storage references are not canonicalized (Diff 3, lines 384-387).
    • Updated a comment in the test suite from 'Also for now, we do not canonicalize OCI references' to 'Other cases are not canonicalized' (Diff 3, lines 383-383).
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Storage path complex,
Parser cannot understand,
Leave it as it is.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses the issue of incorrectly canonicalizing containers-storage image references. The change is clear, concise, and the accompanying test modifications accurately reflect the new behavior. Well done!

Summary of Findings

  • Corrected Canonicalization Logic: The primary change correctly prevents ImageReference::canonicalize from attempting to parse and modify image strings for the Transport::ContainerStorage type using logic designed for Transport::Registry. This resolves an issue where complex containers-storage options or formats could be mishandled by the OCI distribution reference parser.
  • Test Coverage: The unit tests in lib/src/spec.rs have been updated appropriately. The removal of the old test case expecting canonicalization for containers-storage and the addition of a new test case asserting no canonicalization for containers-storage effectively verify the intended behavior.

Merge Readiness

The changes in this pull request are well-implemented and address the described issue effectively. The code is clear, and the tests are updated accordingly. Based on this review, the pull request appears ready for merging. As an AI, I am not authorized to approve pull requests; please ensure it undergoes any further required review processes.

Copy link
Contributor

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jmarrero jmarrero merged commit c582017 into bootc-dev:main May 28, 2025
30 of 31 checks passed
mvo5 added a commit to mvo5/bootc-image-builder that referenced this pull request Jun 3, 2025
The bootc fedora43 install was failing but with:
bootc-dev/bootc#1337
this should now be fixed (thanks Colin!).
supakeen pushed a commit to mvo5/bootc-image-builder that referenced this pull request Jun 10, 2025
The bootc fedora43 install was failing but with:
bootc-dev/bootc#1337
this should now be fixed (thanks Colin!).
achilleas-k pushed a commit to osbuild/bootc-image-builder that referenced this pull request Jun 10, 2025
The bootc fedora43 install was failing but with:
bootc-dev/bootc#1337
this should now be fixed (thanks Colin!).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bootc switch --transport dir /var/tmp/tmt/repo failed

2 participants