From 5a0352a79cfc1675fc50f8648075810eee2bc68c Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 13 2018 04:32:11 +0000 Subject: [PATCH 1/4] Dry run module rebuild for GitRPMSpecChangeEvent Signed-off-by: Chenxiong Qi --- diff --git a/conf/config.py b/conf/config.py index 7fc1c22..45d4695 100644 --- a/conf/config.py +++ b/conf/config.py @@ -242,6 +242,8 @@ class DevConfiguration(BaseConfiguration): AUTH_BACKEND = 'noauth' AUTH_OPENIDC_USERINFO_URI = 'https://iddev.fedorainfracloud.org/openidc/UserInfo' + DRY_RUN = True + class TestConfiguration(BaseConfiguration): LOG_BACKEND = 'console' diff --git a/freshmaker/handlers/git/rpm_spec_change.py b/freshmaker/handlers/git/rpm_spec_change.py index c1ce5a5..b15b07d 100644 --- a/freshmaker/handlers/git/rpm_spec_change.py +++ b/freshmaker/handlers/git/rpm_spec_change.py @@ -19,11 +19,12 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. -from freshmaker import log, conf, utils +from freshmaker import db, log, conf, utils from freshmaker.types import ArtifactType from freshmaker.pdc import PDC from freshmaker.handlers import BaseHandler from freshmaker.events import GitRPMSpecChangeEvent +from freshmaker.models import ArtifactBuild class GitRPMSpecChangeHandler(BaseHandler): @@ -35,10 +36,19 @@ class GitRPMSpecChangeHandler(BaseHandler): return False + def _init_max_build_id(self): + """Ensure the uniqueness of ArtifactBuild.build_id in dry run mode""" + rows = db.session.query(ArtifactBuild.build_id).filter_by( + type=ArtifactType.MODULE.value).all() + self._max_build_id = max((row[0] for row in rows)) if rows else 0 + def handle(self, event): """ Rebuild module when spec file of rpm in module is updated. """ + if conf.dry_run: + self._init_max_build_id() + rpm = event.rpm branch = event.branch rev = event.rev @@ -51,18 +61,39 @@ class GitRPMSpecChangeHandler(BaseHandler): component_branch=branch, active='true') - for module in modules: - 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", - name, version) - continue - log.info("Going to rebuild module '%s:%s'.", name, version) - commit_msg = "Bump to rebuild because of %s rpm spec update (%s)." % (rpm, rev) - rev = utils.bump_distgit_repo('modules', name, branch=version, commit_msg=commit_msg, logger=log) - build_id = self.build_module(name, version, rev) - if build_id is not None: - self.record_build(event, name, ArtifactType.MODULE, build_id) + if conf.dry_run: + for module in modules: + name = module['variant_name'] + version = module['variant_version'] + log.info('DRY-RUN: Allow to build fake module %s', module) + log.info("DRY-RUN: Going to rebuild module '%s:%s'.", name, version) + commit_msg = "Bump to rebuild because of %s rpm spec update (%s)." % (rpm, rev) + log.info('DRY-RUN: Bump dist-git repo, modules/%s, branch %s, commit message %s', + name, version, commit_msg) + rev = '1234567' + log.info('DRY-RUN: Start to build module %s, version %s, rev %s', + name, version, rev) + + build_id = self.build_module(name, version, rev) + if build_id is not None: + build_id = self._max_build_id + build_id + self._max_build_id = build_id + + log.info('DRY-RUN: Record fake MBS module build ID %s', build_id) + build = self.record_build(event, name, ArtifactType.MODULE, build_id) + log.info('DRY-RUN: Recorded build: %s', build) + else: + for module in modules: + 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", name, version) + continue + log.info("Going to rebuild module '%s:%s'.", name, version) + commit_msg = "Bump to rebuild because of %s rpm spec update (%s)." % (rpm, rev) + rev = utils.bump_distgit_repo('modules', name, branch=version, commit_msg=commit_msg, logger=log) + build_id = self.build_module(name, version, rev) + if build_id is not None: + self.record_build(event, name, ArtifactType.MODULE, build_id) return [] diff --git a/freshmaker/mbs.py b/freshmaker/mbs.py index b58a76d..84111ff 100644 --- a/freshmaker/mbs.py +++ b/freshmaker/mbs.py @@ -23,7 +23,7 @@ import requests -from freshmaker import log +from freshmaker import conf, log class MBS(object): @@ -62,8 +62,12 @@ class MBS(object): body = {'scmurl': scm_url, 'branch': branch} url = "%s/module-build-service/1/module-builds/" % self.base_url - resp = requests.request("POST", url, headers=headers, json=body) - data = resp.json() + if conf.dry_run: + log.info('DRY-RUN: Request MBS to rebuild: %s', ('POST', url, headers, body)) + data = {'id': 1} + else: + resp = requests.request("POST", url, headers=headers, json=body) + data = resp.json() if 'id' in data: log.info("Triggered build of %s, MBS build_id=%s", scm_url, data['id']) return data['id'] diff --git a/tests/test_git_rpm_spec_change_handler.py b/tests/test_git_rpm_spec_change_handler.py index 568e21c..b0c9e7d 100644 --- a/tests/test_git_rpm_spec_change_handler.py +++ b/tests/test_git_rpm_spec_change_handler.py @@ -68,18 +68,17 @@ class GitRPMSpecChangeHandlerTest(helpers.ModelsTestCase): @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.handlers.git.rpm_spec_change.conf, + 'git_base_url', new='git://pkgs.fedoraproject.org') @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): + def test_can_rebuild_modules_has_rpm_included(self, utils, PDC): """ Test handler can rebuild modules which include the rpm. """ - conf.git_base_url = "git://pkgs.fedoraproject.org" - m = helpers.DistGitMessage('rpms', 'bash', 'master', '123') m.add_changed_file('bash.spec', 1, 1) msg = m.produce() From 30372ecebaff3984b69f46191ed301b1e1f5e46a Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 13 2018 04:39:49 +0000 Subject: [PATCH 2/4] Dry run module rebuild for GitModuleMetadataChangeEvent Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/__init__.py b/freshmaker/handlers/__init__.py index e5db5e8..178f9db 100644 --- a/freshmaker/handlers/__init__.py +++ b/freshmaker/handlers/__init__.py @@ -33,7 +33,7 @@ from freshmaker.kojiservice import koji_service, parse_NVR from freshmaker.mbs import MBS from freshmaker.models import ArtifactBuildState from freshmaker.types import EventState -from freshmaker.models import ArtifactBuild, Event +from freshmaker.models import ArtifactBuild, ArtifactType, Event from freshmaker.utils import krb_context, get_rebuilt_nvr from freshmaker.errors import UnprocessableEntity, ProgrammingError from freshmaker.odcsclient import create_odcs_client @@ -117,6 +117,15 @@ class BaseHandler(object): self._db_artifact_build_id = None self._log_prefix = "" + if conf.dry_run: + self._init_max_build_id() + + def _init_max_build_id(self): + """Ensure the uniqueness of ArtifactBuild.build_id in dry run mode""" + rows = db.session.query(ArtifactBuild.build_id).filter_by( + type=ArtifactType.MODULE.value).all() + self._max_build_id = max((row[0] for row in rows)) if rows else 0 + def _log(self, log_fnc, msg, *args, **kwargs): """ Logs the message `msg` using `log_fnc`, passing msg, *args and **kwargs @@ -153,6 +162,10 @@ class BaseHandler(object): """ return self._log(log.error, msg, *args, **kwargs) + def log_dry_run(self, msg, *args, **kwargs): + """Wrap log.info by adding prefix DRY-RUN: """ + return self._log(log.info, 'DRY-RUN: {}'.format(msg), *args, **kwargs) + @property def current_db_event_id(self): return self._db_event_id diff --git a/freshmaker/handlers/git/module_metadata_change.py b/freshmaker/handlers/git/module_metadata_change.py index 5575df0..4be2b5d 100644 --- a/freshmaker/handlers/git/module_metadata_change.py +++ b/freshmaker/handlers/git/module_metadata_change.py @@ -22,7 +22,7 @@ # Written by Jan Kaluza -from freshmaker import log +from freshmaker import conf, log from freshmaker.types import ArtifactType from freshmaker.handlers import BaseHandler from freshmaker.events import GitModuleMetadataChangeEvent @@ -40,13 +40,28 @@ 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(ArtifactType.MODULE, name=event.module, branch=event.branch): - log.info("Skip rebuild of %s:%s as it's not allowed by configured whitelist", - event.module, event.branch) - return [] - - build_id = self.build_module(event.module, event.branch, event.rev) - if build_id is not None: - self.record_build(event, event.module, ArtifactType.MODULE, build_id) + + if conf.dry_run: + self.log_dry_run('Allow build module: name %s, branch %s', + event.module, event.branch) + else: + 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", + event.module, event.branch) + return [] + + if conf.dry_run: + self.log_dry_run('Build module: (%s, %s, %s)', + event.module, event.branch, event.rev) + build_id = self.build_module(event.module, event.branch, event.rev) + if build_id is not None: + build_id = self._max_build_id + build_id + self._max_build_id = build_id + build = self.record_build(event, event.module, ArtifactType.MODULE, build_id) + self.log_dry_run('Recorded build: %s', build) + else: + build_id = self.build_module(event.module, event.branch, event.rev) + if build_id is not None: + self.record_build(event, event.module, ArtifactType.MODULE, build_id) return [] diff --git a/freshmaker/handlers/git/rpm_spec_change.py b/freshmaker/handlers/git/rpm_spec_change.py index b15b07d..c85bb92 100644 --- a/freshmaker/handlers/git/rpm_spec_change.py +++ b/freshmaker/handlers/git/rpm_spec_change.py @@ -19,12 +19,11 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. -from freshmaker import db, log, conf, utils +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 -from freshmaker.models import ArtifactBuild class GitRPMSpecChangeHandler(BaseHandler): @@ -36,19 +35,10 @@ class GitRPMSpecChangeHandler(BaseHandler): return False - def _init_max_build_id(self): - """Ensure the uniqueness of ArtifactBuild.build_id in dry run mode""" - rows = db.session.query(ArtifactBuild.build_id).filter_by( - type=ArtifactType.MODULE.value).all() - self._max_build_id = max((row[0] for row in rows)) if rows else 0 - def handle(self, event): """ Rebuild module when spec file of rpm in module is updated. """ - if conf.dry_run: - self._init_max_build_id() - rpm = event.rpm branch = event.branch rev = event.rev @@ -65,23 +55,23 @@ class GitRPMSpecChangeHandler(BaseHandler): for module in modules: name = module['variant_name'] version = module['variant_version'] - log.info('DRY-RUN: Allow to build fake module %s', module) - log.info("DRY-RUN: Going to rebuild module '%s:%s'.", name, version) + self.log_dry_run('Allow to build fake module %s', module) + self.log_dry_run("Going to rebuild module '%s:%s'.", name, version) commit_msg = "Bump to rebuild because of %s rpm spec update (%s)." % (rpm, rev) - log.info('DRY-RUN: Bump dist-git repo, modules/%s, branch %s, commit message %s', - name, version, commit_msg) + self.log_dry_run('Bump dist-git repo, modules/%s, branch %s, commit message %s', + name, version, commit_msg) rev = '1234567' - log.info('DRY-RUN: Start to build module %s, version %s, rev %s', - name, version, rev) + self.log_dry_run('Start to build module %s, version %s, rev %s', + name, version, rev) build_id = self.build_module(name, version, rev) if build_id is not None: build_id = self._max_build_id + build_id self._max_build_id = build_id - log.info('DRY-RUN: Record fake MBS module build ID %s', build_id) + self.log_dry_run('Record fake MBS module build ID %s', build_id) build = self.record_build(event, name, ArtifactType.MODULE, build_id) - log.info('DRY-RUN: Recorded build: %s', build) + self.log_dry_run('Recorded build: %s', build) else: for module in modules: name = module['variant_name'] diff --git a/tests/test_handler.py b/tests/test_handler.py index 0c9b3c8..ff5f535 100644 --- a/tests/test_handler.py +++ b/tests/test_handler.py @@ -221,12 +221,13 @@ class TestGetRepoURLs(helpers.ModelsTestCase): class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): """Test BaseHandler.allow_build""" - @patch('freshmaker.handlers.conf') - def test_allow_build_in_whitelist(self, conf): + @patch('freshmaker.handlers.conf.handler_build_whitelist', + new_callable=PropertyMock) + def test_allow_build_in_whitelist(self, handler_build_whitelist): """ Test if artifact is in the handlers whitelist """ whitelist_rules = {"image": [{'name': "test"}]} handler = MyHandler() - conf.handler_build_whitelist.get.return_value = whitelist_rules + handler_build_whitelist.get.return_value = whitelist_rules container = {"name": "test", "branch": "branch"} allow = handler.allow_build(ArtifactType.IMAGE, @@ -234,12 +235,13 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): branch=container["branch"]) assert allow - @patch('freshmaker.handlers.conf') - def test_allow_build_not_in_whitelist(self, conf): + @patch('freshmaker.handlers.conf.handler_build_whitelist', + new_callable=PropertyMock) + def test_allow_build_not_in_whitelist(self, handler_build_whitelist): """ 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 + handler_build_whitelist.get.return_value = whitelist_rules container = {"name": "test", "branch": "branch"} allow = handler.allow_build(ArtifactType.IMAGE, @@ -247,13 +249,13 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): branch=container["branch"]) assert not allow - @patch('freshmaker.handlers.conf') - def test_allow_build_regex_exception(self, conf): + @patch('freshmaker.handlers.conf.handler_build_whitelist') + def test_allow_build_regex_exception(self, handler_build_whitelist): """ 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 + handler_build_whitelist.get.return_value = whitelist_rules container = {"name": "test", "branch": "branch"} with self.assertRaises(UnprocessableEntity): From b0d94ac5250f82dd61ed8bee8503c5f138b37b25 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 13 2018 07:27:00 +0000 Subject: [PATCH 3/4] Do not bypass allow_build check during dry run in GitModuleMetadataChangeHandler Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/git/module_metadata_change.py b/freshmaker/handlers/git/module_metadata_change.py index 4be2b5d..8c865ae 100644 --- a/freshmaker/handlers/git/module_metadata_change.py +++ b/freshmaker/handlers/git/module_metadata_change.py @@ -41,14 +41,10 @@ class GitModuleMetadataChangeHandler(BaseHandler): log.info("Triggering rebuild of module %s:%s, metadata updated (%s).", event.module, event.branch, event.rev) - if conf.dry_run: - self.log_dry_run('Allow build module: name %s, branch %s', - event.module, event.branch) - else: - 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", - event.module, event.branch) - return [] + 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", + event.module, event.branch) + return [] if conf.dry_run: self.log_dry_run('Build module: (%s, %s, %s)', From d0f00b4ecbfeabaeaf99d02c6e485fc23ae88a8a Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Feb 13 2018 07:28:31 +0000 Subject: [PATCH 4/4] Do not set DRY_RUN explicitly in dev configuration section Signed-off-by: Chenxiong Qi --- diff --git a/conf/config.py b/conf/config.py index 45d4695..7fc1c22 100644 --- a/conf/config.py +++ b/conf/config.py @@ -242,8 +242,6 @@ class DevConfiguration(BaseConfiguration): AUTH_BACKEND = 'noauth' AUTH_OPENIDC_USERINFO_URI = 'https://iddev.fedorainfracloud.org/openidc/UserInfo' - DRY_RUN = True - class TestConfiguration(BaseConfiguration): LOG_BACKEND = 'console'