From ed2651ef43dfe6a0f8dd83a03f0e3406c1e14233 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Thu, 14 Sep 2017 14:43:23 -0400 Subject: [PATCH 01/10] Polished the setup.py --- requirements.txt | 3 +++ setup.py | 13 ++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) create mode 100644 requirements.txt diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..2b0614c --- /dev/null +++ b/requirements.txt @@ -0,0 +1,3 @@ +flask>=0.12.2 +gunicorn>=19.7.1 +GitPython>=2.1.5 \ No newline at end of file diff --git a/setup.py b/setup.py index 9f117f1..ebbe9ce 100644 --- a/setup.py +++ b/setup.py @@ -1,5 +1,5 @@ """ -Primarily jacked from Django's setup.py +Python setup file for the gitreload flask app. """ import os import sys @@ -16,28 +16,27 @@ for lib_path in lib_paths: existing_path = os.path.abspath(os.path.join(lib_path, "gitreload")) -EXCLUDE_FROM_PACKAGES = [] - version = __import__('gitreload').VERSION +install_requires = open('requirements.txt').read().splitlines() + setup( name='gitreload', version=version, url='https://github.com/mitodl/gitreload', - author='MITx', + author='MIT Office of Digital Learning', author_email='mitx-devops@mit.edu', description=('Github Web hook consumer for reloading ' 'courses in edx-platform, or generally ' 'updating local repositories'), license='AGPLv3', - packages=find_packages(exclude=EXCLUDE_FROM_PACKAGES), + packages=find_packages(), include_package_data=True, entry_points={'console_scripts': [ 'gitreload = gitreload.web:run_web', ]}, zip_safe=True, - requires=('flask', 'gunicorn', 'GitPython', ), - install_requires=['flask', 'gunicorn', 'GitPython==0.3.2.RC1', ], + install_requires=install_requires, data_files=[], classifiers=[ 'Development Status :: 3 - Alpha', From 83ad5fd3927e577e922d18463902ba09cbf81fdb Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 10:46:39 -0400 Subject: [PATCH 02/10] Fixing tests --- .travis.yml | 2 +- gitreload/config.py | 94 +++++++++--------------------- gitreload/processing.py | 12 ++-- gitreload/tests/test_config.py | 63 ++------------------ gitreload/tests/test_processing.py | 25 ++++---- gitreload/tests/test_web.py | 23 +++----- gitreload/web.py | 12 ++-- test_requirements.txt | 4 +- 8 files changed, 69 insertions(+), 166 deletions(-) diff --git a/.travis.yml b/.travis.yml index 67913e5..b6a95dd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,7 +2,7 @@ language: python python: - "2.7" install: - - pip install -r test_requirements.txt + - pip install -r requirements.txt -r test_requirements.txt script: bash test.sh -q after_success: coveralls diff --git a/gitreload/config.py b/gitreload/config.py index a2bda4c..f5954e2 100644 --- a/gitreload/config.py +++ b/gitreload/config.py @@ -1,7 +1,6 @@ """ Setup configuration from a json file with defaults """ -import json import logging from logging.handlers import SysLogHandler import os @@ -9,30 +8,35 @@ log = logging.getLogger('gitreload') # pylint: disable=C0103 -CONFIG_PATHS = [ - os.environ.get('GITRELOAD_CONFIG', ''), - os.path.join(os.getcwd(), 'gr.env.json'), - os.path.join(os.path.expanduser('~'), '.gr.env.json'), - '/etc/gr.env.json', -] - -def configure_logging(level_override=None): +class Config(object): + """ + Configuration for the app + """ + REPODIR = os.environ.get('REPODIR', '/mnt/data/repos') + VIRTUAL_ENV = os.environ.get('VIRTUAL_ENV', '/edx/app/edxapp/venvs/edxapp') + DJANGO_SETTINGS = os.environ.get('DJANGO_SETTINGS', 'aws') + EDX_PLATFORM = os.environ.get('EDX_PLATFORM', '/edx/app/edxapp/edx-platform') + LINKED_REPOS = os.environ.get('LINKED_REPOS', {}) + ALSO_CLONE_REPOS = os.environ.get('ALSO_CLONE_REPOS', {}) + NUM_THREADS = int(os.environ.get('NUM_THREADS', 1)) + LOG_LEVEL = os.environ.get('LOG_LEVEL', None) + HOSTNAME = platform.node().split('.')[0] + LOG_FORMATTER = ('%(asctime)s %(levelname)s %(process)d [%(name)s] ' + '%(filename)s:%(lineno)d - ' + '{hostname}- %(message)s').format(hostname=HOSTNAME) + LOG_FILE_PATH = os.environ.get('LOG_FILE_PATH', '') + + +def configure_logging(level_override=None, config=Config): """ Set the log level for the application """ - # Set up format for default logging - hostname = platform.node().split('.')[0] - formatter = ('%(asctime)s %(levelname)s %(process)d [%(name)s] ' - '%(filename)s:%(lineno)d - ' - '{hostname}- %(message)s').format(hostname=hostname) - set_level = level_override - # Grab config from settings if set, else allow system/language - # defaults. - config_log_level = settings.get('LOG_LEVEL', None) + # Grab config from settings if set, else allow system/language defaults. + config_log_level = config.LOG_LEVEL config_log_int = None if config_log_level and not set_level: @@ -47,12 +51,13 @@ def configure_logging(level_override=None): # Setup logging with format and level (do setup incase we are # main, or change root logger if we aren't. - logging.basicConfig(level=level_override, format=formatter) + logging.basicConfig(level=level_override, format=config.LOG_FORMATTER) root_logger = logging.getLogger() root_logger.setLevel(set_level) - address = None - if os.path.exists('/dev/log'): + if config.LOG_FILE_PATH: + address = config.LOG_FILE_PATH + elif os.path.exists('/dev/log'): address = '/dev/log' elif os.path.exists('/var/run/syslog'): address = '/var/run/syslog' @@ -64,51 +69,6 @@ def configure_logging(level_override=None): ) for handler in root_logger.handlers: - handler.setFormatter(logging.Formatter(formatter)) + handler.setFormatter(logging.Formatter(config.LOG_FORMATTER)) return config_log_int - - -def get_config(): - """ - Find and load the configuration file values - """ - conf = {} - config_file = None - config = {} - - for conf_path in CONFIG_PATHS: - if os.path.isfile(conf_path): - config_file = conf_path - break - if config_file: - with open(config_file) as env_file: - conf = json.load(env_file) - - config['REPODIR'] = conf.get( - 'REPODIR', - '/mnt/data/repos' - ) - config['VIRTUAL_ENV'] = conf.get( - 'VIRTUAL_ENV', - '/edx/app/edxapp/venvs/edxapp' - ) - config['DJANGO_SETTINGS'] = conf.get( - 'DJANGO_SETTINGS', - 'aws' - ) - config['EDX_PLATFORM'] = conf.get( - 'EDX_PLATFORM', - '/edx/app/edxapp/edx-platform' - ) - config['LOG_LEVEL'] = conf.get( - 'LOG_LEVEL', - None - ) - config['LINKED_REPOS'] = conf.get('LINKED_REPOS', {}) - config['ALSO_CLONE_REPOS'] = conf.get('ALSO_CLONE_REPOS', {}) - config['NUM_THREADS'] = int(conf.get('NUM_THREADS', 1)) - - return config - -settings = get_config() # pylint: disable=C0103 diff --git a/gitreload/processing.py b/gitreload/processing.py index 14e3da0..a3b56f8 100644 --- a/gitreload/processing.py +++ b/gitreload/processing.py @@ -10,7 +10,7 @@ from git import Repo -from gitreload.config import settings +from gitreload import config log = logging.getLogger('gitreload') # pylint: disable=C0103 @@ -22,14 +22,14 @@ def import_repo(action_call): """ os.environ['SERVICE_VARIANT'] = 'lms' cmd = [ - '{0}/bin/python'.format(settings['VIRTUAL_ENV']), + '{0}/bin/python'.format(config.Config.VIRTUAL_ENV), 'manage.py', 'lms', - '--settings={0}'.format(settings['DJANGO_SETTINGS']), + '--settings={0}'.format(config.Config.DJANGO_SETTINGS), 'git_add_course', action_call.repo_url, '--directory_path', - os.path.join(settings['REPODIR'], action_call.repo_name), + os.path.join(config.Config.REPODIR, action_call.repo_name), ] log.info('Beginning import of course repo %s with command %s', @@ -37,7 +37,7 @@ def import_repo(action_call): try: import_process = subprocess.check_output( cmd, - cwd=settings['EDX_PLATFORM'], + cwd=config.Config.EDX_PLATFORM, stderr=subprocess.STDOUT ) except subprocess.CalledProcessError as ex: @@ -54,7 +54,7 @@ def git_get_latest(action_call): and `git reset --hard origin/` on the passed in repo. """ - repo = Repo(os.path.join(settings['REPODIR'], action_call.repo_name)) + repo = Repo(os.path.join(config.Config.REPODIR, action_call.repo_name)) # Grab HEAD sha to see if we actually are updating orig_head = repo.head.commit.tree.hexsha repo.git.fetch('--all') diff --git a/gitreload/tests/test_config.py b/gitreload/tests/test_config.py index 59a25f5..6b810b5 100644 --- a/gitreload/tests/test_config.py +++ b/gitreload/tests/test_config.py @@ -5,63 +5,10 @@ import unittest import mock -from .base import TEST_ROOT - TEST_NUM_THREADS = 10 TEST_LOG_LEVEL = 'DEBUG' -@mock.patch('gitreload.config.CONFIG_PATHS') -class TestConfiguration(unittest.TestCase): - """ - Test out configuration defaults, loading json config, etc. - """ - # pylint: disable=R0904 - - DEFAULT_CONFIG_EXPECT = { - 'REPODIR': '/mnt/data/repos', - 'VIRTUAL_ENV': '/edx/app/edxapp/venvs/edxapp', - 'DJANGO_SETTINGS': 'aws', - 'EDX_PLATFORM': '/edx/app/edxapp/edx-platform', - 'LOG_LEVEL': None, - 'LINKED_REPOS': {}, - 'ALSO_CLONE_REPOS': {}, - 'NUM_THREADS': 1, - } - - def test_defaults(self, config_path_mock): - """ - Simply validate that config settings are what we expect above - """ - config_path_mock.__iter__.return_value = [] - from gitreload.config import get_config - self.assertEqual(get_config(), self.DEFAULT_CONFIG_EXPECT) - - def test_overrides(self, config_path_mock): - """ - Load our test config and make sure things are working right - """ - config_path_mock.__iter__.return_value = [ - os.path.join(TEST_ROOT, 'gr.env.json'), - ] - from gitreload.config import get_config - local_settings = get_config() - self.assertEqual(local_settings['NUM_THREADS'], TEST_NUM_THREADS) - self.assertEqual(local_settings['LOG_LEVEL'], TEST_LOG_LEVEL) - - def test_bad_json(self, config_path_mock): - """ - Load up a bad json file to make sure that we raise and exit - """ - config_path_mock.__iter__.return_value = [ - os.path.join(TEST_ROOT, 'gr.env.bad.json'), - ] - from gitreload.config import get_config - with self.assertRaisesRegexp(ValueError, - "No JSON object could be decoded"): - get_config() - - class TestLogConfiguration(unittest.TestCase): """ Make sure we are setting up logging like we expect. @@ -72,6 +19,7 @@ def test_log_override(self): """ Make sure we can setup logging with our own level """ + import pdb; pdb.set_trace() import logging root_logger = logging.getLogger() log_level = root_logger.level @@ -82,8 +30,7 @@ def test_log_override(self): root_logger = logging.getLogger() self.assertEqual(root_logger.level, logging.INFO) - @mock.patch.dict('gitreload.config.settings', - {'LOG_LEVEL': TEST_LOG_LEVEL}) + @mock.patch('gitreload.config.Config.LOG_LEVEL', TEST_LOG_LEVEL) def test_config_log_level(self,): """ Patch config and make sure we are setting to it @@ -98,8 +45,7 @@ def test_config_log_level(self,): root_logger = logging.getLogger() self.assertEqual(root_logger.level, getattr(logging, TEST_LOG_LEVEL)) - @mock.patch.dict('gitreload.config.settings', - {'LOG_LEVEL': 'Not a real thing'}) + @mock.patch('gitreload.config.Config.LOG_LEVEL', 'Not a real thing') def test_bad_log_level(self,): """ Set a non-existent log level and make sure we raise properly @@ -113,8 +59,7 @@ def test_bad_log_level(self,): with self.assertRaisesRegexp(ValueError, 'Invalid log level.+'): log_level = configure_logging() - @mock.patch.dict('gitreload.config.settings', - {'LOG_LEVEL': None}) + @mock.patch('gitreload.config.Config.LOG_LEVEL', None) def test_no_log_level(self): """ Make sure we leave things alone if no log level is set. diff --git a/gitreload/tests/test_processing.py b/gitreload/tests/test_processing.py index 4456966..842584a 100644 --- a/gitreload/tests/test_processing.py +++ b/gitreload/tests/test_processing.py @@ -8,7 +8,7 @@ from git import Repo -from .base import GitreloadTestBase, TEST_ROOT +from gitreload.tests.base import GitreloadTestBase, TEST_ROOT class TestProcessing(GitreloadTestBase): @@ -49,8 +49,7 @@ def test_import_failure(self, mocked_logging): from gitreload.processing import import_repo, ActionCall # Call with bad edx-platform path to prevent actual execution - with mock.patch.dict('gitreload.config.settings', - {'EDX_PLATFORM': '/dev/null'}): + with mock.patch('gitreload.config.Config.EDX_PLATFORM', '/dev/null'): import_repo(ActionCall( 'NOTREAL', 'NOTREAL', ActionCall.ACTION_TYPES['COURSE_IMPORT'] @@ -69,12 +68,20 @@ def test_command_error_and_input(self, mocked_logging): # pylint: disable=R0201 from gitreload.processing import import_repo, ActionCall - from .test_config import TestConfiguration # Setup default settings, have mock get called on import, # check parameters, and have side_effect raise the right Exception - with mock.patch.dict('gitreload.config.settings', - TestConfiguration.DEFAULT_CONFIG_EXPECT): + with mock.patch('gitreload.config.Config') as mock_config: + mock_config.configure_mock(**{ + 'REPODIR': '/mnt/data/repos', + 'VIRTUAL_ENV': '/edx/app/edxapp/venvs/edxapp', + 'DJANGO_SETTINGS': 'aws', + 'EDX_PLATFORM': '/edx/app/edxapp/edx-platform', + 'LOG_LEVEL': None, + 'LINKED_REPOS': {}, + 'ALSO_CLONE_REPOS': {}, + 'NUM_THREADS': 1, + }) with mock.patch('subprocess.check_output') as check_output: check_output.side_effect = subprocess.CalledProcessError( 10, 'test_command', output='Test output' @@ -243,8 +250,7 @@ def test_git_get_latest(self, mocked_log): ActionCall.ACTION_TYPES['GET_LATEST'] ) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': TEST_ROOT}): + with mock.patch('gitreload.config.Config.REPODIR', TEST_ROOT): git_get_latest(action_call) mocked_log.warn.assert_called_with( @@ -262,8 +268,7 @@ def test_git_get_latest(self, mocked_log): repo.remotes.origin.push() repo.head.reset(index=True, commit='HEAD~1', working_tree=True) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': TEST_ROOT}): + with mock.patch('gitreload.config.Config.REPODIR', TEST_ROOT): git_get_latest(action_call) mocked_log.info.assert_called_with( 'Updated to latest revision of repo %s. ' diff --git a/gitreload/tests/test_web.py b/gitreload/tests/test_web.py index c25c3d8..ef9d27b 100644 --- a/gitreload/tests/test_web.py +++ b/gitreload/tests/test_web.py @@ -150,7 +150,7 @@ def test_bad_push(self): headers={'X-Github-Event': 'push'}) self.assertEqual(response.status_code, 500) - @mock.patch.dict('gitreload.config.settings', {'REPODIR': '/dev/null'}) + @mock.patch('gitreload.config.Config.REPODIR', '/dev/null') def test_bad_repodir(self): """ Test that a bad repodir is handled right @@ -169,8 +169,7 @@ def test_missing_repo(self): Test to confirm that we don't want to clone repos that haven't already been checked out. """ - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_COURSE_URL, data={ @@ -198,8 +197,7 @@ def test_bad_repo_state(self): commit = repo.index.commit('test commit') repo.git.checkout(commit.hexsha) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_COURSE_URL, data={ @@ -220,8 +218,7 @@ def test_wrong_branch(self): repo_name = 'test' self._make_repo(repo_name) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_COURSE_URL, data={ @@ -244,8 +241,7 @@ def test_queue_put(self): self.assertEqual(len(gitreload.web.queued_jobs), 0) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_COURSE_URL, data={ @@ -275,8 +271,7 @@ def test_full_json_content_type(self): self.assertEqual(len(gitreload.web.queued_jobs), 0) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_COURSE_URL, data=self._make_payload(repo_name, 'master'), @@ -319,8 +314,7 @@ def test_update_repo(self): self.assertEqual(len(gitreload.web.queued_jobs), 0) - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_GET_LATEST_URL, data={ @@ -344,8 +338,7 @@ def test_update_verified(self): Validate with simple test that update is protected by same git hook validation code that course import is. """ - with mock.patch.dict('gitreload.config.settings', - {'REPODIR': self.tmpdir}): + with mock.patch('gitreload.config.Config.REPODIR', self.tmpdir): response = self.client.post( self.HOOK_GET_LATEST_URL, data={ diff --git a/gitreload/web.py b/gitreload/web.py index 5773d69..6b75d41 100644 --- a/gitreload/web.py +++ b/gitreload/web.py @@ -9,7 +9,7 @@ from flask import Flask, request, Response from git import Repo, InvalidGitRepositoryError, NoSuchPathError -from gitreload.config import settings, configure_logging +from gitreload.config import Config, configure_logging from gitreload.processing import GitAction, ActionCall @@ -78,14 +78,14 @@ def verify_hook(): # Check that repo is already checked out as that is our method for # validating this repo is good to pull. - if not os.path.isdir(settings['REPODIR']): - log.critical("Repo directory %s doesn't exist", settings['REPODIR']) + if not os.path.isdir(Config.REPODIR): + log.critical("Repo directory %s doesn't exist", Config.REPODIR) return Response(json_dump_msg('Server configuration issue'), 500), None # Get GitPython repo object from disk try: - repo = Repo(os.path.join(settings['REPODIR'], repo_name)) - except (InvalidGitRepositoryError, NoSuchPathError): + repo = Repo(os.path.join(Config.REPODIR, repo_name)) + except (InvalidGitRepositoryError, NoSuchPathError, ): log.critical('Repository %s (%s) not in list of available ' 'repositories', repo_name, owner) return Response(json_dump_msg('Repository not valid'), 500), None @@ -190,7 +190,7 @@ def get_queue_length(): # Application startup configuration configure_logging() -workers = start_workers(settings['NUM_THREADS']) # pylint: disable=C0103 +workers = start_workers(Config.NUM_THREADS) # pylint: disable=C0103 # Manual startup overrides (e.g. command line or direct run). diff --git a/test_requirements.txt b/test_requirements.txt index b4001be..cd2e2c4 100644 --- a/test_requirements.txt +++ b/test_requirements.txt @@ -3,7 +3,7 @@ coverage diff-cover pylint pep8 -flask mock -GitPython==0.3.2.RC1 coveralls +ipython +pytest From 748394ed5b63f34ca681f0bea433ac85b3fa8b76 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 11:12:52 -0400 Subject: [PATCH 03/10] Fixed unit tests --- gitreload/tests/test_config.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/gitreload/tests/test_config.py b/gitreload/tests/test_config.py index 6b810b5..f7fb9bc 100644 --- a/gitreload/tests/test_config.py +++ b/gitreload/tests/test_config.py @@ -1,6 +1,7 @@ """ Unit tests to validate configuration loading """ +import logging import os import unittest import mock @@ -15,15 +16,21 @@ class TestLogConfiguration(unittest.TestCase): """ # pylint: disable=R0904 + def setUp(self): + """ + Set up method + """ + # reset the log level before each test + logger = logging.getLogger() + logger.setLevel(logging.WARNING) + def test_log_override(self): """ Make sure we can setup logging with our own level """ - import pdb; pdb.set_trace() - import logging root_logger = logging.getLogger() log_level = root_logger.level - self.assertEqual(logging.NOTSET, log_level) + self.assertEqual(logging.WARNING, log_level) from gitreload.config import configure_logging log_level = configure_logging(logging.INFO) @@ -35,10 +42,9 @@ def test_config_log_level(self,): """ Patch config and make sure we are setting to it """ - import logging root_logger = logging.getLogger() log_level = root_logger.level - self.assertEqual(logging.NOTSET, log_level) + self.assertEqual(logging.WARNING, log_level) from gitreload.config import configure_logging log_level = configure_logging() @@ -50,10 +56,9 @@ def test_bad_log_level(self,): """ Set a non-existent log level and make sure we raise properly """ - import logging root_logger = logging.getLogger() log_level = root_logger.level - self.assertEqual(logging.NOTSET, log_level) + self.assertEqual(logging.WARNING, log_level) from gitreload.config import configure_logging with self.assertRaisesRegexp(ValueError, 'Invalid log level.+'): @@ -64,10 +69,9 @@ def test_no_log_level(self): """ Make sure we leave things alone if no log level is set. """ - import logging root_logger = logging.getLogger() log_level = root_logger.level - self.assertEqual(logging.NOTSET, log_level) + self.assertEqual(logging.WARNING, log_level) from gitreload.config import configure_logging log_level = configure_logging() @@ -77,7 +81,6 @@ def test_syslog_devices(self): """ Test syslog address handling and handler """ - import logging for log_device in ['/dev/log', '/var/run/syslog', '']: root_logger = logging.getLogger() From 061871a35de1f07f76feccde54552b7d968b9a2c Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 12:10:50 -0400 Subject: [PATCH 04/10] Tests are passing with pytest --- .pylintrc | 2 +- .travis.yml | 2 +- gitreload/config.py | 2 +- gitreload/tests/test_processing.py | 8 +++++--- pytest.ini | 4 ++++ requirements.txt | 2 +- setup.py | 5 ++++- test_requirements.txt | 13 ++++++++----- 8 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 pytest.ini diff --git a/.pylintrc b/.pylintrc index a271dc2..12e38db 100644 --- a/.pylintrc +++ b/.pylintrc @@ -179,7 +179,7 @@ ignore-imports=no [FORMAT] # Maximum number of characters on a single line. -max-line-length=80 +max-line-length=120 # Maximum number of lines in a module max-module-lines=1000 diff --git a/.travis.yml b/.travis.yml index b6a95dd..059a412 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,6 +3,6 @@ python: - "2.7" install: - pip install -r requirements.txt -r test_requirements.txt -script: bash test.sh -q +script: py.test after_success: coveralls diff --git a/gitreload/config.py b/gitreload/config.py index f5954e2..68bab28 100644 --- a/gitreload/config.py +++ b/gitreload/config.py @@ -62,7 +62,7 @@ def configure_logging(level_override=None, config=Config): elif os.path.exists('/var/run/syslog'): address = '/var/run/syslog' else: - address = ('127.0.0.1', 514) # pylint: disable=redefined-variable-type + address = ('127.0.0.1', 514) # pylint: disable=bad-option-value # Add syslog handler before adding formatters root_logger.addHandler( SysLogHandler(address=address, facility=SysLogHandler.LOG_LOCAL0) diff --git a/gitreload/tests/test_processing.py b/gitreload/tests/test_processing.py index 842584a..6db2422 100644 --- a/gitreload/tests/test_processing.py +++ b/gitreload/tests/test_processing.py @@ -72,7 +72,8 @@ def test_command_error_and_input(self, mocked_logging): # Setup default settings, have mock get called on import, # check parameters, and have side_effect raise the right Exception with mock.patch('gitreload.config.Config') as mock_config: - mock_config.configure_mock(**{ + mock_config.configure_mock( + **{ 'REPODIR': '/mnt/data/repos', 'VIRTUAL_ENV': '/edx/app/edxapp/venvs/edxapp', 'DJANGO_SETTINGS': 'aws', @@ -81,7 +82,8 @@ def test_command_error_and_input(self, mocked_logging): 'LINKED_REPOS': {}, 'ALSO_CLONE_REPOS': {}, 'NUM_THREADS': 1, - }) + } + ) with mock.patch('subprocess.check_output') as check_output: check_output.side_effect = subprocess.CalledProcessError( 10, 'test_command', output='Test output' @@ -188,7 +190,7 @@ def mock_effect(*args): # pragma: no cover due to multiprocessing workers = start_workers(1) # Wait for item to be processed - while len(queued_jobs) > 0: + while len(queued_jobs) > 0: # pylint: disable=len-as-condition pass # Assert that our side effect worked and was called diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..fe5c27d --- /dev/null +++ b/pytest.ini @@ -0,0 +1,4 @@ +[pytest] +addopts = --cov gitreload --pep8 --pylint --cov-report term --cov-report html -p no:warnings +norecursedirs = .git .tox .* CVS _darcs {arch} *.egg +pep8maxlinelength = 119 diff --git a/requirements.txt b/requirements.txt index 2b0614c..379ff92 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,3 @@ flask>=0.12.2 gunicorn>=19.7.1 -GitPython>=2.1.5 \ No newline at end of file +GitPython>=2.1.5 diff --git a/setup.py b/setup.py index ebbe9ce..0a826da 100644 --- a/setup.py +++ b/setup.py @@ -1,11 +1,14 @@ """ Python setup file for the gitreload flask app. """ +# pylint: disable=import-error, invalid-name + import os import sys +from distutils.sysconfig import get_python_lib from setuptools import setup, find_packages -from distutils.sysconfig import get_python_lib + if "install" in sys.argv: lib_paths = [get_python_lib()] diff --git a/test_requirements.txt b/test_requirements.txt index cd2e2c4..f3ecb9a 100644 --- a/test_requirements.txt +++ b/test_requirements.txt @@ -1,9 +1,12 @@ -nose coverage -diff-cover -pylint -pep8 -mock coveralls +diff-cover ipython +mock +nose +pep8 +pylint pytest +pytest-cov +pytest-pep8 +pytest-pylint==0.6.0 From 808dc656b3c2ce7f498fda399ca4ce6f08235730 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 12:11:09 -0400 Subject: [PATCH 05/10] Removed test.sh: we are better than shell scripts --- test.sh | 121 -------------------------------------------------------- 1 file changed, 121 deletions(-) delete mode 100755 test.sh diff --git a/test.sh b/test.sh deleted file mode 100755 index cbaf65a..0000000 --- a/test.sh +++ /dev/null @@ -1,121 +0,0 @@ -#!/bin/bash - -EXPECTED_ARGS=1 -E_BADARGS=65 -MAX_PYLINT_VIOLATIONS=0 -MAX_PEP8_VIOLATIONS=0 -PACKAGE_NAME=gitreload - -progname=$(basename $0) -usage() -{ - - cat </dev/null 2>&1) ; [ $? = 4 ] ; then # New longopts getopt. - OPTS=$(getopt -o $SHORTOPTS --long $LONGOPTS -n "$progname" -- "$@") -else # Old classic getopt. - # Special handling for --help on old getopt. - case $1 in --help) usage ; exit 0 ;; esac - OPTS=$(getopt $SHORTOPTS "$@") -fi - -if [ $? -ne 0 ]; then - echo "'$progname --help' for more information" 1>&2 - exit 1 -fi - -eval set -- "$OPTS" -quality=false -coveralls=false -diffcover=false -while [ $# -gt 0 ]; do - : debug: $1 - case $1 in - --help) - usage - exit 0 - ;; - -q|--with-quality) - quality=true - shift - ;; - -c|--with-coveralls) - coveralls=true - shift - ;; - -d|--diff-cover) - diffcover=true - shift - ;; - --) - shift - break - ;; - *) - echo "Internal Error: option processing error: $1" 1>&2 - exit 1 - ;; - esac -done - - -DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" - -echo "Using git version $(git --version)" - -nosetests --with-coverage --cover-html --cover-package=$PACKAGE_NAME,$PACKAGE_NAME.tests -test_results=$? - -if $quality; then - # Show nice reports - pylint --rcfile=$DIR/.pylintrc $PACKAGE_NAME - pep8 $PACKAGE_NAME - # Run again for automated violation testing - pylint_violations=$(pylint --rcfile=$DIR/.pylintrc $PACKAGE_NAME -r n | grep -v '\*\*\*\*\*\*\*\*\*\*' | wc -l) - pep8_violations=$(pep8 $PACKAGE_NAME | wc -l) -fi - -if $diffcover; then - coverage xml -i - diff-cover coverage.xml - rm coverage.xml -fi - -if $coveralls; then - echo "What is the coverall repo token?" - read token - echo "repo_token: $token" > $DIR/.coveralls.yml - coveralls - rm $DIR/.coveralls.yml -fi - -if [[ $test_results -ne 0 ]]; then - echo "Unit tests failed, failing test" - exit 1 -fi - -if [[ pylint_violations!="" && pylint_violations -gt MAX_PYLINT_VIOLATIONS ]]; then - echo "Too many PyLint Violations, failing test" - exit 1 -fi - -if [[ pep8_violations!="" && pep8_violations -gt MAX_PEP8_VIOLATIONS ]]; then - echo "Too many PEP-8 Violations, failing test" - exit 1 -fi From ddd25a0e46ab9064d85e5fbc9f6a87bb96ba339b Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 12:39:31 -0400 Subject: [PATCH 06/10] Small change --- gitreload/tests/test_processing.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gitreload/tests/test_processing.py b/gitreload/tests/test_processing.py index 6db2422..a29d53d 100644 --- a/gitreload/tests/test_processing.py +++ b/gitreload/tests/test_processing.py @@ -273,7 +273,6 @@ def test_git_get_latest(self, mocked_log): with mock.patch('gitreload.config.Config.REPODIR', TEST_ROOT): git_get_latest(action_call) mocked_log.info.assert_called_with( - 'Updated to latest revision of repo %s. ' - 'Original SHA: %s. Head SHA: %s', + 'Updated to latest revision of repo %s. Original SHA: %s. Head SHA: %s', repo_name, orig_head, repo.head.commit.tree.hexsha ) From 6444a8f11c6befae6eb3bb5dd4c2d7e68d2a584f Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Fri, 15 Sep 2017 13:51:42 -0400 Subject: [PATCH 07/10] Small change --- gitreload/processing.py | 2 +- gitreload/web.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/gitreload/processing.py b/gitreload/processing.py index a3b56f8..83c44a5 100644 --- a/gitreload/processing.py +++ b/gitreload/processing.py @@ -169,7 +169,7 @@ def run(self): # pragma: no cover due to multiprocessing action_call.action_type) self.ACTION_COMMANDS[action_call.action_type](action_call) except Exception: # pylint: disable=W0703 - log.exception('Failed to run command') + log.exception('Failed to run command GitAction') finally: self.queued_jobs.pop() self.queue.task_done() diff --git a/gitreload/web.py b/gitreload/web.py index 6b75d41..ade01ba 100644 --- a/gitreload/web.py +++ b/gitreload/web.py @@ -188,6 +188,23 @@ def get_queue_length(): return json.dumps(queue_object) +@app.route('/queue/pop_first', methods=['GET']) +def pop_first_in_queue(): + """ + Removes the first item from the queue and returns it + """ + # Format ActionCall to a dictionary for serializing + job_list = [] + for item in queued_jobs: + job_list.append({ + 'repo_name': item.repo_name, + 'repo_url': item.repo_url, + 'action': item.action_text + }) + queue_object = {'queue_length': len(queued_jobs), 'queue': job_list} + return json.dumps(queue_object) + + # Application startup configuration configure_logging() workers = start_workers(Config.NUM_THREADS) # pylint: disable=C0103 From 7a99d6de1b031b959d7aad9de7d0365a8866663a Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Mon, 18 Sep 2017 14:13:18 -0400 Subject: [PATCH 08/10] Added timeout to subprocess --- gitreload/config.py | 3 +++ gitreload/processing.py | 13 +++++++----- gitreload/tests/test_processing.py | 33 ++++++++++++++++++++++++------ requirements.txt | 1 + 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/gitreload/config.py b/gitreload/config.py index 68bab28..f789f7b 100644 --- a/gitreload/config.py +++ b/gitreload/config.py @@ -8,6 +8,8 @@ log = logging.getLogger('gitreload') # pylint: disable=C0103 +MINUTE = 60 # seconds + class Config(object): """ @@ -26,6 +28,7 @@ class Config(object): '%(filename)s:%(lineno)d - ' '{hostname}- %(message)s').format(hostname=HOSTNAME) LOG_FILE_PATH = os.environ.get('LOG_FILE_PATH', '') + SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT', 60 * MINUTE)) def configure_logging(level_override=None, config=Config): diff --git a/gitreload/processing.py b/gitreload/processing.py index 83c44a5..5a8ada7 100644 --- a/gitreload/processing.py +++ b/gitreload/processing.py @@ -6,7 +6,7 @@ import logging import multiprocessing import os -import subprocess +import subprocess32 from git import Repo @@ -35,13 +35,16 @@ def import_repo(action_call): log.info('Beginning import of course repo %s with command %s', action_call.repo_name, ' '.join(cmd)) try: - import_process = subprocess.check_output( + import_process = subprocess32.check_output( cmd, cwd=config.Config.EDX_PLATFORM, - stderr=subprocess.STDOUT + stderr=subprocess32.STDOUT, + timeout=config.Config.SUBPROCESS_TIMEOUT, ) - except subprocess.CalledProcessError as ex: - log.exception('Import command failed with: %s', ex.output) + except subprocess32.CalledProcessError as exc: + log.exception('Import command failed with: %s', exc.output) + except subprocess32.TimeoutExpired as exc: + log.exception('Import command timed out after %s seconds with: %s', exc.timeout, exc.output) except OSError as ex: log.exception('System or configuration error occurred: %s', str(ex)) else: diff --git a/gitreload/tests/test_processing.py b/gitreload/tests/test_processing.py index a29d53d..04ce612 100644 --- a/gitreload/tests/test_processing.py +++ b/gitreload/tests/test_processing.py @@ -3,7 +3,7 @@ """ import os import shutil -import subprocess +import subprocess32 import mock from git import Repo @@ -56,7 +56,7 @@ def test_import_failure(self, mocked_logging): )) mocked_logging.exception.assert_called_with( 'System or configuration error occurred: %s', - "[Errno 20] Not a directory: '/dev/null'" + "[Errno 20] Not a directory" ) @mock.patch('gitreload.processing.log') @@ -82,10 +82,11 @@ def test_command_error_and_input(self, mocked_logging): 'LINKED_REPOS': {}, 'ALSO_CLONE_REPOS': {}, 'NUM_THREADS': 1, + 'SUBPROCESS_TIMEOUT': 59, } ) - with mock.patch('subprocess.check_output') as check_output: - check_output.side_effect = subprocess.CalledProcessError( + with mock.patch('subprocess32.check_output') as check_output: + check_output.side_effect = subprocess32.CalledProcessError( 10, 'test_command', output='Test output' ) import_repo(ActionCall( @@ -102,7 +103,8 @@ def test_command_error_and_input(self, mocked_logging): '--directory_path', '/mnt/data/repos/NOTREAL'], cwd='/edx/app/edxapp/edx-platform', - stderr=-2 + stderr=-2, + timeout=59, ) mocked_logging.exception.assert_called_with( @@ -122,7 +124,7 @@ def test_command_success(self, mocked_logging): # Have mock get called on import and check parameters and have # return raise the right Exception - with mock.patch('subprocess.check_output') as check_output: + with mock.patch('subprocess32.check_output') as check_output: check_output.return_value = "Test Success" import_repo(ActionCall( 'NOTREAL', 'NOTREAL', @@ -134,6 +136,25 @@ def test_command_success(self, mocked_logging): 'Test Success' ) + @mock.patch('gitreload.processing.log') + def test_import_timeout(self, mocked_logging): + """ + Run an import that raises a timeout + """ + # pylint: disable=R0201 + + from gitreload.processing import import_repo, ActionCall + + # Call with bad edx-platform path to prevent actual execution + with mock.patch('subprocess32.check_output') as check_output: + check_output.side_effect = subprocess32.TimeoutExpired(cmd='ls', output='foooo', timeout=39) + import_repo(ActionCall( + 'NOTREAL', 'NOTREAL', + ActionCall.ACTION_TYPES['COURSE_IMPORT'] + )) + mocked_logging.exception.assert_called_with( + 'Import command timed out after %s seconds with: %s', 39, 'foooo') + def test_worker_count_and_stop(self): """ Make sure the number of workers started is properly configurable. diff --git a/requirements.txt b/requirements.txt index 379ff92..57d407f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ flask>=0.12.2 gunicorn>=19.7.1 GitPython>=2.1.5 +subprocess32 From d18ec7b3ff1c7d9ff78efdf39b0fa92399c70ea6 Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Wed, 20 Sep 2017 10:35:15 -0400 Subject: [PATCH 09/10] Minor changes --- gitreload/config.py | 2 +- gitreload/web.py | 17 ----------------- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/gitreload/config.py b/gitreload/config.py index f789f7b..830b553 100644 --- a/gitreload/config.py +++ b/gitreload/config.py @@ -28,7 +28,7 @@ class Config(object): '%(filename)s:%(lineno)d - ' '{hostname}- %(message)s').format(hostname=HOSTNAME) LOG_FILE_PATH = os.environ.get('LOG_FILE_PATH', '') - SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT', 60 * MINUTE)) + SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT', 60)) * MINUTE def configure_logging(level_override=None, config=Config): diff --git a/gitreload/web.py b/gitreload/web.py index ade01ba..6b75d41 100644 --- a/gitreload/web.py +++ b/gitreload/web.py @@ -188,23 +188,6 @@ def get_queue_length(): return json.dumps(queue_object) -@app.route('/queue/pop_first', methods=['GET']) -def pop_first_in_queue(): - """ - Removes the first item from the queue and returns it - """ - # Format ActionCall to a dictionary for serializing - job_list = [] - for item in queued_jobs: - job_list.append({ - 'repo_name': item.repo_name, - 'repo_url': item.repo_url, - 'action': item.action_text - }) - queue_object = {'queue_length': len(queued_jobs), 'queue': job_list} - return json.dumps(queue_object) - - # Application startup configuration configure_logging() workers = start_workers(Config.NUM_THREADS) # pylint: disable=C0103 From fb4ac0993f697da71f4710a00ac2f1101a3af83c Mon Sep 17 00:00:00 2001 From: Giovanni Di Milia Date: Tue, 24 Oct 2017 12:39:11 -0400 Subject: [PATCH 10/10] Changed name of env variable --- gitreload/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gitreload/config.py b/gitreload/config.py index 830b553..a87b37a 100644 --- a/gitreload/config.py +++ b/gitreload/config.py @@ -28,7 +28,7 @@ class Config(object): '%(filename)s:%(lineno)d - ' '{hostname}- %(message)s').format(hostname=HOSTNAME) LOG_FILE_PATH = os.environ.get('LOG_FILE_PATH', '') - SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT', 60)) * MINUTE + SUBPROCESS_TIMEOUT = int(os.environ.get('SUBPROCESS_TIMEOUT_MINUTES', 60)) * MINUTE def configure_logging(level_override=None, config=Config):