Skip to content
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.

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

Fix version of go get #1370

Merged
merged 1 commit into from Jan 10, 2018
Merged

Fix version of go get #1370

merged 1 commit into from Jan 10, 2018

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Jan 10, 2018

1. What does this pull request do?

This fix fixes version fetched from go get so that versions are guarded.

github.com/mholt/caddy              v0.10.10
github.com/miekg/dns                v1.0.3
github.com/prometheus/client_golang v0.8.0
golang.org/x/net                    release-branch.go1.9 (branch)
golang.org/x/text                   e19ae1496984b1c655b8044a65c0300a3c878dd3

2. Which issues (if any) are related?

This fix is related #1368.

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

N/A

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This fix fixes version fetched from `go get` so
that versions are guarded.

github.com/mholt/caddy              v0.10.10
github.com/miekg/dns                v1.0.3
github.com/prometheus/client_golang v0.8.0
golang.org/x/net                    release-branch.go1.9 (branch)
golang.org/x/text                   e19ae1496984b1c655b8044a65c0300a3c878dd3

This fix fixes 1368.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1370   +/-   ##
=======================================
  Coverage   51.17%   51.17%           
=======================================
  Files         171      171           
  Lines        8248     8248           
=======================================
  Hits         4221     4221           
  Misses       3701     3701           
  Partials      326      326

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 b7476d0...09bf564. Read the comment docs.

@miekg
Copy link
Member

miekg commented Jan 10, 2018

I was wondering if we should do this in code? I.e have a failing test have a version is too low?

This might be good enough though. WDYT?

@yongtang
Copy link
Member Author

@miekg I think we could use Makefile like that to make sure when we release, the versions of other packages are truly "pinned".

The version check in the code is also useful, as some developers may choose to rebuild CoreDNS with customized plugins (and not use the Makefile here).

@yongtang yongtang merged commit 2ead19f into coredns:master Jan 10, 2018
@yongtang yongtang deleted the 1368-version branch January 10, 2018 18:10
@miekg
Copy link
Member

miekg commented Jan 10, 2018

I did run into

cd /home/miek/g/src/golang.org/x/text                   && git checkout -q e19ae1496984b1c655b8044a65c0300a3c878dd3)
fatal: reference is not a tree: e19ae1496984b1c655b8044a65c0300a3c878dd3
Makefile:27: recipe for target 'godeps' failed
make: *** [godeps] Error 128
72.63s user 13.73s system 64.89s elapsed 133%CPU (make)

Maybe get get -u would be better?

@yongtang
Copy link
Member Author

@miekg I think go get -u is better. Also there are some additional issues. If repo is not a branch then go get will fail as well:

# git checkout eb22672bea55af56d225d4e35405f4d2e9f062a0
Previous HEAD position was e19ae14... currency: fix format in example
HEAD is now at eb22672... cmd/gotext: add update command
# go get -u golang.org/x/text
cd /go/src/golang.org/x/text; git pull --ff-only
You are not currently on a branch.
Please specify which branch you want to merge with.
See git-pull(1) for details.

    git pull <remote> <branch>

package golang.org/x/text: exit status 1

Added #1372 to try to address the above issue.

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.

None yet

3 participants