syncstorage-rs
syncstorage-rs copied to clipboard
refactor: add settings crates
Description
I revamped the naming in this PR to make things clearer. At first, I considered simply adding a "syncstorage-settings" crate with a "tokenserver-settings" crate as a dependency, but I don't think that scheme makes sense right now. Our eventual goal is to roll Tokenserver into Syncstorage so they run as one service, but for right now, Tokenserver and Syncstorage exist separately. If we made the top-level settings crate the Syncstorage settings crate, a bunch of the settings would either need to be Option
s or they would need to be set but unused in the Tokenserver deployment. I think it's cleaner to have separate, composable settings crates for Tokenserver and Syncstorage, with settings common to the two in a separate crate.
As I implemented this, the name "syncstorage" quickly became overloaded. It was unclear whether "syncstorage" was referring to both Tokenserver and Syncstorage (i.e. the whole server) or just the actual Syncstorage part. Following the naming convention here, I think it makes sense to rename syncstorage-rs to "syncserver-rs". I've updated the names of the crates to reflect this, but we may want to consider renaming this repository at some point too.
If we do roll Tokenserver into Syncstorage some day, it might make sense to revert the naming to "syncstorage-rs," but for now, I think it would be too confusing not to rename things. We need the flexibility to run Syncstorage and Tokenserver separately (for our production deployment) and together (for self-hosted users), and I think it would be much cleaner and more obvious to think of Tokenserver and Syncstorage as smaller, composable parts of a larger system.
❗ Breaking Changes ❗
Because of the naming changes in this PR, the names of environment variables and settings in local.toml
will need to change. Specifically, there are some settings that were previously global in scope that are now scoped specifically to Syncstorage. These settings will need to be prefixed with SYNC_SYNCSTORAGE__
for env vars and syncstorage.
for settings in local.toml
. The Syncstorage-specific settings can be found in syncstorage-settings/src/lib.rs
.
Testing
There should be no change in functionality. Successful compilation and runs of the unit and integration tests should be sufficient
Issue(s)
Closes #1276
FWIW, I think we can avoid having to change the SYNC_
prefix to SYNC__
if we use Environment::prefix_separator()
(relevant PR), so this may not depend on upgrading the config crate
I plan to make substantial updates to the README in a future ticket to reflect the new crate structure and naming scheme.
Waiting to merge this until after 0.12 is tagged
@jrconlin Oh, did I accidentally add a change that references the vendored mozilla-rust-sdk
? It looks like the current version of the README references it, and I agree that we should update that, though maybe in a separate PR?
(had to rebase)