-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Clarifying API Conventions regarding Kind and Resource in object refs #5973
Clarifying API Conventions regarding Kind and Resource in object refs #5973
Conversation
@robscott: GitHub didn't allow me to request PR reviews from the following users: toumorokoshi. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. 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/test-infra repository. |
|
||
If implementations of an object reference will always have a clear way to map | ||
kinds to resources, it is acceptable to use "kind" in the object reference. | ||
Although less precise than "resource", it is more familiar to many Kubernetes |
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.
Would probably prefer to describe a 1:1 relationship between a particular object and the canonical resource here, vs this more generally vague sentence.
Typically all objects in Kubernetes have a canonical primary resource - such as “pods” representing the way to create and delete resources of the “Pod” schema. While it is possible a resource schema cannot be directly created, such as a “Scale” object which is only used within the “scale” subresource of a number of workloads, most object references address the primary resource via its schema.
Or similar
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.
Thanks for the suggestion @smarterclayton! I've added this in and removed the sentence referenced here, but not completely sure I added this content in the right place. Let me know if you had a different idea for where/how it could fit in.
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 like Clayton's suggestion, but otherwise this feels like a pretty decent edit.
/assign |
/assign lavalamp |
8ad0909
to
c4641de
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.
Overall looks like a good step! I left a couple comments.
In particular I think we need to more clearly express the exact conditions where a kind-resource mapping is actually safe to use, since unfortunately it'll be easy to do incorrectly until changes / additions are made to some core K8S libraries.
“pods” representing the way to create and delete resources of the “Pod” schema. | ||
While it is possible a resource schema cannot be directly created, such as a | ||
“Scale” object which is only used within the “scale” subresource of a number of | ||
workloads, most object references address the primary resource via its schema |
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.
workloads, most object references address the primary resource via its schema | |
workloads, most object references address the primary resource via its kind |
For the sake of clarity, could you just add a separate sentence clarifying that "kind" is a schema, and then use "kind" in further sentences?
I think it's a little confusing to say things like "object references address the resource via it's schema": it introduces a third term, and the user may not know if it's a kubernetes-specific term or a general one (in this case it's the latter).
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 added a sentence to clarify this relationship. I'm not an expert on the terminology here, but the original uses of "schema" were added as a suggestion from @smarterclayton. I likely didn't merge that in quite as cleanly as I could have, so any suggestions are very appreciated.
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 newest changes look good. My suggestion to clarify kind vs resource is just to add a sentence at the top clarifying that "kind" refers to a schema, while resource refers to a collection of objects, sharing a schema, grouped under an API path.
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.
It may just be my lack of knowledge here, but I think "resource refers to a collection of objects, sharing a schema, grouped under an API path" actually confuses me more than the current text. I think this may be more accurate, but also potentially more information than is required in this context.
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.
sure, I can see that. There's possibly better wording here, but if I saw this PR without some of this context I would imagine I'd ask myself what the difference between kind vs resource actually is. I hope there's some succinct way to clarify that without introducing a third term or overcomplicating.
c4641de
to
28b67f1
Compare
/approve /hold To give @deads2k a chance to look, if he wants. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lavalamp, robscott 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 |
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.
clearing my requested changes. Left another comment but it's non-blocking.
28b67f1
to
8387703
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.
Good clarification!
This clarification looks very good. Thank you. /hold cancel |
/lgtm |
This is a follow up to the earlier sig-arch/apimachinery thread and sig-arch meeting today. I hope I've represented the conclusion of that meeting reasonably accurately here.
/cc @deads2k @lavalamp @liggitt @smarterclayton @thockin @toumorokoshi @youngnick