Review comments

client/client.go

@@ -5,7 +5,9 @@ import (
 	"net/http"
 )
 
-// Generic client used to communicate to external services
+// I don't see any reason why this is in a separate file, it would be better in the other file (lorawan).
+
+// Client generic client used to communicate to external services
 type Client interface {
 	Post(url string, contentType string, body io.Reader) (resp *http.Response, err error)
 }

client/lorawan_client.go

@@ -4,22 +4,47 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"time"
 )
 
-// Client used to communicate to LoRaWAN external system
+// I'd be inclined to rename this to just lorawan.go, because it's already in the client package.
+// comments in Go start with the name of the described entity
+
+// LoraWanClient used to communicate to LoRaWAN external system
 type LoraWanClient struct {
-	Client Client
+	client *http.Client // the internal field can be the direct type, the struct itself will implement the interface
 }
 
 const endpoint = "/sensor-onboarding-sample" // endpoint for saving DevEUI via LoRaWAN
 
-// Send data via POST (HTTP) request
+// Unless you want to share the same http client with other code, it's better to hide it as an internal field.
+// The users of the client package are not interested in it anyway, they only care about the interface methods.
+
+// NewLoraWanClient creates a new LoraWanClient that implements the Client interface
+func NewLoraWanClient(timeout time.Duration) *LoraWanClient {
+	return &LoraWanClient{
+		client: &http.Client{
+			Timeout: timeout * time.Second,
+		},
+	}
+}
+
+// Post sends data via POST (HTTP) request
 func (h *LoraWanClient) Post(url string, contentType string, body io.Reader) (resp *http.Response, err error) {
+	// Probably use url.JoinPath instead of fmt.Sprintf
+	// Also, I think your url won't change throughout the app, so could be on the struct as an internal field, because
+	// at the moment it's calculated with every call of the Post method.
+	// contentType also doesn't change, could just be "application/json" as a const
 	fullUrl := fmt.Sprintf("%s/%s", url, endpoint)
-	resp, err = h.Client.Post(fullUrl, contentType, body)
+	resp, err = h.client.Post(fullUrl, contentType, body)
 	if err != nil {
+		// some people find it annoying, but I prefer wrapping errors, which can greatly help during debugging
 		return nil, err
 	}
 
 	return resp, nil
+
+	// Another way to further improve this is to use the http.Client.Do method that requires a http.Request.
+	// The request can be created with the http.NewRequestWithContext method, so you can pass down the context
+	// all the way. When the parent context is cancelled, the request will be cancelled too.
 }

client/lorawan_client_test.go

@@ -5,22 +5,28 @@ import (
 	"encoding/json"
 	"io"
 	"net/http"
+	"reflect"
 	"testing"
+	"time"
+
+	"github.com/stretchr/testify/mock"
 )
 
 type MockClient struct {
 	DoPost func(url string, contentType string, body io.Reader) (resp *http.Response, err error)
 }
 
+// TestLorawanClientHappyPath doesn't seem to test anything really.
+// You should use httptest.NewServer to test your Post method.
 func TestLorawanClientHappyPath(t *testing.T) {
-	mockClient := &MockClient{
-		DoPost: func(url string, contentType string, body io.Reader) (resp *http.Response, err error) {
-			return &http.Response{}, nil
-		},
-	}
+	//mockClient := &MockClient{
+	//	DoPost: func(url string, contentType string, body io.Reader) (resp *http.Response, err error) {
+	//		return &http.Response{}, nil
+	//	},
+	//}
 
 	loraWanClient := LoraWanClient{
-		Client: mockClient,
+		//client: mockClient,
 	}
 
 	b := new(bytes.Buffer)
@@ -39,10 +45,55 @@ func TestLorawanClientHappyPath(t *testing.T) {
 	}
 }
 
+// unused parameters should be removed or replaced with underscores
 func (m *MockClient) Post(url string, contentType string, body io.Reader) (resp *http.Response, err error) {
 	return &http.Response{
 			StatusCode: http.StatusOK,
 			Body:       io.NopCloser(bytes.NewReader(nil)),
 			Status:     "200 OK"},
 		nil
 }
+
+type ClientMock struct {
+	mock.Mock
+}
+
+func (m *ClientMock) Post(url string, contentType string, body io.Reader) (resp *http.Response, err error) {
+	m.Called(url, contentType, body)
+
+	return
+}
+
+// This is a table test, my Goland IDE can generate the skeleton for methods easily.
+func TestNewLoraWanClient(t *testing.T) {
+	t.Parallel() // this makes sure that TestNewLoraWanClient is executed in parallel with other tests
+	type args struct {
+		timeout time.Duration
+	}
+	tests := []struct {
+		name string
+		args args
+		want *LoraWanClient
+	}{
+		{
+			name: "create-new-lorawan-client",
+			args: args{
+				timeout: 30,
+			},
+			want: &LoraWanClient{
+				client: &http.Client{
+					Timeout: 30 * time.Second,
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt // it is important to capture range variable
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel() // this makes sure that all cases from the table here are executed in parallel
+			if got := NewLoraWanClient(tt.args.timeout); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("NewLoraWanClient() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

device/device.go

@@ -16,14 +16,15 @@ type Device struct {
 	Code       string
 }
 
-// Build new device with DevEUI identifier and code values.
-//
+// NewDevice build new device with DevEUI identifier and code values
 // # Example
 //
 //	1CEB0080F074F750 4F750
 func NewDevice() *Device {
 	hex, err := generateHexString()
 	if err != nil {
+		// this here will crash your whole program
+		// Instead, return an error and handle it at the place of call, possibly log it and generate a new device.
 		log.Fatal(err)
 	}
 
@@ -33,8 +34,7 @@ func NewDevice() *Device {
 	}
 }
 
-// Generate valid DevEUI identifier value.
-//
+// generateHexString generate valid DevEUI identifier value.
 // # Example
 //
 //	1CEB0080F074F750

device/device_test.go

@@ -4,6 +4,9 @@ import (
 	"testing"
 )
 
+// there seems to be a lot of repetitions in this test
+// in most cases, parallel table driven tetst are easier to follow and quicker to execute
+// however, to test the resulting code, you could just use a nice regex pattern :)
 func TestCanGenerateValidCode(t *testing.T) {
 	allowedChars := []string{"A", "B", "C", "D", "E", "F", "0", "1", "2", "3", "4", "5", "6", "7", "8", "9"}
 	device := NewDevice()

dockerfile

@@ -1,20 +1,37 @@
-FROM golang:1.19-alpine
+# use the latest Go version and also give it a name so that we can leverage multi-stage builds
+FROM golang:1.20-alpine AS base
 
+# why is git added, it doesn't seem to be used
 RUN apk add --no-cache git
 
+# it's common practice to create and use a non-root user in the image
+RUN adduser -D -g '' nonroot_user
+
 # Set the Current Working Directory inside the container
 WORKDIR /app/deveui-cli
 
 # We want to populate the module cache based on the go.{mod,sum} files.
+# this won't copy go.sum, just use COPY go.*  .
 COPY go.mod .
 
 RUN go mod download
 
 COPY . .
 
+# At this point everything from the golang alpine image will be part of your application, so the image
+# size will be fairly large. Instead, add a build step and at the end use scratch and copy the compiled
+# files to the final image.
+FROM base AS go-builder
+
 # Build the Go app
 RUN go build -o ./deveui-cli .
 
+# this will make your final image size considerably smaller
+FROM scratch AS production
+COPY --from=go-builder ./deveui-cli /deveui-cli
+
+# if you used a non-root user, then add them here
+USER nonroot_user
 
 # Run the binary program produced by `go install`
-ENTRYPOINT ["./deveui-cli"]
+ENTRYPOINT ["/deveui-cli"]

go.mod

@@ -1,5 +1,14 @@
 module github.com/NickGowdy/deveui-cli
 
-go 1.19
+go 1.20
 
 require github.com/joho/godotenv v1.5.1 // direct
+
+require github.com/stretchr/testify v1.8.2
+
+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	github.com/stretchr/objx v0.5.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
+)

go.sum

@@ -1,2 +1,20 @@
+github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/joho/godotenv v1.5.1 h1:7eLL/+HRGLY0ldzfGMeQkb7vMd0as4CfYvUVzLqw0N0=
 github.com/joho/godotenv v1.5.1/go.mod h1:f4LDr5Voq0i2e/R5DDNOoa2zzDfwtkZa6DnEwAbqwq4=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
+github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
+github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8=
+github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

main.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"log"
-	"net/http"
 	"os"
 	"os/signal"
 	"syscall"
@@ -16,6 +15,7 @@ import (
 	"github.com/joho/godotenv"
 )
 
+// I'd make them configurable from env vars
 const (
 	MAX_CONCURRENT_JOBS     = /* Buffer limit for channel */ 10
 	CODE_REGISTRATION_LIMIT = /* Maximum number of devices that will be registered */ 100
@@ -37,16 +37,24 @@ Once this program starts, it will listen to syscall.SIGTERM, syscall.SIGINT via
 This is to handle any unexpected terminations of the program and to resume processing DevEUIs.
 */
 func main() {
-	godotenv.Load(".env")
+	// always check for errors
+	if err := godotenv.Load(".env"); err != nil {
+		// since this is just the app init in main, you can either panic or have a log.Fatal to abort further
+		// execution on critical errors
+		panic("error loading.env file")
+	}
+
 	baseurl := os.Getenv("BASE_URL")
+	if baseurl == "" {
+		// either error or set a default value, but in the latter probably log the fact
+		// I like to use github.com/kelseyhightower/envconfig package for configuration that comes from
+		// environment variables
+		panic("BASE_URL environment variable is not set")
+	}
 
 	// setup client for requests
-	httpClient := &http.Client{
-		Timeout: time.Second * TIMEOUT,
-	}
-	loraWanClient := &client.LoraWanClient{
-		Client: httpClient,
-	}
+	// I'd have the http client on the LoraWanClient struct as an internal field, I'll explain in the other file
+	loraWanClient := client.NewLoraWanClient(TIMEOUT)
 
 	// setup processor to do work
 	codeProcessor := &processor.CodeProcessor{
@@ -66,13 +74,19 @@ func main() {
 	// goroutine to listen for syscall.SIGINT
 	go func() {
 		signal.Notify(listener, syscall.SIGINT)
+		// this goroutine doesn't do anything useful, just burns resources
+		// you start a new goroutine that sleeps for 1000 in an endless loop
 		go func() {
 			for {
-				time.Sleep(1000)
+				time.Sleep(1000) // add a unit of time, eg. time.Nanosecond
 			}
 		}()
+		// your code blocks here until you receive a syscall.SIGINT
 		sig := <-listener
+		// then print a message to the console
 		log.Printf("Caught signal %v", sig)
+		// but since this is all in a goroutine, your app keeps running on the main thread
+		// at least you should call cancel(), but still the rest of the app would need some adjustments
 	}()
 
 	// Fill work buffer so we can start processing work
@@ -83,6 +97,7 @@ func main() {
 	}()
 
 	// Spawn workers
+	//the 10 workers will sit there waiting even if there isn't that much work to do
 	for job := 0; job < MAX_CONCURRENT_JOBS; job++ {
 		go codeProcessor.Worker(ctx, work)
 	}
@@ -91,7 +106,7 @@ func main() {
 	count := 0
 	for d := range codeProcessor.Device {
 		fmt.Printf("device: %d has identifier: %s and code: %s\n", count+1, d.Identifier, d.Code)
-		count += 1
+		count += 1 // count++
 		if count == CODE_REGISTRATION_LIMIT {
 			break
 		}