-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
gomod #440
gomod #440
Conversation
I may be missing something but why not just add the v3 tag? Update I was missing something, just read the fine print here: |
The CI failures here are from go1.7 and 1.8. Versions prior to go1.9.7 do not support minimal module compatibility. Additionally, at this point, these versions are well outside the window of support from the golang team anyway. Only the 2 most recent major versions are supported. Is it a requirement to continue to support these older versions? |
I'm glad to support Go Modules. Wouldn't it be okay to place only one go.mod at the top level of the repository like the following. I think that is easier to understand. In this way, we just only add v3.0.0 tag. Certainly, redisx is an experimental package, so it's hard to follow the semantic version and keep compatibility. But, isn't this package exceptionally acceptable for incompatible fixes? |
It looks like that would work just fine. Please go that route if it's your preference.
I'm not certain what you mean, here, but I think you're asking if it's OK to not semantically version
|
Remove travis validation against go prior to 1.9.x to allow v3 tagging to take place: #440
Ok I think we've let this one sit long enough. I've just put a PR to remove 1.7.x and 1.8.x validation which had their last release at the beginning of 2017. If anyone has any objections to this please say now, otherwise I intend to merge: #465 and then this PR which I'm hoping @dcormier can rebase. Please use 👍 or 👎 to express your preference! |
Rebased. |
Is there anything else preventing this from moving forward? |
Just giving people time to respond, its only been a few days; end of the week or beginning of next. |
Remove travis validation against go prior to 1.9.x to allow v3 tagging to take place: #440
Remove travis validation against go prior to 1.9.x to allow v3 tagging to take place: gomodule#440
So I've been testing this morning how this impacts existing projects and I must be missing something as without changing consumer import to includes the
This is a terrible user experience as it will literally break everyones projects, did I miss something? |
No, you didn't miss something. Unfortunately, that is how gomod works. The docs linked in my comment at the beginning of this PR lay it out. Unfortunately, they are lengthy. But since this repo has version tags, the repo has to play by gomod's rules if consumers are to get the version of this package that one would expect. As a reminder, the state this repo is currently in is that if you're using a supported version of go (currently go1.13+) and go a You have to jump through additional hoops to get the most recently tagged version of this package, anyway. |
I can corroborate that, tried to |
I was afraid of that, I think the guide could do with clarifying when it comes to the user side as it gives the impression it will just work which is not the case. I kind of expected it given the internal URL's had to change but it's definitely not 100% clear. So given this I think we need to get an update to this to detail the changes people need front an center in the README, seems like the only tool out there that that does this currently is https://github.com/marwan-at-work/mod, there's thread here about making a core tool. Is this something you have some time do to @dcormier? |
I've updated the readme to give the correct import path and docs link for once the If you'd like some additional text, feel free to provide some. You can either reply with it (and where you want it) and I'll update the readme in this PR, open another PR against my branch and I'll merge it, or just update it after this PR is merged. For what it's worth, as a golang module user, I don't expect every module author to be providing instructions on how to make the tools consume the latest major version upgrade. I just expect repos to be tagged properly, and, if it's present, to show the correct import path in their readme or package-level godoc. It's on me, a golang module user, to know that I have to update my import paths to take major version upgrades. Relevant docs here:
And, in the same section:
Available versions for a module are discoverable in a repo itself, and on pkg.go.dev (example, another). |
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.
Thanks for doing this I've added a suggestion to help our users upgrade to this new way of working. I agree for new users the go get ...
is all that's needed but given this will come as a shock for existing consumers detailing out how they can easily fix this I think is important.
Fixes gomodule#366
Thanks @dcormier I've asked some colleagues to review, make sure I've not missed something. Once that's done I'll look to get it merged. |
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
Thanks to bmills in the other thread, looks like go 1.14 has an enhancement that's relevant here: With 1.14 and later we could enable |
If it works, that's the easy way, for sure. |
Feels like we should go that route, its less compatible needs people to be on 1.14 but with that it totally eliminates the need for switching to v3 which feels like a pain for a huge amount users. Thoughts? |
More information:
So why does the explicit version work but latest doesn't? Note that the hash latest is picking up (e14091d) is older than v1.8.0 (898fbd0) I have the horrible feeling that now the proxy has seen a |
It works because you are asking for a package at a given vcs tag: at
Yeah, I was wanting confirmation from @bcmills to see if this was a known issue. But that is the way it looks, once you go submodule you cannot go back? That being said, this exact behaviour does seem like a bug, since an untagged, pre-release |
Indeed, any small slip and its impossible to recover, which is far from ideal. |
The fix for the accidental nested modules is to remove the packages from the “latest” version of them, so that the One easy way to do that is with a (presumably off-master) commit that provides trivial |
You might also be able to achieve the same effect by changing the |
Have tried this approach but didn't seem to work. To clarify I created a branch with just a I then used
Then tried to get the mainline (v1.8.0)
This is still picking up the original nested Did this agree with the process you had in mind @bcmills ? |
Also seeing the following if I use
|
@stevenh, here's what I see at the moment:
So the
|
I think there is a problem with the |
Aha! I think I've got it. You can tag commit Then that tagged version will show up in https://proxy.golang.org/github.com/gomodule/redigo/redis/@v/list, and the |
I apologize for this mess. I know it wasn't working right before (with gomod picking up the old v2.0.0 tag), but I shouldn't have put this forward. I should have just left it as an issue. Feel free to completely ignore this and not respond to it at all: <rant> We're suddenly forced to adopt it in some cases to get packages to work right, but it has so many gotchas that it can be tricky to adopt for a package that existed prior to gomod. And if you don't know all the rules, it's easy to break them and end up in a bad situation with no obvious way out. And gomod suddenly changes the behavior of some preexisting mechanisms (like git tags). Golang users shouldn't be reliant on members of the golang team to jump in and give direction. It's great that they do that, and I very much appreciate it, but that it's needed seems like a bad sign. Not to mention that it provides no way to discover that a new major upgrade is available for a package. I miss |
@bcmills, does |
@dcormier, to be fair, the Wiki recommends against multi-module repos, and https://blog.golang.org/migrating-to-go-modules explicitly says “If you have existing version tags, you should increment the minor version.” (And yeah, we know the reference documentation needs work. That's golang/go#33637, and @jayconrod in particular is actively working on it.) |
|
To confirm my understanding I need to tag create the two tags |
Correct. The proxy won't remove the old records per se, but it will permanently cache the existence of those versions (and the contents at those versions), and the |
Looks like we have a winner!
Thank you so much for your help @bcmills it would have taken ages to fix that without your help! Feels like this is a new |
Help what is happening.
That is the wrong version right? |
@domino14, you need to be using Go 1.14 or higher for |
@domino14 your path is wrong you want:
|
Fixes #366 (I think).
Once merged, additional tags need to be added to the repo:
redis/v3.0.0
redisx/v3.0.0
This will make this repo support go modules, and follows the official guidance of bumping the major version when there are released tags that are not actually for
go mod
.It maybe worth considering not having the
redis
andredisx
modules in the same repo. Before taking this change might be a good opportunity to do that since consumers will have to change the import path, anyway.At the very least, it may be worthwhile to move the
redisx
package out of this repo, since it's intended to be experimental and should probably remainv0
.