4
\$\begingroup\$

I'm creating a command-line tool which is supposed to work on both Windows and Linux OS. It will be used to manage our application - start, stop, deploy etc.

We have it in bash scripts now (and it works very well for about 6 month), but I want rewrite it in Python with classes.

It's big enough, so here are only a few main parts.

I'm first trying to use classes here. My idea is to have one Main class, which will be used by all other child classes to get necessary variables (does classes && inheritance intend at all for such "role"?).

Whole system based on two environment variables - ENV (environment, where tool started, depending on it - will use different setting files for application on so on), and HOME - path to where application files, manager-tool, Java, Tomcat etc placed.

I'm afraid classes here will only it more "spaghetti" with my hands. What am I doing wrong here, mainly about classes usage?

Main file - APPmanager.py contains:

#!/usr/bin/env python

import argparse, os, sys, time

# I have directory 'lib' with empty __init__.py there
# in this dir - all calsses files will be placed
from lib import main_functions

def getopts():

    # totally - there is about 20 options

    parser = argparse.ArgumentParser()

    parser.add_argument('-s', '--start', help=('Start APP'), action='store_true')
    parser.add_argument('-S', '--stop', help=('Stop APP'), action='store_true')
    parser.add_argument('-F', '--stopforce', help=('Shutdown-force APP (30 sec wit, then kill -KILL)'), action='store_true')
    parser.add_argument('-c', '--status', help=('Display APP status'), action='store_true')
    parser.add_argument('-R', '--restart', help=('Restart APP'), action='store_true')

    parser.add_argument('-v', '--version', help=('Set new APP VERSION'), metavar = 'BUILD NUMBER')

    parser.add_argument('-t', '--talog', help=('Display content of app-app.log with tail'), action='store_true')
    parser.add_argument('-l', '--catalog', help=('Display content of catalina.out with less'), action='store_true')
    parser.add_argument('-L', '--acclog', help=('Show localhost_access_log.(date).txt output on screen'), action='store_true')
    parser.add_argument('-q', '--manalog', help=('Display logs/app-manager.log with less'), action='store_true')

    parser.add_argument('-C', '--runconfig', help=('Run Tomcat reconfiguration process - creates server.xml, some symlinks etc'), action='store_true')
    parser.add_argument('-u', '--selfupgrade', help=('Download archive with app-manager directory from Nexus, delete current one, and unpack new fiels'), action='store_true')
    parser.add_argument('-d', '--deployapp', help=('Download archive with given APP version in to app-application directory'), action='store_true')
    parser.add_argument('-e', '--encryptconf', help=('Encrypting configuration files in conf/catalina directory'), action='store_true')
    parser.add_argument('-b', '--decryptconf', help=('Decrypting configuration files in conf/catalina directory'), action='store_true')
    parser.add_argument('-E', '--encryptpwd', help=('Encrypting input passwords and return hashed password'), action='store_true')
    parser.add_argument('-D', '--checkdbcon', help=('Check connections to all databases in external.properties'), action='store_true')
    parser.add_argument('-Q', '--scp', help=('Copy specified file to main host'), action='store_true')

if len(sys.argv) <= 1:
    parser.print_help()
    sys.exit(1)
else:
    return(parser.parse_args())

def runaction(ENV):

    # will select each option = True
    # import necessary class fil from 'lib'
    # and call appropriate method

    if options_set.start:
        from lib import appmanage_functions
        print('Start Tomcat')
        '''appmanage_functions.AppManage - contains all application commands - start, stop, status, etc'''
        a = appmanage_functions.AppManage(ENV)
        a.start(ENV)

       if options_set.status:
           from lib import appmanage_functions
           print('Tomcat status')
           appmanage_functions.status(main_functions.CATALINA_PID)

       [other options below]

if __name__ == '__main__':

    env = os.environ['ENV']

    options_set = getopts()

    main_f = main_functions.Main(env)
    main_f.act_opts(options_set)

    print('\nAPPmanager started at %s ' % (time.strftime('%d, %b %Y at %H:%M:%S')))
    print('Working on %s environment with APP_BASEDIR=%s.' % (env, main_f.APP_BASEDIR))
    print('Running with options: %s \n' % main_f.opts2)

    runaction(env)

File main_functions, with class Main will be parent for all other classes, to pass "global variables" to them:

import os, sys, platform

class Main:

    def __init__ (self, env):

        '''Will set APP_BASEDIR - main APP variable, pointed to $HOME/APP directory.
           Also - will set $smem and $xmem settings for CATALINA_OPTS'''

        print('ENV: %s\n' % env)

        list = ('SIT', 'DEV', 'UAT', 'REG')

        try:

            self.HOME = os.environ['HOME']

            if env == 'PROD':
                self.APP_BASEDIR = '/usr/local/app_install/APP'
                self.smem = '64G'
                self.xmem = '64G'
            else:
                self.APP_BASEDIR = os.path.join(self.HOME, 'APP')

                if env in list:
                    self.smem = '32G'
                    self.xmem = '32G'
                else:
                    self.smem = '2G'
                    self.xmem =  '2G'

        except KeyError as e:
            print('Can\'t find HOME variable! Exit: %s' % e)

    def setvars(self, env):

        APP_INSTALL = os.path.join(self.APP_BASEDIR, 'app-manager')

        self.APP_APP = os.path.join(self.APP_BASEDIR, 'app-application')

        self.APP_CONFDIR = os.path.join(APP_INSTALL, 'conf')

        self.APP_CONFIGFILE = os.path.join(self.APP_CONFDIR, 'server', 'server.xml')

        APP_ENCRYPTION_PASSWORD = 'secret'

        APP_MARKER = 'secret'

        os.environ['CATALINA_HOME'] = os.path.join(self.APP_BASEDIR, 'app-server')

        os.environ['BASEDIR'] = os.environ['CATALINA_HOME']

        APP_LIBS = os.path.join(self.APP_CONFDIR, 'lib')

        CATALINA_PID  = os.path.join(os.environ['CATALINA_HOME'], 'conf', 'catalina.pid')

        os.environ['CATALINA_OUT'] = os.path.join(self.APP_BASEDIR, 'logs', 'catalina.out')

        self.LOGGING_CONFIG = os.path.join('-Djava.util.logging.config.file=' + self.APP_CONFDIR, 'server', 'logging.properties')

        LOG4J = os.path.join(self.APP_CONFDIR, 'log4j', 'log4j.' + env + '.xml')
        os.environ['LOG4J'] = LOG4J

        if platform.system() == 'Windows':
            JAVA_HOME = os.path.join(self.APP_BASEDIR, 'Java', 'Windows', 'jre')
            os.environ['JAVA_HOME'] = JAVA_HOME
            os.environ['JRE_HOME'] = os.path.join(JAVA_HOME)
            os.environ['APP_SECURE_LOCATION_PWD'] = os.path.join(self.HOME, 'security', 'secret.key')
            os.environ['APP_EXT_PROP'] = os.path.join(self.HOME, 'security', 'external.properties')
        elif platform.system() == 'Linux':
            JAVA_HOME = os.path.join(self.APP_BASEDIR, 'Java', 'Linux')
        else:
            print('Unknow OS type, exit.')
            sys.exit(1)

        JRE_HOME = os.path.join(JAVA_HOME, 'jre')

        print('JAVA_HOME = %s' % (os.environ['JAVA_HOME']))
        print('JRE_HOME = %s' % JRE_HOME)

        if env == 'PROD':
            self.OUTPUT_DIR = os.path.join('var', 'app_logs' + os.environ['hostname'])
        else:
            self.OUTPUT_DIR = os.path.join(self.APP_BASEDIR, 'logs')

        print('app_version_file = %s' % (os.path.join(self.APP_CONFDIR, 'app.version')))

        with open(os.path.join(self.APP_CONFDIR, 'app.version'), 'r') as app_version_file:
            self.APP_VERSION = app_version_file.readline()

        print('APP_VERSION: %s' % self.APP_VERSION)

    def act_opts(self, RES):

        self.opts = vars(RES)
        self.opts2 = {}

        for key in self.opts.keys():
            if self.opts[key]:
                self.opts2[key] = self.opts[key]

