CORS-4495: Populate RHCOS10 Marketplace Stream#10556
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request references CORS-4495 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 "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. |
WalkthroughThe PR extends the marketplace stream population script to support dual RHEL 9 and RHEL 10 generation with fallback logic, and updates marketplace metadata. The script now accepts configuration to parameterize input and output paths per stream, orchestrates sequential RHEL 9 and RHEL 10 population, and falls back to RHEL 9 data when Azure marketplace content is missing or population fails. ChangesMarketplace Dual-Stream Generation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[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)
hack/rhcos/populate-marketplace-imagestream.go (1)
152-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard release parsing to avoid panic on malformed stream names.
strings.Split(st.Stream, "-")[1]can panic ifst.Streamis empty or missing-. Return a structured error instead.Proposed fix
func getReleaseFromStream(cfg streamConfig) (string, error) { @@ - return strings.Split(st.Stream, "-")[1], nil + parts := strings.SplitN(st.Stream, "-", 2) + if len(parts) < 2 || strings.TrimSpace(parts[1]) == "" { + return "", fmt.Errorf("unexpected stream format %q in %s", st.Stream, cfg.inputFile) + } + return parts[1], nil }🤖 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 `@hack/rhcos/populate-marketplace-imagestream.go` around lines 152 - 167, The getReleaseFromStream function assumes st.Stream contains a hyphen and indexes strings.Split(st.Stream, "-")[1], which can panic for empty or malformed values; update the function to validate st.Stream (and the result of strings.Split) before indexing: if STREAM_RELEASE_OVERRIDE is not set, ensure st.Stream is non-empty, split into parts (or find the first '-') and check there is at least two parts (or a valid index) and return a clear error (e.g., fmt.Errorf("invalid stream format: %q", st.Stream)) instead of panicking; keep the existing STREAM_RELEASE_OVERRIDE behavior and use the same error wrapping pattern used elsewhere in the function.
🤖 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 `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 90-94: The code currently falls back to RHEL9 on any error from
s.azure; change the error handling in the block calling s.azure(ctx,
supportedArch, cfg) so it only falls back when the error represents the expected
“RHEL10 marketplace unavailable/missing” case (use a specific sentinel error,
errors.Is, or a string match like strings.Contains(err.Error(), "marketplace
missing" / the exact provider message) to detect that case); if that specific
condition is true, perform the existing log.Printf and s[supportedArch] =
fallback[supportedArch] and continue, otherwise return or propagate the original
err instead of falling back (so non-marketplace errors are surfaced).
---
Outside diff comments:
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 152-167: The getReleaseFromStream function assumes st.Stream
contains a hyphen and indexes strings.Split(st.Stream, "-")[1], which can panic
for empty or malformed values; update the function to validate st.Stream (and
the result of strings.Split) before indexing: if STREAM_RELEASE_OVERRIDE is not
set, ensure st.Stream is non-empty, split into parts (or find the first '-') and
check there is at least two parts (or a valid index) and return a clear error
(e.g., fmt.Errorf("invalid stream format: %q", st.Stream)) instead of panicking;
keep the existing STREAM_RELEASE_OVERRIDE behavior and use the same error
wrapping pattern used elsewhere in the function.
🪄 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: db61ce43-37ee-437f-928a-c8ddad4f7fa4
📒 Files selected for processing (3)
data/data/coreos/marketplace/coreos-rhel-10.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/rhcos/populate-marketplace-imagestream.go
Updates the script to populate azure marketplace images to include the rhel10 stream. If RHCOS10 images are not present, we can fall back to RHCOS9, as the first-boot image will be updated to RHCOS10.
patrickdillon
force-pushed
the
pop-rhcos10-az-mkt
branch
from
bef823d to
15331f6
Compare
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)
hack/rhcos/populate-marketplace-imagestream.go (1)
165-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard release parsing to avoid panic on malformed stream names.
Line 165 indexes into
strings.Split(...)[1]without validating shape. Ifst.Streamis malformed, this panics and aborts the script.Proposed fix
- return strings.Split(st.Stream, "-")[1], nil + parts := strings.SplitN(st.Stream, "-", 2) + if len(parts) != 2 || parts[1] == "" { + return "", fmt.Errorf("invalid stream format %q, expected <name>-<release>", st.Stream) + } + return parts[1], nil🤖 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 `@hack/rhcos/populate-marketplace-imagestream.go` at line 165, The code blindly returns strings.Split(st.Stream, "-")[1] which can panic if st.Stream doesn't contain a '-' or has fewer than two parts; update the parsing around strings.Split(st.Stream, "-") to check the resulting slice length (e.g., parts := strings.Split(st.Stream, "-"); if len(parts) < 2 { return "", fmt.Errorf("malformed stream: %q", st.Stream) }) and then return parts[1], nil, ensuring the function (the one using st.Stream) returns an error on malformed input instead of panicking.
🤖 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 `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 84-86: Update the doc comment for the function
populateWithFallback to reflect the current behavior: remove the claim that a
fallback occurs when population "fails" and state that the function falls back
only when marketplace data is empty; ensure the comment mentions that errors
(e.g., Azure errors) are still returned immediately rather than triggering the
fallback.
---
Outside diff comments:
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Line 165: The code blindly returns strings.Split(st.Stream, "-")[1] which can
panic if st.Stream doesn't contain a '-' or has fewer than two parts; update the
parsing around strings.Split(st.Stream, "-") to check the resulting slice length
(e.g., parts := strings.Split(st.Stream, "-"); if len(parts) < 2 { return "",
fmt.Errorf("malformed stream: %q", st.Stream) }) and then return parts[1], nil,
ensuring the function (the one using st.Stream) returns an error on malformed
input instead of panicking.
🪄 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: 1e556c67-c83f-4a9e-9bce-f04aeabec6ab
📒 Files selected for processing (3)
data/data/coreos/marketplace/coreos-rhel-10.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/rhcos/populate-marketplace-imagestream.go
🚧 Files skipped from review as they are similar to previous changes (2)
- data/data/coreos/marketplace/coreos-rhel-10.json
- data/data/coreos/marketplace/coreos-rhel-9.json
| // populateWithFallback attempts to populate marketplace data for each architecture. | ||
| // If population fails or returns empty data, it falls back to the provided fallback stream. | ||
| func (s marketplaceStream) populateWithFallback(ctx context.Context, cfg streamConfig, fallback marketplaceStream) error { |
There was a problem hiding this comment.
Fallback comment is out of date with current behavior.
Line 85 says fallback happens when population fails, but Lines 90-92 return the Azure error immediately. Please update the comment to “fallback on empty marketplace data” only.
🤖 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 `@hack/rhcos/populate-marketplace-imagestream.go` around lines 84 - 86, Update
the doc comment for the function populateWithFallback to reflect the current
behavior: remove the claim that a fallback occurs when population "fails" and
state that the function falls back only when marketplace data is empty; ensure
the comment mentions that errors (e.g., Azure errors) are still returned
immediately rather than triggering the fallback.
|
@patrickdillon: 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. |
2 participants
Updates the script to populate azure marketplace images to include the rhel10 stream. If RHCOS10 images are not present, we can fall back to RHCOS9, as the first-boot image will be updated to RHCOS10.
Summary by CodeRabbit
New Features
Updates
Chores