Open goodboy opened 7 years ago
Comment by moises-silva Friday Feb 10, 2017 at 19:12 GMT
@tgoodlet Yeah from reading the first snippet of code, even before reading the second, it kinda felt odd to have stop()/start() methods in what appeared to be a configuration object. This might be a personal perception only but I see configuration & service management as related but different things. You certainly want to restart services (a profile would be a service for example in this case) when changing configuration, but that's sort of a separate responsibility that I'm not even sure should be in the same class/object (manage in this case seems to be doing both).
I'd see something like these clearer.
fs.sofia.config.profiles['external']
fs.sofia.api.restart_profile('external')
That way you separate configuration management from service/module/command execution. The 'api' object for every module/component at some point could even be smart enough to auto-generate methods based on FS help (if fs help is good enough for some simple cases). Some methods are standard, like:
fs.sofia.api.reload()
That should be avail for any module cuz it's just a 'reload mod_sofia' or 'reload mod_xxx'
I just called the obj api cuz that's what they use for their python/lua bindings, but could be 'control', 'management' or whatever else.
So the general gist of the organization is fs.<module>.<config|api>.<property|method>
But what you propose at the end looks also good, just less explicit, you're making the api calls an implicit method of the sofia object and the properties implicitly part of the config.
Comment by vodik Friday Feb 10, 2017 at 21:03 GMT
I agree, as I see it, I could, in theory define a profile that's completely detached from sandswitches, and then push it:
myprofile = make_me_a_profile()
confmng.sofia.profiles['external'] = myprofile # In theory, at least
So what does myprofile
do if you did api calls on it then?
I would suggest a slightly different API:
fs = manage('myfshost.com', keyfile='mykey.rsa')
# Deal with configuration explicitly:
config = fs.config()
# ... do stuff with config
fs.load(config)
And that leaves fs
to directly expose API:
fs.sofia.restart_profile(myprofile.name)
Just off-the-cuff ideas.
Comment by tgoodlet Friday Feb 10, 2017 at 21:43 GMT
@moises-silva @vodik thanks guys both good ideas. I'm gonna draft a PR over the weekend hopefully.
Comment by tgoodlet Monday Feb 13, 2017 at 02:40 GMT
@vodik just to clarify regarding:
I agree, as I see it, I could, in theory define a profile that's completely detached from sandswitches, and then push it:
myprofile = make_me_a_profile() confmng.sofia.profiles['external'] = myprofile # In theory, at least``` So what does myprofile do if you did api calls on it then?
Is already supported and shown to work with the test here. I do agree that it becomes unclear why the configuration object defines an API while the original dict
input does not. This is the initial reason for my questioning this API.
Also you suggested:
# Deal with configuration explicitly: config = fs.config() # ... do stuff with config fs.load(config)```
I like this. My only quiff (besides that it's outside the scope of this issue) is that it requires creating a copy of the currently mapped config sections (not a big deal) and also overwriting only the appropriate sections when load()
is called (also not that hard) since we don't yet support the entire FS config document via object mappings.
The other thing to note that that shorthand for pushing a quick change with your interface looks like:
conf = confmng.config()
conf['sofia']['profiles']['external']['settings']['sip-port'] = '9999'
confmng.load(conf)
versus
confmng.sofia.config['profiles']['external']['settings']['sip-port'] = '9999'
confgmng.commit()
Now I totally get the argument for immutability and the corollary pushing state via inputs, but it's still less efficient both memory and run-time wise. That being said I do like it a lot and think it's worth further discussion. What's your opinion on supporting both?
Also what about the idea of a config
per section?
Say,
sofia_confg = confmng.sofia.config()
dir_conf = confmng.directory.config()
sofia_conf['profiles']['internal']['settings']['sip-port'] = '9999'
dir_conf['default']['groups']['default']['users']['myuser'] = {
'params': {
'password': pw,
'vm-password': pw,
}
}
confmng.sofia.load(sofia_conf)
confmng.directory.load(dir_conf)
It means letting the user deal with smaller more manageable piece-wise config data structures.
@moises-silva I'm liking the confmanager.<module>.<config>/<method>
scheme. Like you guessed, I'm not sure I like .api
too much; seems a bit redundant. Also note there's a confmng.cli
which is a wrapper around making simple fs_cli
calls so maybe I'm suffering some distinction bias with that thinking...
Issue by tgoodlet Wednesday Oct 12, 2016 at 16:39 GMT Originally opened as https://github.com/sangoma/sandswitches/issues/9
I was thinking about a more clean way to design the API for components in the config which can be started/stopped.
Currently you can do something like:
But I was thinking maybe it makes more sense to have a standard method on the
ConfigManager
or elsewhere that supports this:Any thoughts @moises-silva @vodik?