-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
SSA request bodies can include either JSON or YAML #50116
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
All JSON messages are valid YAML. Some clients specify Server-Side Apply requests using JSON | ||
request bodies that are also valid YAML. |
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.
application/apply-patch+yaml
is always formally YAML, not JSON.
This change doesn't convey the nuance originally intended. You are welcome to write something new, but we shouldn't make the swap you're proposing here.
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.
JSON is YAML, but the reverse is not true. The sentence in the documentation currently states "Some clients specify Server-Side Apply requests using YAML request bodies that are also valid JSON", which is not true and the exact opposite. You can provide JSON (in addition to YAML) request bodies because JSON is valid YAML.
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.
A better wording might be:
All JSON messages are valid YAML. Therefore, in addition to using YAML request bodies for Server-Side Apply requests, you can also use JSON request bodies, as they are also valid YAML.
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.
All JSON messages are valid YAML. Therefore, in addition to using YAML request bodies for Server-Side Apply requests, you can also use JSON request bodies, as they are also valid YAML.
You could try that. Actually, the existing text is factually wrong. JSON allows tabs as whitespace; YAML does not. Something to bear in mind.
You can convert all JSON to valid YAML by replacing all ASCII tab characters with the appropriate number of space (0x20
) characters. If you do that, the resulting YAML will also be valid 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.
YAML is a superset of JSON, and any compliant YAML parser should be able to parse JSON. There’s no need to convert a JSON document before passing it to a YAML parser.
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.
Can you cite a source for that assertion [that the existing docs are correct and that YAML is a pure superset of JSON]? I checked against the official grammars.
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.
The documentation is incorrect, as it states the opposite. SSA request bodies (application/apply-patch+yaml
content type) are expected to be in YAML. However, since a YAML parser can also parse JSON (YAML is a superset of JSON), you can use JSON as well as YAML for request bodies.
The existing documentation states:
Some clients specify Server-Side Apply requests using YAML request bodies that are also valid JSON.
This should be corrected to:
Some clients specify Server-Side Apply requests using JSON request bodies that are also valid YAML.
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 think no one is wrong or right here, but if there's confusion, we should probably update it.
The existing documentation states:
Some clients specify Server-Side Apply requests using YAML request bodies that are also valid JSON.
I don't think that's wrong, I could slightly rephrase as:
Some clients specify Server-Side Apply requests using YAML request bodies, and they happen to also be valid JSON (which they can be), but that's irrelevant, since it's still YAML at the end of the day. So in other word, however you format your YAML, in the form of json or not, it doesn't matter, as long as it's yaml.
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.
Some clients specify Server-Side Apply requests using YAML request bodies that are also valid JSON.
From this statement, it can be inferred that all YAML are valid JSON. At least, this is my initial interpretation. While this is true for some YAML documents, it is not true for all YAML. Furthermore, since the content type for SSA requests is application/apply-patch+yaml
, the request body is expected to be in YAML. Given that YAML is a superset of JSON, I thought there was a typo in the statement and you meant to say that some clients use JSON for request bodies, which is also valid YAML.
/hold I have a concern about technical accuracy. Let's get a tech review from SIG API Machinery before unholding. |
According to the metadata, that's YAML. I don't see how it's more correct to say that it is JSON; it is YAML that is also valid JSON. Another analogy: think of a rational number under 1000000, and then divide it by 3. |
Not all YAML are JSON, but all JSON are YAML |
|
In response to the comment, I stated that YAML is a superset of JSON, meaning any YAML parser should be able of parsing JSON, even if it includes tab characters. I also referenced the YAML specification to support this point. However, it seems that you do not accept that YAML is a superset of JSON. To clarify, here is a simple Go app that demonstrates a YAML parser successfully parsing JSON with tab characters:
|
Ah, you know what? I'm out of date. YAML 1.2.2 does have full JSON compatibility. Great. The existing text is fine and doesn't need to be changed in order to become correct. We can make a different change to what you've proposed @MohammadAlavi1986 and a rewording to clarify is welcome, but I really don't support what you're proposing here. In my considered opinion, it would confuse more people than it would help. |
what is the confusion in the following sentence? |
SSA request bodies can include either JSON or YAML
Since JSON messages are valid YAML, the request body for a server-side apply request (
application/apply-patch+yaml
content type) can be in either JSON or YAML.Example: