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

Deprecate safe_core environment variables in favor of configuring through the API #742

Open
m-cat opened this Issue Feb 18, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@m-cat
Copy link
Contributor

m-cat commented Feb 18, 2019

Some background from here:

We use config files throughout the backend – Crust and Routing both use them, allowing changing settings without recompiling. But it’s certainly possible to also support passing a config object to the SAFE App API – app_unregistered already accepts a crust config object as an optional parameter, and we could extend this to the safe_core config.

I believe that doing so would make things cleaner and would allow us to deprecate the environment variables. I was part of the design and implementation of the env vars but I have to admit that they are technically lacking: any app can change them and thus change the behavior of other apps. We also run into possible issues with multithreading: one thread can write to a var while another reads it – there is no concurrency guard on env vars, which is they are known to be a bad solution in these cases. We actually ran into this problem while writing tests because cargo test runs each test in a separate thread, so using env vars to test for certain things was tricky and we (well, I) ended up putting some dirty hacks in place.

Of course, we only use the config for the mock network, so the security points are moot. But passing the config in through the API would just be better, easier, and allow us to remove the hacks we use for testing.

The config object we use can be like the crust BootstrapConfig option and we can pass it through to the API entrypoints as we do with BootstrapConfig, e.g. here.

@m-cat m-cat added the help wanted label Feb 18, 2019

@m-cat

This comment has been minimized.

Copy link
Contributor Author

m-cat commented Feb 22, 2019

We should also add some more logging as described here:

I think it would be helpful for us to add more logging in the code (e.g. “config found at path X with settings: […]”). That in combination with removing the environment variables ... should make these situations must[sic] easier to debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.