Fixing oc client cases for microshift and metal ipv6 profliles#2192
Open
gangwgr wants to merge 1 commit intoopenshift:mainfrom
Open
Fixing oc client cases for microshift and metal ipv6 profliles#2192gangwgr wants to merge 1 commit intoopenshift:mainfrom
gangwgr wants to merge 1 commit intoopenshift:mainfrom
Conversation
WalkthroughAdds extensive OpenShift CLI end-to-end tests and helpers: new e2e test suites, a large CLI/testing utility layer, embedded fixture extraction utilities, and many YAML test manifests (CRDs, CRs, pods, templates, quotas, image mirrorsets) used by those tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/payload 4.22 nightly blocking |
Contributor
|
@gangwgr: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ac0a49c0-fdc7-11f0-88f6-772b4a411e87-0 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr 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 |
gangwgr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 546-553: The code incorrectly treats the variable output as a file
path by running exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'");
instead, treat output (the byte/string from oc get events) as the data to parse:
remove the exec.Command call and compute result by splitting string(output) into
lines, extracting the first whitespace-separated field of each non-empty line
(e.g., using strings.Split and strings.Fields), joining or scanning those first
fields into a single result string, then run regexp.MatchString("unknown",
result) as before; update references to output and result accordingly.
In `@test/e2e/util.go`:
- Around line 847-853: The function getShouldPruneRSFromCreateTime can panic
when keepNum > totalCompletedRsListNum; add a bounds check to compute a safe end
index (e.g., end := totalCompletedRsListNum - keepNum; if end < 0 { end = 0 })
before slicing, then slice using rsList := totalCompletedRsList[0:end], and only
call sort.Strings and log when end > 0; return the resulting rsList (which will
be an empty slice when keepNum >= totalCompletedRsListNum).
- Around line 913-930: getLatestPayload currently calls http.Get without a
timeout and defers res.Body.Close() after ioutil.ReadAll, risking hangs and
leaking the response body on read errors; replace the call with an http.Client
that has a sensible Timeout (e.g., few seconds) and check err before using res,
then immediately defer res.Body.Close() right after verifying res != nil and err
== nil, and keep the existing error checks for ioutil.ReadAll/json.Unmarshal;
update references inside getLatestPayload accordingly.
In `@test/testdata/oc_cli/case72217/cr-custom-72217.yaml`:
- Around line 1-6: The CR includes an undefined field spec.test which will be
pruned because the CRD crd-customtask-72217.yaml only defines spec.cronSpec,
spec.image, and spec.replicas; update the CR (cr-custom-72217.yaml) to only use
the defined fields or modify the CRD (crd-customtask-72217.yaml) to add
spec.test to the structural schema (or enable preserveUnknownFields) so the
field is accepted—also apply the same fix for cr-cat-72217.yaml and
cr-cron-72217.yaml where spec.test is present.
In `@test/testdata/oc_cli/initContainer.yaml`:
- Around line 8-20: Add a securityContext to both the initContainer "wait" and
the main container "hello-pod" to satisfy restricted PodSecurity/SCC rules: set
allowPrivilegeEscalation: false, ensure runAsNonRoot: true (and a non-root
runAsUser if needed), and enable readOnlyRootFilesystem where appropriate;
update the initContainers and containers entries to include these
securityContext fields so the pod is accepted under restricted policies and
matches other fixtures like debugpod_48681.yaml.
🧹 Nitpick comments (7)
test/testdata/fixtures.go (1)
117-128: ListFixtures silently ignores WalkDir errors.If
fs.WalkDirencounters an error, it's returned but never checked or logged after the function completes. This could hide issues during test debugging.♻️ Suggested fix
func ListFixtures() []string { var fixtures []string - fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { + err := fs.WalkDir(embeddedFixtures, ".", func(path string, d fs.DirEntry, err error) error { if err != nil { return err } if !d.IsDir() { fixtures = append(fixtures, path) } return nil }) + if err != nil { + panic(fmt.Sprintf("failed to list fixtures: %v", err)) + } return fixtures }test/e2e/cli.go (1)
484-508: Unconventional assertion pattern may cause confusing test failures.Using
o.Expect(...).To(o.BeTrue())insideifconditions is unusual. If the expectation fails, Gomega will fail the test immediately regardless of theiflogic, making the control flow misleading.♻️ Suggested refactor
- if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) { - if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) { + if strings.Contains(architectureStr, "x86_64") { + o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue(), "Expected amd64 arch for x86_64 platform") e2e.Logf("Found the expected Arch amd64") - } else { - e2e.Failf("Failed to find the expected Arch for mirrored image") - } - } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) { + } else if strings.Contains(architectureStr, "aarch64") { + o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue(), "Expected arm64 arch for aarch64 platform") - if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) { e2e.Logf("Found the expected Arch aarch64") - } else { - e2e.Failf("Failed to find the expected Arch for mirrored image") - } } // ... similar pattern for other architecturestest/e2e/util.go (5)
11-11: Replace deprecatedio/ioutilpackage.
io/ioutilhas been deprecated since Go 1.16. The functions are now available inioandospackages. This affects multiple usages in this file (lines 918, 1058, 1087, 1151, 1324).Suggested replacements
- "io/ioutil"Then update usages:
ioutil.ReadAll→io.ReadAllioutil.ReadFile→os.ReadFileioutil.ReadDir→os.ReadDirioutil.WriteFile→os.WriteFile
622-630: Remove duplicategetRandomStringfunction.This function duplicates
GetRandomStringdefined at line 363. Use the exported version instead to avoid maintaining duplicate code.Suggested fix
-func getRandomString() string { - chars := "abcdefghijklmnopqrstuvwxyz0123456789" - seed := rand.New(rand.NewSource(time.Now().UnixNano())) - buffer := make([]byte, 8) - for index := range buffer { - buffer[index] = chars[seed.Intn(len(chars))] - } - return string(buffer) -}Replace any calls to
getRandomString()withGetRandomString().
477-488: Consider migrating from deprecatedwait.Polltowait.PollUntilContextTimeout.
wait.Pollis deprecated and doesn't support context cancellation. This pattern appears throughout the file. Line 933 already uses the newerwait.PollUntilContextTimeoutcorrectly - consider applying the same pattern to other poll usages for consistency and proper cancellation support.Example migration
- err := wait.Poll(5*time.Second, 5*time.Minute, func() (bool, error) { + err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { output, err := c.AsAdmin().WithoutNamespace().Run("get").Args("pod", podName, "-n", namespace, "-o=jsonpath={.status.conditions[?(@.type=='Ready')].status}").Output()
1047-1049: Simplify file reading - avoid unnecessary shell command.Using
exec.Command("bash", "-c", "cat ...")to read a file is unnecessarily complex whenos.ReadFilecan be used directly.Suggested fix
oauthFilePath := dirName + "/.dockerconfigjson" - secretContent, err := exec.Command("bash", "-c", fmt.Sprintf("cat %v", oauthFilePath)).Output() + secretContent, err := os.ReadFile(oauthFilePath) o.Expect(err).NotTo(o.HaveOccurred())
1322-1343: Temp file is never cleaned up.The temp file created at line 1323 is never deleted, which can cause temp directory to fill up during long test runs. Consider returning a cleanup function or using
os.CreateTempwith proper cleanup.Suggested approach
Either document that callers must clean up the returned file path, or use a pattern like:
// At the call site, ensure cleanup: tmpFile, err := applyResourceFromTemplate48681(oc, args...) if err == nil { defer os.Remove(tmpFile) }Or modify the function to accept a cleanup callback.
Comment on lines
+546
to
+555
| if match, _ := regexp.MatchString("No resources found", string(output)); match { | ||
| e2e.Logf("No events in project: %v", projectN) | ||
| } else { | ||
| result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output() | ||
| if match, _ := regexp.MatchString("unknown", string(result)); match { | ||
| e2e.Failf("Does not show timestamp as expected: %v", result) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: cat command used on event output string instead of file path.
The variable output contains the actual events text from oc get events, not a file path. The cat command will fail or produce incorrect results.
🐛 Suggested fix
if match, _ := regexp.MatchString("No resources found", string(output)); match {
e2e.Logf("No events in project: %v", projectN)
} else {
- result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output()
+ // Parse the output directly instead of using cat
+ lines := strings.Split(output, "\n")
+ for _, line := range lines {
+ fields := strings.Fields(line)
+ if len(fields) > 0 && fields[0] == "unknown" {
+ e2e.Failf("Does not show timestamp as expected: %v", line)
+ }
+ }
- if match, _ := regexp.MatchString("unknown", string(result)); match {
- e2e.Failf("Does not show timestamp as expected: %v", result)
- }
}📝 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 match, _ := regexp.MatchString("No resources found", string(output)); match { | |
| e2e.Logf("No events in project: %v", projectN) | |
| } else { | |
| result, _ := exec.Command("bash", "-c", "cat "+output+" | awk '{print $1}'").Output() | |
| if match, _ := regexp.MatchString("unknown", string(result)); match { | |
| e2e.Failf("Does not show timestamp as expected: %v", result) | |
| } | |
| } | |
| if match, _ := regexp.MatchString("No resources found", string(output)); match { | |
| e2e.Logf("No events in project: %v", projectN) | |
| } else { | |
| // Parse the output directly instead of using cat | |
| lines := strings.Split(output, "\n") | |
| for _, line := range lines { | |
| fields := strings.Fields(line) | |
| if len(fields) > 0 && fields[0] == "unknown" { | |
| e2e.Failf("Does not show timestamp as expected: %v", line) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 546 - 553, The code incorrectly treats the
variable output as a file path by running exec.Command("bash", "-c", "cat
"+output+" | awk '{print $1}'"); instead, treat output (the byte/string from oc
get events) as the data to parse: remove the exec.Command call and compute
result by splitting string(output) into lines, extracting the first
whitespace-separated field of each non-empty line (e.g., using strings.Split and
strings.Fields), joining or scanning those first fields into a single result
string, then run regexp.MatchString("unknown", result) as before; update
references to output and result accordingly.
Comment on lines
+847
to
+853
| func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string { | ||
| rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)] | ||
| sort.Strings(rsList) | ||
| e2e.Logf("rsList %v:", rsList) | ||
| return rsList | ||
|
|
||
| } |
There was a problem hiding this comment.
Missing bounds check can cause panic.
If keepNum exceeds totalCompletedRsListNum, the slice operation [0:(totalCompletedRsListNum - keepNum)] will panic with a negative index.
Suggested fix
func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string {
+ if keepNum >= totalCompletedRsListNum {
+ return []string{}
+ }
rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)]📝 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 getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string { | |
| rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)] | |
| sort.Strings(rsList) | |
| e2e.Logf("rsList %v:", rsList) | |
| return rsList | |
| } | |
| func getShouldPruneRSFromCreateTime(totalCompletedRsList []string, totalCompletedRsListNum int, keepNum int) []string { | |
| if keepNum >= totalCompletedRsListNum { | |
| return []string{} | |
| } | |
| rsList := totalCompletedRsList[0:(totalCompletedRsListNum - keepNum)] | |
| sort.Strings(rsList) | |
| e2e.Logf("rsList %v:", rsList) | |
| return rsList | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 847 - 853, The function
getShouldPruneRSFromCreateTime can panic when keepNum > totalCompletedRsListNum;
add a bounds check to compute a safe end index (e.g., end :=
totalCompletedRsListNum - keepNum; if end < 0 { end = 0 }) before slicing, then
slice using rsList := totalCompletedRsList[0:end], and only call sort.Strings
and log when end > 0; return the resulting rsList (which will be an empty slice
when keepNum >= totalCompletedRsListNum).
Comment on lines
+913
to
+930
| func getLatestPayload(url string) string { | ||
| res, err := http.Get(url) | ||
| if err != nil { | ||
| e2e.Failf("unable to get http with error: %v", err) | ||
| } | ||
| body, err := ioutil.ReadAll(res.Body) | ||
| defer res.Body.Close() | ||
| if err != nil { | ||
| e2e.Failf("unable to parse the http result with error: %v", err) | ||
| } | ||
|
|
||
| var data map[string]interface{} | ||
| if err := json.Unmarshal(body, &data); err != nil { | ||
| e2e.Failf("unable to parse JSON with error: %v", err) | ||
| } | ||
| pullSpec, _ := data["pullSpec"].(string) | ||
| return pullSpec | ||
| } |
There was a problem hiding this comment.
HTTP request lacks timeout and has potential resource leak.
http.Gethas no timeout and can hang tests indefinitely if the server doesn't respond.defer res.Body.Close()is placed afterioutil.ReadAll, so ifReadAllfails, the body won't be closed.
Suggested fix
func getLatestPayload(url string) string {
- res, err := http.Get(url)
+ client := &http.Client{Timeout: 30 * time.Second}
+ res, err := client.Get(url)
if err != nil {
e2e.Failf("unable to get http with error: %v", err)
}
- body, err := ioutil.ReadAll(res.Body)
defer res.Body.Close()
+ body, err := io.ReadAll(res.Body)
if err != nil {
e2e.Failf("unable to parse the http result with error: %v", 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 getLatestPayload(url string) string { | |
| res, err := http.Get(url) | |
| if err != nil { | |
| e2e.Failf("unable to get http with error: %v", err) | |
| } | |
| body, err := ioutil.ReadAll(res.Body) | |
| defer res.Body.Close() | |
| if err != nil { | |
| e2e.Failf("unable to parse the http result with error: %v", err) | |
| } | |
| var data map[string]interface{} | |
| if err := json.Unmarshal(body, &data); err != nil { | |
| e2e.Failf("unable to parse JSON with error: %v", err) | |
| } | |
| pullSpec, _ := data["pullSpec"].(string) | |
| return pullSpec | |
| } | |
| func getLatestPayload(url string) string { | |
| client := &http.Client{Timeout: 30 * time.Second} | |
| res, err := client.Get(url) | |
| if err != nil { | |
| e2e.Failf("unable to get http with error: %v", err) | |
| } | |
| defer res.Body.Close() | |
| body, err := io.ReadAll(res.Body) | |
| if err != nil { | |
| e2e.Failf("unable to parse the http result with error: %v", err) | |
| } | |
| var data map[string]interface{} | |
| if err := json.Unmarshal(body, &data); err != nil { | |
| e2e.Failf("unable to parse JSON with error: %v", err) | |
| } | |
| pullSpec, _ := data["pullSpec"].(string) | |
| return pullSpec | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 913 - 930, getLatestPayload currently calls
http.Get without a timeout and defers res.Body.Close() after ioutil.ReadAll,
risking hangs and leaking the response body on read errors; replace the call
with an http.Client that has a sensible Timeout (e.g., few seconds) and check
err before using res, then immediately defer res.Body.Close() right after
verifying res != nil and err == nil, and keep the existing error checks for
ioutil.ReadAll/json.Unmarshal; update references inside getLatestPayload
accordingly.
Comment on lines
+8
to
+20
| initContainers: | ||
| - name: wait | ||
| image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c | ||
| command: ["/bin/sh", "-c", "sleep 10"] | ||
| containers: | ||
| - name: hello-pod | ||
| image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83 | ||
| ports: | ||
| - containerPort: 8080 | ||
| volumeMounts: | ||
| - mountPath: /tmp | ||
| name: tmp | ||
| volumes: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its full content
find . -name "initContainer.yaml" -type fRepository: openshift/oc
Length of output: 97
🏁 Script executed:
# Also search for the file path mentioned in the review
if [ -f "test/testdata/oc_cli/initContainer.yaml" ]; then
cat -n test/testdata/oc_cli/initContainer.yaml
fiRepository: openshift/oc
Length of output: 811
🏁 Script executed:
# Check context: is there a similar test setup in other testdata files?
find test/testdata -name "*.yaml" -type f | head -10Repository: openshift/oc
Length of output: 556
🏁 Script executed:
# Check if there are other pod fixtures in testdata for comparison
grep -l "kind: Pod" test/testdata/oc_cli/*.yaml 2>/dev/null | head -5Repository: openshift/oc
Length of output: 181
🏁 Script executed:
# Examine one or two similar pod fixtures to see if they have securityContext
for f in $(grep -l "kind: Pod" test/testdata/oc_cli/*.yaml 2>/dev/null | head -2); do
echo "=== $f ==="
head -30 "$f"
doneRepository: openshift/oc
Length of output: 1430
🌐 Web query:
Kubernetes Pod securityContext allowPrivilegeEscalation default value
💡 Result:
allowPrivilegeEscalation is a container-level field (spec.containers[*].securityContext.allowPrivilegeEscalation, not spec.securityContext).
- Default (when unset): effectively
true(privilege escalation is allowed unless you explicitly set it tofalse). (dynatrace.com) - Forced behavior: it is always
trueif the container is privileged or hasCAP_SYS_ADMIN, regardless of what you set. (v1-33.docs.kubernetes.io) - Note: Kubernetes Pod Security Standards (restricted) require this field to be set to
false. (kubernetes.io)
Citations:
- 1: https://www.dynatrace.com/engineering/blog/kubernetes-security-part-3-security-context/?utm_source=openai
- 2: https://v1-33.docs.kubernetes.io/docs/tasks/configure-pod-container/security-context/?utm_source=openai
- 3: https://kubernetes.io/docs/concepts/security/pod-security-standards/?utm_source=openai
Add securityContext to keep this pod valid under restricted policies.
Lines 8-20 omit securityContext for both containers. In clusters enforcing Pod Security "restricted" (or strict SCCs), this causes admission failures because allowPrivilegeEscalation defaults to true. Other test fixtures in this repo (e.g., debugpod_48681.yaml) follow this hardening pattern; add it here for consistency and test portability.
Suggested hardening
spec:
+ securityContext:
+ runAsNonRoot: true
initContainers:
- name: wait
image: quay.io/openshifttest/base-alpine@sha256:3126e4eed4a3ebd8bf972b2453fa838200988ee07c01b2251e3ea47e4b1f245c
command: ["/bin/sh", "-c", "sleep 10"]
+ securityContext:
+ allowPrivilegeEscalation: false
containers:
- name: hello-pod
image: quay.io/openshifttest/hello-openshift@sha256:4200f438cf2e9446f6bcff9d67ceea1f69ed07a2f83363b7fb52529f7ddd8a83
ports:
- containerPort: 8080
volumeMounts:
- mountPath: /tmp
name: tmp
+ securityContext:
+ allowPrivilegeEscalation: false🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/initContainer.yaml` around lines 8 - 20, Add a
securityContext to both the initContainer "wait" and the main container
"hello-pod" to satisfy restricted PodSecurity/SCC rules: set
allowPrivilegeEscalation: false, ensure runAsNonRoot: true (and a non-root
runAsUser if needed), and enable readOnlyRootFilesystem where appropriate;
update the initContainers and containers entries to include these
securityContext fields so the pod is accepted under restricted policies and
matches other fixtures like debugpod_48681.yaml.
Contributor
Author
|
/test e2e-aws-ovn-serial-1of2 |
Member
|
It would make sense running the payload informing jobs as well. |
Contributor
Author
|
/payload 4.22 nightly informing |
Contributor
|
@gangwgr: trigger 68 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a03e5980-fff0-11f0-9ec7-55e9a19a1e66-0 |
ardaguclu
reviewed
Feb 2, 2026
|
|
||
| // author: [email protected] | ||
| g.It("Author:yinzhou-NonPreRelease-Longduration-High-45307-Critical-45327-check oc adm prune deployments to prune RS [Serial][Timeout:30m]", func() { | ||
| skipIfMicroShift(oc) |
Member
There was a problem hiding this comment.
Why do we skip this for microshift?
Contributor
Author
There was a problem hiding this comment.
oc adm not support microshift
Member
There was a problem hiding this comment.
You are right. For reference https://github.com/openshift/oc/blob/main/tools/gendocs/gendocs/microshift.go
|
|
||
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-Author:knarra-Medium-48681-Could start debug pod using pod definition yaml", func() { | ||
| skipIfMicroShift(oc) |
Member
There was a problem hiding this comment.
Why do we skip for microshift?
Contributor
Author
There was a problem hiding this comment.
apiVersion: template.openshift.io/v1
kind: Template
metadata:
name: debug-testpod
template api not supported in. microshift
|
|
||
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-Medium-49859-should failed when oc import-image setting with Garbage values for --reference-policy", func() { | ||
| skipIfMicroShift(oc) |
Member
There was a problem hiding this comment.
Why do we skip for microshift?
Contributor
Author
There was a problem hiding this comment.
oc get is
error: the server doesn't have a resource type "is"
rgangwar@rgangwar-mac oc %
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-NonPreRelease-Longduration-Author:yinzhou-High-42982-Describe quota output should always show units [Timeout:30m]", func() { | ||
| skipIfMicroShift(oc) | ||
| skipIfDisconnected(oc) |
Member
There was a problem hiding this comment.
This test should work on microshift and disconnected?
Contributor
Author
There was a problem hiding this comment.
these cases not supported on microshift
Member
There was a problem hiding this comment.
hmmm.. The reason because microshift does not run cluster-policy-controller?
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-High-38178-oc should be able to debug init container", func() { | ||
| skipIfMicroShift(oc) | ||
| skipIfDisconnected(oc) |
Member
There was a problem hiding this comment.
This test should work on disconnected and microshift
|
|
||
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:yinzhou-Medium-51018-oc adm release extract support manifest list", func() { | ||
| skipIfMicroShift(oc) |
Member
There was a problem hiding this comment.
Why do we skip for microshift?
Contributor
Author
There was a problem hiding this comment.
we don't have this ns openshift-config in microshift
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-ConnectedOnly-Author:knarra-Medium-66989-Workloads oc debug with or without init container for pod", func() { | ||
| skipIfMicroShift(oc) | ||
| skipIfDisconnected(oc) |
Member
There was a problem hiding this comment.
same. This test should work anywhere
Contributor
Author
There was a problem hiding this comment.
oc new-project supported
|
|
||
| // author: [email protected] | ||
| g.It("Author:yinzhou-ROSA-OSD_CCS-ARO-NonPreRelease-High-68647-oc whoami must work without oauth-apiserver", func() { | ||
| skipIfMicroShift(oc) |
Member
There was a problem hiding this comment.
This test should work on microshift
Contributor
Author
There was a problem hiding this comment.
oc new-project not supported on microshift
test/e2e/cli.go
Outdated
| // author: [email protected] | ||
| g.It("ROSA-OSD_CCS-ARO-Author:yinzhou-High-10136-Project should only watch its owned cache events", func() { | ||
| skipIfDisconnected(oc) | ||
| skipIfMicroShift(oc) |
Contributor
Author
|
@ardaguclu these cases are not supported on microshift. In our qe repo these also not run on microshift |
Contributor
Author
|
/payload 4.22 nightly informing |
Contributor
Author
|
/payload 4.22 nightly blocking |
Contributor
|
@gangwgr: trigger 68 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3a015580-0047-11f1-97c6-2554e560c965-0 |
Contributor
|
@gangwgr: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/40483cb0-0047-11f1-8625-764fae3be063-0 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 286-288: The defer of cmd.Process.Kill() is added before verifying
err from oc.Run(...).Background(), which can leave cmd non-nil but cmd.Process
nil if Start() failed; update each occurrence (the Background() calls that
assign cmd, _, _, err) to only defer Process.Kill() after confirming err == nil
and cmd.Process != nil — i.e., check err and cmd.Process != nil (or nil-check
cmd.Process) and then defer cmd.Process.Kill(); apply this pattern to the three
instances around the Background() calls in test/e2e/cli.go (the cmd variable
created at the shown diff and the two other spots referenced in the review).
- Around line 129-133: The skip condition around isBaselineCapsSet and
isEnabledCapability has incorrect operator precedence (&& binds tighter than ||)
causing unintended skips; update the if in test/e2e/cli.go so that all baseline
checks (calls to isBaselineCapsSet(oc, "...") for "None", "v4.13", "v4.12",
"v4.14", "v4.15", "v4.11") are grouped with parentheses and then &&
isEnabledCapability(oc, "DeploymentConfig") is negated outside that group (i.e.,
(A || B || C || ...) && !isEnabledCapability(...)) so the g.Skip(skipMsg) only
runs when any baseline is set AND DeploymentConfig is not enabled, leaving
skipMsg, e2e.Warningf and g.Skip unchanged.
- Around line 485-505: The branch conditions incorrectly use
o.Expect(...).To(...) (which panics) instead of plain checks, so replace each
conditional like if o.Expect(strings.Contains(architectureStr,
"x86_64")).To(o.BeTrue()) with plain logic using strings.Contains(e.g. if
strings.Contains(architectureStr, "x86_64") {) and keep the Gomega assertions
only inside the chosen branch to validate imageInfo (e.g. use
o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) or call
e2e.Failf/logging as currently done). Update all branches referencing
architectureStr ("x86_64", "aarch64", "ppc64le", "s390x") and ensure no o.Expect
is used in the if/else-if conditions; only use o.Expect inside branch bodies to
assert imageInfo.
In `@test/e2e/util.go`:
- Around line 973-979: getClusterRegion currently indexes the first worker with
getWorkersList(oc)[0] without guarding for an empty slice; change it to first
assign workers := getWorkersList(oc), then if len(workers) == 0 fail early using
the test expectation (e.g. o.Expect(len(workers)).NotTo(o.Equal(0)) or
o.Expect(err).NotTo...) or return a sensible zero value, and thereafter use
workers[0]; this mirrors the existing guard used elsewhere and prevents a panic
when no worker-labeled nodes exist.
Comment on lines
+129
to
+133
| if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") { | ||
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" | ||
| e2e.Warningf("SKIPPING TEST: %s", skipMsg) | ||
| g.Skip(skipMsg) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/cli.go | sed -n '120,140p'Repository: openshift/oc
Length of output: 1413
🏁 Script executed:
rg -A 5 "func isBaselineCapsSet" test/e2e/cli.goRepository: openshift/oc
Length of output: 38
🏁 Script executed:
rg -A 5 "func isEnabledCapability" test/e2e/cli.goRepository: openshift/oc
Length of output: 38
🏁 Script executed:
find test/e2e -name "*.go" -type f | head -20Repository: openshift/oc
Length of output: 104
🏁 Script executed:
rg "func isBaselineCapsSet|func isEnabledCapability" test/e2e/Repository: openshift/oc
Length of output: 205
🏁 Script executed:
rg -A 10 "func isBaselineCapsSet" test/e2e/util.goRepository: openshift/oc
Length of output: 822
🏁 Script executed:
rg -A 10 "func isEnabledCapability" test/e2e/util.goRepository: openshift/oc
Length of output: 686
🏁 Script executed:
rg -B 2 -A 3 "isBaselineCapsSet.*&&.*isEnabledCapability|isEnabledCapability.*&&.*isBaselineCapsSet" test/e2e/cli.goRepository: openshift/oc
Length of output: 541
🏁 Script executed:
rg -B 1 -A 2 "isBaselineCapsSet.*\|\|.*isBaselineCapsSet" test/e2e/cli.go | head -50Repository: openshift/oc
Length of output: 499
🏁 Script executed:
python3 << 'EOF'
# Go operator precedence (from Go spec):
# In decreasing order:
# Precedence Operator
# 5 * / % << >> & &^
# 4 + - | ^
# 3 == != < <= > >=
# 2 &&
# 1 ||
# Current code analysis:
code = 'isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig")'
# && has precedence 2, || has precedence 1
# So && binds tighter than ||
print("Current code evaluates as:")
print("A || B || C || D || E || (F && !G)")
print()
print("Where:")
print("A = isBaselineCapsSet(oc, 'None')")
print("B = isBaselineCapsSet(oc, 'v4.13')")
print("C = isBaselineCapsSet(oc, 'v4.12')")
print("D = isBaselineCapsSet(oc, 'v4.14')")
print("E = isBaselineCapsSet(oc, 'v4.15')")
print("F = isBaselineCapsSet(oc, 'v4.11')")
print("G = isEnabledCapability(oc, 'DeploymentConfig')")
print()
print("Skip message intent: skip if ANY baseline is set AND capability NOT enabled")
print()
print("Proposed fix evaluates as:")
print("(A || B || C || D || E || F) && !G")
print()
print("This correctly applies the capability check to ALL baseline versions.")
EOFRepository: openshift/oc
Length of output: 593
Fix operator precedence in skip condition so the test doesn't skip unintentionally.
Line 129: && binds tighter than || in Go, so the condition currently evaluates as A || B || C || D || E || (F && !G). This skips the test if any of {None, v4.13, v4.12, v4.14, v4.15} is set, regardless of whether DeploymentConfig is enabled. The skip message and logic suggest the intent is to skip only when any baseline capability is set and DeploymentConfig is not enabled. Wrap all baseline checks in parentheses to apply the capability check uniformly.
Proposed fix
- if isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") {
+ if (isBaselineCapsSet(oc, "None") ||
+ isBaselineCapsSet(oc, "v4.13") ||
+ isBaselineCapsSet(oc, "v4.12") ||
+ isBaselineCapsSet(oc, "v4.14") ||
+ isBaselineCapsSet(oc, "v4.15") ||
+ isBaselineCapsSet(oc, "v4.11")) && !isEnabledCapability(oc, "DeploymentConfig") {
skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!"
e2e.Warningf("SKIPPING TEST: %s", skipMsg)
g.Skip(skipMsg)
}📝 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 isBaselineCapsSet(oc, "None") || isBaselineCapsSet(oc, "v4.13") || isBaselineCapsSet(oc, "v4.12") || isBaselineCapsSet(oc, "v4.14") || isBaselineCapsSet(oc, "v4.15") || isBaselineCapsSet(oc, "v4.11") && !isEnabledCapability(oc, "DeploymentConfig") { | |
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" | |
| e2e.Warningf("SKIPPING TEST: %s", skipMsg) | |
| g.Skip(skipMsg) | |
| } | |
| if (isBaselineCapsSet(oc, "None") || | |
| isBaselineCapsSet(oc, "v4.13") || | |
| isBaselineCapsSet(oc, "v4.12") || | |
| isBaselineCapsSet(oc, "v4.14") || | |
| isBaselineCapsSet(oc, "v4.15") || | |
| isBaselineCapsSet(oc, "v4.11")) && !isEnabledCapability(oc, "DeploymentConfig") { | |
| skipMsg := "Skipping the test as baselinecaps have been set to None and some of API capabilities are not enabled!" | |
| e2e.Warningf("SKIPPING TEST: %s", skipMsg) | |
| g.Skip(skipMsg) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 129 - 133, The skip condition around
isBaselineCapsSet and isEnabledCapability has incorrect operator precedence (&&
binds tighter than ||) causing unintended skips; update the if in
test/e2e/cli.go so that all baseline checks (calls to isBaselineCapsSet(oc,
"...") for "None", "v4.13", "v4.12", "v4.14", "v4.15", "v4.11") are grouped with
parentheses and then && isEnabledCapability(oc, "DeploymentConfig") is negated
outside that group (i.e., (A || B || C || ...) && !isEnabledCapability(...)) so
the g.Skip(skipMsg) only runs when any baseline is set AND DeploymentConfig is
not enabled, leaving skipMsg, e2e.Warningf and g.Skip unchanged.
Comment on lines
+286
to
+289
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | ||
| defer cmd.Process.Kill() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file around the mentioned lines
cat -n test/e2e/cli.go | sed -n '280,295p'Repository: openshift/oc
Length of output: 1019
🏁 Script executed:
# Find all Background() usages in this file to see the pattern
rg "Background\(\)" test/e2e/cli.go -B 1 -A 1Repository: openshift/oc
Length of output: 691
🏁 Script executed:
# Search for Background method definition in the codebase
rg "func.*Background\(\)" --type go -A 5Repository: openshift/oc
Length of output: 2589
🏁 Script executed:
# Read the full Background() implementation
cat -n test/e2e/util.go | sed -n '/func (cmd \*CLICommand) Background()/,/^[0-9]*\t}/p'Repository: openshift/oc
Length of output: 50368
🏁 Script executed:
# Find all Background() usages in cli.go with line numbers and context
rg "Background\(\)" test/e2e/cli.go -B 2 -A 2 -nRepository: openshift/oc
Length of output: 986
Add nil checks before calling Process.Kill() on Background() failures.
The Background() method returns a non-nil *exec.Cmd with a nil Process field if Start() fails. Deferring cmd.Process.Kill() before validating err will panic on any Background() failure. Fix all three instances (lines 286–288, 822–824, and 835–837):
Proposed fix
cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background()
- defer cmd.Process.Kill()
o.Expect(err).NotTo(o.HaveOccurred())
+ if cmd != nil && cmd.Process != nil {
+ defer cmd.Process.Kill()
+ }Apply the same pattern to lines 822 and 835.
📝 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
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | |
| defer cmd.Process.Kill() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| cmd, _, _, err := oc.Run("debug").Args("pod/hello-pod", "--keep-init-containers=true").Background() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| if cmd != nil && cmd.Process != nil { | |
| defer cmd.Process.Kill() | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 286 - 288, The defer of cmd.Process.Kill() is
added before verifying err from oc.Run(...).Background(), which can leave cmd
non-nil but cmd.Process nil if Start() failed; update each occurrence (the
Background() calls that assign cmd, _, _, err) to only defer Process.Kill()
after confirming err == nil and cmd.Process != nil — i.e., check err and
cmd.Process != nil (or nil-check cmd.Process) and then defer cmd.Process.Kill();
apply this pattern to the three instances around the Background() calls in
test/e2e/cli.go (the cmd variable created at the shown diff and the two other
spots referenced in the review).
Comment on lines
+485
to
+506
| if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch amd64") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch aarch64") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) { | ||
| if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch ppc64le") | ||
| } else { | ||
| e2e.Failf("Failed to find the expected Arch for mirrored image") | ||
| } | ||
| } else { | ||
| if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) { | ||
| e2e.Logf("Found the expected Arch s390x") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/e2e && wc -l cli.goRepository: openshift/oc
Length of output: 67
🏁 Script executed:
cd test/e2e && sed -n '485,505p' cli.goRepository: openshift/oc
Length of output: 1038
🏁 Script executed:
# Let's also see more context around these lines
cd test/e2e && sed -n '475,515p' cli.goRepository: openshift/oc
Length of output: 1707
🏁 Script executed:
# Search for how Expect is used elsewhere in the codebase to understand patterns
rg "Expect.*To.*BeTrue" test/ -A 2 -B 2 --max-count=10Repository: openshift/oc
Length of output: 3230
🏁 Script executed:
# Check if there are imports related to Gomega/testing framework
rg "import.*Gomega|import.*ginkgo|import.*gomega" test/e2e/ -A 2 -B 2Repository: openshift/oc
Length of output: 38
🏁 Script executed:
cd test/e2e && head -50 cli.go | grep -E "^import|^package"Repository: openshift/oc
Length of output: 76
🏁 Script executed:
cd test/e2e && rg "var o " cli.go -B 5 -A 5Repository: openshift/oc
Length of output: 38
🏁 Script executed:
cd test/e2e && rg "onsi/gomega|Omega" cli.go -B 2 -A 2Repository: openshift/oc
Length of output: 154
🌐 Web query:
Gomega Expect assertion in if condition branch control flow behavior
💡 Result:
In Gomega, Expect(...).To(...) (and Should, NotTo, etc.) does return a bool (true if the assertion passed, false if it failed). (pkg.go.dev)
However, whether your code actually reaches the if branch after a failure depends on the registered fail handler:
- With Ginkgo, failed Gomega assertions call
ginkgo.Fail(...), and Ginkgo’sFailrecords the failure and then panics to stop further test execution. So an assertion failure will typically abort control flow immediately, and you won’t reliably “branch on the returned bool.” (pkg.go.dev) - If you register a non-panicking fail handler (custom handler), then the
boolreturn can be used for branching—but that’s not Ginkgo’s default behavior. (stackoverflow.com)
Practical implication: in normal Ginkgo+Gomega usage, treat Expect(...).To(...) as terminal on failure, not as a boolean predicate for if/else logic.
Citations:
- 1: https://pkg.go.dev/github.com/onsi/gomega
- 2: https://pkg.go.dev/github.com/onsi/ginkgo?utm_source=openai
- 3: https://stackoverflow.com/questions/61180950/using-an-assertion-library-with-godog-test-framework?utm_source=openai
🏁 Script executed:
cd test/e2e && sed -n '470,520p' cli.goRepository: openshift/oc
Length of output: 2163
Remove Expect assertions from branch conditions; use plain logic for control flow.
In Ginkgo+Gomega, Expect(...).To(...) panics on failure, which aborts test execution. Using it in if/else-if conditions (lines 485, 491, 497) causes the test to fail and exit on the first assertion failure, never reaching other branches. This breaks the intended branching logic based on architecture.
Use plain strings.Contains() to determine which branch to take, and place Expect assertions only inside the branches to validate the expected behavior:
Proposed fix
- if o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch amd64")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch aarch64")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) {
- if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch ppc64le")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- } else {
- if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) {
- e2e.Logf("Found the expected Arch s390x")
- } else {
- e2e.Failf("Failed to find the expected Arch for mirrored image")
- }
- }
+ switch {
+ case strings.Contains(architectureStr, "x86_64"):
+ o.Expect(imageInfo).To(o.ContainSubstring("amd64"))
+ case strings.Contains(architectureStr, "aarch64"):
+ o.Expect(imageInfo).To(o.ContainSubstring("arm64"))
+ case strings.Contains(architectureStr, "ppc64le"):
+ o.Expect(imageInfo).To(o.ContainSubstring("ppc64le"))
+ default:
+ o.Expect(imageInfo).To(o.ContainSubstring("s390x"))
+ }📝 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 o.Expect(strings.Contains(architectureStr, "x86_64")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch amd64") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else if o.Expect(strings.Contains(architectureStr, "aarch64")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "arm64")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch aarch64") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else if o.Expect(strings.Contains(architectureStr, "ppc64le")).To(o.BeTrue()) { | |
| if o.Expect(strings.Contains(imageInfo, "ppc64le")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch ppc64le") | |
| } else { | |
| e2e.Failf("Failed to find the expected Arch for mirrored image") | |
| } | |
| } else { | |
| if o.Expect(strings.Contains(imageInfo, "s390x")).To(o.BeTrue()) { | |
| e2e.Logf("Found the expected Arch s390x") | |
| switch { | |
| case strings.Contains(architectureStr, "x86_64"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("amd64")) | |
| case strings.Contains(architectureStr, "aarch64"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("arm64")) | |
| case strings.Contains(architectureStr, "ppc64le"): | |
| o.Expect(imageInfo).To(o.ContainSubstring("ppc64le")) | |
| default: | |
| o.Expect(imageInfo).To(o.ContainSubstring("s390x")) | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 485 - 505, The branch conditions incorrectly
use o.Expect(...).To(...) (which panics) instead of plain checks, so replace
each conditional like if o.Expect(strings.Contains(architectureStr,
"x86_64")).To(o.BeTrue()) with plain logic using strings.Contains(e.g. if
strings.Contains(architectureStr, "x86_64") {) and keep the Gomega assertions
only inside the chosen branch to validate imageInfo (e.g. use
o.Expect(strings.Contains(imageInfo, "amd64")).To(o.BeTrue()) or call
e2e.Failf/logging as currently done). Update all branches referencing
architectureStr ("x86_64", "aarch64", "ppc64le", "s390x") and ensure no o.Expect
is used in the if/else-if conditions; only use o.Expect inside branch bodies to
assert imageInfo.
Comment on lines
+973
to
+979
| func getClusterRegion(oc *CLI) string { | ||
| e2e.Logf("Getting cluster region") | ||
| node := getWorkersList(oc)[0] | ||
| region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| e2e.Logf("Cluster region: %s", region) | ||
| return region |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/e2e && grep -n "func getWorkersList" util.goRepository: openshift/oc
Length of output: 100
🏁 Script executed:
cd test/e2e && grep -n "getClusterRegion" util.goRepository: openshift/oc
Length of output: 135
🏁 Script executed:
cd test/e2e && rg -A 10 "func getWorkersList"Repository: openshift/oc
Length of output: 432
🏁 Script executed:
cd test/e2e && sed -n '990,1010p' util.goRepository: openshift/oc
Length of output: 816
Guard against empty worker list before indexing.
Line 975 indexes [0] without checking for workers, which will panic on clusters without worker-labeled nodes. Note that the code already handles this case properly elsewhere in the same context (line 998-1003).
Proposed fix
func getClusterRegion(oc *CLI) string {
e2e.Logf("Getting cluster region")
- node := getWorkersList(oc)[0]
+ workers := getWorkersList(oc)
+ if len(workers) == 0 {
+ e2e.Warningf("No worker nodes found; defaulting region to unknown")
+ return "unknown"
+ }
+ node := workers[0]
region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output()
o.Expect(err).NotTo(o.HaveOccurred())
e2e.Logf("Cluster region: %s", region)
return region
}📝 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 getClusterRegion(oc *CLI) string { | |
| e2e.Logf("Getting cluster region") | |
| node := getWorkersList(oc)[0] | |
| region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| e2e.Logf("Cluster region: %s", region) | |
| return region | |
| func getClusterRegion(oc *CLI) string { | |
| e2e.Logf("Getting cluster region") | |
| workers := getWorkersList(oc) | |
| if len(workers) == 0 { | |
| e2e.Warningf("No worker nodes found; defaulting region to unknown") | |
| return "unknown" | |
| } | |
| node := workers[0] | |
| region, err := oc.AsAdmin().WithoutNamespace().Run("get").Args("node", node, "-o=jsonpath={.metadata.labels.failure-domain\\.beta\\.kubernetes\\.io\\/region}").Output() | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| e2e.Logf("Cluster region: %s", region) | |
| return region | |
| } |
🤖 Prompt for AI Agents
In `@test/e2e/util.go` around lines 973 - 979, getClusterRegion currently indexes
the first worker with getWorkersList(oc)[0] without guarding for an empty slice;
change it to first assign workers := getWorkersList(oc), then if len(workers) ==
0 fail early using the test expectation (e.g.
o.Expect(len(workers)).NotTo(o.Equal(0)) or o.Expect(err).NotTo...) or return a
sensible zero value, and thereafter use workers[0]; this mirrors the existing
guard used elsewhere and prevents a panic when no worker-labeled nodes exist.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/e2e/cli.go`:
- Around line 233-237: Replace the hard-coded temp directory string
(extractTmpDirName := "/tmp/case51018") with a call to os.MkdirTemp to create a
unique temporary directory, assign its returned path to extractTmpDirName, check
the error and keep the existing defer os.RemoveAll(extractTmpDirName); update
the error expectation (err) handling as used in the surrounding test
(o.Expect(err).NotTo(o.HaveOccurred())); apply the same change to the other
fixed /tmp usages referenced in the review (the other test temp dirs at the
noted locations) so each test uses os.MkdirTemp with a unique prefix and defers
os.RemoveAll.
- Around line 365-372: The test builds shell commands (sedCmd, sedCmdOne) using
cluster-derived values (imageRegistryName and pullSpec parts) and executes them
via exec.Command("bash", "-c", ...), which risks command injection; instead,
locate the file referenced by idmsFile64921, read its contents in Go, perform
safe string replacements for "localhost" -> imageRegistryName and "target" ->
strings.Split(pullSpec, "/")[1] (or compute the intended segment into a variable
first), and write the modified content back to idmsFile64921, replacing the
exec.Command calls and o.Expect(err).NotTo(HaveOccurred()) checks with proper Go
file IO error handling in the same test function.
In `@test/testdata/oc_cli/case72217/cr-cat-72217.yaml`:
- Line 1: The apiVersion string in the manifest is mistyped as
"bate.example.com/v1"; update the apiVersion value to "beta.example.com/v1" so
that the resource uses the correct group/version identifier (look for the
apiVersion field in the YAML containing "bate.example.com/v1" and replace "bate"
with "beta").
In `@test/testdata/oc_cli/deploymentconfig_with_quota.yaml`:
- Line 42: The memory unit in the deployment config is inconsistent: change the
memory value from "40M" to "40Mi" (match the other container's "40Mi") to ensure
consistent binary units for quota calculations; update the memory: 40M entry so
both containers use "40Mi".
🧹 Nitpick comments (1)
test/testdata/oc_cli/initContainer66989.yaml (1)
1-22: Same securityContext gap asinitContainer.yaml.Consider adding consistent securityContext settings across both init container test fixtures for portability on restricted clusters.
Comment on lines
+233
to
+237
| extractTmpDirName := "/tmp/case51018" | ||
| err := os.MkdirAll(extractTmpDirName, 0700) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer os.RemoveAll(extractTmpDirName) | ||
|
|
There was a problem hiding this comment.
Use unique temp directories to avoid cross-test collisions.
Fixed /tmp/... paths can collide in parallel runs and can be pre-populated. Prefer os.MkdirTemp and defer os.RemoveAll.
Proposed fix (apply to all fixed /tmp dirs)
- extractTmpDirName := "/tmp/case51018"
- err := os.MkdirAll(extractTmpDirName, 0700)
+ extractTmpDirName, err := os.MkdirTemp("", "case51018-")
o.Expect(err).NotTo(o.HaveOccurred())
defer os.RemoveAll(extractTmpDirName)Also applies to: 375-378, 873-877, 914-918
🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 233 - 237, Replace the hard-coded temp
directory string (extractTmpDirName := "/tmp/case51018") with a call to
os.MkdirTemp to create a unique temporary directory, assign its returned path to
extractTmpDirName, check the error and keep the existing defer
os.RemoveAll(extractTmpDirName); update the error expectation (err) handling as
used in the surrounding test (o.Expect(err).NotTo(o.HaveOccurred())); apply the
same change to the other fixed /tmp usages referenced in the review (the other
test temp dirs at the noted locations) so each test uses os.MkdirTemp with a
unique prefix and defers os.RemoveAll.
Comment on lines
+365
to
+372
| sedCmd := fmt.Sprintf(`sed -i 's/localhost/%s/g' %s`, imageRegistryName, idmsFile64921) | ||
| _, err = exec.Command("bash", "-c", sedCmd).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| // Replace target correctly in the icsp file | ||
| sedCmdOne := fmt.Sprintf(`sed -i 's/target/%s/g' %s`, strings.Split(pullSpec, "/")[1], idmsFile64921) | ||
| _, err = exec.Command("bash", "-c", sedCmdOne).Output() | ||
| o.Expect(err).NotTo(o.HaveOccurred()) |
There was a problem hiding this comment.
Avoid shell command construction with cluster-derived values.
imageRegistryName and pull-spec fragments are injected into a shell string. If they contain shell metacharacters, this allows command injection on the test runner. Replace with direct file edits in Go.
Proposed fix
- sedCmd := fmt.Sprintf(`sed -i 's/localhost/%s/g' %s`, imageRegistryName, idmsFile64921)
- _, err = exec.Command("bash", "-c", sedCmd).Output()
- o.Expect(err).NotTo(o.HaveOccurred())
-
- // Replace target correctly in the icsp file
- sedCmdOne := fmt.Sprintf(`sed -i 's/target/%s/g' %s`, strings.Split(pullSpec, "/")[1], idmsFile64921)
- _, err = exec.Command("bash", "-c", sedCmdOne).Output()
- o.Expect(err).NotTo(o.HaveOccurred())
+ content, err := os.ReadFile(idmsFile64921)
+ o.Expect(err).NotTo(o.HaveOccurred())
+ updated := strings.ReplaceAll(string(content), "localhost", imageRegistryName)
+ updated = strings.ReplaceAll(updated, "target", strings.Split(pullSpec, "/")[1])
+ o.Expect(os.WriteFile(idmsFile64921, []byte(updated), 0644)).NotTo(o.HaveOccurred())🤖 Prompt for AI Agents
In `@test/e2e/cli.go` around lines 365 - 372, The test builds shell commands
(sedCmd, sedCmdOne) using cluster-derived values (imageRegistryName and pullSpec
parts) and executes them via exec.Command("bash", "-c", ...), which risks
command injection; instead, locate the file referenced by idmsFile64921, read
its contents in Go, perform safe string replacements for "localhost" ->
imageRegistryName and "target" -> strings.Split(pullSpec, "/")[1] (or compute
the intended segment into a variable first), and write the modified content back
to idmsFile64921, replacing the exec.Command calls and
o.Expect(err).NotTo(HaveOccurred()) checks with proper Go file IO error handling
in the same test function.
| @@ -0,0 +1,6 @@ | |||
| apiVersion: "bate.example.com/v1" | |||
There was a problem hiding this comment.
Typo in apiVersion: bate should likely be beta.
The apiVersion uses bate.example.com/v1 which appears to be a typo for beta.example.com/v1. While this may work for isolated testing, it could cause confusion if used as a reference.
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/case72217/cr-cat-72217.yaml` at line 1, The apiVersion
string in the manifest is mistyped as "bate.example.com/v1"; update the
apiVersion value to "beta.example.com/v1" so that the resource uses the correct
group/version identifier (look for the apiVersion field in the YAML containing
"bate.example.com/v1" and replace "bate" with "beta").
| memory: 60Mi | ||
| requests: | ||
| cpu: 50m | ||
| memory: 40M |
There was a problem hiding this comment.
Inconsistent memory unit: 40M vs 40Mi in second container.
Line 42 uses 40M (decimal megabytes) while line 54 uses 40Mi (binary mebibytes). This inconsistency could cause subtle differences in quota calculations. Use 40Mi consistently.
Suggested fix
requests:
cpu: 50m
- memory: 40M
+ memory: 40Mi📝 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
| memory: 40M | |
| memory: 40Mi |
🤖 Prompt for AI Agents
In `@test/testdata/oc_cli/deploymentconfig_with_quota.yaml` at line 42, The memory
unit in the deployment config is inconsistent: change the memory value from
"40M" to "40Mi" (match the other container's "40Mi") to ensure consistent binary
units for quota calculations; update the memory: 40M entry so both containers
use "40Mi".
Contributor
Author
|
/test e2e-aws-ovn-serial-1of2 |
Contributor
Author
|
/payload 4.22 nightly blocking |
Contributor
Author
|
/payload 4.22 nightly informing |
Contributor
|
@gangwgr: trigger 14 job(s) of type blocking for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ca187c80-00e1-11f1-8ded-f0c4a708978a-0 |
Contributor
|
@gangwgr: trigger 68 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d1095f50-00e1-11f1-9da6-4d072c857558-0 |
Contributor
Author
|
/test e2e-aws-ovn-serial-2of2 |
Contributor
Author
|
/test e2e-aws-ovn-serial-1of2 |
Contributor
Author
|
/test e2e-aws-ovn-serial-1of2 |
Contributor
|
@gangwgr: all tests passed! 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. |
Contributor
Author
|
/payload 4.22 nightly informing |
Contributor
|
@gangwgr: trigger 68 job(s) of type informing for the nightly release of OCP 4.22
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/4f187e80-019b-11f1-8944-0c5d2627f776-0 |
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
2 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.
Fixing oc client caes for microshift and metal ipv6 profliles