Repository navigation
Upgrade k8s depencencies for Openshift 4.16 - #615
Conversation
ranakan19
changed the title
| LeaderElectionID: "2fc71baf.toolchain.member.operator", | ||
| Namespace: namespace, | ||
| ClientDisableCacheFor: []client.Object{&kmetrics.NodeMetrics{}}, | ||
| Client: client.Options{Cache: &client.CacheOptions{DisableFor: []client.Object{&kmetrics.NodeMetrics{}}}}, |
There was a problem hiding this comment.
why do we need to exclude the kmetrics.NodeMetrics objects?
There was a problem hiding this comment.
I went back in the history of commits and found where it was added: 38d239f
Maybe worth adding a comment
| BindAddress: metricsAddr, | ||
| }, | ||
| Cache: cache.Options{DefaultNamespaces: map[string]cache.Config{namespace: {}}}, | ||
| WebhookServer: webhookServer, |
There was a problem hiding this comment.
why do we need this webhook server?
There was a problem hiding this comment.
Yeah, I have the very same question - we didn't have any webhook configuration before (at least I don't see i there) and we don't use OLM's way of deploying admission webhooks, so do we really need it?
There was a problem hiding this comment.
we had port before which was deprecated and now removed.
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
for more details I've linked the PR removing deprecated manager options field in the description.
There was a problem hiding this comment.
I'm curious if the default value is the same as we set here (port 9443)?
There was a problem hiding this comment.
yes the default port value is 9443, good thinking/curiosity.
From the following doc - https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/manager/manager.go#L244-L247 - Manager will create a default webhook server with default setting which works for us.
Let me remove the webhook server then.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #615 +/- ##
=======================================
Coverage 81.66% 81.66%
=======================================
Files 29 29
Lines 3305 3305
=======================================
Hits 2699 2699
Misses 457 457
Partials 149 149 |
rsoaresd
left a comment
rsoaresd
left a comment
There was a problem hiding this comment.
Nice work 🚀 Just on question:
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, fbm3307, MatousJobanek, ranakan19, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest - flaky test |
|
@ranakan19: The Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
Upgrades:
Changes in code wrt to controller-runtime upgrade (introduced in controller-runtime v0.16.0:
wrt: https://issues.redhat.com/browse/SANDBOX-686