Merge lp:~donkirkby/openobject-server/config-defaults into lp:openobject-server/5.0

[Don Kirkby]

Make all configuration options available on command line and in configuration file as discussed in bug lp:392114

[Don Kirkby (donkirkby)]

This is the work that I proposed in the bug discussion. I have tested it with all the options described in the code, and compared the in-memory values to those generated by

the old code. I also compared the configuration files saved by my new code and the old code.

In addition to adding support for all options in the configuration file and the command line, the following behaviours have changed:

- The reference to the soap option had been commented out from openerp-server.py and it wasn't allowed on the command line, so I removed it from the defaults.
- translate_modules now reads and writes to the configuration file in the same format that it uses on the command line. If it sees the old format, it continues to ignore it

during load, just like the old code did.
- netport and port used to be strings when loaded from a configuration file, and integers when loaded from the command line. They are now always integers.
- demo is no longer saved to the configuration file. (It was ignored during load, so this should be irrelevant.)

I did not get around to adding new names for the command line options that don't match their names in the configuration file.

I tried to break up the changes into smaller revisions to make the review easier. If there's something I can do to make my next proposal easier to review, please let me know.

[lp:~donkirkby/openobject-server/config-defaults]

1954. By Don Kirkby on 2010-04-29

merge 5.0.9 from 5.0 branch

[Don Kirkby (donkirkby)]

When I was merging release 5.0.9, I noticed that three people have touched the tools/config.py file recently, so I requested reviews from them for my fix to bug lp:392114 about configurations options not always working in both the config file and the command line.

[lp:~donkirkby/openobject-server/config-defaults]

1955. By Don Kirkby on 2010-11-05

[MERGE] release 5.0.15 from branch 5.0

[Olivier Dony (Odoo) (odo-openerp)]

Hello,

This is interesting, but please resubmit your changes for trunk, as we cannot merge any improvement on 5.0.
Also I'm not sure it makes sense to expose _all_ options as command-line args, because some could be harmful, like the super-admin password or the root_path...

Thanks!

review: Needs Resubmitting

Unmerged revisions

1955. By Don Kirkby on 2010-11-05

[MERGE] release 5.0.15 from branch 5.0

1954. By Don Kirkby on 2010-04-29

merge 5.0.9 from 5.0 branch

1953. By Don <don@don-desktop-openerp> on 2010-02-11

Remove debug code that I added in revision 1949.

1952. By Don <don@don-desktop-openerp> on 2010-02-11

Remove default values of None. Add some options to the command line that were only available in config files. Convert some parameters to strings for backward compatibility. Save translate_modules as comma separated list without brackets or quotes.

1951. By Don <don@don-desktop-openerp> on 2010-02-10

copy all options from OptionParser to config.options

1950. By Don <don@don-desktop-openerp> on 2010-02-09

move all default values from the option definitions to a dictionary called defaults.

1949. By Don <don@don-desktop-openerp> on 2010-02-09

some debug code to dump the contents of tools.config.options (I'll remove it before making a merge proposal)

=== modified file 'bin/tools/config.py'
--- bin/tools/config.py	2010-09-08 12:55:30 +0000
+++ bin/tools/config.py	2010-11-05 21:37:16 +0000
@@ -40,7 +40,7 @@
 class configmanager(object):
     def __init__(self, fname=None):
         hasSSL = check_ssl()
-        self.options = {
+        defaults = {
             'email_from':False,
             'interface': '',    # this will bind the server to all interfaces
             'port': 8069,
@@ -55,19 +55,11 @@
             'reportgz': False,
             'netrpc': True,
             'xmlrpc': True,
-            'soap': False,
-            'translate_in': None,
-            'translate_out': None,
-            'language': None,
-            'pg_path': None,
+#            'soap': False,  #reference is commented out in bin/openerp-server.py
             'admin_passwd': 'admin',
             'csv_internal_sep': ',',
-            'addons_path': None,
-            'root_path': None,
             'debug_mode': False,
             'import_partial': "",
-            'pidfile': None,
-            'logfile': None,
             'smtp_server': 'localhost',
             'smtp_user': False,
             'smtp_port':25,
@@ -76,14 +68,17 @@
             'price_accuracy': 2,
             'secure' : False,
             'syslog' : False,
-            'log_level': logging.INFO,
-            'assert_exit_level': logging.WARNING, # level above which a failed assert will be raise
+            'log_level': 'info',
+            'assert_exit_level': 'warn', # level above which a failed assert will be raise
             'cache_timeout': 100000,
             'login_message': False,
             'list_db': True,
+            'server_actions_allow_code': False,
+            'translate_modules': 'all',
+            'without_demo': False
         }
         if hasSSL:
-            self.options.update({
+            defaults.update({
                 'secure_cert_file': 'server.cert',
                 'secure_pkey_file': 'server.pkey',
                 'smtp_ssl': False,
@@ -97,7 +92,7 @@
         parser = optparse.OptionParser(version=version)
 
         parser.add_option("-c", "--config", dest="config", help="specify alternate config file")
-        parser.add_option("-s", "--save", action="store_true", dest="save", default=False,
+        parser.add_option("-s", "--save", action="store_true", dest="save",
                           help="save configuration to ~/.openerp_serverrc")
         parser.add_option("--pidfile", dest="pidfile", help="file where the server pid will be stored")
 
@@ -109,16 +104,16 @@
         parser.add_option("--no-xmlrpc", dest="xmlrpc", action="store_false", help="disable xmlrpc")
         parser.add_option("-i", "--init", dest="init", help="init a module (use \"all\" for all modules)")
         parser.add_option("--without-demo", dest="without_demo",
-                          help="load demo data for a module (use \"all\" for all modules)", default=False)
+                          help="load demo data for a module (use \"all\" for all modules)")
         parser.add_option("-u", "--update", dest="update",
                           help="update a module (use \"all\" for all modules)")
         parser.add_option("--cache-timeout", dest="cache_timeout",
                           help="set the timeout for the cache system", type="int")
 
         # stops the server from launching after initialization
-        parser.add_option("--stop-after-init", action="store_true", dest="stop_after_init", default=False,
+        parser.add_option("--stop-after-init", action="store_true", dest="stop_after_init",
                           help="stop the server after it initializes")
-        parser.add_option('--debug', dest='debug_mode', action='store_true', default=False, help='enable debug mode')
+        parser.add_option('--debug', dest='debug_mode', action='store_true', help='enable debug mode')
         parser.add_option("--assert-exit-level", dest='assert_exit_level', type="choice", choices=self._LOGLEVELS.keys(),
                           help="specify the level at which a failed assertion will stop the server. Accepted values: %s" % (self._LOGLEVELS.keys(),))
         parser.add_option('--price_accuracy', dest='price_accuracy', type='int', help='specify the price accuracy')
@@ -133,11 +128,24 @@
                               help="specify the private key file for the SSL connection")
             parser.add_option_group(group)
 
+        parser.add_option("--admin-passwd",
+                          help="set the super administrator password " +
+                          "(Warning: it is much better to set this in a " +
+                          "configuration file because command line arguments " +
+                          "are visible to other users.)")
+        parser.add_option("--csv-internal-sep",
+                          help="set the separator character to use in import files")
+        parser.add_option("--login-message",
+                          help="set the message displayed to users at log in")
+        parser.add_option("--reportgz",
+                          action="store_true",
+                          help="return reports in a compressed format")
+
         # Logging Group
         group = optparse.OptionGroup(parser, "Logging Configuration")
         group.add_option("--logfile", dest="logfile", help="file where the server log will be stored")
         group.add_option("--syslog", action="store_true", dest="syslog",
-                         default=False, help="Send the log to the syslog server")
+                         help="Send the log to the syslog server")
         group.add_option('--log-level', dest='log_level', type='choice', choices=self._LOGLEVELS.keys(),
                          help='specify the level of the logging. Accepted values: ' + str(self._LOGLEVELS.keys()))
         parser.add_option_group(group)
@@ -163,7 +171,7 @@
         group.add_option("--db_maxconn", dest="db_maxconn", type='int',
                          help="specify the the maximum number of physical connections to posgresql")
         group.add_option("-P", "--import-partial", dest="import_partial",
-                         help="Use this for big data importation, if it crashes you will be able to continue at the current state. Provide a filename to store intermediate importation states.", default=False)
+                         help="Use this for big data importation, if it crashes you will be able to continue at the current state. Provide a filename to store intermediate importation states.")
         parser.add_option_group(group)
 
         group = optparse.OptionGroup(parser, "Internationalisation options",
@@ -183,12 +191,16 @@
         group.add_option("--addons-path", dest="addons_path",
                          help="specify an alternative addons path.",
                          action="callback", callback=self._check_addons_path, nargs=1, type="string")
+        group.add_option("--root-path",
+                         help="path to the core server code (defaults to the location of this script)",
+                         action="callback", callback=self._check_addons_path, nargs=1, type="string")
         parser.add_option_group(group)
+        parser.set_defaults(**defaults)
 
         security = optparse.OptionGroup(parser, 'Security-related options')
         security.add_option('--no-database-list', action="store_false", dest='list_db', help="disable the ability to return the list of databases")
         security.add_option('--enable-code-actions', action='store_true',
-                            dest='server_actions_allow_code', default=False,
+                            dest='server_actions_allow_code',
                             help='Enables server actions of state "code". Warning, this is a security risk.')
         parser.add_option_group(security)
 
@@ -199,15 +211,6 @@
                 print msg
                 sys.exit(1)
 
-        die(bool(opt.syslog) and bool(opt.logfile),
-            "the syslog and logfile options are exclusive")
-
-        die(opt.translate_in and (not opt.language or not opt.db_name),
-            "the i18n-import option cannot be used without the language (-l) and the database (-d) options")
-
-        die(opt.translate_out and (not opt.db_name),
-            "the i18n-export option cannot be used without the database (-d) option")
-
         # place/search the config file on Win32 near the server installation
         # (../etc from the server)
         # if the server is run by an unprivileged user, he has to specify location of a config file where he has the rights to write,
@@ -219,8 +222,34 @@
 
         self.rcfile = os.path.abspath(
             fname or opt.config or os.environ.get('OPENERP_SERVER') or rcfilepath)
-        self.load()
-
+        if self.__load_defaults(defaults):
+            parser.set_defaults(**defaults)
+            (opt, args) = parser.parse_args()
+        
+        die(bool(opt.syslog) and bool(opt.logfile),
+            "the syslog and logfile options are exclusive")
+
+        die(opt.translate_in and (not opt.language or not opt.db_name),
+            "the i18n-import option cannot be used without the language (-l) and the database (-d) options")
+
+        die(opt.translate_out and (not opt.db_name),
+            "the i18n-export option cannot be used without the database (-d) option")
+
+        self.options = {}
+        for (arg, value) in opt.__dict__.iteritems():
+            if arg in ('config', 'save'):
+                continue #options are only used during parse process
+            elif arg in ('db_maxconn', 'db_port', 'price_accuracy'):
+                #convert some numeric parameters back to strings just
+                #for backward compatibility
+                #netport and port used to be strings from a config file
+                #and ints from the command line, so now we leave them as ints
+                value = str(value)
+                if value == 'False':
+                    value = False
+            elif arg == 'smtp_ssl' and value is None:
+                continue #backward compatibility
+            self.options[arg] = value
 
         # Verify that we want to log or not, if not the output will go to stdout
         if self.options['logfile'] in ('None', 'False'):
@@ -229,29 +258,6 @@
         if self.options['pidfile'] in ('None', 'False'):
             self.options['pidfile'] = False
 
-        keys = ['interface', 'port', 'db_name', 'db_user', 'db_password', 'db_host',
-                'db_port', 'logfile', 'pidfile', 'smtp_port', 'cache_timeout',
-                'email_from', 'smtp_server', 'smtp_user', 'smtp_password', 'price_accuracy',
-                'netinterface', 'netport', 'db_maxconn', 'import_partial', 'addons_path']
-
-        if hasSSL:
-            keys.extend(['secure_cert_file', 'secure_pkey_file'])
-
-        for arg in keys:
-            if getattr(opt, arg):
-                self.options[arg] = getattr(opt, arg)
-
-        keys = ['language', 'translate_out', 'translate_in', 'debug_mode',
-                'stop_after_init', 'without_demo', 'netrpc', 'xmlrpc', 'syslog',
-                'list_db', 'server_actions_allow_code']
-
-        if hasSSL and not  self.options['secure']:
-            keys.extend(['secure', 'smtp_ssl'])
-
-        for arg in keys:
-            if getattr(opt, arg) is not None:
-                self.options[arg] = getattr(opt, arg)
-
         if opt.assert_exit_level:
             self.options['assert_exit_level'] = self._LOGLEVELS[opt.assert_exit_level]
         else:
@@ -352,33 +358,59 @@
 
 
         setattr(parser.values, option.dest, res)
-
+    
     def load(self):
+        """ Load values from a configuration file specified by self.rcfile
+        into the dictionary self.options.
+        
+        Not used in the main code, just here for backward compatibility.
+        """
+        self.__load_defaults(self.options)
+
+    def __load_defaults(self, defaults):
+        """ Load values from a configuration specified by self.rcfile.
+        
+        Returns true if the values were successfully loaded. Any reading
+        errors are swallowed and the method returns false.
+        """
         p = ConfigParser.ConfigParser()
+        isLoaded = False
         try:
             p.read([self.rcfile])
             for (name,value) in p.items('options'):
-                if value=='True' or value=='true':
+                if name == 'demo':
+                    continue
+                elif name in ('init', 'update', 'translate_modules') and value.startswith(('[', '{')):
+                    continue #old config file with invalid format so ignore
+                elif value=='True' or value=='true':
                     value = True
-                if value=='False' or value=='false':
+                elif value=='False' or value=='false':
                     value = False
-                self.options[name] = value
+                defaults[name] = value
+            isLoaded = True
         except IOError:
             pass
         except ConfigParser.NoSectionError:
             pass
+        return isLoaded
 
     def save(self):
         p = ConfigParser.ConfigParser()
         loglevelnames = dict(zip(self._LOGLEVELS.values(), self._LOGLEVELS.keys()))
         p.add_section('options')
-        for opt in self.options.keys():
-            if opt in ('version', 'language', 'translate_out', 'translate_in', 'init', 'update'):
+        for (opt, value) in self.options.iteritems():
+            if opt in ('version', 'language', 'translate_out', 'translate_in', 'init', 'update', 'demo'):
                 continue
-            if opt in ('log_level', 'assert_exit_level'):
-                p.set('options', opt, loglevelnames.get(self.options[opt], self.options[opt]))
-            else:
-                p.set('options', opt, self.options[opt])
+            if opt == 'translate_modules':
+                if not isinstance(value, list) or len(value) == 0:
+                    continue
+                value.sort()
+                value = ','.join(value)
+            elif opt in ('log_level', 'assert_exit_level'):
+                default = value
+                value = loglevelnames.get(value, default)
+                
+            p.set('options', opt, value)
 
         # try to create the directories and write the file
         try: