Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This tool adds a new OS user, create directories and configuration files for Apache + NGINX, MySQL (MariaDB) database and user.

I tried to take into account the comments from my previous post. Hope - this look little better.

Tool is big enough, so - will not post all it's code here, just few main parts - I'm sure, there is some places to improve (as I'm newbie in Python). Full version available here.

First - some classes.

Next class takes TEMPLATE file, and edit it to create new virtualhost config:

class MainClass(object):

        user = None

        def __init__(self, user, password, domain):
                self.user = user
                self.password = password
                self.domain = domain
    ...
class SystemOperations(MainClass):
    ...
    def createconf(self, configin, configout, oflist):
            self.configin = configin
            self.configout = configout
            self.oflist = oflist
            tolist = (self.user, self.domain)

            shutil.copy(self.configin, self.configout)

            for of, to in zip(self.oflist, tolist):
                    with open(self.configout, 'r') as infile:
                            data = infile.readlines()

                    with open(self.configout, 'w') as infile:
                            for string in data:
                                    infile.write(re.sub(of, to, string))
     ...

And it's call inside script:

    ...
    # used when creates Apache and NGINX configuration files
    config_dirs = ('/etc/httpd/conf.d/', '/etc/nginx/conf.d/')
    ...
    ## create config file
    # data which will looking for
    # in template configs to change it
    oflist = ('USER', 'DOMAIN')
    # will change to this variables
    tolist = (user, domain)

    # for both Apache and NGINX config dirs
    for i in config_dirs:
            # create var with name of template file
            configin = i + 'TEMPLATE'
            # creat var with name of new config file
            configout = i + domain + '.conf'
            print('\nCreating new configuration file %s.' % configout)
            if not os.path.isfile(configout):
                    try:
                            sysacts.createconf(configin, configout, oflist)
                            print('Done.')
                    except IOError as e:
                            print('ERROR: %s' %e)
                            sys.exit(6)
            else:
                    print('File %s already present, skipping.' % configout)
     ...

As example - Apache TEMPLATE file:

# cat /etc/httpd/conf.d/TEMPLATE
<VirtualHost 127.0.0.1:8080>
  DocumentRoot /var/www/vhosts/USER/DOMAIN
  ServerName DOMAIN
  ServerAlias www.DOMAIN
  CustomLog /var/log/httpd/DOMAIN-access.log combined
  ErrorLog /var/log/httpd/DOMAIN-error.log

  <IfModule mod_fcgid.c>
    SuexecUserGroup USER USER
    <Directory /var/www/vhosts/USER/DOMAIN>
      Options +ExecCGI
      AllowOverride All
      AddHandler fcgid-script .php
      FCGIWrapper /var/www/php-cgi/USER/DOMAIN/php.cgi .php
      Order allow,deny
      Allow from all
    </Directory>
  </IfModule>
</VirtualHost>

And second part - MySQL user and database creation.

Its class:

... class mysqlUserDb(MainClass):

        warnings.filterwarnings('error')

        def __init__(self, dbroot, dbhost, dbrootpw, *args):
                super(mysqlUserDb, self).__init__(*args)

                self.dbroot = dbroot
                self.dbhost = dbhost
                self.dbrootpw = dbrootpw

                self.db = MySQLdb.connect(self.dbhost, self.dbroot, self.dbrootpw)
                self.cursor = self.db.cursor()

        def createdb(self, dbuserdb):

                self.dbuserdb = dbuserdb

                self.cursor.execute('create database if not exists ' + self.dbuserdb)
                self.cursor.execute("show databases like '%s' " % self.dbuserdb)
                self.dbs = self.cursor.fetchone()
                return self.dbs
  ...

And its call in script:

if options.mysql:

        print('\nStarting MySQL actions.')

        if options.dbrootpw == None:
                dbrootpw = getpass.getpass('\nPlease, enter MySQL root password: ')


        # we must have database name
        if dbuserdb == None:
                print('\nERROR! please, set `-m` option with User database name.\n')
                sys.exit(7)

        warnings.filterwarnings('error')

        # Open and check MySQL connection
        try:
                dbconnect = fun.mysqlUserDb(dbroot, dbhost, dbrootpw, user, password, domain)
                print('Connection opened successfully.')
        except MySQLdb.Error as e:
                print('MySQL error: %s \nExit.\n' % e)
                sys.exit(8)

        # create database
        try:
                print('\nCreating database %s.' % dbuserdb)
                dbconnect.createdb(dbuserdb)
                print 'Database created: %s' % dbconnect.dbs
        except Warning as error:
                print('MySQL warning: %s' % error)
        except MySQLdb.Error as e:
                print('MySQL error: %s \nExit.\n' % e)
                sys,exit(9)

        # add user and grant prielegies
        try:
                print('\nAdding privilegies to user %s with password %s to database %s.' % (user, password, dbuserdb))
                dbconnect.grants(dbuserdb)
                print('Access granted: %r' % dbconnect.grs)
        except MySQLdb.Error as e:
                print('MySQL error: %s \nExit.\n' % e)
                sys,exit(10)
    # close connection
    print('\nFinishing MySQL actions...')
    del dbconnect
    print 'Done.\n'

else:
    print('Skipping MySQL actions.')
share|improve this question

1 Answer 1

up vote 1 down vote accepted

The user = None here is pointless, confusing, and you're not using it anyway, so just drop it:

class MainClass(object):

        user = None

        def __init__(self, user, password, domain):
                self.user = user

There are multiple inefficiencies here:

shutil.copy(self.configin, self.configout)

for of, to in zip(self.oflist, tolist):
        with open(self.configout, 'r') as infile:
                data = infile.readlines()

        with open(self.configout, 'w') as infile:
                for string in data:
                        infile.write(re.sub(of, to, string))
  1. You copy a file with shutil.copy, but then you rewrite it anyway in the loop.
  2. For each of, to pair, you rewrite the output file
  3. You apply the pattern substitution line by line

Refactor like this:

  1. Drop the shutil.copy step
  2. Read from the input file into a buffer, instead of getting its lines
  3. Loop over the of, to pairs, and apply the pattern substitutions to the data buffer
  4. Write the output file

This is not Pythonic:

if options.dbrootpw == None:
    # ...

if dbuserdb == None:
    # ...

Write like this:

if not options.dbrootpw:
    # ...

if not dbuserdb:
    # ...

Some of your variable names are really not so good, for example:

for i in config_dirs:

i is commonly used for loop counters. In general, avoid single-letter variable names. In this example, path would have been a better name.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.