-
Notifications
You must be signed in to change notification settings - Fork 119
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
Conversation
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
There was a problem hiding this 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 acontainers-storage
reference has been removed, and a new test case has been added to explicitly assert thatcontainers-storage
references are not canonicalized. - Fixes issue
bootc switch --transport dir /var/tmp/tmt/repo
failed #1308: This change directly addresses and closes issuebootc switch --transport dir /var/tmp/tmt/repo
failed #1308, which reported problems parsing complexcontainers-storage
references.
Changelog
- lib/src/spec.rs
- Modified the
ImageReference::canonicalize
method to removeTransport::ContainerStorage
from thematch
arm that performs canonicalization usingoci_spec::distribution::Reference::parse
(Diff 1, lines 116-116). - Removed a test case in the
canonicalize
test suite that expected acontainers-storage
reference to be canonicalized (Diff 2, lines 353-357). - Added a new test case to the
canonicalize
test suite confirming thatcontainers-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).
- Modified the
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
-
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. ↩
There was a problem hiding this 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 theTransport::ContainerStorage
type using logic designed forTransport::Registry
. This resolves an issue where complexcontainers-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 forcontainers-storage
and the addition of a new test case asserting no canonicalization forcontainers-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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Our parser here doesn't handle complex cases that may specify c/storage options, as used by bootc-image-builder for example.
Closes: #1308