diff --git a/client/client.go b/client/client.go index e4f580d..1bb14f0 100644 --- a/client/client.go +++ b/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) } diff --git a/client/lorawan_client.go b/client/lorawan_client.go index 79c2f7b..05f0bd0 100644 --- a/client/lorawan_client.go +++ b/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. } diff --git a/client/lorawan_client_test.go b/client/lorawan_client_test.go index e7b8cd5..f8c071f 100644 --- a/client/lorawan_client_test.go +++ b/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,6 +45,7 @@ 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, @@ -46,3 +53,47 @@ func (m *MockClient) Post(url string, contentType string, body io.Reader) (resp 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) + } + }) + } +} diff --git a/device/device.go b/device/device.go index c1fb085..fa9cc8b 100644 --- a/device/device.go +++ b/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 diff --git a/device/device_test.go b/device/device_test.go index 700a890..bf6598f 100644 --- a/device/device_test.go +++ b/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() diff --git a/dockerfile b/dockerfile index 467cc91..3ad29fb 100644 --- a/dockerfile +++ b/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"] \ No newline at end of file +ENTRYPOINT ["/deveui-cli"] diff --git a/go.mod b/go.mod index aa4b41a..6a15cb7 100644 --- a/go.mod +++ b/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 +) diff --git a/go.sum b/go.sum index d61b19e..a90a2b0 100644 --- a/go.sum +++ b/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= diff --git a/main.go b/main.go index d38928c..c8b1c32 100644 --- a/main.go +++ b/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 } diff --git a/processor/code_processor.go b/processor/code_processor.go index 60ff29a..c738af1 100644 --- a/processor/code_processor.go +++ b/processor/code_processor.go @@ -14,13 +14,13 @@ import ( type CodeProcessor struct { CodeRegistrationLimit int MaxConcurrentJobs int - BaseUrl string + BaseUrl string // seems like something the client should know about, not this processor Client client.Client Device chan device.Device } // Worker attempts to register a valid DevEUI via external LoRaWAN API. -// If successfull, a RegisterDevice struct with it's Identifier and Code will be sent to the work channel. +// If successful, a RegisterDevice struct with its Identifier and Code will be sent to the work channel. // // # Example // @@ -41,6 +41,13 @@ func (cp *CodeProcessor) Worker(ctx context.Context, work chan struct{}) error { } } +// I think you should store the devices in a map or something to be able to ensure their uniqueness. +// The reason is that even if the 16 char code is unique, the last 5 chars used for the lookup can be the same. +// This wasn't an explicit requirement, but from the description it would just make sense. + +// if this was a method of CodeProcessor then you wouldn't need the input parameters +// The bool return parameter is redundant, you can just return the device pointer and check if it is nil. +// Even better, return the pointer AND an error. func registerDevice(client client.Client, url string) (bool, *device.Device) { device := device.NewDevice() @@ -49,12 +56,14 @@ func registerDevice(client client.Client, url string) (bool, *device.Device) { err := json.NewEncoder(b).Encode(&reqBody) if err != nil { + // if you just log the error here, the POST request will still hit the endpoint with invalid data log.Print(err) } resp, err := client.Post(url, "application/json", b) if err != nil { + // this will kill the whole app on the first POST error log.Fatal(err) } diff --git a/processor/code_processor_test.go b/processor/code_processor_test.go index 7528101..3f4acdc 100644 --- a/processor/code_processor_test.go +++ b/processor/code_processor_test.go @@ -10,6 +10,8 @@ import ( "github.com/NickGowdy/deveui-cli/device" ) +// for mocking in tests I highly recommend using the github.com/stretchr/testify/mock package + type MockClient struct { DoPost func(url string, contentType string, body io.Reader) (resp *http.Response, err error) }