Skip to content

Conversation

@legionus
Copy link
Contributor

@legionus legionus commented May 19, 2016

Resolves #9491, closes #9284 (hopefully), closes #8596 (hopefully), closes #9122

@pweil- @mfojtik @soltysh @miminar ptal

@legionus legionus force-pushed the docker-registry-v2.4.0 branch from 20ff4e1 to cdc92b8 Compare May 19, 2016 12:17
@miminar
Copy link

miminar commented May 19, 2016

You should bump all the storage driver dependencies as well.

@miminar
Copy link

miminar commented May 19, 2016

Aha, you have them in the vendor/ directory. Not a good place for them. They used to use Godeps dir, didn't they?

}
]
},
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seem unrelated.

@legionus
Copy link
Contributor Author

yes. I made a mistake :(

case *schema1.SignedManifest:
case *schema2.DeserializedManifest:
default:
return "", fmt.Errorf("unrecognized manifest type %T", manifest)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ErrorCodeManifestInvalid?

return distribution.ErrTagUnknown{Tag: tag}
}

if !t.repo.pullthrough {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above, who is authorized to make this call?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the Tag() case. It can be invoked only from imageManifestHandler.DeleteImageManifest(). Not something client can call directly.

Since the docker/distribution has a lot of changes we have to adapt code to use it.

* Adapt code to changes in the API.
* Add support of Manifest Version 2 Schema 2.
@legionus legionus force-pushed the docker-registry-v2.4.0 branch 2 times, most recently from e97eb27 to f655085 Compare June 26, 2016 09:44
@legionus
Copy link
Contributor Author

@miminar @soltysh @smarterclayton I made changes according to your comments. Please review.

@legionus legionus force-pushed the docker-registry-v2.4.0 branch from f655085 to 7ab1b84 Compare June 26, 2016 12:18
@legionus legionus force-pushed the docker-registry-v2.4.0 branch from 7ab1b84 to a536ad1 Compare June 26, 2016 13:34
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to a536ad1

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5435/)

@smarterclayton
Copy link
Contributor

Lgtm [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jun 26, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5437/) (Image: devenv-rhel7_4466)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to a536ad1

@0xmichalis
Copy link
Contributor

🎉

@mfojtik
Copy link
Contributor

mfojtik commented Jun 26, 2016

omg :)

Michal Fojtik

On 26 June 2016 at 23:09:11, Michail Kargakis ([email protected])
wrote:

🎉


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8938 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACsaEnD3z1nHooL0CR1idRu1iiG0e2Pks5qPup3gaJpZM4IiLYZ
.

@legionus
Copy link
Contributor Author

@Kargakis @mfojtik 🍻

@soltysh
Copy link
Contributor

soltysh commented Jun 27, 2016

🎉

@smarterclayton
Copy link
Contributor

smarterclayton commented Jun 28, 2016 via email

@miminar
Copy link

miminar commented Jun 28, 2016

@smarterclayton Here's the the only issue I know of: #9540, PR with fix is #9582. There's also @liggitt's #9593 which I'll integrete into the former fix PR.

@miminar
Copy link

miminar commented Mar 21, 2017 via email

imageConfig, err := b.Get(ctx, deserializedManifest.Config.Digest)
if err != nil {
glog.V(5).Infof("unable to access the image config using digest %q for repository %#v: %#v", d, repository, err)
if isDockerError(err, v2.ErrorCodeManifestUnknown) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legionus @miminar do you know when it can happen? Looks like dead code for me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be v2.ErrorCodeBlobUnknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmage basically, we can't be sure that the blob is there. This code is just error handling the Get method. I think it's better than not to handle the error at all.

@miminar yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

9 participants