OTA-1604: migrate ocp-46922 from otp to cvo repo #1279
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jiajliu: This pull request references OTA-1604 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a new payload JSON test entry and implements a cluster-live Cluster Version Operator test plus supporting utilities for REST/kube/config clients and HyperShift detection/skipping. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)test/cvo/cvo.go (1)
🔇 Additional comments (7)
✏️ Tip: You can disable this entire section by setting Comment |
jiajliu
force-pushed
the
rewrite-46922
branch
from
381275d to
76ea813
Compare
|
@jiajliu: This pull request references OTA-1604 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
jiajliu
force-pushed
the
rewrite-46922
branch
from
df298e2 to
160d2ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cvo/cvo.go (2)
16-16: Consider removing or relocating GinkgoRecover.The
defer GinkgoRecover()at the Describe block level executes during test registration, not during test execution. It would only catch panics during the registration phase. Ginkgo v2 already handles panics in test specs automatically, making this defer unnecessary. If you need to recover from panics in spawned goroutines within tests, moveGinkgoRecover()into those goroutines instead.🔎 Proposed change
var _ = Describe(`[Jira:"Cluster Version Operator"] cluster-version-operator-tests`, func() { - defer GinkgoRecover() - const cvoNamespace = "openshift-cluster-version"
24-40: Consider using consistent error handling.The BeforeEach block mixes two error handling styles:
Fail()at line 35 andExpect().NotTo(HaveOccurred())at line 39. For consistency and better integration with Ginkgo/Gomega patterns, consider usingExpect().NotTo(HaveOccurred())throughout.🔎 Proposed change for consistency
restCfg, err = rest.InClusterConfig() if err != nil { // Fall back to kubeconfig (respects KUBECONFIG env var for local execution) loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() cfg := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, &clientcmd.ConfigOverrides{}) restCfg, err = cfg.ClientConfig() - if err != nil { - Fail("Failed to load Kubernetes configuration. Please ensure KUBECONFIG environment variable is set or running in-cluster.") - } + Expect(err).NotTo(HaveOccurred(), "Failed to load Kubernetes configuration. Please ensure KUBECONFIG environment variable is set or running in-cluster.") } kubeClient, err = kubernetes.NewForConfig(restCfg) Expect(err).NotTo(HaveOccurred(), "Failed to create Kubernetes client")
📜 Review details
Configuration used: Organization 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)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/cvo/cvo.go
🔇 Additional comments (3)
test/cvo/cvo.go (3)
4-9: LGTM! Appropriate imports for Kubernetes client setup.The added imports correctly support the test's requirements: context for test lifecycle, k8s.io client libraries for cluster interaction, and clientcmd for kubeconfig fallback.
18-22: LGTM! Well-structured test setup variables.The constant for the CVO namespace and variables for the REST config and Kubernetes client are appropriately scoped and typed.
48-69: LGTM! Well-structured test with clear validation logic.The test correctly validates:
- CVO namespace has no explicit run-level label (empty string = default early-start priority)
- Exactly one running CVO pod exists with the expected label
- CVO pod uses the "hostaccess" SCC for necessary host-level privileges
The use of
By()statements, descriptive assertions, and reference comments to the migration source all contribute to maintainability.
jiajliu
force-pushed
the
rewrite-46922
branch
from
160d2ac to
221cf46
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cvo/cvo.go (1)
22-33: Consider usingBeforeAllfor efficiency.The BeforeEach creates a new Kubernetes client before every test, including the trivial "should support passing tests" which doesn't need one. Since the client configuration doesn't change between tests, consider using Ginkgo's
BeforeAll(or wrapping the client-dependent test in its ownContextwith a dedicatedBeforeEach).This is a minor efficiency concern given the small number of tests.
📜 Review details
Configuration used: Organization 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)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/cvo/cvo.go
🔇 Additional comments (1)
test/cvo/cvo.go (1)
41-62: LGTM!The migrated test logic is correct:
- Properly retrieves the namespace and checks the
openshift.io/run-levellabel (empty string check works correctly even if label is absent, since Go maps return zero value).- Pod listing with label and field selectors is appropriate.
- Single pod assertion and SCC annotation check are well-structured.
The migration preserves the original test intent while using idiomatic client-go patterns.
jiajliu
force-pushed
the
rewrite-46922
branch
from
221cf46 to
56162c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/cvo/cvo.go (1)
46-49: Consider distinguishing "not found" from other errors for clearer debugging.If a network or API error occurs (not just NotFound), the skip message "CVO deployment not found!" may be misleading. For better debuggability in CI logs, consider checking specifically for NotFound errors:
🔎 Suggested improvement
+ "k8s.io/apimachinery/pkg/api/errors"cvoDeployment, err := kubeClient.AppsV1().Deployments(cvoNamespace).Get(ctx, "cluster-version-operator", metav1.GetOptions{}) - if err != nil || cvoDeployment == nil { + if errors.IsNotFound(err) { Skip("Skipping test: CVO deployment not found!") } + Expect(err).NotTo(HaveOccurred(), "Failed to get CVO deployment")
📜 Review details
Configuration used: Organization 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)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/cvo/cvo.go
🔇 Additional comments (3)
test/cvo/cvo.go (3)
4-10: LGTM!The imports are appropriate for the Kubernetes client-go operations being performed.
27-38: LGTM!The Kubernetes client setup follows idiomatic patterns for client-go. Using
NewDefaultClientConfigLoadingRulescorrectly respects theKUBECONFIGenvironment variable, and error messages are descriptive.
51-68: LGTM!The test assertions are well-structured with clear
By()statements for documentation. The runlevel and SCC verification logic is correct, and error messages are descriptive for debugging test failures.
jiajliu
force-pushed
the
rewrite-46922
branch
from
56162c0 to
ba5e305
Compare
|
@jiajliu: This pull request references OTA-1604 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Local test result: From pre-submits jobs e2e-aws-ovn-techpreview and e2e-agnostic-ovn, the new case cc @hongkailiu @DavidHurta to help review, thanks |
|
@hongkailiu @DavidHurta just bring your attention in case you missed my last comment for your year-end break. Could you help review the pr when you have a moment? |
openshift-merge-robot
added
the
needs-rebase
|
/test okd-scos-images |
|
/cc |
hongkailiu
left a comment
hongkailiu
left a comment
There was a problem hiding this comment.
Jia, off the record, this is a perfect example that does not use any oc command. 😸
jiajliu
force-pushed
the
rewrite-46922
branch
from
ba5e305 to
8680e60
Compare
openshift-merge-robot
removed
the
needs-rebase
hongkailiu
left a comment
hongkailiu
left a comment
There was a problem hiding this comment.
The comments are only about wording of specs. Otherwise, LGTM.
jiajliu
force-pushed
the
rewrite-46922
branch
from
8680e60 to
db7cc73
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/cvo/util.go`:
- Around line 32-37: SkipIfHypershift currently ignores errors from
IsHypershift; change it to check the returned error and fail the test when
non-nil instead of proceeding silently: in SkipIfHypershift call
IsHypershift(ctx, restConfig), if err != nil invoke the test failure helper
(e.g., Fail or Failf) with a clear message including the error, otherwise keep
the existing behavior of calling Skip when isHypershift is true.
- Around line 23-26: The infrastructure fetch currently swallows errors by
returning (false, nil) when configClient.ConfigV1().Infrastructures().Get(...)
fails; change this to propagate the error or explicitly handle NotFound: if
apierrors.IsNotFound(err) then treat as missing infrastructure (return false,
nil) else return false, err (or log and fail the test). Update the code around
the Get call (the block using configClient.ConfigV1().Infrastructures().Get and
the variables infrastructure/err) to perform an apierrors.IsNotFound check and
only suppress NotFound, otherwise return or propagate the actual error.
📜 Review details
Configuration used: Organization 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 (3)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/cvo/util.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/cvo/cvo.gotest/cvo/util.go
🧬 Code graph analysis (1)
test/cvo/cvo.go (1)
test/cvo/util.go (3)
GetRestConfig(40-42)GetKubeClient(45-47)SkipIfHypershift(32-37)
🔇 Additional comments (5)
.openshift-tests-extension/openshift_payload_cluster-version-operator.json (1)
21-31: LGTM!The new test entry is correctly structured, matches the existing entries' format, and the test name properly aligns with the
Describe/Itblocks intest/cvo/cvo.go.test/cvo/util.go (1)
39-52: LGTM!The client factory functions are clean and follow idiomatic patterns for Kubernetes client initialization.
test/cvo/cvo.go (3)
4-8: LGTM!Imports are necessary for the new cluster-access test functionality.
35-51: LGTM!The new
Describeblock follows the agreed-upon naming convention and properly initializes the Kubernetes client for cluster-access tests.
53-78: LGTM!The test implementation correctly validates the CVO namespace label and pod annotation. The logic follows the migration source and past reviewer feedback has been addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
jiajliu
force-pushed
the
rewrite-46922
branch
from
db7cc73 to
72a6c34
Compare
|
/lgtm /hold if you want to address this. |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
lgtm
jiajliu
force-pushed
the
rewrite-46922
branch
from
72a6c34 to
204dd75
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, jiajliu 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 |
|
@jiajliu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/retest-required |
4 participants
This PR refactors test case OCP-46922 with client-go and migrated it from the openshift-tests-private repository to the CVO repository as an openshift-tests-extension test.