Migration from CircleCI1.0 to CircleCI2.0 by selansen · Pull Request #2168 · moby/libnetwork

selansen · 2018-05-29T20:57:09Z

This commit will allow us to use newer version of CircleCI.

Signed-off-by: selansen elango.siva@docker.com

ctelfer

This doesn't seem ready to be merged right? There are various commented sections in the config.yml and Makefile that presumably should be either uncommented or removed.

ctelfer · 2018-05-30T19:50:02Z

+ exclusive: false
+ - run: make circle-ci
+ #- run: make build-builder
+ # - run: mkdir -p images


I'm a bit confused here w.r.t. the build-builder target. It's added to the Makefile below, but it seems to be commented out here. Is it needed or not?

selansen · 2018-05-30T21:17:55Z

@ctelfer , This is still WIP. Forgot to add it on the title. Modified it now.

selansen · 2018-06-12T01:05:32Z

Now it is ready for review.

thaJeztah · 2018-06-12T03:59:48Z

+FROM golang:1.10
+RUN apt-get update && apt-get -y install iptables
+
+RUN go get github.com/tools/godep \


don't think godep is still used in this repo

will remove it and test it once to make sure there is no failure.

fcrisciani · 2018-06-12T04:20:37Z

+ - image: 'circleci/golang:1.10'
+ environment:
+ Dockerfile: Dockerfile.ci
+ dockerargs: --privileged -e CIRCLECI


not sure if here we need privileged

if we dont run container without privileged option, lot of testing which invoke system calls failed. I already came across during my testing

fcrisciani · 2018-06-12T04:22:45Z

-./bin/dnet:
-	make build
-
 coveralls:


I guess we can remove coveralls

fcrisciani · 2018-06-12T04:23:43Z

-all-local: build-local check-local integration-tests-local clean
+all-local: build-local check-local clean

-${build_image}.created:


this was convenient locally to avoid building all the time the image. In theory the cache should do the job but we should give it a try

This commit will allow us to use newer version of CircleCI. Signed-off-by: selansen <elango.siva@docker.com>

- cleaned the make check - local build do not require context Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani · 2018-06-16T18:06:16Z

@ctelfer @euanh can you guys give it try to see that your local development flow on linux is not affected?
Only thing that I noticed and is also mentioned in this old commit is that the ctrl+c is not working properly for example on the make check (2528ec5). Have to understand better what is going on, but a part from that everything looks good locally on my mac.

fcrisciani

Let's do this

thaJeztah

LOL, noticed I had some review comment pending 😅

thaJeztah · 2018-06-16T19:43:51Z

+
+WORKDIR /go/src/github.com/docker/libnetwork
+
+COPY . .


Looks like this Dockerfile is exactly the same as the other, except for the COPY, correct?
Perhaps it's better to make it a multi-stage build, and for dev specify the target (docker build --target=dev)?

have to check if the engine version of circle CI supports the multi stage builds

thaJeztah · 2018-06-16T19:46:17Z

-RUN go get github.com/tools/godep \
- github.com/golang/lint/golint \
+RUN go get github.com/golang/lint/golint \
 golang.org/x/tools/cmd/cover \


Just thinking of this, but I don't think combining all these in a single RUN provides any space saving (because there's no overlap and/or files being removed afterwards).
You could split these all into their own RUN

go get is called one time with multiple arguments, so maybe can do the fetch in parallel or something, instead splitting it will force to have 1 call per argument. Would keep this as is, hoping that go get can do some magic :D

selansen · 2018-06-19T18:39:29Z

I saw only one comment from you last week and addressed it :) 1) We have only two targets, local machine and circleCI. For CircleCI mount doesn't work anymore. So we are using old dockerfile as it is for local and using new Dockerfile.ci for circleCI. makefile will choose right one depending on target. 2) Will spilt the run command as per your suggestion.

…

On Tue, Jun 19, 2018 at 2:31 PM, Sebastiaan van Stijn < ***@***.***> wrote: ***@***.**** commented on this pull request. LOL, noticed I had some review comment pending 😅 ------------------------------ In Dockerfile.ci <#2168 (comment)>: > @@ -0,0 +1,13 @@ +FROM golang:1.10 +RUN apt-get update && apt-get -y install iptables + +RUN go get github.com/golang/lint/golint \ + golang.org/x/tools/cmd/cover \ + github.com/mattn/goveralls \ + github.com/gordonklaus/ineffassign \ + github.com/client9/misspell/cmd/misspell \ + honnef.co/go/tools/cmd/gosimple + +WORKDIR /go/src/github.com/docker/libnetwork + +COPY . . Looks like this Dockerfile is exactly the same as the other, except for the COPY, correct? Perhaps it's better to make it a multi-stage build, and for dev specify the target (docker build --target=dev)? ------------------------------ In Dockerfile.build <#2168 (comment)>: > @@ -1,10 +1,11 @@ FROM golang:1.10 RUN apt-get update && apt-get -y install iptables -RUN go get github.com/tools/godep \ - github.com/golang/lint/golint \ +RUN go get github.com/golang/lint/golint \ golang.org/x/tools/cmd/cover \ Just thinking of this, but I don't think combining all these in a single RUN provides any space saving (because there's no overlap and/or files being removed afterwards). You could split these all into their own RUN — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2168 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKEKnw8Dg9bUbDg9li75WhR_-1tdWentks5t-UOUgaJpZM4USNc_> .

selansen force-pushed the circle2.0 branch from 8e7f3af to 205d8abCompare May 29, 2018 21:05

ctelfer reviewed May 30, 2018
View reviewed changes

selansen force-pushed the circle2.0 branch from 205d8ab to 025f6ecCompare May 30, 2018 20:36

selansen changed the title ~~Migration from CircleCI1.0 to CircleCI2.0~~[Work In Progress] Migration from CircleCI1.0 to CircleCI2.0May 30, 2018

selansen force-pushed the circle2.0 branch 25 times, most recently from f328905 to 8351d54Compare May 31, 2018 21:50

selansen force-pushed the circle2.0 branch 15 times, most recently from 13a699b to 8f61d48Compare June 7, 2018 22:00

thaJeztah reviewed Jun 12, 2018
View reviewed changes

fcrisciani reviewed Jun 12, 2018
View reviewed changes

fcrisciani mentioned this pull request Jun 15, 2018
[WIP] migration to circleci 2.0 #2107
Closed

selansenand others added 2 commits June 16, 2018 11:03

Migration from CircleCI1.0 to CircleCI2.0
2df9b0a
This commit will allow us to use newer version of CircleCI. Signed-off-by: selansen <elango.siva@docker.com>

Further makefile cleanup
27aa67c
- cleaned the make check - local build do not require context Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

euanh approved these changes Jun 19, 2018
View reviewed changes

fcrisciani approved these changes Jun 19, 2018
View reviewed changes

thaJeztah reviewed Jun 19, 2018
View reviewed changes

thaJeztah mentioned this pull request Jun 26, 2018
[18.03] Migration from CircleCI1.0 to CircleCI2.0 #2207
Merged

euanh mentioned this pull request Jul 10, 2018
Makefile: Run containers with --init for proper signal handling #2228
Merged


		WORKDIR /go/src/github.com/docker/libnetwork

		COPY . .

Uh oh!

Conversation

selansen commented May 29, 2018

Uh oh!

ctelfer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen commented May 30, 2018

Uh oh!

selansen commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcrisciani commented Jun 16, 2018

Uh oh!

fcrisciani left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen commented Jun 19, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants