CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 test case#694
Open
anuragthehatter wants to merge 2 commits into
Open
CORENET-6822: OTE framework for Ingress Node Firewall with LEVEL0 test case#694anuragthehatter wants to merge 2 commits into
anuragthehatter wants to merge 2 commits into
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anuragthehatter 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 |
Author
|
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.
Comment on lines
+52
to
+59
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | ||
| ns := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
| return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | ||
| } |
There was a problem hiding this comment.
CreateNamespace should handle AlreadyExists for idempotency.
Unlike the existing namespaces.Create in test/e2e/namespaces/namespaces.go (lines 32-42), this function returns an error if the namespace already exists. This will cause test failures on reruns or when the namespace pre-exists.
Proposed fix
+import k8serrors "k8s.io/apimachinery/pkg/api/errors"
+
// CreateNamespace creates a namespace with the given name
func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) {
ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
- return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{})
+ created, err := client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{})
+ if k8serrors.IsAlreadyExists(err) {
+ return client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{})
+ }
+ return created, err
}📝 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.
Suggested change
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | |
| ns := &corev1.Namespace{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| } | |
| return client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | |
| } | |
| func CreateNamespace(ctx context.Context, client kubernetes.Interface, name string) (*corev1.Namespace, error) { | |
| ns := &corev1.Namespace{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| }, | |
| } | |
| created, err := client.CoreV1().Namespaces().Create(ctx, ns, metav1.CreateOptions{}) | |
| if k8serrors.IsAlreadyExists(err) { | |
| return client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{}) | |
| } | |
| return created, err | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 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.
Comment on lines
+91
to
+92
| if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas && | ||
| deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { |
There was a problem hiding this comment.
Potential nil pointer dereference on Spec.Replicas.
deployment.Spec.Replicas can be nil (defaults to 1 when unset). Dereferencing without a nil check will panic.
Proposed fix
+func int32PtrDerefOr(ptr *int32, def int32) int32 {
+ if ptr != nil {
+ return *ptr
+ }
+ return def
+}
+
// WaitForDeploymentReady waits for a deployment to be ready
func WaitForDeploymentReady(ctx context.Context, client kubernetes.Interface, namespace, deploymentName string) error {
return wait.PollUntilContextTimeout(ctx, DefaultInterval, DefaultTimeout, true, func(ctx context.Context) (bool, error) {
deployment, err := client.AppsV1().Deployments(namespace).Get(ctx, deploymentName, metav1.GetOptions{})
if err != nil {
return false, err
}
- if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas &&
- deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas {
+ replicas := int32PtrDerefOr(deployment.Spec.Replicas, 1)
+ if deployment.Status.ReadyReplicas == replicas &&
+ deployment.Status.UpdatedReplicas == replicas {
return true, nil
}
return false, nil
})
}📝 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.
Suggested change
| if deployment.Status.ReadyReplicas == *deployment.Spec.Replicas && | |
| deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas { | |
| func int32PtrDerefOr(ptr *int32, def int32) int32 { | |
| if ptr != nil { | |
| return *ptr | |
| } | |
| return def | |
| } | |
| // WaitForDeploymentReady waits for a deployment to be ready | |
| func WaitForDeploymentReady(ctx context.Context, client kubernetes.Interface, namespace, deploymentName string) error { | |
| return wait.PollUntilContextTimeout(ctx, DefaultInterval, DefaultTimeout, true, func(ctx context.Context) (bool, error) { | |
| deployment, err := client.AppsV1().Deployments(namespace).Get(ctx, deploymentName, metav1.GetOptions{}) | |
| if err != nil { | |
| return false, err | |
| } | |
| replicas := int32PtrDerefOr(deployment.Spec.Replicas, 1) | |
| if deployment.Status.ReadyReplicas == replicas && | |
| deployment.Status.UpdatedReplicas == replicas { | |
| return true, nil | |
| } | |
| return false, nil | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 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.
|
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 |
anuragthehatter
force-pushed
the
infw_ote
branch
from
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.
Author
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 :( |
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
zhaozhanqi
reviewed
Jan 29, 2026
anuragthehatter
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
anuragthehatter
force-pushed
the
infw_ote
branch
from
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
Author
|
/remove-lifecycle stale |
openshift-ci
Bot
removed
the
lifecycle/stale
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/Makefile (1)
34-37: 💤 Low valueConsider building the package (
./cmd) instead of a single file.
go build ... ./cmd/main.goonly compiles that one file; if a second file is later added tocmd, the build breaks confusingly. Building the package is more robust.♻️ Proposed change
CGO_ENABLED=0 GO_COMPLIANCE_POLICY="exempt_all" $(GO) build \ -a -mod=mod \ -ldflags "$(LDFLAGS)" \ - -o $(BINARY_PATH) ./cmd/main.go + -o $(BINARY_PATH) ./cmd🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/Makefile` around lines 34 - 37, The Makefile currently invokes go build on the single file "./cmd/main.go" which will break if additional files are added; change the build invocation in the target that sets CGO_ENABLED/GO_COMPLIANCE_POLICY/$(GO) to build the package path "./cmd" instead of the single file. Locate the rule containing the variables CGO_ENABLED, GO_COMPLIANCE_POLICY, $(GO) build and the -o $(BINARY_PATH) invocation and replace "./cmd/main.go" with "./cmd" so the package is compiled as a whole.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/Makefile`:
- Around line 23-37: The Makefile is injecting linker -X values for
github.com/openshift/ingress-node-firewall/test/version.{Version,GitCommit,BuildDate}
but there is no package test/version with exported Version/GitCommit/BuildDate
variables to receive them; add a new package "test/version" that declares
exported string vars Version, GitCommit, BuildDate (or update the LDFLAGS to
point at an existing package) so the values passed by the LDFLAGS in the
build-e2e-tests target actually get embedded into the binary; ensure the package
path and symbol names exactly match the -X flags used in LDFLAGS.
---
Nitpick comments:
In `@test/Makefile`:
- Around line 34-37: The Makefile currently invokes go build on the single file
"./cmd/main.go" which will break if additional files are added; change the build
invocation in the target that sets CGO_ENABLED/GO_COMPLIANCE_POLICY/$(GO) to
build the package path "./cmd" instead of the single file. Locate the rule
containing the variables CGO_ENABLED, GO_COMPLIANCE_POLICY, $(GO) build and the
-o $(BINARY_PATH) invocation and replace "./cmd/main.go" with "./cmd" so the
package is compiled as a whole.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a7431ad-96b9-492f-a937-9ff2c3ae623a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Dockerfile.openshiftMakefilego.modtest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/operator/operator.gotest/e2e/util.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/operator/operator.go
- Dockerfile.openshift
- go.mod
Comment on lines
+23
to
+37
| LDFLAGS += -X github.com/openshift/ingress-node-firewall/test/version.Version=$(VERSION) | ||
| LDFLAGS += -X github.com/openshift/ingress-node-firewall/test/version.GitCommit=$(GIT_COMMIT) | ||
| LDFLAGS += -X github.com/openshift/ingress-node-firewall/test/version.BuildDate=$(BUILD_DATE) | ||
|
|
||
| .PHONY: all | ||
| all: build-e2e-tests | ||
|
|
||
| .PHONY: build-e2e-tests | ||
| build-e2e-tests: ## Build the extended e2e test binary (static, ART compliant) | ||
| @echo "Building $(BINARY_NAME)..." | ||
| @mkdir -p $(BUILD_DIR) | ||
| CGO_ENABLED=0 GO_COMPLIANCE_POLICY="exempt_all" $(GO) build \ | ||
| -a -mod=mod \ | ||
| -ldflags "$(LDFLAGS)" \ | ||
| -o $(BINARY_PATH) ./cmd/main.go |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the version package and confirm the three exported vars exist.
fd -t f -e go . test/version 2>/dev/null
rg -nP '\b(Version|GitCommit|BuildDate)\b\s*(=|string)' -g '*.go' test/version 2>/dev/null || \
echo "test/version package or vars not found"Repository: openshift/ingress-node-firewall
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test/version directory listing (if present) =="
ls -la test/version 2>/dev/null || echo "test/version directory not found"
echo
echo "== Go files under test/version =="
fd -t f -e go test/version 2>/dev/null || true
echo
echo "== Find package version declarations under test/ =="
rg -n --hidden --no-ignore '^\s*package\s+version\s*$' -g '*.go' test 2>/dev/null || echo "No 'package version' found under test/"
echo
echo "== Find exported vars/constants for Version/GitCommit/BuildDate =="
rg -n --hidden --no-ignore '^(export\s+)?(var|const)\s+(Version|GitCommit|BuildDate)\b' -g '*.go' test 2>/dev/null || echo "No exported Version/GitCommit/BuildDate declarations found under test/"
echo
echo "== Locate references to github.com/openshift/ingress-node-firewall/test/version =="
rg -n 'github\.com/openshift/ingress-node-firewall/test/version' -S -g '*.go' . 2>/dev/null || echo "No references found"Repository: openshift/ingress-node-firewall
Length of output: 528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for -X injections (LDFLAGS) =="
rg -n --hidden --no-ignore 'LDFLAGS\s*\+?=' -g 'Makefile' . || true
rg -n --hidden --no-ignore '-X\s+github\.com/openshift/ingress-node-firewall/[^ ]+' -g 'Makefile' . || true
rg -n --hidden --no-ignore '-X\s+github\.com/openshift/ingress-node-firewall/test/version' -S -g 'Makefile' . || true
echo
echo "== Search for exported identifiers =="
rg -n --hidden --no-ignore '\b(GitCommit|BuildDate|Version)\b' -S -g '*.go' . || true
echo
echo "== Find any version packages under test/ =="
fd -t d test -a 2>/dev/null | rg '(^|/)version$' || true
fd -t d -a . 2>/dev/null | rg '(^|/)test/version$|(^|/)version$' || true
echo
echo "== Search for package name 'version' anywhere =="
rg -n --hidden --no-ignore '^\s*package\s+version\s*$' -g '*.go' . || trueRepository: openshift/ingress-node-firewall
Length of output: 51216
Fix linker -X version injection: test/version package is missing
test/version(and anypackage versionundertest/) doesn’t exist, and there are no exportedVersion,GitCommit, orBuildDatevars to receive the linker values.- The
-X github.com/openshift/ingress-node-firewall/test/version.{Version,GitCommit,BuildDate}=...flags intest/Makefilewill therefore be silently ignored, so the e2e test binary won’t embed version info.
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 31-31: Target body for "build-e2e-tests" exceeds allowed length of 5 lines (7).
(maxbodylength)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/Makefile` around lines 23 - 37, The Makefile is injecting linker -X
values for
github.com/openshift/ingress-node-firewall/test/version.{Version,GitCommit,BuildDate}
but there is no package test/version with exported Version/GitCommit/BuildDate
variables to receive them; add a new package "test/version" that declares
exported string vars Version, GitCommit, BuildDate (or update the LDFLAGS to
point at an existing package) so the values passed by the LDFLAGS in the
build-e2e-tests target actually get embedded into the binary; ensure the package
path and symbol names exactly match the -X flags used in LDFLAGS.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/operator/operator.go (2)
52-55: ⚡ Quick winPrefer
o.ContainSubstringoverstrings.Contains+BeTruefor better failure output.
BeTrue()produces an uninformative message (Expected <bool>: false to be true) on failure, hiding which CRD was missing and the actual output. Asserting the substring directly surfaces the diff.♻️ Proposed refactor
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) }Note: if this is the only remaining use of
strings, drop the now-unused"strings"import on Line 6.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/operator/operator.go` around lines 52 - 55, Replace the boolean substring assertion using strings.Contains with a direct substring matcher for clearer failure output: change the loop that uses o.Expect(strings.Contains(crdOutput, crd)).To(o.BeTrue(), ...) to assert on crdOutput with o.ContainSubstring(crd) (e.g., o.Expect(crdOutput).To(o.ContainSubstring(crd), "CRD %s should be installed", crd)); also remove the "strings" import if it becomes unused after this change.
68-69: 💤 Low valueDrop
fmt.Printlnin favor of Ginkgo output.
fmt.Printlnwrites toos.Stdoutinstead ofGinkgoWriter, so the line isn't tied to the spec in parallel/CI runs. The precedingg.Byalready records the success step, making this redundant.♻️ Proposed change
g.By("SUCCESS - Ingress Node Firewall operator and CRDs installed") - fmt.Println("Operator install and CRDs check successful!")If you remove it, also drop the now-unused
"fmt"import on Line 5.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/operator/operator.go` around lines 68 - 69, Remove the redundant fmt.Println("Operator install and CRDs check successful!") call in the test where g.By("SUCCESS - Ingress Node Firewall operator and CRDs installed") already records the step; this also requires removing the unused "fmt" import at the top of the file so the build won’t complain. Locate the g.By call in operator.go and delete the subsequent fmt.Println line, then remove the "fmt" entry from the import block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 16: The go.mod currently requires github.com/onsi/ginkgo/v2 v2.23.3 but
then replaces it with github.com/openshift/onsi-ginkgo/v2
v2.6.1-0.20241205171354-8006f302fd12; update the module comment near the
require/replace entries to explain why the declared require differs from the
effective replace (e.g., compatibility, forked bugfixes) and ensure the replace
target is intentional, then run go mod vendor to regenerate vendor/modules.txt
so the vendored dependency graph includes the effective module
(github.com/onsi/ginkgo/v2 / github.com/openshift/onsi-ginkgo/v2) and commit the
updated vendor directory.
---
Nitpick comments:
In `@test/e2e/operator/operator.go`:
- Around line 52-55: Replace the boolean substring assertion using
strings.Contains with a direct substring matcher for clearer failure output:
change the loop that uses o.Expect(strings.Contains(crdOutput,
crd)).To(o.BeTrue(), ...) to assert on crdOutput with o.ContainSubstring(crd)
(e.g., o.Expect(crdOutput).To(o.ContainSubstring(crd), "CRD %s should be
installed", crd)); also remove the "strings" import if it becomes unused after
this change.
- Around line 68-69: Remove the redundant fmt.Println("Operator install and CRDs
check successful!") call in the test where g.By("SUCCESS - Ingress Node Firewall
operator and CRDs installed") already records the step; this also requires
removing the unused "fmt" import at the top of the file so the build won’t
complain. Locate the g.By call in operator.go and delete the subsequent
fmt.Println line, then remove the "fmt" entry from the import block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c49da77-9ccf-4fb7-8442-0b0a4b906173
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
Dockerfile.openshiftMakefilego.modmanifests/stable/image-referencestest/Makefiletest/cmd/main.gotest/e2e/cli.gotest/e2e/operator/operator.gotest/e2e/util.go
✅ Files skipped from review due to trivial changes (1)
- manifests/stable/image-references
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/util.go
- Dockerfile.openshift
- test/e2e/cli.go
- test/cmd/main.go
anuragthehatter
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@anuragthehatter: This pull request references CORENET-7206 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 story to target the "5.0.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. |
2 tasks
anuragthehatter
changed the title
|
@anuragthehatter: This pull request references CORENET-6822 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 story to target the "5.0.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. |
…ss-node-firewall Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 7 new test cases migrated from openshift-tests-private into the OTE (OpenShift Tests Extension) framework, alongside the existing LEVEL0 test: - 54714: TCP NodePort Allow/Deny (Baremetal, Disruptive) - 54992: UDP NodePort Allow/Deny (Baremetal) - 55411: ICMP Allow/Deny (single-stack only) - 55410: SCTP Allow/Deny - 54973: Daemon metrics verification - 55414: Multiple CIDRs with multiple rules (single-stack only) - 73844: SSH traffic blocking (single-stack only) Updates suite definitions to use valid CEL qualifier syntax and adds an AWS suite that excludes Baremetal-labeled tests. LEVEL0 tests are LifecycleBlocking; all others are LifecycleInforming. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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