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.

Sign up for GitHub

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

Jump to bottom

schema/config-linux: fix namespace based on runtime spec #555

Closed
wants to merge 1 commit into from
Closed

schema/config-linux: fix namespace based on runtime spec #555

wants to merge 1 commit into from

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

schema/config-linux: fix namespace based on runtime spec
8e59229 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@wking
Copy link
Contributor

wking commented Sep 6, 2016

I don't see a need to support ‘null’ when callers can use an empty
array or leave the property unset. And the Go has:

type Linux struct {
…
Namespaces []Namespace json:"namespaces,omitempty"
…
}

so I expect that will either generate non-empty array or
unset-property JSON.

However, my only concern with allowing ‘null’ is that it makes the
spec and JSON Schema slightly more complicated without providing more
power. If folks are fine making any optional setting nullable, even
if we don't use a pointer for the Go property 1, I'll go along with
it. And in that case, 8e59229 looks good to me.

@Mashimiao
Copy link
Author

Mashimiao commented Sep 7, 2016 •
edited

On 09/07/2016 02:40 AM, W. Trevor King wrote:

ype Linux struct {
…
Namespaces []Namespace json:"namespaces,omitempty"
…
}

so I expect that will either generate non-empty array or
unset-property JSON.

That's right.
But, as schema is used to check json file.
null, [] both can be applied to namespaces in json file and
converted to GO code.
So, I think if setted null to namespaces in json file, we should accept it.

However, my only concern with allowing ‘null’ is that it makes the
spec and JSON Schema slightly more complicated without providing more
power.

I agree.
But I think we should think based on writing json file directly.
If write json file directly, null is an possible case for namespaces.
Unless we appoint it to be invalid.

@wking
Copy link
Contributor

wking commented Sep 7, 2016

On Tue, Sep 06, 2016 at 10:46:43PM -0700, Ma Shimiao wrote:

But I think we should think based on writing json file directly. If
write json file directly, null is an possible case for namespaces.
Unless we appoint it to be invalid.

Right. I'm saying I prefer making making ‘null’ invalid, because in
the namespaces case there's nothing you say with ‘null’ that you
aren't saying with ‘[]’. Folks who want to say “use all the host
namespaces” can write ‘[]’. Also allowing them to write ‘null’
doesn't matter to Go consumers, but makes the JSON Schema slightly
more complicated (this PR) and may also make non-Go implementations
slightly more complicated.

On the other hand, those are small slightlies, so I'm ok with allowing
‘null’ if there is a need to cater to folks who want two ways to say
the same thing ;).

@Mashimiao
Copy link
Author

ping @opencontainers/runtime-spec-maintainers
any comments?

@dqminh
Copy link
Contributor

dqminh commented Jan 11, 2017

I agree with not allowing null for an array.

@mrunalp
Copy link
Contributor

mrunalp commented Jan 11, 2017

We discussed this on the call and decided that having null doesn't make sense.

@mrunalp mrunalp closed this Jan 11, 2017
This was referenced Jan 12, 2017
Merged
Merged
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jan 19, 2017
@wking
schema: Drop pointers and nulls
b554d2e 
Maintainers feel (and I agree) that there's no point in explicitly
allowing a null value when callers can simply leave the property unset
[1].  This commit removes all references to "pointer" and "null" from
the JSON Schema to support that decision.  While optional properties
may sometimes be represented as pointer types in Go [2], optional
properties should be represented in JSON Schema by not including the
properties in the 'required' array.

[1]: opencontainers#555 (comment)
[2]: style.md "Optional settings should not have pointer Go types"

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request Jan 19, 2017
Merged
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jan 23, 2017
@wking
schema: Drop pointers and nulls
0927437 
Maintainers feel (and I agree) that there's no point in explicitly
allowing a null value when callers can simply leave the property unset
[1].  This commit removes all references to "pointer" and "null" from
the JSON Schema to support that decision.  While optional properties
may sometimes be represented as pointer types in Go [2], optional
properties should be represented in JSON Schema by not including the
properties in the 'required' array.

[1]: opencontainers#555 (comment)
[2]: style.md "Optional settings should not have pointer Go types"

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request Jan 25, 2017
Closed
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request May 11, 2017
@wking
config-linux: Remove explicit 'null' from device cgroup values
f070429 
Catch the Markdown spec up with the JSON Schema change in 0927437
(schema: Drop pointers and nulls, 2017-01-18, opencontainers#662).  The Markdown is
canonical, so we could restore the explicit-null handling to the JSON
Schema instead, but the maintainers feel (and I agree) that there's no
point in explicitly allowing a null value when callers can simply
leave the property unset [1].

[1]: opencontainers#555 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request May 11, 2017
@wking
config-linux: Remove explicit 'null' from device cgroup values
10ab597 
Catch the Markdown spec up with the JSON Schema change in 0927437
(schema: Drop pointers and nulls, 2017-01-18, opencontainers#662).  The Markdown is
canonical, so we could restore the explicit-null handling to the JSON
Schema instead, but the maintainers feel (and I agree) that there's no
point in explicitly allowing a null value when callers can simply
leave the property unset [1].

[1]: opencontainers#555 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request May 22, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Mashimiao @wking @dqminh @mrunalp