Skip to content

CORS-4495: Populate RHCOS10 Marketplace Stream#10556

Merged
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
patrickdillon:pop-rhcos10-az-mkt
May 26, 2026
Merged

CORS-4495: Populate RHCOS10 Marketplace Stream#10556
openshift-merge-bot[bot] merged 2 commits into
openshift:mainfrom
patrickdillon:pop-rhcos10-az-mkt

Conversation

@patrickdillon
Copy link
Copy Markdown
Contributor

@patrickdillon patrickdillon commented May 14, 2026

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

    • Added CoreOS RHEL 10 marketplace configurations covering multiple architectures, regions, and Azure image variants.
  • Updates

    • Updated CoreOS RHEL 9 marketplace metadata: SKUs and version strings adjusted across offers and regions.
  • Bug Fixes

    • Prevented returning empty/invalid Azure image matches and refined version comparison to improve selection.
  • Chores

    • Marketplace generation now emits RHEL 9 and RHEL 10 streams with configurable inputs/outputs and fallback merging.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 14, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented May 14, 2026

@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.

Details

In response to this:

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.

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.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a new RHEL 10 marketplace JSON, updates RHEL 9 marketplace entries, introduces azure.FillMissing to merge missing Azure fields, and refactors the marketplace population script to generate both RHEL 9 and RHEL 10 outputs with fallback and configurable input/output per stream.

Changes

Marketplace Dual-Stream Generation

Layer / File(s) Summary
Azure FillMissing helper
pkg/rhcos/marketplace/azure/azure.go
Adds exported FillMissing(target, fallback *rhcos.AzureMarketplace) that copies non-nil sub-objects from fallback into target when target fields are nil; also updates getImage, parseAROSKUs, and checkIfNewer logic.
Populate script refactor and orchestration
hack/rhcos/populate-marketplace-imagestream.go
Introduces streamConfig to parameterize input/output and name. main writes RHEL 9 then RHEL 10 using populateWithFallback. populate accepts streamConfig, getReleaseFromStream supports env override or input file parsing, and write writes to cfg.outputFile.
RHEL 9 marketplace data updates
data/data/coreos/marketplace/coreos-rhel-9.json
Updated aarch64 no-purchase-plan sku/version, updated x86_64 no-purchase-plan hyperVGen1/hyperVGen2 SKUs and versions, and advanced ocp/opp/oke (and *-emea) version strings.
RHEL 10 marketplace metadata
data/data/coreos/marketplace/coreos-rhel-10.json
New static lookup mapping architectures and Azure channels (ocp/opp/oke, EMEA variants, hyperVGen1/2) to publisher/offer/sku/version identifiers.

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 11 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: populating RHCOS10 Marketplace Stream, which aligns with the primary objective of adding RHEL 10 marketplace stream generation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR does not modify or add any Ginkgo tests. Custom check for stable Ginkgo test names is not applicable; this repository uses standard Go testing, not Ginkgo.
Test Structure And Quality ✅ Passed PR contains no Ginkgo test code: only JSON data files and Go source modules (populate-marketplace-imagestream.go, azure.go). Check is not applicable.
Microshift Test Compatibility ✅ Passed No Ginkgo e2e tests are added in this PR. The changes only affect JSON data files, a utility script, and a Go package. The check does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No Ginkgo e2e tests are added. Changes are JSON data files and Go utility/library code for Azure marketplace metadata.
Topology-Aware Scheduling Compatibility ✅ Passed PR modifies only Marketplace metadata (JSON) and utility scripts, not deployment manifests or operator code. No pod scheduling constraints or topology assumptions introduced.
Ote Binary Stdout Contract ✅ Passed PR contains no OTE binaries. Changes are to utility script in hack/ and library code; both use standard logging (stderr) without stdout corruption.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No Ginkgo e2e tests were added. PR contains only JSON marketplace data and utility Go code, outside the scope of this check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Guard release parsing to avoid panic on malformed stream names.

strings.Split(st.Stream, "-")[1] can panic if st.Stream is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc515d and bef823d.

📒 Files selected for processing (3)
  • data/data/coreos/marketplace/coreos-rhel-10.json
  • data/data/coreos/marketplace/coreos-rhel-9.json
  • hack/rhcos/populate-marketplace-imagestream.go

Comment thread hack/rhcos/populate-marketplace-imagestream.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Guard release parsing to avoid panic on malformed stream names.

Line 165 indexes into strings.Split(...)[1] without validating shape. If st.Stream is 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

📥 Commits

Reviewing files that changed from the base of the PR and between bef823d and 15331f6.

