Switch from config init to BaseSettings

[shyamd]

Closes #152

This is going to be a complicated PR since this SeverConfig object is spiderwebed all over the server module. The goal is to swtich to BaseSettings which will make using this as a standalone server given a mongoDB much easier

[codecov]

Codecov Report

Merging #226 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #226   +/-   ##
=======================================
  Coverage   87.11%   87.11%           
=======================================
  Files          43       43           
  Lines        1862     1862           
=======================================
  Hits         1622     1622           
  Misses        240      240           

Flag	Coverage Δ
#unittests	87.11% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76d2e37...76d2e37. Read the comment docs.

[shyamd]

Ok, this looks like it works. I've got to add some tests in the morning.

[CasperWA]

Look's great!
I know this is still WIP, I was just curious to see how this was done, and added some comments along the way.
The tests look a bit mangled at this point, but I guess that aligns well with your last comment 👍

[ml-evs]

Thanks for this @shyamd, this is certainly better than our current system!

[shyamd]

@CasperWA @ml-evs
What is the purpose of the docker-compose and Dockerfile. Is it just for testing or is it for general use?

[shyamd]

One of the two of you has to review for the merging to become unblocked btw.

[ml-evs]

Great work @shyamd, thanks. This definitely simplifies the config and has caught a few other bugs.

I'm happy to approve this as is, my only minor comment is whether you think it makes sense for the example_config file to contain the defaults only? I know it's not much of an ask, but the only other way to see the defaults is to check the code.

We should definitely do a 0.8 release after this (or at least soon).

[ml-evs]

I get that this is just an example config, but does it make more sense for this file to contain the default values?

Suggested change

	"use_real_mongo": true,
	"use_real_mongo": false,

[shyamd]

I'm sorta back and forth on this issue. Part of me agrees with you and part of me wants a more real-world example. Obviously we can't make the defaults real-world example as we want it to just run as-is.

[ml-evs]

I'm sorta back and forth on this issue. Part of me agrees with you and part of me wants a more real-world example. Obviously we can't make the defaults real-world example as we want it to just run as-is.

Perhaps we could include the default config elsewhere?

[shyamd]

Good idea. I've added that for now. The next big PR might be official docs of some sort since there is enough complexity now that they are needed.

[ml-evs]

Good idea. I've added that for now. The next big PR might be official docs of some sort since there is enough complexity now that they are needed.

Great, agreed. I'm happy to help with that as its quite a big task, we have an issue at #176 to start tracking this if we want to delegate the work.

[ml-evs]

👍

Thanks again @shyamd and @CasperWA for the previous thorough review.

Mostly fixed

[CasperWA]

I think this should have been squashed in some way - 53 commits is a bit of an extensive addition to the history for a single PR...
It's done now, but maybe for the future we should be more considerate of future colleagues/versions of us, who may want to cherry-pick commits or otherwise try to untangle some piece of code history?