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

config.go:Drop local rootfs.type validation #85

Merged
merged 1 commit into from
Nov 18, 2016

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Nov 17, 2016

In this type of authentication is not required, because the json file has been identified type value ,if not the case, it will never perform type validation.

Signed-off-by: zhouhao [email protected]

@wking
Copy link
Contributor

wking commented Nov 17, 2016

On Thu, Nov 17, 2016 at 12:09:44AM -0800, Zhou Hao wrote:

  • config.go:Changes the verifying position of layers

This is checked in the JSON Schema since
opencontainers/image-spec#358, so I'd rather just drop the local check
entirely. The current, generic JSON-Schema-failure:

errors.Wrapf(err, "%s: config validation failed", path)

will give you a message about the invalid rootfs.type without us
needing special-case JSON-Schema-failure handling for this situation.

@zhouhao3
Copy link
Author

This is checked in the JSON Schema since opencontainers/image-spec#358, so I'd rather just drop the local check entirely. The current, generic JSON-Schema-failure:

I think if the type validation fails or need a separate error message, because in accordance with the provisions of the meaning, the error message should be more detailed.

@wking
Copy link
Contributor

wking commented Nov 18, 2016

On Thu, Nov 17, 2016 at 05:38:49PM -0800, Zhou Hao wrote:

I think if the type validation fails or need a separate error
message, because in accordance with the provisions of the
meaning
,
the error message should be more detailed.

The JSON Schema error from the current master looks like:

$ git checkout origin/master
$ git describe --always
5702049
$ make tools
$ cat >config.json <<EOF
{
"rootfs": {
"type": "something-else",
"diff_ids": []
}
}
EOF
$ ./oci-image-validate --type config config.json
config.json: validation failed: rootfs.type: rootfs.type must be one of the following: "layers"

That's pretty specific. It doesn't link to the relevant spec line,
but then neither does your patch. And the JSON Schema message is
invoked regardless of how we find the config, while the line you're
shifting here only fires when you're fetching the config from a walker
(my oci-image-validate line above is getting the file directly,
without going through a walker).

@zhouhao3
Copy link
Author

$ ./oci-image-validate --type config config.json
config.json: validation failed: rootfs.type: rootfs.type must be one of the following: "layers"

So to say, this type of authentication is not necessary

@wking
Copy link
Contributor

wking commented Nov 18, 2016 via email

@zhouhao3 zhouhao3 changed the title config.go:Changes the verifying position of type config.go:Drop local rootfs.type validation Nov 18, 2016
@coolljt0725
Copy link
Member

coolljt0725 commented Nov 18, 2016

LGTM we could drop local rootfs.type validation since it has been done in image-spec schema

Approved with PullApprove

@xiekeyang
Copy link
Contributor

xiekeyang commented Nov 18, 2016

LGTM

Approved with PullApprove

@xiekeyang xiekeyang merged commit b082914 into opencontainers:master Nov 18, 2016
@zhouhao3 zhouhao3 deleted the test-layers branch December 6, 2016 06:21
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