Update k8s dependencies to v0.20.7 #1344
Conversation
knative-prow-robot
added
the
approved
There was a problem hiding this comment.
@dsimansk: 1 warning.
In response to this:
Description
Update K8s dependencies to follow-up on knative/pkg#2145.
Changes
- Update k8s version
- Fix dynamic client tests
Reference
Fixes #
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.
pkg/dynamic/fake/fake.go
Outdated
| @@ -26,6 +26,19 @@ import ( | |||
| servingv1 "knative.dev/serving/pkg/apis/serving/v1" | |||
| ) | |||
|
|
|||
| var GvrToListKind = map[schema.GroupVersionResource]string{ | |||
There was a problem hiding this comment.
Golint comments: exported var GvrToListKind should have comment or be unexported. More info.
knative-prow-robot
added
the
size/XXL
Codecov Report
@@ Coverage Diff @@
## main #1344 +/- ##
==========================================
- Coverage 72.97% 72.89% -0.08%
==========================================
Files 160 160
Lines 8151 8151
==========================================
- Hits 5948 5942 -6
- Misses 1476 1480 +4
- Partials 727 729 +2
Continue to review full report at Codecov.
|
|
/test pull-knative-client-build-tests |
1 similar comment
|
/test pull-knative-client-build-tests |
|
/cc @dprotaso |
| {Group: "eventing.knative.dev", Version: "v1", Resource: "channels"}: "ChannelList", | ||
| {Group: "messaging.knative.dev", Version: "v1", Resource: "inmemorychannels"}: "InMemoryChannelsList", | ||
| } | ||
|
|
||
| // createFakeKnDynamicClient gives you a dynamic client for testing containing the given objects. | ||
| // See also the one in the fake package. Duplicated here to avoid a dependency loop. | ||
| func createFakeKnDynamicClient(testNamespace string, objects ...runtime.Object) KnDynamicClient { | ||
| scheme := runtime.NewScheme() |
There was a problem hiding this comment.
You could technically drop the need for the gvrToListKind if you add the correct types to the scheme
ie. for k8s types invoke scheme.AddToScheme(scheme) from https://github.com/kubernetes/client-go/blob/1bccfc8c60977e2ce3235394daff128996818f7b/kubernetes/scheme/register.go#L135
You can do this with eventing and serving and save yourself from maintaining this list. Unsure if there's something that prevents you from importing those.
There was a problem hiding this comment.
I also had some issues where I couldn't provide unstructured types to the fake dynamic client. There would be an error with the way the fake client interacts with its object tracker
So I wrote some code for the fake injection client to rewire these schemes to use unstructured.* types
https://github.com/knative/pkg/pull/2145/files#diff-8438a08a79f266761b674dbf8748084e62be792ce56e7d7b598d148c069946e5L40
Unsure if you would need to do the same - if so maybe we pull that bit of logic out out injection and into some sort of fakedynamic package in knative.dev/pkg
dsimansk
force-pushed
the
pr/k8s-update
branch
2 times, most recently
from
f8d48fd to
ae74c32
Compare
| sourcesv1.AddToScheme(scheme) | ||
| sourcesv1beta2.AddToScheme(scheme) | ||
| apiextensionsv1.AddToScheme(scheme) | ||
| _, dynamicClient := dynamicclientfake.With(context.TODO(), scheme, objects...) |
There was a problem hiding this comment.
@dprotaso @navidshaikh wdyt about the above change? It works for our test setup, I've gone through and changed various initialization of fake dynamic client to use the common function.
Indeed I had to use knative/pkg for unstructured object to get them converted properly.
Finally, Navid do you see any problem in bringing apiextensions-apiserver dependency for the scheme?
There was a problem hiding this comment.
Yeah looks good - I might PR a change to k8s to fix this problem but it'll take weeks/months for us to switch over to it
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, navidshaikh 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 |
Description
Update K8s dependencies to follow-up on knative/pkg#2145.
Changes
Reference
Fixes #