Viceroy icon indicating copy to clipboard operation
Viceroy copied to clipboard

Allow specifying top-level `file` and `format` for KVStore

Open GeeWee opened this issue 1 year ago • 2 comments

It's very practical to be able to keep all your local configuration in separate files, if they are e.g. generated by automatic scripts. This extends the fastly.toml kv_store configuration to accept either the current inline API or a new file and format table, that allows loading of an external JSON file.

Fixes #364

GeeWee avatar May 03 '24 08:05 GeeWee

Thank you for the contribution! We're actually in the midst of some discussions about how to redesign the way that Viceroy handles 'resources' needed by the service, so we'll have to hold off on reviewing this PR for a few weeks until we've reached some decisions there. The good news is that the idea you've presented here is directly in line with our goals :-)

Please stay tuned, we'll get back to you soon.

kpfleming avatar May 08 '24 21:05 kpfleming

Gotcha - thanks for the feedback :)

GeeWee avatar May 10 '24 03:05 GeeWee

Overall this looks fine, but as you can probably see from the conflicts the term 'object store' shouldn't be used for this feature, it should be 'kv store' (in various forms) everywhere. If you get the PR updated I'll give it a formal review and work towards getting it merged. Thanks!

kpfleming avatar Jul 24 '24 14:07 kpfleming

@kpfleming the test file is reasonably inconsistent (many tests still refer to object store). However I've fixed my changes on top of the latest main and changed to use kv-store instead of object store in my tests.

I've rebased and squashed my changes into one commit. Let me know if you don't care about that (e.g. if you'll end up squashing when merging anyways) and I'll save the effort :)

GeeWee avatar Jul 25 '24 07:07 GeeWee

Another approach we can take is to point a KV Store to a folder and use the file-paths as the KV Store Keys and the file contents as the value -- This would allow for simpler setup for the scenario where applications are wanting to store their website static assets in a KV Store

JakeChampion avatar Jul 25 '24 07:07 JakeChampion

Another approach we can take is to point a KV Store to a folder and use the file-paths as the KV Store Keys and the file contents as the value -- This would allow for simpler setup for the scenario where applications are wanting to store their website static assets in a KV Store

I have a branch which implements this at https://github.com/fastly/Viceroy/tree/jake/kv-store-folder but I stalled out on the work unfortunately

JakeChampion avatar Jul 25 '24 07:07 JakeChampion

Definitely also an option - I do wonder if keeping parity with the dictionary interface isn't a good thing though, considering that we can't be the only migrating I imagine :)

GeeWee avatar Jul 25 '24 09:07 GeeWee

Another approach we can take is to point a KV Store to a folder and use the file-paths as the KV Store Keys and the file contents as the value -- This would allow for simpler setup for the scenario where applications are wanting to store their website static assets in a KV Store

I have a branch which implements this at https://github.com/fastly/Viceroy/tree/jake/kv-store-folder but I stalled out on the work unfortunately

We've got another PoC implementation around which does the same thing as a WASM component :-)

For now I'm OK with merging the approach in this PR because it doesn't add a new 'style' of data population, it just replicates the existing style offered for config stores. We can talk about offering other styles in the larger topic of redesigning the package manifest/configuration file.

kpfleming avatar Jul 25 '24 15:07 kpfleming

@kpfleming I believe I've incorporated all of your feedback :)

GeeWee avatar Jul 26 '24 06:07 GeeWee

The tests all pass now, I'll add an entry for CHANGELOG.md and get this merged.

kpfleming avatar Jul 26 '24 14:07 kpfleming