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

schema: allow Device.fileMode set to null. #670

Closed
wants to merge 1 commit into from

Conversation

Kxuan
Copy link

@Kxuan Kxuan commented Jan 25, 2017

The fileMode is marked optional in the description
In spec-go, fileMode can be set to null.
However, in the schema, a null type is not allowed for fileMode.

This patch allows fileMode set to null, and consistent with other optional fields.

The `fileMode` is marked optional in the description (https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices)
In spec-go, fileMode can be set to null. (https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L347).
However, in the schema, a null type is not allowed for fileMode.

This patch allows fileMode set to null, and consistent with other optional fields.

Signed-off-by: Zhai ZhaoXuan <zhaizx.fnst@cn.fujitsu.com>
@Kxuan Kxuan force-pushed the optional_fileMode branch from 3f72813 to 1c140fe Compare January 25, 2017 02:09
@wking
Copy link
Contributor

wking commented Jan 25, 2017 via email

@Kxuan
Copy link
Author

Kxuan commented Jan 26, 2017

specs-go has it as an omitempty pointer, so instead of setting it to
null in the JSON, you should not set the property at all. More on
this in #662 and in [1].

@wking ok, but I think we should make the optional fields unified. All of them could accept null or all of them could not accept null.

@wking
Copy link
Contributor

wking commented Jan 26, 2017

I think we should make the optional fields unified. All of them could accept null or all of them could not accept null.

I agree. #662 removes all instances of null from the JSON Schema, after which it will be consistent.

@mikebrow
Copy link
Member

mikebrow commented Feb 2, 2017

So we're closing this PR in lieu of 662 correct?

@Kxuan Kxuan closed this Feb 5, 2017
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.

3 participants