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 duplicate test entry to the cluster-version-operator payload JSON and implements a Go test that initializes a Kubernetes client (KUBECONFIG/in-cluster), then verifies the CVO namespace run-level label, asserts exactly one running CVO pod, and checks the pod SCC annotation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jiajliu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jiajliu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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. |
|
@jiajliu: The following test 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. |
|
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 |
2 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.