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

validate/validate_test: Add linux.rootfsPropagation checks #476

Merged

Conversation

wking
Copy link
Contributor

@wking wking commented Sep 14, 2017

To demonstrate that this is covered by the 1.0.0 JSON Schema checks, and show that the omitempty protects us from errors for empty-string values despite there being no empty-string entry in the JSON Schema.

These tests are in support for #473.

@wking wking force-pushed the json-schema-rootfs-propagation-test branch from e149fb6 to c66321d Compare September 14, 2017 17:05
To demonstrate that this is covered by the 1.0.0 JSON Schema checks,
and show that the omitempty protects us from errors for empty-string
values despite there being no empty-string entry in [1].

[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/schema/defs-linux.json#L4-L11

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking force-pushed the json-schema-rootfs-propagation-test branch from c66321d to 2cbb341 Compare September 14, 2017 21:21
@Mashimiao Mashimiao added this to the v0.2.0 milestone Sep 15, 2017
config: &rspec.Spec{
Version: "1.0.0",
Linux: &rspec.Linux{
RootfsPropagation: "",

Choose a reason for hiding this comment

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

"" is not valid value for RootfsPropagation why error is ""?

Copy link
Contributor Author

@wking wking Sep 15, 2017

Choose a reason for hiding this comment

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

"" is not valid value for RootfsPropagation why error is ""?

I expect because it's omitempty, although I haven't tracked that down to specific gojsonschema code.

Choose a reason for hiding this comment

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

I think omitempty means it can be unset but not ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think omitempty means it can be unset but not ""

For a non-pointer omitempty like RootfsPropagation, an empty string is as close as you can get to unset.

Choose a reason for hiding this comment

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

I think the RootfsPropagation: "" test should be deleted.

Copy link
Contributor Author

@wking wking Sep 18, 2017

Choose a reason for hiding this comment

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

I think the RootfsPropagation: "" test should be deleted.

Why? With the current non-pointer property, that's how you explicitly unset the property. I think we need to be sure clearing RootfsPropagation is legal, because as this thread demonstrates, it's not immediately obvious.

Choose a reason for hiding this comment

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

If it is not set, its value is also "", which is repeated with the previous test (without setting the value), and I think the test of this value is meaningless.

Copy link
Contributor Author

@wking wking Sep 18, 2017

Choose a reason for hiding this comment

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

If it is not set, its value is also "", which is repeated with the previous test (without setting the value)...

Yeah, but it doesn't hurt to double check, and the explicit-empty-string case surprised @Mashimiao at first. If test rephrasings help expose surprises, I think we want them.

@Mashimiao
Copy link

Mashimiao commented Sep 20, 2017

LGTM

Approved with PullApprove

1 similar comment
@liangchenye
Copy link
Member

liangchenye commented Sep 22, 2017

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit bd16d0d into opencontainers:master Sep 22, 2017
@wking wking deleted the json-schema-rootfs-propagation-test branch September 22, 2017 17:35
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.

None yet

4 participants