Open
Conversation
See https://groups.google.com/g/metal3-dev/c/8-rbzNZagqg Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Removes old overlays for release v0.9 Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
Signed-off-by: Lennart Jern <lennart.jern@est.tech>
…kage-name 🌱 Exclude revive var-naming check for test/vbmctl/pkg/api
…1-12 🌱 Adds overlays for release v0.12
Signed-off-by: MahnoorAsghar <masghar@redhat.com>
Zizmor scans workflow files to keep them hardened. It runs on push to produce a report in security tab, and it runs on PRs and blocks them if they is any errors. - Harden dependabot workflow: disable persist-credentials on checkout, pass token explicitly to add-and-commit action - Suppress artipacked on release workflow checkout that needs credentials for pushing branches and tags - Suppress superfluous-actions on softprops/action-gh-release pending replacement with gh CLI Signed-off-by: Tuomo Tanskanen <tuomo.tanskanen@est.tech>
🌱 add zizmor scanner
…issue 🌱 Update vbmctlapi package name to fix lint error
Use more recent gophercloud version, which forces go bump to v1.25.7 Signed-off-by: MahnoorAsghar <masghar@redhat.com>
✨ Add PCI Address field to HardwareData NIC
This addresses: - https://osv.dev/GO-2026-4601 - https://osv.dev/GO-2026-4602 - https://osv.dev/GO-2026-4603 Signed-off-by: Lennart Jern <lennart.jern@est.tech>
⚠️ Remove the iRMC driver, deprecate BMH.Spec.Firmware
🌱 Update Go version to 1.25.8
WalkthroughPR introduces static analysis workflow, updates Go toolchain/dependencies, adds NIC PCIAddress field across APIs and schemas, deprecates firmware fields, removes IRMC BMC driver, updates e2e test overlays to release-0.12, refactors IRSO deployment handling in upgrade tests, renames test API package alias to vbmctlapi, and refactors error handling by discarding unused return values. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MahnoorAsghar 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/provisioner/ironic/register_test.go (2)
1345-1362:⚠️ Potential issue | 🟠 MajorMissing
.WithDrivers()will causeTryInit()to fail.The test server setup lacks
.WithDrivers(), but the newly addedTryInit()call internally invokescheckIronicConductor(), which makes an HTTP GET request to/v1/drivers. Without proper endpoint setup, the server returns an empty or malformed response causing parsing errors thatTryInit()now returns as a non-nil error—failing the test.Other tests that explicitly or implicitly use
TryInit()(e.g.,TestRegisterDeprovisioningNeedsPreprovisioningImageat line 1407) include.WithDrivers()in their server setup.🐛 Proposed fix to add missing driver configuration
// Set up ironic server to return the node ironic := testserver.NewIronic(t). + WithDrivers(). Node(nodes.Node{ UUID: host.Status.Provisioning.ID, }).NodeUpdate(nodes.Node{ UUID: host.Status.Provisioning.ID, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/register_test.go` around lines 1345 - 1362, The test server created by testserver.NewIronic(...) is missing a drivers endpoint, causing prov.TryInit (which calls checkIronicConductor and issues a GET /v1/drivers) to fail; fix by updating the NewIronic chain in register_test.go to include .WithDrivers() on the ironic test server instance (the same place you call NewIronic(...).Node(...).NodeUpdate(...)) so the drivers endpoint returns a valid response for TryInit/checkIronicConductor.
1375-1392:⚠️ Potential issue | 🟠 MajorSame issue: missing
.WithDrivers()will causeTryInit()to fail.Same problem as
TestRegisterDisablePowerOff—the test server is configured with.WithVersion("1.87")but lacks.WithDrivers(). TheTryInit()call will fail when querying/v1/drivers.🐛 Proposed fix
// Set up ironic server to return the node - ironic := testserver.NewIronic(t).WithVersion("1.87"). + ironic := testserver.NewIronic(t).WithVersion("1.87").WithDrivers(). Node(nodes.Node{ UUID: host.Status.Provisioning.ID, }).NodeUpdate(nodes.Node{ UUID: host.Status.Provisioning.ID, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/register_test.go` around lines 1375 - 1392, The test server is missing a drivers list so prov.TryInit() fails when it queries /v1/drivers; update the testserver setup chain (the NewIronic(...).WithVersion("1.87") call before .Node(...) / .NodeUpdate(...)) to include a .WithDrivers(...) call supplying the drivers that TryInit expects (e.g., include the BMC/provisioning drivers used elsewhere such as "ipmi" or whatever drivers your TryInit logic asserts), so the server responds to /v1/drivers and TryInit succeeds.pkg/provisioner/ironic/factory.go (1)
371-383:⚠️ Potential issue | 🟠 MajorDon't swallow
Close()errors here.When writing the CA certificate file, ignoring the
Close()error means the final flush to disk can fail silently. The function would still return a path to a potentially incomplete file, causing downstream TLS configuration errors that are disconnected from the actual I/O failure.💾 Suggested fix
func writeTempFile(prefix string, data []byte) (string, error) { tmpFile, err := os.CreateTemp("", prefix) if err != nil { return "", err } - defer func() { _ = tmpFile.Close() }() - - _, err = tmpFile.Write(data) - if err != nil { + if _, err = tmpFile.Write(data); err != nil { + _ = tmpFile.Close() + return "", err + } + if err = tmpFile.Close(); err != nil { return "", err } return tmpFile.Name(), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/factory.go` around lines 371 - 383, The writeTempFile function currently defers and ignores tmpFile.Close() which can hide flush/close errors; modify writeTempFile so Close() error is captured and returned if non-nil (or combined with any existing write error) instead of being swallowed: perform the write to tmpFile, then call tmpFile.Close() (capturing its error), and if either the write or the Close returned an error return that error (or a wrapped/combined error) and avoid returning a filename on failure; update references to the tmpFile variable and the writeTempFile function accordingly.test/vbmctl/pkg/config/config_test.go (1)
8-9:⚠️ Potential issue | 🔴 CriticalCompilation error:
vbmctlapiis undefined.The import at line 8 brings in the
apipackage with its default aliasapi, but lines 79, 165, and 198 referencevbmctlapi.VMConfig. This will fail to compile with "undefined: vbmctlapi".Either alias the import as
vbmctlapior change the code to useapi.VMConfig.🐛 Proposed fix: Add import alias
import ( "os" "path/filepath" "testing" - "github.com/metal3-io/baremetal-operator/test/vbmctl/pkg/api" + vbmctlapi "github.com/metal3-io/baremetal-operator/test/vbmctl/pkg/api" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/vbmctl/pkg/config/config_test.go` around lines 8 - 9, The tests reference vbmctlapi.VMConfig but the import brings in the package as api, causing "undefined: vbmctlapi"; fix by either changing the import to use the alias vbmctlapi (e.g., import vbmctlapi "github.com/metal3-io/baremetal-operator/test/vbmctl/pkg/api") or update all references to use api.VMConfig (and similar references at the locations that currently use vbmctlapi), ensuring consistency for symbols like VMConfig across the file.
🧹 Nitpick comments (5)
pkg/provisioner/ironic/testserver/server.go (1)
253-253: Surface response write failures here.
_, _ = fmt.Fprint(...)still swallows the write error, so this shared test helper can hide truncated/failed responses. If the write is worth making, it’s worth failing the test onerr; otherwise a barefmt.Fprint(...)call is clearer than the blank-identifier assignment.Proposed fix
- _, _ = fmt.Fprint(w, payload) + if _, err := fmt.Fprint(w, payload); err != nil { + m.t.Errorf("%s: failed to write response payload: %v", m.name, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/testserver/server.go` at line 253, The current helper swallows errors by using "_, _ = fmt.Fprint(w, payload)"; change it to check the returned error from fmt.Fprint(w, payload) and surface failures instead of discarding them — e.g., replace the blank-assignment with an error check and fail the test or propagate the error (call t.Fatalf/require.Fail or return the error) when fmt.Fprint returns a non-nil err so truncated/failed writes are not hidden.internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go (1)
107-107: Add one explicitirmc://rejection case.Lines 107, 139, 179, 345, and 361 correctly move the supported-path fixtures to Redfish, but that also removes direct regression coverage for the behavior this PR changes:
irmc://...should now be rejected. SincevalidateHostnow goes throughbmc.NewAccessDetailsininternal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go:40-80, keeping one negativeirmc://test would lock in the driver removal.Example of the missing regression case
+ { + name: "removedIRMCBMCType", + newBMH: &metal3api.BareMetalHost{ + TypeMeta: tm, + ObjectMeta: om, + Spec: metal3api.BareMetalHostSpec{ + BMC: metal3api.BMCDetails{ + Address: "irmc://127.0.1.1", + CredentialsName: "test1", + }, + }}, + oldBMH: nil, + wantedErr: "Unknown BMC type 'irmc' for address irmc://127.0.1.1", + },Also applies to: 139-139, 179-179, 345-345, 361-361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go` at line 107, The test suite removed direct coverage for rejecting irmc:// BMC URLs; add an explicit negative test case in internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go that supplies Address: "irmc://bmc.example.com" and asserts validateHost (which now calls bmc.NewAccessDetails) rejects it; specifically, add one test entry that expects an error for an irmc:// access string to lock in the behavior change and prevent regression of driver removal.pkg/provisioner/ironic/factory_test.go (1)
31-33: Fail fast on env mutation errors in the test fixture.Swallowing
os.Setenv/os.Unsetenverrors here can leave a subtest running with a partially applied environment and turn a setup failure into a misleading assertion failure later. Since this is test infrastructure, it’s better to surface the error immediately.Proposed fix
-func (f *EnvFixture) TearDown() { +func (f *EnvFixture) TearDown(t *testing.T) { + t.Helper() for e, v := range f.origEnv { if v == "" { - _ = os.Unsetenv(e) + require.NoError(t, os.Unsetenv(e)) } else { - _ = os.Setenv(e, v) + require.NoError(t, os.Setenv(e, v)) } } } -func (f *EnvFixture) replace(env, value string) { +func (f *EnvFixture) replace(t *testing.T, env, value string) { + t.Helper() f.origEnv[env] = os.Getenv(env) if value == "" { - _ = os.Unsetenv(env) + require.NoError(t, os.Unsetenv(env)) } else { - _ = os.Setenv(env, value) + require.NoError(t, os.Setenv(env, value)) } } -func (f *EnvFixture) SetUp() { +func (f *EnvFixture) SetUp(t *testing.T) { f.origEnv = map[string]string{} - f.replace("IRONIC_ENDPOINT", f.ironicEndpoint) - f.replace("DEPLOY_KERNEL_URL", f.kernelURL) - f.replace("DEPLOY_RAMDISK_URL", f.ramdiskURL) - f.replace("DEPLOY_ISO_URL", f.isoURL) - f.replace("LIVE_ISO_FORCE_PERSISTENT_BOOT_DEVICE", f.liveISOForcePersistentBootDevice) - f.replace("DIRECT_DEPLOY_FORCE_PERSISTENT_BOOT_DEVICE", f.directDeployForcePersistentBootDevice) - f.replace("IRONIC_CACERT_FILE", f.ironicCACertFile) - f.replace("IRONIC_CLIENT_CERT_FILE", f.ironicClientCertFile) - f.replace("IRONIC_CLIENT_PRIVATE_KEY_FILE", f.ironicClientPrivateKeyFile) - f.replace("IRONIC_INSECURE", f.ironicInsecure) - f.replace("IRONIC_SKIP_CLIENT_SAN_VERIFY", f.ironicSkipClientSANVerify) + f.replace(t, "IRONIC_ENDPOINT", f.ironicEndpoint) + f.replace(t, "DEPLOY_KERNEL_URL", f.kernelURL) + f.replace(t, "DEPLOY_RAMDISK_URL", f.ramdiskURL) + f.replace(t, "DEPLOY_ISO_URL", f.isoURL) + f.replace(t, "LIVE_ISO_FORCE_PERSISTENT_BOOT_DEVICE", f.liveISOForcePersistentBootDevice) + f.replace(t, "DIRECT_DEPLOY_FORCE_PERSISTENT_BOOT_DEVICE", f.directDeployForcePersistentBootDevice) + f.replace(t, "IRONIC_CACERT_FILE", f.ironicCACertFile) + f.replace(t, "IRONIC_CLIENT_CERT_FILE", f.ironicClientCertFile) + f.replace(t, "IRONIC_CLIENT_PRIVATE_KEY_FILE", f.ironicClientPrivateKeyFile) + f.replace(t, "IRONIC_INSECURE", f.ironicInsecure) + f.replace(t, "IRONIC_SKIP_CLIENT_SAN_VERIFY", f.ironicSkipClientSANVerify) }- defer tc.env.TearDown() - tc.env.SetUp() + defer tc.env.TearDown(t) + tc.env.SetUp(t)Also applies to: 41-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/factory_test.go` around lines 31 - 33, The test fixture currently ignores errors from os.Setenv/os.Unsetenv which can leave subtests with a corrupt environment; update the environment setup in pkg/provisioner/ironic/factory_test.go to check the return errors and fail the test immediately (e.g., call t.Fatalf("failed to set env %s=%s: %v", e, v, err) or use require.NoError(t, err)) inside the loop that calls os.Setenv/os.Unsetenv, and apply the same change to the second occurrence (the block around lines 41-43) so any env mutation error surfaces immediately rather than being swallowed.pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go (1)
220-221: CoverPCIAddresson the IPv6 append path too.The new expectation only exercises the IPv4 branch.
getNICDetailsnow copiesPCIAddressin both append sites, so a regression in the IPv6/dual-stack path would still pass.🧪 Small test extension
{ Name: "eth1", IPV6Address: "2001:db8::1", MACAddress: "66:77:88:99:aa:bb", + PCIAddress: "0000:00:01.0", SpeedMbps: 1000},if (!reflect.DeepEqual(nics[1], metal3api.NIC{ - Name: "eth1", - MAC: "66:77:88:99:aa:bb", - IP: "2001:db8::1", - SpeedGbps: 1, + Name: "eth1", + MAC: "66:77:88:99:aa:bb", + IP: "2001:db8::1", + PCIAddress: "0000:00:01.0", + SpeedGbps: 1, })) {Also applies to: 242-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go` around lines 220 - 221, The test currently only asserts that PCIAddress is copied for the IPv4 append branch; extend the expectations in hardwaredetails_test.go to also assert PCIAddress for the IPv6/dual-stack append path so both branches of getNICDetails are covered—locate the test that builds expected NIC entries (the entries with MACAddress "00:11:22:33:44:55" and the second appended IPv6 entry) and add PCIAddress: "0000:00:00.0" to the expected struct for the IPv6/dual-stack entry (and any other appended entry created by getNICDetails) so the test fails if PCIAddress is not propagated in the IPv6 path.test/vbmctl/pkg/libvirt/templates.go (1)
141-143: Pick one owner for VM defaulting.
VMManager.Createalready normalizescfgbefore calling this helper, so this secondDefaults()pass creates another place that has to stay behaviorally in sync when defaulting rules change. I'd keep the defaulting in one layer only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/vbmctl/pkg/libvirt/templates.go` around lines 141 - 143, VMConfigToTemplateData currently calls cfg.Defaults() again, duplicating defaulting already performed by VMManager.Create; remove the extra cfg = cfg.Defaults() line from VMConfigToTemplateData so defaulting is only done by VMManager.Create, and ensure callers rely on VMManager.Create to normalize VMConfig before invoking VMConfigToTemplateData (keep VMConfigToTemplateData pure and document that it expects an already-defaulted cfg).
🤖 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/make-bm-worker/main.go`:
- Line 107: The stdout write ignores fmt.Fprint(os.Stdout, result) errors;
change it to check the returned error from that call and propagate failures
instead of discarding them so the command exits non-zero on write errors. Locate
the fmt.Fprint invocation that writes the result variable and, on err != nil,
log the error and exit with a non-zero status (or return the error from main to
be handled consistently with the existing error pattern used earlier in this
file).
In `@go.mod`:
- Line 3: Multiple go.mod files declare different Go versions causing mismatch;
update the four module go.mod files (apis/go.mod, test/go.mod,
pkg/hardwareutils/go.mod, hack/tools/go.mod) to use the same Go version as the
root (change their "go" directive from 1.25.0 to 1.25.7) or, if you prefer the
older version, change the root go.mod "go" directive in the same way so all
modules align; ensure each file's "go" directive is identical across modules and
commit the changes.
In `@test/e2e/config/fixture.yaml`:
- Around line 70-80: The images preload list in test/e2e/config/fixture.yaml
lacks the quay.io/metal3-io/baremetal-operator:release-0.12 entry referenced by
the new initBMOKustomization (fixture-release-0.12); update the images array to
include quay.io/metal3-io/baremetal-operator:release-0.12 so the release-0.12
case uses the cached/tryLoad path instead of pulling from the registry. Ensure
the exact tag string quay.io/metal3-io/baremetal-operator:release-0.12 is added
alongside the existing release-0.11 entry.
In `@test/e2e/e2e_config.go`:
- Around line 66-67: Config.Validate() currently doesn't check the new
IrsoKustomization field, so typos only surface later; update Config.Validate to,
when c.IrsoKustomization is non-empty, call os.Stat on that path and return a
validation error if stat fails (wrap the underlying error and include the field
name "IrsoKustomization" and the given path in the message) so invalid paths are
caught during config load.
In `@test/e2e/upgrade_test.go`:
- Around line 48-63: The IRSO installation is created outside the same
setup/teardown lifecycle causing leftover namespaces and nil cleanup errors;
modify the logic that checks deployIRSO and calls BuildAndApplyKustomization
(the block using deployIRSO, input.IrsoKustomization and
BuildAndApplyKustomization) so that either (A) it runs after namespace/watch
initialization and registers the created resources in the test’s teardown (e.g.,
record the applied kustomization identifier and ensure AfterEach removes
"ironic-standalone-operator-system"), or (B) track the IRSO deployment with a
new variable (e.g., irsoApplied boolean or irsoCleanup func) and invoke that
cleanup from AfterEach alongside existing namespace/cancelWatches cleanup;
ensure AfterEach no longer assumes namespace/cancelWatches are non-nil before
attempting teardown.
In `@test/vbmctl/pkg/api/types.go`:
- Around line 1-3: Update the package documentation comment in types.go so it
names the current package (change "Package api" to "Package vbmctlapi"), and
update the example references in test/vbmctl/pkg/libvirt/doc.go to use the new
package qualifier by replacing occurrences of api.VMConfig, api.VolumeConfig,
and api.PoolConfig with vbmctlapi.VMConfig, vbmctlapi.VolumeConfig, and
vbmctlapi.PoolConfig respectively so the examples match the renamed package.
---
Outside diff comments:
In `@pkg/provisioner/ironic/factory.go`:
- Around line 371-383: The writeTempFile function currently defers and ignores
tmpFile.Close() which can hide flush/close errors; modify writeTempFile so
Close() error is captured and returned if non-nil (or combined with any existing
write error) instead of being swallowed: perform the write to tmpFile, then call
tmpFile.Close() (capturing its error), and if either the write or the Close
returned an error return that error (or a wrapped/combined error) and avoid
returning a filename on failure; update references to the tmpFile variable and
the writeTempFile function accordingly.
In `@pkg/provisioner/ironic/register_test.go`:
- Around line 1345-1362: The test server created by testserver.NewIronic(...) is
missing a drivers endpoint, causing prov.TryInit (which calls
checkIronicConductor and issues a GET /v1/drivers) to fail; fix by updating the
NewIronic chain in register_test.go to include .WithDrivers() on the ironic test
server instance (the same place you call
NewIronic(...).Node(...).NodeUpdate(...)) so the drivers endpoint returns a
valid response for TryInit/checkIronicConductor.
- Around line 1375-1392: The test server is missing a drivers list so
prov.TryInit() fails when it queries /v1/drivers; update the testserver setup
chain (the NewIronic(...).WithVersion("1.87") call before .Node(...) /
.NodeUpdate(...)) to include a .WithDrivers(...) call supplying the drivers that
TryInit expects (e.g., include the BMC/provisioning drivers used elsewhere such
as "ipmi" or whatever drivers your TryInit logic asserts), so the server
responds to /v1/drivers and TryInit succeeds.
In `@test/vbmctl/pkg/config/config_test.go`:
- Around line 8-9: The tests reference vbmctlapi.VMConfig but the import brings
in the package as api, causing "undefined: vbmctlapi"; fix by either changing
the import to use the alias vbmctlapi (e.g., import vbmctlapi
"github.com/metal3-io/baremetal-operator/test/vbmctl/pkg/api") or update all
references to use api.VMConfig (and similar references at the locations that
currently use vbmctlapi), ensuring consistency for symbols like VMConfig across
the file.
---
Nitpick comments:
In `@internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go`:
- Line 107: The test suite removed direct coverage for rejecting irmc:// BMC
URLs; add an explicit negative test case in
internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go that
supplies Address: "irmc://bmc.example.com" and asserts validateHost (which now
calls bmc.NewAccessDetails) rejects it; specifically, add one test entry that
expects an error for an irmc:// access string to lock in the behavior change and
prevent regression of driver removal.
In `@pkg/provisioner/ironic/factory_test.go`:
- Around line 31-33: The test fixture currently ignores errors from
os.Setenv/os.Unsetenv which can leave subtests with a corrupt environment;
update the environment setup in pkg/provisioner/ironic/factory_test.go to check
the return errors and fail the test immediately (e.g., call t.Fatalf("failed to
set env %s=%s: %v", e, v, err) or use require.NoError(t, err)) inside the loop
that calls os.Setenv/os.Unsetenv, and apply the same change to the second
occurrence (the block around lines 41-43) so any env mutation error surfaces
immediately rather than being swallowed.
In `@pkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.go`:
- Around line 220-221: The test currently only asserts that PCIAddress is copied
for the IPv4 append branch; extend the expectations in hardwaredetails_test.go
to also assert PCIAddress for the IPv6/dual-stack append path so both branches
of getNICDetails are covered—locate the test that builds expected NIC entries
(the entries with MACAddress "00:11:22:33:44:55" and the second appended IPv6
entry) and add PCIAddress: "0000:00:00.0" to the expected struct for the
IPv6/dual-stack entry (and any other appended entry created by getNICDetails) so
the test fails if PCIAddress is not propagated in the IPv6 path.
In `@pkg/provisioner/ironic/testserver/server.go`:
- Line 253: The current helper swallows errors by using "_, _ = fmt.Fprint(w,
payload)"; change it to check the returned error from fmt.Fprint(w, payload) and
surface failures instead of discarding them — e.g., replace the blank-assignment
with an error check and fail the test or propagate the error (call
t.Fatalf/require.Fail or return the error) when fmt.Fprint returns a non-nil err
so truncated/failed writes are not hidden.
In `@test/vbmctl/pkg/libvirt/templates.go`:
- Around line 141-143: VMConfigToTemplateData currently calls cfg.Defaults()
again, duplicating defaulting already performed by VMManager.Create; remove the
extra cfg = cfg.Defaults() line from VMConfigToTemplateData so defaulting is
only done by VMManager.Create, and ensure callers rely on VMManager.Create to
normalize VMConfig before invoking VMConfigToTemplateData (keep
VMConfigToTemplateData pure and document that it expects an already-defaulted
cfg).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62c94aaf-df1b-4bf0-98bd-efc635225dc7
⛔ Files ignored due to path filters (31)
go.sumis excluded by!**/*.sumtest/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**test/vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hardwaredata_types.gois excluded by!**/vendor/**test/vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.gois excluded by!**/vendor/**vendor/github.com/gophercloud/gophercloud/v2/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/httpbasic/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/inventory/plugindata.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/inventory/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/drivers/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/nodes/requests.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/nodes/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetal/v1/ports/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/baremetalintrospection/v1/introspection/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/openstack/utils/choose_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/pagination/pager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/params.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/provider_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gophercloud/gophercloud/v2/results.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/baremetalhost_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1/hardwaredata_types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc/irmc.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ioctl_signed.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/ioctl_unsigned.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_solaris.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/unix/syscall_unix.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/syscall_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/types_windows.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/sys/windows/zsyscall_windows.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (43)
.github/workflows/zizmor.ymlDockerfileMakefileapis/metal3.io/v1alpha1/baremetalhost_types.goapis/metal3.io/v1alpha1/hardwaredata_types.gocmd/make-bm-worker/main.goconfig/base/crds/bases/metal3.io_baremetalhosts.yamlconfig/base/crds/bases/metal3.io_hardwaredata.yamlconfig/overlays/e2e-release-0.12/ironic.envconfig/overlays/e2e-release-0.12/kustomization.yamlconfig/overlays/fixture-release-0.12/kustomization.yamlconfig/render/capm3.yamlgo.modhack/ci-e2e.shinternal/controller/metal3.io/hostfirmwaresettings_controller.gointernal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.gopkg/hardwareutils/bmc/access_test.gopkg/hardwareutils/bmc/irmc.gopkg/provisioner/ironic/factory.gopkg/provisioner/ironic/factory_test.gopkg/provisioner/ironic/hardwaredetails/hardwaredetails.gopkg/provisioner/ironic/hardwaredetails/hardwaredetails_test.gopkg/provisioner/ironic/raid_test.gopkg/provisioner/ironic/register_test.gopkg/provisioner/ironic/testserver/ironic_test.gopkg/provisioner/ironic/testserver/server.gotest/e2e/config/fixture.yamltest/e2e/config/ironic.yamltest/e2e/data/ironic-standalone-operator/ironic/overlays/e2e-release-32.0/kustomization.yamltest/e2e/data/ironic-standalone-operator/ironic/overlays/e2e-release-33.0/kustomization.yamltest/e2e/data/ironic-standalone-operator/operator/irso-v0.5/kustomization.yamltest/e2e/data/ironic-standalone-operator/operator/irso-v0.7/kustomization.yamltest/e2e/e2e_config.gotest/e2e/upgrade_test.gotest/vbmctl/cmd/vbmctl/main.gotest/vbmctl/main.gotest/vbmctl/pkg/api/types.gotest/vbmctl/pkg/api/types_test.gotest/vbmctl/pkg/config/config.gotest/vbmctl/pkg/config/config_test.gotest/vbmctl/pkg/libvirt/pool.gotest/vbmctl/pkg/libvirt/templates.gotest/vbmctl/pkg/libvirt/vm.go
💤 Files with no reviewable changes (2)
- pkg/hardwareutils/bmc/access_test.go
- pkg/hardwareutils/bmc/irmc.go
| } | ||
|
|
||
| fmt.Fprint(os.Stdout, result) | ||
| _, _ = fmt.Fprint(os.Stdout, result) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l cmd/make-bm-worker/main.goRepository: openshift/baremetal-operator
Length of output: 102
🏁 Script executed:
head -120 cmd/make-bm-worker/main.go | tail -30Repository: openshift/baremetal-operator
Length of output: 1113
Handle stdout write failures instead of discarding them.
Line 107 ignores the fmt.Fprint error. If this command is piped and the reader closes early, it silently returns success after emitting only part of the manifest. Propagate the write failure and exit non-zero to match the error handling pattern established above.
Suggested fix
result, err := template.Render()
if err != nil {
fmt.Fprintf(os.Stderr, "ERROR: %s\n", err)
os.Exit(1)
}
- _, _ = fmt.Fprint(os.Stdout, result)
+ if _, err := fmt.Fprint(os.Stdout, result); err != nil {
+ fmt.Fprintf(os.Stderr, "ERROR: %s\n", err)
+ os.Exit(1)
+ }
}📝 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.Fprint(os.Stdout, result) | |
| result, err := template.Render() | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "ERROR: %s\n", err) | |
| os.Exit(1) | |
| } | |
| if _, err := fmt.Fprint(os.Stdout, result); err != nil { | |
| fmt.Fprintf(os.Stderr, "ERROR: %s\n", err) | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/make-bm-worker/main.go` at line 107, The stdout write ignores
fmt.Fprint(os.Stdout, result) errors; change it to check the returned error from
that call and propagate failures instead of discarding them so the command exits
non-zero on write errors. Locate the fmt.Fprint invocation that writes the
result variable and, on err != nil, log the error and exit with a non-zero
status (or return the error from main to be handled consistently with the
existing error pattern used earlier in this file).
| module github.com/metal3-io/baremetal-operator | ||
|
|
||
| go 1.25.0 | ||
| go 1.25.7 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Go version declarations across all go.mod files in the repo
# Expect: Consistent Go versions or documented reason for divergence
fd -t f -e mod 'go.mod' --exec grep -H '^go ' {}Repository: openshift/baremetal-operator
Length of output: 205
Go version mismatch across modules.
The main go.mod declares go 1.25.7, while four other modules—apis/go.mod, test/go.mod, pkg/hardwareutils/go.mod, and hack/tools/go.mod—still declare go 1.25.0. This divergence in a multi-module workspace may cause build inconsistencies or CI issues when modules are built independently. Align Go versions across all modules or document why the mismatch is necessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 3, Multiple go.mod files declare different Go versions
causing mismatch; update the four module go.mod files (apis/go.mod, test/go.mod,
pkg/hardwareutils/go.mod, hack/tools/go.mod) to use the same Go version as the
root (change their "go" directive from 1.25.0 to 1.25.7) or, if you prefer the
older version, change the root go.mod "go" directive in the same way so all
modules align; ensure each file's "go" directive is identical across modules and
commit the changes.
Comment on lines
+70
to
+80
| initBMOKustomization: "../../config/overlays/fixture-release-0.10" | ||
| upgradeEntityKustomization: "../../config/overlays/fixture" | ||
| upgradeEntityName: "bmo" | ||
| - deployIronic: false | ||
| deployBMO: true | ||
| initBMOKustomization: "../../config/overlays/fixture-release-0.10" | ||
| initBMOKustomization: "../../config/overlays/fixture-release-0.11" | ||
| upgradeEntityKustomization: "../../config/overlays/fixture" | ||
| upgradeEntityName: "bmo" | ||
| - deployIronic: false | ||
| deployBMO: true | ||
| initBMOKustomization: "../../config/overlays/fixture-release-0.11" | ||
| initBMOKustomization: "../../config/overlays/fixture-release-0.12" |
There was a problem hiding this comment.
Add the matching release-0.12 image preload.
These upgrade specs now reference ../../config/overlays/fixture-release-0.12, but the images list in this file still stops at quay.io/metal3-io/baremetal-operator:release-0.11. That makes the new case fall back to a registry pull instead of the cached/tryLoad path, which is slower and more brittle in CI. Please add quay.io/metal3-io/baremetal-operator:release-0.12 to the preload list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/config/fixture.yaml` around lines 70 - 80, The images preload list
in test/e2e/config/fixture.yaml lacks the
quay.io/metal3-io/baremetal-operator:release-0.12 entry referenced by the new
initBMOKustomization (fixture-release-0.12); update the images array to include
quay.io/metal3-io/baremetal-operator:release-0.12 so the release-0.12 case uses
the cached/tryLoad path instead of pulling from the registry. Ensure the exact
tag string quay.io/metal3-io/baremetal-operator:release-0.12 is added alongside
the existing release-0.11 entry.
Comment on lines
+66
to
+67
| // Path to the Irso kustomization. | ||
| IrsoKustomization string `yaml:"irsoKustomization,omitempty"` |
There was a problem hiding this comment.
Validate IrsoKustomization during config load.
This new path is not checked in Config.Validate(), so a typo survives config parsing and only fails later when the upgrade flow tries to apply it. Please stat it when the field is non-empty.
🛠️ Suggested validation
if _, err := os.Stat(spec.UpgradeEntityKustomization); err != nil {
return fmt.Errorf("UpgradeEntityKustomization file not found: %s. Error %w", spec.UpgradeEntityKustomization, err)
}
+ if spec.IrsoKustomization != "" {
+ if _, err := os.Stat(spec.IrsoKustomization); err != nil {
+ return fmt.Errorf("IRSO kustomization file not found: %s. Error %w", spec.IrsoKustomization, err)
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/e2e_config.go` around lines 66 - 67, Config.Validate() currently
doesn't check the new IrsoKustomization field, so typos only surface later;
update Config.Validate to, when c.IrsoKustomization is non-empty, call os.Stat
on that path and return a validation error if stat fails (wrap the underlying
error and include the field name "IrsoKustomization" and the given path in the
message) so invalid paths are caught during config load.
Comment on lines
+48
to
+63
| if deployIRSO { | ||
| if input.IrsoKustomization == "" { | ||
| Fail("deployIRSO is enabled but IrsoKustomization is empty; please specify a valid kustomization path in the test config") | ||
| } | ||
| err := BuildAndApplyKustomization(ctx, &BuildAndApplyKustomizationInput{ | ||
| Kustomization: input.IrsoKustomization, | ||
| ClusterProxy: upgradeClusterProxy, | ||
| WaitForDeployment: true, | ||
| WatchDeploymentLogs: true, | ||
| DeploymentName: "ironic-standalone-operator-controller-manager", | ||
| DeploymentNamespace: "ironic-standalone-operator-system", | ||
| LogPath: filepath.Join(testCaseArtifactFolder, "logs", "ironic-standalone-operator-system"), | ||
| WaitIntervals: e2eConfig.GetIntervals("default", "wait-deployment"), | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| } |
There was a problem hiding this comment.
Bring IRSO setup under the same cleanup lifecycle.
This install path now runs before the namespace/watch handles exist, but AfterEach still assumes namespace/cancelWatches are initialized and only tears down baremetal-operator-system. In existing-cluster mode that leaves ironic-standalone-operator-system behind between cases, so the irso-v0.5 and irso-v0.7 entries become order-dependent; if IRSO setup fails early, cleanup can also trip over nil state and mask the real failure. Please move this under the same setup/teardown path or explicitly track and remove the applied IRSO kustomization.
Also applies to: 320-346, 353-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/upgrade_test.go` around lines 48 - 63, The IRSO installation is
created outside the same setup/teardown lifecycle causing leftover namespaces
and nil cleanup errors; modify the logic that checks deployIRSO and calls
BuildAndApplyKustomization (the block using deployIRSO, input.IrsoKustomization
and BuildAndApplyKustomization) so that either (A) it runs after namespace/watch
initialization and registers the created resources in the test’s teardown (e.g.,
record the applied kustomization identifier and ensure AfterEach removes
"ironic-standalone-operator-system"), or (B) track the IRSO deployment with a
new variable (e.g., irsoApplied boolean or irsoCleanup func) and invoke that
cleanup from AfterEach alongside existing namespace/cancelWatches cleanup;
ensure AfterEach no longer assumes namespace/cancelWatches are non-nil before
attempting teardown.
Comment on lines
1
to
+3
| // Package api defines the core types used by vbmctl for managing virtual | ||
| // bare metal environments. | ||
| package api | ||
| package vbmctlapi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,3p' test/vbmctl/pkg/api/types.go
sed -n '20,45p' test/vbmctl/pkg/libvirt/doc.goRepository: openshift/baremetal-operator
Length of output: 851
Update package documentation comment to match renamed package.
The package comment at line 1 still refers to Package api while the package is now named vbmctlapi. Additionally, example code in test/vbmctl/pkg/libvirt/doc.go (lines 20–45) references the old api.VMConfig, api.VolumeConfig, and api.PoolConfig fully qualified names, which will be incorrect after this rename.
📝 Minimal fix for types.go
-// Package api defines the core types used by vbmctl for managing virtual
+// Package vbmctlapi defines the core types used by vbmctl for managing virtual
// bare metal environments.
package vbmctlapiUpdate the example code references in libvirt/doc.go to use vbmctlapi. instead of api..
📝 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
| // Package api defines the core types used by vbmctl for managing virtual | |
| // bare metal environments. | |
| package api | |
| package vbmctlapi | |
| // Package vbmctlapi defines the core types used by vbmctl for managing virtual | |
| // bare metal environments. | |
| package vbmctlapi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/vbmctl/pkg/api/types.go` around lines 1 - 3, Update the package
documentation comment in types.go so it names the current package (change
"Package api" to "Package vbmctlapi"), and update the example references in
test/vbmctl/pkg/libvirt/doc.go to use the new package qualifier by replacing
occurrences of api.VMConfig, api.VolumeConfig, and api.PoolConfig with
vbmctlapi.VMConfig, vbmctlapi.VolumeConfig, and vbmctlapi.PoolConfig
respectively so the examples match the renamed package.
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
6 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.
Summary by CodeRabbit
New Features
Deprecations
Breaking Changes
Chores