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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 ignore namespace key when getting cluster-scoped resources #1326
馃悰 ignore namespace key when getting cluster-scoped resources #1326
Conversation
/retest |
By("listing all namespaces - should still be able to get a cluster-scoped resource") | ||
namespaceList := &unstructured.UnstructuredList{} | ||
namespaceList.SetGroupVersionKind(schema.GroupVersionKind{ | ||
By("listing all nodes - should still be able to list a cluster-scoped resource") |
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.
Why did you change this from Namespace to Node?
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.
Mainly to avoid confusion of trying to explain "setting a namespace key on a namespace"
pkg/cache/cache_test.go
Outdated
namespaceList := &unstructured.UnstructuredList{} | ||
namespaceList.SetGroupVersionKind(schema.GroupVersionKind{ | ||
By("listing all nodes - should still be able to list a cluster-scoped resource") | ||
nodeList := &kmetav1.PartialObjectMetadataList{} |
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.
This needs to remain an unstructured.UnstructuredList, otherwise the two testcases are identical
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 catch! That was an accidental copy/paste error.
90d071d
to
f6f9ff4
Compare
f6f9ff4
to
2c0bd8f
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, joelanford 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 makes the controller-runtime cache consistent with the behavior of the client when working with cluster-scoped resources.
Closes #1323
Current behavior:
When fetching a cluster-scoped resource using a direct client (e.g.
mgr.GetAPIReader
), the namespace in the object key is ignored. When fetching a cluster-scoped resource via the cache, the namespace key MUST be set to an empty string. Otherwise the cache will return a not found error.Expected behavior:
When fetching a cluster-scoped resource via the cache, the namespace key is ignored, just like with a direct client. The APIReader and Cache behavior should be consistent.