From 449abbd64936c09e20394f49bdde7a1a1ede5c63 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 16 2017 16:16:20 +0000 Subject: [PATCH 1/4] Improve the error message on error pulling changes of remote pull-request With remote pull-request, the changes are pulled from the remote repo when the PR is accessed, in order to offer to the reviewer, the latest changes. If for some reason the pull errors out, it will raise a GitError. With this PR we catch this exception and return it to the user with an error message, being a little more friendly than we were. Fixes https://pagure.io/pagure/issue/1755 --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 6a8e79a..2eb076d 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -610,10 +610,18 @@ def get_remote_repo_path(remote_git, branch_from, loop=False): repo = pagure.lib.repo.PagureRepo(repopath) try: repo.pull(branch=branch_from, force=True) + except pygit2.GitError as err: # pragma: no-cover + LOG.debug(err) + LOG.exception(err) + flask.abort( + 500, + 'The following error was raised when trying to pull the ' + 'changes from the remote: %s' % str(err) + ) except pagure.exceptions.PagureException as err: LOG.debug(err) LOG.exception(err) - flask.abort(500, err.message) + flask.abort(500, str(err)) return repopath From 85facfb4c001bbf68ad8da25259abbe57455aa5c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 16 2017 16:16:20 +0000 Subject: [PATCH 2/4] Improve the error returned when cloning a remote git repo fails and logging We used to log the errors twice, at as debug and once as exception. @jcline pointed it out in the review, so here it is :) --- diff --git a/pagure/__init__.py b/pagure/__init__.py index 2eb076d..5aa0768 100644 --- a/pagure/__init__.py +++ b/pagure/__init__.py @@ -603,15 +603,17 @@ def get_remote_repo_path(remote_git, branch_from, loop=False): pygit2.clone_repository( remote_git, repopath, checkout_branch=branch_from) except Exception as err: - LOG.debug(err) LOG.exception(err) - flask.abort(500, 'Could not clone the remote git repository') + flask.abort( + 500, + 'The following error was raised when trying to clone the ' + 'remote repo: %s' % str(err) + ) else: repo = pagure.lib.repo.PagureRepo(repopath) try: repo.pull(branch=branch_from, force=True) - except pygit2.GitError as err: # pragma: no-cover - LOG.debug(err) + except pygit2.GitError as err: LOG.exception(err) flask.abort( 500, @@ -619,7 +621,6 @@ def get_remote_repo_path(remote_git, branch_from, loop=False): 'changes from the remote: %s' % str(err) ) except pagure.exceptions.PagureException as err: - LOG.debug(err) LOG.exception(err) flask.abort(500, str(err)) From 17b6f6272c15739d5fb16f60e84332fe7c1f5234 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 16 2017 16:16:20 +0000 Subject: [PATCH 3/4] Add unit-tests checking the get_remote_repo_path method in pagure --- diff --git a/tests/__init__.py b/tests/__init__.py index 14605fa..e1af9ae 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -149,7 +149,7 @@ class Modeltests(unittest.TestCase): # Clean up eventual git repo left in the present folder. self.path = tempfile.mkdtemp(prefix='pagure-tests') for folder in ['tickets', 'repos', 'forks', 'docs', 'requests', - 'releases']: + 'releases', 'remotes']: os.mkdir(os.path.join(self.path, folder)) self.session = pagure.lib.model.create_tables( diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py new file mode 100644 index 0000000..b20b91e --- /dev/null +++ b/tests/test_pagure_flask.py @@ -0,0 +1,85 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2017 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import unittest +import shutil +import sys +import os + +import mock +import pygit2 +import werkzeug + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib +import pagure.lib.model +import tests + +class PagureGetRemoteRepoPath(tests.Modeltests): + """ Tests for pagure """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureGetRemoteRepoPath, self).setUp() + + pagure.APP.config['GIT_FOLDER'] = os.path.join(self.path, 'repos') + pagure.APP.config['REMOTE_GIT_FOLDER'] = os.path.join( + self.path, 'remotes') + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + tests.add_content_git_repo(os.path.join(self.path, 'repos', 'test2.git')) + + def test_failed_clone(self): + """ Test get_remote_repo_path in pagure. """ + with self.assertRaises(werkzeug.exceptions.InternalServerError) as cm: + pagure.get_remote_repo_path('remote_repo', 'branch') + + self.assertEqual( + cm.exception.get_description(), + '

The following error was raised when trying to clone the ' + 'remote repo: Unsupported URL protocol

') + + @mock.patch( + 'pagure.lib.repo.PagureRepo.pull', + mock.MagicMock(side_effect=pygit2.GitError)) + def test_failed_pull(self): + """ Test get_remote_repo_path in pagure. """ + pagure.get_remote_repo_path( + os.path.join(self.path, 'repos', 'test2.git'), 'master') + + with self.assertRaises(werkzeug.exceptions.InternalServerError) as cm: + pagure.get_remote_repo_path( + os.path.join(self.path, 'repos', 'test2.git'), 'master') + + self.assertEqual( + cm.exception.get_description(), + '

The following error was raised when trying to pull the ' + 'changes from the remote:

') + + @mock.patch( + 'pagure.lib.repo.PagureRepo.pull', + mock.MagicMock(side_effect=pygit2.GitError)) + def test_passing(self): + """ Test get_remote_repo_path in pagure. """ + output = pagure.get_remote_repo_path( + os.path.join(self.path, 'repos', 'test2.git'), 'master') + + self.assertTrue(output.endswith('repos_test2.git_master')) + + +if __name__ == '__main__': + SUITE = unittest.TestLoader().loadTestsFromTestCase( + PagureGetRemoteRepoPath) + unittest.TextTestRunner(verbosity=2).run(SUITE) From 78e6f40e9c558eefdf51fa09a20ea2161bcdb375 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Jan 16 2017 16:16:20 +0000 Subject: [PATCH 4/4] Adjust how are called the unit-tests if the file is run in itself --- diff --git a/tests/test_pagure_flask.py b/tests/test_pagure_flask.py index b20b91e..759d4b6 100644 --- a/tests/test_pagure_flask.py +++ b/tests/test_pagure_flask.py @@ -80,6 +80,4 @@ class PagureGetRemoteRepoPath(tests.Modeltests): if __name__ == '__main__': - SUITE = unittest.TestLoader().loadTestsFromTestCase( - PagureGetRemoteRepoPath) - unittest.TextTestRunner(verbosity=2).run(SUITE) + unittest.main(verbosity=2)