8000 Tolerate go get failures for coredns/forward by chrisohaver · Pull Request #1435 · coredns/coredns · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Tolerate go get failures for coredns/forward #1435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jan 26, 2018
Merged

Tolerate go get failures for coredns/forward #1435

merged 5 commits into from
Jan 26, 2018

Conversation

chrisohaver
Copy link
Member
@chrisohaver chrisohaver commented Jan 25, 2018 •
edited
Loading

1. What does this pull request do?

Make will tolerate failures when go-getting coredns/forward. Specifically, the failure to get coredns/coredns, when coredns/coredns is the current project.

This fix is a hack. But its IMO a workable one until we identify a better fix.

2. Which issues (if any) are related?

Fixes #1442

3. Which documentation changes (if any) need to be made?

n/a

@miekg
Copy link
Member
miekg commented Jan 25, 2018

is forward being pulled in the dir where we just checkout coredns?

@chrisohaver
Copy link
Member Author
chrisohaver commented Jan 25, 2018 •
edited
Loading

Travis builds ok... but it seems that the travis is building with a simple "go test"... which means its getting dependency heads, and building on those.
It's not being built on the same versions of dependencies checked out by that unusual "godeps" section in the Makefile (where it's checking out specific branches). We should fix that.

@chrisohaver
Copy link
Member Author
chrisohaver commented Jan 25, 2018 •
edited
Loading

Nevermind - i see that the "test" target does call "godeps"... so we should be good on the unit tests.
(it calls it as part of the "check" target ... which i was assuming was only doing "checks" for some reason)

I suppose travis build OK because its building in master... vs building in a branch in local builds and in the CI...

@chrisohaver
Copy link
Member Author

I confirmed that when building locally on my laptop, I get the same error when building from within a branch, but everything builds ok when building in master. I haven't yet figured out why that makes a difference...

@chrisohaver
Copy link
Member Author

/integration-cipr27

1 similar comment
@chrisohaver
Copy link
Member Author

/integration-cipr27

@codecov-io
Copy link
codecov-io commented Jan 26, 2018 •
edited
Loading

Codecov Report

Merging #1435 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1435   +/-   ##
=======================================
  Coverage   52.01%   52.01%           
=======================================
  Files         173      173           
  Lines        8465     8465           
=======================================
  Hits         4403     4403           
  Misses       3724     3724           
  Partials      338      338

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c56fa8d...06fd046. Read the comment docs.

@chrisohaver
Copy link
Member Author

Well - the hack above (in "chop" 206817f) makes things work with brute force.

But this wont work for local builds with uncommitted changes. To account for those we'd have to stash changes, switch to master, do the go gets, then switch back and unstash. Which, IMO, is too obnoxious.

So we need a better fix.

@chrisohaver
Copy link
Member Author
chrisohaver commented Jan 26, 2018 •
edited
Loading

Last commit tolerates failures in the go get of coredns/forward. Since the only dependent package giving trouble is coredns/coredns (which we have already), it seems OK to ignore.

Travis and coredns/ci tests look like they are doing ok with this change. Also this fix should work locally with uncommitted changes in a branch since it doesn't involve switching back to master.

It's still a hack... but IMO it's one we can work with.

@chrisohaver chrisohaver changed the title testpr3 Tolerate go get failures for coredns/forward Jan 26, 2018
@fturib fturib mentioned this pull request Jan 26, 2018
Closed
@fturib
Copy link
Contributor
fturib commented Jan 26, 2018

@miekg : are you ok to merge this fix so we can have build on local branches and ci working again ?

@johnbelamaric
Copy link
Member

Can we just remove the go get for the coredns/forward? Because this line is already there:

        (cd $(GOPATH)/src/github.com/coredns/forward 2>/dev/null          && git checkout -q master 2>/dev/null || true)

@johnbelamaric
Copy link
Member

Ok, I see now that -f basically lets coredns/coredns be a fork (or branch?), which otherwise go get checks on and rejects.

@johnbelamaric johnbelamaric merged commit 8005076 into coredns:master Jan 26, 2018
@chrisohaver chrisohaver mentioned this pull req 9338 uest Feb 13, 2018
Closed
@chrisohaver chrisohaver deleted the testpr3 branch February 13, 2018 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miekg miekg Awaiting requested review from miekg miekg is a code owner

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric johnbelamaric is a code owner

@yongtang yongtang Awaiting requested review from yongtang yongtang is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Builds from within a branch fail
5 participants
@chrisohaver @miekg @codecov-io @fturib @johnbelamaric
0