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

Prefer digest over tag on pull #33214

Merged
merged 1 commit into from May 16, 2017

Conversation

Projects
None yet
6 participants
@aaronlehmann
Contributor

aaronlehmann commented May 16, 2017

If a reference passed to the pull code contains both a tag and a digest, currently the tag is used instead of the digest in the request to the registry. This is the wrong behavior. Change it to favor the digest.

Fixes #33165

cc @dmcgowan

Prefer digest over tag on pull
If a reference passed to the pull code contains both a tag and a digest,
currently the tag is used instead of the digest in the request to the
registry. This is the wrong behavior. Change it to favor the digest.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

@aaronlehmann aaronlehmann added this to the 17.06.0 milestone May 16, 2017

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan May 16, 2017

Member

I don't believe this code was originally considering the case where both a tag and digest are given, I believe it was impossible when it was originally written. Is it still adding the tag when both are given? I agree this is the correct ordering but want to make sure the rest of the code is handling it properly. What code paths currently allow passing in both?

Member

dmcgowan commented May 16, 2017

I don't believe this code was originally considering the case where both a tag and digest are given, I believe it was impossible when it was originally written. Is it still adding the tag when both are given? I agree this is the correct ordering but want to make sure the rest of the code is handling it properly. What code paths currently allow passing in both?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 16, 2017

Contributor

I don't believe this code was originally considering the case where both a tag and digest are given, I believe it was impossible when it was originally written. Is it still adding the tag when both are given?

No, it stores it under the digest in this case. I think this is the correct behavior, because we don't want to overwrite the tag with an older version that we pulled by digest.

What code paths currently allow passing in both?

build

Contributor

aaronlehmann commented May 16, 2017

I don't believe this code was originally considering the case where both a tag and digest are given, I believe it was impossible when it was originally written. Is it still adding the tag when both are given?

No, it stores it under the digest in this case. I think this is the correct behavior, because we don't want to overwrite the tag with an older version that we pulled by digest.

What code paths currently allow passing in both?

build

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi May 16, 2017

Member

LGTM

@aaronlehmann I think v17.03 should be fine as that still used the old reference pkg?

Member

tonistiigi commented May 16, 2017

LGTM

@aaronlehmann I think v17.03 should be fine as that still used the old reference pkg?

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann May 16, 2017

Contributor

Yes, I believe the reference package change happened after 17.03.

Contributor

aaronlehmann commented May 16, 2017

Yes, I believe the reference package change happened after 17.03.

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan May 16, 2017

Member

LGTM

Member

dmcgowan commented May 16, 2017

LGTM

@thaJeztah

LGTM

for a follow up; should we extract this part to a separate function so that we can have unit tests?

@tonistiigi tonistiigi merged commit c93a48e into moby:master May 16, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 34159 has succeeded
Details
janky Jenkins build Docker-PRs 42771 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 3156 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 13993 has succeeded
Details
z Jenkins build Docker-PRs-s390x 2872 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment