Use go mod #2314

wxing1292 · 2019-07-30T05:39:23Z

Modify Makefile accordingly

After this PR is landed, Cadence repo can live at anywhere.
For development, please move the Cadence repo out of GOPATH

* Modify Docker files and Makefile accordingly

shreyassrivatsan · 2019-07-30T20:22:10Z

let's land this after you are back..

wxing1292 · 2019-07-30T20:36:42Z

let's land this after you are back..

sure

sagikazarmark · 2019-08-01T02:42:45Z

Makefile

-	dep ensure
+go-mod:
+	go mod tidy
+	go mod vendor


This is only necessary if you want to vendor dependencies and commit the vendor directory. Since you are not doing it with dep, I'm guessing you don't really need it.

ah, that is true, previously the yarpc was using something from vendor dir

yarpc-install: go get './vendor/go.uber.org/thriftrw' go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc'

basically the orioginal code was trying to use the same version of thriftrw and thriftrw-plugin-yarpc for code generation.

the thing is, go get './vendor/go.uber.org/thriftrw' can work
however go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc' cannot, due to missing dir (probably due to something similar to golang/go#32504).

any suggestion?

If you want to make sure that the same version is used for both libraries, you should probably use the replace directive

that is not what I meant, not version control.

the thriftrw and thriftrw-plugin-yarpc are both tool dependency, and previously go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc cannot be successfully imported due to it being a main package

I think the correct solution is tools dependency

Ah, okay, got it.

