[#310] Persist sqlite to disk #321
Conversation
evan2645
requested review from
amartinezfayo,
kunzimariano,
paul-argeniss,
walmav and
y2bishop2y
as code owners
ghost
added
the
conf/server/plugin/ds_sqlite.conf
Outdated
| @@ -3,4 +3,6 @@ pluginCmd = "plugin/server/datastore-sqlite/datastore-sqlite" | |||
| pluginChecksum = "" | |||
| enabled = true | |||
| pluginType = "DataStore" | |||
| pluginData {} | |||
| pluginData { | |||
| file_name = "./conf/server/spire.db" | |||
There was a problem hiding this comment.
I'm wondering if having this database file under the config folder would be confusing.
The nature of the data stored looks more like operational data rather than configuration settings.
Also, from an end-user perspective, I'm thinking if I see this file here, how do I know that belongs to the datastore-sqlite plugin? What if other server plugins use a database file in the future?
There was a problem hiding this comment.
Agreed... I struggled a little trying to come up with the correct location for this.
I think we need a general directory for runtime file storage... use cases like attestation nonce come to mind. This seems like it may fit the bill as well... perhaps I'll create a top-level data directory, and stick it in there with a name like datastore_sqlite.db or datastore.sqlite3? Would that suffice?
| } | ||
| path = "file:" + path |
There was a problem hiding this comment.
Maybe is useful to log the path that is being used?
|
@amartinezfayo pushed a commit to address your comments - let me know if that covers it or not. Thanks! |
|
|
||
| db.LogMode(true) | ||
| log.Printf("opening sqlite database with path %s", path) | ||
| db, err := gorm.Open("sqlite3", path) |
There was a problem hiding this comment.
In case of a file path, I think that we will now need to check if the folder exists.
I guess that there will be an error here if you don't have the .data folder. We can automatically create the folder in that case at this point.
There was a problem hiding this comment.
I'm hesitant to create directories if they don't exist, since it might be an error condition rather than an omission.
How about I create an empty .data dir and check it in?
There was a problem hiding this comment.
@amartinezfayo fixed. Git automatically removes empty directories, so I put a placeholder file there.
This PR exposes a file path configuration option for sqlite datastore. In order to do that, a number of changes were required:
If a file path is not configured, the plugin will default to an in-memory database. Fixes #310