remove rootscopedkinds from groupmeta#63309
remove rootscopedkinds from groupmeta#63309k8s-github-robot merged 1 commit intokubernetes:masterfrom
Conversation
9787ce9 to
cf3dc39
Compare
|
/uncc @dims |
|
/cc @wenjiaswe @yliaog |
|
@roycaihw: GitHub didn't allow me to request PR reviews from the following users: wenjiaswe. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn 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. |
There was a problem hiding this comment.
defaulting is dangerous... we should require the storage to implement this.
There was a problem hiding this comment.
defaulting is dangerous... we should require the storage to implement this.
I will point out that you'll notice very quickly. Now that I found parents, this may not be so bad. I double check.
There was a problem hiding this comment.
If you do not specify it, you are assumed to be namespace scoped.
that's a dangerous default, and we shouldn't assume either way
There was a problem hiding this comment.
this is going to drift and get weird quick...
There was a problem hiding this comment.
this is going to drift and get weird quick...
It's for tests. Those tests should really be using a very tight version of this. They won't be affected by changes to the actual objects. This isn't used to make contact to a real server.
There was a problem hiding this comment.
is the subresource storage required to implement this any more?
|
I am strongly in favor of having this info provided in exactly one place for a resource, and the REST handler is the logical owner of this information (having it both in type registration and in the REST handler has already led to mismatches (xref #50945, #53030). I would strengthen the requirement for the storage to provide this info rather than assuming it is namespaced |
cf3dc39 to
b3b4f58
Compare
|
Rebased. No defaulting. Added TODO about removing static restmapper usage. |
There was a problem hiding this comment.
update comment to "namespace scoping is defined by the parent resource"?
|
some doc nits, a couple unit test failures, lgtm otherwise |
b3b4f58 to
a63086e
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: deads2k, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a63086e to
8ae6251
Compare
|
New changes are detected. LGTM label has been removed. |
enj
left a comment
There was a problem hiding this comment.
+1 for the change overall, definitely prefer the stores handling this.
| return e.UpdateStrategy.NamespaceScoped() | ||
| } | ||
|
|
||
| panic("programmer error: no CRUD for resource, you're crazy, override NamespaceScoped too") |
| // It is usually provided automatically based on your strategy. | ||
| type Scoper interface { | ||
| // NamespaceScoped returns true if the storage is namespaced | ||
| NamespaceScoped() bool |
There was a problem hiding this comment.
Seems like this just needs to be part of type Storage interface { above? It is required in all cases from what I can tell so why not make it a compile time check?
There was a problem hiding this comment.
Seems like this just needs to be part of type Storage interface { above? It is required in all cases from what I can tell so why not make it a compile time check?
No. Subresource storage doesn't have it.
|
/retest |
1 similar comment
|
/retest |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
builds on #63206
Since, a RESTMapping can only be determined based on a connection to a server, the only thing that needs to know the namespaced-ness of a resource is the code doing the registration. Everything else is derived from that source of truth. This removes the other dangling references and collapses down onto the existing namespaced-ness methods in the strategies backing the stores.
@kubernetes/sig-api-machinery-pr-reviews