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

Upgrade k8s depencencies in host for Openshift 4.16 #1125

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

Conversation

ranakan19
Copy link
Contributor

Upgrades:

Tool/Library Current Version Updates to Version
Golang 1.20 1.21
Operator SDK 1.25 1.36
k8s.io/* v0.27.0 v0.29.2
controller-runtime v0.15 v0.17.3
controller-gen v0.12.0 v0.14.0

Changes in code wrt to controller-runtime upgrade (introduced in controller-runtime v0.16.0:

wrt: https://issues.redhat.com/browse/SANDBOX-686

Metrics: metricsserver.Options{
BindAddress: metricsAddr,
},
WebhookServer: webhookServer,
Copy link
Contributor

Choose a reason for hiding this comment

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

the same question as in the member-operator PR - do we need the webhookServer configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its because port is deprecated and webhookServer replaces it.
from the documentation:

        // Port is the port that the webhook server serves at.
	// It is used to set webhook.Server.Port if WebhookServer is not set.
	//
	// Deprecated: Use WebhookServer instead. A WebhookServer can be created via webhook.NewServer.
	Port int

Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 79.10%. Comparing base (561d8a7) to head (34db249).

Files with missing lines Patch % Lines
main.go 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
- Coverage   79.16%   79.10%   -0.07%     
==========================================
  Files          78       78              
  Lines        7814     7819       +5     
==========================================
- Hits         6186     6185       -1     
- Misses       1449     1455       +6     
  Partials      179      179              
Files with missing lines Coverage Δ
main.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

HealthProbeBindAddress: probeAddr,
LeaderElection: enableLeaderElection,
LeaderElectionID: "dc07038f.toolchain.host.operator",
Namespace: namespace,
Cache: cache.Options{DefaultNamespaces: map[string]cache.Config{namespace: {}}},
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember we discussed it and you explained how this new Cache configuration works but I already forgot :)
This configuration looks confusing (why there is default namespace as a map with a key but no value) so maybe you can try to add some comments explaining how it works? Or maybe a link to docs where it's explained. But feel free to leave it as is if it's not trivial.

Copy link

openshift-ci bot commented Jan 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, ranakan19

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:
  • OWNERS [alexeykazakov,ranakan19]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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

Successfully merging this pull request may close these issues.

4 participants