Phase 2: Implement Server and serve command #2
Conversation
|
|
There was a problem hiding this comment.
Initial review. Looking good. In addition to the line comments — for CLI commands, I'd like to clean up error messaging, because right now this is very verbose:
./relay add -vvvv --file tmp/2.lic --key $(cat tmp/2.key) --public-key $(cat tmp/keygen.pub)
# time=2024-09-27T10:36:39.330-05:00 level=ERROR msg="failed to insert license into store" licenseID=e9b6f649-d8c2-49e3-9d41-aab32f0d08a3 error="UNIQUE constraint failed: licenses.key"
# error creating license record: failed to insert license: UNIQUE constraint failed: licenses.key
# Error: failed to insert license: UNIQUE constraint failed: licenses.key
# time=2024-09-27T10:36:39.330-05:00 level=ERROR msg="failed to execute command" error="failed to insert license: UNIQUE constraint failed: licenses.key"This type of logging is fine for the server (e.g. timestamps, etc.), but the CLI should have better error messaging, with a single clear and colored error message. There are also duplicate error logs in the above example that I'd like to consolidate.
| @@ -55,13 +65,13 @@ func Run() int { | |||
|
|
|||
| rootCmd.PersistentFlags().StringVar(&cfg.DB.DatabaseFilePath, "database", "./relay.sqlite", "specify an alternate database path") | |||
| rootCmd.PersistentFlags().CountVarP(&cfg.Logger.Verbosity, "verbose", "v", "counted verbosity") | |||
There was a problem hiding this comment.
I'm not sure this flag is working as expected. By default, the log level should be error, then:
-vshould be error.-vvshould be warn.-vvvshould be info.-vvvv+ should be debug.
Right now, I'm not really sure what the order is, but e.g. it seems like -vv enables debug logging. The expected behavior would be the more -v flags, the more verbose logging should get.
There was a problem hiding this comment.
oh yea, my bad
I have fixed it
|
|
||
| handler := server.NewHandler(srv.Manager()) | ||
| router := mux.NewRouter() | ||
| handler.RegisterRoutes(router) |
There was a problem hiding this comment.
Can we add general server logging to stdout e.g. I want to log 200s, 404s, 500s, etc.?
There was a problem hiding this comment.
I have added the logger middleware, so now we can track HTTP requests like this
2024-09-28 18:40:32 INF server/middleware.go:35 HTTP request method=PUT path=/v1/nodes/329ecfa4c5d24084b416fc35a306cf65 status=201 remote_addr=127.0.0.1:57798 user_agent=curl/7.81.0 duration=8.893744ms
I also updated the CLI responses to be more user-friendly and added some color for better readability
internal/licenses/manager.go
Outdated
| return store.ClaimUnclaimedLicenseFIFO(ctx, nodeID) | ||
| case "lifo": | ||
| return store.ClaimUnclaimedLicenseLIFO(ctx, nodeID) | ||
| case "random": |
There was a problem hiding this comment.
| case "random": | |
| case "rand": |
There was a problem hiding this comment.
Nice catch, fixed!
| } | ||
|
|
||
| cmd.Flags().IntVarP(&cfg.ServerPort, "port", "p", cfg.ServerPort, "Port to run the relay server on") | ||
| cmd.Flags().DurationVarP(&cfg.TTL, "ttl", "t", cfg.TTL, "Time-to-live for license claims") |
There was a problem hiding this comment.
Let's add a validation to this flag for it to be at minimum 30s.
There was a problem hiding this comment.
done!
I also added tests for it
This pull request implements the server functionality and the
servecommand.It includes significant refactoring for modularity and better test coverage
Key Features:
fifo,lifo,random) with the--strategyflag