METAL-1728: rhcos: add multi-stream format to rhcos.json for RHCOS 10 support#10394
METAL-1728: rhcos: add multi-stream format to rhcos.json for RHCOS 10 support#10394honza wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@honza: This pull request references METAL-1728 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 story to target the "4.22.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-robot
added
the
jira/valid-reference
WalkthroughRestructures Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ 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 Tip Migrating from UI to YAML configuration.Use the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rhcos/builds.go (1)
71-92:⚠️ Potential issue | 🟠 MajorScope marketplace merging to the selected stream.
FetchCoreOSBuildByStreamis stream-aware, but the marketplace merge is still global. Oncerhel-coreos-10is populated, this will return 10.x stream data decorated with the 4.21 marketplace payload.🛠️ Minimal safeguard
st, ok := store[streamName] if !ok { return nil, fmt.Errorf("stream %q not found in CoreOS stream store", streamName) } + + if streamName != DefaultCoreOSStreamName { + return st, nil + } // Merge marketplace json file into stream json file mktBody, err := fetchRawMarketplaceStream()Longer term, the marketplace file likely needs the same multi-stream shape as
rhcos.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rhcos/builds.go` around lines 71 - 92, The marketplace merge is applied globally but needs to be scoped to the selected stream returned by FetchCoreOSBuildByStream; change the merge to only apply marketplace entries for the specific stream key in the parsed marketplace payload (mktSt) instead of using mktSt directly as a global map. After json.Unmarshal into marketplaceStream, select the sub-map for the current stream (use the stream identifier available on st, e.g. st.Stream or st.Name) and then iterate st.Architectures merging only those entries into arch.RHELCoreOSExtensions.Marketplace; keep the existing nil-check/initialization for RHELCoreOSExtensions and return st as before.
🧹 Nitpick comments (1)
hack/build-coreos-manifest.go (1)
74-87: Reuse the shared default stream constant here.This hardcodes the default stream a second time. The next stream bump now has to update both this script and
pkg/rhcos.DefaultCoreOSStreamName, or the generatedcoreos-bootimagesConfigMap can drift fromFetchCoreOSBuild().♻️ Suggested change
-const rhcosStreamName = "rhcos-4.21" - func getBootImages() ([]byte, error) { @@ - streamName = rhcosStreamName + streamName = installerrhcos.DefaultCoreOSStreamNameAdd an aliased import for
github.com/openshift/installer/pkg/rhcosabove to avoid the existingrhcosimport collision.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-coreos-manifest.go` around lines 74 - 87, Replace the hardcoded rhcosStreamName usage in getBootImages with the shared constant from pkg/rhcos (pkg/rhcos.DefaultCoreOSStreamName) so the default stream value is maintained in one place; to do this update the imports to add an aliased import for github.com/openshift/installer/pkg/rhcos to avoid colliding with the existing rhcos symbol, then use that aliased package's DefaultCoreOSStreamName inside getBootImages instead of the local rhcosStreamName constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev/pinned-coreos.md`:
- Around line 47-59: The generated stream JSON lacks a "stream" field because
the plume cosa2stream invocation doesn't set the stream name; update the plume
cosa2stream command (the invocation of plume cosa2stream that writes to
/tmp/rhcos10.json) to include the flag --name rhel-coreos-10 so the produced
JSON contains the correct "stream" value before the jq merge into the
rhel-coreos-10 key in data/data/coreos/rhcos.json.
---
Outside diff comments:
In `@pkg/rhcos/builds.go`:
- Around line 71-92: The marketplace merge is applied globally but needs to be
scoped to the selected stream returned by FetchCoreOSBuildByStream; change the
merge to only apply marketplace entries for the specific stream key in the
parsed marketplace payload (mktSt) instead of using mktSt directly as a global
map. After json.Unmarshal into marketplaceStream, select the sub-map for the
current stream (use the stream identifier available on st, e.g. st.Stream or
st.Name) and then iterate st.Architectures merging only those entries into
arch.RHELCoreOSExtensions.Marketplace; keep the existing
nil-check/initialization for RHELCoreOSExtensions and return st as before.
---
Nitpick comments:
In `@hack/build-coreos-manifest.go`:
- Around line 74-87: Replace the hardcoded rhcosStreamName usage in
getBootImages with the shared constant from pkg/rhcos
(pkg/rhcos.DefaultCoreOSStreamName) so the default stream value is maintained in
one place; to do this update the imports to add an aliased import for
github.com/openshift/installer/pkg/rhcos to avoid colliding with the existing
rhcos symbol, then use that aliased package's DefaultCoreOSStreamName inside
getBootImages instead of the local rhcosStreamName constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50358391-549e-48e4-9ac6-928a3e678038
📒 Files selected for processing (4)
data/data/coreos/rhcos.jsondocs/dev/pinned-coreos.mdhack/build-coreos-manifest.gopkg/rhcos/builds.go
Convert data/data/coreos/rhcos.json from a single stream.Stream object to a named map keyed by stream name. The existing RHCOS 9 content is placed under the "rhcos-4.21" key and an empty "rhel-coreos-10" placeholder is added for future population by ART. Update pkg/rhcos/builds.go to parse the new multi-stream format: - Add streamStore type and DefaultCoreOSStreamName constant - Add FetchCoreOSBuildByStream for selecting a stream by name - Keep FetchCoreOSBuild as a backward-compatible wrapper Update hack/build-coreos-manifest.go to extract the target stream from the multi-stream map before generating the coreos-bootimages ConfigMap. Update docs/dev/pinned-coreos.md with the new jq-based workflow for updating individual streams within the multi-stream file.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/dev/pinned-coreos.md (1)
65-67: Missing blank line before heading.There should be a blank line between line 66 and the heading on line 67 for proper Markdown rendering.
- https://github.com/coreos/coreos-assembler/pull/2000 - https://github.com/coreos/coreos-assembler/pull/2052 + ### Origin of stream metadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev/pinned-coreos.md` around lines 65 - 67, Add a blank line before the "### Origin of stream metadata" heading so the preceding list items render correctly as separate block and the heading is recognized; edit the markdown around the "### Origin of stream metadata" heading to insert an empty line between the list lines ("https://github.com/coreos/coreos-assembler/pull/2000" / "https://github.com/coreos/coreos-assembler/pull/2052") and the heading.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/dev/pinned-coreos.md`:
- Around line 65-67: Add a blank line before the "### Origin of stream metadata"
heading so the preceding list items render correctly as separate block and the
heading is recognized; edit the markdown around the "### Origin of stream
metadata" heading to insert an empty line between the list lines
("https://github.com/coreos/coreos-assembler/pull/2000" /
"https://github.com/coreos/coreos-assembler/pull/2052") and the heading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c60c4bd-8820-4144-8d16-f153cc05852e
📒 Files selected for processing (4)
data/data/coreos/rhcos.jsondocs/dev/pinned-coreos.mdhack/build-coreos-manifest.gopkg/rhcos/builds.go
Parse the new multi-stream rhcos.json format (keyed by stream name) introduced in openshift/installer#10394. A COREOS_VERSIONS build arg controls which versions to bundle (default: 9), and a COREOS_VERSION runtime env var selects which version the ironic-python-agent symlinks point to. Backward compatible: default behavior is unchanged.
|
See #10321, which is adding logic for |
|
@honza: The following test 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. |
|
Closing in favour of #10321 |
3 participants
Convert data/data/coreos/rhcos.json from a single stream.Stream object to a named map keyed by stream name. The existing RHCOS 9 content is placed under the "rhcos-4.21" key and an empty "rhel-coreos-10" placeholder is added for future population by ART.
Update pkg/rhcos/builds.go to parse the new multi-stream format:
Update hack/build-coreos-manifest.go to extract the target stream from the multi-stream map before generating the coreos-bootimages ConfigMap.
Update docs/dev/pinned-coreos.md with the new jq-based workflow for updating individual streams within the multi-stream file.