bump controller-runtime #335
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @stephanhorsthemke. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XL
stephanhorsthemke
changed the title
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/assign @justinsb |
|
I checked the result on prow job. But I wonder if my update is valid, because I updated the expected data. |
| golang.org/x/crypto v0.1.0 | ||
| golang.org/x/tools v0.3.0 | ||
| k8s.io/api v0.26.3 | ||
| k8s.io/apimachinery v0.26.3 |
There was a problem hiding this comment.
Thank you a lot for this PR. Could you check the errors in the CI test?
We need to fix those before running the others.
|
Hey all and thanks for the swift reviews! |
| } | ||
| } | ||
|
|
||
| var restMapper meta.RESTMapper | ||
| if options.Manager != nil { | ||
| restMapper = options.Manager.GetRESTMapper() | ||
| } else { | ||
| rm, err := apiutil.NewDiscoveryRESTMapper(options.RESTConfig) | ||
| client, err := restclient.HTTPClientFor(options.RESTConfig) |
There was a problem hiding this comment.
This creates a new HTTP client and I suspect that this results in the changed timeout IIRC.
An alternative to change the tests (which IMO should be alright, the timeout feels sane?) would be to change the timeout, or rather remove the timeout, here! Probably we could change the RESTConfig?
There was a problem hiding this comment.
In the previous function apiutil.NewDiscoveryRESTMapper the Client was created by a small helper function which we might also be able to use again. I remember that I had trouble with this somehow
- update expected*.yaml - update test.sh for getting kubectl v1.27 instead of v1.26 - update guestbook_controller
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stephanhorsthemke The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Alright. I just went ahead and at least did what @atoato88 did in his branch to see if that fixes all the tests! What I can't answer is if it is alright and fine that the expected client is different now! |
|
Umm, test result was not success, I will update my commit. |
|
I created the update. |
examples/guestbook-operator/controllers/guestbook_controller.go
Outdated
Show resolved
Hide resolved
@camilamacedo86 @stephanhorsthemke the update is usefully to It's block kubernetes-sigs/kubebuilder#3421 |
|
@justinsb |
Co-authored-by: atoato88 <akihito-inou@nec.com>
|
@stephanhorsthemke: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
Amazing @kkkkun Thanks for taking over. |
What this PR does / why we need it:
Bumps controller-runtime and fixes breaking changes
Which issue(s) this PR fixes:
Fixes #333.
Special notes for your reviewer:
This PR already unblocks me as I'll just point the go mod to this commit. I don't have much familiarity with the changes and we should maybe put some more thoughts into some of the changes.
Locally I had some test failures also on master and they looked similar
Additional documentation: