nitishm / go-rejson

Golang client for redislabs' ReJSON module with support for multilple redis clients (redigo, go-redis)
MIT License
343 stars 47 forks source link

Switch over to go modules from dep for dependency management #31

Closed nitishm closed 5 years ago

nicfb commented 5 years ago

@nitishm I would like to resolve this issue for you. Should be fairly simple..just pull over the existing dependencies from Dep and remove Dep files (Gopkg.lock, Gopkg.toml) and vendor directory, does that sound right?

nitishm commented 5 years ago

That sounds good. Go for it !!!

nicfb commented 5 years ago

After I pulled over the dependencies into the go.mod file, it shows them as "incompatible". Do you know if this is an issue? It seems to be building fine.

nitishm commented 5 years ago

That should be fine. All it means is that the dependencies don't utilize go.mod/go.sum themselves, so go modules resort to using commit hashes.

nicfb commented 5 years ago

Oh ok. I'll submit a pull request then.

nitishm commented 5 years ago

Let's kick off a discussion about how we want to handle the following,

Shivam010 commented 5 years ago

Quoting back from https://github.com/nitishm/go-rejson/pull/33#discussion_r310354937

I think omitting 1.12 directive and using module path github.com/nitishm/go-rejson will not fail builds for the older versions, as go semantics will help.

Using github.com/nitishm/go-rejson module path will keep the backward compatibility as the import path will be as it is.

Also, we do not require Gopkg.* files, in this case, go.mod will handle that for us.

nicfb commented 5 years ago

So using github.com/nitishm/go-rejson instead of just go-rejson will take care of handling version v2 onward? And for handling Go versions before 1.11, is there something in place in Go modules that takes care of this? Or do we have to keep Go Dep or something similar around?

Shivam010 commented 5 years ago

Yes, it will. The latest patch of 1.9 and 1.10 understand go.mod, so there will be no problem with them. And if we do not change the import path, I think it will also work for 1.8 or earlier.

nitishm commented 5 years ago

So our current module version is v2 so we must set the module github.com/nitishm/go-rejson/v2 and the require github.com/nitishm/go-rejson/v2. Everywhere this is imported, when using with modules enabled we must change the import path to include the major version. This doesn’t apply to go 1.9+ and go 1.10+ because they have been patched in the newer releases to not require an import change. So just switching to github.com/nitishm/go-rejson isn’t going to cut it.

nitishm commented 5 years ago

So it seems like both me and @Shivam010 are right. My explanation stems from the fact that I want v1.x.x to support go modules as well, while @Shivam010 is thinking from the perspective of v2.0.0 being the version that adopts go modules, which would mean if we went to v3.0.0, the go modules would bump up to v3 major version.

I suppose the choice now is to either support go modules only starting from v2.0.0 or also support this in a v1.0.x minor patch version.

I vote for supporting v1.x.x. What do you guys think ?

More info in this thread - https://www.reddit.com/r/golang/comments/clqdib/initializing_go_module_for_library_at_git_release/evx7drb?utm_source=share&utm_medium=web2x

Shivam010 commented 5 years ago

So it seems like both me and @Shivam010 are right. My explanation stems from the fact that I want v1.x.x to support go modules as well, while @Shivam010 is thinking from the perspective of v2.0.0 being the version that adopts go modules, which would mean if we went to v3.0.0, the go modules would bump up to v3 major version.

Yes, I was thinking to adopt go modules from v2, I didn't want to disturb v1.

But if we want to have support in a minor patch, then we will have to keep the module path intake, as I was saying before.

Still, I vote for bringing support from v2.

nicfb commented 5 years ago

Oh ok. I don't have any experience with this, but I can see both sides. Is there any harm in either just doing a major version bump, or try and retrofit it for v1?

nitishm commented 5 years ago

Retrofitting isn’t really needed and by default we should promote v2 as the standard version because of its support for more than one redis client.

So let’s proceed with v2 as the first to adopt go modules as @Shivam010 suggested.

@nicfb can you make the changes wrt this and create a new PR ?

nicfb commented 5 years ago

Ah that makes sense. Sounds good, I can do that.

nicfb commented 5 years ago

Let me know if that covers everything..

nitishm commented 5 years ago

@nicfb if you are familiar with TravisCI can you update the .travis.yaml file to remove any dependence on godep and change the test-against go versions to one pre 1.11 version, that supports go-modules (whatever patch release @Shivam010 noted with the added support) and couple of post 1.11 versions with go module support.

We want to make sure that our CI tests pass with this change. Currently they are failing. You can check this by going to https://travis-ci.org/nitishm/go-rejson and looking at the output from your last commit.

Shivam010 commented 5 years ago

@nicfb @nitishm I have updated .travis.yml file for 4 Go versions.

go 1.11.x, 1.12.x, 1.10.x, 1.9.x

I have not specified patch number in any version, cause I think it would pick the latest patch by default, though I am not sure about it, but is working in the same way.

Also, go 1.9.7 is failing the golangci-lint run ./... check, it is because golangci-lint doesn't support go 1.9.7.

xref https://github.com/golangci/golangci-lint/issues/358

nitishm commented 5 years ago

@Shivam010 So any reason not to exclude this from the CI test matrix ?

Shivam010 commented 5 years ago

I didn't know that golangci-lint doesn't support 1.9.x, before I tried the test. I am trying to add allow_failures element but could not end up with a correct syntax

Shivam010 commented 5 years ago

@nitishm Build succeeds, we can merge, now

nicfb commented 5 years ago

Sorry - just now seeing this. Thanks for taking care of that @Shivam010

nitishm commented 5 years ago

Thanks for all the work @nicfb @Shivam010 🚀 🥇