-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bump controller-runtime to v0.14.x #420
Conversation
Issues linked to changelog: |
@@ -0,0 +1,5 @@ | |||
changelog: | |||
- type: BREAKING_CHANGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also list the DEPENDENCY_BUMPS, particularly k8s.io/api and controller-runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, those seem reasonable. All were a consequence of pulling in controller-runtime.
@@ -129,10 +129,10 @@ type {{ $resource.Kind }}Writer interface { | |||
type {{ $resource.Kind }}StatusWriter interface { | |||
// Update updates the fields corresponding to the status subresource for the | |||
// given {{ $resource.Kind }} object. | |||
Update{{ $resource.Kind }}Status(ctx context.Context, obj *{{ $import_prefix }}{{ $resource.Kind }}, opts ...client.UpdateOption) error | |||
Update{{ $resource.Kind }}Status(ctx context.Context, obj *{{ $import_prefix }}{{ $resource.Kind }}, opts ...client.SubResourceUpdateOption) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know why these had to be renamed? as far as i can tell, UpdateOption
and PatchOption
both still exist in controller-runtime. do they behave differently in the newest controller-runtime (e.g., are they enforcing that we call the 'subresource' methods on statuses now)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From kubernetes-sigs/controller-runtime#2072
Use distinct options for subresource Update and Patch which makes this a breaking change
So I can't comment on why these API breakages had to happen other than link to the upstream pull request, but I can say that while UpdateOption
and PatchOption` still exist, but now can only be applied to resources that aren't subresources.
@@ -8076,6 +8194,67 @@ | |||
"type": "null" | |||
} | |||
] | |||
}, | |||
"schedulingGates": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like these and other schema.json updates came from new fields being added to the pod spec in the latest k8s api
The GME PR that uses this branch is here and has trouble compiling. |
what lol |
k8s.io/code-generator v0.26.4 | ||
k8s.io/klog/v2 v2.80.1 | ||
k8s.io/utils v0.0.0-20221128185143-99ec85e7a448 | ||
sigs.k8s.io/controller-runtime v0.14.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/kubernetes-sigs/controller-runtime/blob/main/go.mod#L3
We should bump our go version to align with this dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, will do. Is that something that would be called out in the Changelog as well?
controller-runtime
introduced breaking changes in the status API from v0.13.x -> v0.14.x. Unfortunately, upstream Istio is now aligned with v1.14.x, so bumping skv2 is a necessary prerequisite for repos that want to includeistio.io/istio@v1.17.0
or newer.This PR bumps the dependencies, updates API signatures, and updates codegen templates to comply. I don't know why codegen updated schema json files.
BOT NOTES:
resolves #419