adjustments from pr review

Signed-off-by: Mike Mason <[email protected]>
tinkerbell · Aug 11, 2021 · 2216b3d · 2216b3d
1 parent 19bd269
commit 2216b3d
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 52 deletions.
diff --git a/installers/custom_ipxe/main.go b/installers/custom_ipxe/main.go
@@ -1,7 +1,6 @@
 package custom_ipxe
 
 import (
-	"encoding/json"
 	"strings"
 
 	"github.com/packethost/pkg/log"
@@ -16,7 +15,7 @@ func init() {
 }
 
 func ipxeScript(j job.Job, s *ipxe.Script) {
-	logger := installers.Logger("ipxe")
+	logger := installers.Logger("custom_ipxe")
 	if j.InstanceID() != "" {
 		logger = logger.With("instance.id", j.InstanceID())
 	}
@@ -25,21 +24,19 @@ func ipxeScript(j job.Job, s *ipxe.Script) {
 	var err error
 
 	if j.OperatingSystem().Installer == "ipxe" {
-		cfg, err = ipxeConfigFromJob(j)
-		if err != nil {
-			s.Echo("Failed to decode installer data")
+		cfg = ipxeConfigFromJob(j)
+		if cfg == nil {
+			s.Echo("Installer data not provided")
 			s.Shell()
-			logger.Error(err, "decoding installer data")
+			logger.Error(err, "empty installer data")
 
 			return
 		}
 	} else {
-		cfg = &Config{}
-
 		if strings.HasPrefix(j.UserData(), "#!ipxe") {
-			cfg.Script = j.UserData()
+			cfg = &Config{Script: j.UserData()}
 		} else {
-			cfg.Chain = j.IPXEScriptURL()
+			cfg = &Config{Chain: j.IPXEScriptURL()}
 		}
 	}
 
@@ -48,7 +45,7 @@ func ipxeScript(j job.Job, s *ipxe.Script) {
 
 func IpxeScriptFromConfig(logger log.Logger, cfg *Config, j job.Job, s *ipxe.Script) {
 	if err := cfg.validate(); err != nil {
-		s.Echo("Invalid ipxe configuration")
+		s.Echo(err.Error())
 		s.Shell()
 		logger.Error(err, "validating ipxe config")
 
@@ -69,17 +66,17 @@ func IpxeScriptFromConfig(logger log.Logger, cfg *Config, j job.Job, s *ipxe.Scr
 	}
 }
 
-func ipxeConfigFromJob(j job.Job) (*Config, error) {
+func ipxeConfigFromJob(j job.Job) *Config {
 	data := j.OperatingSystem().InstallerData
 
-	cfg := &Config{}
-
-	err := json.NewDecoder(strings.NewReader(data)).Decode(&cfg)
-	if err != nil {
-		return nil, err
+	if data == nil {
+		return nil
 	}
 
-	return cfg, nil
+	return &Config{
+		Chain:  data["chain"],
+		Script: data["script"],
+	}
 }
 
 type Config struct {

diff --git a/installers/custom_ipxe/main_test.go b/installers/custom_ipxe/main_test.go
@@ -32,21 +32,21 @@ func TestMain(m *testing.M) {
 func TestIpxeScript(t *testing.T) {
 	var testCases = []struct {
 		name          string
-		installerData string
+		installerData map[string]string
 		want          string
 	}{
 		{
 			"invalid config",
-			"",
+			nil,
 			`#!ipxe
 
-			echo Failed to decode installer data
+			echo Installer data not provided
 			shell
 			`,
 		},
 		{
 			"valid config",
-			`{"chain": "http://url/path.ipxe"}`,
+			map[string]string{"chain": "http://url/path.ipxe"},
 			`#!ipxe
 
 
@@ -90,7 +90,7 @@ func TestIpxeScriptFromConfig(t *testing.T) {
 			&Config{},
 			`#!ipxe
 
-			echo Invalid ipxe configuration
+			echo ipxe config URL or Script must be defined
 			shell
 			`,
 		},
@@ -164,33 +164,23 @@ func TestIpxeScriptFromConfig(t *testing.T) {
 func TestIpxeConfigFromJob(t *testing.T) {
 	var testCases = []struct {
 		name          string
-		installerData string
+		installerData map[string]string
 		want          *Config
-		expectError   string
 	}{
 		{
 			"valid chain",
-			`{"chain": "http://url/path.ipxe"}`,
+			map[string]string{"chain": "http://url/path.ipxe"},
 			&Config{Chain: "http://url/path.ipxe"},
-			"",
 		},
 		{
 			"valid script",
-			`{"script": "echo script"}`,
+			map[string]string{"script": "echo script"},
 			&Config{Script: "echo script"},
-			"",
 		},
 		{
-			"empty json error",
-			``,
+			"empty config",
 			nil,
-			"EOF",
-		},
-		{
-			"invalid json error",
-			`{"error"`,
 			nil,
-			"unexpected EOF",
 		},
 	}
 
@@ -202,13 +192,7 @@ func TestIpxeConfigFromJob(t *testing.T) {
 
 			mockJob.SetOSInstallerData(tc.installerData)
 
-			cfg, err := ipxeConfigFromJob(mockJob.Job())
-
-			if tc.expectError == "" {
-				assert.Nil(err)
-			} else {
-				assert.EqualError(err, tc.expectError)
-			}
+			cfg := ipxeConfigFromJob(mockJob.Job())
 
 			assert.Equal(tc.want, cfg)
 		})

diff --git a/job/mock.go b/job/mock.go
@@ -115,7 +115,7 @@ func (m *Mock) SetOSInstaller(installer string) {
 	m.hardware.OperatingSystem().Installer = installer
 }
 
-func (m *Mock) SetOSInstallerData(installerData string) {
+func (m *Mock) SetOSInstallerData(installerData map[string]string) {
 	m.hardware.OperatingSystem().InstallerData = installerData
 }
 

diff --git a/packet/models.go b/packet/models.go
@@ -221,13 +221,13 @@ type IP struct {
 
 // OperatingSystem holds details for the operating system
 type OperatingSystem struct {
-	Slug          string `json:"slug"`
-	Distro        string `json:"distro"`
-	Version       string `json:"version"`
-	ImageTag      string `json:"image_tag"`
-	OsSlug        string `json:"os_slug"`
-	Installer     string `json:"installer,omitempty"`
-	InstallerData string `json:"installer_data,omitempty"`
+	Slug          string            `json:"slug"`
+	Distro        string            `json:"distro"`
+	Version       string            `json:"version"`
+	ImageTag      string            `json:"image_tag"`
+	OsSlug        string            `json:"os_slug"`
+	Installer     string            `json:"installer,omitempty"`
+	InstallerData map[string]string `json:"installer_data,omitempty"`
 }
 
 // Port represents a network port