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 all dependencies to latest to upgrade controller-runtime #1050

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

markusthoemmes
Copy link
Collaborator

@markusthoemmes markusthoemmes commented Jan 30, 2024

What this PR does / why we need it:

This upgrades all dependencies of the project to their latest releases. The most notable of those is controller-runtime, which has seen some significant rework. No semantic changes are intended in this change.

Does this PR introduced a user-facing change?

None

Additional documentation, usage docs, etc.:


@@ -87,9 +86,10 @@ func main() {

ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
ctrlOpts := ctrl.Options{
Scheme: scheme,
MetricsBindAddress: metricsAddr,
Port: 9443,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the default of the Webhook server created in the new version as well.

@benjaminhuo
Copy link
Member

@markusthoemmes Thanks again for the PR.
There are conflicts that need to be resolved, you'll need to rebase the master

This upgrades all dependencies of the project to their latest releases. The most notable of those is controller-runtime, which has seen some significant rework. No semantic changes are intended in this change.

Signed-off-by: Markus Thömmes <markusthoemmes@me.com>
@markusthoemmes
Copy link
Collaborator Author

Done. I was staging PRs and didn't notice the other one actually merged.

Comment on lines +101 to +103
ctrlOpts.Cache.DefaultNamespaces = make(map[string]cache.Config, len(namespaces))
for _, ns := range namespaces {
ctrlOpts.Cache.DefaultNamespaces[ns] = cache.Config{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@markusthoemmes
Copy link
Collaborator Author

Hmm. Gotta fix the code generation. Will take care of that.

@benjaminhuo
Copy link
Member

Signed-off-by: Markus Thömmes <markusthoemmes@me.com>
Signed-off-by: Markus Thömmes <markusthoemmes@me.com>
@markusthoemmes
Copy link
Collaborator Author

Hmm. This diff is becoming much larger than intended because the PodSpec grew quiet a bit between versions, I suppose.

Signed-off-by: Markus Thömmes <markusthoemmes@me.com>
if [[ $ret -eq 0 ]]
then
echo "${DIFFROOT} up to date."
else
echo "${DIFFROOT} is out of date. Please rerun make generate"
echo "${DIFFROOT} is out of date. Please run hack/update-codegen.sh"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This had to be changed because generate-groups.sh seems to delete the tree now and thus the comparison was wrong.

This change essentially copies the current tree into a temp dir, then generates the clients "as normal" (full on replacing the tree) and then diffs the copied out tree with the now regenerated clients. Has the added benefit, that the actual generation is deduplicated.

@markusthoemmes
Copy link
Collaborator Author

@benjaminhuo As mentioned above, this diff has grown quite a bit. We could try to make it smaller by upgrading controller-runtime in smaller steps or trying to break the K8s upgrades out. Let me know if you'd like me to invest some effort into trying to make the diff smaller.

@benjaminhuo
Copy link
Member

Hmm. This diff is becoming much larger than intended because the PodSpec grew quiet a bit between versions, I suppose.

@benjaminhuo As mentioned above, this diff has grown quite a bit. We could try to make it smaller by upgrading controller-runtime in smaller steps or trying to break the K8s upgrades out. Let me know if you'd like me to invest some effort into trying to make the diff smaller.

@wanjunlei what do you think?

@wanjunlei
Copy link
Collaborator

Hmm. This diff is becoming much larger than intended because the PodSpec grew quiet a bit between versions, I suppose.

@benjaminhuo As mentioned above, this diff has grown quite a bit. We could try to make it smaller by upgrading controller-runtime in smaller steps or trying to break the K8s upgrades out. Let me know if you'd like me to invest some effort into trying to make the diff smaller.

@wanjunlei what do you think?

I think it's more appropriate to upgrade the controller runtime in smaller steps.

@benjaminhuo
Copy link
Member

@benjaminhuo As mentioned above, this diff has grown quite a bit. We could try to make it smaller by upgrading controller-runtime in smaller steps or trying to break the K8s upgrades out. Let me know if you'd like me to invest some effort into trying to make the diff smaller.

We might need to catch up with the upstream k8s community updates and drop supports for lower version K8s.
The dependencies are quite obsolete, so maybe big changes are also acceptable as long as we add a k8s support metrics @markusthoemmes @wanjunlei

@benjaminhuo
Copy link
Member

We might need to catch up with the upstream k8s community updates and drop supports for lower version K8s.

we really need to catch up with upstream now because there're quite big differences and the code cannot compile with latest k8s dependencies:

image

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.

None yet

3 participants