Skip to content
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

Modify the test's config type #510

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

zhouhao3
Copy link

When I execute the test command, I get the following warning, so I made the following changes.
$ go test
warning: config sha256:5359a4f250650c20227055957e353e8f8a74152f35fe36f00b6b1f9fc19c8861 has an unknown media type: application/octet-stream

Signed-off-by: zhouhao [email protected]

@@ -136,14 +136,14 @@ func TestBackwardsCompatibilityManifest(t *testing.T) {
//
// curl -L -H "Authorization: Bearer ..." -H \
// "Accept: application/vnd.docker.distribution.manifest.v2+json" \
// https://registry-1.docker.io/v2/library/docker/manifests/sha256:888206c77cd2811ec47e752ba291e5b7734e3ef137dfd222daadaca39a9f17bc
// https://registry-1.docker.io/v2/library/docker/manifests/sha256:d901d5244c94db8151415bf3a1d3a0b10e7468e4997786b01c287cbcbaeca446
Copy link
Contributor

@wking wking Jan 3, 2017

Choose a reason for hiding this comment

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

Have you tested this? I expect the curl call will not succeed. I think the right solution to this problem is to make these child-media-type checks optional, with something like a strict option. More background in opencontainers/image-tools#66, #403, and #452.

Copy link
Author

Choose a reason for hiding this comment

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

I have tested the changes and the results are correct:

go test
PASS
ok _/home/zhouhao/image-spec/schema 0.033s

Copy link
Contributor

@wking wking Jan 4, 2017

Choose a reason for hiding this comment

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

On Tue, Jan 03, 2017 at 05:00:57PM -0800, Zhou Hao wrote:

go test
PASS

I agree that the tests pass with your 3607393, but the point of these tests is to show that the current OCI spec is compatible with existing Docker-hub blob instances with minimal media type replacements. If you change the Docker-hub blobs locally, you break that. Using the token fetching comment from #370:

$ TOKEN=$(curl https://auth.docker.io/token\?service\=registry.docker.io\&scope\=repository:library/docker:pull  | jq -r .token)
$ curl -sILH "Authorization: Bearer ${TOKEN}" -H "Accept: application/vnd.docker.distribution.manifest.v2+json" https://registry-1.docker.io/v2/library/docker/manifests/sha256:888206c77cd2811ec47e752ba291e5b7734e3ef137dfd222daadaca39a9f17bc | head -n1
HTTP/1.1 200 OK
$ curl -sILH "Authorization: Bearer ${TOKEN}" -H "Accept: application/vnd.docker.distribution.manifest.v2+json" https://registry-1.docker.io/v2/library/docker/manifests/sha256:d901d5244c94db8151415bf3a1d3a0b10e7468e4997786b01c287cbcbaeca446 | head -n1
HTTP/1.1 404 Not Found

The tests in this backwards-compat file need to be blobs you can successfully pull from the Docker hub, not ones that 404.

Copy link
Author

Choose a reason for hiding this comment

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

Why do I enter the results of both cases are HTTP/1.1 200 Connection established

Copy link
Contributor

Choose a reason for hiding this comment

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

The "Connection established" line is probably from a proxy. You'll see the 404 if you drop the head calls.

Copy link
Author

Choose a reason for hiding this comment

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

However, when the modified mediatype, we must then modify the digest. This will resolve the warning

Copy link
Contributor

Choose a reason for hiding this comment

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

However, when the modified mediatype, we must then modify the digest. This will resolve the warning

To satisfy the spirit of these compat tests, you cannot modify the source content (or its digest) at all. The point is that these examples are caught-in-the-wild blobs, and that after flushing that wild-caught content through convertFormats you get a valid OCI blob. The compatibility claim will be less convincing if it becomes “we edit the wild blob by hand and then flush it through convertFormats so we're compatible” ;).

And you do get a valid OCI blob (because we only SHOULD application/vnd.oci.image.config.v1+json for a manifest's config descriptor). The “has an unknown media type” error you get is because the current tests are stricter than the spec and provide no way to select between “MUST all compliant handlers support this blob?” and “is this blob valid?”.

Copy link
Author

Choose a reason for hiding this comment

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

thanks,updated.

@philips
Copy link
Contributor

philips commented Jan 18, 2017

LGTM

Approved with PullApprove

1 similar comment
@vbatts
Copy link
Member

vbatts commented Jan 18, 2017

LGTM

Approved with PullApprove

@vbatts vbatts merged commit ea63d33 into opencontainers:master Jan 18, 2017
@zhouhao3 zhouhao3 deleted the backwards-config branch January 19, 2017 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants