From 9e2264c8eb66b3e1695e28f46da8222a3ebceea5 Mon Sep 17 00:00:00 2001
From: Martin Curlej <mcurlej@redhat.com>
Date: Oct 16 2017 15:24:42 +0000
Subject: Removed blacklist from freshmaker, added error for regex failure


Signed-off-by: Martin Curlej <mcurlej@redhat.com>

Removed a blacklist test

---

diff --git a/conf/config.py b/conf/config.py
index d17181d..5220288 100644
--- a/conf/config.py
+++ b/conf/config.py
@@ -90,7 +90,7 @@ class BaseConfiguration(object):
 
     SSL_ENABLED = False
 
-    # whitelist and blacklist for handlers to decide whether an artifact
+    # whitelist for handlers to decide whether an artifact
     # can be built.
     #
     # In format of:
@@ -100,10 +100,7 @@ class BaseConfiguration(object):
     # }
     #
     # Here is an example of allowing MBSModuleStateChangeHandler to build
-    # any module that module name matches 'base-.*' but not:
-    #   1. module name matches 'base-test-module'
-    # or:
-    #   2. module from branch 'rawhide'
+    # any module that module name matches 'base-.*' or branch rawhide
     #
     # HANDLER_BUILD_WHITELIST = {
     #     "MBSModuleStateChangeHandler": {
@@ -111,15 +108,6 @@ class BaseConfiguration(object):
     #             {
     #                 'name': 'base-.*',
     #             },
-    #         ],
-    #     },
-    # }
-    # HANDLER_BUILD_BLACKLIST = {
-    #     "MBSModuleStateChangeHandler": {
-    #         "module": [
-    #             {
-    #                 'name': 'base-test-module',
-    #             },
     #             {
     #                 'branch': 'rawhide',
     #             },
diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py
index 3bcab33..5e08df6 100644
--- a/freshmaker/handlers/__init__.py
+++ b/freshmaker/handlers/__init__.py
@@ -32,6 +32,7 @@ from freshmaker.models import ArtifactBuildState
 from freshmaker.types import ArtifactType
 from freshmaker.models import ArtifactBuild, Event
 from freshmaker.utils import krb_context, get_rebuilt_nvr
+from freshmaker.errors import UnprocessableEntity
 
 from freshmaker.odcsclient import ODCS
 from freshmaker.odcsclient import AuthMech
@@ -108,7 +109,7 @@ class BaseHandler(object):
     def allow_build(self, artifact_type, **kwargs):
         """
         Check whether the artifact is allowed to be built by checking
-        HANDLER_BUILD_WHITELIST and HANDLER_BUILD_BLACKLIST in config.
+        HANDLER_BUILD_WHITELIST in config.
 
         :param artifact_type: an enum member of ArtifactType.
         :param kwargs: dictionary of arguments to check against
@@ -116,21 +117,16 @@ class BaseHandler(object):
         """
         # If there is a whitelist specified for the (handler, artifact_type),
         # the build target of (name, branch) need to be in that whitelist first.
-        # After that (if the build target is in whitelist), check the build target
-        # is not in the specified blacklist.
 
         # by default we assume the artifact is in whitelist and not in blacklist
         in_whitelist = True
-        in_blacklist = False
 
         # Global rules
         whitelist_rules = conf.handler_build_whitelist.get("global", {})
-        blacklist_rules = conf.handler_build_blacklist.get("global", {})
 
         # This handler rules
         handler_name = self.name
         whitelist_rules.update(conf.handler_build_whitelist.get(handler_name, {}))
-        blacklist_rules.update(conf.handler_build_blacklist.get(handler_name, {}))
 
         def match_rule(kwargs, rule):
             for key, value in kwargs.items():
@@ -152,20 +148,15 @@ class BaseHandler(object):
                           kwargs, artifact_type.name.lower())
                 in_whitelist = False
 
-            # only need to check blacklist when it is in whitelist first
-            if in_whitelist:
-                blacklist = blacklist_rules.get(artifact_type.name.lower(), [])
-                if blacklist and any([match_rule(kwargs, rule) for rule in blacklist]):
-                    log.debug('%r, type=%r is blacklisted.',
-                              kwargs, artifact_type.name.lower())
-                    in_blacklist = True
-
         except re.error as exc:
-            log.error("Error while compiling blacklist/whilelist rule for <handler(%s) artifact(%s)>:\n"
-                      "Incorrect regular expression: %s\nBlacklist and Whitelist will not take effect",
-                      handler_name, artifact_type.name.lower(), str(exc))
-            return True
-        return in_whitelist and not in_blacklist
+            err_msg = ("Error while compiling whilelist rule "
+                       "for <handler(%s) artifact(%s)>:\n"
+                       "Incorrect regular expression: %s\n"
+                       "Whitelist will not take effect" %
+                       (handler_name, artifact_type.name.lower(), str(exc)))
+            log.error(err_msg)
+            raise UnprocessableEntity(err_msg)
+        return in_whitelist
 
 
 class ContainerBuildHandler(BaseHandler):
