adjustments from pr review

Signed-off-by: Mike Mason <[email protected]>

installers/custom_ipxe/main.go

@@ -1,13 +1,13 @@
 package custom_ipxe
 
 import (
-	"encoding/json"
 	"strings"
 
 	"github.com/packethost/pkg/log"
 	"github.com/tinkerbell/boots/installers"
 	"github.com/tinkerbell/boots/ipxe"
 	"github.com/tinkerbell/boots/job"
+	"github.com/tinkerbell/boots/packet"
 )
 
 func init() {
@@ -16,39 +16,37 @@ 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())
 	}
 
-	var cfg *Config
+	var cfg *packet.InstallerData
 	var err error
 
 	if j.OperatingSystem().Installer == "ipxe" {
-		cfg, err = ipxeConfigFromJob(j)
-		if err != nil {
-			s.Echo("Failed to decode installer data")
+		cfg = j.OperatingSystem().InstallerData
+		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 = &packet.InstallerData{Script: j.UserData()}
 		} else {
-			cfg.Chain = j.IPXEScriptURL()
+			cfg = &packet.InstallerData{Chain: j.IPXEScriptURL()}
 		}
 	}
 
 	IpxeScriptFromConfig(logger, cfg, j, s)
 }
 
-func IpxeScriptFromConfig(logger log.Logger, cfg *Config, j job.Job, s *ipxe.Script) {
-	if err := cfg.validate(); err != nil {
-		s.Echo("Invalid ipxe configuration")
+func IpxeScriptFromConfig(logger log.Logger, cfg *packet.InstallerData, j job.Job, s *ipxe.Script) {
+	if err := validateConfig(cfg); err != nil {
+		s.Echo(err.Error())
 		s.Shell()
 		logger.Error(err, "validating ipxe config")
 
@@ -63,31 +61,10 @@ func IpxeScriptFromConfig(logger log.Logger, cfg *Config, j job.Job, s *ipxe.Scr
 		s.Chain(cfg.Chain)
 	} else if cfg.Script != "" {
 		s.AppendString(strings.TrimPrefix(cfg.Script, "#!ipxe"))
-	} else {
-		s.Echo("Unknown ipxe config path")
-		s.Shell()
 	}
 }
 
-func ipxeConfigFromJob(j job.Job) (*Config, error) {
-	data := j.OperatingSystem().InstallerData
-
-	cfg := &Config{}
-
-	err := json.NewDecoder(strings.NewReader(data)).Decode(&cfg)
-	if err != nil {
-		return nil, err
-	}
-
-	return cfg, nil
-}
-
-type Config struct {
-	Chain  string `json:"chain,omitempty"`
-	Script string `json:"script,omitempty"`
-}
-
-func (c *Config) validate() error {
+func validateConfig(c *packet.InstallerData) error {
 	if c.Chain == "" && c.Script == "" {
 		return ErrEmptyIpxeConfig
 	}

installers/custom_ipxe/main_test.go

@@ -10,6 +10,7 @@ import (
 	"github.com/tinkerbell/boots/installers"
 	"github.com/tinkerbell/boots/ipxe"
 	"github.com/tinkerbell/boots/job"
+	"github.com/tinkerbell/boots/packet"
 )
 
 var (
@@ -32,21 +33,21 @@ func TestMain(m *testing.M) {
 func TestIpxeScript(t *testing.T) {
 	var testCases = []struct {
 		name          string
-		installerData string
+		installerData *packet.InstallerData
 		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"}`,
+			&packet.InstallerData{Chain: "http://url/path.ipxe"},
 			`#!ipxe
 
 
@@ -82,21 +83,21 @@ func TestIpxeScript(t *testing.T) {
 func TestIpxeScriptFromConfig(t *testing.T) {
 	var testCases = []struct {
 		name   string
-		config *Config
+		config *packet.InstallerData
 		want   string
 	}{
 		{
 			"invalid config",
-			&Config{},
+			&packet.InstallerData{},
 			`#!ipxe
 
-			echo Invalid ipxe configuration
+			echo ipxe config URL or Script must be defined
 			shell
 			`,
 		},
 		{
 			"valid chain",
-			&Config{Chain: "http://url/path.ipxe"},
+			&packet.InstallerData{Chain: "http://url/path.ipxe"},
 			`#!ipxe
 
 
@@ -113,7 +114,7 @@ func TestIpxeScriptFromConfig(t *testing.T) {
 		},
 		{
 			"valid script",
-			&Config{Script: "echo my test script"},
+			&packet.InstallerData{Script: "echo my test script"},
 			`#!ipxe
 
 
@@ -130,7 +131,7 @@ func TestIpxeScriptFromConfig(t *testing.T) {
 		},
 		{
 			"valid script with header",
-			&Config{Script: "#!ipxe\necho my test script"},
+			&packet.InstallerData{Script: "#!ipxe\necho my test script"},
 			`#!ipxe
 
 
@@ -161,60 +162,6 @@ func TestIpxeScriptFromConfig(t *testing.T) {
 	}
 }
 
-func TestIpxeConfigFromJob(t *testing.T) {
-	var testCases = []struct {
-		name          string
-		installerData string
-		want          *Config
-		expectError   string
-	}{
-		{
-			"valid chain",
-			`{"chain": "http://url/path.ipxe"}`,
-			&Config{Chain: "http://url/path.ipxe"},
-			"",
-		},
-		{
-			"valid script",
-			`{"script": "echo script"}`,
-			&Config{Script: "echo script"},
-			"",
-		},
-		{
-			"empty json error",
-			``,
-			nil,
-			"EOF",
-		},
-		{
-			"invalid json error",
-			`{"error"`,
-			nil,
-			"unexpected EOF",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			assert := require.New(t)
-
-			mockJob := job.NewMock(t, "test.slug", "test.facility")
-
-			mockJob.SetOSInstallerData(tc.installerData)
-
-			cfg, err := ipxeConfigFromJob(mockJob.Job())
-
-			if tc.expectError == "" {
-				assert.Nil(err)
-			} else {
-				assert.EqualError(err, tc.expectError)
-			}
-
-			assert.Equal(tc.want, cfg)
-		})
-	}
-}
-
 func TestConfigValidate(t *testing.T) {
 	var testCases = []struct {
 		name   string
@@ -232,12 +179,12 @@ func TestConfigValidate(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			assert := require.New(t)
 
-			cfg := &Config{
+			cfg := &packet.InstallerData{
 				Chain:  tc.chain,
 				Script: tc.script,
 			}
 
-			got := cfg.validate()
+			got := validateConfig(cfg)
 
 			if tc.want == "" {
 				assert.Nil(got)

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 *packet.InstallerData) {
 	m.hardware.OperatingSystem().InstallerData = installerData
 }
 

packet/models.go

@@ -221,13 +221,19 @@ 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 *InstallerData `json:"installer_data,omitempty"`
+}
+
+// InstallerData holds a number of fields that may be used by an installer.
+type InstallerData struct {
+	Chain  string `json:"chain,omitempty"`
+	Script string `json:"script,omitempty"`
 }
 
 // Port represents a network port