Repository: metal3-io/baremetal-operator
Pull request #1007 (2021-10-21 20:13:42)

Remove bmc package dependency on API
We'll want to use this package in validation (which must be defined in
the API), so don't have it depend on the API. Instead, define a local
structure for the FirmwareConfig data.

s3rj1k: shouldn't we move `type FirmwareConfig struct {` to separate package?
something like `pkg/ironic/bmc/api/`?
zaneb: > shouldn't we move `type FirmwareConfig struct {` to separate package? something like `pkg/ironic/bmc/api/`?

No, why?
ardaguclu: /test-v1b1-integration

Patch:
diff --git a/pkg/ironic/bmc/access.go b/pkg/ironic/bmc/access.go
index 8d3d22c0..751fcf8f 100644
--- a/pkg/ironic/bmc/access.go
+++ b/pkg/ironic/bmc/access.go
@@ -7,8 +7,6 @@ import (
 	"strings"
 
 	"github.com/pkg/errors"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 // AccessDetailsFactory describes a callable that returns a new
@@ -30,6 +28,17 @@ func RegisterFactory(name string, factory AccessDetailsFactory, schemes []string
 	}
 }
 
+type FirmwareConfig struct {
+	// Supports the virtualization of platform hardware.
+	VirtualizationEnabled *bool
+
+	// Allows a single physical processor core to appear as several logical processors.
+	SimultaneousMultithreadingEnabled *bool
+
+	// SR-IOV support enables a hypervisor to create virtual instances of a PCI-express device, potentially increasing performance.
+	SriovEnabled *bool
+}
+
 // AccessDetails contains the information about how to get to a BMC.
 //
 // NOTE(dhellmann): This structure is very likely to change as we
@@ -74,7 +83,7 @@ type AccessDetails interface {
 	RequiresProvisioningNetwork() bool
 
 	// Build bios clean steps for ironic
-	BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error)
+	BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error)
 }
 
 func getParsedURL(address string) (parsedURL *url.URL, err error) {
diff --git a/pkg/ironic/bmc/ibmc.go b/pkg/ironic/bmc/ibmc.go
index aebf6012..ba8185b3 100644
--- a/pkg/ironic/bmc/ibmc.go
+++ b/pkg/ironic/bmc/ibmc.go
@@ -4,8 +4,6 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -117,7 +115,7 @@ func (a *ibmcAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *ibmcAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *ibmcAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
diff --git a/pkg/ironic/bmc/idrac.go b/pkg/ironic/bmc/idrac.go
index 71b5e665..b7b8cb84 100644
--- a/pkg/ironic/bmc/idrac.go
+++ b/pkg/ironic/bmc/idrac.go
@@ -3,8 +3,6 @@ package bmc
 import (
 	"net/url"
 	"strings"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -114,7 +112,7 @@ func (a *iDracAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *iDracAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *iDracAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig == nil {
 		return nil, nil
 	}
diff --git a/pkg/ironic/bmc/idrac_virtualmedia.go b/pkg/ironic/bmc/idrac_virtualmedia.go
index 8aae4f60..7ae916d8 100644
--- a/pkg/ironic/bmc/idrac_virtualmedia.go
+++ b/pkg/ironic/bmc/idrac_virtualmedia.go
@@ -3,8 +3,6 @@ package bmc
 import (
 	"fmt"
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -107,7 +105,7 @@ func (a *redfishiDracVirtualMediaAccessDetails) RequiresProvisioningNetwork() bo
 	return false
 }
 
-func (a *redfishiDracVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *redfishiDracVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
diff --git a/pkg/ironic/bmc/ilo4.go b/pkg/ironic/bmc/ilo4.go
index 850267c9..6dffa10f 100644
--- a/pkg/ironic/bmc/ilo4.go
+++ b/pkg/ironic/bmc/ilo4.go
@@ -4,8 +4,6 @@ package bmc
 
 import (
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -108,7 +106,7 @@ func (a *iLOAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *iLOAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *iLOAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig == nil {
 		return nil, nil
 	}
diff --git a/pkg/ironic/bmc/ilo5.go b/pkg/ironic/bmc/ilo5.go
index 7ded4fcb..2cfbee1f 100644
--- a/pkg/ironic/bmc/ilo5.go
+++ b/pkg/ironic/bmc/ilo5.go
@@ -4,8 +4,6 @@ package bmc
 
 import (
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -108,7 +106,7 @@ func (a *iLO5AccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *iLO5AccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *iLO5AccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig == nil {
 		return nil, nil
 	}
diff --git a/pkg/ironic/bmc/ipmi.go b/pkg/ironic/bmc/ipmi.go
index 90e935c9..c4a29d00 100644
--- a/pkg/ironic/bmc/ipmi.go
+++ b/pkg/ironic/bmc/ipmi.go
@@ -3,8 +3,6 @@ package bmc
 import (
 	"fmt"
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -127,7 +125,7 @@ func (a *ipmiAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *ipmiAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *ipmiAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
diff --git a/pkg/ironic/bmc/irmc.go b/pkg/ironic/bmc/irmc.go
index a17e8f40..c6b12f83 100644
--- a/pkg/ironic/bmc/irmc.go
+++ b/pkg/ironic/bmc/irmc.go
@@ -2,8 +2,6 @@ package bmc
 
 import (
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -106,7 +104,7 @@ func (a *iRMCAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *iRMCAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *iRMCAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig == nil {
 		return nil, nil
 	}
diff --git a/pkg/ironic/bmc/redfish.go b/pkg/ironic/bmc/redfish.go
index eeb3748f..c2d78c33 100644
--- a/pkg/ironic/bmc/redfish.go
+++ b/pkg/ironic/bmc/redfish.go
@@ -4,8 +4,6 @@ import (
 	"fmt"
 	"net/url"
 	"strings"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -137,7 +135,7 @@ func (a *redfishAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *redfishAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *redfishAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
@@ -174,7 +172,7 @@ func (a *redfishiDracAccessDetails) VendorInterface() string {
 	return "idrac-redfish"
 }
 
-func (a *redfishiDracAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *redfishiDracAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
diff --git a/pkg/ironic/bmc/redfish_virtualmedia.go b/pkg/ironic/bmc/redfish_virtualmedia.go
index 0c8e71eb..bd319d17 100644
--- a/pkg/ironic/bmc/redfish_virtualmedia.go
+++ b/pkg/ironic/bmc/redfish_virtualmedia.go
@@ -3,8 +3,6 @@ package bmc
 import (
 	"fmt"
 	"net/url"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -105,7 +103,7 @@ func (a *redfishVirtualMediaAccessDetails) RequiresProvisioningNetwork() bool {
 	return false
 }
 
-func (a *redfishVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *redfishVirtualMediaAccessDetails) BuildBIOSSettings(firmwareConfig *FirmwareConfig) (settings []map[string]string, err error) {
 	if firmwareConfig != nil {
 		return nil, fmt.Errorf("firmware settings for %s are not supported", a.Driver())
 	}
diff --git a/pkg/ironic/go.mod b/pkg/ironic/go.mod
index ceb7a707..128d9601 100644
--- a/pkg/ironic/go.mod
+++ b/pkg/ironic/go.mod
@@ -9,7 +9,6 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/stretchr/testify v1.7.0
 	go.etcd.io/etcd v0.5.0-alpha.5.0.20200910180754-dd1b699fc489
-	sigs.k8s.io/controller-runtime v0.9.7
 )
 
 replace github.com/metal3-io/baremetal-operator/apis => ../../apis
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index 3be305c9..4431701d 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1156,7 +1156,12 @@ func (p *ironicProvisioner) buildManualCleaningSteps(bmcAccess bmc.AccessDetails
 	cleanSteps = append(cleanSteps, raidCleanSteps...)
 
 	// Get the subset (currently 3) of vendor specific BIOS settings converted from common names
-	bmcsettings, err := bmcAccess.BuildBIOSSettings(data.FirmwareConfig)
+	var firmwareConfig *bmc.FirmwareConfig
+	if data.FirmwareConfig != nil {
+		bmcConfig := bmc.FirmwareConfig(*data.FirmwareConfig)
+		firmwareConfig = &bmcConfig
+	}
+	bmcsettings, err := bmcAccess.BuildBIOSSettings(firmwareConfig)
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/pkg/ironic/bmc/access_test.go b/pkg/ironic/bmc/access_test.go
index f4df5c6b..cc67f4f7 100644
--- a/pkg/ironic/bmc/access_test.go
+++ b/pkg/ironic/bmc/access_test.go
@@ -3,17 +3,8 @@ package bmc
 import (
 	"reflect"
 	"testing"
-
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	logz "sigs.k8s.io/controller-runtime/pkg/log/zap"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
-func init() {
-	logf.SetLogger(logz.New(logz.UseDevMode(true)))
-}
-
 func TestParse(t *testing.T) {
 	for _, tc := range []struct {
 		Scenario    string
@@ -1162,7 +1153,7 @@ func TestBuildBIOSCleanSteps(t *testing.T) {
 	cases := []struct {
 		name          string
 		address       string
-		firmware      *metal3v1alpha1.FirmwareConfig
+		firmware      *FirmwareConfig
 		expected      []map[string]string
 		expectedError bool
 	}{
@@ -1170,7 +1161,7 @@ func TestBuildBIOSCleanSteps(t *testing.T) {
 		{
 			name:    "ilo4",
 			address: "ilo4://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{
+			firmware: &FirmwareConfig{
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
@@ -1194,14 +1185,14 @@ func TestBuildBIOSCleanSteps(t *testing.T) {
 		{
 			name:     "ilo4, firmware is empty",
 			address:  "ilo4://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{},
+			firmware: &FirmwareConfig{},
 			expected: nil,
 		},
 		// ilo5
 		{
 			name:    "ilo5",
 			address: "ilo5://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{
+			firmware: &FirmwareConfig{
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
@@ -1225,14 +1216,14 @@ func TestBuildBIOSCleanSteps(t *testing.T) {
 		{
 			name:     "ilo5, firmware is empty",
 			address:  "ilo5://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{},
+			firmware: &FirmwareConfig{},
 			expected: nil,
 		},
 		// irmc
 		{
 			name:    "irmc",
 			address: "irmc://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{
+			firmware: &FirmwareConfig{
 				VirtualizationEnabled:             &True,
 				SimultaneousMultithreadingEnabled: &False,
 			},
@@ -1256,7 +1247,7 @@ func TestBuildBIOSCleanSteps(t *testing.T) {
 		{
 			name:     "irmc, firmware is empty",
 			address:  "irmc://192.168.122.1",
-			firmware: &metal3v1alpha1.FirmwareConfig{},
+			firmware: &FirmwareConfig{},
 			expected: nil,
 		},
 	}
diff --git a/pkg/ironic/bmc/credentials_test.go b/pkg/ironic/bmc/credentials_test.go
index 3377c3ea..02cf2ef0 100644
--- a/pkg/ironic/bmc/credentials_test.go
+++ b/pkg/ironic/bmc/credentials_test.go
@@ -2,15 +2,8 @@ package bmc
 
 import (
 	"testing"
-
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	logz "sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
-func init() {
-	logf.SetLogger(logz.New(logz.UseDevMode(true)))
-}
-
 func TestValidCredentials(t *testing.T) {
 	creds := Credentials{
 		Username: "username",
diff --git a/pkg/ironic/testbmc/testbmc.go b/pkg/ironic/testbmc/testbmc.go
index b51fa885..996e510c 100644
--- a/pkg/ironic/testbmc/testbmc.go
+++ b/pkg/ironic/testbmc/testbmc.go
@@ -4,8 +4,6 @@ import (
 	"net/url"
 
 	"github.com/metal3-io/baremetal-operator/pkg/ironic/bmc"
-
-	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 )
 
 func init() {
@@ -100,7 +98,7 @@ func (a *testAccessDetails) RequiresProvisioningNetwork() bool {
 	return true
 }
 
-func (a *testAccessDetails) BuildBIOSSettings(firmwareConfig *metal3v1alpha1.FirmwareConfig) (settings []map[string]string, err error) {
+func (a *testAccessDetails) BuildBIOSSettings(firmwareConfig *bmc.FirmwareConfig) (settings []map[string]string, err error) {
 
 	// Return sample BMC data for test purposes
 	if firmwareConfig == nil {
diff --git a/pkg/provisioner/ironic/prepare_test.go b/pkg/provisioner/ironic/prepare_test.go
index aaacf3d6..88b9074d 100644
--- a/pkg/provisioner/ironic/prepare_test.go
+++ b/pkg/provisioner/ironic/prepare_test.go
@@ -33,7 +33,7 @@ func (r *RAIDTestBMC) RAIDInterface() string                                 { r
 func (r *RAIDTestBMC) VendorInterface() string                               { return "" }
 func (r *RAIDTestBMC) SupportsSecureBoot() bool                              { return false }
 func (r *RAIDTestBMC) RequiresProvisioningNetwork() bool                     { return true }
-func (r *RAIDTestBMC) BuildBIOSSettings(fwConf *metal3v1alpha1.FirmwareConfig) ([]map[string]string, error) {
+func (r *RAIDTestBMC) BuildBIOSSettings(fwConf *bmc.FirmwareConfig) ([]map[string]string, error) {
 	return nil, nil
 }