📒 Files selected for processing (3)
  • data/data/coreos/marketplace/coreos-rhel-10.json
  • data/data/coreos/marketplace/coreos-rhel-9.json
  • hack/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

Comment thread hack/rhcos/populate-marketplace-imagestream.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

154-154: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add bounds check before indexing Split result.

strings.Split(st.Stream, "-")[1] will panic if st.Stream does not contain a "-" character. While the RHCOS stream format is expected to be "rhel-X.Y", a malformed input file would crash the script.

🛡️ Proposed fix
-	return strings.Split(st.Stream, "-")[1], nil
+	parts := strings.Split(st.Stream, "-")
+	if len(parts) < 2 {
+		return "", fmt.Errorf("invalid stream format %q: expected 'rhel-X.Y'", 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 154, The current code
uses strings.Split(st.Stream, "-")[1] which will panic for malformed st.Stream
values; change it to split := strings.Split(st.Stream, "-") (or use
strings.Index/strings.Contains) and check len(split) >= 2 before accessing
split[1]; if the check fails, return a clear error (or handle the fallback)
instead of indexing into the slice. Ensure you update the caller/signature if
needed to propagate the error from this function and reference the existing
st.Stream and strings.Split usage when making the change.
🤖 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 `@data/data/coreos/marketplace/coreos-rhel-10.json`:
- Around line 16-23: The RHEL10 x86_64 "no-purchase-plan" only defines
"hyperVGen1" (SKU "aro_422") so Gen2 won't be backfilled by FillMissing; either
add a "hyperVGen2" entry mirroring "hyperVGen1" but with the Gen2 SKU expected
by parseAROSKUs (use SKU "aro_422-v2" or the generated "422-v2" pattern) and the
same publisher/offer/version, or add a short doc comment in the surrounding JSON
explaining why "hyperVGen2" is intentionally unavailable for the
"no-purchase-plan" / NoPurchasePlan case so reviewers know this omission is
deliberate.

---

Outside diff comments:
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Line 154: The current code uses strings.Split(st.Stream, "-")[1] which will
panic for malformed st.Stream values; change it to split :=
strings.Split(st.Stream, "-") (or use strings.Index/strings.Contains) and check
len(split) >= 2 before accessing split[1]; if the check fails, return a clear
error (or handle the fallback) instead of indexing into the slice. Ensure you
update the caller/signature if needed to propagate the error from this function
and reference the existing st.Stream and strings.Split usage when making the
change.
🪄 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: 4a49f1b8-0f90-4ebd-b16b-d80c9aad4fe3

📥 Commits

Reviewing files that changed from the base of the PR and between 15331f6 and ded1192.

📒 Files selected for processing (4)
  • data/data/coreos/marketplace/coreos-rhel-10.json
  • data/data/coreos/marketplace/coreos-rhel-9.json
  • hack/rhcos/populate-marketplace-imagestream.go
  • pkg/rhcos/marketplace/azure/azure.go

Comment thread data/data/coreos/marketplace/coreos-rhel-10.json
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

139-155: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate stream/override release format before returning it.

Line [154] indexes strings.Split(st.Stream, "-")[1] without checking shape, and Line [140] returns STREAM_RELEASE_OVERRIDE unvalidated. A malformed value can panic downstream instead of returning a clean error.

Suggested patch
 func getReleaseFromStream(cfg streamConfig) (string, error) {
 	if rel, ok := os.LookupEnv("STREAM_RELEASE_OVERRIDE"); ok {
+		if parts := strings.Split(rel, "."); len(parts) < 2 {
+			return "", fmt.Errorf("invalid STREAM_RELEASE_OVERRIDE %q, expected <major>.<minor>", rel)
+		}
 		log.Printf("Found STREAM_RELEASE_OVERRIDE %s", rel)
 		return rel, nil
 	}
 	fileContents, err := os.ReadFile(cfg.inputFile)
 	if err != nil {
 		return "", err
 	}
@@
 	if err := json.Unmarshal(fileContents, st); err != nil {
 		return "", fmt.Errorf("failed to unmarshal RHCOS stream: %w", err)
 	}
-
-	return strings.Split(st.Stream, "-")[1], nil
+	parts := strings.SplitN(st.Stream, "-", 2)
+	if len(parts) != 2 || parts[1] == "" {
+		return "", fmt.Errorf("unexpected stream format %q", st.Stream)
+	}
+	rel := parts[1]
+	if mm := strings.Split(rel, "."); len(mm) < 2 {
+		return "", fmt.Errorf("invalid release %q extracted from stream %q", rel, st.Stream)
+	}
+	return rel, 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 139 - 155, In
getReleaseFromStream validate the release string shape before returning: when
STREAM_RELEASE_OVERRIDE is set, check that it matches the expected dashed format
(e.g., contains a '-' and splits into at least two parts) and return a
descriptive error if not; after unmarshalling, verify that st.Stream is
non-empty and that strings.Split(st.Stream, "-") yields at least two elements
before indexing [1], returning an error if the shape is wrong; ensure error
messages mention the source (override vs stream) and the invalid value so
callers do not panic downstream.
🤖 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.

Outside diff comments:
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 139-155: In getReleaseFromStream validate the release string shape
before returning: when STREAM_RELEASE_OVERRIDE is set, check that it matches the
expected dashed format (e.g., contains a '-' and splits into at least two parts)
and return a descriptive error if not; after unmarshalling, verify that
st.Stream is non-empty and that strings.Split(st.Stream, "-") yields at least
two elements before indexing [1], returning an error if the shape is wrong;
ensure error messages mention the source (override vs stream) and the invalid
value so callers do not panic downstream.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 498ca4fe-914a-4308-96c3-25919b8f1f69

📥 Commits

Reviewing files that changed from the base of the PR and between ded1192 and 5fc071d.

📒 Files selected for processing (4)
  • data/data/coreos/marketplace/coreos-rhel-10.json
  • data/data/coreos/marketplace/coreos-rhel-9.json
  • hack/rhcos/populate-marketplace-imagestream.go
  • pkg/rhcos/marketplace/azure/azure.go

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/marketplace/azure/azure.go (1)

287-307: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Backfill missing Gen1/Gen2 entries inside partially populated families.

getImages can return a non-nil *rhcos.AzureMarketplaceImages with only one generation set. This helper only copies the fallback when the whole family is nil, so a partial RHEL10 result like {Gen1:nil, Gen2:...} will skip the RHEL9 fallback for Gen1 and emit incomplete marketplace data.

Suggested fix
 func FillMissing(target, fallback *rhcos.AzureMarketplace) {
 	if target == nil || fallback == nil {
 		return
 	}
-	if target.NoPurchasePlan == nil {
-		target.NoPurchasePlan = fallback.NoPurchasePlan
-	}
-	if target.OCP == nil {
-		target.OCP = fallback.OCP
-	}
-	if target.OPP == nil {
-		target.OPP = fallback.OPP
-	}
-	if target.OKE == nil {
-		target.OKE = fallback.OKE
-	}
-	if target.OCPEMEA == nil {
-		target.OCPEMEA = fallback.OCPEMEA
-	}
-	if target.OPPEMEA == nil {
-		target.OPPEMEA = fallback.OPPEMEA
-	}
-	if target.OKEEMEA == nil {
-		target.OKEEMEA = fallback.OKEEMEA
-	}
+	fillImages := func(dst **rhcos.AzureMarketplaceImages, src *rhcos.AzureMarketplaceImages) {
+		if src == nil {
+			return
+		}
+		if *dst == nil {
+			*dst = src
+			return
+		}
+		if (*dst).Gen1 == nil {
+			(*dst).Gen1 = src.Gen1
+		}
+		if (*dst).Gen2 == nil {
+			(*dst).Gen2 = src.Gen2
+		}
+	}
+
+	fillImages(&target.NoPurchasePlan, fallback.NoPurchasePlan)
+	fillImages(&target.OCP, fallback.OCP)
+	fillImages(&target.OPP, fallback.OPP)
+	fillImages(&target.OKE, fallback.OKE)
+	fillImages(&target.OCPEMEA, fallback.OCPEMEA)
+	fillImages(&target.OPPEMEA, fallback.OPPEMEA)
+	fillImages(&target.OKEEMEA, fallback.OKEEMEA)
 }
🤖 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 `@pkg/rhcos/marketplace/azure/azure.go` around lines 287 - 307, The current
backfill only replaces entire family pointers (target.NoPurchasePlan, OCP, OPP,
OKE, OCPEMEA, OPPEMEA, OKEEMEA) when nil, but if a family exists with missing
generation fields (Gen1/Gen2) those subfields remain empty; update the logic to
also copy missing generation entries: for each family (NoPurchasePlan, OCP, OPP,
OKE, OCPEMEA, OPPEMEA, OKEEMEA) if target.<Family> is nil set target.<Family> =
fallback.<Family> as before, otherwise if target.<Family> != nil and
fallback.<Family> != nil then if target.<Family>.Gen1 == nil set it to
fallback.<Family>.Gen1 and likewise for Gen2 so partially populated results from
getImages are fully backfilled.
🤖 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 `@pkg/rhcos/marketplace/azure/azure.go`:
- Around line 310-341: The helper checkIfNewer(candidate, release string)
currently treats any imgMajor==9 or 10 as non-comparable and returns false,
which allows cross-family selection; update checkIfNewer to accept an expected
marketplace major (e.g., add an extra parameter expectedMajor int or string) and
immediately reject candidates whose imgMajor does not match expectedMajor
(return false) before performing the greater-than comparison; keep the existing
parsing/validation of candidate and release (semver.MajorMinor, strconv.Atoi)
but when imgMajor is 9 or 10 ensure you compare it against the passed
expectedMajor and only allow further comparison when they match, otherwise
return false so RHEL9/10 families remain distinct (refer to function
checkIfNewer and callers like getImage to thread the new parameter).

---

Outside diff comments:
In `@pkg/rhcos/marketplace/azure/azure.go`:
- Around line 287-307: The current backfill only replaces entire family pointers
(target.NoPurchasePlan, OCP, OPP, OKE, OCPEMEA, OPPEMEA, OKEEMEA) when nil, but
if a family exists with missing generation fields (Gen1/Gen2) those subfields
remain empty; update the logic to also copy missing generation entries: for each
family (NoPurchasePlan, OCP, OPP, OKE, OCPEMEA, OPPEMEA, OKEEMEA) if
target.<Family> is nil set target.<Family> = fallback.<Family> as before,
otherwise if target.<Family> != nil and fallback.<Family> != nil then if
target.<Family>.Gen1 == nil set it to fallback.<Family>.Gen1 and likewise for
Gen2 so partially populated results from getImages are fully backfilled.
🪄 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: adde3022-6bcb-4703-8629-d74a1b15360d

📥 Commits

Reviewing files that changed from the base of the PR and between 5fc071d and af1359b.

📒 Files selected for processing (2)
  • hack/rhcos/populate-marketplace-imagestream.go
  • pkg/rhcos/marketplace/azure/azure.go

Comment thread pkg/rhcos/marketplace/azure/azure.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.
In 4.22, ARO changed the SKU format so that it includes -v2 or -arm
prefixes. This commit updates the populate script to take these changes
into account and find these images.

Also, a slight tweak to adjust for the version discrepency. Newer
ARO versions use RHEL versions, while older versions use OCP versions.
@tthvo
Copy link
Copy Markdown
Member

tthvo commented May 20, 2026

/test azure-ovn-marketplace-images

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 20, 2026

@patrickdillon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azurestack 5c761c3 link false /test e2e-azurestack

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Copy Markdown
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a few questions, but looks good to me :D

Comment on lines +322 to 326
imgMinor, err := strconv.Atoi(strings.Split(mm, ".")[1])
if err != nil {
logrus.Infof("Error converting minor version to int with version %s", candidate)
return true
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From line 322 - 341, we may be able to simplify it to:

rel := release
if !strings.HasPrefix(rel, "v") {
  rel = "v" + rel
}
return semver.Compare(mm, semver.MajorMinor(rel)) > 0

because mm and release/rel are both in form v<major>.<minor>.0 (e.g. 4.22.0), right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, somehow I feel like we talked about this before... Does this make sense at all though?

@@ -0,0 +1,116 @@
{
"aarch64": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because there is no rhel-10 image yet, this file is exactly the same as coreos-rhel-9.json and there will be a follow-up PR to update this 🤔?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if there is no rhel-10 image we can use rhel-9 and the os will be automagically updated to rhel10

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this was the direction given by the dual stream team--not a workaround I came up with

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh okayy, make sense to me! Thanks!

// to OCP release versions and should never be filtered out.
if imgMajor == 9 || imgMajor == 10 {
return false
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there won't be ever be overlapping between ocp and RHEL major version, right? Then, this check would be safe :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, yeah I guess so, I didn't think of that.

Copy link
Copy Markdown
Member

@tthvo tthvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

Thanks for checking my questions! The one suggestion is a nit anyway XD

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 21, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tthvo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2026
@patrickdillon
Copy link
Copy Markdown
Contributor Author

/verified by multi-pr test #10533 (comment)

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label May 26, 2026
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

@patrickdillon: This PR has been marked as verified by multi-pr test https://github.com/openshift/installer/pull/10533#issuecomment-4501635620.

Details

In response to this:

/verified by multi-pr test #10533 (comment)

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-merge-bot openshift-merge-bot Bot merged commit 9f29a75 into openshift:main May 26, 2026
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants