OCPBUGS-61465: Updated openshift-tests images to utilize mapped images from external binary#30873
OCPBUGS-61465: Updated openshift-tests images to utilize mapped images from external binary#30873jubittajohn wants to merge 2 commits intoopenshift:mainfrom
Conversation
Signed-off-by: jubittajohn <jujohn@redhat.com>
… binary Signed-off-by: jubittajohn <jujohn@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
jira/valid-reference
|
@jubittajohn: This pull request references Jira Issue OCPBUGS-61465, which is invalid:
Comment 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jubittajohn 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 |
WalkthroughThe changes refactor the image handling pipeline by updating the extension binary interface to return raw image data directly ([]extension.Image) instead of constructed ImageSets. The command layer is modified to handle multiple image set configurations with exception filtering and conditional branching based on the OPENSHIFT_SKIP_EXTERNAL_TESTS flag. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jubittajohn: This pull request references Jira Issue OCPBUGS-61465, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/openshift-tests/images/images_command.go`:
- Around line 190-210: The issue is that defaultImageSets and updatedImageSets
can diverge because when image.Mapped is nil you append to defaultImageSets but
skip updatedImageSets; fix by always appending an entry to updatedImageSets for
each defaultImageSets entry: inside the loop over imagesFromBinaries (the block
that builds initialSet/defaultImageSets), create a mappedConfig variable set to
convertImageToImageConfig(*image.Mapped) when image.Mapped != nil otherwise
fallback to origImageConfig, then always append extensions.ImageSet{imageID:
mappedConfig} to updatedImageSets so indexes of defaultImageSets and
updatedImageSets remain aligned for later iteration over updatedImageSets.
In `@pkg/test/extensions/binary.go`:
- Around line 759-765: In the loop calling binary.ListImages, after sending the
error to errCh you must immediately skip further processing for that iteration
(add a continue) so you don't append a potentially empty/invalid images slice;
update the block referencing binary.ListImages, errCh, images, allImages and mu
to send the error then continue. Also apply the same fix to the analogous error
handling in ListTests (the block around ListTests/errCh/images at lines
~824-826) so both places consistently return to the loop after reporting errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6bebf10-e522-4ab1-a986-c8981f1a086c
📒 Files selected for processing (2)
pkg/cmd/openshift-tests/images/images_command.gopkg/test/extensions/binary.go
| for _, image := range imagesFromBinaries { | ||
| // Add original image | ||
| imageID := k8simage.ImageID(image.Index) | ||
| origImageConfig := convertImageToImageConfig(image) | ||
|
|
||
| initialSet := extensions.ImageSet{imageID: origImageConfig} | ||
| initialImageSets = append(initialImageSets, initialSet) | ||
|
|
||
| // Only add to default and mapped if original image passes exceptions | ||
| if !imageMatchesExceptions(origImageConfig, exceptions) { | ||
| defaultImageSets = append(defaultImageSets, initialSet) | ||
|
|
||
| if image.Mapped != nil { | ||
| mappedSet := extensions.ImageSet{ | ||
| imageID: convertImageToImageConfig(*image.Mapped), | ||
| } | ||
| updatedImageSets = append(updatedImageSets, mappedSet) | ||
| } | ||
| } | ||
| } | ||
| if len(filtered) > 0 { | ||
| defaultImageSets = append(defaultImageSets, filtered) | ||
| } | ||
| } | ||
|
|
||
| // Created a new slice with the updatedImageSets addresses for the images | ||
| updatedImageSets := []extensions.ImageSet{} | ||
| for i := range defaultImageSets { | ||
| updatedImageSets = append(updatedImageSets, k8simage.GetMappedImageConfigs(defaultImageSets[i], ref.Exact())) | ||
| } |
There was a problem hiding this comment.
Potential index mismatch between defaultImageSets and updatedImageSets.
When image.Mapped is nil (Line 202), the image is added to defaultImageSets but not to updatedImageSets. This creates a size mismatch between the two slices.
Later at Lines 270-272, the code iterates over updatedImageSets using index i and accesses defaultImageSets[i][imageID]:
for i := range updatedImageSets {
for imageID := range updatedImageSets[i] {
a, b := defaultImageSets[i][imageID], updatedImageSets[i][imageID]If defaultImageSets has more entries than updatedImageSets (because some images lack Mapped data), accessing defaultImageSets[i] won't correspond to the correct original image for updatedImageSets[i].
Consider either:
- Always adding an entry to
updatedImageSets(using the original config as a fallback whenMappedis nil), or - Using a different data structure that maintains the relationship between original and mapped images.
Proposed fix: Always populate updatedImageSets
// Only add to default and mapped if original image passes exceptions
if !imageMatchesExceptions(origImageConfig, exceptions) {
defaultImageSets = append(defaultImageSets, initialSet)
if image.Mapped != nil {
mappedSet := extensions.ImageSet{
imageID: convertImageToImageConfig(*image.Mapped),
}
updatedImageSets = append(updatedImageSets, mappedSet)
+ } else {
+ // Use original config as fallback when no mapping exists
+ updatedImageSets = append(updatedImageSets, initialSet)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/openshift-tests/images/images_command.go` around lines 190 - 210, The
issue is that defaultImageSets and updatedImageSets can diverge because when
image.Mapped is nil you append to defaultImageSets but skip updatedImageSets;
fix by always appending an entry to updatedImageSets for each defaultImageSets
entry: inside the loop over imagesFromBinaries (the block that builds
initialSet/defaultImageSets), create a mappedConfig variable set to
convertImageToImageConfig(*image.Mapped) when image.Mapped != nil otherwise
fallback to origImageConfig, then always append extensions.ImageSet{imageID:
mappedConfig} to updatedImageSets so indexes of defaultImageSets and
updatedImageSets remain aligned for later iteration over updatedImageSets.
| images, err := binary.ListImages(ctx) | ||
| if err != nil { | ||
| errCh <- err | ||
| } | ||
| mu.Lock() | ||
| allImages = append(allImages, imageConfig) | ||
| allImages = append(allImages, images...) | ||
| mu.Unlock() |
There was a problem hiding this comment.
Missing continue after error handling.
When ListImages returns an error, the error is sent to errCh, but execution continues to append the (likely empty) images slice. Consider adding a continue statement after sending the error to errCh for cleaner control flow, consistent with typical error-then-continue patterns.
Note: The same pattern exists at Line 824-826 in ListTests, so this appears to be a pre-existing pattern in this file.
Proposed fix
images, err := binary.ListImages(ctx)
if err != nil {
errCh <- err
+ continue
}
mu.Lock()
allImages = append(allImages, images...)
mu.Unlock()📝 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.
| images, err := binary.ListImages(ctx) | |
| if err != nil { | |
| errCh <- err | |
| } | |
| mu.Lock() | |
| allImages = append(allImages, imageConfig) | |
| allImages = append(allImages, images...) | |
| mu.Unlock() | |
| images, err := binary.ListImages(ctx) | |
| if err != nil { | |
| errCh <- err | |
| continue | |
| } | |
| mu.Lock() | |
| allImages = append(allImages, images...) | |
| mu.Unlock() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/test/extensions/binary.go` around lines 759 - 765, In the loop calling
binary.ListImages, after sending the error to errCh you must immediately skip
further processing for that iteration (add a continue) so you don't append a
potentially empty/invalid images slice; update the block referencing
binary.ListImages, errCh, images, allImages and mu to send the error then
continue. Also apply the same fix to the analogous error handling in ListTests
(the block around ListTests/errCh/images at lines ~824-826) so both places
consistently return to the loop after reporting errors.
|
Scheduling required tests: |
|
@jubittajohn: 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. |
2 participants
Updated openshift-tests images to utilize mapped images from external binary
Summary by CodeRabbit
Release Notes