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.

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

Add support for go modules #75

Merged
merged 7 commits into from
Sep 5, 2019
Merged

Add support for go modules #75

merged 7 commits into from
Sep 5, 2019

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Sep 4, 2019 •
edited
Loading

Resolves #74

  • Switches to Go modules as the primary dependency manager
  • Remove Glide from the build's critical paths
  • Still keep Glide and Dep files for the time being, including Travis matrix steps for testing with them
  • Always test with -race
  • Use multi-package test coverage ability of go test (remove cover.sh script)

Yuri Shkuro added 5 commits September 4, 2019 13:44
Add support for go modules
99dea0e 
Signed-off-by: Yuri Shkuro <ys@uber.com>
Add GO111MODULE=on
9786934 
Signed-off-by: Yuri Shkuro <ys@uber.com>
Limit use of glide
bc29157 
Signed-off-by: Yuri Shkuro <ys@uber.com>
Do not use glide for package list
34b2b69 
Signed-off-by: Yuri Shkuro <ys@uber.com>
Add doc.go
67fcb52 
Signed-off-by: Yuri Shkuro <ys@uber.com>
@codecov
Copy link

codecov bot commented Sep 4, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #75   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          26     26           
  Lines         890    890           
=====================================
  Hits          890    890

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 fc6bea7...54515da. Read the comment docs.

Make Go modules the primary mode
959cea9 
Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro
Copy link
Member Author

cc @jpkrohling @objectiser please review

jpkrohling
jpkrohling approved these changes Sep 5, 2019
View reviewed changes
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. I have one small suggestion about gathering the parent directories containing go sources, but that can be safely ignored.

Makefile Outdated
@@ -1,11 +1,12 @@
PROJECT_ROOT=github.com/uber/jaeger-lib
PACKAGES := $(shell glide novendor | grep -v ./thrift-gen/...)
PACKAGES := $(shell go list ./... | grep -v '^github.com/uber/jaeger-lib$$' | sed 's|github.com/uber/jaeger-lib/||g' | sed 's|/.*$$||g' | sort -u | sed 's|^|./|g' | sed 's|$$|/...|g')
Copy link
Contributor

Choose a reason for hiding this comment

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

go list ./... | awk -F\/ 'NR>1 {print "./"$4"/..."}' | sort -u has the same effect. I find it more readable, but feel free to keep the chained sed if you think it's clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice! Sadly, I never learned awk to the point of day-to-day usefulness.

Simplify shell expression
54515da 
Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit b84e3e3 into jaegertracing:master Sep 5, 2019
@yurishkuro yurishkuro deleted the gomod branch September 5, 2019 20:10
@yurishkuro yurishkuro mentioned this pull request Sep 5, 2019
Closed
@sagikazarmark
Copy link
Contributor

This is going to be a bit problematic:

go get github.com/jaegertracing/jaeger-lib@v2.1.0                                                                                                                                                                           
go: finding github.com/jaegertracing/jaeger-lib v2.1.0
go: finding github.com/jaegertracing/jaeger-lib v2.1.0
go get github.com/jaegertracing/jaeger-lib@v2.1.0: github.com/jaegertracing/jaeger-lib@v2.1.0: invalid version: module contains a go.mod file, so major version must be compatible: should be v0 or v1, not v2

Go modules requires you to increase the major version and add it to the module path as /v3.

The go modules wiki contains detailed information about the process:

https://github.com/golang/go/wiki/Modules

@yurishkuro
Copy link
Member Author

Thanks for the reference. I am going to revert the change for now. I think it would be best to combine the switch to go modules with changing the path from uber to jaegertracing, and start that with v1.

@sagikazarmark
Copy link
Contributor

That sounds reasonable to me. IMHO you should also consider using vanity import paths, like jeagertracing.io/something.

@jpkrohling
Copy link
Contributor

Agree with everything, except with the vanity import path.

@sagikazarmark
Copy link
Contributor

@jpkrohling care to elaborate? IMHO all major Go projects (or at least many of them) are moving towards vanity imports (k8s, opencensus, opentelemetry to name a few).

I've heard arguments like enterprise security making things hard for vanity imports, but proxies should make that much easier.

@jpkrohling
Copy link
Contributor

I've been bitten by servers that are misconfigured or unavailable before. I'm not sure we in the Jaeger project have the bandwidth to setup/maintain this.

@sagikazarmark
Copy link
Contributor

Well, proxies also help with unavailable packages. Also, import paths can be served using static site hosting (eg. Netlify). I understand the concern, but Go undoubtedly goes into this direction.

@jpkrohling
Copy link
Contributor

If we have a maintainer that volunteers to set it up and run/maintain, I'm all for it :)

This was referenced Sep 6, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpkrohling jpkrohling jpkrohling approved these changes

@objectiser objectiser Awaiting requested review from objectiser

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

Successfully merging this pull request may close these issues.

Please support go mod
3 participants
@yurishkuro @sagikazarmark @jpkrohling