-
Notifications
You must be signed in to change notification settings - Fork 15.1k
Add stub reference for KYAML #52583
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
base: dev-1.35
Are you sure you want to change the base?
Add stub reference for KYAML #52583
Conversation
👷 Deploy Preview for kubernetes-io-vnext-staging processing.
|
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/retitle [WIP] KYAML reference placeholder |
|
/milestone 1.35 |
20c76f6 to
647d12a
Compare
|
CC: @thockin |
soltysh
left a comment
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 few comments
|
@rxinui is it still WIP, or this is ready for finalization? |
5370f0d to
a4520eb
Compare
f8fbca0 to
dd4b9ac
Compare
lmktfy
left a comment
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 feedback
064785e to
6d5f559
Compare
|
The required follow-on work includes checking each of the examples (either with a linter program, or manually) to make sure it really is idiomatic, pretty printed, valid KYAML. |
No problem, will be happy to continue and help!
I have updated the examples to provide working examples. All KYAML manifests have been tested using |
|
|
In kubernetes/enhancements#5295 (comment) @thockin mentioned The test I want to use is: are our examples of KYAML correct enough that people will look at them and not feel misled? (a later PR might want to hint at KYAML as an output format, which as a user you can always consider "correct"; versus KYAML as something that you might want to recommend in code style guidelines etc) If a tool like Helm rendered to KYAML, we don't and I think won't have a conformance test that Helm needs to pass. Except, of course, that the YAML does have to be valid YAML. |
|
I am happy to give this another LGTM unless it has substantial changes since I approved it. |
|
/label tide/merge-method-squash |
soltysh
left a comment
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.
Two nits regarding the links, otherwise this lgtm from sig-cli pov
Signed-off-by: rxinui <[email protected]>
21d1816 to
0eced77
Compare
soltysh
left a comment
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lmktfy, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
lmktfy
left a comment
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.
@rxinui I would like to merge this, but even as a stub, I think accepting it would not make the docs better. I am sorry that I need to cancel the LGTM on this PR but it just doesn't seem correct.
A stub reference page should not materially mislead readers, and this would. Someone looking for a KYAML reference who came across this page as drafted could well build a mental model of KYAML that ends up annoyingly wrong. The accuracy bar is high, I'm afraid (we are fine with incomplete, but we are going to insist on accurate).
You must, must read https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/5295-kyaml/README.md and make sure that you understand it enough to document the format.
If the KEP is wrong – which can happen – we need to point that out to reviewers. If the KEP is correct, what we document must materially match the KEP.
There are some places where the PR doesn't align with the documentation style guide, and I hope you'll fix those too, but if there were only some style issues, I would have approved the PR.
We can still fix it. I hope you will look at the KEP and at example KYAML outputs and at this draft document and work out which of those three is wrong. It does seem to me that at least one of them is.
/lgtm cancel
/retitle Add stub reference for KYAML
@lmktfy give me more time to re-read and correct the kyaml example. For my information, how would you approve whether or not the example given matches KYAML format (more than my current example?) |
|
(I helped out a bit with the design of KYAML, so drawing on what I remember I can glance at an example and see if it looks right - but that probably won't help you). |
Description
As requested here by @lmktfy - this PR
serves as a placeholder andaims to provide KYAML supports reference.Issue
Linked to #52523 (review)