CNTRLPLANE-3021: Multi-stream CoreOS boot image parsing in releaseinfo#8711
CNTRLPLANE-3021: Multi-stream CoreOS boot image parsing in releaseinfo#8711sdminonne wants to merge 4 commits into
Conversation
Replace 18 custom CoreOS stream metadata Go types with canonical equivalents from github.com/coreos/stream-metadata-go v0.4.11. This eliminates duplicated type definitions and gains upstream helper methods (e.g. GetAMI(), URN()), laying groundwork for the dual-stream RHEL NodePool feature. Key type mappings: - CoreOSStreamMetadata -> stream.Stream - CoreOSArchitecture -> stream.Arch - CoreRHCOSImage -> *rhcos.Extensions (pointer) - CoreOSAWSImages -> *stream.AwsImage (pointer) - CoreOSGCPImage -> *stream.GcpImage (pointer) - CoreOSPowerVSImage -> *stream.SingleObject - CoreOSKubevirtImages -> *stream.ContainerImage (pointer) - HyperVGen1/HyperVGen2 -> Gen1/Gen2 - SHA256 -> Sha256, URL -> Url (PowerVS) Pure refactor with no behavioral changes. All platform controllers updated with nil checks for pointer-typed upstream fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eam types Split the triple nil-check in getWindowsAMI into separate checks for RHELCoreOSExtensions and AwsWinLi/Regions to produce distinct error messages that make debugging easier. Add test cases exercising nil pointer paths introduced by adopting pointer-typed upstream stream-metadata-go fields: - AWS: RHELCoreOSExtensions == nil, AwsWinLi == nil - AWS nodepool controller: Aws images == nil - KubeVirt: KubeVirt == nil - PowerVS: PowerVS == nil (new test file) - Azure: RHELCoreOSExtensions == nil Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend the releaseinfo package to parse ConfigMaps containing the new "streams" key (a JSON map keyed by stream name like "rhel-9", "rhel-10") alongside the legacy "stream" key. This enables HyperShift to support dual-stream RHEL boot images from the OpenShift installer. Add DeserializeMultiStreamImageMetadata with StreamsResult, extend ReleaseImage with Streams/DefaultStream fields and a StreamForPool resolver, and update both provider implementations to populate the new fields. All existing callers using StreamMetadata continue working unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sdminonne: This pull request references CNTRLPLANE-3021 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR migrates HyperShift's release image metadata handling from an internal CoreOS schema to the external Sequence Diagram(s)sequenceDiagram
participant RegistryClient
participant Deserialize as DeserializeMultiStreamImageMetadata
participant ReleaseImage
participant NodePoolController
participant CloudProviders as {AWS,Azure,GCP,PowerVS,OpenStack,KubeVirt}
RegistryClient->>Deserialize: fetch metadata ConfigMap
Deserialize->>ReleaseImage: return Default stream + Streams map
ReleaseImage->>NodePoolController: StreamMetadata or named stream via StreamForPool
NodePoolController->>CloudProviders: lookup/construct provider images (AMI, Marketplace, GCP images, PowerVS object, OpenStack artifact, KubeVirt)
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
openshift-ci
Bot
added
area/control-plane-operator
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
hypershift-operator/controllers/nodepool/powervs_test.go (1)
13-54: ⚡ Quick winAdd one success-path case for
getPowerVSImage.Line 20 onward only validates failures; please add a valid metadata/region case that asserts returned image fields, so the migrated happy path is guarded too. As per coding guidelines,
**/*_test.go: Unit test any code changes and additions, and include e2e tests when changes impact consumer behavior.🤖 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 `@hypershift-operator/controllers/nodepool/powervs_test.go` around lines 13 - 54, Add a happy-path subtest to TestGetPowerVSImage that constructs a releaseinfo.ReleaseImage whose StreamMetadata contains an entry for the "ppc64le" architecture with Images.PowerVS including a "us-south" entry (populate the image struct fields you expect), call getPowerVSImage("us-south", releaseImage) and assert no error and that the returned image values (ID/Name/other returned fields) match the expected values using the existing NewWithT/Expect helpers; place this as an additional element in testCases and mirror the existing test loop so the success case verifies returned image fields rather than expecting an error.Source: Coding guidelines
🤖 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 `@support/releaseinfo/deserialize.go`:
- Around line 74-99: The code currently allows an empty or nil-valued entry in
the streams map to slip through and later cause Default=nil or a panic when
accessing s.Stream; after unmarshalling, validate the streams map in the same
function that builds the StreamsResult: reject an entirely empty map (streams ==
nil or len(streams)==0) or return an error if any map value is nil, and when
calling matchDefaultStreamName ensure it checks nil entries before dereferencing
(update matchDefaultStreamName to skip nil values). Also ensure the branch that
picks firstAlphabeticalKey uses only keys whose values are non-nil. Reference:
StreamsResult, defaultStream, streams, matchDefaultStreamName,
firstAlphabeticalKey.
In `@support/releaseinfo/releaseinfo_test.go`:
- Around line 206-219: The test currently uses value equality
(g.Expect(result).To(Equal(...))) which can pass for different pointer instances
with identical fields; update the assertions to check pointer identity instead
by replacing Equal(...) with BeIdenticalTo(...) for the StreamForPool results
(e.g., change g.Expect(result).To(Equal(defaultStream)) to
g.Expect(result).To(BeIdenticalTo(defaultStream)) and similarly replace the
other Equal(rhel10Stream)/Equal(rhel9Stream) assertions with
BeIdenticalTo(rhel10Stream)/BeIdenticalTo(rhel9Stream) so the test verifies the
exact pointer returned by StreamForPool on the ReleaseImage fixtures
(ReleaseImage, defaultStream, rhel9Stream, rhel10Stream).
---
Nitpick comments:
In `@hypershift-operator/controllers/nodepool/powervs_test.go`:
- Around line 13-54: Add a happy-path subtest to TestGetPowerVSImage that
constructs a releaseinfo.ReleaseImage whose StreamMetadata contains an entry for
the "ppc64le" architecture with Images.PowerVS including a "us-south" entry
(populate the image struct fields you expect), call getPowerVSImage("us-south",
releaseImage) and assert no error and that the returned image values
(ID/Name/other returned fields) match the expected values using the existing
NewWithT/Expect helpers; place this as an additional element in testCases and
mirror the existing test loop so the success case verifies returned image fields
rather than expecting an error.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2087aabc-7853-499c-b1de-55617f3d2f8b
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/coreos/stream-metadata-go/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/artifact_utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/rhcos/rhcos.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/coreos/stream-metadata-go/stream/stream_utils.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (26)
go.modhypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/azure.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/gcp_test.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt.gohypershift-operator/controllers/nodepool/kubevirt/kubevirt_test.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/openstack/openstack.gohypershift-operator/controllers/nodepool/openstack/openstack_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/powervs_test.gohypershift-operator/controllers/nodepool/token_test.gosupport/releaseinfo/deserialize.gosupport/releaseinfo/deserialize_test.gosupport/releaseinfo/fake/fake.gosupport/releaseinfo/fixtures/5.0-installer-coreos-bootimages.yamlsupport/releaseinfo/fixtures/fixtures.gosupport/releaseinfo/registry_image_content_policies_test.gosupport/releaseinfo/registry_mirror_provider.gosupport/releaseinfo/registryclient_provider.gosupport/releaseinfo/releaseinfo.gosupport/releaseinfo/releaseinfo_test.gosupport/releaseinfo/testutils/testutils.go
…m parsing Validate the streams map after unmarshal to reject empty maps and nil entries that would cause Default=nil or a panic in matchDefaultStreamName. Add a happy-path test for getPowerVSImage covering the success case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8711 +/- ##
==========================================
+ Coverage 41.50% 41.58% +0.08%
==========================================
Files 758 758
Lines 93689 93931 +242
==========================================
+ Hits 38882 39064 +182
- Misses 52070 52116 +46
- Partials 2737 2751 +14
... and 8 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe project's
The current import block in import (
"testing" // group 1: standard ✓
. "github.com/onsi/gomega" // group 2: dot ✓
"github.com/coreos/stream-metadata-go/stream" // group 9: default ✗ WRONG POSITION
imageapi "github.com/openshift/api/image/v1" // group 4: openshift ✗ WRONG POSITION
"github.com/openshift/hypershift/support/releaseinfo/fixtures" // group 3: hypershift ✗ WRONG POSITION
)Three violations exist:
The correct import ordering should be: import (
"testing"
. "github.com/onsi/gomega"
"github.com/openshift/hypershift/support/releaseinfo/fixtures"
imageapi "github.com/openshift/api/image/v1"
"github.com/coreos/stream-metadata-go/stream"
)Recommendations
Evidence
|
sdminonne
force-pushed
the
CNTRLPLANE-3021
branch
from
21b6898 to
6acb740
Compare
|
Addressed the nitpick for |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@sdminonne: 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. |
|
Superseded by #8669 |
2 participants
What this PR does / why we need it:
Extends the
releaseinfopackage to parse ConfigMaps containing the newstreamskey (a JSON map keyed by stream name likerhel-9,rhel-10) alongside the legacystreamkey. This enables HyperShift to support dual-stream RHEL boot images from the OpenShift installer.Builds on top of #8673 (CNTRLPLANE-3020: adopt
coreos/stream-metadata-goupstream library).Key additions:
DeserializeMultiStreamImageMetadatawithStreamsResultfor parsing multi-stream ConfigMapsReleaseImageextended withStreams/DefaultStreamfields and aStreamForPoolresolver methodregistryclient_provider.go,registry_mirror_provider.go) updated to populate the new fields5.0-installer-coreos-bootimages.yamlfor multi-stream testingStreamMetadatacontinue working unchangedWhich issue(s) this PR fixes:
Fixes CNTRLPLANE-3021
Special notes for your reviewer:
StreamForPool("")returns the default stream;StreamForPool("rhel-10")returns the RHEL 10 streamstreamkey) continue to work identically via the existingStreamMetadatafieldChecklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores