#147 Two fixes to whitelist
Merged 6 years ago by jkaluza. Opened 6 years ago by cqi.
cqi/freshmaker fix-whitelist  into  master

file modified
+4 -12
@@ -41,11 +41,8 @@ 

  

      # automagically detect production environment:

      #   - existing and readable config_file presets ProdConfiguration

-     try:

-         with open(config_file):

-             config_section = 'ProdConfiguration'

-     except:

-         pass

+     if os.path.exists(config_file) and os.access(config_file, os.O_RDONLY):

+         config_section = 'ProdConfiguration'

  

      # try getting config_file from os.environ

      if 'FRESHMAKER_CONFIG_FILE' in os.environ:
@@ -81,7 +78,7 @@ 

          try:

              config_module = imp.load_source('freshmaker_runtime_config',

                                              config_file)

-         except:

+         except IOError:

              raise SystemError("Configuration file {} was not found."

                                .format(config_file))

  
@@ -204,11 +201,6 @@ 

              'default': {},

              'desc': 'Whitelist for build targets of handlers',

          },

-         'handler_build_blacklist': {

-             'type': dict,

-             'default': {},

-             'desc': 'Blacklist for build targets of handlers',

-         },

          'lightblue_server_url': {

              'type': str,

              'default': '',
@@ -334,7 +326,7 @@ 

                      # Do no try to convert None...

                      if value is not None:

                          value = convert(value)

-                 except:

+                 except (TypeError, ValueError):

                      raise TypeError("Configuration value conversion failed for name: %s" % key)

              # unknown type/unsupported conversion

              elif convert is not None:

file modified
+25 -21
@@ -24,6 +24,7 @@ 

  import abc

  import json

  import re

+ import itertools

  from functools import wraps

  

  from freshmaker import conf, log, db, models
@@ -208,21 +209,20 @@ 

          db.session.commit()

          return build

  

-     def allow_build(self, artifact_type, **kwargs):

+     def allow_build(self, artifact_type, **criteria):

          """

          Check whether the artifact is allowed to be built by checking

          HANDLER_BUILD_WHITELIST in config.

  

          :param artifact_type: an enum member of ArtifactType.

-         :param kwargs: dictionary of arguments to check against

-         :return: True or False.

+         :param criteria: keyword arguments listing criteria that will be

+             checked against whitelist to determine whether build is allowed.

+             There is not specific order or logical relationship to these

+             criteria. How they are checked depends on how whitelist is

+             configured.

+         :return: True if build is allowed, otherwise False is returned.

+         :rtype: bool

          """

-         # If there is a whitelist specified for the (handler, artifact_type),

-         # the build target of (name, branch) need to be in that whitelist first.

- 

-         # by default we assume the artifact is in whitelist and not in blacklist

-         in_whitelist = True

- 

          # Global rules

          whitelist_rules = conf.handler_build_whitelist.get("global", {})

  
@@ -230,26 +230,29 @@ 

          handler_name = self.name

          whitelist_rules.update(conf.handler_build_whitelist.get(handler_name, {}))

  

-         def match_rule(kwargs, rule):

-             for key, value in kwargs.items():

-                 value_rule = rule.get(key, None)

-                 if not value_rule:

+         def match_rule(criteria, rule):

+             for name, value in criteria.items():

+                 value_patterns = rule.get(name, None)

+                 if not value_patterns:

                      continue

  

-                 if not isinstance(value_rule, list):

-                     value_rule = [value_rule]

+                 if not isinstance(value_patterns, (tuple, list)):

+                     value_patterns = [value_patterns]

  

-                 if not any((re.compile(r).match(value) for r in value_rule)):

+                 if not any((re.match(regex, value) for regex in value_patterns)):

                      return False

              return True

  

          try:

              whitelist = whitelist_rules.get(artifact_type.name.lower(), [])

-             if whitelist and not any([match_rule(kwargs, rule) for rule in whitelist]):

+             # If none of passed criteria matches configured rule, build is not allowed

+             if not (set(itertools.chain(*[rule.keys() for rule in whitelist])) &

+                     set(criteria.keys())):

+                 return False

+             if whitelist and any([match_rule(criteria, rule) for rule in whitelist]):

                  log.debug('%r, type=%r is not whitelisted.',

-                           kwargs, artifact_type.name.lower())

-                 in_whitelist = False

- 

+                           criteria, artifact_type.name.lower())

+                 return True

          except re.error as exc:

              err_msg = ("Error while compiling whilelist rule "

                         "for <handler(%s) artifact(%s)>:\n"
@@ -258,7 +261,8 @@ 

                         (handler_name, artifact_type.name.lower(), str(exc)))

              log.error(err_msg)

              raise UnprocessableEntity(err_msg)

-         return in_whitelist

+ 

+         return False

  

  

  class ContainerBuildHandler(BaseHandler):

@@ -51,7 +51,7 @@ 

  

          for container in containers:

              if not self.allow_build(ArtifactType.IMAGE, name=container['name'], branch=container['branch']):

-                 log.info("Skip rebuild of image %s:%s as it's not allowed by configured whitelist/blacklist",

+                 log.info("Skip rebuild of image %s:%s as it's not allowed by configured whitelist",

                           container['name'], container['branch'])

                  continue

              try:
@@ -69,7 +69,7 @@ 

                  task_id = self.build_container(scm_url, branch, build_target)

                  if task_id is not None:

                      self.record_build(event, container['name'], ArtifactType.IMAGE, task_id)

-             except:

+             except Exception:

                  log.exception('Error when rebuild %s', container)

  

          return []

@@ -40,7 +40,7 @@ 

          log.info('Start to rebuild docker image %s.', event.container)

  

          if not self.allow_build(ArtifactType.IMAGE, name=event.container, branch=event.branch):

-             log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist",

+             log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist",

                       event.container, event.branch)

              return []

  
@@ -60,7 +60,7 @@ 

  

          except koji.krbV.Krb5Error as e:

              log.exception('Failed to login Koji via Kerberos using GSSAPI. %s', e.args[1])

-         except:

+         except Exception:

              log.exception('Could not create task to build docker image %s', event.container)

  

          return []

@@ -41,7 +41,7 @@ 

          log.info("Triggering rebuild of module %s:%s, metadata updated (%s).",

                   event.module, event.branch, event.rev)

          if not self.allow_build(ArtifactType.MODULE, name=event.module, branch=event.branch):

-             log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist",

+             log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist",

                       event.module, event.branch)

              return []

  

@@ -55,7 +55,7 @@ 

              name = module['variant_name']

              version = module['variant_version']

              if not self.allow_build(ArtifactType.MODULE, name=name, branch=version):

-                 log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist",

+                 log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist",

                           name, version)

                  continue

              log.info("Going to rebuild module '%s:%s'.", name, version)

@@ -92,7 +92,7 @@ 

                  name = mod['variant_name']

                  version = mod['variant_version']

                  if not self.allow_build(ArtifactType.MODULE, name=name, branch=version):

-                     log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist",

+                     log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist",

                               name, version)

                      continue

                  # bump module repo first

file modified
+1 -1
@@ -82,7 +82,7 @@ 

          logging.basicConfig(level=conf.log_level, format=log_format)

          try:

              from systemd import journal

-         except:

+         except ImportError:

              raise ValueError("systemd.journal module is not installed")

  

          log = logging.getLogger()

file modified
+1 -1
@@ -125,7 +125,7 @@ 

      msg_cert_subject.C = 'US'

      msg_cert_subject.ST = 'MA'

      msg_cert_subject.L = 'Boston'

-     msg_cert_subject.O = 'Development'

+     msg_cert_subject.O = 'Development'  # noqa

      msg_cert_subject.CN = 'localhost'

      cert.set_serial_number(2)

      cert.gmtime_adj_notBefore(0)

file modified
+1 -1
@@ -83,7 +83,7 @@ 

      def _decorator(*args, **kwargs):

          try:

              return func(*args, **kwargs)

