NO-ISSUE: OTA-1605 Automate OCP-42543 #1309
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@JianLi-RH: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a new CVO test that asserts resources annotated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/cvo/cvo.go`:
- Around line 99-105: Update the assertion message to accurately describe the
expectation: change the misleading message on the
o.Expect(err).NotTo(o.HaveOccurred(), ...) call so it states that manifest
extraction should succeed (or that no error is expected) when invoking
ocClient.AdmReleaseExtract with manifestDir
(ocClient.AdmReleaseExtract(manifestDir)); reference the manifest extraction
operation and use clear text like "expected manifest extraction to succeed" or
"no error expected when extracting manifests" instead of "The NotFound error
should occur when extract manifests".
- Around line 117-119: The loop opens files with os.Open and uses defer
file.Close(), which leaks descriptors until the enclosing function returns;
replace the deferred close by closing each file immediately after its processing
(call file.Close() directly at the end of the loop iteration) or move the
per-file logic into a helper function (e.g., processManifestFile(filePath) that
opens the file and defers file.Close() inside that helper) so file handles are
released promptly; update references to the file variable and remove the in-loop
defer file.Close() in the code around os.Open and file usage.
🧹 Nitpick comments (1)
test/oc/cli/cli.go (1)
118-125: Duplicate logging inRunmethod.The
ocExecutor.runmethod (lines 38-42) already logs success/error with command details. ThisRunwrapper adds redundant logging, resulting in double log entries for each command execution.Consider removing the duplicate logging here since the executor already handles it:
♻️ Proposed simplification
func (c *client) Run(args ...string) ([]byte, error) { - b, err := c.executor.run(args...) - if err != nil { - c.logger.Error(err, "running command failed", "output", string(b)) - } else { - c.logger.Info("running command succeeded.") - } - return b, err + return c.executor.run(args...) }
test/cvo/cvo.go
Outdated
| g.By("Extract manifests") | ||
| annotation := "release.openshift.io/delete" | ||
| manifestDir := ocapi.ReleaseExtractOptions{To: "/tmp/OTA-42543-manifest"} | ||
| logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To)) | ||
| defer func() { _ = os.RemoveAll(manifestDir.To) }() | ||
| err = ocClient.AdmReleaseExtract(manifestDir) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "The NotFound error should occur when extract manifests") |
There was a problem hiding this comment.
Minor: assertion message is misleading.
Line 105's error message says "The NotFound error should occur when extract manifests" but this assertion expects extraction to succeed (no error). The message appears to be copy-pasted from elsewhere.
📝 Suggested fix
err = ocClient.AdmReleaseExtract(manifestDir)
- o.Expect(err).NotTo(o.HaveOccurred(), "The NotFound error should occur when extract manifests")
+ o.Expect(err).NotTo(o.HaveOccurred(), "Failed to extract manifests")📝 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.
| g.By("Extract manifests") | |
| annotation := "release.openshift.io/delete" | |
| manifestDir := ocapi.ReleaseExtractOptions{To: "/tmp/OTA-42543-manifest"} | |
| logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To)) | |
| defer func() { _ = os.RemoveAll(manifestDir.To) }() | |
| err = ocClient.AdmReleaseExtract(manifestDir) | |
| o.Expect(err).NotTo(o.HaveOccurred(), "The NotFound error should occur when extract manifests") | |
| g.By("Extract manifests") | |
| annotation := "release.openshift.io/delete" | |
| manifestDir := ocapi.ReleaseExtractOptions{To: "/tmp/OTA-42543-manifest"} | |
| logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To)) | |
| defer func() { _ = os.RemoveAll(manifestDir.To) }() | |
| err = ocClient.AdmReleaseExtract(manifestDir) | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to extract manifests") |
🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 99 - 105, Update the assertion message to
accurately describe the expectation: change the misleading message on the
o.Expect(err).NotTo(o.HaveOccurred(), ...) call so it states that manifest
extraction should succeed (or that no error is expected) when invoking
ocClient.AdmReleaseExtract with manifestDir
(ocClient.AdmReleaseExtract(manifestDir)); reference the manifest extraction
operation and use clear text like "expected manifest extraction to succeed" or
"no error expected when extracting manifests" instead of "The NotFound error
should occur when extract manifests".
test/cvo/cvo.go
Outdated
| file, err := os.Open(filePath) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer file.Close() |
There was a problem hiding this comment.
Resource leak: defer file.Close() inside loop.
The defer statement accumulates all file handles until the function returns, rather than closing each file after processing. With many manifest files, this could exhaust file descriptors.
🐛 Proposed fix: close file immediately after processing
filePath := filepath.Join(manifestDir.To, entry.Name())
file, err := os.Open(filePath)
o.Expect(err).NotTo(o.HaveOccurred())
- defer file.Close()
decoder := yamlv3.NewDecoder(file)
for {
var doc map[string]interface{}
if err := decoder.Decode(&doc); err != nil {
if err == io.EOF {
break
}
continue
}
meta, _ := doc["metadata"].(map[string]interface{})
ann, _ := meta["annotations"].(map[string]interface{})
if ann == nil || ann[annotation] != "true" {
continue
}
kind, _ := doc["kind"].(string)
name, _ := meta["name"].(string)
namespace, _ := meta["namespace"].(string)
args := []string{"get", kind, name}
if namespace != "" {
args = append(args, "-n", namespace)
}
_, err := ocClient.Run(args...)
o.Expect(err).To(o.HaveOccurred(), "The deleted manifest should not be installed, but actually installed")
}
+ file.Close()
}Alternatively, extract the file processing into a helper function where defer would work correctly.
📝 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.
| file, err := os.Open(filePath) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| defer file.Close() | |
| filePath := filepath.Join(manifestDir.To, entry.Name()) | |
| file, err := os.Open(filePath) | |
| o.Expect(err).NotTo(o.HaveOccurred()) | |
| decoder := yamlv3.NewDecoder(file) | |
| for { | |
| var doc map[string]interface{} | |
| if err := decoder.Decode(&doc); err != nil { | |
| if err == io.EOF { | |
| break | |
| } | |
| continue | |
| } | |
| meta, _ := doc["metadata"].(map[string]interface{}) | |
| ann, _ := meta["annotations"].(map[string]interface{}) | |
| if ann == nil || ann[annotation] != "true" { | |
| continue | |
| } | |
| kind, _ := doc["kind"].(string) | |
| name, _ := meta["name"].(string) | |
| namespace, _ := meta["namespace"].(string) | |
| args := []string{"get", kind, name} | |
| if namespace != "" { | |
| args = append(args, "-n", namespace) | |
| } | |
| _, err := ocClient.Run(args...) | |
| o.Expect(err).To(o.HaveOccurred(), "The deleted manifest should not be installed, but actually installed") | |
| } | |
| file.Close() | |
| } |
🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 117 - 119, The loop opens files with os.Open
and uses defer file.Close(), which leaks descriptors until the enclosing
function returns; replace the deferred close by closing each file immediately
after its processing (call file.Close() directly at the end of the loop
iteration) or move the per-file logic into a helper function (e.g.,
processManifestFile(filePath) that opens the file and defers file.Close() inside
that helper) so file handles are released promptly; update references to the
file variable and remove the in-loop defer file.Close() in the code around
os.Open and file usage.
JianLi-RH
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
JianLi-RH
force-pushed
the
automate_42543
branch
4 times, most recently
from
8d09bb9 to
f175131
Compare
JianLi-RH
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@test/cvo/cvo.go`:
- Around line 134-140: The YAML decode loop currently swallows decode errors
with a continue, but the test should fail instead; replace the silent continue
with an assertion that fails the test on decode errors by calling
o.Expect(err).NotTo(o.HaveOccurred(), "decode manifest %s failed", filePath)
immediately after decoder.Decode(&doc) (remove the unreachable continue) so any
decoder.Decode failure in the loop (the symbol decoder.Decode and variable doc)
causes the test to fail and reports the filePath context.
- Around line 141-145: The annotation check in the loop (variables meta, ann and
the key annotation in test/cvo/cvo.go) wrongly assumes the value is the string
"true"; change the check to handle both boolean true and the string "true" (and
treat other truthy forms if present) by retrieving v := ann[annotation] and
using a type switch or explicit type assertions to accept v == true or v ==
"true" (and skip if ann is nil or the value is absent), so resources with
unquoted YAML true are correctly detected for deletion.
- Around line 103-108: The current code uses a fixed manifestDir.To =
"/tmp/OTA-42543-manifest" which can collide in parallel runs; replace that with
a unique temp directory created via os.MkdirTemp and assign the returned path to
manifestDir.To (handle and return/log any error from MkdirTemp), keep the defer
to os.RemoveAll(manifestDir.To) for cleanup, and then call
ocClient.AdmReleaseExtract(manifestDir) as before; update the code around the
manifestDir variable, ocapi.ReleaseExtractOptions initialization, and the defer
cleanup to use the MkdirTemp-created path.
test/cvo/cvo.go
Outdated
| g.By("Extract manifests") | ||
| annotation := "release.openshift.io/delete" | ||
| manifestDir := ocapi.ReleaseExtractOptions{To: "/tmp/OTA-42543-manifest"} | ||
| logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To)) | ||
| defer func() { _ = os.RemoveAll(manifestDir.To) }() | ||
| err = ocClient.AdmReleaseExtract(manifestDir) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read lines 103-108
wc -l test/cvo/cvo.go
head -n 110 test/cvo/cvo.go | tail -n 15Repository: openshift/cluster-version-operator
Length of output: 723
🏁 Script executed:
# Look for the test function and Ginkgo setup
head -n 103 test/cvo/cvo.go | tail -n 40Repository: openshift/cluster-version-operator
Length of output: 2398
🏁 Script executed:
# Find the test structure and look for Describe block
head -n 103 test/cvo/cvo.go | head -n 50Repository: openshift/cluster-version-operator
Length of output: 1418
🏁 Script executed:
# Check the Describe block more carefully for parallelism setup
sed -n '47,164p' test/cvo/cvo.go | head -n 50Repository: openshift/cluster-version-operator
Length of output: 2921
🏁 Script executed:
# Check if there's a .Parallel() call on the Describe block
grep -n "\.Parallel\|Describe.*func\|BeforeSuite\|AfterSuite" test/cvo/cvo.goRepository: openshift/cluster-version-operator
Length of output: 269
🏁 Script executed:
# Search for MkdirTemp usage in test files
rg -t go "MkdirTemp" test/Repository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Search for temporary directory patterns in test files
rg -t go "TempDir|ioutil\.TempDir|os\.MkdirTemp" test/Repository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Search for /tmp paths in test files
rg -t go '/tmp/' test/Repository: openshift/cluster-version-operator
Length of output: 169
🏁 Script executed:
# Check the full test function to understand context better
sed -n '85,125p' test/cvo/cvo.goRepository: openshift/cluster-version-operator
Length of output: 1927
Use a unique temp directory for manifest extraction.
A fixed /tmp/OTA-42543-manifest can collide in parallel test runs, causing test flakiness and data loss. Use os.MkdirTemp() to create unique, isolated directories for each test execution.
🔧 Suggested fix
- manifestDir := ocapi.ReleaseExtractOptions{To: "/tmp/OTA-42543-manifest"}
- logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To))
- defer func() { _ = os.RemoveAll(manifestDir.To) }()
- err = ocClient.AdmReleaseExtract(manifestDir)
+ tempDir, err := os.MkdirTemp("", "OTA-42543-manifest-")
+ o.Expect(err).NotTo(o.HaveOccurred(), "create temp manifest dir failed")
+ manifestDir := ocapi.ReleaseExtractOptions{To: tempDir}
+ logger.Info(fmt.Sprintf("Extract manifests to: %s", manifestDir.To))
+ defer func() { _ = os.RemoveAll(manifestDir.To) }()
+ err = ocClient.AdmReleaseExtract(manifestDir)🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 103 - 108, The current code uses a fixed
manifestDir.To = "/tmp/OTA-42543-manifest" which can collide in parallel runs;
replace that with a unique temp directory created via os.MkdirTemp and assign
the returned path to manifestDir.To (handle and return/log any error from
MkdirTemp), keep the defer to os.RemoveAll(manifestDir.To) for cleanup, and then
call ocClient.AdmReleaseExtract(manifestDir) as before; update the code around
the manifestDir variable, ocapi.ReleaseExtractOptions initialization, and the
defer cleanup to use the MkdirTemp-created path.
| var doc map[string]interface{} | ||
| if err := decoder.Decode(&doc); err != nil { | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd test/cvo && cat -n cvo.go | sed -n '120,155p'Repository: openshift/cluster-version-operator
Length of output: 1428
🏁 Script executed:
cd test/cvo && grep -n "o.Expect" cvo.go | head -20Repository: openshift/cluster-version-operator
Length of output: 1819
🏁 Script executed:
cd test/cvo && grep -n "decoder.Decode\|HaveOccurred" cvo.goRepository: openshift/cluster-version-operator
Length of output: 1432
Fail the test on YAML decode errors instead of silently continuing.
Silently continuing on decode failures can skip manifests and yield false negatives. However, the suggested fix has unreachable code—continue would never execute after o.Expect().NotTo(o.HaveOccurred()) fails. Either use o.Expect(err).NotTo(o.HaveOccurred(), "decode manifest %s failed", filePath) without the continue, or handle the error differently.
🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 134 - 140, The YAML decode loop currently
swallows decode errors with a continue, but the test should fail instead;
replace the silent continue with an assertion that fails the test on decode
errors by calling o.Expect(err).NotTo(o.HaveOccurred(), "decode manifest %s
failed", filePath) immediately after decoder.Decode(&doc) (remove the
unreachable continue) so any decoder.Decode failure in the loop (the symbol
decoder.Decode and variable doc) causes the test to fail and reports the
filePath context.
| meta, _ := doc["metadata"].(map[string]interface{}) | ||
| ann, _ := meta["annotations"].(map[string]interface{}) | ||
| if ann == nil || ann[annotation] != "true" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code at lines 141-145 in test/cvo/cvo.go
cd /tmp && find . -name "cvo.go" -path "*/test/cvo/*" 2>/dev/null | head -5Repository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Check if the file exists and read around lines 141-145
cat -n test/cvo/cvo.go | sed -n '130,155p'Repository: openshift/cluster-version-operator
Length of output: 1035
🏁 Script executed:
# Let's search for where annotations come from - look for YAML unmarshaling
rg -A 5 -B 5 "annotations" test/cvo/cvo.go | head -40Repository: openshift/cluster-version-operator
Length of output: 373
🏁 Script executed:
# Search for YAML parsing in the file to understand the source of data
rg -i "yaml\|unmarshal" test/cvo/cvo.goRepository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Check imports to understand if YAML parsing is used
head -30 test/cvo/cvo.goRepository: openshift/cluster-version-operator
Length of output: 830
🏁 Script executed:
# Look for YAML/manifest files that might have annotations
find test -name "*.yaml" -o -name "*.yml" | head -20Repository: openshift/cluster-version-operator
Length of output: 60
🏁 Script executed:
# Check if there are any test manifests with annotations
rg -l "annotation" test/ --type yaml 2>/dev/null || rg "annotation" test/ -A 2 -B 2 | head -30Repository: openshift/cluster-version-operator
Length of output: 1529
🏁 Script executed:
# Look for the test data or fixtures that might show annotation usage
fd -e yaml -e yml test/Repository: openshift/cluster-version-operator
Length of output: 403
🏁 Script executed:
# Check what annotation value this test is looking for
rg "annotation.*=" test/cvo/cvo.go -B 10 -A 10 | head -50Repository: openshift/cluster-version-operator
Length of output: 1596
🏁 Script executed:
# Let's look at more context around the annotation checking logic
cat -n test/cvo/cvo.go | sed -n '115,160p'Repository: openshift/cluster-version-operator
Length of output: 1887
Fix annotation comparison to handle both boolean and string values from YAML decoding.
When YAML contains unquoted true (e.g., release.openshift.io/delete: true), the decoder produces a Go boolean instead of the string "true". The current strict string comparison misses these cases, causing resources marked for deletion to skip verification.
🔧 Suggested fix
- ann, _ := meta["annotations"].(map[string]interface{})
- if ann == nil || ann[annotation] != "true" {
- continue
- }
+ ann, _ := meta["annotations"].(map[string]interface{})
+ if ann == nil {
+ continue
+ }
+ if !strings.EqualFold(fmt.Sprint(ann[annotation]), "true") {
+ continue
+ }📝 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.
| meta, _ := doc["metadata"].(map[string]interface{}) | |
| ann, _ := meta["annotations"].(map[string]interface{}) | |
| if ann == nil || ann[annotation] != "true" { | |
| continue | |
| } | |
| meta, _ := doc["metadata"].(map[string]interface{}) | |
| ann, _ := meta["annotations"].(map[string]interface{}) | |
| if ann == nil { | |
| continue | |
| } | |
| if !strings.EqualFold(fmt.Sprint(ann[annotation]), "true") { | |
| continue | |
| } |
🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 141 - 145, The annotation check in the loop
(variables meta, ann and the key annotation in test/cvo/cvo.go) wrongly assumes
the value is the string "true"; change the check to handle both boolean true and
the string "true" (and treat other truthy forms if present) by retrieving v :=
ann[annotation] and using a type switch or explicit type assertions to accept v
== true or v == "true" (and skip if ann is nil or the value is absent), so
resources with unquoted YAML true are correctly detected for deletion.
test/cvo/cvo.go
Outdated
| err = ocClient.AdmReleaseExtract(manifestDir) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "extract manifests failed") | ||
|
|
||
| entries, err := os.ReadDir(manifestDir.To) |
There was a problem hiding this comment.
nit:
| entries, err := os.ReadDir(manifestDir.To) | |
| files, err := os.ReadDir(manifestDir.To) |
test/cvo/cvo.go
Outdated
| nameLower := strings.ToLower(entry.Name()) | ||
| if strings.Contains(nameLower, "cleanup") { |
There was a problem hiding this comment.
nit: we do not have to introduce a new var and use it only once.
| nameLower := strings.ToLower(entry.Name()) | |
| if strings.Contains(nameLower, "cleanup") { | |
| if strings.Contains(strings.ToLower(entry.Name()), "cleanup") { |
| nameLower := strings.ToLower(entry.Name()) | ||
| if strings.Contains(nameLower, "cleanup") { | ||
| logger.Info(fmt.Sprintf("Skipping file %s because it matches cleanup filter", entry.Name())) | ||
| continue |
There was a problem hiding this comment.
Even with the logging I do not see why we skip it.
Could a resource have cleanup in its file name and annotation release.openshift.io/delete=true?
There was a problem hiding this comment.
Yes, a manifest can have cleanup in its name and annotation release.openshift.io/delete=true in its content at same time.
We can ignore this situation, in my impression this object can be deployed (not entirely sure).
| args = append(args, "-n", namespace) | ||
| } | ||
| _, err := ocClient.Run(args...) | ||
| o.Expect(err).To(o.HaveOccurred(), "The deleted manifest should not be installed, but actually installed") |
There was a problem hiding this comment.
Ha. If I understand the code correctly, you are doing the following command with oc-cli:
$ oc get <kind> <name> -n <namespace>Because KIND|GROUP|VERSION are dynamic, it is not easy to do it via client-go (See how CVO does it). Correct?
It is really nasty, and I really do not want it but I do not have a better way.
HOWEVER, I think this should work (which is much simpler if it does) for your case:
- Parse manifests out of files in payload
- check if a manifest.Raw contains string
release.openshift.io/delete=true;
if yes, save it to a temp file and dooc get -fcommand with the temp file and expect not-found error(s).
You do not need to any yaml/json parsing here. And you will get Manifest for free. GVK is also difficult to use correctly and the way you do it now might not be accurate (for example, you are using Kind only, not Version nor Group).
Let me know what you think about it or you need more clarification.
Unlike other cases, a simple shell script would do the case. But we like Go code more. 🤷
There was a problem hiding this comment.
let me give a try today
| type OC interface { | ||
| AdmReleaseExtract(o ReleaseExtractOptions) error | ||
| Version(o VersionOptions) (string, error) | ||
| Run(args ...string) ([]byte, error) |
There was a problem hiding this comment.
Get(args ...string) (string, error) should be enough for your case.
The method Run() is be for oc run command.
If the idea https://github.com/openshift/cluster-version-operator/pull/1309/changes#r2751255955 works out, I would just do
GetFileExpectNotFoundError(args ...string) (string, error) to avoid abuse of oc get.
There was a problem hiding this comment.
Like I said before https://github.com/openshift/cluster-version-operator/pull/1267/changes#r2579341159
If we adding GetFileExpectNotFoundError(), Run to OC client, we have to adding them to the interface as well. This is really not a good practice for using interface.
There was a problem hiding this comment.
I really don't like to implement interface for a single instance.
| _, err := os.Stat(o.To) | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| c.logger.Info(fmt.Sprintf("the output directory does not exist, will create it: %s", o.To)) | ||
| if err = os.Mkdir(o.To, 0755); err != nil { |
There was a problem hiding this comment.
Let us make the directory in the case, instead of the function.
The method here just calls oc adm release extract (maybe include some logs for debugging), nothing else.
| @@ -70,12 +70,13 @@ func NewOCCli(logger logr.Logger) (api.OC, error) { | |||
| timeout := 30 * time.Second | |||
| timeoutStr := os.Getenv("OC_CLI_TIMEOUT") | |||
There was a problem hiding this comment.
Is 30s too short for oc adm release extract? You want 90s for it?
In that case, let us do another function.
func NewOCCliWithTimeout(logger logr.Logger, timeout time.Duration) (api.OC, error)
and
func NewOCCli(logger logr.Logger) (api.OC, error) {
return NewOCCli(logger, 30 * time.Second) (api.OC, error)
}
We could remove the logic about OC_CLI_TIMEOUT (i think no one is using it at the moment). I have to admit that I did not understand your request here.
Please do it in another commit. I can do it too if you want.
There was a problem hiding this comment.
ok, please go ahead, I will not update my code for now.
BTW, I really do not want to introduce a new function for a parameter.
Today we add NewOCCliWithTimeout for timeout, tomorrow we may add other functions.
test/cvo/cvo.go
Outdated
|
|
||
| g.It(`should not install resources annotated with release.openshift.io/delete=true`, g.Label("Conformance", "High", "42543"), func() { | ||
| // Initialize the ocapi.OC instance | ||
| g.By("Setup ocapi.OC") |
There was a problem hiding this comment.
| g.By("Setup ocapi.OC") | |
| g.By("Setting up oc") |
test/cvo/cvo.go
Outdated
| o.Expect(err).NotTo(o.HaveOccurred(), "Unset environment variable OC_CLI_TIMEOUT failed") | ||
| }() | ||
|
|
||
| g.By("Extract manifests") |
There was a problem hiding this comment.
| g.By("Extract manifests") | |
| g.By("Extracting manifests in the release") |
test/cvo/cvo.go
Outdated
|
|
||
| entries, err := os.ReadDir(manifestDir.To) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| g.By("Start to iterate all manifests") |
There was a problem hiding this comment.
| g.By("Start to iterate all manifests") | |
| g.By("Checking if getting manifests with elease.openshift.io/delete=true on the cluster led to not-found error") |
|
@JianLi-RH: 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. |
…se.openshift.io/delete=true
JianLi-RH
force-pushed
the
automate_42543
branch
from
f175131 to
788dc98
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/cvo/cvo.go`:
- Around line 121-160: The deferred close inside the loop captures the loop
variable file by reference and is unnecessary because file.Close() is already
called explicitly later; remove the inline defer that references closeFilePass
and the closeFilePass variable declaration, and rely on the existing explicit
err = file.Close() error handling block (keep the o.Expect checks around
file.Close()); ensure no other code paths expect closeFilePass and that
file.Close() remains invoked for each opened file.
| file, err := os.Open(filePath) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| defer func() { | ||
| if !closeFilePass { | ||
| // Close the file again | ||
| if err = file.Close(); err != nil { | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "close file failed") | ||
| } | ||
| } | ||
| }() | ||
| decoder := yamlv3.NewDecoder(file) | ||
| for { | ||
| var doc map[string]interface{} | ||
| if err := decoder.Decode(&doc); err != nil { | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| continue | ||
| } | ||
| meta, _ := doc["metadata"].(map[string]interface{}) | ||
| ann, _ := meta["annotations"].(map[string]interface{}) | ||
| if ann == nil || ann[annotation] != "true" { | ||
| continue | ||
| } | ||
| kind, _ := doc["kind"].(string) | ||
| name, _ := meta["name"].(string) | ||
| namespace, _ := meta["namespace"].(string) | ||
| args := []string{"get", kind, name} | ||
| if namespace != "" { | ||
| args = append(args, "-n", namespace) | ||
| } | ||
| _, err := ocClient.Run(args...) | ||
| o.Expect(err).To(o.HaveOccurred(), "The deleted manifest should not be installed, but actually installed") | ||
| } | ||
| // close each file | ||
| err = file.Close() | ||
| if err != nil { | ||
| closeFilePass = false | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "close file failed") | ||
| } |
There was a problem hiding this comment.
File handle management is overly complex and still has a subtle bug.
The defer at line 123 captures file by reference in a loop—all accumulated defers will operate on the last assigned file when the function exits. The closeFilePass flag adds complexity without fixing this. Since you already call file.Close() explicitly at line 156, remove the inner defer entirely.
🐛 Proposed simplification
filePath := filepath.Join(manifestDir.To, manifest.Name())
file, err := os.Open(filePath)
o.Expect(err).NotTo(o.HaveOccurred())
- defer func() {
- if !closeFilePass {
- // Close the file again
- if err = file.Close(); err != nil {
- o.Expect(err).NotTo(o.HaveOccurred(), "close file failed")
- }
- }
- }()
decoder := yamlv3.NewDecoder(file)
for {
// ... decode loop unchanged ...
}
- // close each file
- err = file.Close()
- if err != nil {
- closeFilePass = false
- o.Expect(err).NotTo(o.HaveOccurred(), "close file failed")
- }
+ file.Close()
}Also remove the closeFilePass variable declaration at line 114.
🤖 Prompt for AI Agents
In `@test/cvo/cvo.go` around lines 121 - 160, The deferred close inside the loop
captures the loop variable file by reference and is unnecessary because
file.Close() is already called explicitly later; remove the inline defer that
references closeFilePass and the closeFilePass variable declaration, and rely on
the existing explicit err = file.Close() error handling block (keep the o.Expect
checks around file.Close()); ensure no other code paths expect closeFilePass and
that file.Close() remains invoked for each opened file.
3 participants
Test case: https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-42543
Test it locally:
/cc @hongkailiu @DavidHurta