Bump Kubernetes dependencies to 1.26.x #287
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
/test all |
erikgb
changed the title
|
/test all |
|
Hey @erikgb is there some reason to bump this now? |
|
@adrianludwin I tried to write tests in #285, and is blocked by features added to newer controller-runtime versions. I can always work around it, by copying code. Is there a reason for not upgrading? 😉 |
|
Nope, that sounds great! Just wanted to know :) Thanks! |
erikgb
force-pushed
the
bump-controller-tools
branch
2 times, most recently
from
c3c3028 to
51e29ff
Compare
erikgb
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @adrianludwin @rjbez17 |
|
/test all |
|
/retest |
|
/retest pull-hnc-test |
|
@erikgb: The
Use In response to this:
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. |
|
@adrianludwin Any idea what has happened to the CI? It ran yesterday.... |
erikgb
changed the title
erikgb
force-pushed
the
bump-controller-tools
branch
from
51e29ff to
a61dfa9
Compare
| @@ -112,8 +111,8 @@ spec: | |||
| description: "Condition contains details for one aspect of the current | |||
| state of this API Resource. --- This struct is intended for direct | |||
| use as an array at the field path .status.conditions. For example, | |||
| type FooStatus struct{ // Represents the observations of a foo's | |||
| current state. // Known .status.conditions.type are: \"Available\", | |||
| \n type FooStatus struct{ // Represents the observations of a | |||
There was a problem hiding this comment.
Any idea what this \n is?
There was a problem hiding this comment.
I think it originates from the upstream K8s Condition type doc. It is what controller-gen "spits out" now. I have seen occasional changes to our CRDs (in other controllers) when reusing upstream types and docs updated upstream.
There was a problem hiding this comment.
That newline just looks weird, it's not showing up anywhere else? Maybe check the comment in the source file to make sure there's no weird newline there? But obviously not a big deal if it's not obvious.
There was a problem hiding this comment.
This is how the source looks like:
// Condition contains details for one aspect of the current state of this API Resource.
// ---
// This struct is intended for direct use as an array at the field path .status.conditions. For example,
//
// type FooStatus struct{
// // Represents the observations of a foo's current state.
// // Known .status.conditions.type are: "Available", "Progressing", and "Degraded"
// // +patchMergeKey=type
// // +patchStrategy=merge
// // +listType=map
// // +listMapKey=type
// Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
//
// // other fields
// }
type Condition struct {It seems like the empty line adds /n to the CRD field description.
|
No idea, seeing it on the postsubmit as well:
https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/hnc-postsubmit-test/1663185368003907584
…On Mon, May 29, 2023 at 12:54 PM Erik Godding Boye ***@***.***> wrote:
@adrianludwin <https://github.com/adrianludwin> Any idea what has
happened to the CI? It ran yesterday....
—
Reply to this email directly, view it on GitHub
<#287 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AE43PZF3EJN3GMHZPW4CDHTXITICZANCNFSM6AAAAAAYSCPRPM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Maybe file a bug to the Prow folks? Other repos seem to have passing tests today, so maybe it's just us? A quick Google search didn't turn up any obvious problems. |
@adrianludwin There is already an issue on this: kubernetes/test-infra#29622. Let's hope it will be sorted out soon. |
|
Great, glad to know it's not just us :) |
|
/retest |
1 similar comment
|
/retest |
|
The tests look to be passing now, @adrianludwin ok to merge? |
|
/lgtm |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: erikgb, rjbez17 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 |
k8s-ci-robot
added
the
approved
Tested: All tests run fine, including e2e-tests in HNC_REPAIR mode. While running the e2e-tests I noticed some TLS handshake errors in HNC controller logs, but I don't know if it is due to my corporate network environment.
erikgb
force-pushed
the
bump-controller-tools
branch
from
a61dfa9 to
4b7fb70
Compare
k8s-ci-robot
removed
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This PR bumps the K8s dependencies to 1.26.x. My main motivation is to get access to new features available in recent versions of controller-runtime. I have not bumped to the latest version of c-r, since it contains a lot of breaking changes that will require changing a lot more code, ref. https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0. I could eventually fix this in a follow-up PR.
As a consequence of the bump of K8s dependencies, it is required to bump other dependencies like cert-controller as well.
All tests run fine: normal tests and e2e-tests (also in
HNC_REPAIRmode). I notice a few TLS errors in controller logs, but I do not know if this is caused by my corporate network environment. But I wouldn't mind someone else checking it!