upgrade controller-runtime to v0.16 #496
Conversation
Signed-off-by: Davide Bianchi <[email protected]>
davidebianchi
requested review from
kensipe,
asekretenko and
kaiwalyajoshi
as code owners
Signed-off-by: Davide Bianchi <[email protected]>
| @@ -289,7 +289,8 @@ func TestStepDeleteExistingLabelMatch(t *testing.T) { | |||
| }), podSpec) | |||
|
|
|||
| step := Step{ | |||
| Logger: testutils.NewTestLogger(t, ""), | |||
| Logger: testutils.NewTestLogger(t, ""), | |||
| Timeout: 60, | |||
There was a problem hiding this comment.
@davidebianchi I'm curious why this timeout field setting was needed? 🤔
There was a problem hiding this comment.
After the upgrade, this test will timeout. Adding this setting, it works correctly (without sensible increase in testing time).
There was a problem hiding this comment.
Thanks. I wonder what changed. Have we been not enforcing a timeout (which I assume was 0 by default) before? 🤔
There was a problem hiding this comment.
Mmh maybe it is related with this function change: https://github.com/kudobuilder/kuttl/pull/496/files#diff-f83daba5f9026600aa38b2b1e5a0c585535dc3355457e4762c50bdb0f6b0febdR160 (the PollImmediate function is deprecated). It could be that with 0, the context goes now immediately in timeout.
Maybe we could change the implementation to use PollUntilContextCancel if timeout is 0. What do you think?
There was a problem hiding this comment.
Ah, I think I know why. Previously this Get would run with an unbounded (TODO) context. Now it runs with a context with a 0 timeout.
I think the current code is fine, the real default timeout is not 0 in kuttl anyway.
What this PR does / why we need it:
Upgrade controller-runtime to version 16.
Fixes #473