Update driver name after ASF donation #1817
Conversation
version.go
Outdated
| @@ -27,7 +27,7 @@ package gocql | |||
| import "runtime/debug" | |||
|
|
|||
| const ( | |||
| mainModule = "github.com/gocql/gocql" | |||
| mainModule = "github.com/apache/cassandra-gocql-driver" | |||
There was a problem hiding this comment.
Have you tested this? It's using this string to match the dependency path but I'm not sure where the dependency path comes from. If it comes from the actual module name then I don't think this will work.
There was a problem hiding this comment.
It comes from the module name as defined in go.mod, which won't change until v2.
The reason why it checks BuildInfo is so that when someone uses a fork with the replace directive, the replaced version/path is used.
We need to keep mainModule intact and do
driverName = "github.com/apache/cassandra-gocql-driver"instead of
driverName = mainModuleThere was a problem hiding this comment.
if d.Path == mainModule {
driverName = "github.com/apache/cassandra-gocql-driver"
driverVersion = d.Version
if d.Replace != nil {
driverName = d.Replace.Path
driverVersion = d.Replace.Version
}
break
}
There was a problem hiding this comment.
🤦 No, really... this is what I get for rushing. Completely agree that "mainModule" is entirely the wrong name here although I still think that there's merit in encapsulating that value in a constant, albeit a better-named constant.
I'll tweak this a bit and see if I can come up with something that (a) is clearer and (b) is very much in the spirit of what's proposed above.
There was a problem hiding this comment.
Perhaps we could rename mainModule to upstreamModule ?
There was a problem hiding this comment.
How about just "mainPacakge"? That has the advantage of matching the terminology in the runtime/debug docs... ?
There was a problem hiding this comment.
package in the Go land means something different than module, in this case it's referring to the module so I'm fine with keeping it mainModule or renaming it to upstreamModule, both work for me.
There was a problem hiding this comment.
Oh wait we're looking at d.Path instead of d.Main, in that case it is referring to the package you're right.
There was a problem hiding this comment.
mainPackage or upstreamPackage would be more suitable then
There was a problem hiding this comment.
Thanks @joao-r-reis . I kinda like "mainPackage" in this case so I ran with it... if anybody has objections let me know!
| @@ -27,7 +27,7 @@ package gocql | |||
| import "runtime/debug" | |||
|
|
|||
| const ( | |||
| mainModule = "github.com/gocql/gocql" | |||
| defaultDriverName = "github.com/apache/cassandra-gocql-driver" | |||
There was a problem hiding this comment.
Allows us to contain the default value in a distinct location (i.e. not embedded within the code below) with a much more descriptive name.
|
Please squash the commits and update the commit message: https://github.com/apache/cassandra-gocql-driver/blob/trunk/CONTRIBUTING.md#commit-message |
patch by Bret McGuire; reviewed by Joao Reis, Martin Sucha, Bret McGuire reference: apache#1817
absurdfarce
force-pushed
the
asf-driver-name
branch
from
ab69e37 to
ce76a83
Compare
| @@ -38,8 +44,8 @@ func init() { | |||
| buildInfo, ok := debug.ReadBuildInfo() | |||
There was a problem hiding this comment.
Since there was some confusion in the PR it would be good to document the point of this code especially as it relates to the replace directive
There was a problem hiding this comment.
This should be addressed below... let me know if you were thinking of something else @jameshartig !
patch by Bret McGuire; reviewed by Joao Reis, Martin Sucha, Andy Tolbert and James Hartig reference: apache#1817
absurdfarce
force-pushed
the
asf-driver-name
branch
from
dad2fbe to
a48a468
Compare
absurdfarce
requested review from
joao-r-reis,
tolbertam,
martin-sucha and
jameshartig
|
@tolbertam The test failures here seem to be coming from expired certs for TLS tests. Is fixing that just a matter of getting #1805 merged? |
|
That's right @absurdfarce , #1805 aims to resolve the issues around the certs used by the tests expiring. |
|
Thanks @tolbertam! I'm gonna merge #1805 and confirm that that addresses the test failures before calling this one good. |
patch by Bret McGuire; reviewed by Joao Reis, Martin Sucha, Andy Tolbert and James Hartig reference: apache#1817
|
Bah, I mucked this up with the merge commit 🤦 . Closing this out and replacing it with #1824 |
patch by Bret McGuire; reviewed by Joao Reis, Martin Sucha, Andy Tolbert and James Hartig reference: #1817
patch by Bret McGuire; reviewed by Joao Reis, Martin Sucha, Andy Tolbert and James Hartig reference: apache#1817
Issue originally identified by @tolbertam
Update the driver name to allow servers to clearly distinguish between gocql and cassandra-gocql-driver. Note that something similar was done for the Java driver when it was donated.