And file application_functions.py to manage application:

from main_functions import Main
import os, subprocess

class AppManage(Main):

    ...

    def start(self, env):

        self.setvars(env)

        os.environ['CATALINA_OPTS'] = '-d64 -server -Xms%s -Xmx%s\
        -XX:+UseCodeCacheFlushing -XX:ReservedCodeCacheSize=64M\
        -XX:ReservedCodeCacheSize=64M -XX:MaxPermSize=2048M\
        -Djavax.net.debug=ssl,handshake\
        -Denv=%s\
        -Doutput.dir=%s\
        -Dapp.app=%s\
        -Dapp.version=%s\
        -Dapp.conf=%s' % (self.smem, self.xmem, env, self.OUTPUT_DIR, self.APP_APP, self.APP_VERSION.rstrip(), self.APP_CONFDIR)

        # will fix here for Linux/Windows
        cata = os.path.join(os.environ['CATALINA_HOME'], 'bin', 'catalina.bat')

        subprocess.call(("%s start -config %s" % (cata, self.APP_CONFIGFILE)), shell=True)
\$\endgroup\$
  • \$\begingroup\$ Could you include the rest of the code, or at the very least, include the functions as a whole? Right now it's very fragmented code and I worry that answers may not be able to for see all side effects. \$\endgroup\$ – Pimgd Feb 23 '15 at 14:41
  • \$\begingroup\$ @Pimgd I don't sure... Some of them are really big, and have lot of "repeatable" parts (getopts() for example - all the same, but different options, or __init__ im Main - there is just lot of variables definitions). Is it have sense to past them too? Or - may be you could specify - what function need here to be in 'full view'? \$\endgroup\$ – setevoy Feb 23 '15 at 14:48
  • 1
    \$\begingroup\$ Repeatable parts are something you will most definitely get an answer on which you can use to improve it, so why not post it? It's okay to leave functions out, but don't post half functions. Then it just becomes hard to see what the code does where. For you it's boilerplate, for us it's "Okay, so this class manages this, and that class manages that... ... here's where it configures the things..." It's like a guidepost, so to say. Right now there's 3 votes to close your question and it's likely it will get closed soon if the code isn't expanded. \$\endgroup\$ – Pimgd Feb 23 '15 at 14:56
  • 1
    \$\begingroup\$ @setevoy I know, but just make note of that in your question and you will probably get an answer on it. If you don't, you can comment on the answer and ask about it. \$\endgroup\$ – Hosch250 Feb 23 '15 at 16:18
  • 1
    \$\begingroup\$ Have a look at this list of libraries for easy creation of CLI tools here. \$\endgroup\$ – theWanderer4865 Feb 23 '15 at 16:29
2
\$\begingroup\$

I see several issues with the first script, APPmanager.py, the others seem quite fine.

Imports

From PEP8:

  • Imports should usually be on separate lines: split import argparse, os, sys, time to multiple lines
  • Imports should be put at the top of the file: move from lib import appmanage_functions to the top, there's no good reason to do this import later

Don't execute code in global scope

It's good to make it possible to import your Python scripts as modules without executing anything. This can be useful to reuse the code in other scripts, and in unit testing too. Code like this in the global scope makes that impossible:

if len(sys.argv) <= 1:
    parser.print_help()
    sys.exit(1)
else:
    return(parser.parse_args())

Move this snippet out of the global scope.

Also, you could simplify the condition to this:

if not sys.argv:

Move code out of global scope

Code in global scope guarded by if __name__ == '__main__' is ok, but it's even better to move such code to a function, for example called main. The difference is that variables you define within the if __name__ == '__main__' may be visible in the functions you call, which can lead to confusion and bugs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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