Refactor godocs for API fields to start with the serialised versions #11238
Assignees
Labels
help wanted
Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
kind/cleanup
Categorizes issue or PR as related to cleaning up code, process, or technical debt.
priority/backlog
Higher priority than priority/awaiting-more-evidence.
triage/accepted
Indicates an issue or PR is ready to be actively worked on.
Comments
|
@chrischdi: The label(s) In response to this:
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-sigs/prow repository. |
k8s-ci-robot
added
needs-priority
chrischdi
changed the title
|
Sounds good to me! (cc @vincepri @fabriziopandini @JoelSpeed) /kind cleanup |
|
@sbueringer: GuidelinesPlease ensure that the issue body includes answers to the following questions:
For more details on the requirements of such an issue, please see here and ensure that they are met. If this request no longer meets these requirements, the label can be removed In response to this:
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-sigs/prow repository. |
k8s-ci-robot
added
kind/cleanup
|
/assign |
|
I couldn't find an existing linter/formatter for this. I'm not sure it can be maintained in cluster-api project or not. |
|
q: is this something for the current API or it is part of the work for v1beta2 API? |
|
No strong opinion because its only about the godoc comment. I'd like to ensure we follow it for new fields, for old ones best-effort I'd say. |
|
so should the linter be enforced to files only in |
|
Let's do it everywhere. Changing one character in a field description is not a breaking change in any way The v1beta2 work is already so much that we have to scope it down because we can't get everything done in a release cycle. Let's not defer further work to v1beta2 if not strictly necessary |
|
Let me clarity the scope of this issue. Fields not starting with field names like below should be fixed? cluster-api/api/v1beta1/cluster_types.go Lines 103 to 104 in 48ee02a Then, I'll prepare to fix such godocs. |
|
not a fan as i commented on the PR: |
|
cc @JoelSpeed ^^ |
|
Left a comment on the PR in response to @neolit123 Some additional thoughts, changing and improving documentation is not a breaking change as has been mentioned. We have docs that current don't even start with the field name, can we fix those as well? We probably want all references to fields to use the serialised versions when in the godocs on the fields themselves (these are the ones that end up in the generated CRD), so this may also mean changing some instances mid sentence as well |
|
Even after #11273, the number of missing or unnormalized comments is still over 400.
An example of Other is: cluster-api/version/version.go Lines 69 to 77 in 0f8e172 We need to fix Types at least because they'll appear in the descriptions of CRD. Then, I'd like to start from Full list |
Do they? Are you meaning the definitions on the structs themselves? I didn't think they typically ended up in the generated schema. Especially for things like type aliases where we use a string alias to apply validation to items within a list, or as values for an enum |
The numbers above are about fields of structs. Type aliases and enums aren't counted. |
|
@neolit123 Are you okay with making this change given comments above? |
|
not exactly, but don't mind me as there seems to be agreement for a way forward. there was a similar proposal coming from SIG docs about component config APIs (e.g. kubelet, kubeadm) but it was denied and the API field comments remained PascalCase. |
|
Hm that's actually an interesting alternative. I guess the limitation is that this works only well for the first word of the godoc comment? (not the ones mid-sentence) |
why? it can replace it as a pattern everywhere in a field comment. |
|
So basically there is a search and replace over all comments of all fields which replace all field names with camelCase? There are no accidental / unintentional replacements with this approach? |
|
actually checking https://kubernetes.io/docs/reference/config-api/kubelet-config.v1beta1/ seems like the comments are not processed at all so there is the inconsistency of camelCase and PascalCase...so seems like i though it was added as a feature, probably due to prior discussions, but it's not. the generator is at https://github.com/kubernetes-sigs/reference-docs/tree/91e1ff6d9599f32de2d748e7cd7ff7ffc1be8bb2/genref |
|
I'm sorry for the delayed update. I'd like to make it clear what the scope and detailed tasks of this issue are.
Considering to introduce a linter, I think that it's better to have the common rule across the repository. so I'm fixing all comments even for non-custom resources. |
|
I recently introduced a linter to openshifts API repos that enforces comments start with the serialised version of the name, and it also has an auto fix flag that can easily replace where the first word only differs by casing. Where it does not start with the same word, manual fixes are required. I'm working on this linter in https://github.com/JoelSpeed/kal, and after discussion at the sig apimachinery call last week, plan to submit a KEP for inclusion of this project as a kubernetes-sigs repo in the future. Before we write any more linters, perhaps let's try extend this one so we can help the wider community too? We could try running this linter auto-fixing against Cluster API types if we think that's helpful? WDYT? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What would you like to be added (User Story)?
Godocs for api types in core k8s normally start with the serialised version for better readability in e.g.
kubectl describe.Example:
Source: https://github.com/kubernetes/api/blob/master/core/v1/types.go#L60-L63
This issue tracks refactoring all relevant godocs of our API types.
Maybe there could also be a linter to enforce this.
Detailed Description
[0]
Anything else you would like to add?
xref: Discussion at #11166 (comment)
Label(s) to be applied
/kind api
The text was updated successfully, but these errors were encountered: