Conversation
WalkthroughThe changes add support for propagating OS image stream information through the machine generation pipeline. A utility constant and helper function are introduced, AWS machine set inputs are extended with a new field, and master/worker generation orchestration threads this value through platform-specific machine creation calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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
🧹 Nitpick comments (3)
pkg/asset/machines/master.go (1)
271-272: AWS master machines don't receive the OS image stream value.The call to
aws.MachineshasGetOSImageStream(ic)commented out, while GCP and Azure calls include it. This aligns with the commented-out implementation inaws/machines.go, but creates inconsistent behavior across platforms.If this is intentional WIP, consider adding a brief comment explaining the plan or opening a tracking issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/master.go` around lines 271 - 272, The call to aws.Machines is missing the GetOSImageStream(ic) argument, causing AWS master machines to not receive the OS image stream; update the aws.Machines invocation to pass GetOSImageStream(ic) (matching GCP/Azure calls) and ensure aws.Machines and the AWS implementation in aws/machines.go accept and use that parameter, or if this omission is intentional WIP, add a brief inline comment at the aws.Machines call referencing the tracking issue/PR and the reason for omission so behavior is explicit.pkg/asset/machines/aws/machinesets.go (1)
116-118: Use theOSImageStreamLabelconstant instead of hardcoding the label key.The TODO at line 116 acknowledges this. The label key
"machine.openshift.io/os-image-stream"is hardcoded here and at line 157, butOSImageStreamLabelis already defined inpkg/asset/machines/util.go. Using the constant improves maintainability.♻️ Suggested refactor
- // TODO: use const for label - machineSpecLabels["machine.openshift.io/os-image-stream"] = in.OSImageStream + machineSpecLabels[machines.OSImageStreamLabel] = in.OSImageStreamAnd at line 157:
- "machine.openshift.io/os-image-stream": in.OSImageStream, + machines.OSImageStreamLabel: in.OSImageStream,This requires importing the parent
machinespackage (may need alias to avoid cycle) or moving the constant to a shared location.Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machinesets.go` around lines 116 - 118, Replace the hardcoded label key "machine.openshift.io/os-image-stream" in machinesets.go (the machineSpecLabels assignments) with the existing OSImageStreamLabel constant defined in pkg/asset/machines/util.go; update imports or add an alias import for the parent machines package to reference util.OSImageStreamLabel (or move the constant to a shared package if importing causes a cycle) and change both occurrences (the assignment at machineSpecLabels[...] = in.OSImageStream and the other use at the second occurrence) to use that constant.pkg/asset/machines/aws/machines.go (1)
44-45: Clarify the intended behavior for OS image stream labels on AWS Machines and ControlPlaneMachineSets.The commented-out code and TODOs suggest uncertainty about whether the
machine.openshift.io/os-image-streamlabel should be set on individualMachineobjects andControlPlaneMachineSettemplates. Meanwhile,machinesets.goactively sets this label.This inconsistency may be intentional (per the TODOs), but consider:
- Removing the commented code if the decision is to not add labels here
- Implementing the labels if they should be consistent with MachineSet behavior
Also applies to: 102-104, 184-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/aws/machines.go` around lines 44 - 45, Clarify and make consistent the OS image stream label behavior in Machines: decide whether to apply the "machine.openshift.io/os-image-stream" label on individual Machine objects and on the ControlPlaneMachineSet template used in Machines (function Machines) to match machinesets.go; then either remove the commented-out label code and TODOs in Machines (and corresponding commented blocks around ControlPlaneMachineSet template creation) if labels should not be added here, or implement setting the label on each Machine and on the ControlPlaneMachineSet template (same key/value used in machinesets.go) so behavior is consistent; update any related TODOs/comments to reflect the decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/asset/machines/worker.go`:
- Line 557: The calls to azure.MachineSets(...) and gcp.MachineSets(...) are
supplying GetOSImageStream(ic) but their function signatures still expect one
fewer argument; update the azure.MachineSets and gcp.MachineSets function
signatures (and any related MachineSetInput/parameter handling inside those
functions) to accept and propagate the OS image stream parameter (same type as
used by AWS's MachineSetInput.OSImageStream) so the callers at the spots using
GetOSImageStream(ic) compile and the OS image stream is used consistently across
MachineSets implementations.
---
Nitpick comments:
In `@pkg/asset/machines/aws/machines.go`:
- Around line 44-45: Clarify and make consistent the OS image stream label
behavior in Machines: decide whether to apply the
"machine.openshift.io/os-image-stream" label on individual Machine objects and
on the ControlPlaneMachineSet template used in Machines (function Machines) to
match machinesets.go; then either remove the commented-out label code and TODOs
in Machines (and corresponding commented blocks around ControlPlaneMachineSet
template creation) if labels should not be added here, or implement setting the
label on each Machine and on the ControlPlaneMachineSet template (same key/value
used in machinesets.go) so behavior is consistent; update any related
TODOs/comments to reflect the decision.
In `@pkg/asset/machines/aws/machinesets.go`:
- Around line 116-118: Replace the hardcoded label key
"machine.openshift.io/os-image-stream" in machinesets.go (the machineSpecLabels
assignments) with the existing OSImageStreamLabel constant defined in
pkg/asset/machines/util.go; update imports or add an alias import for the parent
machines package to reference util.OSImageStreamLabel (or move the constant to a
shared package if importing causes a cycle) and change both occurrences (the
assignment at machineSpecLabels[...] = in.OSImageStream and the other use at the
second occurrence) to use that constant.
In `@pkg/asset/machines/master.go`:
- Around line 271-272: The call to aws.Machines is missing the
GetOSImageStream(ic) argument, causing AWS master machines to not receive the OS
image stream; update the aws.Machines invocation to pass GetOSImageStream(ic)
(matching GCP/Azure calls) and ensure aws.Machines and the AWS implementation in
aws/machines.go accept and use that parameter, or if this omission is
intentional WIP, add a brief inline comment at the aws.Machines call referencing
the tracking issue/PR and the reason for omission so behavior is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c96c851e-5167-4302-969b-94d2bffe94c1
📒 Files selected for processing (5)
pkg/asset/machines/aws/machines.gopkg/asset/machines/aws/machinesets.gopkg/asset/machines/master.gopkg/asset/machines/util.gopkg/asset/machines/worker.go
| Role: pool.Name, | ||
| UserDataSecret: workerUserDataSecretName, | ||
| Hosts: dHosts, | ||
| OSImageStream: GetOSImageStream(ic), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify MachineSets function signatures in azure and gcp packages
echo "=== Azure MachineSets signature ==="
ast-grep --pattern 'func MachineSets($$$) ($$$) {
$$$
}' | grep -A2 "azure/machinesets.go" || rg -n "^func MachineSets" --type go -g '**/azure/machinesets.go'
echo "=== GCP MachineSets signature ==="
ast-grep --pattern 'func MachineSets($$$) ($$$) {
$$$
}' | grep -A2 "gcp/machinesets.go" || rg -n "^func MachineSets" --type go -g '**/gcp/machinesets.go'Repository: openshift/installer
Length of output: 14768
🏁 Script executed:
# Check the actual function calls in worker.go at the mentioned lines
echo "=== Worker.go lines 555-560 (around line 557 for AWS) ==="
sed -n '555,560p' pkg/asset/machines/worker.go
echo "=== Worker.go lines 625-635 (around line 629 for Azure) ==="
sed -n '625,635p' pkg/asset/machines/worker.go
echo "=== Worker.go lines 663-673 (around line 667 for GCP) ==="
sed -n '663,673p' pkg/asset/machines/worker.go
echo "=== Check MachineSetInput struct in aws package ==="
rg -n "type MachineSetInput" --type go -g '**/aws/**'
echo "=== Check GetOSImageStream function ==="
rg -n "GetOSImageStream" --type go -A 2Repository: openshift/installer
Length of output: 3672
🏁 Script executed:
# Verify the exact function signatures to confirm argument count mismatch
echo "=== Azure MachineSets signature (parameters) ==="
sed -n '21p' pkg/asset/machines/azure/machinesets.go | sed 's/func MachineSets(//' | sed 's/) \(\[//'
echo "=== GCP MachineSets signature (parameters) ==="
sed -n '18p' pkg/asset/machines/gcp/machinesets.go | sed 's/func MachineSets(//' | sed 's/) \(\[//'
# Check MachineSetInput to confirm AWS accepts OSImageStream
echo "=== MachineSetInput struct ==="
sed -n '19,45p' pkg/asset/machines/aws/machinesets.goRepository: openshift/installer
Length of output: 1202
🏁 Script executed:
# Get full Azure and GCP MachineSets signatures to check if osImageStream was added
echo "=== Full Azure MachineSets signature ==="
head -1 pkg/asset/machines/azure/machinesets.go | grep -A 1 "^package"
sed -n '21p' pkg/asset/machines/azure/machinesets.go
echo "=== Full GCP MachineSets signature ==="
sed -n '18p' pkg/asset/machines/gcp/machinesets.go
# Check if there are multiple versions or overloads
echo "=== All azure.MachineSets definitions ==="
rg "func MachineSets" pkg/asset/machines/azure/ --type go -A 1
echo "=== All gcp.MachineSets definitions ==="
rg "func MachineSets" pkg/asset/machines/gcp/ --type go -A 1Repository: openshift/installer
Length of output: 1310
🏁 Script executed:
# Double-check the exact argument count being passed to confirm the mismatch
echo "=== Line 629 (Azure call) - count arguments ==="
sed -n '629p' pkg/asset/machines/worker.go | tr ',' '\n' | nl
echo "=== Line 667 (GCP call) - count arguments ==="
sed -n '667p' pkg/asset/machines/worker.go | tr ',' '\n' | nlRepository: openshift/installer
Length of output: 257
🏁 Script executed:
# Check the exact argument count being passed
echo "=== Line 629 (Azure call) ==="
sed -n '629p' pkg/asset/machines/worker.go
echo "=== Line 667 (GCP call) ==="
sed -n '667p' pkg/asset/machines/worker.go
# Count commas to verify argument count
echo "=== Azure arguments (count commas + 1) ==="
sed -n '629p' pkg/asset/machines/worker.go | grep -o ',' | wc -l
echo "=== GCP arguments (count commas + 1) ==="
sed -n '667p' pkg/asset/machines/worker.go | grep -o ',' | wc -lRepository: openshift/installer
Length of output: 538
Azure and GCP function signatures need updating to accept the OS image stream parameter.
The code at lines 629 and 667 passes GetOSImageStream(ic) to azure.MachineSets() and gcp.MachineSets(), but neither function signature has been updated to accept this parameter:
- Azure (line 629): Passing 10 arguments; function accepts only 9
- GCP (line 667): Passing 7 arguments; function accepts only 6
The AWS implementation is correct—MachineSetInput struct includes the OSImageStream field. Update the Azure and GCP MachineSets() function signatures to accept the OS image stream parameter, or adjust the calling code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/asset/machines/worker.go` at line 557, The calls to
azure.MachineSets(...) and gcp.MachineSets(...) are supplying
GetOSImageStream(ic) but their function signatures still expect one fewer
argument; update the azure.MachineSets and gcp.MachineSets function signatures
(and any related MachineSetInput/parameter handling inside those functions) to
accept and propagate the OS image stream parameter (same type as used by AWS's
MachineSetInput.OSImageStream) so the callers at the spots using
GetOSImageStream(ic) compile and the OS image stream is used consistently across
MachineSets implementations.
1 participant
/cc @isabella-janssen