From 65271ec40c6e0144b8a89c136d988b637dabdcb0 Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Jun 07 2017 09:00:13 +0000 Subject: [PATCH 1/3] Create an enum for artifact types And update allow_build to use the enum members instead of strings. --- diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index bc1456a..23f0abf 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -111,7 +111,7 @@ class BaseHandler(object): Check whether the artifact is allowed to be built by checking HANDLER_BUILD_WHITELIST and HANDLER_BUILD_BLACKLIST in config. - :param artifact_type: 'module' or 'image'. + :param artifact_type: an enum member of ArtifactType. :param name: name of the artifact. :param branch: branch name of the artifact. :return: True or False. @@ -139,23 +139,23 @@ class BaseHandler(object): return True try: - whitelist = whitelist_rules.get(artifact_type, []) + whitelist = whitelist_rules.get(artifact_type.name.lower(), []) if whitelist and not any([match_rule(name, branch, rule) for rule in whitelist]): log.debug('name=%r, branch=%r, type=%r is not whitelisted.', - name, branch, artifact_type) + name, branch, 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, []) + blacklist = blacklist_rules.get(artifact_type.name.lower(), []) if blacklist and any([match_rule(name, branch, rule) for rule in blacklist]): log.debug('name=%r, branch=%r, type=%r is blacklisted.', - name, branch, artifact_type) + name, branch, artifact_type.name.lower()) in_blacklist = True except re.error as exc: log.error("Error while compiling blacklist/whilelist rule for :\n" "Incorrect regular expression: %s\nBlacklist and Whitelist will not take effect", - handler_name, artifact_type, str(exc)) + handler_name, artifact_type.name.lower(), str(exc)) return True return in_whitelist and not in_blacklist diff --git a/freshmaker/handlers/bodhi/update_complete_stable.py b/freshmaker/handlers/bodhi/update_complete_stable.py index a62aa82..2609148 100644 --- a/freshmaker/handlers/bodhi/update_complete_stable.py +++ b/freshmaker/handlers/bodhi/update_complete_stable.py @@ -26,6 +26,7 @@ from itertools import chain from freshmaker import conf from freshmaker import log from freshmaker import utils +from freshmaker.types import ArtifactType from freshmaker.handlers import BaseHandler from freshmaker.events import BodhiUpdateCompleteStableEvent from freshmaker.pdc import PDC @@ -49,7 +50,7 @@ class BodhiUpdateCompleteStableHandler(BaseHandler): log.info('Found docker images to rebuild: %s', containers) for container in containers: - if not self.allow_build('image', container['name'], container['branch']): + if not self.allow_build(ArtifactType.IMAGE, container['name'], container['branch']): log.info("Skip rebuild of image %s:%s as it's not allowed by configured whitelist/blacklist", container['name'], container['branch']) continue diff --git a/freshmaker/handlers/git/dockerfile_change.py b/freshmaker/handlers/git/dockerfile_change.py index bb0d667..9f38ea2 100644 --- a/freshmaker/handlers/git/dockerfile_change.py +++ b/freshmaker/handlers/git/dockerfile_change.py @@ -22,6 +22,7 @@ # Written by Chenxiong Qi from freshmaker import log +from freshmaker.types import ArtifactType from freshmaker.handlers import BaseHandler from freshmaker.events import GitDockerfileChangeEvent @@ -38,7 +39,7 @@ class GitDockerfileChangeHandler(BaseHandler): log.info('Start to rebuild docker image %s.', event.container) - if not self.allow_build('image', event.container, event.branch): + if not self.allow_build(ArtifactType.IMAGE, event.container, event.branch): log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist", event.container, event.branch) return [] diff --git a/freshmaker/handlers/git/module_metadata_change.py b/freshmaker/handlers/git/module_metadata_change.py index 8da9f9f..9f1f436 100644 --- a/freshmaker/handlers/git/module_metadata_change.py +++ b/freshmaker/handlers/git/module_metadata_change.py @@ -23,6 +23,7 @@ from freshmaker import log +from freshmaker.types import ArtifactType from freshmaker.handlers import BaseHandler from freshmaker.events import GitModuleMetadataChangeEvent @@ -39,7 +40,7 @@ class GitModuleMetadataChangeHandler(BaseHandler): def handle(self, event): log.info("Triggering rebuild of module %s:%s, metadata updated (%s).", event.module, event.branch, event.rev) - if not self.allow_build('module', event.module, event.branch): + if not self.allow_build(ArtifactType.MODULE, event.module, event.branch): log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist", event.module, event.branch) return [] diff --git a/freshmaker/handlers/git/rpm_spec_change.py b/freshmaker/handlers/git/rpm_spec_change.py index 59f9a71..43d2dc1 100644 --- a/freshmaker/handlers/git/rpm_spec_change.py +++ b/freshmaker/handlers/git/rpm_spec_change.py @@ -20,6 +20,7 @@ # SOFTWARE. from freshmaker import log, conf, utils +from freshmaker.types import ArtifactType from freshmaker.pdc import PDC from freshmaker.handlers import BaseHandler from freshmaker.events import GitRPMSpecChangeEvent @@ -53,7 +54,7 @@ class GitRPMSpecChangeHandler(BaseHandler): for module in modules: name = module['variant_name'] version = module['variant_version'] - if not self.allow_build('module', name, version): + if not self.allow_build(ArtifactType.MODULE, name, version): log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist", name, version) continue diff --git a/freshmaker/handlers/mbs/module_state_change.py b/freshmaker/handlers/mbs/module_state_change.py index e37bd57..d868721 100644 --- a/freshmaker/handlers/mbs/module_state_change.py +++ b/freshmaker/handlers/mbs/module_state_change.py @@ -22,6 +22,7 @@ # Written by Jan Kaluza from freshmaker import log, conf, utils, db, models +from freshmaker.types import ArtifactType from freshmaker.mbs import MBS from freshmaker.pdc import PDC from freshmaker.handlers import BaseHandler @@ -77,7 +78,7 @@ class MBSModuleStateChangeHandler(BaseHandler): for mod in modules: name = mod['variant_name'] version = mod['variant_version'] - if not self.allow_build('module', name, version): + if not self.allow_build(ArtifactType.MODULE, name, version): log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist/blacklist", name, version) continue diff --git a/freshmaker/types.py b/freshmaker/types.py new file mode 100644 index 0000000..64b304c --- /dev/null +++ b/freshmaker/types.py @@ -0,0 +1,28 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2017 Red Hat, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +from enum import Enum + + +class ArtifactType(Enum): + RPM = 0 + IMAGE = 1 + MODULE = 2 diff --git a/requirements.txt b/requirements.txt index 83cef9f..3b165cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,3 +21,4 @@ Flask-Migrate Flask-SQLAlchemy Flask-Script requests +enum34 ; python_version <= '2.7' From 9af368f380b2c1b7b4604b9ad908e3de1d22eabc Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Jun 07 2017 09:00:13 +0000 Subject: [PATCH 2/3] Use ArtifactType enum to replace models.ARTIFACT_TYPES --- diff --git a/freshmaker/handlers/koji/task_state_change.py b/freshmaker/handlers/koji/task_state_change.py index 39d676d..212aaf8 100644 --- a/freshmaker/handlers/koji/task_state_change.py +++ b/freshmaker/handlers/koji/task_state_change.py @@ -20,6 +20,7 @@ # SOFTWARE. from freshmaker import log, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers import BaseHandler from freshmaker.events import KojiTaskStateChangeEvent @@ -39,7 +40,7 @@ class KojiTaskStateChangeHandler(BaseHandler): # check whether the task exists in db as image build builds = db.session.query(models.ArtifactBuild).filter_by(build_id=task_id, - type=models.ARTIFACT_TYPES['image']).all() + type=ArtifactType.IMAGE.value).all() if len(builds) > 1: raise RuntimeError("Found duplicate image build '%s' in db" % task_id) if len(builds) == 1: diff --git a/freshmaker/handlers/mbs/module_state_change.py b/freshmaker/handlers/mbs/module_state_change.py index d868721..f347407 100644 --- a/freshmaker/handlers/mbs/module_state_change.py +++ b/freshmaker/handlers/mbs/module_state_change.py @@ -52,7 +52,7 @@ class MBSModuleStateChangeHandler(BaseHandler): # update build state if the build is submitted by Freshmaker builds = db.session.query(models.ArtifactBuild).filter_by(build_id=build_id, - type=models.ARTIFACT_TYPES['module']).all() + type=ArtifactType.MODULE.value).all() if len(builds) > 1: raise RuntimeError("Found duplicate module build '%s' in db" % build_id) if len(builds) == 1: diff --git a/freshmaker/models.py b/freshmaker/models.py index f829117..04c4554 100644 --- a/freshmaker/models.py +++ b/freshmaker/models.py @@ -28,6 +28,7 @@ from datetime import datetime from sqlalchemy.orm import (validates, relationship) from freshmaker import db +from freshmaker.types import ArtifactType from freshmaker.events import ( MBSModuleStateChangeEvent, GitModuleMetadataChangeEvent, GitRPMSpecChangeEvent, TestingEvent, GitDockerfileChangeEvent, @@ -47,14 +48,6 @@ BUILD_STATES = { INVERSE_BUILD_STATES = {v: k for k, v in BUILD_STATES.items()} -ARTIFACT_TYPES = { - "rpm": 0, - "image": 1, - "module": 2, -} - -INVERSE_ARTIFACT_TYPES = {v: k for k, v in ARTIFACT_TYPES.items()} - EVENT_TYPES = { MBSModuleStateChangeEvent: 0, GitModuleMetadataChangeEvent: 1, @@ -160,13 +153,13 @@ class ArtifactBuild(FreshmakerBase): @validates('type') def validate_type(self, key, field): - if field in ARTIFACT_TYPES.values(): + if field in [t.value for t in list(ArtifactType)]: return field - if field in ARTIFACT_TYPES: - return ARTIFACT_TYPES[field] - raise ValueError("%s: %s, not in %r" % (key, field, ARTIFACT_TYPES)) + if field in [t.name.lower() for t in list(ArtifactType)]: + return ArtifactType[field.upper()].value + raise ValueError("%s: %s, not in %r" % (key, field, list(ArtifactType))) def __repr__(self): return "" % ( - self.name, INVERSE_ARTIFACT_TYPES[self.type], + self.name, ArtifactType(self.type).name, INVERSE_BUILD_STATES[self.state], self.event.message_id) diff --git a/tests/test_bodhi_update_complete_stable_handler.py b/tests/test_bodhi_update_complete_stable_handler.py index b8751ba..70b899d 100644 --- a/tests/test_bodhi_update_complete_stable_handler.py +++ b/tests/test_bodhi_update_complete_stable_handler.py @@ -31,6 +31,7 @@ from tests import helpers from tests import get_fedmsg from freshmaker import events, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers.bodhi import BodhiUpdateCompleteStableHandler from freshmaker.parsers.bodhi import BodhiUpdateCompleteStableParser @@ -152,10 +153,10 @@ class BodhiUpdateCompleteStableHandlerTest(helpers.FreshmakerTestCase): builds = models.ArtifactBuild.query.all() self.assertEqual(len(builds), 2) self.assertEqual(builds[0].name, 'testimage1') - self.assertEqual(builds[0].type, models.ARTIFACT_TYPES['image']) + self.assertEqual(builds[0].type, ArtifactType.IMAGE.value) self.assertEqual(builds[0].build_id, 123) self.assertEqual(builds[1].name, 'testimage2') - self.assertEqual(builds[1].type, models.ARTIFACT_TYPES['image']) + self.assertEqual(builds[1].type, ArtifactType.IMAGE.value) self.assertEqual(builds[1].build_id, 456) @mock.patch('freshmaker.handlers.bodhi.update_complete_stable.PDC') diff --git a/tests/test_git_dockerfile_change_handler.py b/tests/test_git_dockerfile_change_handler.py index 915d325..2ce5b33 100644 --- a/tests/test_git_dockerfile_change_handler.py +++ b/tests/test_git_dockerfile_change_handler.py @@ -31,6 +31,7 @@ from mock import MagicMock from freshmaker import db, models from freshmaker.consumer import FreshmakerConsumer +from freshmaker.types import ArtifactType from tests import get_fedmsg @@ -89,7 +90,7 @@ class GitDockerfileChangeHandlerTest(BaseTestCase): builds = models.ArtifactBuild.query.all() self.assertEqual(len(builds), 1) self.assertEqual(builds[0].name, 'testimage') - self.assertEqual(builds[0].type, models.ARTIFACT_TYPES['image']) + self.assertEqual(builds[0].type, ArtifactType.IMAGE.value) self.assertEqual(builds[0].build_id, 123) @patch('freshmaker.handlers.git.dockerfile_change.GitDockerfileChangeHandler.build_container') diff --git a/tests/test_git_module_metadata_change_handler.py b/tests/test_git_module_metadata_change_handler.py index a02903c..5d7482b 100644 --- a/tests/test_git_module_metadata_change_handler.py +++ b/tests/test_git_module_metadata_change_handler.py @@ -27,6 +27,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import helpers from freshmaker import events, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers.git import GitModuleMetadataChangeHandler from freshmaker.parsers.git import GitReceiveParser @@ -84,7 +85,7 @@ class GitModuleMetadataChangeHandlerTest(helpers.FreshmakerTestCase): builds = models.ArtifactBuild.query.all() self.assertEqual(len(builds), 1) self.assertEqual(builds[0].name, 'testmodule') - self.assertEqual(builds[0].type, models.ARTIFACT_TYPES['module']) + self.assertEqual(builds[0].type, ArtifactType.MODULE.value) self.assertEqual(builds[0].build_id, 123) diff --git a/tests/test_git_rpm_spec_change_handler.py b/tests/test_git_rpm_spec_change_handler.py index 1cae34f..d5e5a11 100644 --- a/tests/test_git_rpm_spec_change_handler.py +++ b/tests/test_git_rpm_spec_change_handler.py @@ -27,6 +27,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import helpers from freshmaker import events, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers.git import GitRPMSpecChangeHandler from freshmaker.parsers.git import GitReceiveParser @@ -111,7 +112,7 @@ class GitRPMSpecChangeHandlerTest(helpers.FreshmakerTestCase): builds = models.ArtifactBuild.query.all() self.assertEqual(len(builds), 1) self.assertEqual(builds[0].name, 'testmodule') - self.assertEqual(builds[0].type, models.ARTIFACT_TYPES['module']) + self.assertEqual(builds[0].type, ArtifactType.MODULE.value) self.assertEqual(builds[0].build_id, 123) diff --git a/tests/test_koji_task_state_change_handler.py b/tests/test_koji_task_state_change_handler.py index 07123a9..c130c5c 100644 --- a/tests/test_koji_task_state_change_handler.py +++ b/tests/test_koji_task_state_change_handler.py @@ -26,6 +26,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import helpers from freshmaker import events, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers.koji import KojiTaskStateChangeHandler from freshmaker.parsers.koji import KojiTaskStateChangeParser @@ -64,7 +65,7 @@ class KojiTaskStateChangeHandlerTest(helpers.FreshmakerTestCase): build = models.ArtifactBuild.create(db.session, ev, 'testimage', - models.ARTIFACT_TYPES['image'], + ArtifactType.IMAGE.value, task_id) db.session.add(ev) db.session.add(build) diff --git a/tests/test_mbs_module_state_change_handler.py b/tests/test_mbs_module_state_change_handler.py index 82cd04c..3e56983 100644 --- a/tests/test_mbs_module_state_change_handler.py +++ b/tests/test_mbs_module_state_change_handler.py @@ -27,6 +27,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import helpers from freshmaker import events, db, models +from freshmaker.types import ArtifactType from freshmaker.handlers.mbs import MBSModuleStateChangeHandler from freshmaker.parsers.mbs import MBSModuleStateChangeParser @@ -101,11 +102,11 @@ class MBSModuleStateChangeHandlerTest(helpers.FreshmakerTestCase): builds = models.ArtifactBuild.query.all() self.assertEqual(len(builds), 2) self.assertEqual(builds[0].name, mod2_r1['variant_name']) - self.assertEqual(builds[0].type, models.ARTIFACT_TYPES['module']) + self.assertEqual(builds[0].type, ArtifactType.MODULE.value) self.assertEqual(builds[0].build_id, 123) self.assertEqual(builds[1].name, mod3_r1['variant_name']) self.assertEqual(builds[1].build_id, 456) - self.assertEqual(builds[1].type, models.ARTIFACT_TYPES['module']) + self.assertEqual(builds[1].type, ArtifactType.MODULE.value) @mock.patch('freshmaker.handlers.mbs.module_state_change.PDC') @mock.patch('freshmaker.handlers.mbs.module_state_change.utils') From 1492e08c7ed6104b1280ae9409fed2fa067bff99 Mon Sep 17 00:00:00 2001 From: Qixiang Wan Date: Jun 07 2017 09:00:13 +0000 Subject: [PATCH 3/3] Add ArtifactBuildState enum to replace models.BUILD_STATES --- diff --git a/freshmaker/handlers/koji/task_state_change.py b/freshmaker/handlers/koji/task_state_change.py index 212aaf8..1bfe470 100644 --- a/freshmaker/handlers/koji/task_state_change.py +++ b/freshmaker/handlers/koji/task_state_change.py @@ -20,7 +20,7 @@ # SOFTWARE. from freshmaker import log, db, models -from freshmaker.types import ArtifactType +from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.handlers import BaseHandler from freshmaker.events import KojiTaskStateChangeEvent @@ -48,10 +48,10 @@ class KojiTaskStateChangeHandler(BaseHandler): if task_state in ['CLOSED', 'FAILED']: log.info("Image build '%s' state changed in koji, updating it in db.", task_id) if task_state == 'CLOSED': - build.state = models.BUILD_STATES['done'] + build.state = ArtifactBuildState.DONE.value db.session.commit() if task_state == 'FAILED': - build.state = models.BUILD_STATES['failed'] + build.state = ArtifactBuildState.FAILED.value db.session.commit() return [] diff --git a/freshmaker/models.py b/freshmaker/models.py index 04c4554..25daf1a 100644 --- a/freshmaker/models.py +++ b/freshmaker/models.py @@ -28,26 +28,12 @@ from datetime import datetime from sqlalchemy.orm import (validates, relationship) from freshmaker import db -from freshmaker.types import ArtifactType +from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.events import ( MBSModuleStateChangeEvent, GitModuleMetadataChangeEvent, GitRPMSpecChangeEvent, TestingEvent, GitDockerfileChangeEvent, BodhiUpdateCompleteStableEvent, KojiTaskStateChangeEvent) -# BUILD_STATES for the builds submitted by Freshmaker -BUILD_STATES = { - # Artifact is building. - "build": 0, - # Artifact build is sucessfuly done. - "done": 1, - # Artifact build has failed. - "failed": 2, - # Artifact build is canceled. - "canceled": 3, -} - -INVERSE_BUILD_STATES = {v: k for k, v in BUILD_STATES.items()} - EVENT_TYPES = { MBSModuleStateChangeEvent: 0, GitModuleMetadataChangeEvent: 1, @@ -145,11 +131,11 @@ class ArtifactBuild(FreshmakerBase): @validates('state') def validate_state(self, key, field): - if field in BUILD_STATES.values(): + if field in [s.value for s in list(ArtifactBuildState)]: return field - if field in BUILD_STATES: - return BUILD_STATES[field] - raise ValueError("%s: %s, not in %r" % (key, field, BUILD_STATES)) + if field in [s.name.lower() for s in list(ArtifactBuildState)]: + return ArtifactBuildState[field.upper()].value + raise ValueError("%s: %s, not in %r" % (key, field, list(ArtifactBuildState))) @validates('type') def validate_type(self, key, field): @@ -162,4 +148,4 @@ class ArtifactBuild(FreshmakerBase): def __repr__(self): return "" % ( self.name, ArtifactType(self.type).name, - INVERSE_BUILD_STATES[self.state], self.event.message_id) + ArtifactBuildState(self.state).name, self.event.message_id) diff --git a/freshmaker/types.py b/freshmaker/types.py index 64b304c..1ee75b0 100644 --- a/freshmaker/types.py +++ b/freshmaker/types.py @@ -26,3 +26,10 @@ class ArtifactType(Enum): RPM = 0 IMAGE = 1 MODULE = 2 + + +class ArtifactBuildState(Enum): + BUILD = 0 + DONE = 1 + FAILED = 2 + CANCELED = 3 diff --git a/tests/test_koji_task_state_change_handler.py b/tests/test_koji_task_state_change_handler.py index c130c5c..0f9462d 100644 --- a/tests/test_koji_task_state_change_handler.py +++ b/tests/test_koji_task_state_change_handler.py @@ -26,7 +26,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) # noqa from tests import helpers from freshmaker import events, db, models -from freshmaker.types import ArtifactType +from freshmaker.types import ArtifactType, ArtifactBuildState from freshmaker.handlers.koji import KojiTaskStateChangeHandler from freshmaker.parsers.koji import KojiTaskStateChangeParser @@ -81,7 +81,7 @@ class KojiTaskStateChangeHandlerTest(helpers.FreshmakerTestCase): handler.handle(event) build = models.ArtifactBuild.query.all()[0] - self.assertEqual(build.state, models.BUILD_STATES['failed']) + self.assertEqual(build.state, ArtifactBuildState.FAILED.value) m = helpers.KojiTaskStateChangeMessage(task_id, 'OPEN', 'CLOSED') msg = m.produce() @@ -91,7 +91,7 @@ class KojiTaskStateChangeHandlerTest(helpers.FreshmakerTestCase): handler.handle(event) build = models.ArtifactBuild.query.all()[0] - self.assertEqual(build.state, models.BUILD_STATES['done']) + self.assertEqual(build.state, ArtifactBuildState.DONE.value) if __name__ == '__main__':