-
Notifications
You must be signed in to change notification settings - Fork 328
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 from gopkg.in to github.com, to support 'go mod' more cleanly #275
Conversation
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.
Codewise looks good but I'm far from 100% so it's best to wait for someone else
@coryb are you happy for me to merge this? |
go.mod
Outdated
@@ -15,6 +15,7 @@ require ( | |||
github.com/guelfey/go.dbus v0.0.0-20131113121618-f6a3a2366cc3 // indirect | |||
github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c // indirect | |||
github.com/jinzhu/copier v0.0.0-20180308034124-7e38e58719c3 | |||
github.com/karalabe/xgo v0.0.0-20190301120235-2d6d1848fb02 // indirect |
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.
dont think we need this dep, it is only for our cross compile, not sure how to exclude it from the go.mod
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.
i've removed this as part of the clean-up rebase.
I have only limited use of go modules, but it seems like to use non-released code you can use the |
Hmm, i'd missed that. D'oh. I'll give it a try (I'm trying to port go-jira-ui over to use go-jira.v1, for context). I'll pull the fix for #228 into its own PR. What's your thoughts generally on moving away from gopkg.in though @coryb ? My understand is that modules kinda removes the need for it, but 🤷 I'm not really expert enough to comment bar that. |
I am not sure of the complications of moving away from gopkg.in, I like the idea of removing the layer of indirection, but I think it will also break existing users using go modules given comments from this other PR #267. I think the usage of this is very low, like just admins on this project, so I think we can make this change without to many complications. Worst-case we can revert if we end up breaking people. |
I don't think we're going to affect folk using go modules -- I'm pretty sure modules support was broken until #228 was fixed, and to use modules effectively you'd need to do the replace trick to the current github repo anyway. I disagree that we need to bump the major version in order to shift away from gopkg.in (re #267) - it's not really a breaking change, it just requires an update to your imports if you wish to use future versions - the code/API remains at v1. I say let's do it :) |
There should be no reason to use gopkg.in versioned imports now that we're using go modules. I think, IANAE. gopkg.in kind of gets in the way of modules, as it only pulls over tagged releases from github.com -- this then means that you need to use go modules 'replace' syntax in the go.mod to use a non-versioned commit or branch. This is feasible, but kind of ugly. go modules defaults to pulling the latest version, so the default behavior is the same as when pulling go-jira.v1 from gopkg.in.
937f08a
to
27f57b2
Compare
bc503a0
to
27f57b2
Compare
I'm going to hold off on merging this until I've had a chance to check it with Go 1.13 too -- I've heard it surfaces some module problems that can take a bit of unpicking. |
You can try to update the .travis.yml from:
to:
That should let travis test for you |
I'm not 100% on this PR, so would really like a second pair of eyes from someone who's used to using modules.
This PR removes reference to the old repository and in particular the use of gopkg.in as an import source - it appears to have been preventing go module support from being useful for pinning against a non-released SHA or branch. Essentially, since gopkg.in only pulls vX.Y.Z tagged releases from github, there's nothing there to pull from if you want to use the 'go get {repo}@{git-sha} syntax.
In the process of this, I noticed that #228 was still a problem, despite the move to using '_t' as the prefix for the cli tests. I implemented a temp-file fix to address the problem, so it should now work - at least the tests pass.