syncstorage-rs icon indicating copy to clipboard operation
syncstorage-rs copied to clipboard

refactor: add settings crates

Open ethowitz opened this issue 2 years ago • 5 comments

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 Options 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

ethowitz avatar May 09 '22 16:05 ethowitz

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

ethowitz avatar Jun 25 '22 00:06 ethowitz

I plan to make substantial updates to the README in a future ticket to reflect the new crate structure and naming scheme.

ethowitz avatar Jun 27 '22 15:06 ethowitz

Waiting to merge this until after 0.12 is tagged

ethowitz avatar Jun 29 '22 18:06 ethowitz

@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?

ethowitz avatar Oct 03 '22 21:10 ethowitz

(had to rebase)

ethowitz avatar Oct 03 '22 21:10 ethowitz