Skip to content

Update to k8s 1.30 #26

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

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Update to k8s 1.30 #26

merged 2 commits into from
Jul 8, 2024

Conversation

mhenriks
Copy link
Member

@mhenriks mhenriks commented Jul 5, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

k8s 1.30 libs and controller-runtime 0.18.4

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Jul 5, 2024
@akalenyu
Copy link
Contributor

akalenyu commented Jul 7, 2024

struggling to view the changes on my browser, meanwhile, I believe you'll need to bump golang with something like
a879235

@@ -1,27 +1,28 @@
module kubevirt.io/controller-lifecycle-operator-sdk/api

go 1.17
go 1.22.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kubevirt/kubevirt#12130 is a clearer format IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it matters much given this is a library repo but can we stick with this for now until we have a consensus in kubevirt?

Signed-off-by: Michael Henriksen <[email protected]>
@@ -544,7 +544,7 @@ func (r *Reconciler) GetAllDeployments(cr client.Object) ([]*appsv1.Deployment,
// WatchCR registers watch for the managed CR
func (r *Reconciler) WatchCR() error {
// Watch for changes to managed CR
return r.controller.Watch(source.Kind(r.getCache(), r.crManager.Create()), &handler.EnqueueRequestForObject{})
return r.controller.Watch(source.Kind(r.getCache(), r.crManager.Create(), &handler.EnqueueRequestForObject{}))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was going to tag this and ask how we feel about not doing TypedEnqueueRequestForObject here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is perfectly fine because the type returned by r.crManager.Create() is client.Object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but couldn't we get rid of this in favor of TypedEnqueueRequestForObject to receive the benefits of kubernetes-sigs/controller-runtime#2783

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much of an advantage to us. What specifically are you keen on?

If you feel strongly, it would require an API change (add template param to cr manager) and best handled in subsequent PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the main thing would be something like avoiding a type assertion inside the predicate which we're anyway not using here, so I am fine

@@ -26,7 +26,8 @@ set -ex

# Extend path to use also local installation of the golang
export PATH=$PATH:/usr/local/go/bin
export KUBEVIRT_PROVIDER=$TARGET
#project-infra is passing in 1.24 but let's ignore that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved

@akalenyu
Copy link
Contributor

akalenyu commented Jul 8, 2024

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
@mhenriks
Copy link
Member Author

mhenriks commented Jul 8, 2024

/approve

@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhenriks

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mhenriks mhenriks merged commit 76d2eae into kubevirt:master Jul 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants