Basic structure #1
Conversation
| all: build | ||
| @echo "Built" | ||
|
|
||
| start: build | ||
| (cd ./tests; ./scheduled > scheduled.log 2>&1 &) | ||
| (cd ./tests; tail -f *.log) | ||
|
|
||
| stop: | ||
| -pkill scheduled | ||
|
|
||
| clean: stop | ||
| rm -rf ./tests/data | ||
| rm -f ./tests/*.log | ||
| rm -f ./tests/*.key | ||
|
|
||
| fulltest: build | ||
| (cd ./tests; ./simple_test.sh) | ||
| tail ./tests/*.log | ||
|
|
||
| build: | ||
| go build -o tests/register cmd/register.go | ||
| go build -o tests/scheduled cmd/scheduled.go | ||
| go build -o tests/schedule cmd/schedule.go | ||
|
|
||
| install: | ||
| @echo "Install into root" | ||
|
|
||
| test: | ||
| go test -v github.com/phomer/scheduler/datastore |
There was a problem hiding this comment.
Most of those targets should be marked as phony
There was a problem hiding this comment.
Yes, I think all of them are currently phony, but it got me wondering if I could restructure the build targets to be driven from their file names?
There was a problem hiding this comment.
There is probably some way you could do that, but don't get hung up on the makefile 🙂 the current targets are pretty standard and well known, I would just declare them as phony so files or folders are not interfering with them
| // Lock the Accounts database and update it | ||
| auth.Lock() | ||
| auth.Reload() | ||
| auth.UpdateAccount(account) | ||
| auth.Update() | ||
|
|
||
| // Release the lock | ||
| auth.Unlock() |
There was a problem hiding this comment.
This seems like a very verbose API to update an account. Any reason why those steps are not inlined in UpdateAccount itself?
There was a problem hiding this comment.
I had buried them originally, but then I needed transactional integrity for Reload, UpdateAccount and Update, so I pushed the locks up to external. Later I realized that since there are really only 2 locations in the code that play here, the decomposition is wrong and it should all be internal (as I normally prefer).
| func Hostname() string { | ||
| hostname, err := os.Hostname() | ||
| if err != nil { | ||
| log.Fatal("Finding Hostname", err) | ||
| } | ||
| return hostname | ||
| } |
There was a problem hiding this comment.
I'm not a big fan of panic'ing on every error - this will start to be very hard to keep track of (in my opinion, it already is). I see that you're doing this across the entire project for now, so I'm not commenting on every single occurrence.
There was a problem hiding this comment.
No, it's pretty bad. It's an old habit that when I don't want to focus on it, just go fatal and stop. Works for a first pass and then I can grep Fatal later and fix it properly.
There was a problem hiding this comment.
That's fine. I think I left a similar comment on this PR already, but it's sometimes hard to grasp the intention of committed code. When in doubt, just add some TODO comments explaining any shortcuts.
There was a problem hiding this comment.
I'm actually of the opposite position, I'm a fan of crash only software, where errors that aren't recoverable and just surface should crash the process immediately. This isn't a model we follow at gravitational however.
So most of the team does prefer to see a sort of normal error handling, but if you document that this is intentionally modelled on unrecoverable errors should just immediately crash the process, I don't think this is a bad model at all, especially for a project that would be considered a prototype.
I agree with pierre though, if this is the model it should be commented somewhere.
There was a problem hiding this comment.
I've worked with different teams over the years that have essentially spanned the spectrum of error philosophies, my favorite is to stop immediately in dev, but muddle through in prod. I've written about it a few times: http://theprogrammersparadox.blogspot.com/2014/11/error-handling.html?m=0
| fmt.Println("Failed to Sign Token", err, token) | ||
| panic("Goodbye") |
There was a problem hiding this comment.
Should be, I had originally started typing in panics, then when I got tired of that I added the Fatal call. In normal circumstances, I would have set up the logging, error handler, and most other common utils before starting to code. I'm a big fan of reuse and consistency. This was just quick and dirty coding ...
| func Validate() { | ||
| } |
There was a problem hiding this comment.
This can be removed for now. I understand that it's likely a work-in-progress, but it's a bit confusing to see unrelated or dead code in commits. A TODO comment might do as well
| name := config.Hostname + "-" + config.Username + ".key" | ||
| return name |
There was a problem hiding this comment.
Personally, I find fmt.Sprintf more readable in those scenarios, but this works too
| for i := 0; i < len(paths); i++ { | ||
| path := "./" | ||
| filename := "elephant-paulwhomer.key" | ||
| datastore.TouchFile(path, filename) | ||
|
|
||
| return filepath.Join(path, filename) | ||
| } |
There was a problem hiding this comment.
I fail to see how this actually cycles through the paths supplied as the argument
There was a problem hiding this comment.
Yea, I had started to code it properly, then realized it was a bit messier than expected (I was thinking of calling out to the console if in either dir I hit multiple .key files), then I just hardcoded it to me and my machine to get it to work :-(
| switch value := err.(type) { | ||
| default: | ||
| log.Dump("error", err) | ||
| log.Fatal("Get", err) | ||
| _ = value | ||
| } |
There was a problem hiding this comment.
I had some code in there to switch on the error, to print out nicer messages. I cut it, but left the type switch on the assumption that I'd get back to it.
| _ = status | ||
| _ = args | ||
|
|
||
| data, err := ioutil.ReadAll(read_closer) | ||
| if err != nil { | ||
| fmt.Println("Buffer read err", err) | ||
| } else { | ||
| fmt.Println("Buffer:", string(data)) | ||
| } | ||
|
|
||
| fmt.Println("Command is Sent", status) | ||
|
|
||
| return "Sometext" |
There was a problem hiding this comment.
I assume this is a placeholder for now? It's always a bit confusing to have a mixture between finished code, work-in-progress, and placeholders when reviewing a PR. Comments help 🙂
There was a problem hiding this comment.
Yea. It needs to be switched steam stuff until the pipe is closed or a ^C is caught. Going forward, I'll try to be nicer with the TODOs for you. Normally I wouldn't check in code this rough, but these are special circumstances.
There was a problem hiding this comment.
no worries. I understand that this is work in progress, it just affects the quality of feedback I could give you.
There was a problem hiding this comment.
@phomer Sounds like a good start! One thing I'm noticing so far (this is why it's good to have these early reviews) is I think the scope of the project is expanding quite a bit beyond what's actually required for the challenge. For example, I would see if it's possible to simplify everything by removing your datastore and account management layers altogether, like I think we pointed out during the design doc review.
What the interview team will be assessing is the client/server communication, the jobs management layer and the overall code quality so I would focus on those and try to keep everything as simple as possible. For example, for the sake of the challenge it's okay to keep stuff in memory, use some pre-generated secrets and so on. The less code the better.
| go build -o tests/schedule cmd/schedule.go | ||
|
|
||
| install: | ||
| @echo "Install into root" |
There was a problem hiding this comment.
Missing implementation, not a big deal, might just need to be marked with a TODO
| ) | ||
|
|
||
| // All of the information necessary for both the client and server to function. | ||
| type Account struct { |
There was a problem hiding this comment.
It's a style thing, but generally we encourage following CodeReviewComments for Godoc structure: https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences
| func Hostname() string { | ||
| hostname, err := os.Hostname() | ||
| if err != nil { | ||
| log.Fatal("Finding Hostname", err) | ||
| } | ||
| return hostname | ||
| } |
There was a problem hiding this comment.
I'm actually of the opposite position, I'm a fan of crash only software, where errors that aren't recoverable and just surface should crash the process immediately. This isn't a model we follow at gravitational however.
So most of the team does prefer to see a sort of normal error handling, but if you document that this is intentionally modelled on unrecoverable errors should just immediately crash the process, I don't think this is a bad model at all, especially for a project that would be considered a prototype.
I agree with pierre though, if this is the model it should be commented somewhere.
| } | ||
|
|
||
| // Do all of the fun things necessary to daemonize a background service | ||
| func daemonize() { |
There was a problem hiding this comment.
I wouldn't worry about daemonizing the process, IMO it's becoming less of a thing. If you look at the way alot of software is run these days, someone may want to bring it up in a terminal to test, systemd doesn't require the process to daemonize itself (well there are several ways to integrate) and stdout/stderr will get captured in the system journal, and a docker container will exit if pid 1 exits.
Keep in mind that this is just an excersize, so keeping the scope small and efficient is a benefit.
| username := accounts.Username() // TODO: Opps! | ||
|
|
||
| isValid := true | ||
| if !isValid { |
There was a problem hiding this comment.
I assume this code is incomplete? Similar to pierre, it's hard to give valuable feedback when so much of the code appears to be in flux.
There was a problem hiding this comment.
Yea, it's just roughed in for the PR. It'll get finished on the next one ...
Set up the cmds and basic architecture.
Set up the build files and dependencies
Set up crute persistence and simple accounts