referenceutil cleanup
#3542
Merged
referenceutil cleanup
#3542
Conversation
This file contains 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
apostasie
force-pushed
the
rewrite-refutils
branch
2 times, most recently
from
91b69c4 to
0e9f388
Compare
Signed-off-by: apostasie <[email protected]>
apostasie
force-pushed
the
rewrite-refutils
branch
2 times, most recently
from
3b5eac2 to
c937f3b
Compare
Signed-off-by: apostasie <[email protected]>
apostasie
force-pushed
the
rewrite-refutils
branch
from
c937f3b to
efb9a87
Compare
apostasie
commented
Oct 15, 2024
| @@ -235,19 +235,19 @@ func generateBuildctlArgs(ctx context.Context, client *containerd.Client, option | |||
| } | |||
| if tags = strutil.DedupeStrSlice(options.Tag); len(tags) > 0 { | |||
| ref := tags[0] | |||
| named, err := distributionref.ParseNormalizedNamed(ref) | |||
| parsedReference, err := referenceutil.Parse(ref) | |||
There was a problem hiding this comment.
Harmonizing variables names from named, canonicalRef, etc, to just the same parsedReference.
apostasie
commented
Oct 15, 2024
| if err != nil { | ||
| return nil, "", "", fmt.Errorf("invalid identifier %s: %w", identifier, err) |
There was a problem hiding this comment.
Useless. The underlying implementation does return that already.
apostasie
commented
Oct 15, 2024
| @@ -70,49 +70,47 @@ Properties: | |||
| return imagesCommand | |||
| } | |||
|
|
|||
| func processImageListOptions(cmd *cobra.Command, args []string) (types.ImageListOptions, error) { | |||
| func processImageListOptions(cmd *cobra.Command, args []string) (*types.ImageListOptions, error) { | |||
There was a problem hiding this comment.
Change the signature here so that we can return nil instead of types.ImageListOptions{}.
apostasie
commented
Oct 15, 2024
| } | ||
| filters = append(filters, fmt.Sprintf("name==%s", canonicalRef.String())) | ||
| filters = append(filters, fmt.Sprintf("name==%s", args[0])) |
There was a problem hiding this comment.
This does not seem to serve a purpose.
Am I missing something here?
apostasie
commented
Oct 15, 2024
| @@ -61,7 +61,7 @@ type PullMode = string | |||
| // GetExistingImage returns the specified image if exists in containerd. Return errdefs.NotFound() if not exists. | |||
| func GetExistingImage(ctx context.Context, client *containerd.Client, snapshotter, rawRef string, platform ocispec.Platform) (*EnsuredImage, error) { | |||
| var res *EnsuredImage | |||
| imagewalker := &imagewalker.ImageWalker{ | |||
| imgwalker := &imagewalker.ImageWalker{ | |||
There was a problem hiding this comment.
Not shadowing import.
ktock
approved these changes
Oct 17, 2024
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.
This is a first step at enhancing and fixing issues in imgutil (#3531) - focusing specifically on
referenceutil.We currently use a mix of
nerdctl/referenceutilandgithub.ghproxy.top/distribution/referenceas far as I can tell without much consistency.We obviously introduced our own "helpers" in
referenceutil(primarily to accommodate for ipfs images), sometimes with identical names to the upstream API:... in addition to the existing:
This abundance of methods that are doing almost exactly the same thing - with subtle differences - seems hard to justify - be it on our side, or on the distribution side.
The distribution ref code was designed ten years ago now (fall 2014), with different notions about images names, backward compatibility requirements with the old python registry (prior to the go implem), and quite frankly, a questionable design (the whole "cast it to
XYZif you want to access properties"), and a number of embarrassing bugs (specifically around tags+digests), that IIRC have been creeping far out (into buildkit), causing hard-to-figure-out caching issues.It seems like
referencehas been barely touched since then.At this point, it is doubtful that anyone could say top of the head what are the differences between all of these or what are the ramifications if you use one rather than the other.
While this is challenging already for long-timers (I certainly do need to read the source multiple times every time I touch these), it is just plain bad for newcomers.
Add to that normalization concerns, the familiarization mechanisms that work only post normalization. This is just not a good API.
Finally, we seem to parse these references multiples times during the same flow as we pass strings around - in a context where
distribution/referenceis notoriously hungry for memory.This PR is suggesting that we stop using
distributionrefentirely (eg: except inside referenceutil obviously), and replace all of our stuff with a new, single method -Parse- and one struct.The first commit is the implementation of the new referenceutil + tests.
The second commit adapts the codebase - this is not too aggressive of a refactor, and we might want to further clean this up to reduce duplicate parsing - but that would require changing methods signatures, which this PR tries to avoid.
Let me know your thoughts - especially if I am missing something here that would not be in the tests, and that would justify specialized parsing in certain cases.