Basic automation of SSH tasks in a restricted environment

I'm trying to implement a simple automation solution in a restricted environment. I have actually implemented something but it took a bit longer than I wanted. I want to improve the solution and I'll probably have to extend it in the future so I'm asking for code review - my boss will probably be happier if I can deliver things faster! :)

Anyway, the current setup:

Python 2.6 - installed as a portable package since I don't have administrative rights on my machine.

Fabric - simplest (only Paramiko, pyasn1 and cyrptography as pypi dependencies; a strong bonus in such a restrictive environment) and most robust solution I could find for running on Python 2.6

Git Bash - for smaller scripts that launch Fabric.

• ./launcher - as the name says, the entry point for the tool, a shell script

• ./commands/ - shell scripts that are just parametrized launches of Fabric

• ./configurations/ - shell scripts that contain just environment variables split per environment and application

• ./bash/completion - bash completion script for use with the launcher

And the actual code, starting with the launcher:

./launcher

#!/bin/bash

set -o errexit   # set -e
set -o nounset  # set -u
set -o pipefail

script_usage="Script usage: ${0}   ."

if [[ -z "${1-}" ]]
then echo "Environment name parameter not provided. $script_usage"
    exit 1
fi
if [[ -z "${2-}" ]]
then echo "Application parameter not provided. $script_usage"
    exit 1
fi
if [[ -z "${3-}" ]]
then echo "Command parameter not provided. $script_usage"
    exit 1
fi

export FAB_ENVIRONMENT="${1}"

cd fabfile
python -c "from common import *; environment_prompt(\"${FAB_ENVIRONMENT}\")";
cd ..

source "configurations/${1}/${2}"

./commands/"${3}"

./fabfile/common.py

```
##- Imports.
from fabric.api import *

from getpass import getpass

import os
#-##

##- Environment, user, password.
def get_base_info():
try:
environment = os.environ['FAB_ENVIRONMENT']
except KeyError:
abort('FAB_ENVIRONMENT

./launcher

I'm no Bash person, so I have little to say about your launcher except where you call Python:

python -c "from common import *; environment_prompt(\"${FAB_ENVIRONMENT}\")";

You're inserting the FAB_ENVIRONMENT variable in what seems to me to be an unsafe manner. What if it has a quotation mark in it? Maybe FAB_ENVIRONMENT is some valid argument"); import os; os.remove("some important file. Your closing quotation mark and parenthesis finish the spelling of disaster. A much better way to do it is to let Python get the environment variable:

python -c "from os import getenv; import common; common.environment_prompt(os.getenv(\"FAB_ENVIRONMENT\")"

That's a rather long command, so maybe split it up a bit:

python - <<EOF
import common
from os import getenv
common.environment_prompt(getenv("FAB_ENVIRONMENT"))
EOF

You may notice that I removed your from common import *. I'll talk more about that below.

./fabfile/common.py

def check_empty_arguments(**arguments):
    ...
    abort(...)

I'm not sure it's a good idea for something named check_... to do something when it's wrong. Either make it clear in the name of the function that it takes action or define it so that the calling function decides what to do:

def check_empty_arguments(**arguments):
    return all(arguments.values())

(all() sees if every item given to it is "truthy". An empty string is not, so it will return True only if every string is non-empty.)

def fix_absolute_path(path):
    ...
    return path.replace(git_root, '')

I don't like that replacement. A more reliable method would be to use a built-in function:

if git_root:
    return os.path.relpath(path, git_root)
return path

if environment == ... or environment == ... or environment == ...:

Instead of chaining ors, just use in:

if environment in {'local', 'dev', 'test'}:

I'm also wondering about why you use bash -c ... instead of just .... Maybe you have a good reason, but it seems a little strange to me.

def environment_prompt(environment):
    line_length = 80
    marker = '#'
    line_padding = ' '
    ...

All those variables are really constants. Even though some of them depend on others, they never change once they're first defined. Python's naming convention uses ALL_CAPS for constants. I might also put them at the top of the file (as suggested in PEP 8).

Another advantage to putting them outside the function is that they aren't then re-defined every time the function is called.

./fabfile/package.py

server_folder = re.sub(r'[0-9]*', '', ...)

All you're doing here is removing the digits from the string. A regex is probably not the most efficient way to do that. A solution that I prefer comes from a Stack Overflow answer:

from string import digits

server_folder = target_server.split('.')[0].translate(None, digits)

if new_archive.endswith('.tar'):
elif new_archive.endswith('.tar.gz'):
elif new_archive.endswith('.tar.bz2'):
elif new_archive.endswith('.zip'):

That seems too repetitive to me. Perhaps a better way would be this:

archive_formats = (
    ('.tar', tar_list_command, 'tar -cvf'),
    ('.tar.gz', tar_list_command, 'tar -czvf'),
    ('.tar.bz2', tar_list_command, 'tar -cjvf'),
    ('.zip', zip_list_command, 'zip -r'),
)

for archive_format, archive_list_command, archive_command in archive_formats:
    if new_archive.endswith(archive_format):
        new_archive_basename = new_archive[:-len(archive_format)]
        break
else:
    format_string = ', '.join(format.strip('.') for format,_,_ in archive_formats)
    print('Unsupported archive format. Supported formats: {}'.format(format_string)
    sys.exit(1)

It's shorter and a lot easier to add more formats. You may be surprised that I'm using a tuple of tuples instead of a dictionary. This is because order may matter. For example, you might later handle bz2 even without tar. Therefore, you would want to make sure that the .tar.bz2 extension is checked before .bz2. When order matters, a dictionary becomes infeasible. I might then use collections.OrderedDict, but I'd need to create it from a bunch of tuples, anyway, so I just left it like that.

new_archive_list = []
for ...:
    new_archive_list.append(...)
new_archive_list = set(new_archive_list)

I agree that a set is probably better than a list, but why do you create a list first and then convert that to a set? It would be better to start out a set and stay like that:
new_archive_list = set()
for ...:
new_archive_list.add(...)

exclusions = set(exclusions.split(','))

The first use of exclusions in this function is changing it. The function is assuming that this argument is an unsuitable format, so why is that the format that is used? Why not let the calling functions give it a format it understands instead of making it do the conversion?

if len(to_archive_list):

You don

StackExchange Code Review Q#160105, answer score: 2