Cleanup after new data model (#44)

## Description

<!--- Please describe what this PR is going to change -->

Change from pointers to interface to just an interface for the recent data model split. Took the opportunity to clean up some of the related tests so they would be more correct (when setting env vars) / easier to follow (making TestNewDiscoveryMismatch not panic). 

## Why is this needed

<!--- Link to issue you have raised -->

I think #27 was merged prematurely. Lots of commented out code that should have been deleted, oddness in the pointer-to-interface and some hard to follow tests. This is me polishing some of that up.

Fixes: #

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

`go test ./...`


## How are existing users impacted? What migration steps/scripts do we need?

<!--- Fixes a bug, unblocks installation, removes a component of the stack etc -->
<!--- Requires a DB migration script, etc. -->

No anticipated impact.

## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [x] added unit or e2e tests
- [ ] provided instructions on how to upgrade

go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883
 	github.com/avast/retry-go v2.2.0+incompatible
 	github.com/betawaffle/tftp-go v0.0.0-20160921192434-dc649c1318ff
+	github.com/davecgh/go-spew v1.1.1
 	github.com/gammazero/workerpool v0.0.0-20200311205957-7b00833861c6
 	github.com/golang/groupcache v0.0.0-20190702054246-869f871628b6
 	github.com/google/uuid v1.1.1

installers/vmware/ipxe_script_test.go

@@ -35,7 +35,7 @@ func TestScriptPerType(t *testing.T) {
 
 			want := fmt.Sprintf(script, version)
 			if !strings.Contains(want, version) {
-				t.Fatalf("expected %s to be present in script:%v\n", version, want)
+				t.Fatalf("expected %s to be present in script:%v", version, want)
 			}
 			if want != got {
 				t.Fatalf("%s bad iPXE script:\n%v", typ, diff.LineDiff(want, got))

ipxe/settings.go

@@ -1,7 +1,5 @@
 package ipxe
 
-//import "fmt"
-
 const (
 	AssetTag      = "${asset}"        // Unfilled
 	BoardSerial   = "${board-serial}" // Server Serial Number
@@ -10,15 +8,3 @@ const (
 	ChassisSerial = "${serial}"       // Chassis Serial Number
 	UUID          = "${uuid}"         // 00000000-0000-0000-0000-${mac}
 )
-
-// golangci-lint: unused
-//type smbiosTag struct {
-//	Instance byte
-//	Type     byte
-//	Offset   byte
-//	Len      byte
-//}
-//
-//func (t smbiosTag) String() string {
-//	return fmt.Sprintf("smbios/%d.%d.%d.%d", t.Instance, t.Type, t.Offset, t.Len)
-//}

job/dhcp.go

@@ -69,7 +69,7 @@ func (j Job) configureDHCP(rep, req *dhcp4.Packet) bool {
 }
 
 func (j Job) isPXEAllowed() bool {
-	if (*j.hardware).HardwareAllowPXE(j.mac) {
+	if j.hardware.HardwareAllowPXE(j.mac) {
 		return true
 	}
 	if j.InstanceID() == "" {

job/dhcp_test.go

@@ -58,14 +58,13 @@ func TestSetPXEFilename(t *testing.T) {
 			tt.plan = "0"
 		}
 
-		var hardware packet.Hardware = &packet.HardwareCacher{
-			ID:       "$hardware_id",
-			State:    packet.HardwareState(tt.hState),
-			PlanSlug: "baremetal_" + tt.plan,
-		}
 		j := Job{
-			Logger:   joblog.With("index", i, "hStahe", tt.hState, "id", tt.id, "iState", tt.iState, "slug", tt.slug, "plan", tt.plan, "allowPXE", tt.allowPXE, "packet", tt.packet, "arm", tt.arm, "uefi", tt.uefi, "filename", tt.filename),
-			hardware: &hardware,
+			Logger: joblog.With("index", i, "hStahe", tt.hState, "id", tt.id, "iState", tt.iState, "slug", tt.slug, "plan", tt.plan, "allowPXE", tt.allowPXE, "packet", tt.packet, "arm", tt.arm, "uefi", tt.uefi, "filename", tt.filename),
+			hardware: packet.HardwareCacher{
+				ID:       "$hardware_id",
+				State:    packet.HardwareState(tt.hState),
+				PlanSlug: "baremetal_" + tt.plan,
+			},
 			instance: &packet.Instance{
 				ID:       tt.id,
 				State:    packet.InstanceState(tt.iState),
@@ -99,11 +98,10 @@ func TestAllowPXE(t *testing.T) {
 	} {
 		t.Logf("want=%t, hardware=%t, instance=%t, instance_id=%s",
 			tt.want, tt.hw, tt.instance, tt.iid)
-		var hardware packet.Hardware = packet.HardwareCacher{
-			AllowPXE: tt.hw,
-		}
 		j := Job{
-			hardware: &hardware,
+			hardware: packet.HardwareCacher{
+				AllowPXE: tt.hw,
+			},
 			instance: &packet.Instance{
 				ID:       tt.iid,
 				AllowPXE: tt.instance,

job/events.go

@@ -54,7 +54,7 @@ func (j Job) PostHardwareProblem(slug string) bool {
 		j.With("problem", slug).Error(errors.WithMessage(err, "encoding hardware problem request"))
 		return false
 	}
-	if _, err := client.PostHardwareProblem((*j.hardware).HardwareID(), bytes.NewReader(b)); err != nil {
+	if _, err := client.PostHardwareProblem(j.hardware.HardwareID(), bytes.NewReader(b)); err != nil {
 		j.With("problem", slug).Error(errors.WithMessage(err, "posting hardware problem"))
 		return false
 	}
@@ -94,7 +94,7 @@ func (j Job) phoneHome(body []byte) bool {
 			j.With("state", j.HardwareState()).Info("ignoring hardware phone-home when state is not preinstalling")
 			return false
 		}
-		id = (*j.hardware).HardwareID()
+		id = j.hardware.HardwareID()
 		typ = "hardware"
 		post = p.postHardware
 	}
@@ -138,26 +138,6 @@ func (j Job) postEvent(kind, body string, private bool) bool {
 	return true
 }
 
-// unused, but keeping for now
-// func (j Job) postFailure(reason string) bool {
-// 	f := failure{
-// 		Reason: reason,
-// 	}
-
-// 	if j.InstanceID() != "" {
-// 		if err := f.postInstance(j.instance.ID); err != nil {
-// 			j.Error(errors.WithMessage(err, "posting instance failure"))
-// 			return false
-// 		}
-// 	} else {
-// 		if err := f.postHardware((*j.hardware).HardwareID()); err != nil {
-// 			j.Error(errors.WithMessage(err, "posting hardware failure"))
-// 			return false
-// 		}
-// 	}
-// 	return true
-// }
-
 func posterFromJSON(b []byte) (poster, error) {
 	if len(b) == 0 {
 		return &event{_kind: "phone-home"}, nil

job/events_test.go

@@ -42,14 +42,13 @@ func TestPhoneHome(t *testing.T) {
 		fmt.Println("test:", name)
 		t.Log("test:", name)
 		reqs = nil
-		var hardware packet.Hardware = packet.HardwareCacher{
-			ID:    "$hardware_id",
-			State: packet.HardwareState(test.state),
-		}
 		j := Job{
-			Logger:   joblog.With("test", name),
-			mode:     modeInstance,
-			hardware: &hardware,
+			Logger: joblog.With("test", name),
+			mode:   modeInstance,
+			hardware: packet.HardwareCacher{
+				ID:    "$hardware_id",
+				State: packet.HardwareState(test.state),
+			},
 			instance: &packet.Instance{
 				ID: test.id,
 				OS: packet.OperatingSystem{

job/fetch.go

@@ -11,24 +11,24 @@ var (
 	servers singleflight.Group
 )
 
-func discoverHardwareFromDHCP(mac net.HardwareAddr, giaddr net.IP, circuitID string) (*packet.Discovery, error) {
+func discoverHardwareFromDHCP(mac net.HardwareAddr, giaddr net.IP, circuitID string) (packet.Discovery, error) {
 	fetch := func() (interface{}, error) {
 		return client.DiscoverHardwareFromDHCP(mac, giaddr, circuitID)
 	}
 	v, err := servers.Do(mac.String(), fetch)
 	if err != nil {
 		return nil, err
 	}
-	return v.(*packet.Discovery), nil
+	return v.(packet.Discovery), nil
 }
 
-func discoverHardwareFromIP(ip net.IP) (*packet.Discovery, error) {
+func discoverHardwareFromIP(ip net.IP) (packet.Discovery, error) {
 	fetch := func() (interface{}, error) {
 		return client.DiscoverHardwareFromIP(ip)
 	}
 	v, err := servers.Do(ip.String(), fetch)
 	if err != nil {
 		return nil, err
 	}
-	return v.(*packet.Discovery), nil
+	return v.(packet.Discovery), nil
 }

job/hardware.go

@@ -47,7 +47,7 @@ func (j Job) AddHardware(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	if _, err := client.PostHardwareComponent((*j.hardware).HardwareID(), bytes.NewReader(jsonBody)); err != nil {
+	if _, err := client.PostHardwareComponent(j.hardware.HardwareID(), bytes.NewReader(jsonBody)); err != nil {
 		joblog.Error(errors.Wrap(err, "posting componenents"))
 		w.WriteHeader(http.StatusBadRequest)
 		return

job/helpers.go

@@ -18,14 +18,14 @@ func (j Job) IsARM() bool {
 
 func (j Job) IsUEFI() bool {
 	if h := j.hardware; h != nil {
-		return (*h).HardwareUEFI(j.mac)
+		return h.HardwareUEFI(j.mac)
 	}
 	return false
 }
 
 func (j Job) Arch() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwareArch(j.mac)
+		return h.HardwareArch(j.mac)
 	}
 	return ""
 }
@@ -105,7 +105,7 @@ func (j Job) ID() string {
 
 func (j Job) Interfaces() []packet.Port {
 	if h := j.hardware; h != nil {
-		return (*h).Interfaces()
+		return h.Interfaces()
 	}
 	return nil
 }
@@ -126,35 +126,35 @@ func (j Job) InterfaceMAC(i int) net.HardwareAddr {
 
 func (j Job) HardwareID() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwareID()
+		return h.HardwareID()
 	}
 	return ""
 }
 
 func (j Job) FacilityCode() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwareFacilityCode()
+		return h.HardwareFacilityCode()
 	}
 	return ""
 }
 
 func (j Job) PlanSlug() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwarePlanSlug()
+		return h.HardwarePlanSlug()
 	}
 	return ""
 }
 
 func (j Job) PlanVersionSlug() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwarePlanVersionSlug()
+		return h.HardwarePlanVersionSlug()
 	}
 	return ""
 }
 
 func (j Job) Manufacturer() string {
 	if h := j.hardware; h != nil {
-		return (*h).HardwareManufacturer()
+		return h.HardwareManufacturer()
 	}
 	return ""
 }
@@ -164,48 +164,43 @@ func (j Job) PrimaryNIC() net.HardwareAddr {
 	return j.mac
 }
 
-// unused, but keeping for now
-// func (j Job) isPrimaryNIC(mac net.HardwareAddr) bool {
-// 	return bytes.Equal(mac, j.PrimaryNIC())
-// }
-
 // HardwareState will return (enrolled burn_in preinstallable preinstalling failed_preinstall provisionable provisioning deprovisioning in_use)
 func (j Job) HardwareState() string {
-	if h := j.hardware; h != nil && (*h).HardwareID() != "" {
-		return string((*h).HardwareState())
+	if h := j.hardware; h != nil && h.HardwareID() != "" {
+		return string(h.HardwareState())
 	}
 	return ""
 }
 
 func (j Job) ServicesVersion() string {
-	if h := j.hardware; h != nil && (*h).HardwareID() != "" {
-		return (*h).HardwareServicesVersion()
+	if h := j.hardware; h != nil && h.HardwareID() != "" {
+		return h.HardwareServicesVersion()
 	}
 	return ""
 }
 
 // CanWorkflow checks if workflow is allowed
 func (j Job) CanWorkflow() bool {
-	return (*j.hardware).HardwareAllowWorkflow(j.mac)
+	return j.hardware.HardwareAllowWorkflow(j.mac)
 }
 
 func (j Job) OsieBaseURL() string {
 	if h := j.hardware; h != nil {
-		return (*j.hardware).OsieBaseURL(j.mac)
+		return j.hardware.OsieBaseURL(j.mac)
 	}
 	return ""
 }
 
 func (j Job) KernelPath() string {
 	if h := j.hardware; h != nil {
-		return (*j.hardware).KernelPath(j.mac)
+		return j.hardware.KernelPath(j.mac)
 	}
 	return ""
 }
 
 func (j Job) InitrdPath() string {
 	if h := j.hardware; h != nil {
-		return (*j.hardware).InitrdPath(j.mac)
+		return j.hardware.InitrdPath(j.mac)
 	}
 	return ""
 }

job/job.go

@@ -30,7 +30,7 @@ type Job struct {
 	mode Mode
 	dhcp dhcp.Config
 
-	hardware *packet.Hardware
+	hardware packet.Hardware
 	instance *packet.Instance
 }
 
@@ -74,7 +74,7 @@ func CreateFromIP(ip net.IP) (Job, error) {
 	if err != nil {
 		return Job{}, errors.WithMessage(err, "discovering from ip address")
 	}
-	mac := (*d).GetMAC(ip)
+	mac := d.GetMAC(ip)
 	if mac.String() == packet.ZeroMAC.String() {
 		joblog.With("ip", ip).Fatal(errors.New("somehow got a zero mac"))
 	}
@@ -95,14 +95,8 @@ func (j Job) MarkDeviceActive() {
 	}
 }
 
-// golangci-lint: unused
-//func (j Job) markFailed(reason string) {
-//	j.postFailure(reason)
-//}
-
-func (j *Job) setup(dp *packet.Discovery) error {
-	d := *dp
-	dh := *d.Hardware()
+func (j *Job) setup(d packet.Discovery) error {
+	dh := d.Hardware()
 
 	j.Logger = joblog.With("mac", j.mac, "hardware.id", dh.HardwareID())