-
Notifications
You must be signed in to change notification settings - Fork 557
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
runtime: remove When serialized in JSON, the format MUST adhere to the following pattern
#1178
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -32,7 +32,9 @@ The state of a container includes the following properties: | |||
|
|||
The state MAY include additional properties. | |||
|
|||
When serialized in JSON, the format MUST adhere to the following pattern: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w/o the MUST we lose the normative statement about how the JSON should look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(👋) You mean the fields included? Aren't those already described in the runtimeState section above? Or do you mean the formatting of the JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOWDY! :-)
I mean the format of the JSON. For example, while it seems obvious what it should look like, technically w/o the MUST this JSON could be generated and claim to be compliant:
{
"metadata" {
"ociVersion": "0.2.0",
"id": "oci-container1",
"annotations": {
"myKey": "myValue"
}
},
"bundle": "/containers/redis",
"state": {
"status": "running",
"pid": 4422
}
}
since it still has all of the specified properties. The problem doesn't really show itself in JSON as well as other formats like XML where values could appear as new XML elements or as attributes on XML elements, but I believe the problem still does exist.
Basically, w/o the MUST we're doing "spec by example".
If there is an issue here (which I don't really think there is), we should resolve it by just adding a statement that whitespace isn't significant (per the json spec: https://www.rfc-editor.org/rfc/rfc7159 and https://www.json.org/json-en.html )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe your concern is already covered by https://github.com/opencontainers/runtime-spec/blob/main/schema/state-schema.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the spec said that people MUST adhere to the json schema then probably yes, but it doesn't say that. W/o that normative link there's no requirement to even look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, there's a reason the word "pattern" was used instead of just saying "it MUST adhere to the following".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically w/o the MUST this JSON could be generated and claim to be compliant:
(...) since it still has all of the specified properties.
Hm.. I didn't immediately interpret that definition as "fields may appear anywhere (nested)", and assumed defining them ("The state of a container includes the following properties") there (as "required" for some), should be interpreted as "top level field". Do you think that needs further detailing?
believe your concern is already covered by https://github.com/opencontainers/runtime-spec/blob/main/schema/state-schema.json
We could mention that for a JSON representation, there's a JSON-schema available to verify if the format is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR:
When serialized in JSON, the format MUST adhere to the JSON Schema [`schema/state-schema.json`](schema/state-schema.json).
…he following pattern` The sentence looked like as if it required a specific indentation pattern. Fix issue 1177 Signed-off-by: Akihiro Suda <[email protected]>
d679079
to
72efacb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if this helps removing the ambiguity 👍
LGTM |
The sentence looked like as if it required a specific indentation pattern.
Fix #1177