OCPBUGS-88726: Fix streams in imagemode tests#6195
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughRemoves the static ChangesDynamic OS Image Stream Detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sergiordlr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended-priv/mco_ocb.go (1)
494-526: 💤 Low valueContainerfile generation silently tolerates ncurses override failures.
Line 523 includes
|| trueafter therpm-ostree override replacecommand, which silently ignores all errors from the ncurses override step. While this may be intentional to handle version mismatches across streams, it could also hide legitimate failures (e.g., network issues, authentication problems, or repo configuration errors).Consider logging a warning or verifying the override succeeded when it's expected to work, rather than unconditionally suppressing all errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended-priv/mco_ocb.go` around lines 494 - 526, The containerfile content generated by the getConnectedCustomContainerFileContent function uses `|| true` after the `rpm-ostree override replace` command, which silently suppresses all errors including legitimate failures like network issues or repo configuration errors. Modify the Containerfile string to either remove the `|| true` to allow the build to fail on actual errors, or add logging within the Containerfile itself (using a shell conditional) to document why the override failure is being tolerated. Ensure the approach clearly distinguishes between intentional version mismatch handling and actual failures that should not be hidden.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/mco_osimagestream.go`:
- Around line 308-311: The GetOSImageStreamMajorVersion function lacks input
validation and will return an empty or malformed string if the stream parameter
is empty or doesn't start with "rhel-". Add validation at the beginning of the
function to check that stream is not empty and actually starts with the "rhel-"
prefix before calling strings.TrimPrefix. If the validation fails, return an
empty string or handle the error appropriately to prevent downstream failures in
dependent code.
---
Nitpick comments:
In `@test/extended-priv/mco_ocb.go`:
- Around line 494-526: The containerfile content generated by the
getConnectedCustomContainerFileContent function uses `|| true` after the
`rpm-ostree override replace` command, which silently suppresses all errors
including legitimate failures like network issues or repo configuration errors.
Modify the Containerfile string to either remove the `|| true` to allow the
build to fail on actual errors, or add logging within the Containerfile itself
(using a shell conditional) to document why the override failure is being
tolerated. Ensure the approach clearly distinguishes between intentional version
mismatch handling and actual failures that should not be hidden.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cda77102-e86f-4d4f-b288-2fa6ee999ccc
📒 Files selected for processing (4)
test/extended-priv/const.gotest/extended-priv/mco_metrics.gotest/extended-priv/mco_ocb.gotest/extended-priv/mco_osimagestream.go
💤 Files with no reviewable changes (1)
- test/extended-priv/const.go
| // GetOSImageStreamMajorVersion extracts the major version from a stream name (e.g. "rhel-9" -> "9", "rhel-10" -> "10"). | ||
| func GetOSImageStreamMajorVersion(stream string) string { | ||
| return strings.TrimPrefix(stream, "rhel-") | ||
| } |
There was a problem hiding this comment.
Missing validation in stream major version extraction.
GetOSImageStreamMajorVersion calls strings.TrimPrefix without validating the input format. If stream is empty or doesn't start with "rhel-", it returns an empty or malformed string, causing downstream failures (e.g., invalid CentOS repo URLs in mco_metrics.go line 37, invalid containerfile in mco_ocb.go line 506).
🛡️ Proposed fix with validation
-// GetOSImageStreamMajorVersion extracts the major version from a stream name (e.g. "rhel-9" -> "9", "rhel-10" -> "10").
+// GetOSImageStreamMajorVersion extracts the major version from a stream name (e.g. "rhel-9" -> "9", "rhel-10" -> "10").
+// It validates the stream name format and panics if the input is invalid.
func GetOSImageStreamMajorVersion(stream string) string {
+ o.Expect(stream).NotTo(o.BeEmpty(), "Stream name cannot be empty")
+ o.Expect(stream).To(o.HavePrefix("rhel-"), "Stream name must start with 'rhel-', got: %s", stream)
return strings.TrimPrefix(stream, "rhel-")
}📝 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.
| // GetOSImageStreamMajorVersion extracts the major version from a stream name (e.g. "rhel-9" -> "9", "rhel-10" -> "10"). | |
| func GetOSImageStreamMajorVersion(stream string) string { | |
| return strings.TrimPrefix(stream, "rhel-") | |
| } | |
| // GetOSImageStreamMajorVersion extracts the major version from a stream name (e.g. "rhel-9" -> "9", "rhel-10" -> "10"). | |
| // Returns an error if the stream name format is invalid. | |
| func GetOSImageStreamMajorVersion(stream string) (string, error) { | |
| if stream == "" { | |
| return "", fmt.Errorf("stream name cannot be empty") | |
| } | |
| if !strings.HasPrefix(stream, "rhel-") { | |
| return "", fmt.Errorf("stream name must start with 'rhel-', got: %s", stream) | |
| } | |
| return strings.TrimPrefix(stream, "rhel-"), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended-priv/mco_osimagestream.go` around lines 308 - 311, The
GetOSImageStreamMajorVersion function lacks input validation and will return an
empty or malformed string if the stream parameter is empty or doesn't start with
"rhel-". Add validation at the beginning of the function to check that stream is
not empty and actually starts with the "rhel-" prefix before calling
strings.TrimPrefix. If the validation fails, return an empty string or handle
the error appropriately to prevent downstream failures in dependent code.
|
@sergiordlr: 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. |
sergiordlr
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@sergiordlr: This pull request references Jira Issue OCPBUGS-88726, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
2 participants
- What I did
I made tests to take into account the osImageStream to select the centos stream when they use it create a containerfile
- How to verify it
Modified tests should pass
Summary by CodeRabbit
Tests
Chores