Update k8s dependencies to 4.14 and controller-runtime to v0.15#376
Update k8s dependencies to 4.14 and controller-runtime to v0.15#376ranakan19 merged 39 commits intocodeready-toolchain:masterfrom
Conversation
ranakan19
requested review from
MatousJobanek,
alexeykazakov,
mfrancisc,
rajivnathan,
sbryzak and
xcoulon
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 77.61% 78.23% +0.61%
==========================================
Files 49 49
Lines 2426 2444 +18
==========================================
+ Hits 1883 1912 +29
+ Misses 489 478 -11
Partials 54 54
|
|
|
||
| // NewFakeClient creates a fake K8s client with ability to override specific Get/List/Create/Update/StatusUpdate/Delete functions | ||
| func NewFakeClient(t T, initObjs ...runtime.Object) *FakeClient { | ||
| func NewFakeClient(t T, initObjs ...client.Object) *FakeClient { |
There was a problem hiding this comment.
changes initObjs type from runtime.Object to client.Object - see discussion here
WithStatusSubresource can only accept client.Object was a major reason to consider making this change.
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Nice Job!
I have only few minor comments.
|
|
||
| t.Run("resource with expected label", func(t *testing.T) { | ||
| // given | ||
| ctx := context.TODO() |
There was a problem hiding this comment.
minor - we could move the context creation up in the parent test
| created.Status.Replicas = 2 | ||
| require.NoError(t, fclient.Status().Update(context.TODO(), created)) | ||
| require.NoError(t, fclient.Get(context.TODO(), types.NamespacedName{Namespace: "somenamespace", Name: created.Name}, retrieved)) | ||
| assert.EqualValues(t, 2, retrieved.Status.Replicas) // replicas count changed to 2 |
There was a problem hiding this comment.
should we check that generation is 1 or it doesn't bring any benefit ?
|
| WithScheme(s). | ||
| WithRuntimeObjects(initObjs...). | ||
| WithObjects(initObjs...). | ||
| WithStatusSubresource(initObjs...). |
There was a problem hiding this comment.
Related to our discussion in the host-operator PR, we could try to improve this logic to register status sub-resource for all "toolchain" resources. In this way we would know that, the status sub-resource is registered for all objects we manage.
So, could you go through all toolchain GVKs and add them to the client builder as objects with the status sub-resource? Try to use a generic logic using the scheme and GVKs, we don't want to update the logic as soon as we add or remove a CRD.
alexeykazakov
left a comment
alexeykazakov
left a comment
There was a problem hiding this comment.
Looks good overall. Just one minor comment. And +1 to @MatousJobanek's suggestion regarding registering status sub resources for all our API.
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
MatousJobanek
left a comment
MatousJobanek
left a comment
There was a problem hiding this comment.
Thanks for addressing/answering my comments 👍
…mon into k8s_1_27_common
…on into k8s_1_27_common
|
|
||
| t.Run("status create fails", func(t *testing.T) { | ||
| _, retrieved := createAndGetDeployment(t, fclient) | ||
| require.EqualError(t, fclient.Status().Create(context.TODO(), retrieved, retrieved), "fakeSubResourceWriter does not support create for status") |
There was a problem hiding this comment.
Error returned from fakeclient - https://github.com/kubernetes-sigs/controller-runtime/blob/release-0.15/pkg/client/fake/client.go#L1116
Only substatus resource supported is eviction
|
5 participants
Updates k8s dependencies to 0.27 and controller-runtime to v0.15.
With the update to controller-runtime v0.15 (changes in release detailed here), this PR has following changes to combat the breaking changes:
Changes wrt - https://issues.redhat.com/browse/SANDBOX-558
DO NOT MERGE (All the PRs related to version updates need to be merged at once, but these PR are for early feedback so that there are not a lot of changes to review at once)