Revert "Extend Node Image Info to handle streams"#752
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes machine-OS stream discovery, dual RHCOS (RHEL 10) support, and stream-scoped RPM operations from the release controller. It eliminates a standalone changelog-preview CLI, simplifies node image info rendering, and refactors changelog generation logic to gate node-info retrieval on anchor presence rather than stream availability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
bradmwilliams
approved these changes
Apr 8, 2026
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
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/release-controller-api/http_changelog.go (1)
68-82:⚠️ Potential issue | 🔴 CriticalReturn immediately after emitting JSON.
renderChangeLogonly reads one value fromch. After the JSON send at Line 74, this function keeps treating the JSON payload as Markdown and then sends tochagain at Line 82, so every JSON request leaves the goroutine blocked on the second send.Suggested fix
if isJson { out, err = rhcos.TransformJsonOutput(out, architecture, archExtension) if err != nil { ch <- renderResult{err: err} return } ch <- renderResult{out: out} + return }🤖 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 68 - 82, The JSON branch currently falls through and continues to call rhcos.TransformMarkDownOutput then sends to ch again, causing a blocked goroutine; inside the isJson block in renderChangeLog, after calling rhcos.TransformJsonOutput and sending ch <- renderResult{out: out} you must return immediately to stop further processing. Concretely, in the isJson branch (around the call to rhcos.TransformJsonOutput and the ch <- renderResult{out: out} line) add a return so the function doesn't call rhcos.TransformMarkDownOutput or send a second renderResult; keep existing error handling for TransformJsonOutput and use the same renderResult struct and ch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/release-controller-api/http_changelog.go`:
- Around line 89-107: The producer goroutine currently may send to chNodeInfo
multiple times (once on the no-anchor path and again on error/normal paths),
causing a possible permanent block; update the logic in the block that checks
strings.Contains(out, "#node-image-info") and the subsequent calls to
c.releaseInfo.RpmList and c.releaseInfo.RpmDiff so that chNodeInfo is written
exactly once: on the no-anchor path send a single empty renderResult and return
immediately, and on the error paths send a single renderResult with the error
and return immediately (do not continue to call rhcos.RenderNodeImageInfo);
finally, if no errors occur send the rendered result from
rhcos.RenderNodeImageInfo once. Ensure you reference chNodeInfo, renderResult,
c.releaseInfo.RpmList, c.releaseInfo.RpmDiff, fromImage.GenerateDigestPullSpec,
toImage.GenerateDigestPullSpec, and rhcos.RenderNodeImageInfo when locating the
code to change.
In `@pkg/rhcos/rhcos.go`:
- Around line 368-372: The code prints m[3] from the reMdCoSPost match which can
include the opening <div id="coreos-base-alert"> (appended earlier as
baseLayerAlertBox), causing a duplicate/dangling HTML fragment; update the block
around reMdCoSPost and the fmt.Fprintf call to strip or exclude the
baseLayerAlertBox HTML before printing (e.g., detect and remove the "<div
id=\"coreos-base-alert\">" suffix or adjust the regex capture so m[3] contains
only the release link), ensuring the printed "base layer" footer contains just
the release link and not the alert-box HTML.
- Around line 314-324: Check for empty rpm metadata before printing headline
packages: if rpmList.Packages is nil or has length 0, skip the headline block
and the details section early. In the loop over importantPkgs (the slice
containing "cri-o","kernel","openshift-kubelet","systemd"), only write entries
when rpmList.Packages contains the key (i.e. check presence before using
rpmList.Packages[pkg]) so you don't emit trailing hyphens for missing versions.
Apply the same guard logic before calling
slices.Sorted(maps.Keys(rpmList.Packages)) and generating the full list to avoid
iterating an empty map.
---
Outside diff comments:
In `@cmd/release-controller-api/http_changelog.go`:
- Around line 68-82: The JSON branch currently falls through and continues to
call rhcos.TransformMarkDownOutput then sends to ch again, causing a blocked
goroutine; inside the isJson block in renderChangeLog, after calling
rhcos.TransformJsonOutput and sending ch <- renderResult{out: out} you must
return immediately to stop further processing. Concretely, in the isJson branch
(around the call to rhcos.TransformJsonOutput and the ch <- renderResult{out:
out} line) add a return so the function doesn't call
rhcos.TransformMarkDownOutput or send a second renderResult; keep existing error
handling for TransformJsonOutput and use the same renderResult struct and ch.
🪄 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: a03bbf08-a798-4965-aecd-be4eff89a3b0
📒 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
💤 Files with no reviewable changes (9)
- .gitignore
- pkg/rhcos/rhcos_test.go
- pkg/release-controller/machine_os_tags_test.go
- pkg/release-controller/semver_dual_rhcos_test.go
- pkg/release-controller/semver.go
- pkg/release-controller/release_info.go
- hack/changelog-preview/main.go
- pkg/rhcos/node_image.go
- pkg/release-controller/machine_os_tags.go
Comment on lines
+89
to
+107
| // Only request node image info if it'll be rendered. Use the exact | ||
| // check that renderChangeLog does to know if to consume from us. | ||
| if !strings.Contains(out, "#node-image-info") { | ||
| chNodeInfo <- renderResult{} | ||
| return | ||
| } | ||
|
|
||
| nodeMD, err := rhcos.NodeImageSectionMarkdown(c.releaseInfo, fromImagePullspec, toImagePullspec, out) | ||
| toImagePullspec := toImage.GenerateDigestPullSpec() | ||
| rpmlist, err := c.releaseInfo.RpmList(toImagePullspec) | ||
| if err != nil { | ||
| chNodeInfo <- renderResult{err: err} | ||
| return | ||
| } | ||
| chNodeInfo <- renderResult{out: nodeMD} | ||
|
|
||
| rpmdiff, err := c.releaseInfo.RpmDiff(fromImage.GenerateDigestPullSpec(), toImagePullspec) | ||
| if err != nil { | ||
| chNodeInfo <- renderResult{err: err} | ||
| } | ||
|
|
||
| chNodeInfo <- renderResult{out: rhcos.RenderNodeImageInfo(out, rpmlist, rpmdiff)} |
Contributor
There was a problem hiding this comment.
Send to chNodeInfo exactly once, or not at all.
renderChangeLog skips reading chNodeInfo when the markdown has no #node-image-info, and otherwise it consumes a single value. This block sends on the no-anchor path at Line 92 and can also send an error plus a final rendered result on the failure paths, so the producer goroutine blocks permanently in both cases.
Suggested fix
// Only request node image info if it'll be rendered. Use the exact
// check that renderChangeLog does to know if to consume from us.
if !strings.Contains(out, "#node-image-info") {
- chNodeInfo <- renderResult{}
return
}
toImagePullspec := toImage.GenerateDigestPullSpec()
rpmlist, err := c.releaseInfo.RpmList(toImagePullspec)
if err != nil {
chNodeInfo <- renderResult{err: err}
+ return
}
rpmdiff, err := c.releaseInfo.RpmDiff(fromImage.GenerateDigestPullSpec(), toImagePullspec)
if err != nil {
chNodeInfo <- renderResult{err: err}
+ return
}
chNodeInfo <- renderResult{out: rhcos.RenderNodeImageInfo(out, rpmlist, rpmdiff)}📝 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
| // Only request node image info if it'll be rendered. Use the exact | |
| // check that renderChangeLog does to know if to consume from us. | |
| if !strings.Contains(out, "#node-image-info") { | |
| chNodeInfo <- renderResult{} | |
| return | |
| } | |
| nodeMD, err := rhcos.NodeImageSectionMarkdown(c.releaseInfo, fromImagePullspec, toImagePullspec, out) | |
| toImagePullspec := toImage.GenerateDigestPullSpec() | |
| rpmlist, err := c.releaseInfo.RpmList(toImagePullspec) | |
| if err != nil { | |
| chNodeInfo <- renderResult{err: err} | |
| return | |
| } | |
| chNodeInfo <- renderResult{out: nodeMD} | |
| rpmdiff, err := c.releaseInfo.RpmDiff(fromImage.GenerateDigestPullSpec(), toImagePullspec) | |
| if err != nil { | |
| chNodeInfo <- renderResult{err: err} | |
| } | |
| chNodeInfo <- renderResult{out: rhcos.RenderNodeImageInfo(out, rpmlist, rpmdiff)} | |
| // Only request node image info if it'll be rendered. Use the exact | |
| // check that renderChangeLog does to know if to consume from us. | |
| if !strings.Contains(out, "#node-image-info") { | |
| return | |
| } | |
| toImagePullspec := toImage.GenerateDigestPullSpec() | |
| rpmlist, err := c.releaseInfo.RpmList(toImagePullspec) | |
| if err != nil { | |
| chNodeInfo <- renderResult{err: err} | |
| return | |
| } | |
| rpmdiff, err := c.releaseInfo.RpmDiff(fromImage.GenerateDigestPullSpec(), toImagePullspec) | |
| if err != nil { | |
| chNodeInfo <- renderResult{err: err} | |
| return | |
| } | |
| chNodeInfo <- renderResult{out: rhcos.RenderNodeImageInfo(out, rpmlist, rpmdiff)} |
🤖 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 89 - 107, The
producer goroutine currently may send to chNodeInfo multiple times (once on the
no-anchor path and again on error/normal paths), causing a possible permanent
block; update the logic in the block that checks strings.Contains(out,
"#node-image-info") and the subsequent calls to c.releaseInfo.RpmList and
c.releaseInfo.RpmDiff so that chNodeInfo is written exactly once: on the
no-anchor path send a single empty renderResult and return immediately, and on
the error paths send a single renderResult with the error and return immediately
(do not continue to call rhcos.RenderNodeImageInfo); finally, if no errors occur
send the rendered result from rhcos.RenderNodeImageInfo once. Ensure you
reference chNodeInfo, renderResult, c.releaseInfo.RpmList,
c.releaseInfo.RpmDiff, fromImage.GenerateDigestPullSpec,
toImage.GenerateDigestPullSpec, and rhcos.RenderNodeImageInfo when locating the
code to change.
Comment on lines
+314
to
+324
| fmt.Fprintf(output, "### Package List\n\n") | ||
|
|
||
| importantPkgs := []string{"cri-o", "kernel", "openshift-kubelet", "systemd"} | ||
| for _, pkg := range importantPkgs { | ||
| fmt.Fprintf(out, "* %s-%s\n", pkg, stream.RpmList.Packages[pkg]) | ||
| fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg]) | ||
| } | ||
|
|
||
| fmt.Fprintf(out, "\n<details><summary>Full list (%d packages)</summary>\n\n", len(stream.RpmList.Packages)) | ||
| sortedPkgs := slices.Sorted(maps.Keys(stream.RpmList.Packages)) | ||
| fmt.Fprintf(output, "\n<details><summary>Full list (%d packages)</summary>\n\n", len(rpmList.Packages)) | ||
| sortedPkgs := slices.Sorted(maps.Keys(rpmList.Packages)) | ||
| for _, pkg := range sortedPkgs { | ||
| fmt.Fprintf(out, "* %s-%s\n", pkg, stream.RpmList.Packages[pkg]) | ||
| fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg]) |
Contributor
There was a problem hiding this comment.
Guard against empty RPM metadata before printing headline packages.
pkg/release-controller/release_info.go:165-175 can return RpmList{}, nil when the cache entry is empty. With the current direct lookups, that renders cri-o-, kernel-, etc. as if they were real versions. Please short-circuit when the package data is empty and skip any missing keys.
Suggested fix
func RenderNodeImageInfo(markdown string, rpmList releasecontroller.RpmList, rpmDiff releasecontroller.RpmDiff) string {
output := new(strings.Builder)
+ if len(rpmList.Packages) == 0 && len(rpmList.Extensions) == 0 {
+ return "_Node image package metadata is unavailable for this release._\n"
+ }
+
fmt.Fprintf(output, "### Package List\n\n")
importantPkgs := []string{"cri-o", "kernel", "openshift-kubelet", "systemd"}
for _, pkg := range importantPkgs {
- fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg])
+ if version, ok := rpmList.Packages[pkg]; ok {
+ fmt.Fprintf(output, "* %s-%s\n", pkg, version)
+ }
}📝 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
| fmt.Fprintf(output, "### Package List\n\n") | |
| importantPkgs := []string{"cri-o", "kernel", "openshift-kubelet", "systemd"} | |
| for _, pkg := range importantPkgs { | |
| fmt.Fprintf(out, "* %s-%s\n", pkg, stream.RpmList.Packages[pkg]) | |
| fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg]) | |
| } | |
| fmt.Fprintf(out, "\n<details><summary>Full list (%d packages)</summary>\n\n", len(stream.RpmList.Packages)) | |
| sortedPkgs := slices.Sorted(maps.Keys(stream.RpmList.Packages)) | |
| fmt.Fprintf(output, "\n<details><summary>Full list (%d packages)</summary>\n\n", len(rpmList.Packages)) | |
| sortedPkgs := slices.Sorted(maps.Keys(rpmList.Packages)) | |
| for _, pkg := range sortedPkgs { | |
| fmt.Fprintf(out, "* %s-%s\n", pkg, stream.RpmList.Packages[pkg]) | |
| fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg]) | |
| fmt.Fprintf(output, "### Package List\n\n") | |
| importantPkgs := []string{"cri-o", "kernel", "openshift-kubelet", "systemd"} | |
| for _, pkg := range importantPkgs { | |
| if version, ok := rpmList.Packages[pkg]; ok { | |
| fmt.Fprintf(output, "* %s-%s\n", pkg, version) | |
| } | |
| } | |
| fmt.Fprintf(output, "\n<details><summary>Full list (%d packages)</summary>\n\n", len(rpmList.Packages)) | |
| sortedPkgs := slices.Sorted(maps.Keys(rpmList.Packages)) | |
| for _, pkg := range sortedPkgs { | |
| fmt.Fprintf(output, "* %s-%s\n", pkg, rpmList.Packages[pkg]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/rhcos/rhcos.go` around lines 314 - 324, Check for empty rpm metadata
before printing headline packages: if rpmList.Packages is nil or has length 0,
skip the headline block and the details section early. In the loop over
importantPkgs (the slice containing
"cri-o","kernel","openshift-kubelet","systemd"), only write entries when
rpmList.Packages contains the key (i.e. check presence before using
rpmList.Packages[pkg]) so you don't emit trailing hyphens for missing versions.
Apply the same guard logic before calling
slices.Sorted(maps.Keys(rpmList.Packages)) and generating the full list to avoid
iterating an empty map.
Comment on lines
+368
to
372
| // Reprint the version/diff line, with the browser links for the build itself. | ||
| // But put it last to de-emphasize it. Most people don't need to click on this. | ||
| if m := reMdCoSPost.FindStringSubmatch(markdown); m != nil { | ||
| fmt.Fprintf(output, "<br/>%s **base layer**: %s\n\n", m[1], m[3]) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't re-emit the alert-box HTML as the base-layer footer.
On the node-image-info path, the matched CoreOS line already has baseLayerAlertBox appended. reMdCoSPost stops at the first newline, so m[3] includes the opening <div id="coreos-base-alert">. Printing it here emits a dangling duplicate HTML fragment instead of just the release link.
Suggested fix
// Reprint the version/diff line, with the browser links for the build itself.
// But put it last to de-emphasize it. Most people don't need to click on this.
if m := reMdCoSPost.FindStringSubmatch(markdown); m != nil {
- fmt.Fprintf(output, "<br/>%s **base layer**: %s\n\n", m[1], m[3])
+ baseLayer := strings.TrimSpace(strings.SplitN(m[3], "<div", 2)[0])
+ fmt.Fprintf(output, "<br/>%s **base layer**: %s\n\n", m[1], baseLayer)
}📝 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
| // Reprint the version/diff line, with the browser links for the build itself. | |
| // But put it last to de-emphasize it. Most people don't need to click on this. | |
| if m := reMdCoSPost.FindStringSubmatch(markdown); m != nil { | |
| fmt.Fprintf(output, "<br/>%s **base layer**: %s\n\n", m[1], m[3]) | |
| } | |
| // Reprint the version/diff line, with the browser links for the build itself. | |
| // But put it last to de-emphasize it. Most people don't need to click on this. | |
| if m := reMdCoSPost.FindStringSubmatch(markdown); m != nil { | |
| baseLayer := strings.TrimSpace(strings.SplitN(m[3], "<div", 2)[0]) | |
| fmt.Fprintf(output, "<br/>%s **base layer**: %s\n\n", m[1], baseLayer) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/rhcos/rhcos.go` around lines 368 - 372, The code prints m[3] from the
reMdCoSPost match which can include the opening <div id="coreos-base-alert">
(appended earlier as baseLayerAlertBox), causing a duplicate/dangling HTML
fragment; update the block around reMdCoSPost and the fmt.Fprintf call to strip
or exclude the baseLayerAlertBox HTML before printing (e.g., detect and remove
the "<div id=\"coreos-base-alert\">" suffix or adjust the regex capture so m[3]
contains only the release link), ensuring the printed "base layer" footer
contains just the release link and not the alert-box HTML.
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
2 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.
Reverts #749
Summary by CodeRabbit
Refactor
Chores