From 434e666ec1496f8464cef9fd11bd36568153ee58 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Mar 20 2018 10:14:53 +0000 Subject: [PATCH 1/2] Add dry run mode for module build Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/handlers/git/rpm_spec_change.py b/freshmaker/handlers/git/rpm_spec_change.py index 1ce5c5c..861ad5f 100644 --- a/freshmaker/handlers/git/rpm_spec_change.py +++ b/freshmaker/handlers/git/rpm_spec_change.py @@ -19,11 +19,16 @@ # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE # SOFTWARE. +import random +import string + 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.events import MBSModuleStateChangeEvent +from freshmaker.handlers import BaseHandler +from freshmaker.mbs import MBS +from freshmaker.pdc import PDC +from freshmaker.types import ArtifactType class GitRPMSpecChangeHandler(BaseHandler): @@ -51,6 +56,8 @@ class GitRPMSpecChangeHandler(BaseHandler): component_branch=branch, active='true') + result = [] + for module in modules: name = module['name'] version = module['stream'] @@ -65,4 +72,16 @@ class GitRPMSpecChangeHandler(BaseHandler): if build_id is not None: self.record_build(event, name, ArtifactType.MODULE, build_id) - return [] + # While freshmaker is running in dry run mode, no real MBS build is + # requested and scheduled in MBS instance. So, create a module + # state change event manually to allow freshmaker to continue + # handling in MBSModuleStateChangeHandler. + if conf.dry_run: + fake_msg_id = ''.join( + [random.choice(string.hexdigits) for i in range(20)]) + result.append( + MBSModuleStateChangeEvent( + fake_msg_id, + name, version, build_id, MBS.BUILD_STATES['ready'])) + + return result diff --git a/freshmaker/mbs.py b/freshmaker/mbs.py index ccb74ee..78faf3e 100644 --- a/freshmaker/mbs.py +++ b/freshmaker/mbs.py @@ -22,9 +22,12 @@ # Written by Jan Kaluza import requests +import itertools from freshmaker import log +fake_build_id_counter = itertools.count(1) + class MBS(object): BUILD_STATES = { @@ -44,6 +47,7 @@ class MBS(object): self.base_url = config.mbs_base_url self.auth_token = config.mbs_auth_token self.git_base_url = config.git_base_url + self.dry_run = config.dry_run def build_module(self, name, branch, rev): """ @@ -62,6 +66,15 @@ class MBS(object): body = {'scmurl': scm_url, 'branch': branch} url = "%s/module-build-service/1/module-builds/" % self.base_url + if self.dry_run: + log.info( + 'DRY RUN: start to build module. URL: %s Request body: %r', + url, body) + fake_build_id = fake_build_id_counter.next() + log.info('DRY RUN: Triggered build of %s, MBS build_id=%s', + scm_url, fake_build_id) + return fake_build_id + resp = requests.request("POST", url, headers=headers, json=body) data = resp.json() if data['status'] == 409: @@ -75,5 +88,3 @@ class MBS(object): return data['id'] else: log.error("Error when triggering build of %s: %s", scm_url, data) - - return None diff --git a/freshmaker/utils.py b/freshmaker/utils.py index f3191b7..b353b99 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -24,13 +24,15 @@ import contextlib import errno import functools import getpass +import koji import os +import random import shutil +import string import subprocess import sys import tempfile import time -import koji from freshmaker import conf, app, log from freshmaker.types import ArtifactType @@ -166,11 +168,17 @@ def temp_dir(logger=None, *args, **kwargs): def clone_repo(url, dest, branch='master', logger=None, commit=None): cmd = ['git', 'clone', '-b', branch, url, dest] - _run_command(cmd, logger=logger) + if conf.dry_run: + log.info('DRY RUN: clone repo %r', cmd) + else: + _run_command(cmd, logger=logger) if commit: cmd = ['git', 'checkout', commit] - _run_command(cmd, logger=logger, rundir=dest) + if conf.dry_run: + log.info('DRY RUN: %r', cmd) + else: + _run_command(cmd, logger=logger, rundir=dest) return dest @@ -198,14 +206,21 @@ def add_empty_commit(repo, msg="bump", author=None, logger=None): if author is None: author = conf.git_author cmd = ['git', 'commit', '--allow-empty', '-m', msg, '--author={}'.format(author)] - _run_command(cmd, logger=logger, rundir=repo) - return get_commit_hash(repo) + if conf.dry_run: + log.info('DRY RUN: add empty commit %r', cmd) + return ''.join([random.choice(string.hexdigits) for i in range(40)]) + else: + _run_command(cmd, logger=logger, rundir=repo) + return get_commit_hash(repo) def push_repo(repo, logger=None): """Push repo""" cmd = ['git', 'push'] - _run_command(cmd, logger=logger, rundir=repo) + if conf.dry_run: + log.info('DRY RUN: push changes %r', cmd) + else: + _run_command(cmd, logger=logger, rundir=repo) def get_commit_hash(repo, branch='master', revision='HEAD', logger=None): From 1490db7e6b45ce90649f73cb9725e08ae28bb368 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Mar 20 2018 14:17:25 +0000 Subject: [PATCH 2/2] Add tests for some methods in utils.py This patch contains another fix of clone_distgit_repo. Config always has git_user and its default value is an empty string. So, it makes no sense to call hasattr to check if git_user exists. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/utils.py b/freshmaker/utils.py index b353b99..850fda4 100644 --- a/freshmaker/utils.py +++ b/freshmaker/utils.py @@ -188,7 +188,7 @@ def clone_distgit_repo(namespace, name, dest, branch='master', ssh=True, """clone a git repo""" if ssh: if user is None: - if hasattr(conf, 'git_user'): + if conf.git_user: user = conf.git_user else: user = getpass.getuser() @@ -208,10 +208,9 @@ def add_empty_commit(repo, msg="bump", author=None, logger=None): cmd = ['git', 'commit', '--allow-empty', '-m', msg, '--author={}'.format(author)] if conf.dry_run: log.info('DRY RUN: add empty commit %r', cmd) - return ''.join([random.choice(string.hexdigits) for i in range(40)]) else: _run_command(cmd, logger=logger, rundir=repo) - return get_commit_hash(repo) + return get_commit_hash(repo) def push_repo(repo, logger=None): @@ -226,15 +225,23 @@ def push_repo(repo, logger=None): def get_commit_hash(repo, branch='master', revision='HEAD', logger=None): """Get commit hash from revision""" commit_hash = None + fake_commit_hash = ''.join( + [random.choice(string.hexdigits) for i in range(40)]) cmd = ['git', 'rev-parse', revision] if '://' in repo: # this is a remote repo url with temp_dir(prefix='freshmaker-%s-' % repo.split('/').pop()) as repodir: clone_repo(repo, repodir, branch=branch, logger=logger) - commit_hash = _run_command(cmd, rundir=repodir, return_output=True).strip() + if conf.dry_run: + commit_hash = fake_commit_hash + else: + commit_hash = _run_command(cmd, rundir=repodir, return_output=True).strip() else: # repo is local dir - commit_hash = _run_command(cmd, rundir=repo, return_output=True).strip() + if conf.dry_run: + commit_hash = fake_commit_hash + else: + commit_hash = _run_command(cmd, rundir=repo, return_output=True).strip() return commit_hash diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 0000000..d8307d3 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,303 @@ +# -*- 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. +# +# Written by Chenxiong Qi + +import six +import unittest +import errno +from mock import call, patch, Mock + +import freshmaker +from freshmaker import utils + + +class TestMakeDirs(unittest.TestCase): + """Test makedirs""" + + @patch('os.makedirs', side_effect=OSError(errno.EEXIST, '...')) + def test_ignore_error_if_dir_exists(self, makedirs): + try: + utils.makedirs('/some-dir') + except OSError: + self.fail('Should not raise any error from makedirs.') + + @patch('os.makedirs', side_effect=OSError(errno.EPERM, '...')) + def test_raise_error_if_other_os_error_is_raised(self, makedirs): + self.assertRaises(OSError, utils.makedirs, '/some-dir') + + +class TestCloneDistgitRepo(unittest.TestCase): + """Test clone_distgit_repo""" + + @patch.object(freshmaker.conf, 'git_user', new='someone') + @patch.object(freshmaker.conf, 'git_ssh_base_url', new='ssh://%s@localhost/') + @patch('freshmaker.utils.clone_repo') + def test_clone_via_ssh_using_configured_user(self, clone_repo): + utils.clone_distgit_repo('modules', 'testmodule', '/some-destdir') + expected_repo_url = 'ssh://someone@localhost/modules/testmodule' + clone_repo.assert_called_once_with( + expected_repo_url, '/some-destdir', + branch='master', logger=None, commit=None) + + @patch.object(freshmaker.conf, 'git_user', new='') + @patch.object(freshmaker.conf, 'git_ssh_base_url', new='ssh://%s@localhost/') + @patch('freshmaker.utils.clone_repo') + @patch('getpass.getuser', return_value='cqi') + def test_clone_via_ssh_using_user_got_from_system(self, getuser, clone_repo): + utils.clone_distgit_repo('modules', 'testmodule', '/some-destdir') + expected_repo_url = 'ssh://cqi@localhost/modules/testmodule' + clone_repo.assert_called_once_with( + expected_repo_url, '/some-destdir', + branch='master', logger=None, commit=None) + + @patch.object(freshmaker.conf, 'git_base_url', new='https://localhost/') + @patch('freshmaker.utils.clone_repo') + def test_clone_via_anonymous(self, clone_repo): + utils.clone_distgit_repo( + 'modules', 'testmodule', '/some-destdir', ssh=False) + expected_repo_url = 'https://localhost/modules/testmodule' + clone_repo.assert_called_once_with( + expected_repo_url, '/some-destdir', + branch='master', logger=None, commit=None) + + +class TestRunCommand(unittest.TestCase): + """Test _run_command""" + + @patch('subprocess.Popen') + @patch('tempfile.gettempdir') + def test_use_system_tmp_dir_as_rundir(self, gettempdir, Popen): + Popen.return_value.returncode = 0 + Popen.return_value.communicate.return_value = ['stdout', 'stderr'] + utils._run_command(['git', 'status'], rundir=None) + gettempdir.assert_called_once() + + @patch('subprocess.Popen') + def test_log_stdout(self, Popen): + Popen.return_value.returncode = 0 + Popen.return_value.communicate.return_value = ['stdout', 'stderr'] + logger = Mock() + utils._run_command(['git'], rundir='/some-rundir', logger=logger) + logger.debug.assert_has_calls([call('stdout')]) + + @patch('subprocess.Popen') + def test_log_error(self, Popen): + Popen.return_value.returncode = 1 + Popen.return_value.communicate.return_value = ['stdout', 'stderr'] + logger = Mock() + six.assertRaisesRegex( + self, OSError, r'Got an error.+', + utils._run_command, ['git'], rundir='/some-rundir', logger=logger) + logger.error.assert_has_calls([ + call('Got an error from %s', 'git'), + call('stderr'), + ]) + + @patch('subprocess.Popen') + def test_get_output_from_stdout(self, Popen): + Popen.return_value.returncode = 0 + Popen.return_value.communicate.return_value = ['stdout', 'stderr'] + result = utils._run_command( + ['git'], rundir='/some-rundir', return_output=True) + self.assertEqual('stdout', result) + + +class TestBumpDistgitRepo(unittest.TestCase): + """Test bump_distgit_repo""" + + @patch('tempfile.mkdtemp') + @patch('shutil.rmtree') + @patch('freshmaker.utils.clone_distgit_repo') + @patch('freshmaker.utils.add_empty_commit') + @patch('freshmaker.utils.push_repo') + def test_bump_repo(self, push_repo, add_empty_commit, clone_distgit_repo, + rmtree, mkdtemp): + rev = utils.bump_distgit_repo('modules', 'testmodule') + self.assertEqual(add_empty_commit.return_value, rev) + clone_distgit_repo.assert_called_once_with( + 'modules', 'testmodule', mkdtemp.return_value, branch='master', + ssh=True, user=None, logger=None) + add_empty_commit.assert_called_once_with( + mkdtemp.return_value, msg='Bump', author=None, logger=None) + + @patch('freshmaker.utils.clone_distgit_repo', side_effect=ValueError) + def test_fail_to_bump(self, bump_distgit_repo): + logger = Mock() + rev = utils.bump_distgit_repo('modules', 'testmodule', logger=logger) + self.assertIsNone(rev) + logger.error.assert_called_once() + + +class TestCloneRepo(unittest.TestCase): + """Test clone_repo""" + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + def test_dry_run_clone_only(self, run_command): + result = utils.clone_repo('http://remote-gitweb/testmodule.git', + '/some-dest-dir') + self.assertEqual('/some-dest-dir', result) + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + def test_dry_run_both_clone_and_checkout(self, run_command): + result = utils.clone_repo('http://remote-gitweb/testmodule.git', + '/some-dest-dir', + commit='some-commit') + self.assertEqual('/some-dest-dir', result) + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + def test_clone_only(self, run_command): + git_url = 'http://remote-gitweb/testmodule.git' + dest_dir = '/some-dest-dir' + result = utils.clone_repo(git_url, dest_dir) + self.assertEqual(dest_dir, result) + run_command.assert_called_once_with( + ['git', 'clone', '-b', 'master', git_url, dest_dir], + logger=None) + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + def test_both_clone_and_checkout(self, run_command): + git_url = 'http://remote-gitweb/testmodule.git' + dest_dir = '/some-dest-dir' + result = utils.clone_repo(git_url, dest_dir, commit='some-commit') + self.assertEqual(dest_dir, result) + self.assertEqual(2, run_command.call_count) + run_command.assert_has_calls([ + call(['git', 'clone', '-b', 'master', git_url, dest_dir], + logger=None), + call(['git', 'checkout', 'some-commit'], + logger=None, rundir=dest_dir) + ]) + + +class TestAddEmptyCommit(unittest.TestCase): + """Test add_empty_commit""" + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + @patch('freshmaker.utils.get_commit_hash') + def test_dry_run(self, get_commit_hash, run_command): + repo_dir = '/some-repo' + commit_hash = utils.add_empty_commit(repo_dir) + self.assertEqual(get_commit_hash.return_value, commit_hash) + get_commit_hash.assert_called_once_with(repo_dir) + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + @patch('freshmaker.utils.get_commit_hash') + def test_add_emtpy_commit(self, get_commit_hash, run_command): + repo_dir = '/some-repo' + commit_hash = utils.add_empty_commit(repo_dir, author='cqi') + self.assertEqual(get_commit_hash.return_value, commit_hash) + get_commit_hash.assert_called_once_with(repo_dir) + run_command.assert_called_once_with( + ['git', 'commit', '--allow-empty', '-m', 'bump', '--author=cqi'], + logger=None, rundir=repo_dir) + + +class TestPushRepo(unittest.TestCase): + """Test push_repo""" + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + def test_dry_run(self, run_command): + utils.push_repo('/some-repo') + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + def test_push_a_repo(self, run_command): + repo_dir = '/some-repo' + utils.push_repo(repo_dir) + run_command.assert_called_once_with( + ['git', 'push'], logger=None, rundir=repo_dir) + + +class TestGetCommitHash(unittest.TestCase): + """Test get_commit_hash""" + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + @patch('random.choice') + def test_dry_run_from_a_cloned_repo(self, choice, run_command): + choice.return_value = '0' + commit_hash = utils.get_commit_hash('/some-repo') + self.assertEqual(choice.return_value * 40, commit_hash) + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=True) + @patch('freshmaker.utils._run_command') + @patch('freshmaker.utils.clone_repo') + @patch('tempfile.mkdtemp') + @patch('shutil.rmtree') + @patch('random.choice') + def test_dry_run_from_remote_repo( + self, choice, rmtree, mkdtemp, clone_repo, run_command): + choice.return_value = '0' + mkdtemp.return_value = '/some-dir' + git_url = 'http://remote-gitweb/modules/testmodule.git' + + commit_hash = utils.get_commit_hash(git_url) + + self.assertEqual(choice.return_value * 40, commit_hash) + clone_repo.assert_called_once_with( + git_url, mkdtemp.return_value, branch='master', logger=None) + run_command.assert_not_called() + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + def test_get_from_a_cloned_repo(self, run_command): + run_command.return_value = '123456' + + commit_hash = utils.get_commit_hash('/some-repo') + + self.assertEqual(run_command.return_value, commit_hash) + run_command.assert_called_once_with( + ['git', 'rev-parse', 'HEAD'], + rundir='/some-repo', + return_output=True) + + @patch.object(freshmaker.conf, 'dry_run', new=False) + @patch('freshmaker.utils._run_command') + @patch('freshmaker.utils.clone_repo') + @patch('tempfile.mkdtemp') + @patch('shutil.rmtree') + def test_get_from_remote_repo(self, rmtree, mkdtemp, clone_repo, run_command): + run_command.return_value = '123456' + mkdtemp.return_value = '/some-dir' + git_url = 'http://remote-gitweb/modules/testmodule.git' + + commit_hash = utils.get_commit_hash(git_url) + + self.assertEqual(run_command.return_value, commit_hash) + clone_repo.assert_called_once_with( + git_url, mkdtemp.return_value, branch='master', logger=None) + run_command.assert_called_once_with( + ['git', 'rev-parse', 'HEAD'], + rundir=mkdtemp.return_value, + return_output=True)