Argument parser for a PostgreSQL backup utility

Question

I'm sure it's still not yet perfect and tips will be greatly appreciated!

I just re-read the script and I realised I'm not handling exceptions! The script is unfinished but I'm looking for WIP (work in progress) feedback to shape the direction I continue. You can avoid commenting on the lack of exception handling.

I'd like help in particular with the parse args part of the script. I'm unsure how I can pass the keyword variables to different parts of the scripts while maintaining default values. Is it possible to leave the parameters required=False and have default values in the fs_backup or sql_dump methods? Currently, if the parameters are empty it will pass None and override the defaults. I know this comes down to how I call the backup() method, but I don't think I'm ready for passing args* and kwargs** throughout the script yet! How can I fix this?

#!/usr/bin/python
#########################################################################################
#                                                                                       #
#                           PostgreSQL Backup Database'                                 #
#                                                                                       #
#                            The MIT License (MIT)                                      #
#                                                                                       #
#                        Copyright (c) 2016 Alan Kavanagh                               #
#                                                                                       #
#   Permission is hereby granted, free of charge, to any person obtaining a copy        #
#     of this software and associated documentation files (the "Software"),             #
# to deal in the Software without restriction, including without limitation the rights  #
#     to use, copy, modify, merge, publish, distribute, sublicense, and/or sell         #
#  copies of the Software, and to permit persons to whom the Software is furnished      #
#          to do so, subject to the following conditions:                               #
#                                                                                       #
#   The above copyright notice and this permission notice shall be included in all      #
#                 copies or substantial portions of the Software.                       #
#                                                                                       #
#########################################################################################
import sys
from os.path import exists
from argparse import ArgumentParser
from lib.common.utils import kb_interruptable
from lib.common.utils import database_exists, exec_cmd, is_valid_user, timestamp, get_database_oid
from lib.common.utils import syslog_info, syslog_warning, syslog_error


BIN = '/usr/bin'
PSQL = '{0}/psql'.format(BIN)
PGDUMP = '{0}/pg_dump'.format(BIN)
PGDATA_BASE = '/opt/dydev/postgresql/data/base'
DEFAULT_BACKUP_DIR = '/var/lib/pgsql/9.5/backups'


def sql_dump(database=None, location=DEFAULT_BACKUP_DIR, filename='pg_backup'):
    """
    Creates an SQL dump file of a database
    :param database: Database name
    :param location: Where to store the SQL dump
    :param filename: Backup filename
    """
    try:
        if not database_exists(database):
            syslog_error('{0} doesnt exist'.format(database))
            raise Exception

        dump_database = '{0} {1} > {2}/{3}-{4}.tgz'.format(
            PGDUMP, database, location, filename, timestamp())

        exec_cmd(dump_database)
        syslog_info('{0} dumped to {1}'.format(database, location))
    except Exception:
        raise


def fs_backup(database=None, location=DEFAULT_BACKUP_DIR, filename='pg_backup'):
    """
    Creates a filesystem backup of a database
    :param database: Database name
    :param location: Where to store the FS backup
    :param filename: Backup Filename
    """
    try:
        fs = PGDATA_BASE

        if database:
            fs += '/{0}'.format(get_database_oid(database))

        if not exists(location) or not exists(fs):
            syslog_error('{0} or {1} doesnt exist'.format(location, fs))
            raise Exception

        backup_database = \
            'tar -cf {0}/{1}-{2}.tgz -C {3} .'.format(location, filename, timestamp(), fs)

        exec_cmd(backup_database)
        syslog_info('{0} backed up to {1}'.format(fs, location))
    except Exception:
        raise


def backup(option, database, location,  filename):
    """
    Calls the appropriate backup method
    :param option: Type of backup
    :param database: Database to backup
    :param location: Where to store the backup file
    :param verbose: Enable logging
    """
    option_function = {'sql': sql_dump,
                       'fs': fs_backup}

    option_function[option](database, location, filename)


def create_parser():
    """
    Creates the argument parser of the parameters passed to the script
    :return: parsed arguments
    """
    parser = ArgumentParser(description='Argument parser for PG backup')
    parser.add_argument('--option', dest='option', 
                        required=True, choices=['fs', 'sql'],
                        help='Backup selection sqldump/backup')
    parser.add_argument('--database', dest='database', required=True,
                        help='Database to backup')
    parser.add_argument('--location', dest='location', required=True,
                        help='Location to store the backup')
    parser.add_argument('--filename', dest='filename', required=True,
                        help='The named of backup file')
    return parser


def interrupt_handler():
    """
    Callback for CTRL-c handling
    Cleanup method!
    """
    syslog_error('CTRL-C: Interrupting restore..')


@kb_interruptable(callback=interrupt_handler)
def main(args):
    """
    Executes the main thread of the script creating a backup of the PG DB
    :param args: script arguments
    """
    parser = create_parser()
    parsed_args = parser.parse_args(args[1:])
    backup(option=parsed_args.option,
           database=parsed_args.database,
           location=parsed_args.location,
           filename=parsed_args.filename)


if __name__ == '__main__':
    if not is_valid_user('postgres'):
        exit('User must be postgres')
    main(sys.argv)

Answer 1

You raise many exceptions, but you are no more specific than Exception. You should use a more specific class and give a message of some kind. You give the message to the error log, but it should also be in the traceback.

Your create_parser function says that it returns the parsed arguments, but it actually returns the parser. Fix your documentation. Your description of the program seems more like a comment about the current line. Again, fix your documentation. I like that you split up your lines. That makes it much more readable. A small tidbit of information: if you say parser.add_argument("--options"), the location will by default be options. One last point, I would add some short options. That is, add '-o' when you add the options argument, etc. That way, the user can use some shortcuts. When you call parser.parse_args(), you pass it argv[1:]. You don't need to do that. Just don't pass it anything, and it will figure it out.

Overall, your code looks pretty good. It is well laid out, your functions are all documented, you sanitize your input, and I haven't noticed any violations of PEP 8.

Answer 2

Sorry to say that, your code is far from perfect.

Use os.path.join to join paths:

PSQL = os.path.join(BIN, 'psql')

Use standard libraries instead of self written ones. logging.info instead syslog_info, subprocess.call instead of exec_cmd, etc.

Use more specific exceptions than Exception. An except-Block without any error handling is superfluous.

Default values for function arguments, which aren't a default, is weird.

def sql_dump(database, location=DEFAULT_BACKUP_DIR, filename='pg_backup'):
    """
    Creates an SQL dump file of a database
    :param database: Database name
    :param location: Where to store the SQL dump
    :param filename: Backup filename

    Raises subprocess.CalledProcessError on error.
    """
    filename = os.path.join(location, '{0}-{1:%Y-%m-%d}.tgz'.format(filename, datetime.datetime.now()))
    with open(filename, 'wb') as output:
        subprocess.check_call([PGDUMP, database], stdout=output)
    logging.info('{0} dumped to {1}'.format(database, location))