[go modules] migrating to go modules #1052

benjaminch · 2019-10-01T18:25:52Z

This CL migrates prebid-server to Go Modules getting rid of dep.

Issue: #998

SyntaxNode · 2019-10-08T18:36:37Z

.travis.yml

@@ -1,20 +1,13 @@
 language: go

 go:
-  - '1.11.1'
+  - '1.11.13'


Why did you change the Go 1.11.x version?

I didn't commented it, but because of golang/go#30446
1.11.4 would probably have fixed it but well, latest version cannot hurt.

Gotcha. I was taking this change to mean that the lowest version of Go now supported is 1.11.13.

What do you think of including '1.11.4' as the first build target and mentioning in the docs that's it the minimum supported version? Then adding '1.11.x' and '1.12.x' for the latest version of both of those?

Ok, let's use > 1.11.4.
1.11 will be removed soon I guess since PBS supports only the last 2 Go versions (your PR is on its way for 1.13 #1058 :))

Ah, that's a very good point.

SyntaxNode · 2019-10-08T18:38:34Z

Makefile

-install:
-	export DEP_RELEASE_TAG=v0.4.1
-	curl https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
-
 # deps will clean out the vendor directory and use dep for a fresh install


please update comment to reference go modules

Oups, Sure !

SyntaxNode · 2019-10-08T18:51:24Z

Dockerfile

 ENV CGO_ENABLED 0
 COPY ./ ./
-RUN dep ensure && \
-    go build .
+RUN go build -mod=vendor .


Why do you recommend using the vendor directory versus the default $GOPATH\pkg\mod directory?

No preferences, but IMO modules allowing to work outside of the GOPATH is one of the best features, so just wanted to take this opportunity :)

I am not opposed :), was just curious if you ran into an issue specific to building the Docker container. I like that it allows for a more targeted cache clean by the deps command.

So it's ok for you this way? Also, I might introduce in another PR later a GOPROXY for modules to speed up the build.

Yes. I don't see a problem with it.

The proxy sounds interesting. I'm curious to see what that looks like.

Hey @SyntaxNode , here you go for the proxy: #1079 :)

benjaminch · 2019-10-08T21:52:38Z

Thanks @SyntaxNode :)

stale · 2019-10-15T22:41:58Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mansinahar · 2019-10-16T13:20:48Z

README.md

@@ -18,20 +18,15 @@ For more information, see:

 ## Installation

-First install [Go 1.11](https://golang.org/doc/install) or later and [dep](https://golang.github.io/dep/docs/installation.html). Note that dep requires an explicit GOPATH to be set.
+First install [Go 1.11](https://golang.org/doc/install) latest version (1.11.4) or later.
+Note that prebid-server is using [Go modules](https://blog.golang.org/using-go-modules).


Should probably add a note here to set GO111MODULE=on if you're using Go version <1.13 and are inside GOPATH. We can remove that after we stop supporting Go versions <1.13

Yes ! I'll do :)

mansinahar · 2019-10-16T13:43:48Z

Makefile

 	@echo "  test: test prebid-server (via validate.sh)"
 	@echo "  build: build prebid-server"
 	@echo "  image: build docker image"
 	@echo ""

-.PHONY: install deps test build image


Any reason for removing the PHONY targets? Generally, it is recommended that any target in the makefile that doesn't result into an actual file with the same name should be listed as a PHONY target.

More info about PHONY targets: https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html

Oh, you are right, I just wanted to remove the deps part :)

mansinahar · 2019-10-17T15:22:23Z

Dockerfile

 COPY static static/
 COPY stored_requests/data stored_requests/data
-RUN apt-get update && \
-    apt-get install -y ca-certificates mtr && \
-    apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*


Sorry, I didn't catch this earlier but any reason to remove the ca-certificates and mtr package installation? I am not a 100% sure if we even need it in the first place. Likely not and therefore this should be fine but I just wanna confirm. Also, we should still at least keep the apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* clean up command.

LGTM otherwise. Thanks @benjaminch :)

No worries, let's keep it all then, no real reasons here :)

This CL migrates prebid-server to Go Modules getting rid of `dep`. Issue: prebid#998

mansinahar

Thank you for this @benjaminch :)

