NO-ISSUE: pkg/operator/kubeletversionskewcontroller: Explicit next OpenShift version in skew message #1933
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request explicitly references no jira issue. In 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a nextOpenShiftVersion field to the kubelet version skew controller, initializes it in the constructor, and updates all upgrade-related messages and condition strings to include the explicit next OpenShift minor version. Test expectations are updated to match the new message format. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller_test.go (1)
147-156: Initialize the missingnextOpenShiftVersionfield.The test controller initialization is missing the
nextOpenShiftVersionfield that was added to the struct inkubelet_version_skew_controller.go. Without this field, the controller will use the zero value (0.0.0) and the test messages will show "to 0.0" instead of the expected version numbers, causing all affected tests to fail.Apply this diff to add the missing field:
c := &kubeletVersionSkewController{ operatorClient: v1helpers.NewFakeStaticPodOperatorClient( &operatorv1.StaticPodOperatorSpec{OperatorSpec: operatorv1.OperatorSpec{ManagementState: operatorv1.Managed}}, status, nil, nil, ), nodeLister: corev1listers.NewNodeLister(indexer), apiServerVersion: semver.MustParse(apiServerVersion), minSupportedSkew: minSupportedKubeletSkewForOpenShiftVersion(ocpVersion), minSupportedSkewNextVersion: minSupportedKubeletSkewForOpenShiftVersion(nextOpenShiftVersion), + nextOpenShiftVersion: nextOpenShiftVersion, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller.go(3 hunks)pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller_test.go(3 hunks)
🔇 Additional comments (6)
pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller.go (5)
55-55: LGTM!The computation of the next OpenShift version by incrementing the minor version is correct and straightforward.
62-62: LGTM!The field initialization properly assigns the computed next OpenShift version to the controller.
89-89: LGTM!The new field addition is appropriate for storing the next OpenShift version information.
186-190: LGTM!The message formatting correctly includes the explicit next OpenShift version in all three cases (single node, 2-3 nodes, and many nodes), which aligns with the PR's objective to clarify that the
Upgradeablestatus refers to future updates.
197-201: LGTM!The message formatting correctly includes the explicit next OpenShift version in all three cases, making it clear which future upgrade the supportability statement refers to.
pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller_test.go (1)
68-68: LGTM!These test expectations correctly include the explicit next OpenShift version (4.9) in the upgrade messages, matching the updated format in the controller implementation.
Also applies to: 84-84, 92-92, 100-100
| expectedStatus: operatorv1.ConditionTrue, | ||
| expectedReason: KubeletMinorVersionSupportedNextUpgradeReason, | ||
| expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade.", | ||
| expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade 4.10.", |
There was a problem hiding this comment.
Fix the missing "to" preposition.
The test expectation is missing "to" before "4.10". The actual message format in line 197 of kubelet_version_skew_controller.go includes "upgrade to %d.%d", so this test expectation should match.
Apply this diff to fix the test expectation:
- expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade 4.10.",
+ expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade to 4.10.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade 4.10.", | |
| expectedMsgLines: "Kubelet minor version (1.20.0) on node test000 is behind the expected API server version; nevertheless, it will continue to be supported in the next OpenShift minor version upgrade to 4.10.", |
🤖 Prompt for AI Agents
In
pkg/operator/kubeletversionskewcontroller/kubelet_version_skew_controller_test.go
around line 124, the expected message string is missing the preposition "to"
before the OpenShift version; update the expectedMsgLines value to include "to"
so it matches the actual message format ("upgrade to %d.%d") used in
kubelet_version_skew_controller.go.
…rsion in skew message Explicitly name the next OpenShift version, to reduce the chances that cluster administrators think the message applies to a currently in-progress update, when Upgradeable is only talking about future updates.
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Explicitly name the next OpenShift version, to reduce the chances that cluster administrators think the message applies to a currently in-progress update, when
Upgradeableis only talking about future updates.