diff --git a/tests/test_handler.py b/tests/test_handler.py
index 96bd083..05b4cf6 100644
--- a/tests/test_handler.py
+++ b/tests/test_handler.py
@@ -33,11 +33,15 @@ from freshmaker.handlers import ContainerBuildHandler
 from freshmaker.models import ArtifactBuild
 from freshmaker.models import ArtifactBuildState
 from freshmaker.models import Event
+from freshmaker.errors import UnprocessableEntity
+from freshmaker.types import ArtifactType
 
 
 class MyHandler(ContainerBuildHandler):
     """Handler for running tests to test things defined in parents"""
 
+    name = "MyHandler"
+
     def can_handle(self, event):
         """Implement BaseHandler method"""
 
@@ -214,3 +218,43 @@ class TestBuildFirstBatch(TestCase):
             else:
                 self.assertEqual(build.build_id, None)
                 self.assertEqual(build.state, ArtifactBuildState.PLANNED.value)
+
+    @patch('freshmaker.handlers.conf')
+    def test_allow_build_in_whitelist(self, conf):
+        """ Test if artifact is in the handlers whitelist """
+        whitelist_rules = {"image": [{'name': "test"}]}
+        handler = MyHandler()
+        conf.handler_build_whitelist.get.return_value = whitelist_rules
+        container = {"name": "test", "branch": "branch"}
+
+        allow = handler.allow_build(ArtifactType.IMAGE,
+                                    name=container["name"],
+                                    branch=container["branch"])
+        assert allow
+
+    @patch('freshmaker.handlers.conf')
+    def test_allow_build_not_in_whitelist(self, conf):
+        """ Test if artifact is not in the handlers whitelist """
+        whitelist_rules = {"image": [{'name': "test1"}]}
+        handler = MyHandler()
+        conf.handler_build_whitelist.get.return_value = whitelist_rules
+        container = {"name": "test", "branch": "branch"}
+
+        allow = handler.allow_build(ArtifactType.IMAGE,
+                                    name=container["name"],
+                                    branch=container["branch"])
+        assert not allow
+
+    @patch('freshmaker.handlers.conf')
+    def test_allow_build_regex_exception(self, conf):
+        """ If there is a regex error, method will raise UnprocessableEntity error """
+
+        whitelist_rules = {"image": [{'name': "te(st"}]}
+        handler = MyHandler()
+        conf.handler_build_whitelist.get.return_value = whitelist_rules
+        container = {"name": "test", "branch": "branch"}
+
+        with self.assertRaises(UnprocessableEntity):
+            handler.allow_build(ArtifactType.IMAGE,
+                                name=container["name"],
+                                branch=container["branch"])
diff --git a/tests/test_mbs_module_state_change_handler.py b/tests/test_mbs_module_state_change_handler.py
index dcf32f1..107dabb 100644
--- a/tests/test_mbs_module_state_change_handler.py
+++ b/tests/test_mbs_module_state_change_handler.py
@@ -144,39 +144,6 @@ class MBSModuleStateChangeHandlerTest(helpers.FreshmakerTestCase):
 
     @mock.patch('freshmaker.handlers.mbs.module_state_change.PDC')
     @mock.patch('freshmaker.handlers.mbs.module_state_change.utils')
-    @mock.patch('freshmaker.handlers.conf')
-    def test_module_is_not_allowed_in_blacklist(self, conf, utils, PDC):
-        conf.handler_build_whitelist = {}
-        conf.handler_build_blacklist = {
-            "MBSModuleStateChangeHandler": {
-                "module": [
-                    {
-                        'name': 'test.*',
-                    },
-                ],
-            },
-        }
-        msg = helpers.ModuleStateChangeMessage('testmodule', 'master', state='ready').produce()
-        event = self.get_event_from_msg(msg)
-
-        mod2_info = helpers.PDCModuleInfo('testmodule2', 'master', '20170412010101')
-        mod2_info.add_build_dep('testmodule', 'master')
-        mod2 = mod2_info.produce()
-
-        pdc = PDC.return_value
-        pdc.get_latest_modules.return_value = [mod2]
-
-        handler = MBSModuleStateChangeHandler()
-        handler.build_module = mock.Mock()
-        handler.record_build = mock.Mock()
-
-        self.assertTrue(handler.can_handle(event))
-        handler.handle(event)
-
-        handler.build_module.assert_not_called()
-
-    @mock.patch('freshmaker.handlers.mbs.module_state_change.PDC')
-    @mock.patch('freshmaker.handlers.mbs.module_state_change.utils')
     @mock.patch('freshmaker.handlers.mbs.module_state_change.log')
     def test_handler_not_fall_into_cyclic_rebuild_loop(self, log, utils, PDC):
         """

