spec: Note we don't canonicalize oci #1311
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The previous code in trying to parse `oci` was wrong; the syntax for an `oci` transport is the same as oci-archive, which includes a file path (and there's no mechanism to quote `:` note). The canonical logic for all of this stuff is in Go, there's no canonical Rust library (yet, though I did think about putting this in oci-spec). Previous to this attempt to handle tagged+digested, we weren't parsing image references at all. First, factor out a `canonicalize_reference` helper since that's what we're really doing here, it's independent of the *transport*. Fix the canonicalize function to drop out trying to parse `oci`. Add a separate test case that incorrectly passes just so it's a bit more obvious to fix this later. Note that today at least `skopeo` rejects trying to fetch via tagged+digested form from an `oci:` so it's fine if we don't canonicalize here yet, even though it could confuse someone. Signed-off-by: Colin Walters <walters@verbum.org>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors image reference canonicalization logic by extracting a helper function (canonicalize_reference) to manage the transformation, and it adds tests to validate behavior.
- Factored out the canonicalize_reference function to handle tag-and-digest logic
- Adjusted canonicalization flow in ImageReference for different transports
- Introduced tests to verify expected transformations and identify uncanonicalized OCI references
Comments suppressed due to low confidence (1)
lib/src/spec.rs:119
- Typo in comment: 'canonicicalation' should be 'canonicalization'.
// Check if the image reference needs canonicicalation
|
Ohh now that I look more closely it's just the Fedora rawhide provisioning which is failing. I am not sure we should actually be running jobs against Fedora rawhide in a gating fashion here, at least not until that project has more gating. The same CI checks passed with c10s. |
ckyrouac
approved these changes
May 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous code in trying to parse
ociwas wrong; the syntax for anocitransport is the same as oci-archive, which includes a file path (and there's no mechanism to quote:note).The canonical logic for all of this stuff is in Go, there's no canonical Rust library (yet, though I did think about putting this in oci-spec).
Previous to this attempt to handle tagged+digested, we weren't parsing image references at all.
First, factor out a
canonicalize_referencehelper since that's what we're really doing here, it's independent of the transport.Fix the canonicalize function to drop out trying to parse
oci.Add a separate test case that incorrectly passes just so it's a bit more obvious to fix this later.
Note that today at least
skopeorejects trying to fetch via tagged+digested form from anoci:so it's fine if we don't canonicalize here yet, even though it could confuse someone.