-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add tools as go modules #927
Conversation
Welcome @jukie! |
/assign @tariq1890 |
I have seen this practice lately, but I'm unsure how I feel about it. What benefits does this actually have? |
One thing I see is that it pins module versions between different machines so it removes another avenue of the “works on my machine” issue. |
I’m still new at this but it might also help keep your build cleaner according to this - https://stackoverflow.com/questions/56842385/using-go-get-to-download-binaries-without-adding-them-to-go-mod/57313319#57313319 |
hmm .. especially with #903, we just introduced two more rather large tools, I'm not sure that's something that I always want to have checked into the repo. Wdyt @tariq1890 @lilic ? |
It's a separate package so it wouldn't be included in the builds if that's one of the concerns, but if you're checking out the repo isn't it also expected that you'd want to include the build tools? I'm trying to understand what the negatives would be. Doesn't the current local workflow include a |
We check in the vendor directory which is what concerns me in terms of checking in the dependency code as well blowing up the repo size. |
This is highly debated topic (golang/go#25922) but where else should those dependencies be stored? It's certainly not my call, so I can make whatever changes you'd like. |
@brancz I am in favour of tracking tool dependencies this way since we also have better control over which version we'd like to import. Since it's a recommended practice by the Go team, we can follow this. We can also look into removing the |
Ok. Not too strong of an opinion, so I'm ok with it. This does need a rebase and add the new tools added in #903 . |
@brancz |
I'm not yet sure why this is segfaulting
|
The |
Alright it's actually ready this time, sorry about that. |
Thank you for your PR @jukie. Just one review comment needs addressed. Once that is done, please squash the commits. |
Which review comment is that? I applied your suggestion of removing |
The copyright header in tools.go has the wrong year |
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.
/lgtm
/approve
Thank you :) @jukie
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jukie, tariq1890 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Follows go best practices. Imports tool dependencies as go modules instead of with "go get"
Which issue(s) this PR fixes:
Fixes #918