lib / pq

Pure Go Postgres driver for database/sql
https://pkg.go.dev/github.com/lib/pq
MIT License
8.98k stars 909 forks source link

Move Kerberos support to separate module #972

Closed AGWA closed 4 years ago

AGWA commented 4 years ago

As discussed in #971.

Before releasing this, you should update auth/kerberos/go.mod as follows:

Thanks for working on pq!

AGWA commented 4 years ago

The CI failures appear to be unrelated, and caused by a change in SSL certificate validation in Go master.

maddyblue commented 4 years ago

Maybe looks good except for the forced version bumping. I think it's circular and can't work. Say I want to make a new release. I update go.mod to contain the future (but non-existent) tag. I now need to update go.sum, but I haven't pushed the tag, so the go proxy can't download it yet. It seems liked we'd have to refer to the previous lib/pq version, or else make 2 releases every time? I don't like any of these options.

I think we should instead have the krb package not auto-register itself with lib/pq. This would remove the dependency on lib/pq from krb. It would add one other line for GSS-users, but that's fine. It'd be like:

import "github.com/lib/pq/auth/krb"

func init() { pq.RegisterNewGSSFunc(krb.NewGSS) }

Am I correct? I haven't tried any of this so I could be wrong about some details. Or maybe there are better ways to avoid the forced go.sum bump thing?

AGWA commented 4 years ago

I can't think of a way to avoid a two-step release process, I'm afraid.

I can rework this to require explicit registration, but it will have to look like this:

import "github.com/lib/pq/auth/krb"

func init() { pq.RegisterNewGSSFunc(func() (pq.Gss, error) { return krb.NewGSS() }) }

That's because RegisterNewGSSFunc expects a function returning an interface, but to avoid the dependency on pq, NewGSS will need to return a pointer to a concrete type instead.

maddyblue commented 4 years ago

Yes that's fine. I can't see a better way either.

ggilley commented 4 years ago

I came here to write a ticket to suggest doing this. Thanks for being on top of it. I pulled the latest pq and was surprised at all the extra dependencies (especially the gorilla sessions and securecookie)

AGWA commented 4 years ago

I've reworked the patch to remove the dependency and require explicit registration.