Skip to content

Convert the circle config file to v2 format, as Circle v1 is deprecated#2659

Merged
dperny merged 1 commit intomoby:masterfrom
cyli:circle-v2
Jul 5, 2018
Merged

Convert the circle config file to v2 format, as Circle v1 is deprecated#2659
dperny merged 1 commit intomoby:masterfrom
cyli:circle-v2

Conversation

@cyli
Copy link
Copy Markdown
Contributor

@cyli cyli commented Jun 6, 2018

And will be removed at the end of August: https://circleci.com/blog/sunsetting-1-0/

This also bumps the go version up to 1.10.2, superceding #2636 just because these PRs will otherwise conflict.

Unfortunately when we use the docker executor, the flakiness of our tests are exacerbated so we're using the machine executor for now.

@cyli cyli force-pushed the circle-v2 branch 24 times, most recently from 361f848 to eef0869 Compare June 6, 2018 23:58
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 7, 2018

Codecov Report

Merging #2659 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #2659     +/-   ##
=========================================
- Coverage   61.76%   61.67%   -0.1%     
=========================================
  Files         134      134             
  Lines       21760    21760             
=========================================
- Hits        13440    13420     -20     
- Misses       6871     6890     +19     
- Partials     1449     1450      +1

@cyli cyli force-pushed the circle-v2 branch 5 times, most recently from 868c39d to 1da9524 Compare June 7, 2018 17:13
@cyli cyli changed the title [WIP] Convert the circle config file to v2 format, as Circle v1 is deprecated Convert the circle config file to v2 format, as Circle v1 is deprecated Jun 7, 2018
@thaJeztah
Copy link
Copy Markdown
Member

I see CI is green; is this ready for review?

@cyli
Copy link
Copy Markdown
Contributor Author

cyli commented Jun 20, 2018

@thaJeztah Yes

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some nits and questions. Don't have much knowledge about these files, so take them with a pinch of salt.

ping @dnephin - perhaps you have some tips/tricks up your sleeve

Comment thread .circleci/config.yml
version: 2
jobs:
build:
working_directory: ~/go/src/github.com/docker/swarmkit
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this use $GOPATH, or is that not setup when this is used? (otherwise we could use $HOME/go/src/.... for consistency?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately apparently in the new circle version environment variables aren’t interpolated in the working_directory entry :|

Comment thread .circleci/config.yml Outdated
# Needed to install go
OS: linux
ARCH: amd64
GOVERSION: 1.10.2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 1.10.3 now 😃

Comment thread .circleci/config.yml
PROTOC: https://github.com/google/protobuf/releases/download/v3.5.0/protoc-3.5.0-linux-x86_64.zip

# Note(cyli): We create a tmpfs mount to be used for temporary files created by tests
# to mitigate the excessive I/O latencies that sometimes cause the tests to fail.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the latency due to files being written in the container's (CoW) filesystem, or just due to physical disk instead of memory (tmpfs) being used? Wondering if a VOLUME /some/path would be resolving the latency sufficiently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did this in CircleCI V1 as well due to those issues, so possibly just whatever is running the CI's disk may be too slow?

Comment thread .circleci/config.yml
wget -O "$HOME/$(basename $PROTOC)" "$PROTOC"
unzip -o "$HOME/$(basename $PROTOC)" -d "$HOME"
sudo cp -R "$HOME/include/google" /usr/local/include
sudo chmod 777 -R /usr/local/include/google
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess it doesn't matter much, just curious if we know the user that needs access, and if we should make this more granular

Copy link
Copy Markdown
Contributor Author

@cyli cyli Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's just circleci, but I'd rather not have to change this file if CircleCI ever changes that.

Comment thread .circleci/config.yml Outdated

- run:
name: Push coverage info to codecov.io
command: bash <(curl -s https://codecov.io/bash)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use -fsSL here?

Comment thread .circleci/config.yml Outdated
command: |
sudo rm -rf /usr/local/go
rm -rf "$GOPATH"
wget -O "$HOME/go.tar.gz" "https://storage.googleapis.com/golang/go$GOVERSION.$OS-$ARCH.tar.gz"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: noticed we're using a mixture of wget and curl in this file 😇

@thaJeztah
Copy link
Copy Markdown
Member

once merged; this (or a variation on this) should be cherry-picked into the release-branches

@cyli cyli force-pushed the circle-v2 branch 2 times, most recently from ffae2da to 837577b Compare June 22, 2018 00:20
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - CI is green, so 😄

Comment thread .circleci/config.yml
# One possible hack is the following:

# /dev/shm in the container is tmpfs although files in /dev/shm are not executable.
# If we specify TMPDIR=/dev/shm, /dev/shm will be used by our tests, which call
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was going to suggest, but it seems you've already tried it.

and will be removed in August 2018.

Signed-off-by: Ying Li <ying.li@docker.com>
@dperny dperny merged commit 199cf49 into moby:master Jul 5, 2018
@cyli cyli deleted the circle-v2 branch July 5, 2018 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants