Skip to content
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 k8s dependencies to 1.30 #1158

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Bump k8s dependencies to 1.30 #1158

wants to merge 26 commits into from

Conversation

edude03
Copy link
Contributor

@edude03 edude03 commented Nov 29, 2024

Continuing the epic to upgrade from kubernetes 1.28 -> 1.31, bump the k8s dependencies

Copy link

github-actions bot commented Nov 29, 2024

No changes to the coverage.

HTML Report

Click to open

@edude03 edude03 changed the title Bump dependencies to 1.30 neonvm : bump dependencies to 1.30 Dec 3, 2024
@edude03 edude03 changed the title neonvm : bump dependencies to 1.30 neonvm: bump dependencies to 1.30 Dec 9, 2024
@edude03 edude03 marked this pull request as ready for review December 9, 2024 21:49
Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

bump the dependencies for the neonvm components

For clarity, changing the dependencies version in go.mod updates all components except cluster-autoscaler. The biggest ones are neonvm-controller, scheduler, and autoscaler-agent.

Please also update the relevant k8s tool versions defined in the Makefile — see KUSTOMIZE_VERSION, ENVTEST_K8S_VERSION, CONTROLLER_TOOLS_VERSION, CODE_GENERATOR_VERSION, KUTTL_VERSION, KUBECTL_VERSION, KIND_VERSION, K3D_VERSION

(sorry!)

@edude03
Copy link
Contributor Author

edude03 commented Dec 10, 2024

For clarity, changing the dependencies version in go.mod updates all components except cluster-autoscaler. The biggest ones are neonvm-controller, scheduler, and autoscaler-agent.

Is there a name/alias for that or should I mention them all?

Please also update the relevant k8s tool versions defined in the Makefile — see KUSTOMIZE_VERSION, ENVTEST_K8S_VERSION, CONTROLLER_TOOLS_VERSION, CODE_GENERATOR_VERSION, KUTTL_VERSION, KUBECTL_VERSION, KIND_VERSION, K3D_VERSION

(sorry!)

Don't be, thanks for the list. How do we pick what version each tool should be? For envtest and the node version in kind/k3d it should match kubernetes clearly, but everything else, seems like we have a mix?

Personally, I'd say we should do something like this:

  1. Define TARGET_KUBE_VERSION

For the things that rely on a specific kube version, use that variable - either via interpolation OR for things that have a version right now, something like KUBECTL_VERSION=${KUBECTL_VERSION:-$TARGET_KUBE_VERSION}

  1. Define a list of things that can be updated to the latest working version IE: (I believe) kuttl/k3d/kind should work at their latest versions.

  2. Maybe a list of things and were the version comes from? I think we kind of have this now, with links to github releases, but maybe some things need links to the go.mod file for example.

@sharnoff
Copy link
Member

Is there a name/alias for that or should I mention them all?

I would just name the PR something like "Bump k8s dependencies to 1.30". The PR title format checker will complain, but you can add <!-- affects all --> to the PR description to get it to go along with it :)

@edude03 edude03 changed the title neonvm: bump dependencies to 1.30 Bump k8s dependencies to 1.30 Dec 12, 2024
@edude03 edude03 force-pushed the chore/upgrade-deps-1.30 branch from 8e994e2 to c3f0cd3 Compare December 13, 2024 20:12
@edude03 edude03 requested a review from sharnoff December 16, 2024 16:02
Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

Lots of small comments. Assuming all's well, should just be one (shorter) round of review after this.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
Comment on lines 498 to 499
# Should match the kubeernetes minor vesion
CODE_GENERATOR_VERSION ?= v0.30.7
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on why it should match the k8s minor version?

(also: typo: s/kubeernetes/kubernetes/ 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually don't know why it should match, though IIRC it was because of dep resolution errors

Copy link
Member

Choose a reason for hiding this comment

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

Hm, if we're not sure, then let's not add this comment?

Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
neonvm/hack/generate.sh Show resolved Hide resolved
pkg/neonvm/controllers/vm_controller.go Outdated Show resolved Hide resolved
@sharnoff
Copy link
Member

Also, PR description still says

bump the dependencies for the neonvm components

Copy link
Member

@sharnoff sharnoff left a comment

Choose a reason for hiding this comment

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

One comment thread left, otherwise LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants