sangoma / sandswitches

Configure FreeSWITCH using a Python API
Mozilla Public License 2.0
6 stars 2 forks source link

Profile start/stop API? #9

Open goodboy opened 8 years ago

goodboy commented 8 years ago

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:

from sandswitches import manage

confmng = manage('myfshost.com', keyfile='mykey.rsa')
confmng.sofia.profiles['external'].stop()
confmng.sofia.profiles['external']['settings']['auth-calls'] = 'false'
confmng.sofia.profiles['external'].start()

But I was thinking maybe it makes more sense to have a standard method on the ConfigManager or elsewhere that supports this:

confmng.sofia.start('external')

Any thoughts @moises-silva @vodik?

goodboy commented 7 years ago

@vodik @moises-silva @jmesquita ping ping?

moises-silva commented 7 years ago

@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.

vodik commented 7 years ago

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.

goodboy commented 7 years ago

@moises-silva @vodik thanks guys both good ideas. I'm gonna draft a PR over the weekend hopefully.

goodboy commented 7 years ago

@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...