-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Migrate project to go modules #2098
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2098 +/- ##
=======================================
Coverage 96.29% 96.29%
=======================================
Files 214 214
Lines 10535 10535
=======================================
Hits 10145 10145
Misses 331 331
Partials 59 59 Continue to review full report at Codecov.
|
Makefile
Outdated
sed -i.bak 's|"zipkincore"|"$(PROJECT_ROOT)/thrift-gen/zipkincore"|g' $(THRIFT_GEN_DIR)/agent/*.go | ||
sed -i.bak 's|"jaeger"|"$(PROJECT_ROOT)/thrift-gen/jaeger"|g' $(THRIFT_GEN_DIR)/agent/*.go | ||
sed -i.bak 's|"zipkincore"|"github.com/jaegertracing/jaeger/thrift-gen/zipkincore"|g' $(THRIFT_GEN_DIR)/agent/*.go | ||
sed -i.bak 's|"jaeger"|"github.com/jaegertracing/jaeger/thrift-gen/jaeger"|g' $(THRIFT_GEN_DIR)/agent/*.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we keep using var instead of explicit path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I have changed it back
github.com/golang/protobuf/protoc-gen-go \ | ||
github.com/gogo/protobuf/protoc-gen-gogo \ | ||
github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \ | ||
github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does go install
work with modules? Is it going to install the version from go.mod?
Perhaps we can wait for @annanay25 to change the gen to use Docker image, then we won't have to do these things here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does go install work with modules? Is it going to install the version from go.mod?
That is my understanding.
Perhaps we can wait for @annanay25 to change the gen to use Docker image, then we won't have to do these things here.
I don't want to block this PR on that. That can be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it going to install the version from go.mod?
That is my understanding.
Can we find some evidence to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance version of github.com/sectioneight/md-to-godoc
is defined in go.mod
. The never version does not work due to conflicts. I specified the older version explicitly like we had in dep
.
// https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module | ||
import ( | ||
_ "honnef.co/go/tools/cmd/staticcheck" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why blank line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our auto-formatting magic :)
|
||
package jaeger | ||
|
||
// https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
@yurishkuro PR updated based on the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavolloffay - I've created #2102, which should be merged soon. Can you please rebase this on top of that PR?
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
star |
As it is approved and the build is passing I will merge. Feel free to comment after the merge I will apply fixes in subsequent PR(s). |
Resolves #2019
Supersedes: #1546