-
Notifications
You must be signed in to change notification settings - Fork 425
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
⚠️ Strip comments from CRD descriptions #877
Conversation
Hi @tsaarni. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @tsaarni! |
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.
For my use this is lgtm
Since it discriminates between code blocks and "normal" |
LGTM label has been added. Git tree hash: 037e0f10c0dbb0835758444fadb42fe99470ea1c
|
/assign @vincepri |
Hi @vincepri, If you have a moment, could you please take a look? Your feedback would be greatly appreciated 🙏 |
/ok-to-test |
pkg/markers/zip.go
Outdated
// If we are not inside markdown code black, follow the Kubernetes formatting conventions: | ||
// - Lines after --- are comments and should be ignored. |
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 we are not inside markdown code black, follow the Kubernetes formatting conventions: | |
// - Lines after --- are comments and should be ignored. | |
// If we are not inside markdown code block, follow the Kubernetes formatting conventions: | |
// - Lines after --- are comments and should be ignored. |
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.
Fixed!
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.
One minor comment, otherwise lgtm!
Kubernetes has some conventions for type descriptions: - Line that only contains --- separates the description from comments. The leading lines are part of public documentation, the trailing lines are internal instructions such as code examples etc. - Line that begins with TODO are internal notes for implementers, and not part of public documentation of the type. This change strips above informational text away from the generated CRDs while keeping them if they appear inside markdown fenced code block. Signed-off-by: Tero Saarni <tero.saarni@est.tech>
/lgtm |
LGTM label has been added. Git tree hash: 1ba4d5d7f9a835609602cfaf6af71fc535e56c34
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai, tsaarni, vincepri 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 |
This PR strips comments from the generated CRDs for clarity of the public API documentation.
The change aligns controller-tools with Kubernetes conventions for type descriptions:
---
separates the API description from comments. The leading lines are part of public documentation, the trailing lines are internal instructions such as code examples etc. As an example, see widely used typeCondition
that includes some example code that should not be considered as part of API documentation.LocalObjectReference
for example.Background information
When Kubernetes itself generates OpenAPI docs for its internal resources, it will strip the implementation related comments from the go docs. I believe it is done by this code.
However with controller-tools the implementation comments "leak" to public documentation. This causes confusion and usability issues since the users have no context to understand the implementation comments.
Google search for "Represents the observations of a foo's current state" shows that the problem is wide spread.
Limited compatibility with markdown formatting
I've marked the PR as⚠️ breaking with following reasoning:
While I first thought it is very unlikely to bump into
---
in type's documentation, I'm proven wrong by #870 which shows that sometimes markdown formatting is used inside go docs.kubectl explain
will not understand how to format markdown, but the reasoning for markdown inside go docs might come from OpenAPI spec which supports "rich text formatting" with CommonMark 0.27There is a limitation: There is no reliable way to know if go doc is in plain text or markdown, therefore
---
could mean comment separator (according to Kubernetes convention) or alternatively thematic break or heading (according to markdown). The implementation takes an approach where---
inside markdown fenced code blocks is kept, but when seen in plain, it is considered as comment separator and rest of the doc is stripped. This might be wrong assumption in some corner cases. As result the description in the generated CRD will be cut short.Fixes #649, #728, #875