SQL Storage Support #37
Conversation
|
|
docs/configuration.md
Outdated
| @@ -42,6 +42,9 @@ disableMetadata: false | |||
| # Optional, defaults to 8000 | |||
| port: 8000 | |||
| storage: | |||
| # What type of storage do you want? inmemory (default), directory, or postgresql | |||
| type: "" | |||
There was a problem hiding this comment.
I'm thinking we make a breaking change here (we're not 1.0.0 yet).
I'd like to use URI schemes so that it's really easy to add different storage backends in the future.
file://for filesystempostgresql://user:pass@localhost:5432/databasefor postgres
I think we should update the config so that it's just storage: <uri>
There was a problem hiding this comment.
We can then probably refactor the name PostgresStorage to be SQLStorage and implement it to support all SQL backends that gorm supports (pg, mysql and sqlite3 at least).
There was a problem hiding this comment.
I tried hard not to break anything :)
https://gorm.io/docs/connecting_to_the_database.html#Sqlite3
Could just drop json disk and use sqlite in memory, sqlite on disk, mysql, and postgres
There was a problem hiding this comment.
oooh, i can use my new yaml unmarshaling learnings so it could be done directly in storage and not in main? is that dumb?
add in sqlite, postgres, mysql Tests don't pass cause they don't have a db to talk to Signed-off-by: Gavin Mogan <git@gavinmogan.com>
halkeye
changed the title
|
Okay, lemme know what you think. I don't like the db.Open inside the new function cause it makes it impossible to test. |
There was a problem hiding this comment.
I'm happy with this PR. There are a few minor modifications/refactors I might apply but you've nailed the parts the matter I think.
if you're happy, i'd like to merge this, do a few small changes, update the change log and then release an RC so we can test a little.
| Directory string `yaml:"directory"` | ||
| } `yaml:"storage"` | ||
| Port int `yaml:"port"` | ||
| Storage storage.StorageWrapper `yaml:"storage"` |
There was a problem hiding this comment.
I think it's simpler to just use a string here. This is a bit too fancy I think.
There was a problem hiding this comment.
I liked the wrapper solution cause it meant that the logic was in a single place. I'm open to other solution ideas though
|
Sure merge and fix it or let me know |
|
It's been a couple weeks now. Any idea what your plan is? Do you have changes you want to make? Or are you going to merge and make them? |
|
Hey sorry. I'll get this done now! |
|
@halkeye again, sorry I ghosted for the past 17 days. give me a kick up the butt sooner next time :P This is now published as 0.2.0-rc6 for docker and helm. |
|
I usually nudge at two weeks. Everyone gets busy or distracted and open source should be fun and voluntary |
Mostly need help how to non breakingly do the configs
Added a new config.storage.type to set it to postgres
refactored storage interface so it takes in owner/name, or the device only, and only do key logic for disk/in memory