I would consider using a tool like gobin then. (I'm personally not a huge fan of the tool dependency pattern)

Here is an example: https://github.com/sagikazarmark/modern-go-application/blob/master/Makefile#L186-L202

sagikazarmark · 2019-08-01T02:43:58Z

Makefile

-	./install-dep.sh
-	dep ensure
+go-mod:
+	go mod tidy


Go modules are installed automatically when you run go build or go test. There is no need to run tidy every time.

sagikazarmark · 2019-08-01T02:44:42Z

Makefile


 yarpc-install:
-	go get './vendor/go.uber.org/thriftrw'
-	go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc'
+	go get 'go.uber.org/thriftrw'


FYI This will actually update the dependencies to their latest version every time you run this.

sagikazarmark · 2019-08-01T02:45:00Z

Makefile

@@ -97,38 +97,38 @@ thriftc: yarpc-install $(THRIFTRW_GEN_SRC)
 copyright: cmd/tools/copyright/licensegen.go
 	GOOS= GOARCH= go run ./cmd/tools/copyright/licensegen.go --verifyOnly

-cadence-cassandra-tool: dep-ensured $(TOOLS_SRC)
+cadence-cassandra-tool: go-mod $(TOOLS_SRC)


As said above, go-mod is not necessary here.

sagikazarmark · 2019-08-02T23:32:21Z

Dockerfile

-WORKDIR /go/src/github.com/uber/cadence
-
-RUN curl https://raw.githubusercontent.com/golang/dep/master/install.sh | INSTALL_DIRECTORY=/usr/local/bin sh
-COPY Gopkg.* ./


This should be replaced with go.* copied into the image and go mod download ran in order to build an image cache.

Here is a snippet from our Dockerfile:

ENV GOFLAGS="-mod=readonly" ARG GOPROXY COPY go.* . RUN go mod download

The readonly flag makes sure that no new dependencies are installed, thus making sure that there are no surprises around dependencies.

The GOPROXY arg allows using a go modules proxy. It's optional, but allows a developers to build an image with a proxy.

thanks for pointing out, the ENV GOFLAGS="-mod=readonly" and ARG GOPROXY are added.

just 2 questions

on line 10 there is a COPY . ., which should already copy the go.mod and go.sum?

why go mod download? I tested building the docker image locally, and it seems working.

In order to utilize the docker build cache, it's a best practice to place things at the beginning of the build that has the least chance to change.

In this case, by downloading dependencies separately first you can spare some time with the build when only the code changes, but not the list of dependencies (which is usually the case).

Otherwise every time something changes, you would have to download the dependencies (which takes time) even if they haven't actually changed.

Although this does not add much in a build environment (if you can't utilize the build cache), it's really helpful when building locally. It doesn't take more time to build the image this way, so this kind of optimization usually make sense.

sagikazarmark · 2019-08-02T23:32:55Z

Dockerfile

@@ -79,7 +74,7 @@ CMD /start.sh
 # Cadence CLI
 FROM alpine AS cadence-cli

-COPY --from=builder /go/src/github.com/uber/cadence/cadence /usr/local/bin
+COPY --from=builder /cadence /usr/local/bin


Suggested change

COPY --from=builder /cadence /usr/local/bin

COPY --from=builder /cadence/cadence /usr/local/bin

sagikazarmark · 2019-08-02T23:42:43Z

Makefile

 yarpc-install:
+	go mod vendor


I still think using gobin would be better here. In the same issue that you linked eariler (golang/go#32504) it seems to me that some tooling will be the final solution eventually, not tools.go.

for this issue, I need some opinion from my colleagues @shreyassrivatsan @samarabbas @dmetzgar, any idea?

basically, from my understanding, we can either use the following to ensure using the same version defined in go.mod for thriftrw and thriftrw-plugin-yarpc

yarpc-install: go mod vendor go get './vendor/go.uber.org/thriftrw' go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc'

or using gobin which will do the same, i.e. use the same version thriftrw and thriftrw-plugin-yarpc defined in go.mod

FYI, this PR will not be landed until I get back, which is roughly 1 week later.
In the meantime, I will try to do some investigation

By using gobin, those two tools won't be added to go.mod which means less dependencies. Also, you don't need go mod vendor.

By using gobin, those two tools won't be added to go.mod which means less dependencies. Also, you don't need go mod vendor.

for our use case, we prefer to use the binary from dependency go.uber.org/thriftrw and go.uber.org/yarpc (we are already depending on these 2 lib).
assuming that my understanding of gobin is correct (ref), tool dependency still needs to be declared.

so (still assuming that my understanding is correct) the difference is basically whether it is a good idea to include another dependency.

Ah, okay. If they are library dependencies as well, I guess this solution is ok.

sagikazarmark · 2019-08-02T23:44:02Z

docker/buildkite/Dockerfile

@@ -30,5 +30,5 @@ RUN go get golang.org/x/tools/cmd/cover

 # The golang Docker sets the $GOPATH to be /go


I'm guessing this comment doesn't really make sense anymore.

wxing1292 requested review from samarabbas, dmetzgar, yux0 and shreyassrivatsan July 30, 2019 05:39

Use go mod

ddd9c7b

* Modify Docker files and Makefile accordingly

wxing1292 force-pushed the go-mod branch from ac3e699 to ddd9c7b Compare July 30, 2019 06:07

wxing1292 closed this Jul 30, 2019

fix mysql dependency

18e9f97

wxing1292 reopened this Jul 30, 2019

Merge branch 'master' into go-mod

419cda5

wxing1292 requested a review from longquanzheng July 30, 2019 17:25

Wenquan Xing and others added 2 commits July 30, 2019 10:39

remove unused file

0f56b44

Merge branch 'master' into go-mod

1656a7e

shreyassrivatsan approved these changes Jul 30, 2019

View reviewed changes

h7kanna mentioned this pull request Aug 1, 2019

New Dockerfiles #2325

Merged

sagikazarmark reviewed Aug 1, 2019

View reviewed changes

wxing1292 and others added 5 commits August 1, 2019 14:33

address comments

aef8d31

fix build

811f54d

use tools dependency

b57c10d

Merge branch 'master' into go-mod

5fe19bb

Merge branch 'master' into go-mod

0d9dbce

sagikazarmark suggested changes Aug 2, 2019

View reviewed changes

wxing1292 and others added 6 commits August 3, 2019 08:17

address comments

e1f50e0

Merge branch 'go-mod' of github.com:uber/cadence into go-mod

57d2210

address comments

63bee17

Merge branch 'master' into go-mod

94d68eb

address comments

190a386

Merge branch 'go-mod' of github.com:uber/cadence into go-mod

d06b586

wxing1292 and others added 4 commits August 3, 2019 16:50

address comments

6695aa2

address comments

7648fd2

Merge branch 'master' into go-mod

6902612

update go mod / sum

b761811

wxing1292 merged commit dcef9dc into master Aug 14, 2019

wxing1292 deleted the go-mod branch August 14, 2019 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use go mod #2314

Use go mod #2314

wxing1292 commented Jul 30, 2019 •

edited

Loading

shreyassrivatsan commented Jul 30, 2019

wxing1292 commented Jul 30, 2019

sagikazarmark Aug 1, 2019

wxing1292 Aug 1, 2019

sagikazarmark Aug 1, 2019

wxing1292 Aug 1, 2019

sagikazarmark Aug 1, 2019

sagikazarmark Aug 1, 2019

sagikazarmark Aug 1, 2019

sagikazarmark Aug 1, 2019

sagikazarmark Aug 2, 2019

wxing1292 Aug 3, 2019 •

edited

Loading

sagikazarmark Aug 3, 2019

wxing1292 Aug 3, 2019

sagikazarmark Aug 2, 2019

sagikazarmark Aug 2, 2019

wxing1292 Aug 3, 2019 •

edited

Loading

sagikazarmark Aug 3, 2019

wxing1292 Aug 3, 2019

sagikazarmark Aug 5, 2019

sagikazarmark Aug 2, 2019

	COPY --from=builder /cadence /usr/local/bin
	COPY --from=builder /cadence/cadence /usr/local/bin

		@@ -30,5 +30,5 @@ RUN go get golang.org/x/tools/cmd/cover

		# The golang Docker sets the $GOPATH to be /go

Use go mod #2314

Use go mod #2314

Conversation

wxing1292 commented Jul 30, 2019 • edited Loading

shreyassrivatsan commented Jul 30, 2019

wxing1292 commented Jul 30, 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

wxing1292 Aug 3, 2019 • edited Loading

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

wxing1292 Aug 3, 2019 • edited Loading

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

wxing1292 commented Jul 30, 2019 •

edited

Loading

wxing1292 Aug 3, 2019 •

edited

Loading

wxing1292 Aug 3, 2019 •

edited

Loading