feat: Identity overrides in local evaluation mode #20
Conversation
khvn26
requested review from
a team and
zachaysan
and removed request for
a team
khvn26
force-pushed
the
feat/local-eval-identities
branch
4 times, most recently
from
f04ee89 to
c2ab95c
Compare
| environment_url.clone(), | ||
| )?); | ||
| for identity in &environment.as_ref().unwrap().identity_overrides { | ||
| data.identities_with_overrides_by_identifier.insert(identity.identifier.clone(), identity.clone()); |
There was a problem hiding this comment.
I'll happily accept pointers on how to stop copying around the identity structs. This shouldn't be a problem for most environments, but we need to be aware there's room for optimisation here.
src/flagsmith/mod.rs
Outdated
| ); | ||
| let mut data = ds.lock().unwrap(); | ||
| data.environment = environment; | ||
| update_environment(&client, &ds, &environment_url).unwrap(); |
There was a problem hiding this comment.
Interested to know why we use client.clone() above but directly point to the client here.
I suspect it's because we want to update the environment on the instance of the client itself, but just double checking.
There was a problem hiding this comment.
I'm not sure if cloning inside a loop is a good idea.
We're keeping the initial client instance for the background task, and cloning it once for the SDK client's blocking calls.
There was a problem hiding this comment.
I don't see obvious show stoppers, but I have a few small questions. I'm going to leave the final approval up to @matthewelwell since my experience with Rust is quite limited.
| "api_key": "B62qaMZNwfiqT76p38ggrQ", | ||
| "project": { |
There was a problem hiding this comment.
Feels kinda icky to have ENVIRONMENT_JSON in this rust file. Can we not load it from a json file?
There was a problem hiding this comment.
It's surprisingly verbose, see https://stackoverflow.com/questions/66340266/generating-constants-ar-compile-time-from-file-content
src/flagsmith/mod.rs
Outdated
|
|
||
| // When | ||
| let mut _flagsmith = Flagsmith::new(environment_key.to_string(), flagsmith_options); | ||
| thread::sleep(std::time::Duration::from_millis(250)); |
There was a problem hiding this comment.
Is this sleep necessary to allow the local evaluation document to be ready and loaded? In other words, if we don't sleep do we have an empty document? If so, we should rework the blocking logic and block on boot for the first local evaluation like we currently do in the Python client.
khvn26
force-pushed
the
feat/local-eval-identities
branch
from
946e4e0 to
1f338a2
Compare
Closes #17.
Contributes to Flagsmith/flagsmith#3132.
Changes:
update_environmentto share code between theFlagsmith.update_environmentinterface and the threaded routineflagsmith-flag-enginedependency spec will be changed after Flagsmith/flagsmith-rust-flag-engine#12 is merged and released.