Python mirror tool refactor and test (#2851)

fixes #2849

Author: Vladimir Kotal

dev/before_install

@@ -37,7 +37,7 @@ if [[ "$TRAVIS_OS_NAME" == "linux" ]]; then
 elif [[ "$TRAVIS_OS_NAME" == "osx" ]]; then
 	brew update
 
-	brew install cvs
+	brew install cvs libgit2
 	if [[ $? != 0 ]]; then
 		echo "cannot install extra packages"
 		exit 1

opengrok-tools/setup.py

@@ -55,6 +55,7 @@ def my_test_suite():
     ],
     tests_require=[
         'pytest',
+        'pygit2',
     ],
     entry_points={
         'console_scripts': [

opengrok-tools/src/main/python/opengrok_tools/mirror.py

@@ -106,35 +106,36 @@ def main():
     try:
         args = parser.parse_args()
     except ValueError as e:
-        fatal(e)
+        return fatal(e, False)
 
     logger = get_console_logger(get_class_basename(), args.loglevel)
 
     if len(args.project) > 0 and args.all:
-        fatal("Cannot use both project list and -a/--all")
+        return fatal("Cannot use both project list and -a/--all", False)
 
     if not args.all and len(args.project) == 0:
-        fatal("Need at least one project or --all")
+        return fatal("Need at least one project or --all", False)
 
     if args.config:
         config = read_config(logger, args.config)
         if config is None:
-            fatal("Cannot read config file from {}".format(args.config))
+            return fatal("Cannot read config file from {}".
+                         format(args.config), False)
     else:
         config = {}
 
     uri = args.uri
     if not is_web_uri(uri):
-        fatal("Not a URI: {}".format(uri))
+        return fatal("Not a URI: {}".format(uri), False)
     logger.debug("web application URI = {}".format(uri))
 
     if not check_configuration(config):
-        sys.exit(1)
+        return 1
 
     # Save the source root to avoid querying the web application.
     source_root = get_config_value(logger, 'sourceRoot', uri)
     if not source_root:
-        sys.exit(1)
+        return 1
 
     logger.debug("Source root = {}".format(source_root))
 
@@ -157,8 +158,8 @@ def main():
     if args.batch:
         logdir = config.get(LOGDIR_PROPERTY)
         if not logdir:
-            fatal("The {} property is required in batch mode".
-                  format(LOGDIR_PROPERTY))
+            return fatal("The {} property is required in batch mode".
+                         format(LOGDIR_PROPERTY), False)
 
     projects = args.project
     if len(projects) == 1:
@@ -183,19 +184,19 @@ def main():
                 try:
                     project_results = pool.map(worker, worker_args, 1)
                 except KeyboardInterrupt:
-                    sys.exit(FAILURE_EXITVAL)
+                    return FAILURE_EXITVAL
                 else:
                     if any([x == FAILURE_EXITVAL for x in project_results]):
                         ret = FAILURE_EXITVAL
                     if all([x == CONTINUE_EXITVAL for x in project_results]):
                         ret = CONTINUE_EXITVAL
     except Timeout:
         logger.warning("Already running, exiting.")
-        sys.exit(FAILURE_EXITVAL)
+        return FAILURE_EXITVAL
 
     logging.shutdown()
-    sys.exit(ret)
+    return ret
 
 
 if __name__ == '__main__':
-    main()
+    sys.exit(main())

opengrok-tools/src/main/python/opengrok_tools/utils/log.py

@@ -31,13 +31,18 @@
 )
 
 
-def fatal(msg):
+def fatal(msg, exit=True):
     """
     Print message to standard error output and exit
+    unless the exit parameter is False
     :param msg: message
+    :param exit
     """
     print(msg, file=sys.stderr)
-    sys.exit(FAILURE_EXITVAL)
+    if exit:
+        sys.exit(FAILURE_EXITVAL)
+    else:
+        return FAILURE_EXITVAL
 
 
 def add_log_level_argument(parser):

opengrok-tools/src/main/python/opengrok_tools/utils/mirror.py

@@ -200,6 +200,83 @@ def get_project_properties(project_config, project_name, hookdir):
         use_proxy, ignored_repos
 
 
+def process_hook(hook_ident, hook, source_root, project_name, proxy,
+                 hook_timeout):
+    """
+    :param hook_ident: ident of the hook to be used in log entries
+    :param hook: hook
+    :param source_root: source root path
+    :param project_name: project name
+    :param proxy: proxy or None
+    :param hook_timeout: hook run timeout
+    :return: False if hook failed, else True
+    """
+    if hook:
+        logger = logging.getLogger(__name__)
+
+        logger.info("Running {} hook".format(hook_ident))
+        if run_hook(logger, hook,
+                    os.path.join(source_root, project_name), proxy,
+                    hook_timeout) != SUCCESS_EXITVAL:
+            logger.error("{} hook failed for project {}".
+                         format(hook_ident, project_name))
+            return False
+
+    return True
+
+
+def process_changes(repos, project_name, uri):
+    """
+    :param repos: repository list
+    :param project_name: project name
+    :return: exit code
+    """
+    logger = logging.getLogger(__name__)
+
+    changes_detected = False
+
+    # check if the project is a new project - full index is necessary
+    try:
+        r = get(logger, get_uri(uri, 'api', 'v1', 'projects',
+                                urllib.parse.quote_plus(project_name),
+                                'property', 'indexed'))
+        r.raise_for_status()
+        if not bool(r.json()):
+            changes_detected = True
+            logger.info('Project {} has not been indexed yet'
+                        .format(project_name))
+    except ValueError as e:
+        logger.error('Unable to parse project \'{}\' indexed flag: {}'
+                     .format(project_name, e))
+        return FAILURE_EXITVAL
+    except HTTPError as e:
+        logger.error('Unable to determine project \'{}\' indexed flag: {}'
+                     .format(project_name, e))
+        return FAILURE_EXITVAL
+
+    # check if the project has any new changes in the SCM
+    if not changes_detected:
+        for repo in repos:
+            try:
+                if repo.incoming():
+                    logger.debug('Repository {} has incoming changes'.
+                                 format(repo))
+                    changes_detected = True
+                    break
+            except RepositoryException:
+                logger.error('Cannot determine incoming changes for '
+                             'repository {}'.format(repo))
+                return FAILURE_EXITVAL
+
+    if not changes_detected:
+        logger.info('No incoming changes for repositories in '
+                    'project {}'.
+                    format(project_name))
+        return CONTINUE_EXITVAL
+
+    return SUCCESS_EXITVAL
+
+
 def mirror_project(config, project_name, check_changes, uri,
                    source_root):
     """
@@ -213,6 +290,8 @@ def mirror_project(config, project_name, check_changes, uri,
     :return exit code
     """
 
+    ret = SUCCESS_EXITVAL
+
     logger = logging.getLogger(__name__)
 
     project_config = get_project_config(config, project_name)
@@ -235,7 +314,6 @@ def mirror_project(config, project_name, check_changes, uri,
     # Cache the repositories first. This way it will be known that
     # something is not right, avoiding any needless pre-hook run.
     #
-    ret = SUCCESS_EXITVAL
     repos = get_repos_for_project(project_name,
                                   ignored_repos,
                                   uri,
@@ -251,59 +329,17 @@ def mirror_project(config, project_name, check_changes, uri,
 
     # Check if the project or any of its repositories have changed.
     if check_changes:
-        changes_detected = False
+        r = process_changes(repos, project_name, uri)
+        if r != SUCCESS_EXITVAL:
+            return r
 
-        # check if the project is a new project - full index is necessary
-        try:
-            r = get(logger, get_uri(uri, 'api', 'v1', 'projects',
-                                    urllib.parse.quote_plus(project_name),
-                                    'property', 'indexed'))
-            r.raise_for_status()
-            if not bool(r.json()):
-                changes_detected = True
-                logger.debug('Project {} has not been indexed yet'
-                             .format(project_name))
-        except ValueError as e:
-            logger.error('Unable to parse project \'{}\' indexed flag: {}'
-                         .format(project_name, e))
-            return FAILURE_EXITVAL
-        except HTTPError as e:
-            logger.error('Unable to determine project \'{}\' indexed flag: {}'
-                         .format(project_name, e))
-            return FAILURE_EXITVAL
-
-        # check if the project has any new changes in the SCM
-        if not changes_detected:
-            for repo in repos:
-                try:
-                    if repo.incoming():
-                        logger.debug('Repository {} has incoming changes'.
-                                     format(repo))
-                        changes_detected = True
-                        break
-                except RepositoryException:
-                    logger.error('Cannot determine incoming changes for '
-                                 'repository {}'.format(repo))
-                    return FAILURE_EXITVAL
-
-        if not changes_detected:
-            logger.info('No incoming changes for repositories in '
-                        'project {}'.
-                        format(project_name))
-            return CONTINUE_EXITVAL
-
-    if prehook:
-        logger.info("Running pre hook")
-        if run_hook(logger, prehook,
-                    os.path.join(source_root, project_name), proxy,
-                    hook_timeout) != SUCCESS_EXITVAL:
-            logger.error("pre hook failed for project {}".
-                         format(project_name))
-            return FAILURE_EXITVAL
+    if not process_hook("pre", prehook, source_root, project_name, proxy,
+                        hook_timeout):
+        return FAILURE_EXITVAL
 
     #
     # If one of the repositories fails to sync, the whole project sync
-    # is treated as failed, i.e. the program will return 1.
+    # is treated as failed, i.e. the program will return FAILURE_EXITVAL.
     #
     for repo in repos:
         logger.info("Synchronizing repository {}".
@@ -313,14 +349,9 @@ def mirror_project(config, project_name, check_changes, uri,
                          format(repo.path))
             ret = FAILURE_EXITVAL
 
-    if posthook:
-        logger.info("Running post hook")
-        if run_hook(logger, posthook,
-                    os.path.join(source_root, project_name), proxy,
-                    hook_timeout) != SUCCESS_EXITVAL:
-            logger.error("post hook failed for project {}".
-                         format(project_name))
-            return FAILURE_EXITVAL
+    if not process_hook("post", posthook, source_root, project_name, proxy,
+                        hook_timeout):
+        return FAILURE_EXITVAL
 
     return ret
 

opengrok-tools/src/test/python/test_mirror.py

@@ -27,12 +27,19 @@
 import tempfile
 import os
 import stat
+import pygit2
 import pytest
+import sys
 
+from opengrok_tools.scm.repofactory import get_repository
 from opengrok_tools.utils.mirror import check_project_configuration, \
     check_configuration, \
     HOOKS_PROPERTY, PROXY_PROPERTY, IGNORED_REPOS_PROPERTY, \
     PROJECTS_PROPERTY
+import opengrok_tools.mirror
+from opengrok_tools.utils.exitvals import (
+    CONTINUE_EXITVAL,
+)
 
 
 def test_empty_project_configuration():
@@ -109,3 +116,56 @@ def test_valid_config():
     assert check_configuration({PROJECTS_PROPERTY:
                                 {"foo": {PROXY_PROPERTY: True}},
                                 PROXY_PROPERTY: "proxy"})
+
+
+@pytest.mark.skipif(not os.name.startswith("posix"), reason="requires posix")
+def test_incoming_retval(monkeypatch):
+    """
+    Test that the special CONTINUE_EXITVAL value bubbles all the way up to
+    the mirror.py return value.
+    """
+
+    class MockResponse:
+
+        # mock json() method always returns a specific testing dictionary
+        @staticmethod
+        def json():
+            return "true"
+
+        @staticmethod
+        def raise_for_status():
+            pass
+
+    with tempfile.TemporaryDirectory() as source_root:
+        repo_name = "parent_repo"
+        repo_path = os.path.join(source_root, repo_name)
+        cloned_repo_name = "cloned_repo"
+        cloned_repo_path = os.path.join(source_root, cloned_repo_name)
+        project_name = "foo"    # does not matter for this test
+
+        os.mkdir(repo_path)
+
+        def mock_get_repos(*args, **kwargs):
+            return [get_repository(cloned_repo_path,
+                                   "git", project_name,
+                                   None, None, None, None)]
+
+        def mock_get(*args, **kwargs):
+            return MockResponse()
+
+        # Clone a Git repository so that it can pull.
+        pygit2.init_repository(repo_path, True)
+        pygit2.clone_repository(repo_path, cloned_repo_path)
+
+        with monkeypatch.context() as m:
+            m.setattr(sys, 'argv', ['prog', "-I", project_name])
+
+            # With mocking done via pytest it is necessary to patch
+            # the call-site rather than the absolute object path.
+            m.setattr("opengrok_tools.mirror.get_config_value",
+                      lambda x, y, z: source_root)
+            m.setattr("opengrok_tools.utils.mirror.get_repos_for_project",
+                      mock_get_repos)
+            m.setattr("opengrok_tools.utils.mirror.get", mock_get)
+
+            assert opengrok_tools.mirror.main() == CONTINUE_EXITVAL