-         except:

+         except Exception:

              db.session.rollback()

              raise

          finally:

file modified
+1 -1
@@ -60,7 +60,7 @@ 

      for submod_name in dir(mod):

          try:

              submod = getattr(mod, submod_name)

-         except:

+         except AttributeError:

              continue

          key = None

          deps = []

@@ -28,6 +28,8 @@ 

  from tests import helpers

  from tests import get_fedmsg

  

+ import freshmaker

+ 

  from freshmaker import events, db, models

  from freshmaker.types import ArtifactType

  from freshmaker.handlers.bodhi import BodhiUpdateCompleteStableHandler
@@ -105,6 +107,11 @@ 

      @mock.patch('freshmaker.handlers.bodhi.update_complete_stable.PDC')

      @mock.patch('freshmaker.handlers.bodhi.update_complete_stable.utils')

      @mock.patch('freshmaker.handlers.bodhi.update_complete_stable.conf')

+     @mock.patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'BodhiUpdateCompleteStableHandler': {

+             'image': [{'name': r'testimage\d', 'branch': 'f25'}]

+         }

+     })

      def test_trigger_rebuild_container_when_receives_bodhi_update_complete_stable_message(self, conf, utils, PDC):

  

          conf.git_base_url = 'git://pkgs.fedoraproject.org'

@@ -23,10 +23,11 @@ 

  

  from mock import patch

  

- from freshmaker.handlers.errata import ErrataAdvisoryRPMsSignedHandler

- from freshmaker.events import ErrataAdvisoryRPMsSignedEvent

+ import freshmaker

  

  from freshmaker import db

+ from freshmaker.events import ErrataAdvisoryRPMsSignedEvent

+ from freshmaker.handlers.errata import ErrataAdvisoryRPMsSignedHandler

  from freshmaker.models import Event

  from freshmaker.types import EventState

  
@@ -152,6 +153,11 @@ 

          db.drop_all()

          db.session.commit()

  

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'ErrataAdvisoryRPMsSignedHandler': {

+             'image': [{'advisory_name': 'RHBA-2017'}]

+         }

+     })

      def test_event_state_updated_when_no_images_to_rebuild(self):

          self.mock_find_images_to_rebuild.return_value = iter([[[]]])

          event = ErrataAdvisoryRPMsSignedEvent(

@@ -27,6 +27,8 @@ 

  from mock import patch

  from mock import MagicMock, PropertyMock

  

+ import freshmaker

+ 

  from freshmaker import db, models

  from freshmaker.consumer import FreshmakerConsumer

  from freshmaker.types import ArtifactType
@@ -65,6 +67,11 @@ 

      @patch('freshmaker.utils.krbContext')

      @patch("freshmaker.config.Config.krb_auth_principal",

             new_callable=PropertyMock, return_value="user@example.com")

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'GitDockerfileChangeHandler': {

+             'image': [{'name': 'testimage'}, {'branch': 'master'}]

+         }

+     })

      def test_rebuild_if_dockerfile_changed(

              self, auth_principal, krbContext, ClientSession, read_config):

          read_config.return_value = {
@@ -105,6 +112,11 @@ 

  

      @patch('koji.read_config')

      @patch('koji.ClientSession')

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'GitDockerfileChangeHandler': {

+             'image': [{'name': 'testimage'}, {'branch': 'master'}]

+         }

+     })

      def test_ensure_logout_in_whatever_case(self, ClientSession, read_config):

          ClientSession.return_value.buildContainer.side_effect = RuntimeError

          read_config.return_value = {

@@ -26,6 +26,8 @@ 

  sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))  # noqa

  from tests import helpers

  

+ import freshmaker

+ 

  from freshmaker import events, db, models

  from freshmaker.types import ArtifactType

  from freshmaker.handlers.git import GitModuleMetadataChangeHandler
@@ -59,6 +61,11 @@ 

          handler = GitModuleMetadataChangeHandler()

          self.assertTrue(handler.can_handle(event))

  

