Skip to content

spec: Don't canonicalize containers-storage #1337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 28, 2025

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

@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 bootc switch --transport dir /var/tmp/tmt/repo failed #1308: This change directly addresses and closes issue bootc switch --transport dir /var/tmp/tmt/repo failed #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

@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
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