Pull cleanups #1361
Closed
Pull cleanups #1361
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
|
LGTM |
mtrmac
force-pushed
the
pull-cleanups
branch
2 times, most recently
from
a2eb352 to
e9715b1
Compare
Does not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
No need to hard-code the :tag / @digest syntax when there already is an API returning the string representation. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
When ResolveName has already determined that the value is an ID (prefix), and returned the full ID, rely on that knowledge and don't try at all to pull the image from a 'remote transport ""'; also, don't try to match strings that are already known not to be ID prefixes, or that are known to use a different transport, against local storage. Should not change behavior, except possibly in theoretical inconsistency cases when store.Image(knownImageID) fails; the code now does not report other unrelated errors on the transport == "" path below. Signed-off-by: Miloslav Trmač <[email protected]>
This case was originally here for options.Transport, which no longer exists; and the previous commit has made it impossible for transport == "" to reach this code path. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Use the right variable to make sure transport and image are colon-separated in error reports. Changes user-visible strings. Signed-off-by: Miloslav Trmač <[email protected]>
ResolveName now guarantees that the transport, if it exists, is not a part of the image name; the semantics is no longer ambiguous, so use the value only as expected. This could possibly fix incorrect handling of some strings (pull docker://dir:localpath), and the debug log will no longer contain "error parsing image name %q as given, trying with transport" for every name parsing attempt. Signed-off-by: Miloslav Trmač <[email protected]>
The result of ParseImageName("docker://"...) is not a storageRef.
Should not change behavior.
Signed-off-by: Miloslav Trmač <[email protected]>
pullImage has a dedicated transport: parameter, don't pass the transport in the image name as well. The semantics of the imageName parameter to pullImage is now unambiguous. Should not change behavior, pullImage was trying alltransports.ParseImageName(imageName) first. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change behavior, except possibly failing early if the server returns an invalid tag name. Signed-off-by: Miloslav Trmač <[email protected]>
Both callers now consistently pass the transport in the "transport" parameter, so parsing imageName could only be incorrect. This could possibly fix cases like pulling docker://dir:localpath, and the debug log will no longer say "error parsing image name %q, trying with transport %q: %v" on every pull attempt. Signed-off-by: Miloslav Trmač <[email protected]>
…Reference It should always be redundant with the reference itself; so, use srcRef.StringWithinTransport() in the cases where we do need to understand and hard-code the string syntax, after all. Also improve the oci: format parsing a bit, to be robust against including an image name. NOTE: This might change the semantics a bit because StringWithinTransport does not guarantee preserving the original string (e.g. paths tend to be normalized not to contain symlinks). Using local paths as docker/distribution image names is conceptually so problematic that this seems worth the code cleanup - but I might be wrong. Signed-off-by: Miloslav Trmač <[email protected]>
Right now, we (conceptually unnecesarily) require an image with an existing tag on the remote repository to list all other tags. Given that, use the user-specified name:tag, if any, instead of discarding the tag and requiring :latest to exist on the remote registry. Signed-off-by: Miloslav Trmač <[email protected]>
This only moves the code, does not modify it at all; a separate commit to make review easier. pullImage eventually computes the same value anyway, so this should not change behavior. We will soon remove the redundant value in pullImage. Signed-off-by: Miloslav Trmač <[email protected]>
…llImage etc. Use a typed value, to hopefully decrease further temptation to process strings manually, and to avoid the unnecessary alltransports.ParseImageName which resolveImage has already called. This may change the strings used in some error/debug messages, which now use transports.ImageName instead of the original input; the strings should by definition have the same semantics. Signed-off-by: Miloslav Trmač <[email protected]>
The code is already calling docker.GetRepositoryTags immediately below, so the dependency already exists, and this removes an unnecessary dependency on alltransports. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
|
LGTM |
|
bot, retest this please |
|
Red Hat CI is "Pending" a bit too long. If the retest shows no effect, @mtrmac might need to repush. |
|
Code LGTM |
|
bot, retest this please |
|
Changes LGTM assuming happy tests. I'm a little leery about merging this in before we spin 1.7-1 later today. |
|
I agree. |
|
📌 Commit e9715b1 has been approved by |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Does not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
No need to hard-code the :tag / @digest syntax when there already is an API returning the string representation. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Should not change behavior, except possibly failing early if the server returns an invalid tag name. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Both callers now consistently pass the transport in the "transport" parameter, so parsing imageName could only be incorrect. This could possibly fix cases like pulling docker://dir:localpath, and the debug log will no longer say "error parsing image name %q, trying with transport %q: %v" on every pull attempt. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
…Reference It should always be redundant with the reference itself; so, use srcRef.StringWithinTransport() in the cases where we do need to understand and hard-code the string syntax, after all. Also improve the oci: format parsing a bit, to be robust against including an image name. NOTE: This might change the semantics a bit because StringWithinTransport does not guarantee preserving the original string (e.g. paths tend to be normalized not to contain symlinks). Using local paths as docker/distribution image names is conceptually so problematic that this seems worth the code cleanup - but I might be wrong. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Right now, we (conceptually unnecesarily) require an image with an existing tag on the remote repository to list all other tags. Given that, use the user-specified name:tag, if any, instead of discarding the tag and requiring :latest to exist on the remote registry. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
This only moves the code, does not modify it at all; a separate commit to make review easier. pullImage eventually computes the same value anyway, so this should not change behavior. We will soon remove the redundant value in pullImage. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
…llImage etc. Use a typed value, to hopefully decrease further temptation to process strings manually, and to avoid the unnecessary alltransports.ParseImageName which resolveImage has already called. This may change the strings used in some error/debug messages, which now use transports.ImageName instead of the original input; the strings should by definition have the same semantics. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
The code is already calling docker.GetRepositoryTags immediately below, so the dependency already exists, and this removes an unnecessary dependency on alltransports. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
|
@rh-atomic-bot retry |
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
No need to hard-code the :tag / @digest syntax when there already is an API returning the string representation. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
When ResolveName has already determined that the value is an ID (prefix), and returned the full ID, rely on that knowledge and don't try at all to pull the image from a 'remote transport ""'; also, don't try to match strings that are already known not to be ID prefixes, or that are known to use a different transport, against local storage. Should not change behavior, except possibly in theoretical inconsistency cases when store.Image(knownImageID) fails; the code now does not report other unrelated errors on the transport == "" path below. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
This case was originally here for options.Transport, which no longer exists; and the previous commit has made it impossible for transport == "" to reach this code path. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Use the right variable to make sure transport and image are colon-separated in error reports. Changes user-visible strings. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
ResolveName now guarantees that the transport, if it exists, is not a part of the image name; the semantics is no longer ambiguous, so use the value only as expected. This could possibly fix incorrect handling of some strings (pull docker://dir:localpath), and the debug log will no longer contain "error parsing image name %q as given, trying with transport" for every name parsing attempt. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
The result of ParseImageName("docker://"...) is not a storageRef.
Should not change behavior.
Signed-off-by: Miloslav Trmač <[email protected]>
Closes: #1361
Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
pullImage has a dedicated transport: parameter, don't pass the transport in the image name as well. The semantics of the imageName parameter to pullImage is now unambiguous. Should not change behavior, pullImage was trying alltransports.ParseImageName(imageName) first. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Should not change behavior, except possibly failing early if the server returns an invalid tag name. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Both callers now consistently pass the transport in the "transport" parameter, so parsing imageName could only be incorrect. This could possibly fix cases like pulling docker://dir:localpath, and the debug log will no longer say "error parsing image name %q, trying with transport %q: %v" on every pull attempt. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
…Reference It should always be redundant with the reference itself; so, use srcRef.StringWithinTransport() in the cases where we do need to understand and hard-code the string syntax, after all. Also improve the oci: format parsing a bit, to be robust against including an image name. NOTE: This might change the semantics a bit because StringWithinTransport does not guarantee preserving the original string (e.g. paths tend to be normalized not to contain symlinks). Using local paths as docker/distribution image names is conceptually so problematic that this seems worth the code cleanup - but I might be wrong. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Right now, we (conceptually unnecesarily) require an image with an existing tag on the remote repository to list all other tags. Given that, use the user-specified name:tag, if any, instead of discarding the tag and requiring :latest to exist on the remote registry. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
This only moves the code, does not modify it at all; a separate commit to make review easier. pullImage eventually computes the same value anyway, so this should not change behavior. We will soon remove the redundant value in pullImage. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
…llImage etc. Use a typed value, to hopefully decrease further temptation to process strings manually, and to avoid the unnecessary alltransports.ParseImageName which resolveImage has already called. This may change the strings used in some error/debug messages, which now use transports.ImageName instead of the original input; the strings should by definition have the same semantics. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
rh-atomic-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
The code is already calling docker.GetRepositoryTags immediately below, so the dependency already exists, and this removes an unnecessary dependency on alltransports. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]> Closes: #1361 Approved by: rhatdan
|
☀️ Test successful - status-papr, status-travis |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
As a follow-up to #1319 , this cleans up some of the name handling to remove ambiguities and redundancies when parsing image names, e.g. correcting handling of
docker://dir:localpath.Most users should notice only that the debug log no longer contains various variants of
It would be possible to go even further with this, making
ResolveNamereturn completeImageReferencevalues for the source and destination (and perhaps fixing the differences in how various code paths do / don’t uselocalImageNameForReference), but this seemed like a reasonably compact unit of changes and a good place to stop.See the individual commit messages for more details.
WARNING: To be honest, I did only a single smoke-test that all of this works at all; I’m mostly relying on existing tests to detect bugs.