+     @mock.patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'GitModuleMetadataChangeHandler': {

+             'module': [{'name': 'testmodule'}, {'branch': 'master'}]

+         }

+     })

      def test_can_rebuild_module_when_module_metadata_changed(self):

          """

          Tests handler can rebuild module when module metadata is changed in dist-git

@@ -26,6 +26,8 @@ 

  sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))  # noqa

  from tests import helpers

  

+ import freshmaker

+ 

  from freshmaker import events, db, models

  from freshmaker.types import ArtifactType

  from freshmaker.handlers.git import GitRPMSpecChangeHandler
@@ -76,6 +78,11 @@ 

      @mock.patch('freshmaker.handlers.git.rpm_spec_change.PDC')

      @mock.patch('freshmaker.handlers.git.rpm_spec_change.utils')

      @mock.patch('freshmaker.handlers.git.rpm_spec_change.conf')

+     @mock.patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'GitRPMSpecChangeHandler': {

+             'module': [{'name': 'testmodule'}, {'branch': 'master'}]

+         }

+     })

      def test_can_rebuild_modules_has_rpm_included(self, conf, utils, PDC):

          """

          Test handler can rebuild modules which include the rpm.

file modified
+134 -40
@@ -27,6 +27,8 @@ 

  from mock import patch, PropertyMock

  from unittest import TestCase

  

+ import freshmaker

+ 

  from freshmaker import db

  from freshmaker.events import ErrataAdvisoryRPMsSignedEvent

  from freshmaker.handlers import ContainerBuildHandler
@@ -138,6 +140,138 @@ 

          self.assertRaises(ProgrammingError, handler.set_context, "something")

  

  

+ class TestAllowBuildBasedOnWhitelist(TestCase):

+     """Test BaseHandler.allow_build"""

+ 

+     @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"])

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MyHandler': {

+             'image': [

+                 {'advisory_state': ['REL_PREP', 'SHIPPED_LIVE']}

+             ]

+         }

+     })

+     def test_not_allow_if_none_passed_rule_is_configured(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE, state='SHIPPED_LIVE')

+         self.assertFalse(allowed)

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={})

+     def test_not_allow_if_whitelist_is_not_configured(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE, state='SHIPPED_LIVE')

+         self.assertFalse(allowed)

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MyHandler': {

+             'image': [

+                 {'advisory_state': ['REL_PREP', 'SHIPPED_LIVE']}

+             ]

+         }

+     })

+     def test_define_rule_values_as_list(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_state='SHIPPED_LIVE')

+         self.assertTrue(allowed)

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MyHandler': {

+             'image': [

+                 {'advisory_name': 'RHSA-\d+:\d+'}

+             ]

+         }

+     })

+     def test_define_rule_value_as_single_regex_string(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHSA-2017:31861')

+         self.assertTrue(allowed)

+ 

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHBA-2017:31861')

+         self.assertFalse(allowed)

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MyHandler': {

+             'image': [{

+                 'advisory_name': 'RHSA-\d+:\d+',

+                 'advisory_state': 'REL_PREP'

+             }]

+         }

+     })

+     def test_AND_rule(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHSA-2017:1000',

+                                       advisory_state='REL_PREP')

+         self.assertTrue(allowed)

+ 

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHSA-2017:1000',

+                                       advisory_state='SHIPPED_LIVE')

+         self.assertFalse(allowed)

+ 

+     @patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MyHandler': {

+             'image': [

+                 {'advisory_name': 'RHSA-\d+:\d+'},

+                 {'advisory_state': 'REL_PREP'},

+             ]

+         }

+     })

+     def test_OR_rule(self):

+         handler = MyHandler()

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHSA-2017:1000',

+                                       advisory_state='SHIPPED_LIVE')

+         self.assertTrue(allowed)

+ 

+         allowed = handler.allow_build(ArtifactType.IMAGE,

+                                       advisory_name='RHSA-2017',

+                                       advisory_state='REL_PREP')

+         self.assertTrue(allowed)