benjaminch · 2019-10-17T19:36:58Z

Thanks a lot @mansinahar and @SyntaxNode :)

benjaminch force-pushed the move-to-go-modules branch 6 times, most recently from 99593a8 to ecbaf46 Compare October 1, 2019 19:16

benjaminch marked this pull request as ready for review October 1, 2019 19:17

mansinahar self-assigned this Oct 2, 2019

SyntaxNode reviewed Oct 8, 2019

View reviewed changes

benjaminch force-pushed the move-to-go-modules branch from ecbaf46 to 7692413 Compare October 8, 2019 19:55

mansinahar assigned SyntaxNode Oct 8, 2019

benjaminch force-pushed the move-to-go-modules branch from 7692413 to fbafacb Compare October 8, 2019 21:29

SyntaxNode previously approved these changes Oct 8, 2019

View reviewed changes

stale bot added the stale label Oct 15, 2019

mansinahar reviewed Oct 16, 2019

View reviewed changes

stale bot removed the stale label Oct 16, 2019

benjaminch dismissed SyntaxNode’s stale review via 0260fee October 17, 2019 07:08

benjaminch force-pushed the move-to-go-modules branch 2 times, most recently from 0260fee to 88be9cf Compare October 17, 2019 07:13

SyntaxNode previously approved these changes Oct 17, 2019

View reviewed changes

mansinahar reviewed Oct 17, 2019

View reviewed changes

benjaminch dismissed SyntaxNode’s stale review via 012f7f8 October 17, 2019 15:47

benjaminch force-pushed the move-to-go-modules branch from 88be9cf to 012f7f8 Compare October 17, 2019 15:47

[go modules] migrating to go modules

9e5aa39

This CL migrates prebid-server to Go Modules getting rid of `dep`. Issue: prebid#998

benjaminch force-pushed the move-to-go-modules branch from 012f7f8 to 9e5aa39 Compare October 17, 2019 15:51

mansinahar approved these changes Oct 17, 2019

View reviewed changes

SyntaxNode approved these changes Oct 17, 2019

View reviewed changes

mansinahar merged commit 01fb371 into prebid:master Oct 17, 2019

SyntaxNode mentioned this pull request Oct 18, 2019

Move to Go modules? #998

Closed

mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Nov 1, 2019

[go modules] migrating to go modules (prebid#1052)

e2009bc

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020

[go modules] migrating to go modules (prebid#1052)

2e7f92a

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020

[go modules] migrating to go modules (prebid#1052)

32b189b

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 2020

[go modules] migrating to go modules (prebid#1052)

e65e38e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[go modules] migrating to go modules #1052

[go modules] migrating to go modules #1052

benjaminch commented Oct 1, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 8, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 8, 2019

SyntaxNode Oct 8, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 8, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 8, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 8, 2019

SyntaxNode Oct 8, 2019

benjaminch Oct 18, 2019

benjaminch commented Oct 8, 2019

stale bot commented Oct 15, 2019

mansinahar Oct 16, 2019

benjaminch Oct 16, 2019

mansinahar Oct 16, 2019

benjaminch Oct 16, 2019

mansinahar Oct 17, 2019

mansinahar Oct 17, 2019

benjaminch Oct 17, 2019

mansinahar left a comment

benjaminch commented Oct 17, 2019

[go modules] migrating to go modules #1052

[go modules] migrating to go modules #1052

Conversation

benjaminch commented Oct 1, 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

benjaminch commented Oct 8, 2019

stale bot commented Oct 15, 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

mansinahar left a comment

Choose a reason for hiding this comment

benjaminch commented Oct 17, 2019