-
Notifications
You must be signed in to change notification settings - Fork 206
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
manifest add: check for local images last #1919
manifest add: check for local images last #1919
Conversation
if err == nil { | ||
src.Close() | ||
} | ||
} | ||
if err != nil { |
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.
did you mean for this conditional to handle the alltransports.parseimagename from earlier ?
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.
Yes, I expect alltransports.ParseImageName() to fail when given a string that consists of "docker://" followed by exactly 64 hex digits.
@@ -325,8 +325,19 @@ func (m *ManifestList) Add(ctx context.Context, name string, options *ManifestLi | |||
if err != nil { | |||
withDocker := fmt.Sprintf("%s://%s", docker.Transport.Name(), name) | |||
ref, err = alltransports.ParseImageName(withDocker) | |||
if err == nil { |
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.
looks like this whole block up until the LookupImage part is the same in all the places where you touched this so I think it would make sense to move this into a helper function to avoid the duplication
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.
Factored out.
When adding a reference to a manifest list or image index, if we are guessing that it's a reference to an image in a registry, check if we can read something from that location instead of assuming it's correct, and if we can't, check for a local image. This will let use use local image IDs to refer to items that we want to add to a list or index. Signed-off-by: Nalin Dahyabhai <[email protected]>
4f6a77c
to
809a23e
Compare
Signed-off-by: Nalin Dahyabhai <[email protected]>
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
@vrothberg PTAL |
No time to review, please don't block on me. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nalind, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
When adding a reference to a manifest list or image index, if we are guessing that it's a reference to an image in a registry, check if we can read something from that location instead of assuming it's correct, and if we can't, check for a local image. This will let use use local image IDs to refer to items that we want to add to a list or index.
This would at least provide more of a workaround for #1896, in that specifying the desired image using only its ID would start working.