+ 

+ 

  class AnyStringWith(str):

      def __eq__(self, other):

          return self in other
@@ -263,46 +397,6 @@ 

                  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"])

- 

      @patch('freshmaker.handlers.ODCS')

      @patch('koji.ClientSession')

      @patch('freshmaker.utils.krbContext')

@@ -26,6 +26,8 @@ 

  sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))  # noqa

  from tests import helpers

  

+ import freshmaker

+ 

  from freshmaker import events, db, models

  from freshmaker.types import ArtifactType

  from freshmaker.handlers.mbs import MBSModuleStateChangeHandler
@@ -60,6 +62,11 @@ 

      @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.conf')

+     @mock.patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MBSModuleStateChangeHandler': {

+             'module': [{'name': r'testmodule\d*'}, {'branch': 'master'}],

+         }

+     })

      def test_can_rebuild_depending_modules(self, conf, utils, PDC):

          """

          Tests handler can rebuild all modules which depend on the module
@@ -121,7 +128,6 @@ 

                  ],

              },

          }

-         conf.handler_build_blacklist = {}

  

          msg = helpers.ModuleStateChangeMessage('testmodule', 'master', state='ready').produce()

          event = self.get_event_from_msg(msg)
@@ -145,6 +151,11 @@ 

      @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')

+     @mock.patch.object(freshmaker.conf, 'handler_build_whitelist', new={

+         'MBSModuleStateChangeHandler': {

+             'module': [{'name': r'module\d+'}, {'branch': 'master'}]

+         }

+     })

      def test_handler_not_fall_into_cyclic_rebuild_loop(self, log, utils, PDC):

          """

          Tests handler will not fall into cyclic rebuild loop when there is

file modified
+1 -1
@@ -39,4 +39,4 @@ 

  

  [flake8]

  ignore = E501,E731

- exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__

+ exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__,scripts/print_handlers_md.py

In this PR, remaining word blacklist are removed from source code. allow_build is updated to not allow rebuild if rule names passed as parameter to allow_build and configured rule names do not match. For example, handler_build_whitelist is configured as below

HANDLER_BUILD_WHITELIST = {
    'some_handler': {
        'image': [{'name': 'RHSA-0001'}]
    }
}

and if calling if self.allow_build(ArtifactType.IMAGE, advisory_name='RHSA-0001'), False should be returned.

This PR should fix #107 in a way.

2 new commits added

  • Do not allow build if configured whitelist and passed rule name does not match
  • Clean word blacklist since blacklist has been removed
6 years ago

Maybe we can change the default value of in_whitelist to False, and then requires the artifact to be in whitelist explicitly?

Have no idea at this moment. Looks in_whitelist is used to hold the final return value. But, this is out of the scope of this PR. I think it could be fixed in another PR if it's worth to fix.

If this log.debug should stay hee, log also some description saying what those logged values mean.

This only checks that the keys provided to allow_build are set in the whitelist in configuration file and if not, it returns false, right? If so, could you add comment there, it could save some time when reading this code.

I think @qwan is right that if this should fix #107, we should probably remove in_whitelist and change this condition like this:

            if whitelist and any([match_rule(kwargs, rule) for rule in whitelist]):
                log.debug('%r, type=%r is whitelisted.',
                          kwargs, artifact_type.name.lower())
                return True

And also return False in the end of allow_build method instead of return in_whitelist.

rebased onto 5c0bd2afbbed662df6ca43b9fa02220618cb720f

6 years ago

Ah, this log should not present here. I added it for debug purpose. Let me remove it.

2 new commits added

  • Do not allow build if configured whitelist and passed rule name does not match
  • Clean word blacklist since blacklist has been removed
6 years ago

Fixed issues mentioned in comments. PTAL.

1 new commit added

  • Fix flake8 errors
6 years ago

+1, this looks good now :)

This one now conflicts with your previous PR I've just merged... Could you rebase?

rebased onto 7525299

6 years ago

Pull-Request has been merged by jkaluza

6 years ago