Review: Needs Fixing
Nice first merge proposal! :-)
Here are some minor comments on the technical aspects of the patch, related to
our coding guidelines [1]:
l.9: iterating on dict keys is simply done with "for k in dict", no need to
call dict.keys()
l.10: variable names should be self-descriptive as much as possible, one-letter
names are only acceptable in lambda functions, list comprehensions and loop
variables. Here `e` could be "env_var".
And a few thoughts on the idea itself:
I think it is in general useful to support setting startup options in
environment variables. However I'm concerned about the security implications.
For example, allowing users to set the super-admin password or the database
password on the command-line or in their environment is putting them at risk of
leaking it to other operating system users (a big issue on shared hosting
environments, because not all platforms prevent users for seeing each other's
processes). So I'd be in favor of defining a list of "SECURITY_RELATED_OPTIONS"
and adding something along those lines in your patch at line 12:
assert o not in SECURITY_RELATED_OPTIONS,\
"Passing security-related options via environment is forbidden"
Finally, we should avoid conflicting with other environment variables, so
prefixing all options with "OPENERP" in the environment would be a good idea,
e.g.:
os.environ.get("OPENERP_%s" % o.upper())
Passing the options as e.g. "OPENERP_ADDONS_PATH" looks natural enough to me.
Thanks!
[1] http://doc.openerp.com/contribute/15_guidelines/coding_guidelines.html
--
https://code.launchpad.net/~openerp-dev/openobject-server/trunk-environment-vars-cto/+merge/94564
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-server/trunk-environment-vars-cto.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
