-
Notifications
You must be signed in to change notification settings - Fork 166
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
Custom names for sessions tables #180
Comments
Table name is an implementation detail of the relational db stores so I think it's fine to implement it store by store like in the PR at #177. For consistency it may be usefull to add it to all of stores backed by a relational db, more or less the equivalent of the prefix option for the KV stores. Setting the tablename after init though (using SetSessionsTableName like in #177) does not seem right so maybe it would be better to have a new constructor: This again mirrors more or less the |
I like the idea of a type Options struct {
CleanupInterval time.Duration
TableName string
} Which could then be used like: postgresstore.NewWithOptions(db, postgresstore.Options{
CleanupInterval: time.Minute,
TableName: "mycustomsessions",
}) If any fields in I think we could potentially roll this pattern out across all the different stores and deprecate the The only problem I can see right now is that the consulstore package already has a a. Make a breaking change to consulstore. |
That would be a nice feature! Maybe instead of whole separate constructor one might consider using Functional Options pattern. See pull request #190 for the prototype. In this case, no backwards compatibility is broken:
As a bonus, one may change cleanupInterval the same way (also without breaking backwards compatibility):
It seems to me that such implementation would solve all the problems above and is applicable to any other store. Feel free to point at anything I might have forgotten! |
As per the comments on #177, consider supporting custom names for the sessions tables where it makes sense.
The text was updated successfully, but these errors were encountered: