Cli #1

kmacmcfarlane · 2019-01-24T20:59:15Z

I have implemented most of the CLI functionality.

There are two main components:
pkg/command_parser - Used to validate command args and call correct method on ClientService
pkg/client_service - Used to integrate with grpc generated client code

The project can be built by make, which will:

Download dependencies
Generate grpc code
Build the cli executable
Generate mocks
Run automated tests

Let's walk through the code and discuss.

added: CommandParser and associated tests

added: common package for logging

fixed: send PrintUsage() output from flag package to custom logger for cleaner unit test output

added: testing for the ClientService

knisbet · 2019-01-26T05:10:29Z

Makefile

+	@rm -rf ./test/common/mocks/*
+
+deps:
+	@echo syncing dependencies...


I would generally recommend to vendor the dependencies, creates less risk that upstream changes will break things.

We generally use dep (https://github.com/golang/dep), but vgo (https://github.com/golang/go/wiki/vgo) is the current hotness, and there are a few other tools that are all good.

Yes, I agree. I have used Govendor in the past, but I wanted to keep it simple for the prototype. I'll check out vgo.

knisbet · 2019-01-26T05:14:05Z

Makefile

+
+.PHONY: test
+
+all: | clean deps gen_protobuf build gen_mocks test


Nit: missing Phony on most the targets

knisbet · 2019-01-26T05:23:36Z

pkg/cli/client_service.go

+}
+
+type GrpcClientService struct {
+	ctx context.Context


Nit: the recommendations for using context are not to embed within a struct. So I would either refactor to be within recommendations, or document this as an intended deviation.

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:
https://golang.org/pkg/context/

You are correct, I'll refactor.

knisbet · 2019-01-26T21:51:11Z

pkg/model/status/Status.go

+
+var enums []string
+
+func New(str string) Status {


Since this is basically an Enum implementation being passed over GRPC as a string, why not just define the enum in the protobuf, and use it without conversions? https://developers.google.com/protocol-buffers/docs/proto#enum

That looks like a good improvement. I just used the boilerplate enum code that I've used before here. The smaller amount of space used in the protobuf message payload would be a nice advantage here.

knisbet · 2019-01-26T21:57:58Z

pkg/cli/log_reader.go

+}
+
+// Read handles filling the buffer from the LogClient
+func (lr *LogReader) Read(p []byte) (n int, err error){


I'm not sure emulating the Read interface is your best bet here. This feels like it could be significantly simpler, if it just emulates / exposes the interface provided by gRPC, or were to just return a string,error. I'm not sure what storing in an intermediate buffer really buy's here?

I think you are right. Returning a string will make this a simpler interface.

knisbet · 2019-01-26T22:09:40Z

pkg/cli/log_reader.go

+		lr.buffer.WriteString(resp.LogMessages)
+	}
+
+	return lr.buffer.Read(p)


This will work as intended by the implementation of buffer, but I think it's a bit risky, because it seems to rely on an implementation detail of Buffer.Read.

Based on https://golang.org/pkg/io/#Reader

When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call. An instance of this general case is that a Reader returning a non-zero number of bytes at the end of the input stream may return either err == EOF or err == nil. The next Read should return 0, EOF.

So if this were to get changed to a different implementation due to code maintenance that returns the Read + io.EOF in the same call, this will potentially break the implementation since it will return an io.EOF when there is more to read from the upstream gRPC server.

Good catch! I will change the code to handle that situation correctly.

knisbet · 2019-01-26T22:20:48Z

protobuf/master/master.proto

+}
+
+message StartResponse {
+    string error = 1;


I don't think you need to define an error message on top of the protobuf messages, gRPC will pass the errors at the RPC protocol layer. You don't need to do it this way, but this doc has some good error handling advice: https://jbrandhorst.com/post/grpc-errors/

I was wondering about that! I will switch it over to use the recommended method of error propagation.

klizhentas · 2019-01-26T21:16:40Z

Makefile

@@ -0,0 +1,41 @@
+


You may want to remove binary files from the PR

klizhentas · 2019-01-26T21:18:50Z

Makefile

+	@rm -rf ./test/cli/mocks/*
+	@rm -rf ./test/common/mocks/*
+
+deps:


why not use golang's standard dependency manager? e.g dep or vgo

I have used govendor in the past, and simply did it this way because it was faster for getting the prototype going. I am all for dependency management :-)

klizhentas · 2019-01-26T21:19:12Z

Makefile

+	@go get -u google.golang.org/grpc
+	@go get -u github.com/golang/protobuf/protoc-gen-go
+
+build: build_cli


.PHONY for build targets?

klizhentas · 2019-01-26T21:20:00Z

Makefile

+	@${GOPATH}/bin/mockery -all -dir pkg/master -output test/master/mocks -case=underscore
+	@${GOPATH}/bin/mockery -all -dir gen/protobuf/master -output test/master/mocks -case=underscore
+
+gen_protobuf:


this is not reproducible - go get can render various versions and grpc is not pinned.

Can you elaborate? I am not quite following. Are you referring to this line under deps:
go get -u google.golang.org/grpc

klizhentas · 2019-01-26T21:20:50Z

cmd/go-scheduler-cli/main.go

@@ -0,0 +1,33 @@
+package main


We recommend to follow these guidelines for consistency:

https://github.com/golang/go/wiki/CodeReviewComments

I haven't seen this before. I will give it a read.

klizhentas · 2019-01-26T21:29:29Z

pkg/common/closer.go

@@ -0,0 +1,5 @@
+package common
+
+type Closer interface {


io.Closer is exactly this interface, what's the purpose of this one?

I was thinking "don't expose third party things in my lib's public API", but I am now realizing this is in the standard library, so it's fine to use io.Closer instead. Will change.

klizhentas · 2019-01-26T21:34:29Z

pkg/cli/command_parser.go

+	startCommand := flag.NewFlagSet("start", flag.ContinueOnError)
+	startCommand.SetOutput(cp.logger)
+
+	startImage := startCommand.String("image", "", "The docker image name (Required)")


there are libraries like cobra that make it slightly easier to manage required arguments.

klizhentas · 2019-01-26T21:34:54Z

pkg/cli/command_parser.go

@@ -0,0 +1,184 @@
+package cli


appreciate the split into cli and models, this is helpful

klizhentas · 2019-01-26T21:35:59Z

pkg/cli/log_reader.go

+type LogReader struct {
+	logClient master.Master_LogClient
+	closer io.Closer
+	buffer    *bytes.Buffer


bytes.Buffer is not goroutine safe. You will encouter data corruption in case if one go-routine will write while another attempts to read it.

Besides, it's not clear to me what buffering the whole thing in-memory is really buying here.

I am going to change this to just Read() string,err as per your colleague's suggestion and return incremental results as they come from the gRPC client. You are correct, buffering in memory isn't buying me anything.

klizhentas · 2019-01-26T21:37:43Z

pkg/cli/client_service.go

+	client, conn, err := cs.clientFactory.CreateClient(host)
+
+	if err != nil {
+		return logReader, err


logReader is nil at this point, so explicit intent of return nil, err will be easier for to read.

kmacmcfarlane added 6 commits January 21, 2019 15:33

added: ClientService

0fd1655

added: CommandParser and associated tests

added: .gitignore for intellij IDEs

8e2cc6f

fixed: main.go compile error

bd8c3d4

added: common package for logging

added: grpc dependency

7dadf90

fixed: send PrintUsage() output from flag package to custom logger for cleaner unit test output

added: protobuf service definition and makefile targets to support it

9ed0e8d

changed: wrap up the grpc generated code a bit for testability

7a19a3e

added: testing for the ClientService

kmacmcfarlane self-assigned this Jan 24, 2019

knisbet reviewed Jan 26, 2019

View reviewed changes

klizhentas reviewed Jan 27, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cli #1

Cli #1

kmacmcfarlane commented Jan 24, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

knisbet Jan 26, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

knisbet Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

klizhentas Jan 26, 2019

klizhentas Jan 26, 2019

klizhentas Jan 27, 2019

kmacmcfarlane Jan 27, 2019

klizhentas Jan 26, 2019

kmacmcfarlane Jan 27, 2019


		.PHONY: test

		all: \| clean deps gen_protobuf build gen_mocks test

Cli #1

Are you sure you want to change the base?

Cli #1

Conversation

kmacmcfarlane commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment