Milestone 2 - Server and Client #2
Conversation
There was a problem hiding this comment.
High level observations:
In your design you had some testing in milestone 2. Has that been cut?
There is lots of panic()ing going on in the cli logic. Can you explain why you chose this over something like https://github.com/spf13/cobra/tree/eb3b6397b1b5d1b0a2cd66a9afe0520f480c0a87#returning-and-handling-errors
| if err != nil { | ||
| return err // TODO: is this really how we should handle this? | ||
| } |
There was a problem hiding this comment.
This is the context termination I was wondering about in #1 (review). In this case we need to make sure the filehandle/memory/gorutine kicked off here
Lines 124 to 125 in 0c05535
There was a problem hiding this comment.
Of course. I tried to make it clear that this particular section is still in flux. I have a solution locally but It's unpleasant. I'm going to try some variations and see if anything feels better.
proc/proc.go
Outdated
| exitCodeMsg := fmt.Sprintf("Exited with code %v", uint32(p.cmd.ProcessState.ExitCode())) | ||
| stream <- exitCodeMsg |
There was a problem hiding this comment.
It feels "off" to me to inject rc info into the text stream. Why did you choose to change from your previous implementation?
There was a problem hiding this comment.
After combining Stdout and Stderr the only other type was the exit code. I took a look at the complexity required to handle this with the same pattern repeated in multiple packages (2 types for output both implementing an interface and the functions for handling and converting those types on both ends) and I recalled seeing output logs somewhere that just printed the exit code of a process.
With that said, I think one of the most significant cons to this approach is that I've made it much more difficult to automate integration with the Query() because the exit code would have to be parsed from the output. TBH I didn't think of this until I began responding to your comment and I have half a mind to put it right back in.
Thoughts either way? Is it unnecessary scope bloat or feature that you would like to see? I lean towards putting it back, but I'll leave the final say to you.
There was a problem hiding this comment.
I'd recommend not mixing the rc as text into the output stream. I think your previous approach was fine, or you could leave rc out of Query entirely, and make the user do a followup call to Status if they want it. While more work for the user, this does keep the code clean.
proc/proc.go
Outdated
| var copiedBytes []byte | ||
| copy(copiedBytes, buf) |
There was a problem hiding this comment.
Correct me if I am wrong, If we just send the current byte Slice down the stream the next call to file.Read(buf) could overwrite the data in the array under the byte slice we sent.
| type jobStop struct { | ||
| id uuid.UUID | ||
| } |
There was a problem hiding this comment.
Why do you chose a struct and a method here, as opposed to a function or inlining? I ask here, but this could apply to any of the client side files that follow the same pattern.
There was a problem hiding this comment.
It's a pattern that I use in many of the projects I work on, though the job here is so lightweight that it doesn't really display the value of the pattern. I'll try to give you a picture of why this is valuable as the complexity of the command/job scales by explaining the responsibility of each part.
cmdStop.Runis mostly responsible for parsing input of the cli/user into the format required for the job, but it also is the entry point for the job operation. It may eventually handle different forms or sources of input.jobStopis an enumeration of the data/type that the job needs to execute.jobStop.executeis the definition of the job's execution. It pays no mind to what form the input was in or what source it came from, only that it's injobStop
To be clear, I don't think I would write this code this way the first time, but experience with this library has encouraged me to make the division of responsibility this way.
There was a problem hiding this comment.
For me where this starts to become awkward is when you start writing tests for the client.
Obviously we trust grpc so we might not be interested in testing their code, so we'd want some level of separation between the cli itself and the server calls, in that way we can properly write tests for the UI while using mock server calls.
Separation also comes with other benefits such as using the same cli to make requests using different technologies, such as rest...
There was a problem hiding this comment.
For me where this starts to become awkward is when you start writing tests for the client.
Would you clarify what "this" is? I don't know if I understand what you are advocating for.
The client package does depend on BitBoxClient, which is an interface. Would you like tests on the clients?
There was a problem hiding this comment.
By this I meant It's a pattern that I use
I did say awkward not impossible. Ideally you wouldn't want to rework your cli tests, or your cli as a matter of fact, if api changes occurred for example. You also might find yourself held hostage by a third party library if you start mocking their interfaces. It's always good to have a layer of separation between the two.
I can keep on going with why this is not ideal.
This might be totally flying over my head, please correct me if I am wrong.
And no I wouldn't want you to write tests other than the ones you already planned on doing.
| } | ||
|
|
||
| func getOutboundIP() (net.IP, error) { | ||
| conn, err := net.Dial("udp", "8.8.8.8:80") |
There was a problem hiding this comment.
that's assuming the client is on a public network
if you're sure you want to go this route, it'd be better to loop through the network interfaces and get your ip from there
There was a problem hiding this comment.
The way this is ultimately used is as a target for the outbound grpc connection on line 66. I'm not sure a public interface makes more sense than loopback if the client is one the same box as the server -- and using loopback would cut all of this code. If the client isn't on the same box as the server, there is no sane default -- so loopback or making this required seems logical. IMO loopback is fine since that is probably how most of the development/testing will be done in this project.
| job := jobStart{ | ||
| command: args[0], | ||
| arguments: args[1:], | ||
| } |
There was a problem hiding this comment.
have you tried this with implicitly invoking a shell?
example /bin/bash -c "ls -lah ~"
|
@wadells Thanks for the link on errors with cobra. I've never seen that. Implemented |
| for { | ||
| n, err := file.Read(buf) | ||
| if err != nil { | ||
| // TODO: should we log the error somehow? | ||
| return | ||
| return nil |
There was a problem hiding this comment.
Looking at this again is there a chance that this will exit early and miss output? For example think of a process that only prints something every few seconds you will hit an io.EOF error which will cause pollRead to exit but the process may still be running.
No description provided.