-
Notifications
You must be signed in to change notification settings - Fork 2k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update to k8s 1.21.0 tool chain #3926
Conversation
Hi @tamalsaha. Thanks for your PR. I'm waiting for a jetstack or cert-manager 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. |
It seems that the internal api conversion generation is failing. Any idea how to fix this? |
Thank you for the PR!
I think it needs those missing conversion functions implemented. |
It seems like a k8s bug kubernetes/kubernetes#98380 . Those conversion functions exist but the code generator can't "find" them. |
I see that it is all the It seems like the For comparison, the other custom type of which we have fields in other types is |
I believe I have found the root cause of this regression. I have detailed it here kubernetes/kubernetes#101567 and proposed my fix. This fix is working for our repos where
The problem is that when I run the Thanks. |
Thanks for looking into this!
I tried to run it against the fork of this PR, but still got the same compile errors added- I have not done yet done any extensive testing though. |
The generated bazel file does not add https://github.com/jetstack/cert-manager/blob/master/hack/update-bazel.sh |
Do you mean that it doesn't add it when you run gazelle locally? When I run
(Using Bazel |
/ok-to-test |
Now blocked on #3935 |
I think the build currently breaking might be to do with Here is the output of
It seems like there was a breaking change in gnostic in
I see that it that it has been updated to ( |
@irbekrm , thanks for keeping company here :) So, the code-generator issue is resolved. I see that it is working. But the conversion-gen needs things to be done in correct order because the generated code from one steps (meta and acme package) is used in the certmanager packgae. Besides that there are a number of things breaking here:
|
Just a heads up that the latest CI failure for this PR is not from that ^. We only build with Bazel in CI, see #3935 (comment) |
/test pull-cert-manager-bazel |
Hi, sorry for the delay. I see that the unit tests in Logs from running
I have not looked in depth into this yet, I see that the A dynamic client was added to I have not yet looked at why the e2e tests are failing. |
We chatted about this in the standup just now and decided on this approach. Do |
Happy to move ahead with switching to multi-lock - I just think we need to be sure to be consistent between our different binary components! Thanks for all the research into this @irbekrm 😄 |
/lgtm |
Hi @tamalsaha, Apologies, it looks like a fake secrets client that was added in a recent PR is now breaking the tests as its interface has changed in the bumped k8s api library. Would it please be possible to update the fake client in
Doing this and then running |
Thanks for raising the issue with leader election @munnerz 😄 . My bad for initially suggesting that this change will swap |
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Signed-off-by: Tamal Saha <tamal@appscode.com>
Thanks @tamalsaha ! I think the latest error would be fixed by running |
Signed-off-by: Tamal Saha <tamal@appscode.com>
Woohoo 🥳 /lgtm |
Thanks, @irbekrm ! What will get it merged? |
It should get merged automatically by our CI once |
Fixes #3714
What this PR does / why we need it:
Update to k8s 1.21.0 tool chain
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: