Implement changes based on feedback from GH PR#261

bmc/firmware.go

@@ -7,15 +7,23 @@ import (
 
 	bmclibErrs "github.com/bmc-toolbox/bmclib/errors"
 
-	"github.com/pkg/errors"
-
 	"github.com/hashicorp/go-multierror"
+	"github.com/pkg/errors"
 )
 
 // FirmwareInstaller defines an interface to install firmware updates
 type FirmwareInstaller interface {
 	// FirmwareInstall uploads firmware update payload to the BMC returning the task ID
-	FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (jobID string, err error)
+	//
+	// parameters:
+	// component - the component slug for the component update being installed.
+	// applyAt - one of "Immediate", "OnReset".
+	// forceInstall - purge the install task queued/scheduled firmware install BMC task (if any).
+	// reader - the io.reader to the firmware update file.
+	//
+	// return values:
+	// taskID - A taskID is returned if the update process on the BMC returns an identifier for the update process.
+	FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (taskID string, err error)
 }
 
 // firmwareInstallerProvider is an internal struct to correlate an implementation/provider and its name
@@ -24,17 +32,8 @@ type firmwareInstallerProvider struct {
 	FirmwareInstaller
 }
 
-// FirmwareInstall uploads and initiates firmware update for the component
-//
-// parameters:
-// component - the component slug for the component update being installed
-// applyAt - one of "Immediate", "OnReset"
-// forceInstall - purge the install task queued/scheduled firmware install BMC task (if any)
-// reader - the io.reader to the firmware update file
-//
-// return values:
-// taskID - A taskID is returned if the update process on the BMC returns an identifier for the update process
-func FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader, generic []firmwareInstallerProvider) (taskID string, metadata Metadata, err error) {
+// firmwareInstall uploads and initiates firmware update for the component
+func firmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader, generic []firmwareInstallerProvider) (taskID string, metadata Metadata, err error) {
 	var metadataLocal Metadata
 Loop:
 	for _, elem := range generic {
@@ -86,11 +85,20 @@ func FirmwareInstallFromInterfaces(ctx context.Context, component, applyAt strin
 		)
 	}
 
-	return FirmwareInstall(ctx, component, applyAt, forceInstall, reader, implementations)
+	return firmwareInstall(ctx, component, applyAt, forceInstall, reader, implementations)
 }
 
 // FirmwareInstallVerifier defines an interface to check firmware install status
 type FirmwareInstallVerifier interface {
+	// FirmwareInstallStatus returns the status of the firmware install process.
+	//
+	// parameters:
+	// component (optional) - the component slug for the component update being installed.
+	// installVersion (required) - the version this method should check is installed.
+	// taskID (optional) - the task identifier.
+	//
+	// return values:
+	// status - returns one of the FirmwareInstall statuses (see devices/constants.go).
 	FirmwareInstallStatus(ctx context.Context, component, installVersion, taskID string) (status string, err error)
 }
 
@@ -100,15 +108,8 @@ type firmwareInstallVerifierProvider struct {
 	FirmwareInstallVerifier
 }
 
-// FirmwareInstallStatus returns the status of the firmware install process
-//
-// parameters:
-// component (optional) - the component slug for the component update being installed
-// taskID (optional) - the task identifier
-//
-// return values:
-// status - returns one of the FirmwareInstall statuses (see devices/constants.go)
-func FirmwareInstallStatus(ctx context.Context, component, installVersion, taskID string, generic []firmwareInstallVerifierProvider) (status string, metadata Metadata, err error) {
+// firmwareInstallStatus returns the status of the firmware install process
+func firmwareInstallStatus(ctx context.Context, component, installVersion, taskID string, generic []firmwareInstallVerifierProvider) (status string, metadata Metadata, err error) {
 	var metadataLocal Metadata
 Loop:
 	for _, elem := range generic {
@@ -160,5 +161,5 @@ func FirmwareInstallStatusFromInterfaces(ctx context.Context, component, install
 		)
 	}
 
-	return FirmwareInstallStatus(ctx, component, installVersion, taskID, implementations)
+	return firmwareInstallStatus(ctx, component, installVersion, taskID, implementations)
 }

bmc/firmware_test.go

@@ -51,7 +51,7 @@ func TestFirmwareInstall(t *testing.T) {
 			}
 			ctx, cancel := context.WithTimeout(context.Background(), tc.ctxTimeout)
 			defer cancel()
-			taskID, metadata, err := FirmwareInstall(ctx, tc.component, tc.applyAt, tc.forceInstall, tc.reader, []firmwareInstallerProvider{{tc.providerName, &testImplementation}})
+			taskID, metadata, err := firmwareInstall(ctx, tc.component, tc.applyAt, tc.forceInstall, tc.reader, []firmwareInstallerProvider{{tc.providerName, &testImplementation}})
 			if tc.returnError != nil {
 				assert.ErrorIs(t, err, tc.returnError)
 				return
@@ -146,7 +146,7 @@ func TestFirmwareInstallStatus(t *testing.T) {
 			}
 			ctx, cancel := context.WithTimeout(context.Background(), tc.ctxTimeout)
 			defer cancel()
-			taskID, metadata, err := FirmwareInstallStatus(ctx, tc.component, tc.installVersion, tc.taskID, []firmwareInstallVerifierProvider{{tc.providerName, &testImplementation}})
+			taskID, metadata, err := firmwareInstallStatus(ctx, tc.component, tc.installVersion, tc.taskID, []firmwareInstallVerifierProvider{{tc.providerName, &testImplementation}})
 			if tc.returnError != nil {
 				assert.ErrorIs(t, err, tc.returnError)
 				return

bmc/inventory.go

@@ -21,8 +21,8 @@ type inventoryGetterProvider struct {
 	InventoryGetter
 }
 
-// Inventory returns hardware and firmware inventory
-func Inventory(ctx context.Context, generic []inventoryGetterProvider) (device *devices.Device, metadata Metadata, err error) {
+// inventory returns hardware and firmware inventory
+func inventory(ctx context.Context, generic []inventoryGetterProvider) (device *devices.Device, metadata Metadata, err error) {
 	var metadataLocal Metadata
 Loop:
 	for _, elem := range generic {
@@ -74,5 +74,5 @@ func GetInventoryFromInterfaces(ctx context.Context, generic []interface{}) (dev
 		)
 	}
 
-	return Inventory(ctx, implementations)
+	return inventory(ctx, implementations)
 }

bmc/inventory_test.go

@@ -46,7 +46,7 @@ func TestInventory(t *testing.T) {
 			}
 			ctx, cancel := context.WithTimeout(context.Background(), tc.ctxTimeout)
 			defer cancel()
-			device, metadata, err := Inventory(ctx, []inventoryGetterProvider{{tc.providerName, &testImplementation}})
+			device, metadata, err := inventory(ctx, []inventoryGetterProvider{{tc.providerName, &testImplementation}})
 			if tc.returnError != nil {
 				assert.ErrorIs(t, err, tc.returnError)
 				return

bmc/postcode.go

@@ -23,8 +23,8 @@ type postCodeGetterProvider struct {
 	PostCodeGetter
 }
 
-// PostCode returns the device BIOS/UEFI POST code
-func PostCode(ctx context.Context, generic []postCodeGetterProvider) (status string, code int, metadata Metadata, err error) {
+// postCode returns the device BIOS/UEFI POST code
+func postCode(ctx context.Context, generic []postCodeGetterProvider) (status string, code int, metadata Metadata, err error) {
 	var metadataLocal Metadata
 Loop:
 	for _, elem := range generic {
@@ -76,5 +76,5 @@ func GetPostCodeInterfaces(ctx context.Context, generic []interface{}) (status s
 		)
 	}
 
-	return PostCode(ctx, implementations)
+	return postCode(ctx, implementations)
 }

bmc/postcode_test.go

@@ -47,7 +47,7 @@ func TestPostCode(t *testing.T) {
 			}
 			ctx, cancel := context.WithTimeout(context.Background(), tc.ctxTimeout)
 			defer cancel()
-			status, code, metadata, err := PostCode(ctx, []postCodeGetterProvider{{tc.providerName, &testImplementation}})
+			status, code, metadata, err := postCode(ctx, []postCodeGetterProvider{{tc.providerName, &testImplementation}})
 			if tc.returnError != nil {
 				assert.ErrorIs(t, err, tc.returnError)
 				return

providers/asrockrack/firmware.go

@@ -14,8 +14,6 @@ import (
 	bmclibErrs "github.com/bmc-toolbox/bmclib/errors"
 )
 
-
-
 // FirmwareInstall uploads and initiates firmware update for the component
 func (a *ASRockRack) FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (jobID string, err error) {
 	var size int64
@@ -118,10 +116,7 @@ func (a *ASRockRack) firmwareInstallBIOS(ctx context.Context, reader io.Reader,
 
 // firmwareUpdateBMCStatus returns the BMC firmware install status
 func (a *ASRockRack) firmwareUpdateBMCStatus(ctx context.Context, installVersion string) (status string, err error) {
-	os.Setenv("BMCLIB_LOG_LEVEL", "trace")
-	defer os.Unsetenv("BMCLIB_LOG_LEVEL")
-	endpoint := "api/maintenance/firmware/flash-progress"
-	p, progressErr := a.flashProgress(ctx, endpoint)
+	p, progressErr := a.flashProgress(ctx, "api/maintenance/firmware/flash-progress")
 	if progressErr != nil {
 		installed, versionErr := a.versionInstalled(ctx, devices.SlugBMC, installVersion)
 		if err != nil {
@@ -160,11 +155,7 @@ func (a *ASRockRack) firmwareUpdateBMCStatus(ctx context.Context, installVersion
 
 // firmwareUpdateBIOSStatus returns the BIOS firmware install status
 func (a *ASRockRack) firmwareUpdateBIOSStatus(ctx context.Context, installVersion string) (status string, err error) {
-	os.Setenv("BMCLIB_LOG_LEVEL", "trace")
-	defer os.Unsetenv("BMCLIB_LOG_LEVEL")
-
-	endpoint := "api/asrr/maintenance/BIOS/flash-progress"
-	p, progressErr := a.flashProgress(ctx, endpoint)
+	p, progressErr := a.flashProgress(ctx, "api/asrr/maintenance/BIOS/flash-progress")
 	if progressErr != nil {
 		installed, versionErr := a.versionInstalled(ctx, devices.SlugBIOS, installVersion)
 		if versionErr != nil {
@@ -223,7 +214,6 @@ func (a *ASRockRack) versionInstalled(ctx context.Context, component, version st
 	}
 
 	if strings.EqualFold(installed, version) {
-		fmt.Println("YEP!")
 		return true, nil
 	}
 

providers/redfish/firmware.go

@@ -3,6 +3,7 @@ package redfish
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 	"mime/multipart"
@@ -20,25 +21,25 @@ import (
 	bmclibErrs "github.com/bmc-toolbox/bmclib/errors"
 )
 
-var (
-	FirmwareApplyAt = []string{
+// SupportedFirmwareApplyAtValues returns the supported redfish firmware applyAt values
+func SupportedFirmwareApplyAtValues() []string {
+	return []string{
 		devices.FirmwareApplyImmediate,
 		devices.FirmwareApplyOnReset,
 	}
-	FirmwareDownloadProtocols = []string{"HTTP", "NFS", "CIFS", "TFTP", "HTTPS"}
-)
+}
 
 // FirmwareInstall uploads and initiates the firmware install process
-func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (jobID string, err error) {
+func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (taskID string, err error) {
 	// validate firmware update mechanism is supported
 	err = c.firmwareUpdateCompatible(ctx)
 	if err != nil {
 		return "", err
 	}
 
 	// validate applyAt parameter
-	if !stringInSlice(applyAt, FirmwareApplyAt) {
-		return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, "invalid applyAt parameter: %s"+applyAt)
+	if !stringInSlice(applyAt, SupportedFirmwareApplyAtValues()) {
+		return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, "invalid applyAt parameter: "+applyAt)
 	}
 
 	// list redfish firmware install task if theres one present
@@ -61,9 +62,19 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f
 		}
 	}
 
-	updateParameters := []byte(
-		fmt.Sprintf(`{"Targets": [], "@Redfish.OperationApplyTime": "%s", "Oem": {}}`, applyAt),
-	)
+	updateParameters, err := json.Marshal(struct {
+		Targets            []string `json:"Targets"`
+		RedfishOpApplyTime string   `json:"@Redfish.OperationApplyTime"`
+		Oem                struct{} `json:"Oem"`
+	}{
+		[]string{},
+		applyAt,
+		struct{}{},
+	})
+
+	if err != nil {
+		return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, err.Error())
+	}
 
 	payload := map[string]io.Reader{
 		"UpdateParameters": bytes.NewReader(updateParameters),
@@ -72,11 +83,11 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f
 
 	resp, err := c.runRequestWithMultipartPayload(http.MethodPost, "/redfish/v1/UpdateService/MultipartUpload", payload)
 	if err != nil {
-		return jobID, errors.Wrap(bmclibErrs.ErrFirmwareUpload, err.Error())
+		return "", errors.Wrap(bmclibErrs.ErrFirmwareUpload, err.Error())
 	}
 
 	if resp.StatusCode != http.StatusAccepted {
-		return jobID, errors.Wrap(
+		return "", errors.Wrap(
 			bmclibErrs.ErrFirmwareUpload,
 			"non 202 status code returned: "+strconv.Itoa(resp.StatusCode),
 		)
@@ -85,10 +96,10 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f
 	// The response contains a location header pointing to the task URI
 	// Location: /redfish/v1/TaskService/Tasks/JID_467696020275
 	if strings.Contains(resp.Header.Get("Location"), "JID_") {
-		jobID = strings.Split(resp.Header.Get("Location"), "JID_")[1]
+		taskID = strings.Split(resp.Header.Get("Location"), "JID_")[1]
 	}
 
-	return jobID, nil
+	return taskID, nil
 }
 
 // FirmwareInstallStatus returns the status of the firmware install task queued