Skip to content

Just use the default GOPATH in CircleCI - #2690

Merged
anshulpundir merged 1 commit into
moby:masterfrom
cyli:circle-ci-fix
Jul 6, 2018
Merged

Just use the default GOPATH in CircleCI#2690
anshulpundir merged 1 commit into
moby:masterfrom
cyli:circle-ci-fix

Conversation

@cyli

@cyli cyli commented Jul 6, 2018

Copy link
Copy Markdown
Contributor

Apparently CircleCI sets a default GOPATH to /home/circleci/.go_workspace so go stuff won't fall back on the default of ~/go. So we have to set the environment variable for the whole job. Apparently you can use ~ but not other environment variables when setting environment variables in CircleCI v2.

Anyway, I had set a GOPATH just in case for all our test commands, but I forgot it for the dependency validation, which was why #2688 and #2689 were failing.

Edit: Nm, apparently ~ is not valid for environment variables, so I guess we go back to setting the GOPATH for every command, or just go with what CircleCI expects the go path to be I guess, which would hopefully make the circleci script more maintainable (no need to remember to set the GOPATH everywhere)

@cyli cyli changed the title Set the GOPATH for the whole job in CircleCI Just use the default GOPATH in CircleCI Jul 6, 2018
Signed-off-by: Ying Li <ying.li@docker.com>
@cyli
cyli force-pushed the circle-ci-fix branch from 60c740c to 7fc0c4f Compare July 6, 2018 06:43
@cyli

cyli commented Jul 6, 2018

Copy link
Copy Markdown
Contributor Author

Ok ssh'ed in and ran make dep-validate manually, and it succeeds now.

@thaJeztah thaJeztah left a comment

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.

Gosh, that's ugly 😞

LGTM

thanks!!

@thaJeztah thaJeztah left a comment

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.

actually; looks like it's failing now? 😢

should I open a vendor PR with this change?

@cyli

cyli commented Jul 6, 2018

Copy link
Copy Markdown
Contributor Author

I think it failed because of my ssh messing around. Trying again. (It also failed due to flakey test previously :|)

@codecov

codecov Bot commented Jul 6, 2018

Copy link
Copy Markdown

Codecov Report

Merging #2690 into master will increase coverage by 0.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2690      +/-   ##
==========================================
+ Coverage   61.76%   62.15%   +0.39%     
==========================================
  Files         134      134              
  Lines       21760    21760              
==========================================
+ Hits        13440    13526      +86     
+ Misses       6871     6787      -84     
+ Partials     1449     1447       -2

@thaJeztah thaJeztah left a comment

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.

all green again now, thanks!

LGTM

@thaJeztah

Copy link
Copy Markdown
Member

Just cherry-picked this into #2688 to test; and it works!

Comment thread .circleci/config.yml
@@ -79,21 +83,21 @@ jobs:
# The GOPATH setting would not be needed if we used the golang docker image

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: Remove this comment about GOPATH since it is no longer being set?

@anshulpundir
anshulpundir merged commit f6da67d into moby:master Jul 6, 2018
@cyli
cyli deleted the circle-ci-fix branch July 6, 2018 20:24
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