-
Notifications
You must be signed in to change notification settings - Fork 1
review-me #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
NickGowdy
wants to merge
84
commits into
review-me
Choose a base branch
from
main
base: review-me
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
review-me #2
Changes from all commits
Commits
Show all changes
84 commits
Select commit
Hold shift + click to select a range
9dc8757
use helper to generate hex string
NickGowdy 106ff9a
can talk to api
NickGowdy 828bffa
getting 200 response
NickGowdy 8d6431b
WIP - basic version working
NickGowdy 577384b
move http client code to separate module
NickGowdy eb08783
WIP - works but need to keep track of succesful registration
NickGowdy b84e4d9
change setup for client
NickGowdy 9f37db2
small refactor
NickGowdy 12f7d10
use lock to change iterator
NickGowdy 5571400
file for registering code
NickGowdy 306c3f9
changes and file rename
NickGowdy f83a4da
can now listen to signal and restart
NickGowdy 615c9bb
added env vars
NickGowdy d8779ff
basic readme
NickGowdy c52424d
dont need defer here, was causing crash
NickGowdy c50c2d4
use interface so we can mock http request
NickGowdy 877b7d6
unit tests
NickGowdy 18423d8
changed to direct because of linting error
NickGowdy 064cdb7
bug fixing
NickGowdy 95630cf
Seems more stable
NickGowdy 8e823cc
WIP - seems to work better this way
NickGowdy f87debb
Less code, easier to read and stable
NickGowdy b130681
Reuse client
NickGowdy 799b58c
code generator unit tests
NickGowdy adac666
use interface
NickGowdy 07afe6f
comments
NickGowdy 6876d48
Return registered devices
NickGowdy 4c6d036
More tests
NickGowdy 4891795
Formatting of string
NickGowdy c55dfe5
dockerfile
NickGowdy 301a8c3
more readme
NickGowdy 066e961
dont need port number here
NickGowdy 8dd927a
fatal if cant contact server
NickGowdy da047dd
more instructions
NickGowdy 4e31de4
show hex with id
NickGowdy 15eb611
Trying to fix race condition
NickGowdy c93a6df
more changes
NickGowdy ec319bd
Pass in context and use channels in main to fix race cond
NickGowdy a266a5f
delete file
NickGowdy 064be87
fix test errs
NickGowdy 430d45c
comments and tidy up
NickGowdy 66b693d
more proffessional readme
NickGowdy e9f0684
Links now working
NickGowdy 5475149
final changes
NickGowdy 96513d1
Close channels once done
NickGowdy bde4c83
refactor device logic
NickGowdy 213896b
I think these are last changes
NickGowdy 8420c36
Only listen to interrupts
NickGowdy 04c9428
close here to prevent panic
NickGowdy baa96bc
benchmark perf
NickGowdy 5b3a2cb
Added benchmark to readme
NickGowdy 9da4a36
don't need to close channel, GC will handle it
NickGowdy 297b7aa
done
NickGowdy 6762a65
more assertions
ec15adc
Move interface to lorawan_client.go
NickGowdy 2922c10
Merge branch 'main' of https://github.com/NickGowdy/deveui-cli into main
NickGowdy 7668633
NewLoraWAN func to create new client
NickGowdy 87d8798
rename lorawan file
NickGowdy 52ad9f4
Use path.Join instead of fmt
NickGowdy 7158f46
interface changes
NickGowdy e58835b
refactor
NickGowdy f3e4283
pass in interface to lorawan post
NickGowdy c54c8e3
I can set custom timeout this way
NickGowdy 0aef7d2
pass context
NickGowdy f8bcd83
ignore Goland folder
NickGowdy f583b64
Corrections based on Goland feedback
NickGowdy cf8466e
Return error all the way up to main.go
NickGowdy bf7a72a
Use env variables for all config
NickGowdy 0815e22
encapsulate device values and create public funcs to return what we need
NickGowdy 0584c5f
use device public values
NickGowdy 8276236
tests now passing again
NickGowdy 3fd84f0
return nil, err here as well
NickGowdy 49f4420
use request with context instead
NickGowdy 2565ebc
small refactor
NickGowdy e869ad7
use codeRegistrationLimit instead
NickGowdy 179181b
more tests
NickGowdy 16048cd
close channel once done
NickGowdy 6fee7cd
use same buffer
NickGowdy b638ffa
trying to use done pattern
NickGowdy a3f6299
this calls done
NickGowdy fabb4a9
WIP - refactor (encapsulate implementation of registering in LoraWAN
NickGowdy 0843818
refactor of work processor
NickGowdy 8da5472
rename
NickGowdy dad1f98
get it work sequentially
NickGowdy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,102 @@ | ||
| # deveui-cli | ||
| <br /> | ||
| <div align="center"> | ||
| <a href="https://machinemax.com/"> | ||
| <img src="images/logo.jpeg" alt="MachineMax Logo" width="200" height="200"> | ||
| </a> | ||
|
|
||
| <h1 align="center">DevEUI CLI</h1> | ||
|
|
||
| <p align="center"> | ||
| <h2 align="center">A Golang program for concurrently registering DevEUI identifiers for MachineMax.</h2> | ||
| </p> | ||
| </div> | ||
|
|
||
| ## About MachineMax DevEUI | ||
|
|
||
| Each MachineMax sensor has a unique 16-character (hex) identifier called a DevEUI. As part of | ||
| the manufacturing process, it is written onto the internal storage of the sensor. The DevEUI is | ||
| also printed on a label on the side of the sensor alongside a 5-character code (the last 5 | ||
| characters of the DevEUI). For example, a DevEUI of 78111FFFE452555B would have a short | ||
| code of 2555B. | ||
|
|
||
| The sensors communicate with the MachineMax cloud though a LoRaWAN provider and the | ||
| LoRaWAN provider uses the DevEUI to identify the sensor. This means we first have to register | ||
| the DevEUI with the provider before we can use it. We pay for every device registered with the | ||
| LoRaWAN provider, so it is important that we only register DevEUIs that we use. | ||
|
|
||
| When a customer registers a new sensor, they will enter the 5-character short-form code instead | ||
| of the full DevEUI, so it is essential that each DevEUI in the batch has a unique 5-char code (for | ||
| lookups). | ||
|
|
||
| ### Built With | ||
|
|
||
| * Golang | ||
| * Docker | ||
|
|
||
| ## Getting Started | ||
|
|
||
| To run this code, we first need a `.env` file in the root of the project. Once this is done, add these vars: | ||
|
|
||
| ``` | ||
| BASE_URL=http://europe-west1-machinemax-dev-d524.cloudfunctions.net | ||
| TIMEOUT=30000 | ||
| CODE_REGISTRATION_LIMIT=100 | ||
| ``` | ||
|
|
||
| ### Prerequisites | ||
|
|
||
| - You will need Golang to run this program which can be downloaded at: [https://go.dev/](https://go.dev/) | ||
| - This program can also be run using docker, this can be downloaded at: [https://www.docker.com/](https://www.docker.com/) | ||
|
|
||
| ## Usage | ||
|
|
||
| Then to run locally, use: `go run main.go`. | ||
|
|
||
| Alternatively, this code can also be run via docker. To build the docker image use: | ||
| ``` | ||
| docker build -t deveui-cli . --build-arg BASE_URL=${BASE_URL} --build-arg TIMEOUT=${TIMEOUT} --build-arg CODE_REGISTRATION_LIMIT=${CODE_REGISTRATION_LIMIT}. | ||
| ``` | ||
|
|
||
| To run the docker image: | ||
| ``` | ||
| docker run deveui-cli | ||
| ``` | ||
|
|
||
| To run the tests use: | ||
| ``` | ||
| go test ./... | ||
| ``` | ||
| To check code coverage: | ||
| ``` | ||
| go test -coverprofile=coverage.out ./... ; go tool cover -html=coverage.out | ||
| ``` | ||
|
|
||
| And to run benchmark tests for CPU and memory consumption: | ||
| ``` | ||
| go test -bench=. -benchmem | ||
| ``` | ||
|
|
||
| Finally to check for race conditions, use: | ||
| ``` | ||
| go test -race ./... | ||
| ``` | ||
|
|
||
| ## Checklist | ||
|
|
||
| - [x] Implement solution | ||
| - [x] Write unit tests | ||
| - [x] Write readme | ||
| - [ ] More unit tests around unhappy paths for higher code coverage | ||
| - [ ] Discuss with engineers how to improve code | ||
|
|
||
| ## Contact | ||
|
|
||
| Email - nickgowdy87@gmail.com | ||
|
|
||
| Website <a href="http://www.nickgowdy.com/" target="_blank">http://www.nickgowdy.com/</a> | ||
|
|
||
| Github <a href="https://github.com/nickgowdy" target="_blank">https://github.com/nickgowdy</a> | ||
|
|
||
|
|
||
|
|
||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package client | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "encoding/json" | ||
| "errors" | ||
| "net/http" | ||
|
|
||
| "github.com/NickGowdy/deveui-cli/device" | ||
| ) | ||
|
|
||
| // Client used to communicate to external services | ||
| type Client interface { | ||
| Do(*http.Request) (resp *http.Response, err error) | ||
| } | ||
|
|
||
| // LoraWAN used to communicate to LoRaWAN external system | ||
| type LoraWAN struct { | ||
| baseURL string | ||
| client Client | ||
| } | ||
|
|
||
| func NewLoraWAN(baseURL string, client Client) *LoraWAN { | ||
| return &LoraWAN{ | ||
| baseURL: baseURL, | ||
| client: client, | ||
| } | ||
| } | ||
|
|
||
| const endpoint = "/sensor-onboarding-sample" // endpoint for saving DevEUI via LoRaWAN | ||
|
|
||
| // RegisterDevice registers new device using LoraWAN external service | ||
| func (l *LoraWAN) RegisterDevice(ctx context.Context) (*device.Device, error) { | ||
| device := device.NewDevice() | ||
| identifier := device.GetIdentifier() | ||
| b := new(bytes.Buffer) | ||
| reqBody := map[string]string{"Deveui": identifier} | ||
|
|
||
| err := json.NewEncoder(b).Encode(&reqBody) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| fullUrl := l.baseURL + endpoint | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be safer to use something like |
||
| req, err := http.NewRequestWithContext(ctx, "POST", fullUrl, b) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| resp, err := l.client.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err != nil { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error handler doesn't seem to be checking anything new. |
||
| return nil, err | ||
| } | ||
|
|
||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode == http.StatusOK { | ||
| return device, nil | ||
| } else { | ||
| return nil, errors.New(resp.Status) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package client | ||
|
|
||
| import ( | ||
| "net/http" | ||
| ) | ||
|
|
||
| type MockClient struct { | ||
| DoFunc func(*http.Request) (resp *http.Response, err error) | ||
| } | ||
|
|
||
| // func TestLorawanClientHappyPath(t *testing.T) { | ||
| // mockClient := &MockClient{ | ||
| // DoFunc: func(*http.Request) (resp *http.Response, err error) { | ||
| // return &http.Response{}, nil | ||
| // }, | ||
| // } | ||
|
|
||
| // loraWAN := NewLoraWAN("www.example.com", mockClient) | ||
|
|
||
| // b := new(bytes.Buffer) | ||
| // reqBody := map[string]string{"Deveui": "Abcde"} | ||
|
|
||
| // _ = json.NewEncoder(b).Encode(&reqBody) | ||
|
|
||
| // ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| // if cancel == nil { | ||
| // t.Errorf("cancel should not be nil but is: %v", cancel) | ||
| // } | ||
|
|
||
| // resp, err := loraWAN.DoPost(b, ctx) | ||
|
|
||
| // if err != nil { | ||
| // t.Errorf("err should be nil but is: %s", err.Error()) | ||
| // } | ||
| // defer resp.Body.Close() | ||
|
|
||
| // if resp.StatusCode != 200 { | ||
| // t.Errorf("resp should be nil but is: %d", resp.StatusCode) | ||
| // } | ||
|
|
||
| // body, _ := io.ReadAll(resp.Body) | ||
| // val := string(body) | ||
|
|
||
| // if strings.TrimSpace(val) != "true" { | ||
| // t.Errorf("body should equal true but is: %d", body) | ||
| // } | ||
| // } | ||
|
|
||
| // func TestNewLoraWanClient(t *testing.T) { | ||
| // client := &http.Client{ | ||
| // Timeout: 30 * time.Second, | ||
| // } | ||
| // t.Parallel() | ||
| // type args struct { | ||
| // timeout time.Duration | ||
| // } | ||
| // tests := []struct { | ||
| // name string | ||
| // args args | ||
| // want *LoraWAN | ||
| // }{ | ||
| // { | ||
| // name: "create-new-lorawan-client", | ||
| // args: args{ | ||
| // timeout: 30, | ||
| // }, | ||
| // want: &LoraWAN{ | ||
| // baseURL: "https://www.example.com", | ||
| // client: client, | ||
| // }, | ||
| // }, | ||
| // } | ||
| // 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 := NewLoraWAN("https://www.example.com", client); !reflect.DeepEqual(got, tt.want) { | ||
| // t.Errorf("NewLoraWanClient() = %v, want %v", got, tt.want) | ||
| // } | ||
| // }) | ||
| // } | ||
| // } | ||
|
|
||
| // func (m *MockClient) Do(*http.Request) (resp *http.Response, err error) { | ||
| // b := new(bytes.Buffer) | ||
| // reqBody := true | ||
|
|
||
| // _ = json.NewEncoder(b).Encode(&reqBody) | ||
| // return &http.Response{ | ||
| // StatusCode: http.StatusOK, | ||
| // Body: io.NopCloser(b), | ||
| // Status: "200 OK"}, | ||
| // nil | ||
| // } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package device | ||
|
|
||
| import ( | ||
| "crypto/rand" | ||
| "fmt" | ||
| "log" | ||
| "math/big" | ||
| ) | ||
|
|
||
| const ( | ||
| AllowedChars = "ABCDEF0123456789" // accepted chars used to make up DevEUI | ||
| DevEuiLength = 16 // valid DevEUI is string of length 16 | ||
| ) | ||
|
|
||
| type Device struct { | ||
| identifier string | ||
| code string | ||
| } | ||
|
|
||
| // NewDevice Build a new device with DevEUI identifier and code values. | ||
| // | ||
| // # Example | ||
| // | ||
| // 1CEB0080F074F750 4F750 | ||
| func NewDevice() *Device { | ||
| hex, err := generateHexString() | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
|
|
||
| return &Device{ | ||
| identifier: hex, | ||
| code: hex[len(hex)-5:], | ||
| } | ||
| } | ||
|
|
||
| func (d Device) GetIdentifier() string { | ||
| return d.identifier | ||
| } | ||
|
|
||
| func (d Device) GetCode() string { | ||
| return d.code | ||
| } | ||
|
|
||
| func (d Device) Print() { | ||
| fmt.Printf("device has identifier: %s and code: %s\n", d.identifier, d.code) | ||
| } | ||
|
|
||
| // Generate valid DevEUI identifier value. | ||
| // | ||
| // # Example | ||
| // | ||
| // 1CEB0080F074F750 | ||
| func generateHexString() (string, error) { | ||
| max := big.NewInt(int64(len(AllowedChars))) | ||
| b := make([]byte, DevEuiLength) | ||
| for i := range b { | ||
| n, err := rand.Int(rand.Reader, max) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| b[i] = AllowedChars[n.Int64()] | ||
| } | ||
| return string(b), nil | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this client package/struct encapsulate more information about the API endpoints and their request/response?
For example, rather than having the
LoraWan.DoPostmethod, what if we had something that looked more like:This would mean that the specifics of marshalling/unmarshalling the request/response would be contained within this package and wouldn't be leaking out into the
processorpackage. If this endpoint were to change slightly, an engineer would have to make changes in both this package and in theprocessorpackage.