Open
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
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 a blocking Conformance test ensuring CVO does not install resources annotated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JianLi-RH The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Contributor
Author
|
@hongkailiu Because my pr #1309 was reverted by #1353, added back the case in this PR. For any tests on connected/disconnected cluster, please read below comments. |
Contributor
Author
|
Confirmed, the extract command does not work on a disconnected cluster: /cc @hongkailiu @jiajliu |
Contributor
Author
|
/unhold |
Contributor
Author
|
For connected cluster, test passed. Cluster-bot log: https://prow.ci.openshift.org/view/gs/test-platform-results/logs/release-openshift-origin-installer-launch-aws-modern/2036384394893070336 |
Contributor
Author
|
For disconnected cluster, I couldn't create a normal cluster (Cluster-bot seems does not support disconnect, Jenkins Flexy-install job has issue https://jenkins-csb-openshift-qe-mastern.dno.corp.redhat.com/job/ocp-common/job/Flexy-install/372175/), the case failed: |
JianLi-RH
changed the title
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
cd22772 to
c5b4eda
Compare
`oc adm release extract` command accept --registry-config option to set auth file. To avoid future issues, add auth file to method AdmReleaseExtract() And this is also what we did in openshift-tests-private: https://github.com/openshift/openshift-tests-private/blob/49b10e687053061ecc87989335f2908b59c66f98/test/extended/ota/cvo/utils.go#L541-L547
The automation case 42543 was reverted by openshift#1353 This PR will fix the issue on disconnected environment and added back the case which initialized by openshift#1309
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
c5b4eda to
773b61b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Member
|
Looking at what the test is actually doing, could it iterate over the extracted manifests in CVO's container filesystem directly in the cluster? That way the test would not need to do any heavy operations like release extract, it would not need to deal with a pull secret... It would be faster, too. |
Contributor
Author
|
@petr-muller |
Contributor
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Contributor
If the root cause is an authorization issue with the extract command, shall we fix the extract issue first and disable the skip on disconnected cluster? Checked the case history of case-42543 in CI with OTP tests, the case passed in a disconnected cluster as expected, for example: btw, it's suggested to rerun the failed job |
Contributor
Author
|
@petr-muller Thank you for the suggestion, it works fine. The code changes can be found in commit: 0734315 There are some new methods:
Here is the test result: If it work as expected, I will remove prior case (but I want to keep GetAuthFile()) |
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
0734315 to
5823e70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cvo/cvo.go`:
- Around line 116-119: The code dereferences pods[0] after
util.GetPodsByNamespace without checking the slice length and may pick a
non-running CVO causing panics or flaky execs; update the logic around pods (the
variable returned from GetPodsByNamespace in test/cvo/cvo.go) to first assert
len(pods) > 0 and return/test-fail if empty, then choose a pod in Running state
(e.g., inspect pod.Status.Phase or pod.Status.Conditions) rather than assuming
pods[0]; replace direct pods[0] usage (and the same pattern later around the
subsequent pod access) with the guarded selection of a runnable pod and handle
the no-runnable-pod case cleanly.
- Around line 136-142: The test currently parses only the first YAML document
via util.ParseManifest(fileContent) and then checks
manifest.Obj.GetAnnotations() for the annotation variable, which misses later
documents; change the logic to iterate over all YAML documents in fileContent
(either by using a multi-doc parser such as util.ParseManifests if available, or
by splitting on the YAML document separator and calling util.ParseManifest for
each document), and for each parsed manifest check
manifest.Obj.GetAnnotations()[annotation] == "true" and handle accordingly so
deleted resources in later documents are detected.
In `@test/util/util.go`:
- Around line 125-127: Replace the insecure manual temp file creation for
authFile with a call to os.CreateTemp to create a file with restrictive
permissions (0600), write secretData into that file (using the returned
*os.File) and close it, and remove the manual filepath.Join("/tmp/",
fmt.Sprintf("ota-%s", rand.Text())) logic; specifically update the block that
defines authFile, writes secretData, and handles errors so it uses
os.CreateTemp, writes to the created file, sets/ensures file mode 0600, closes
the file, and defers/removes the temp file when done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d7539f7-6dda-4fd2-a47f-2681ad9c4954
📒 Files selected for processing (6)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsonpkg/cvo/external/dynamicclient/client.gotest/cvo/cvo.gotest/oc/api/api.gotest/oc/cli/cli.gotest/util/util.go
Comment on lines
+116
to
+119
| pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| logger.Info("CVO pod found", "name", pods[0].Name) | ||
|
|
There was a problem hiding this comment.
Guard pod lookup before indexing and target a runnable CVO pod.
Line 118 dereferences pods[0] without validating list length, and the current selection may pick a non-running pod, causing panic/flaky exec failures.
Suggested fix
pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"})
o.Expect(err).NotTo(o.HaveOccurred())
- logger.Info("CVO pod found", "name", pods[0].Name)
+ o.Expect(pods).NotTo(o.BeEmpty(), "Expected at least one CVO pod")
+
+ cvoPod := pods[0]
+ o.Expect(cvoPod.Status.Phase).To(o.Equal(corev1.PodRunning), "Expected selected CVO pod to be Running")
+ logger.Info("CVO pod found", "name", cvoPod.Name)
@@
- files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator")
+ files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, cvoPod.Name, "cluster-version-operator")Also applies to: 123-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cvo/cvo.go` around lines 116 - 119, The code dereferences pods[0] after
util.GetPodsByNamespace without checking the slice length and may pick a
non-running CVO causing panics or flaky execs; update the logic around pods (the
variable returned from GetPodsByNamespace in test/cvo/cvo.go) to first assert
len(pods) > 0 and return/test-fail if empty, then choose a pod in Running state
(e.g., inspect pod.Status.Phase or pod.Status.Conditions) rather than assuming
pods[0]; replace direct pods[0] usage (and the same pattern later around the
subsequent pod access) with the guarded selection of a runnable pod and handle
the no-runnable-pod case cleanly.
Comment on lines
+136
to
+142
| manifest, err := util.ParseManifest(fileContent) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f)) | ||
|
|
||
| ann := manifest.Obj.GetAnnotations() | ||
| if ann[annotation] != "true" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This checks only one document per YAML file and can miss deleted resources.
util.ParseManifest(fileContent) returns a single manifest, but release-manifest files may contain multiple YAML documents. If release.openshift.io/delete=true appears in a later document, the test can incorrectly pass.
Suggested fix direction
- manifest, err := util.ParseManifest(fileContent)
- o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f))
-
- ann := manifest.Obj.GetAnnotations()
- if ann[annotation] != "true" {
- continue
- }
- // check object absence ...
+ manifests, err := util.ParseManifests(fileContent) // parse all docs
+ o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f))
+ for _, manifest := range manifests {
+ ann := manifest.Obj.GetAnnotations()
+ if ann[annotation] != "true" {
+ continue
+ }
+ // check object absence ...
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cvo/cvo.go` around lines 136 - 142, The test currently parses only the
first YAML document via util.ParseManifest(fileContent) and then checks
manifest.Obj.GetAnnotations() for the annotation variable, which misses later
documents; change the logic to iterate over all YAML documents in fileContent
(either by using a multi-doc parser such as util.ParseManifests if available, or
by splitting on the YAML document separator and calling util.ParseManifest for
each document), and for each parsed manifest check
manifest.Obj.GetAnnotations()[annotation] == "true" and handle accordingly so
deleted resources in later documents are detected.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/util/util.go (1)
125-130:⚠️ Potential issue | 🟠 MajorProtect pull-secret material with secure temp-file creation and permissions
Line 126 writes registry credentials with
0644, making the auth file broadly readable; plus manual/tmpfilename construction is weaker than atomic temp-file creation. Please switch toos.CreateTempand keep mode0600.As per coding guidelines, `**`: "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Suggested fix
- authFile := filepath.Join("/tmp/", fmt.Sprintf("ota-%s", rand.Text())) - err = os.WriteFile(authFile, secretData, 0644) - if err != nil { - return "", fmt.Errorf("error writing file %s: %v", authFile, err) - } - return authFile, nil + f, err := os.CreateTemp("", "ota-*") + if err != nil { + return "", fmt.Errorf("error creating auth temp file: %w", err) + } + defer func() { + if err != nil { + _ = os.Remove(f.Name()) + } + }() + if err := f.Chmod(0o600); err != nil { + _ = f.Close() + return "", fmt.Errorf("error setting auth temp file permissions: %w", err) + } + if _, err := f.Write(secretData); err != nil { + _ = f.Close() + return "", fmt.Errorf("error writing auth temp file %s: %w", f.Name(), err) + } + if err := f.Close(); err != nil { + return "", fmt.Errorf("error closing auth temp file %s: %w", f.Name(), err) + } + return f.Name(), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/util/util.go` around lines 125 - 130, Replace the manual filepath.Join("/tmp/", ...) + os.WriteFile flow with os.CreateTemp to atomically create a secure temp file (instead of constructing authFile by hand); write secretData into the returned *os.File, ensure the file is closed, explicitly enforce mode 0600 (file.Chmod(0600)) and handle/cleanup on errors, and then return the temp filename (authFile) — update usages around the authFile variable and remove the original os.WriteFile call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/util/util.go`:
- Around line 125-130: Replace the manual filepath.Join("/tmp/", ...) +
os.WriteFile flow with os.CreateTemp to atomically create a secure temp file
(instead of constructing authFile by hand); write secretData into the returned
*os.File, ensure the file is closed, explicitly enforce mode 0600
(file.Chmod(0600)) and handle/cleanup on errors, and then return the temp
filename (authFile) — update usages around the authFile variable and remove the
original os.WriteFile call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f81f7d2-d4a5-4de5-8a14-8fb5b59e2b5c
📒 Files selected for processing (2)
test/cvo/cvo.gotest/util/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cvo/cvo.go
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
5823e70 to
cada5ac
Compare
Contributor
Author
|
@jiajliu With the new approach #1358 (comment), the case passed on a disconnected cluster: |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cvo/cvo.go (2)
134-140:⚠️ Potential issue | 🟠 MajorParse all YAML documents, not only the first one
Line 134 calls
util.ParseManifest(fileContent), which returns only the first non-empty document. Multi-doc manifest files can hiderelease.openshift.io/delete=truein later docs, causing false passes.Proposed fix direction
- manifest, err := util.ParseManifest(fileContent) - o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f)) - - ann := manifest.Obj.GetAnnotations() - if ann[annotation] != "true" { - continue - } - logger.Info("Checking file: ", f, ", GVK:", manifest.GVK.String(), ", Name: ", manifest.Obj.GetName(), ", Namespace: ", manifest.Obj.GetNamespace()) - client, err := dynamicclient.New(restCfg, manifest.GVK, manifest.Obj.GetNamespace()) - o.Expect(err).NotTo(o.HaveOccurred()) - _, err = client.Get(ctx, manifest.Obj.GetName(), metav1.GetOptions{}) - o.Expect(apierrors.IsNotFound(err)).To(o.BeTrue(), - fmt.Sprintf("The deleted manifest should not be installed, but actually installed: manifest: %s %s in namespace %s from file %q, error: %v", - manifest.GVK, manifest.Obj.GetName(), manifest.Obj.GetNamespace(), f, err)) + manifests, err := util.ParseManifests(fileContent) // add helper that decodes all YAML docs + o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("Failed to parse manifest content of file %s in CVO pod", f)) + for _, manifest := range manifests { + ann := manifest.Obj.GetAnnotations() + if ann[annotation] != "true" { + continue + } + logger.Info("Checking file: ", f, ", GVK:", manifest.GVK.String(), ", Name: ", manifest.Obj.GetName(), ", Namespace: ", manifest.Obj.GetNamespace()) + client, err := dynamicclient.New(restCfg, manifest.GVK, manifest.Obj.GetNamespace()) + o.Expect(err).NotTo(o.HaveOccurred()) + _, err = client.Get(ctx, manifest.Obj.GetName(), metav1.GetOptions{}) + o.Expect(apierrors.IsNotFound(err)).To(o.BeTrue(), + fmt.Sprintf("The deleted manifest should not be installed, but actually installed: manifest: %s %s in namespace %s from file %q, error: %v", + manifest.GVK, manifest.Obj.GetName(), manifest.Obj.GetNamespace(), f, err)) + }As per coding guidelines,
**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 134 - 140, The code only parses the first YAML document via util.ParseManifest(fileContent) and will miss later docs that might contain the release.openshift.io/delete=true annotation; update the logic to iterate over all YAML documents in fileContent (split on document separators like "---" or use a YAML multi-doc parser), call util.ParseManifest (or equivalent) for each non-empty doc, then for each parsed manifest check manifest.Obj.GetAnnotations() (the ann map) for ann[annotation] == "true" and continue accordingly; ensure you reference the same variables (fileContent, manifest, ann, annotation, f) so the loop replaces the single-call parsing and correctly handles multi-doc manifests.
114-127:⚠️ Potential issue | 🟠 MajorSelect a running CVO pod before exec calls
Line 121 and Line 126 still use
pods[0]from a label-only list. That can pick a non-running pod and make this test flaky/fail intermittently.Proposed fix
+ corev1 "k8s.io/api/core/v1" @@ pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"}) o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(pods).NotTo(o.BeEmpty(), "Expected at least one CVO pod") + + var cvoPodName string + for _, pod := range pods { + if pod.Status.Phase == corev1.PodRunning { + cvoPodName = pod.Name + break + } + } + o.Expect(cvoPodName).NotTo(o.BeEmpty(), "Expected at least one running CVO pod") @@ - files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator") + files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, cvoPodName, "cluster-version-operator") @@ - fileContent, err := util.GetFileContentInPodContainer(ctx, restCfg, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator", f) + fileContent, err := util.GetFileContentInPodContainer(ctx, restCfg, external.DefaultCVONamespace, cvoPodName, "cluster-version-operator", f)As per coding guidelines,
**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 114 - 127, The code currently uses pods[0] (the first pod from util.GetPodsByNamespace) when calling util.ListFilesInPodContainer and util.GetFileContentInPodContainer which can be a non-running pod; update the test to choose a running CVO pod first (e.g., iterate the returned pods slice and pick the one with Status.Phase == v1.PodRunning or use any helper that returns a running pod), assign it to a variable like runningPod, and replace pods[0].Name with runningPod.Name in the calls to ListFilesInPodContainer and GetFileContentInPodContainer (ensure imports/types for v1.PodPhase are available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cvo/cvo.go`:
- Around line 134-140: The code only parses the first YAML document via
util.ParseManifest(fileContent) and will miss later docs that might contain the
release.openshift.io/delete=true annotation; update the logic to iterate over
all YAML documents in fileContent (split on document separators like "---" or
use a YAML multi-doc parser), call util.ParseManifest (or equivalent) for each
non-empty doc, then for each parsed manifest check manifest.Obj.GetAnnotations()
(the ann map) for ann[annotation] == "true" and continue accordingly; ensure you
reference the same variables (fileContent, manifest, ann, annotation, f) so the
loop replaces the single-call parsing and correctly handles multi-doc manifests.
- Around line 114-127: The code currently uses pods[0] (the first pod from
util.GetPodsByNamespace) when calling util.ListFilesInPodContainer and
util.GetFileContentInPodContainer which can be a non-running pod; update the
test to choose a running CVO pod first (e.g., iterate the returned pods slice
and pick the one with Status.Phase == v1.PodRunning or use any helper that
returns a running pod), assign it to a variable like runningPod, and replace
pods[0].Name with runningPod.Name in the calls to ListFilesInPodContainer and
GetFileContentInPodContainer (ensure imports/types for v1.PodPhase are
available).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88c4ae5d-0d47-47d1-90ae-8196b0510b2a
📒 Files selected for processing (3)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/util/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
- test/util/util.go
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
cada5ac to
6af3a47
Compare
There are some new methods: 1. GetPodsByNamespace() used to get CVO pod 2. ListFilesInPodContainer() used to list files in a pod container, here I use a customized command to reduce files (get file content file takes long time) 3. GetFileContentInPodContainer() used to get file content from a pod container 4. ParseManifest() parse yaml file content to Manifest object.
JianLi-RH
force-pushed
the
adding_auth_file_to_AdmReleaseExtract_method
branch
from
6af3a47 to
a9869a5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cvo/cvo.go (2)
114-127:⚠️ Potential issue | 🟠 MajorPick a runnable CVO pod instead of hard-coding
pods[0].Line 121 and Line 126 execute into
pods[0]without ensuring it is Running. This can make the test flaky when the first listed pod is Pending/Terminating.Suggested fix
+ corev1 "k8s.io/api/core/v1" ... pods, err := util.GetPodsByNamespace(ctx, kubeClient, external.DefaultCVONamespace, map[string]string{"k8s-app": "cluster-version-operator"}) o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(pods).NotTo(o.BeEmpty(), "Expected at least one CVO pod") + + var cvoPodName string + for i := range pods { + if pods[i].Status.Phase == corev1.PodRunning { + cvoPodName = pods[i].Name + break + } + } + o.Expect(cvoPodName).NotTo(o.BeEmpty(), "Expected at least one Running CVO pod") ... - files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator") + files, err := util.ListFilesInPodContainer(ctx, restCfg, command, external.DefaultCVONamespace, cvoPodName, "cluster-version-operator") ... - fileContent, err := util.GetFileContentInPodContainer(ctx, restCfg, external.DefaultCVONamespace, pods[0].Name, "cluster-version-operator", f) + fileContent, err := util.GetFileContentInPodContainer(ctx, restCfg, external.DefaultCVONamespace, cvoPodName, "cluster-version-operator", f)As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 114 - 127, The code currently uses pods[0] when calling util.ListFilesInPodContainer and util.GetFileContentInPodContainer which can be flaky if that pod is not Running; change the selection to pick a runnable CVO pod by iterating the slice returned by util.GetPodsByNamespace and selecting the first pod whose Status.Phase == "Running" and has the Ready condition true (or equivalent readiness check) before using its Name in ListFilesInPodContainer and GetFileContentInPodContainer; if no runnable pod is found, return/fail with a clear error message.
134-140:⚠️ Potential issue | 🟠 MajorParse all YAML documents in each manifest file.
Line 134 uses
util.ParseManifest(fileContent), which parses only one document. Multi-doc manifest files can hiderelease.openshift.io/delete=truein later documents, causing false passes.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 134 - 140, The code currently calls util.ParseManifest(fileContent) which parses only the first YAML document, so change the logic to parse and iterate over all YAML documents in fileContent: either call an existing helper that returns multiple manifests (e.g., util.ParseManifests(fileContent) or similar) and loop over the returned slice, or split fileContent into separate YAML documents and call util.ParseManifest on each document; for each manifest ensure you inspect manifest.Obj.GetAnnotations() and check ann[annotation] == "true" before continuing. Ensure the code iterates all manifests in the file rather than only the first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cvo/cvo.go`:
- Around line 114-127: The code currently uses pods[0] when calling
util.ListFilesInPodContainer and util.GetFileContentInPodContainer which can be
flaky if that pod is not Running; change the selection to pick a runnable CVO
pod by iterating the slice returned by util.GetPodsByNamespace and selecting the
first pod whose Status.Phase == "Running" and has the Ready condition true (or
equivalent readiness check) before using its Name in ListFilesInPodContainer and
GetFileContentInPodContainer; if no runnable pod is found, return/fail with a
clear error message.
- Around line 134-140: The code currently calls util.ParseManifest(fileContent)
which parses only the first YAML document, so change the logic to parse and
iterate over all YAML documents in fileContent: either call an existing helper
that returns multiple manifests (e.g., util.ParseManifests(fileContent) or
similar) and loop over the returned slice, or split fileContent into separate
YAML documents and call util.ParseManifest on each document; for each manifest
ensure you inspect manifest.Obj.GetAnnotations() and check ann[annotation] ==
"true" before continuing. Ensure the code iterates all manifests in the file
rather than only the first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe24f23a-63c3-4f8a-be92-1445d7452b4b
📒 Files selected for processing (3)
.openshift-tests-extension/openshift_payload_cluster-version-operator.jsontest/cvo/cvo.gotest/util/util.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .openshift-tests-extension/openshift_payload_cluster-version-operator.json
- test/util/util.go
Contributor
|
@JianLi-RH: 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
3 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.
For a non-stable cluster, for example nightly cluster, we have to set
--registry-configto theoc adm release extractcommand, otherwise it will fail with authentication error, for example:The extract command will work fine when we give auth file to it, for example:
I have tested the new method AdmReleaseExtract with below test case:
the case passed:
To add back test case 42543, I did below changes to the case: