Page MenuHomePhabricator

Correct behavior when setting new configuration keys
ClosedPublic

Authored by chris on Aug 11 2019, 3:04 PM.

Details

Summary

This corrects an issue where setting a new key (or updating an existing one) in a local config source would totally nuke all other config options you had set previously. This was introduced at simplex/scripts/config/manage_config.py$50 by D20 --- translating period-separated CLI config keys (mysql.user) to a nested dictionary (obj['mysql]['user']) created a bad interaction with SimplexConfigDefaultSource.set_keys() since it turns out that merging two nested dictionaries is a little tricky and requires some recursion that Python doesn't want to do out of the box.

Test Plan
$ ./bin/config --list
INFO:root:[INFO]:
  mysql:  {'host': '127.0.0.1'}

$ ./bin/config --set mysql.test flarp
INFO:root:[INFO]: Set local configuration values for mysql.test

$ ./bin/config --list
INFO:root:[INFO]:
  mysql:  {'host': '127.0.0.1', 'test': 'flarp'}

Diff Detail

Repository
rSIM Simplex
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chris requested review of this revision.Aug 11 2019, 3:04 PM
chris created this revision.
chris edited the summary of this revision. (Show Details)Aug 11 2019, 3:04 PM
brooke added inline comments.Aug 12 2019, 5:30 PM
simplex/infrastructure/env/config_source.py
69

how does this work? can you call self.set_keys under set_keys when it is not set up in def init(self)?

chris added inline comments.Aug 12 2019, 5:32 PM
simplex/infrastructure/env/config_source.py
69

It's just a recursive function call. I'm not sure I understand what init() has to do with anything?

brooke added inline comments.Aug 12 2019, 6:03 PM
simplex/infrastructure/env/config_source.py
69

I thought you can only call self.xxx when you create it in init() first

chris added inline comments.Aug 12 2019, 6:11 PM
simplex/infrastructure/env/config_source.py
69

Oh! Sorry, no, the self prefix just means "look somewhere else inside this class"

brooke added inline comments.Aug 12 2019, 7:08 PM
simplex/infrastructure/env/config_source.py
69

Ah got it. Sorry! here comes my next questions...
I understand that if config = update then pass else set confg = update.
But what do line 65-72 do?

chris added inline comments.Aug 12 2019, 7:45 PM
simplex/infrastructure/env/config_source.py
69

That's just the recursive part of the function. Here all it's doing is saying "Check and see if the next leaf in the sequence is a dictionary, and if it is, run it back through this function". That way we can get any arbitrary level of nesting within the configuration keys

brooke accepted this revision.Aug 14 2019, 8:11 PM
This revision is now accepted and ready to land.Aug 14 2019, 8:11 PM
This revision was automatically updated to reflect the committed changes.