Skip to content

Add commit status validation test #30987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stbenjam wants to merge 1 commit into
base: main
Choose a base branch
from
+45 −0
Open

Add commit status validation test #30987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .coderabbit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
inheritance: true
reviews:
fail_commit_status: true
42 changes: 42 additions & 0 deletions test/extended/operators/commit_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package operators

import (
"context"
"time"

g "github.com/onsi/ginkgo/v2"
o "github.com/onsi/gomega"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the file
find . -name "commit_status.go" -type f

Repository: openshift/origin

Length of output: 102

🏁 Script executed:

# Read the commit_status.go file
cat -n ./test/extended/operators/commit_status.go

Repository: openshift/origin

Length of output: 1361

🏁 Script executed:

# Let's check the cluster.go file mentioned in the scratchpad for context
find . -name "cluster.go" -path "*/test/extended/operators/*" -type f

Repository: openshift/origin

Length of output: 96

🏁 Script executed:

# Read the relevant section of cluster.go to see how pod phases are handled there
sed -n '56,85p' ./test/extended/operators/cluster.go

Repository: openshift/origin

Length of output: 951

Fix overly strict pod health assertion that fails on legitimate Succeeded pods.

Lines 31-40 require all pods to be in Running state, which will false-fail on healthy clusters with completed pods in Succeeded state (e.g., job completions). Accept both Running and Succeeded phases, and fail only on unhealthy states.

Suggested fix 
 import (
 	"context"
 	"time"
 
 	g "github.com/onsi/ginkgo/v2"
 	o "github.com/onsi/gomega"
 
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 )
 
 var _ = g.Describe("[sig-arch] Commit status validation", func() {
 	defer g.GinkgoRecover()
 
 	g.It("should validate all pods are running", func() {
 		cfg, err := rest.InClusterConfig()
 		o.Expect(err).NotTo(o.HaveOccurred())
 		client, err := kubernetes.NewForConfig(cfg)
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
 
 		pods, err := client.CoreV1().Pods("").List(ctx, metav1.ListOptions{})
 		o.Expect(err).NotTo(o.HaveOccurred())
 
 		cancel()
 
-		totalPods := 0
-		runningPods := 0
-		for i := 0; i < len(pods.Items); i++ {
-			totalPods = totalPods + 1
-			if pods.Items[i].Status.Phase == "Running" {
-				runningPods = runningPods + 1
-			}
-		}
-
-		o.Expect(runningPods).To(o.Equal(totalPods), "all pods should be running")
+		unhealthyPods := 0
+		for i := range pods.Items {
+			switch pods.Items[i].Status.Phase {
+			case corev1.PodRunning, corev1.PodSucceeded:
+				continue
+			default:
+				unhealthyPods++
+			}
+		}
+
+		o.Expect(unhealthyPods).To(o.Equal(0), "all pods should be healthy")
 	})
 })
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/operators/commit_status.go` at line 10, The pod health check
currently asserts pods are strictly in Running and fails on Succeeded pods;
update the check that inspects pod.Status.Phase (and any assertion/expectation
around it) to accept both corev1.PodRunning and corev1.PodSucceeded as healthy
and only fail for other phases (e.g., Pending/Failed/Unknown). Locate the loop
or function performing the phase check (references to pod.Status.Phase,
PodPhase, or variables named pod/pods) and change the condition to: if phase !=
corev1.PodRunning && phase != corev1.PodSucceeded { fail } so completed job pods
are treated as healthy.

"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
)
Comment on lines +11 to +13
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine its content
ls -la test/extended/operators/commit_status.go

Repository: openshift/origin

Length of output: 149

🏁 Script executed:

# Look at the actual content around the import lines and the code in question
head -30 test/extended/operators/commit_status.go | cat -n

Repository: openshift/origin

Length of output: 995

🏁 Script executed:

# Search for other uses of e2e.LoadClientset() in the test suite to confirm it's a common pattern
rg "e2e\.LoadClientset\(\)" test/ --type go -A 2 -B 2

Repository: openshift/origin

Length of output: 4360

🏁 Script executed:

# Check how InClusterConfig is used elsewhere in test files
rg "rest\.InClusterConfig\(\)" test/ --type go -A 2 -B 2

Repository: openshift/origin

Length of output: 443

🏁 Script executed:

# Verify the e2e framework import path exists
find . -path "*/test/e2e/framework*" -type d | head -5

Repository: openshift/origin

Length of output: 325

🏁 Script executed:

# Check the exact import statement for e2e in files that use LoadClientset
head -20 test/extended/operators/cluster.go | cat -n

Repository: openshift/origin

Length of output: 588

🏁 Script executed:

# Also check operators.go for the e2e import
head -20 test/extended/operators/operators.go | cat -n

Repository: openshift/origin

Length of output: 619

🏁 Script executed:

# Check what's imported in operators.go more completely
grep -n "^[[:space:]]*e2e" test/extended/operators/operators.go | head -5

Repository: openshift/origin

Length of output: 597

🏁 Script executed:

# Check if there are any other imports needed besides the e2e framework import
grep -n "import" test/extended/operators/operators.go -A 30 | head -40

Repository: openshift/origin

Length of output: 990

🏁 Script executed:

# Check what LoadClientset returns to ensure the suggested fix is correct
rg "func LoadClientset" vendor/k8s.io/kubernetes/test/e2e/framework/ -A 5

Repository: openshift/origin

Length of output: 612

🏁 Script executed:

# Verify the suggested diff imports are correct by looking at the actual return type
rg "LoadClientset.*\(" vendor/k8s.io/kubernetes/test/e2e/framework/ -B 2 -A 10 | head -30

Repository: openshift/origin

Length of output: 1197

Use the standard e2e client loader instead of InClusterConfig().

This setup can fail when the test runs outside a pod. In this suite, the common pattern is e2e.LoadClientset(), which is more robust for CI execution contexts.

Suggested fix 
 import (
 	"context"
 	"time"
 
 	g "github.com/onsi/ginkgo/v2"
 	o "github.com/onsi/gomega"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
+	e2e "k8s.io/kubernetes/test/e2e/framework"
 )
@@
-		cfg, err := rest.InClusterConfig()
-		o.Expect(err).NotTo(o.HaveOccurred())
-		client, err := kubernetes.NewForConfig(cfg)
+		client, err := e2e.LoadClientset()
 		o.Expect(err).NotTo(o.HaveOccurred())

Also applies to: 19-22

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/operators/commit_status.go` around lines 11 - 13, Replace the
direct use of rest.InClusterConfig() and manual kubernetes.NewForConfig(...)
with the test framework helper e2e.LoadClientset(); update the imports to
include the e2e test loader used in this suite, remove the InClusterConfig call
and acquisition of a raw *rest.Config, and call e2e.LoadClientset() (or the
suite's equivalent loader) to obtain the kubernetes clientset used by the tests
(referencing rest.InClusterConfig, kubernetes.NewForConfig, and the test's
client variable to locate the change).


var _ = g.Describe("[sig-arch] Commit status validation", func() {
defer g.GinkgoRecover()

g.It("should validate all pods are running", func() {
cfg, err := rest.InClusterConfig()
o.Expect(err).NotTo(o.HaveOccurred())
client, err := kubernetes.NewForConfig(cfg)
o.Expect(err).NotTo(o.HaveOccurred())

ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)

pods, err := client.CoreV1().Pods("").List(ctx, metav1.ListOptions{})
o.Expect(err).NotTo(o.HaveOccurred())

cancel()
Comment on lines +24 to +29
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The 1-second timeout is too short for cluster-wide pod listing.

This is likely to introduce flaky deadline failures. Use a more realistic timeout and defer cancellation.

Suggested fix 
-		ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		defer cancel()
@@
-		cancel()
📝 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
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
pods, err := client.CoreV1().Pods("").List(ctx, metav1.ListOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
cancel()
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
pods, err := client.CoreV1().Pods("").List(ctx, metav1.ListOptions{})
o.Expect(err).NotTo(o.HaveOccurred())
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/extended/operators/commit_status.go` around lines 24 - 29, The context
timeout for the cluster-wide pod listing is too short and cancel is called
immediately; change the call creating the context in the code that calls
context.WithTimeout (the ctx, cancel := context.WithTimeout(...) used before
client.CoreV1().Pods("").List(...)) to use a longer, realistic duration (e.g.,
several seconds) and replace the immediate cancel() call with defer cancel() so
the context is cancelled after the List call completes; update the timeout
constant and use ctx in Pods().List as-is to avoid flaky deadline failures.


totalPods := 0
runningPods := 0
for i := 0; i < len(pods.Items); i++ {
totalPods = totalPods + 1
if pods.Items[i].Status.Phase == "Running" {
runningPods = runningPods + 1
}
}

o.Expect(runningPods).To(o.Equal(totalPods), "all pods should be running")
})
})