Skip to content
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

Allow specifying top-level file and format for KVStore #365

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

GeeWee
Copy link
Contributor

@GeeWee GeeWee commented May 3, 2024

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.

Part of #364

lib/src/config/object_store.rs Show resolved Hide resolved
lib/src/error.rs Show resolved Hide resolved
@kpfleming
Copy link
Contributor

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.

@GeeWee
Copy link
Contributor Author

GeeWee commented May 10, 2024

Gotcha - thanks for the feedback :)

@kpfleming
Copy link
Contributor

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!

@GeeWee GeeWee force-pushed the main branch 5 times, most recently from 2f2f5a2 to 5e7dbcb Compare July 25, 2024 07:35
This change allows keeping KVStore key/values in a separate file for easier sharing between environments,
and allowing users to only gitignore the actual KVStore content without needing to ignore the whole fastly.toml file.0;9u
@GeeWee
Copy link
Contributor Author

GeeWee commented Jul 25, 2024

@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 :)

@JakeChampion
Copy link
Contributor

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
Copy link
Contributor

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

@GeeWee
Copy link
Contributor Author

GeeWee commented Jul 25, 2024

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 :)

Copy link
Contributor

@kpfleming kpfleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few changes you can commit directly, and some others you can make in a followup commit. You can squash or not squash, whichever is easier for you.

cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
cli/tests/integration/kv_store.rs Outdated Show resolved Hide resolved
lib/src/config/object_store.rs Outdated Show resolved Hide resolved
@kpfleming
Copy link
Contributor

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.

GeeWee and others added 2 commits July 26, 2024 08:42
Rename object_stores to kv_stores

Co-authored-by: Kevin P. Fleming <[email protected]>
…ors, and renamed Object store to KVStore one place
@GeeWee GeeWee requested a review from kpfleming July 26, 2024 06:50
@GeeWee
Copy link
Contributor Author

GeeWee commented Jul 26, 2024

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

kpfleming
kpfleming previously approved these changes Jul 26, 2024
@kpfleming
Copy link
Contributor

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

@kpfleming kpfleming enabled auto-merge (squash) July 26, 2024 14:06
@kpfleming kpfleming merged commit 0596f51 into fastly:main Jul 26, 2024
15 checks passed
joeshaw added a commit that referenced this pull request Sep 16, 2024
This uses the same format as for the KV store, introduced in #365.

Fixes #364.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants