CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 and 7 more test cases - #694
CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 and 7 more test cases#694anuragthehatter wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OpenShift extended e2e tests: dependency pins in go.mod, a test build system and Makefile targets, Dockerfile packaging of the gzipped test binary, a Cobra-based OTE test entrypoint, OCClient and kubeconfig utilities, and an initial Ginkgo operator installation test. ChangesExtended tests (OTE) integration
Sequence DiagramsequenceDiagram
participant DockerBuilder
participant MakeTest
participant GoCompiler
participant gzip
participant RuntimeImage
DockerBuilder->>MakeTest: build e2e tests
MakeTest->>GoCompiler: compile main.go
GoCompiler->>gzip: gzip binary
DockerBuilder->>RuntimeImage: copy gzipped binary to /usr/bin/
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/e2e/operator/operator.go`:
- Around line 15-16: The defer g.GinkgoRecover() call is misplaced inside the
g.Describe callback; remove it from the Describe block and either delete it
entirely or relocate it to the setup of any goroutine-starting tests (e.g.,
inside BeforeEach/It where goroutines are spawned) so that GinkgoRecover() is
deferred in the same function that starts those goroutines; search for
g.Describe and GinkgoRecover to find and update the placement accordingly.
In `@test/e2e/util.go`:
- Around line 91-92: Check for nil before dereferencing
deployment.Spec.Replicas: compute an int32 desiredReplicas := int32(1) and if
deployment.Spec.Replicas != nil set desiredReplicas = *deployment.Spec.Replicas,
then compare deployment.Status.ReadyReplicas == desiredReplicas &&
deployment.Status.UpdatedReplicas == desiredReplicas instead of directly
dereferencing deployment.Spec.Replicas; update the conditional that currently
uses deployment.Spec.Replicas to use this safe desiredReplicas value.
- Around line 52-59: CreateNamespace currently fails if the namespace already
exists; update CreateNamespace to call client.CoreV1().Namespaces().Create and,
if it returns an error, check kubernetes API error using
apierrors.IsAlreadyExists(err) and in that case return the existing namespace
via client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{}) with a nil
error; otherwise propagate the original error. Use the apierrors.IsAlreadyExists
helper and the CreateNamespace function name to locate where to add this
handling.
🧹 Nitpick comments (4)
test/e2e/util.go (1)
70-72: Consider tolerating transient errors during polling.Returning
false, erron Get failure stops polling immediately. ForNotFounderrors (pod not yet created), this may be premature. Consider returningfalse, nilfor transient/expected errors to allow polling to continue.Proposed approach
pod, err := client.CoreV1().Pods(namespace).Get(ctx, podName, metav1.GetOptions{}) if err != nil { + if k8serrors.IsNotFound(err) { + return false, nil // Pod not yet created, keep polling + } return false, err }test/e2e/operator/operator.go (1)
55-58: UseContainSubstringmatcher directly for cleaner assertions.More idiomatic Gomega
for _, crd := range expectedCRDs { - o.Expect(strings.Contains(crdOutput, crd)).To(o.BeTrue(), - "CRD %s should be installed", crd) + o.Expect(crdOutput).To(o.ContainSubstring(crd), + "CRD %s should be installed", crd) }test/extension/registry.go (1)
9-14: Consider if mutex is necessary for the current usage pattern.The
RWMutexprovides thread-safety, but based on the usage intest/cmd/main.go, the registry is created and populated once during startup before any concurrent access. If concurrent registration isn't a requirement, the mutex adds unnecessary complexity. However, this is fine to keep if you anticipate future concurrent usage.test/extension/cmd/commands.go (1)
43-50: Hardcoded test list will become stale.The test names are hardcoded, which means this list must be manually updated whenever tests are added or removed. Consider generating this dynamically from Ginkgo's spec tree, or documenting that this list requires manual maintenance.
|
You are not using OTE framework ? but title named 'OTE' . and you are building the binary with owned options. |
|
Could you make the go vendor and go.sum .etc in one separate commit thus we can review others changes easily? thanks |
c5e16b5 to
50de070
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@go.mod`:
- Line 94: Upgrade the indirect dependency golang.org/x/oauth2 in go.mod from
v0.25.0 to v0.27.0 (or newer) to remediate CVE-2025-22868; update the version
string for golang.org/x/oauth2, run go mod tidy to refresh go.sum, and re-run
your build/tests to ensure no dependency breakage (look for the
golang.org/x/oauth2 entry in go.mod and the resulting changes in go.sum).
🧹 Nitpick comments (2)
test/e2e/cli.go (1)
49-57: Consider adding--ignore-not-foundflag for cleanup resilience.The
Deletemethod may fail if the resource doesn't exist, which can cause issues during test cleanup or idempotent operations.♻️ Proposed enhancement
// Delete deletes a resource -func (c *OCClient) Delete(ctx context.Context, resourceType, name, namespace string) error { - args := []string{"delete", resourceType, name} +func (c *OCClient) Delete(ctx context.Context, resourceType, name, namespace string, ignoreNotFound bool) error { + args := []string{"delete", resourceType, name} + if ignoreNotFound { + args = append(args, "--ignore-not-found") + } if namespace != "" { args = append(args, "-n", namespace) }test/e2e/operator/operator.go (1)
71-72: Useg.By()orGinkgoWriterinstead offmt.Println.
fmt.Printlnoutput may not be captured properly by Ginkgo's test output handling. For consistency with the rest of the test, useg.By()for step logging.♻️ Proposed fix
g.By("SUCCESS - Ingress Node Firewall operator and CRDs installed") - fmt.Println("Operator install and CRDs check successful!")The
g.By()call on line 71 already logs the success message, making thefmt.Printlnredundant.
You're right. This is fixed. Thanks for reviewing that. Re-ran the usecase. It was an experiment and seems like real cimmit was missed :( |
anuragthehatter
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
50de070 to
f519c43
Compare
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
06fe30f to
5e93ac9
Compare
|
/lgtm |
5e93ac9 to
3b9f0ba
Compare
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter, asood-rh 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 |
8a62a5f to
dd78908
Compare
…ss-node-firewall Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dd78908 to
87dba4e
Compare
|
/test e2e-aws-ovn-infw-extension |
|
/testwith e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
@anuragthehatter, |
|
/test e2e-aws-ovn-infw-extension |
|
/payload-job-with-prs e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
@anuragthehatter: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/test e2e-aws-ovn-infw-extension |
|
/payload-job-with-prs pull-ci-openshift-ingress-node-firewall-master-e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
@anuragthehatter: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
@anuragthehatter: 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. |
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
5 similar comments
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
/testwith openshift/ingress-node-firewall/master/e2e-aws-ovn-infw-extension openshift/origin#31245 |
|
@anuragthehatter, |
5 participants
Summary
[OTP][LEVEL0]tag andLifecycleBlockingDockerfile.openshift(gzipped to/usr/bin/ingress-node-firewall-tests.gz)go mod vendorruns in Dockerfile before test binary buildFiles Changed
test/cmd/main.go— OTE entry point with 4 suites (parallel, serial, slow, all),LifecycleBlockingfor all specstest/e2e/operator/operator.go— LEVEL0 test: validates operator namespace, CRDs, and deployment readinesstest/e2e/cli.go/test/e2e/util.go— OC client helper and utilitiestest/Makefile— Buildsingress-node-firewall-testsbinaryMakefile— Addsbuild-e2e-teststarget delegating totest/MakefileDockerfile.openshift— Builds and gzips test binary into the operator imagemanifests/stable/image-references— Addstestextension.redhat.io/componentandtestextension.redhat.io/binaryannotations for non-payload OTE discoverygo.mod/go.sum— OTE and ginkgo dependenciesNext Steps
Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Depends-On: openshift/origin#31245