MGMT-18886: OS Streams#10428
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@giladravid16: This pull request references MGMT-18886 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 epic 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. |
WalkthroughThis PR adds design documentation for an enhancement to support selecting multiple OS streams per Release Image. The proposal separates OS images from release images into a many-to-many mapping, introduces new API fields for cluster and infraenv selection, extends the Agent kube-api with osStream field, and outlines binding logic updates, automation considerations, risks, and test plans. ChangesOS Streams Enhancement Proposal
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
![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
🧹 Nitpick comments (1)
docs/enhancements/os-streams.md (1)
109-113: ⚡ Quick winExpand the test plan beyond a single e2e path.
Please add coverage expectations for resolver logic (default stream selection, invalid stream, backward-compat with existing
osImageVersion, early-binding vs late-binding precedence). This is key risk area for this change.🤖 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 `@docs/enhancements/os-streams.md` around lines 109 - 113, Update the Test Plan to enumerate specific coverage expectations for the resolver logic: add test cases for default stream selection when none provided, handling of invalid/unknown stream values, backward-compatibility checks ensuring existing osImageVersion continues to resolve to the same images, and explicit tests for early-binding vs late-binding precedence (confirm which wins and that conflicts are surfaced or resolved correctly). Reference the resolver behavior by name (resolver logic, default stream selection, osImageVersion, early-binding, late-binding) and state expected outcomes/assertions for each test so assisted-test-infra updates can implement them.
🤖 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 `@docs/enhancements/os-streams.md`:
- Around line 83-86: The os_image_version field currently has dual semantics
(RHCOS or OCP version) which is ambiguous; update the docs and validation logic
to define explicit resolution rules: implement a resolver (e.g.,
resolve_os_image_version) that first attempts an exact RHCOS match, then falls
back to an exact OCP match only if no RHCOS is found, and otherwise return a
clear validation error; document the precedence, input validation, and the error
behavior (including what happens in late-binding) and add guidance to users to
provide an explicit indicator or use distinct fields if both interpretations are
possible.
- Around line 39-46: The proposal currently uses inconsistent names (os_stream,
os_image_version, osStream) which can cause drift between REST, DB and kube CRD;
standardize and document the exact field names for each surface and migration
strategy: use snake_case for REST/DB (os_stream, os_image_version) or camelCase
for kube CRD (osStream, osImageVersion) consistently, update the API contract in
assisted-service to accept and emit the chosen REST names, update the DB model
to match the REST names, update the kube-api CRD and AgentClusterInstalls to use
the chosen kube names (e.g., osStream/osImageVersion), and add a
migration/deprecation path that accepts legacy variants (map incoming os_stream
↔ osStream and os_image_version ↔ osImageVersion), logs deprecation warnings,
and migrates stored records accordingly; reference os_stream, os_image_version,
osStream, osImageVersion, AgentClusterInstalls, assisted-service and kube-api in
the docs and code changes so all surfaces remain aligned.
---
Nitpick comments:
In `@docs/enhancements/os-streams.md`:
- Around line 109-113: Update the Test Plan to enumerate specific coverage
expectations for the resolver logic: add test cases for default stream selection
when none provided, handling of invalid/unknown stream values,
backward-compatibility checks ensuring existing osImageVersion continues to
resolve to the same images, and explicit tests for early-binding vs late-binding
precedence (confirm which wins and that conflicts are surfaced or resolved
correctly). Reference the resolver behavior by name (resolver logic, default
stream selection, osImageVersion, early-binding, late-binding) and state
expected outcomes/assertions for each test so assisted-test-infra updates can
implement them.
🪄 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: 30661950-308a-449d-aafe-beb8c4100084
📒 Files selected for processing (1)
docs/enhancements/os-streams.md
| specifies the OCP version and can only be used in late-binding. We can repurpose | ||
| this field to match our new `os_image_version` field, but still allow setting it to | ||
| the OCP version if the value doesn't match any RHCOS version. | ||
|
|
There was a problem hiding this comment.
Avoid dual semantics in one field without strict resolution rules.
Allowing os_image_version to mean either “RHCOS version” or “OCP version” is ambiguous and can select the wrong image silently. Define explicit precedence, validation, and error behavior (including what happens when both interpretations are possible).
🤖 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 `@docs/enhancements/os-streams.md` around lines 83 - 86, The os_image_version
field currently has dual semantics (RHCOS or OCP version) which is ambiguous;
update the docs and validation logic to define explicit resolution rules:
implement a resolver (e.g., resolve_os_image_version) that first attempts an
exact RHCOS match, then falls back to an exact OCP match only if no RHCOS is
found, and otherwise return a clear validation error; document the precedence,
input validation, and the error behavior (including what happens in
late-binding) and add guidance to users to provide an explicit indicator or use
distinct fields if both interpretations are possible.
|
[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 |
openshift-ci
Bot
added
the
approved
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
I'd really prefer if we could avoid a new map and get the data from the release images.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/enhancements/os-streams.md (1)
32-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin down the cross-surface contract before implementation.
os_stream/osStreamandosImageVersion/openshift_versionare still described inconsistently, and the fallback from RHCOS to OCP version remains ambiguous. That leaves room for REST, kube-api, admission, and controller code to diverge or silently pick the wrong image. Please specify the exact field name and resolution rules for each surface, plus the migration/deprecation behavior for existingInfraEnvvalues.Also applies to: 87-93
🤖 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 `@docs/enhancements/os-streams.md` around lines 32 - 38, Before implementing the os_stream feature, clarify and document the cross-surface contract by specifying the exact field names for each surface (REST API uses `os_stream` while kube-api uses `osStream`), define consistent naming for image version fields (`osImageVersion` vs `openshift_version`), establish clear resolution rules for how os_stream is determined and what the fallback mechanism is from RHCOS to OCP version, and document the migration and deprecation strategy for existing InfraEnv values to ensure REST, kube-api, admission, and controller code remain consistent. Include these specifications in the enhancement document at both the main location and the secondary location (around line 87-93) so all implementation surfaces follow the same contract.
🤖 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.
Duplicate comments:
In `@docs/enhancements/os-streams.md`:
- Around line 32-38: Before implementing the os_stream feature, clarify and
document the cross-surface contract by specifying the exact field names for each
surface (REST API uses `os_stream` while kube-api uses `osStream`), define
consistent naming for image version fields (`osImageVersion` vs
`openshift_version`), establish clear resolution rules for how os_stream is
determined and what the fallback mechanism is from RHCOS to OCP version, and
document the migration and deprecation strategy for existing InfraEnv values to
ensure REST, kube-api, admission, and controller code remain consistent. Include
these specifications in the enhancement document at both the main location and
the secondary location (around line 87-93) so all implementation surfaces follow
the same contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5fd5d703-3b81-4e00-a087-bde9b6018c05
📒 Files selected for processing (1)
docs/enhancements/os-streams.md
|
@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. |
3 participants
Enhancement to define the implementation of os streams.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit