Merge pull request #913 from untergeek/fix/761

Raise exception instead of returning bool
elastic · Mar 27, 2017 · 827a5dd · 827a5dd
2 parents 323970c + 2bc7496
commit 827a5dd
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 23 deletions.
diff --git a/curator/utils.py b/curator/utils.py
@@ -701,9 +701,13 @@ def get_repository(client, repository=''):
     """
     try:
         return client.snapshot.get_repository(repository=repository)
-    except (elasticsearch.TransportError, elasticsearch.NotFoundError):
-        logger.error("Repository {0} not found.".format(repository))
-        return False
+    except (elasticsearch.TransportError, elasticsearch.NotFoundError) as e:
+        raise CuratorException(
+            'Unable to get repository {0}.  Response Code: {1}.  Error: {2}.'
+            'Check Elasticsearch logs for more information.'.format(
+                repository, e.status_code, e.error
+            )
+        )        
 
 def get_snapshot(client, repository=None, snapshot=''):
     """
@@ -944,7 +948,7 @@ def create_repository(client, **kwargs):
         logger.debug(
             'Checking if repository {0} already exists...'.format(repository)
         )
-        result = get_repository(client, repository=repository)
+        result = repository_exists(client, repository=repository)
         logger.debug("Result = {0}".format(result))
         if not result:
             logger.debug(
@@ -953,13 +957,6 @@ def create_repository(client, **kwargs):
                 )
             )
             client.snapshot.create_repository(repository=repository, body=body)
-        elif result is not None and repository not in result:
-            logger.debug(
-                'Repository {0} not in Elasticsearch. Continuing...'.format(
-                    repository
-                )
-            )
-            client.snapshot.create_repository(repository=repository, body=body)
         else:
             raise FailedExecution(
                 'Unable to create repository {0}.  A repository with that name '
@@ -987,12 +984,19 @@ def repository_exists(client, repository=None):
     """
     if not repository:
         raise MissingArgument('No value for "repository" provided')
-    test_result = get_repository(client, repository)
-    if repository in test_result:
-        logger.debug("Repository {0} exists.".format(repository))
-        return True
-    else:
-        logger.debug("Repository {0} not found...".format(repository))
+    try:
+        test_result = get_repository(client, repository)
+        if repository in test_result:
+            logger.debug("Repository {0} exists.".format(repository))
+            return True
+        else:
+            logger.debug("Repository {0} not found...".format(repository))
+            return False
+    except Exception as e:
+        logger.debug(
+            'Unable to find repository "{0}": Error: '
+            '{1}'.format(repository, e)
+        )
         return False
 
 def test_repo_fs(client, repository=None):

diff --git a/docs/Changelog.rst b/docs/Changelog.rst
@@ -34,6 +34,8 @@ production! There `will` be many more changes before 5.0.0 is released.
 
   * Curator now logs version incompatibilities as an error, rather than just
     raising an Exception. #874 (untergeek)
+  * The ``get_repository()`` function now properly raises an exception instead
+    of returning `False` if nothing is found. #761 (untergeek)
 
 
 5.0.0a1 (23 March 2017)

diff --git a/test/integration/test_es_repo_mgr.py b/test/integration/test_es_repo_mgr.py
@@ -112,7 +112,7 @@ def test_delete_repository_success(self):
                     ]
         )
         self.assertFalse(
-            curator.get_repository(self.client, self.args['repository'])
+            curator.repository_exists(self.client, self.args['repository'])
         )
     def test_delete_repository_notfound(self):
         self.write_config(

diff --git a/test/unit/test_utils.py b/test/unit/test_utils.py
@@ -335,15 +335,22 @@ def test_get_repository_missing_arg(self):
     def test_get_repository_positive(self):
         client = Mock()
         client.snapshot.get_repository.return_value = testvars.test_repo
-        self.assertEqual(testvars.test_repo, curator.get_repository(client, repository=testvars.repo_name))
+        self.assertEqual(testvars.test_repo, 
+            curator.get_repository(client, repository=testvars.repo_name))
     def test_get_repository_transporterror_negative(self):
         client = Mock()
-        client.snapshot.get_repository.side_effect = elasticsearch.TransportError
-        self.assertFalse(curator.get_repository(client, repository=testvars.repo_name))
+        client.snapshot.get_repository.side_effect = elasticsearch.TransportError(503,'foo','bar')
+        self.assertRaises(
+            curator.CuratorException,
+            curator.get_repository, client, repository=testvars.repo_name
+        )
     def test_get_repository_notfounderror_negative(self):
         client = Mock()
-        client.snapshot.get_repository.side_effect = elasticsearch.NotFoundError
-        self.assertFalse(curator.get_repository(client, repository=testvars.repo_name))
+        client.snapshot.get_repository.side_effect = elasticsearch.NotFoundError(404,'foo','bar')
+        self.assertRaises(
+            curator.CuratorException,
+            curator.get_repository, client, repository=testvars.repo_name
+        )
     def test_get_repository__all_positive(self):
         client = Mock()
         client.snapshot.get_repository.return_value = testvars.test_repos