MGMT-24783: Allow getting OS Images by RHCOS version#965
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-24783 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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. |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giladravid16 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesRHCOS image storage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Populate
participant deduplicateVersions
participant ISODownloader
participant extractAndCacheNmstatectl
Populate->>deduplicateVersions: configured RHCOS entries
deduplicateVersions-->>Populate: selected entries
Populate->>ISODownloader: download and validate full ISO
Populate->>extractAndCacheNmstatectl: selected imageInfo
extractAndCacheNmstatectl-->>Populate: cached nmstatectl
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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: 2
🤖 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 `@pkg/imagestore/imagestore_test.go`:
- Around line 779-783: The test for deduplicateVersions is flaky because it
assumes a stable map-iteration order and checks dedupedVersions[0] directly.
Update the assertions in imagestore_test.go to locate the expected entry by its
(version, arch) key from deduplicateVersions’ result instead of relying on index
0, and include a failure message so the test clearly reports when the expected
version/arch entry is missing or incorrect.
In `@pkg/imagestore/imagestore.go`:
- Around line 392-429: The deduplicateVersions helper returns results in
non-deterministic order because it ranges over map m, so the flaky index-based
assertion should be fixed in the unit test instead of changing the helper.
Update the deduplicateVersions test to avoid depending on dedupedVersions[0] and
instead verify the expected entries by matching version and cpu_architecture,
keeping the existing Populate behavior unchanged.
🪄 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: f592832b-777d-4d71-bcbc-3002164340d5
📒 Files selected for processing (2)
pkg/imagestore/imagestore.gopkg/imagestore/imagestore_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
==========================================
+ Coverage 62.13% 62.35% +0.22%
==========================================
Files 27 27
Lines 1793 1833 +40
==========================================
+ Hits 1114 1143 +29
- Misses 512 521 +9
- Partials 167 169 +2
🚀 New features to boost your workflow:
|
|
/retest-required |
|
/retest-required |
|
/cc @carbonin |
|
@danielerez please review |
We're still going to allow getting OS Images by openshift version, but we want to also allow and prioritize getting them by RHCOS version. We also don't want to keep in our storage multiple ISOs that are identical which happen when different openshift versions use the same RHCOS version, so we're going to dedup them before populating the image store.
|
@giladravid16: 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. |
4 participants
Description
We're still going to allow getting OS Images by openshift version, but we want to also allow and prioritize getting them by RHCOS version.
We also don't want to keep in our storage multiple ISOs that are identical which happen when different openshift versions use the same RHCOS version, so we're going to dedup them before populating the image store.
How was this code tested?
Manually installed clusters with both full and minimal ISOs.
I had 2 openshift versions with the same RHCOS version, I could get the ISO for both of them and the storage directory only had a single copy.
Assignees
/cc @
/cc @
Links
Closes MGMT-24783
Checklist
docs, README, etc)Summary by CodeRabbit