transfers: add config file for log/db/pid file paths

[hakamine]

Avoid creating log/db/pid files in the code path:

• Make models.py and transfer.py find config information in file /etc/archivematica/automation-tools/automation-tools.conf
• Provide example config files in etc/ directory

[Hwesta]

A bit of minor feedback, otherwise :+1:

[hakamine]

I think it's ready now. I had to move things around a little (such as putting the code of models.py inside a function, and taking ArgumentParser related code out of main in transfers.py) in order to allow the scripts to get config parameters from a config file specified in the command line. I did a few tests, seems to be working fine so far. The README has also been updated, added new information on installation, and how to set up multiple instances.

[Hwesta]

It's promising, but I have a few Python best practices/style suggestions. The docs work looks great!

A few general Python tips:

• Scripts shouldn't have anything at the top level - everything should be in an function or the if '__name__' == __main__ which only runs when it's a run as a script (not on import).
• Classes are only declared at the top level.
• Python prefers snake_case for variable & function names (CamelCase for classes).

So, some suggestions for fixing this:

• In models.init, put the class declaration and Base = declarative_base() back at the top level. Having an init function is a good idea (like Django has setup()), but it should only contain thing the stuff that needs config.
• In transfers, add a setup function that calls the models.init, and configures logging. Also, make the config file var available globally. This is useful for the next point
• Add a get_setting that parses the config file and returns the result, similar to what Archivematica has.
• Since pidFileName/pidFileDirectory, logDirectory/logFileName and databaseDirectory/databaseName are only ever used together, why don't we combine them (pidfile, logfile and database?).

Some suggestions for what this might look like

def get_setting(setting, default=None):
    config = configparser.SafeConfigParser()
    config.read(CONFIG_FILE)
    config.get(setting, default)

# Example:
pid_file = get_setting('pidfile')

# At the top of the file
CONFIG_FILE = None

# Later...
def setup(config_file):
    global CONFIG_FILE
    CONFIG_FILE = config_file
    init(get_setting('database'))  # This is models.init

    # Configure logging
    default_logfile = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'automate-transfer.log')
    CONFIG = {
        'version': 1,
        'disable_existing_loggers': False,
        'formatters': {
            'default': {
                'format': '%(levelname)-8s  %(asctime)s  %(filename)s:%(lineno)-4s %(message)s',
                'datefmt': '%Y-%m-%d %H:%M:%S',
            },
        },
        'handlers': {
            'console': {
                'class': 'logging.StreamHandler',
                'formatter': 'default',
            },
            'file': {
                'class': 'logging.handlers.RotatingFileHandler',
                'formatter': 'default',
                'filename': get_setting('logfile', default_logfile),
                'backupCount': 2,
                'maxBytes': 10 * 1024,
            },
        },
        'loggers': {
            'transfer': {
                'level': 'INFO',  # One of INFO, DEBUG, WARNING, ERROR, CRITICAL
                'handlers': ['console', 'file'],
            },
        },
    }
    logging.config.dictConfig(CONFIG)

    # Other parsing code here...
    parser.add_argument('-c', '--config-file', help='Configuration file (log/db/PID files)', default='/etc/archivematica/automation-tools/transfers.conf')
    args = parser.parse_args()

    setup(args.config_file)
    # Rest of function...

Let me know if you need any clarifications about what I meant or what would be better.

[hakamine]

Thank you for all the advice and suggestions. Made changes following those, code looks more readable now (and is hopefully ready for merging)

[Hwesta]

Looks much better now, thanks! A couple of minor comments again, and you can probably squash the refactor commit into the initial one if you want.

[hakamine]

As mentioned in http://stackoverflow.com/questions/11536764/attempted-relative-import-in-non-package-even-with-init-py there is an issue invoking python scripts when relative imports are used (from . import models). Using the -m flag in python is a workaround that seems to fix this problem. README has been updated with this information (so hopefully it's ready to merge this time)

[Hwesta]

Looks great! Good find with the -m. :+1: