Extend Node Image Info to handle streams#749
Merged
sdodson merged 5 commits intoopenshift:mainfrom Apr 8, 2026
Merged
Conversation
…overy Add ListMachineOSStreams from release payload JSON (coreos + extensions pairing, io.openshift.build.version-display-names for titles). Extend ReleaseInfo with ImageReferenceForComponent, RpmListForStream, RpmDiffForStream, and groupcache keys. Add ReleaseTagIsDualRHCOS semver helper for 4.21+. Co-authored-by: Cursor <noreply@cursor.com> Assisted-by: Cursor AI (Composer)
Transform RHEL 9/10 and CentOS CoreOS lines; add NodeImageSectionMarkdown using ListMachineOSStreams for generic per-stream RPM lists and diffs. Render Node Image Info when streams exist even without #node-image-info anchor. Co-authored-by: Cursor <noreply@cursor.com> Assisted-by: Cursor AI (Composer)
… exist Match changelog-preview behavior: load node section when the changelog has #node-image-info or ListMachineOSStreams returns streams for the target release. Co-authored-by: Cursor <noreply@cursor.com> Assisted-by: Cursor AI (Composer)
Local tool to run ChangeLog + RHCOS transforms without a cluster (requires oc). Ignore generated sample-changelog.md. Co-authored-by: Cursor <noreply@cursor.com> Assisted-by: Cursor AI (Composer)
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
/retest-required |
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkg/release-controller/machine_os_tags_test.go (1)
71-78: Consider testing additional known tag mappings.The test covers the
DisplayNamepresent case and the emptyDisplayNamefallback, but doesn't test the hardcoded fallbacks for known tags likerhel-coreos-10orstream-coreosdefined inMachineOSTitle.💡 Suggested additional assertions
func TestMachineOSTitle(t *testing.T) { if got := MachineOSTitle(MachineOSStreamInfo{Tag: "rhel-coreos", DisplayName: "Red Hat Enterprise Linux CoreOS"}); got != "Red Hat Enterprise Linux CoreOS (`rhel-coreos`)" { t.Errorf("got %q", got) } if got := MachineOSTitle(MachineOSStreamInfo{Tag: "custom-stream", DisplayName: ""}); got != "Machine OS (`custom-stream`)" { t.Errorf("got %q", got) } + // Test hardcoded fallbacks for known tags without DisplayName + if got := MachineOSTitle(MachineOSStreamInfo{Tag: "rhel-coreos", DisplayName: ""}); got != "Red Hat Enterprise Linux CoreOS (`rhel-coreos`)" { + t.Errorf("rhel-coreos fallback: got %q", got) + } + if got := MachineOSTitle(MachineOSStreamInfo{Tag: "rhel-coreos-10", DisplayName: ""}); got != "Red Hat Enterprise Linux CoreOS 10 (`rhel-coreos-10`)" { + t.Errorf("rhel-coreos-10 fallback: got %q", got) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/machine_os_tags_test.go` around lines 71 - 78, Add assertions to TestMachineOSTitle to cover the hardcoded fallback mappings inside MachineOSTitle: call MachineOSTitle with MachineOSStreamInfo having Tag values like "rhel-coreos-10" and "stream-coreos" (and empty DisplayName) and assert the returned string matches the expected human-readable fallbacks (e.g., the specific names MachineOSTitle maps those tags to, including the backticked tag in parentheses). Locate the test function TestMachineOSTitle and extend it with these additional checks using the same pattern as the existing assertions to ensure the known-tag branches are exercised.hack/changelog-preview/main.go (1)
75-77: Consider extracting the "Node Image Info" header as a constant or shared function.The header
"## Node Image Info"is hardcoded here and also rendered incmd/release-controller-api/http_changelog.go(line 197). To maintain consistency, consider extracting this to a shared constant or havingNodeImageSectionMarkdowninclude the header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/changelog-preview/main.go` around lines 75 - 77, The hardcoded header "## Node Image Info" is duplicated; update the code so the header is defined once and reused: either add a shared constant (e.g., NodeImageInfoHeader) and replace the literal in hack/changelog-preview/main.go (where nodeMD is appended to out) and in cmd/release-controller-api/http_changelog.go, or move the header into the NodeImageSectionMarkdown implementation so callers just append nodeMD without adding the header; ensure all references (nodeMD, NodeImageSectionMarkdown) use the single source of truth for the header.pkg/rhcos/rhcos_test.go (1)
234-246: Consider making the assertion more specific.The test checks for at least 2 occurrences of
"coreos-base-alert", which validates that dual CoreOS rendering works. However, checking for exactly 2 would be more precise if dual RHCOS should produce exactly two infoboxes.💡 Suggested improvement
- if strings.Count(out, "coreos-base-alert") < 2 { - t.Fatalf("expected two CoreOS base layer infoboxes, got:\n%s", out) + count := strings.Count(out, "coreos-base-alert") + if count != 2 { + t.Fatalf("expected exactly two CoreOS base layer infoboxes, got %d:\n%s", count, out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rhcos/rhcos_test.go` around lines 234 - 246, The test TestTransformMarkDownOutputDualRHCOSLines currently asserts that strings.Count(out, "coreos-base-alert") < 2 to ensure at least two infoboxes; change this to assert exactly two occurrences (e.g., if strings.Count(out, "coreos-base-alert") != 2) and update the failure message accordingly so the test fails when there are more or fewer than two coreos-base-alert infoboxes, ensuring precise verification of dual RHCOS rendering.pkg/release-controller/semver_dual_rhcos_test.go (1)
5-24: Consider adding edge cases for more complete coverage.The test covers the main scenarios well but could benefit from additional edge cases:
- Major version != 4 (e.g.,
"5.21.0"should returnfalse)- Empty string input
- Partial/incomplete versions that
SemverParseTolerantmight handle (e.g.,"4.21")💡 Suggested additional test cases
tests := []struct { tag string want bool }{ {"4.21.0-ec.1", true}, {"4.21.0", true}, {"4.22.1", true}, {"4.20.0", false}, {"4.20.0-ec.0", false}, {"not-a-version", false}, + {"", false}, + {"5.21.0", false}, // Major != 4 + {"3.21.0", false}, // Major != 4 + {"4.21", true}, // Two-part version (if SemverParseTolerant handles it) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/semver_dual_rhcos_test.go` around lines 5 - 24, Add tests for missing edge cases to TestReleaseTagIsDualRHCOS: include a non-4 major version like "5.21.0" expecting false, an empty-string input "" expecting false, and a partial version "4.21" (and optionally "4" or other incomplete forms) to assert whatever SemverParseTolerant should produce (e.g., "4.21" -> true if parsed as 4.21); use the same t.Run/expect pattern and the ReleaseTagIsDualRHCOS helper so the new cases integrate with existing table-driven tests.pkg/release-controller/machine_os_tags.go (1)
124-147: Consider adding explicit priority forrhel-coreos-10.The sort priority map includes
rhel-coreos(0) andstream-coreos(1), butrhel-coreos-10will fall through to lexicographic sorting. This works correctly today ("rhel-coreos" < "rhel-coreos-10"lexicographically), but if a new variant likerhel-coreos-9is introduced, the ordering might become unexpected.💡 Explicit priority for all known tags
func machineOSTagLess(a, b string) bool { prio := map[string]int{ - "rhel-coreos": 0, - "stream-coreos": 1, + "rhel-coreos": 0, + "rhel-coreos-10": 1, + "stream-coreos": 2, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/machine_os_tags.go` around lines 124 - 147, The current machine OSTag ordering in machineOSTagLess relies on exact keys in the prio map so variants like "rhel-coreos-10" fall back to lexicographic sort; update machineOSTagLess (used by sortMachineOSTags) to either add explicit priority entries for known variants (e.g., "rhel-coreos-10": 0 and any other rhel-coreos-X variants) or implement a prefix-based check (treat strings with prefix "rhel-coreos" as the same priority as "rhel-coreos") so all rhel-coreos variants sort consistently ahead of "stream-coreos". Ensure the chosen change updates the prio lookup logic and retains the existing fallback to lexicographic comparison for unknown tags.cmd/release-controller-api/http_changelog.go (1)
183-195: Consider consolidating duplicate anchor+streams detection logic.The logic to determine whether to show node image info (check anchor, then check streams) is duplicated between
getChangeLog(lines 95-103) andrenderChangeLog(lines 183-192). While they serve slightly different purposes (one for async fetch initiation, one for render decision), the duplication could lead to drift.Note: The silent error handling (
if err == nil) on lines 186 and 188 is acceptable here since this is an optional UI enhancement—failing to show node info shouldn't break the page.💡 Potential consolidation
Consider extracting a helper function:
func (c *Controller) shouldShowNodeImageInfo(out string, toPull string) bool { if strings.Contains(out, "#node-image-info") { return true } toImage, err := releasecontroller.GetImageInfo(c.releaseInfo, c.architecture, toPull) if err != nil { return false } streams, err := c.releaseInfo.ListMachineOSStreams(toImage.GenerateDigestPullSpec()) return err == nil && len(streams) > 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller-api/http_changelog.go` around lines 183 - 195, The duplicate logic that decides whether to display node image info in getChangeLog and renderChangeLog should be extracted into a single helper on Controller (e.g., shouldShowNodeImageInfo) to avoid drift: move the combined check (strings.Contains(render/out, "#node-image-info") then fallback to calling releasecontroller.GetImageInfo and c.releaseInfo.ListMachineOSStreams on the generated digest pull spec) into that helper, update both getChangeLog and renderChangeLog to call Controller.shouldShowNodeImageInfo(out, toPull) (or appropriate parameter names), and preserve the current silent error handling (treat errors as false) so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/changelog-preview/main.go`:
- Around line 29-31: The help text for the flag variables defined in main uses
an incorrect format ("digest or tag@repo"); update the flag descriptions created
via flag.String for the variables from and to so they reflect the standard image
pull spec forms (e.g., "repository:tag or repository@digest (sha256:...)" or
similar), making sure the strings passed to flag.String for from and to are
corrected to the proper wording.
In `@pkg/release-controller/release_info.go`:
- Around line 128-165: Add the same NUL-byte validation used in the existing
handlers to the "rpmliststream", "imagefor", and "machineosstreams" branches:
detect embedded NULs in the relevant parts before using them (as done in the
"rpmdiffstream"/"bugs"/"rpmdiff"/"changelog" handlers) and return an error like
"invalid <key> key" when a NUL is found; update the checks around
info.RpmListForStream, info.ImageReferenceForComponent, and
info.ListMachineOSStreams to perform this validation on parts[1..n] prior to
calling those functions.
---
Nitpick comments:
In `@cmd/release-controller-api/http_changelog.go`:
- Around line 183-195: The duplicate logic that decides whether to display node
image info in getChangeLog and renderChangeLog should be extracted into a single
helper on Controller (e.g., shouldShowNodeImageInfo) to avoid drift: move the
combined check (strings.Contains(render/out, "#node-image-info") then fallback
to calling releasecontroller.GetImageInfo and c.releaseInfo.ListMachineOSStreams
on the generated digest pull spec) into that helper, update both getChangeLog
and renderChangeLog to call Controller.shouldShowNodeImageInfo(out, toPull) (or
appropriate parameter names), and preserve the current silent error handling
(treat errors as false) so behavior remains unchanged.
In `@hack/changelog-preview/main.go`:
- Around line 75-77: The hardcoded header "## Node Image Info" is duplicated;
update the code so the header is defined once and reused: either add a shared
constant (e.g., NodeImageInfoHeader) and replace the literal in
hack/changelog-preview/main.go (where nodeMD is appended to out) and in
cmd/release-controller-api/http_changelog.go, or move the header into the
NodeImageSectionMarkdown implementation so callers just append nodeMD without
adding the header; ensure all references (nodeMD, NodeImageSectionMarkdown) use
the single source of truth for the header.
In `@pkg/release-controller/machine_os_tags_test.go`:
- Around line 71-78: Add assertions to TestMachineOSTitle to cover the hardcoded
fallback mappings inside MachineOSTitle: call MachineOSTitle with
MachineOSStreamInfo having Tag values like "rhel-coreos-10" and "stream-coreos"
(and empty DisplayName) and assert the returned string matches the expected
human-readable fallbacks (e.g., the specific names MachineOSTitle maps those
tags to, including the backticked tag in parentheses). Locate the test function
TestMachineOSTitle and extend it with these additional checks using the same
pattern as the existing assertions to ensure the known-tag branches are
exercised.
In `@pkg/release-controller/machine_os_tags.go`:
- Around line 124-147: The current machine OSTag ordering in machineOSTagLess
relies on exact keys in the prio map so variants like "rhel-coreos-10" fall back
to lexicographic sort; update machineOSTagLess (used by sortMachineOSTags) to
either add explicit priority entries for known variants (e.g., "rhel-coreos-10":
0 and any other rhel-coreos-X variants) or implement a prefix-based check (treat
strings with prefix "rhel-coreos" as the same priority as "rhel-coreos") so all
rhel-coreos variants sort consistently ahead of "stream-coreos". Ensure the
chosen change updates the prio lookup logic and retains the existing fallback to
lexicographic comparison for unknown tags.
In `@pkg/release-controller/semver_dual_rhcos_test.go`:
- Around line 5-24: Add tests for missing edge cases to
TestReleaseTagIsDualRHCOS: include a non-4 major version like "5.21.0" expecting
false, an empty-string input "" expecting false, and a partial version "4.21"
(and optionally "4" or other incomplete forms) to assert whatever
SemverParseTolerant should produce (e.g., "4.21" -> true if parsed as 4.21); use
the same t.Run/expect pattern and the ReleaseTagIsDualRHCOS helper so the new
cases integrate with existing table-driven tests.
In `@pkg/rhcos/rhcos_test.go`:
- Around line 234-246: The test TestTransformMarkDownOutputDualRHCOSLines
currently asserts that strings.Count(out, "coreos-base-alert") < 2 to ensure at
least two infoboxes; change this to assert exactly two occurrences (e.g., if
strings.Count(out, "coreos-base-alert") != 2) and update the failure message
accordingly so the test fails when there are more or fewer than two
coreos-base-alert infoboxes, ensuring precise verification of dual RHCOS
rendering.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78d0ae95-391b-4882-9a65-80b2bef0c4b8
📒 Files selected for processing (11)
.gitignorecmd/release-controller-api/http_changelog.gohack/changelog-preview/main.gopkg/release-controller/machine_os_tags.gopkg/release-controller/machine_os_tags_test.gopkg/release-controller/release_info.gopkg/release-controller/semver.gopkg/release-controller/semver_dual_rhcos_test.gopkg/rhcos/node_image.gopkg/rhcos/rhcos.gopkg/rhcos/rhcos_test.go
Comment on lines
+29
to
+31
| func main() { | ||
| from := flag.String("from", "", "from release image pull spec (digest or tag@repo)") | ||
| to := flag.String("to", "", "to release image pull spec") |
Contributor
There was a problem hiding this comment.
Typo in help text.
Line 30 says "digest or tag@repo" but the standard format is repository:tag or repository@digest (sha256:...). The help text should reflect this.
📝 Suggested fix
- from := flag.String("from", "", "from release image pull spec (digest or tag@repo)")
+ from := flag.String("from", "", "from release image pull spec (repo:tag or repo@sha256:...)")📝 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.
Suggested change
| func main() { | |
| from := flag.String("from", "", "from release image pull spec (digest or tag@repo)") | |
| to := flag.String("to", "", "to release image pull spec") | |
| func main() { | |
| from := flag.String("from", "", "from release image pull spec (repo:tag or repo@sha256:...)") | |
| to := flag.String("to", "", "to release image pull spec") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/changelog-preview/main.go` around lines 29 - 31, The help text for the
flag variables defined in main uses an incorrect format ("digest or tag@repo");
update the flag descriptions created via flag.String for the variables from and
to so they reflect the standard image pull spec forms (e.g., "repository:tag or
repository@digest (sha256:...)" or similar), making sure the strings passed to
flag.String for from and to are corrected to the proper wording.
Collaborator
|
/label tide/merge-method-squash |
Collaborator
|
/approve |
openshift-ci
Bot
added
the
tide/merge-method-squash
Collaborator
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bradmwilliams, sdodson 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
Member
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
to check for null bytes in all inputs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Member
Author
|
Sorry, I hadn't noticed that the PR was still in draft state, should be good to go now. |
Contributor
|
@sdodson: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I have tested this across the following transitions and found results as expected
I've tested this with the sample changelog generator in hack/changelog-preview but not in a full RC deployment.
Sample output at https://gist.github.com/sdodson/91e0703c3b3a5b3b59cce2e83713f370
Co-authored-by: Cursor noreply@cursor.com
Assisted-by: Cursor AI (Composer)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes