From cbe6349b5f5264d2f7737595a0a2ce2084221a6f Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Jul 12 2022 23:30:26 +0000 Subject: `request-branch`: detect existing branch Prevent opening an issue for 'fedora-scm-requests' team to create a new branch when the branch already exists. JIRA: RHELCMP-9404 Fixes: #481 Signed-off-by: Ondrej Nosek --- diff --git a/fedpkg/cli.py b/fedpkg/cli.py index 441f22e..d24e83c 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -24,21 +24,21 @@ from datetime import datetime import pkg_resources import six +from pyrpkg import rpkgError +from pyrpkg.cli import cliClient from six.moves import configparser -from six.moves.configparser import NoOptionError, NoSectionError, ConfigParser +from six.moves.configparser import ConfigParser, NoOptionError, NoSectionError from six.moves.urllib_parse import urlparse from fedpkg.bugzilla import BugzillaClient -from fedpkg.completers import (build_arches, list_targets, - fedpkg_packages, distgit_branches) +from fedpkg.completers import (build_arches, distgit_branches, fedpkg_packages, + list_targets) from fedpkg.utils import (assert_new_tests_repo, assert_valid_epel_package, config_get_safely, do_add_remote, do_fork, expand_release, get_dist_git_url, - get_fedora_release_state, get_release_branches, - get_stream_branches, is_epel, new_pagure_issue, - sl_list_to_dict, verify_sls) -from pyrpkg import rpkgError -from pyrpkg.cli import cliClient + get_fedora_release_state, get_pagure_branches, + get_release_branches, get_stream_branches, is_epel, + new_pagure_issue, sl_list_to_dict, verify_sls) RELEASE_BRANCH_REGEX = r'^(f\d+|el\d+|epel\d+)$' LOCAL_PACKAGE_CONFIG = 'package.cfg' @@ -1063,12 +1063,13 @@ class fedpkgClient(cliClient): no_auto_module=self.args.no_auto_module, name=self.name, config=self.config, + anongiturl=self.cmd.anongiturl, ) @staticmethod def _request_branch(logger, service_levels, all_releases, branch, active_branch, repo_name, ns, no_git_branch, - no_auto_module, name, config): + no_auto_module, name, config, anongiturl): """ Implementation of `request_branch`. Submits a request for a new branch of a given dist-git repo. @@ -1094,6 +1095,8 @@ class fedpkgClient(cliClient): used. Typically the value of `self.name`. :param config: A dict containing the configuration, loaded from file. Typically the value of `self.config`. + :param anongiturl: A string with the name of the anonymous git url. + Typically the value of `self.cmd.anongiturl`. :return: None """ @@ -1193,9 +1196,19 @@ class fedpkgClient(cliClient): ticket_title = 'New Branch "{0}" for "{1}/{2}"'.format( b, ns, repo_name) - print(new_pagure_issue( - logger, pagure_url, pagure_token, ticket_title, ticket_body, - name)) + # check whether the requested branch was already created + if b not in get_pagure_branches( + logger=logger, + url=get_dist_git_url(anongiturl), + namespace=ns, + repo_name=repo_name + ): + print(new_pagure_issue( + logger, pagure_url, pagure_token, ticket_title, ticket_body, + name)) + else: + logger.warning('Requested branch "{0}" already exists. ' + 'Skipping the operation.'.format(b)) # For non-standard rpm branch requests, also request a matching new # module repo with a matching branch. @@ -1235,6 +1248,7 @@ class fedpkgClient(cliClient): no_auto_module=True, # Avoid infinite recursion. name=name, config=config, + anongiturl=anongiturl, ) def do_distgit_fork(self): diff --git a/fedpkg/utils.py b/fedpkg/utils.py index 8f19dd6..301bdab 100644 --- a/fedpkg/utils.py +++ b/fedpkg/utils.py @@ -16,12 +16,11 @@ from datetime import datetime import git import requests +from pyrpkg import rpkgError from requests.exceptions import ConnectionError from six.moves.configparser import NoOptionError, NoSectionError from six.moves.urllib.parse import urlencode, urlparse -from pyrpkg import rpkgError - def query_pdc(server_url, endpoint, params, timeout=60): api_url = '{0}/rest_api/v1/{1}/'.format( @@ -234,6 +233,52 @@ def do_add_remote(base_url, remote_base_url, username, repo, repo_name, return True +def get_pagure_branches(logger, url, namespace, repo_name): + """ + Returns list of branches for the repo from Pagure dist-git. + :param logger: A logger object + :param url: a string of the URL to Pagure + :param namespace: a string determines a type of the repository + :param repo_name: a string of the repository name + :return: a list of branches + """ + api_url = '{0}/api/0'.format(url.rstrip('/')) + get_branches_url = '{0}/{1}/{2}/git/branches'.format(api_url, namespace, repo_name) + + headers = { + 'Accept': 'application/json', + 'Content-Type': 'application/json' + } + try: + logger.debug('Pagure API request: {0}'.format(get_branches_url)) + rv = requests.get( + get_branches_url, headers=headers, timeout=60) + except ConnectionError as error: + error_msg = ('The connection to Pagure failed while getting a list ' + 'of branches from Pagure. The error was: {0}'.format( + str(error))) + raise rpkgError(error_msg) + + base_error_msg = ('The following error occurred while getting a list ' + 'of branches from Pagure: {0}') + + try: + # Extract response error text (and debug data) + rv_json = rv.json() + logger.debug("Pagure API response: '{0}'".format(rv_json)) + rv_error = rv_json.get('error') + except (ValueError, AttributeError): + rv_error = rv.text + + if not rv.ok: + # When specific namespace has no branches, API returns error "Project not found". + # Do not fail. Return "no branches found" instead. + logger.debug(base_error_msg.format(rv_error)) + return () + + return rv_json.get("branches", ()) + + def get_release_branches(server_url): """ Get the active Fedora release branches from PDC diff --git a/test/test_cli.py b/test/test_cli.py index 084f4a9..f956dcc 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -913,17 +913,25 @@ class TestRequestBranch(CliTestCase): with patch('sys.argv', new=cli_cmd): return self.new_cli(name=name, cfg=cfg, user_cfg=user_cfg) + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('sys.stdout', new=StringIO()) - def test_request_branch(self, mock_grb, mock_request_post): + def test_request_branch(self, mock_grb, mock_request_post, mock_request_get): """Tests request-branch""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {'issue': {'id': 2}} mock_request_post.return_value = mock_rv + # Checkout the f27 branch self.run_cmd(['git', 'checkout', 'f27'], cwd=self.cloned_repo_path) @@ -932,6 +940,8 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + self.assertEqual(1, mock_request_get.call_count) + expected_issue_content = { 'action': 'new_branch', 'repo': 'testpkg', @@ -949,13 +959,47 @@ class TestRequestBranch(CliTestCase): 'fedora-scm-requests/issue/2') self.assertEqual(output, expected_output) + @patch('requests.get') + @patch('requests.post') + @patch('fedpkg.cli.get_release_branches') + @patch('sys.stdout', new=StringIO()) + def test_request_existing_branch(self, mock_grb, mock_request_post, mock_request_get): + """Tests request-existing-branch""" + mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], + 'epel': ['el6', 'epel7']} + + # branch 'f27' already exists and there is no POST request sent to Pagure (it would create + # an issue) + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "f27", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + + # Checkout the f27 branch + self.run_cmd(['git', 'checkout', 'f27'], cwd=self.cloned_repo_path) + + cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path, + 'request-branch'] + cli = self.get_cli(cli_cmd) + cli.request_branch() + + self.assertEqual(1, mock_request_get.call_count) + mock_request_post.assert_not_called() + + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('sys.stdout', new=StringIO()) - def test_request_branch_override(self, mock_grb, mock_request_post): + def test_request_branch_override(self, mock_grb, mock_request_post, mock_request_get): """Tests request-branch with an overridden package and branch name""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {'issue': {'id': 2}} @@ -966,6 +1010,8 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + self.assertEqual(1, mock_request_get.call_count) + expected_issue_content = { 'action': 'new_branch', 'repo': 'nethack', @@ -1010,6 +1056,8 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + self.assertEqual(2, mock_request_get.call_count) + expected_issue_content = { 'action': 'new_branch', 'repo': 'sudoku', @@ -1040,15 +1088,17 @@ class TestRequestBranch(CliTestCase): """Tests request-epel-branch-override""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7', 'epel8']} - mock_rv = Mock() - mock_rv.ok = True - mock_rv.json.return_value = {'issue': {'id': 2}} - mock_request_post.return_value = mock_rv mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {"arches": [], "packages": {}} mock_request_get.return_value = mock_rv + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {'issue': {'id': 2}} + mock_request_post.return_value = mock_rv + # Checkout the epel7 branch self.run_cmd(['git', 'checkout', 'epel7'], cwd=self.cloned_repo_path) @@ -1077,13 +1127,20 @@ class TestRequestBranch(CliTestCase): 'fedora-scm-requests/issue/2') self.assertEqual(output, expected_output) + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('sys.stdout', new=StringIO()) - def test_request_branch_module(self, mock_grb, mock_request_post): + def test_request_branch_module(self, mock_grb, mock_request_post, mock_request_get): """Tests request-branch for a new module branch""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {'issue': {'id': 2}} @@ -1112,12 +1169,20 @@ class TestRequestBranch(CliTestCase): 'fedora-scm-requests/issue/2') self.assertEqual(output, expected_output) + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') - def assert_request_branch_container(self, cli_cmd, mock_grb, mock_request_post): + def assert_request_branch_container(self, cli_cmd, mock_grb, mock_request_post, + mock_request_get): """Tests request-branch for a new container branch""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {'issue': {'id': 2}} @@ -1157,15 +1222,28 @@ class TestRequestBranch(CliTestCase): '--repo', 'nethack', '--namespace', 'container', 'f27'] self.assert_request_branch_container(cli_cmd) + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('fedpkg.cli.verify_sls') @patch('sys.stdout', new=StringIO()) def test_request_branch_sls(self, mock_verify_sls, mock_grb, - mock_request_post): + mock_request_post, mock_request_get): """Tests request-branch with service levels""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + responses = [] + mock_rv_get = Mock() + mock_rv_get.ok = True + mock_rv_get.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + responses.append(mock_rv_get) + mock_rv_get = Mock() + mock_rv_get.ok = False + mock_rv_get.json.return_value = {'error': 'Project not found', 'error_code': 'ENOPROJECT'} + responses.append(mock_rv_get) + mock_request_get.side_effect = responses + responses = [] for idx in range(2, 5): mock_rv_post = Mock() @@ -1180,6 +1258,7 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + self.assertEqual(2, mock_request_get.call_count) # Get the data that was submitted to Pagure output = sys.stdout.getvalue().strip() # Three bugs are filed. One for the rpm branch, and one for a new @@ -1247,13 +1326,23 @@ class TestRequestBranch(CliTestCase): post_data)['issue_content'].strip('```')) self.assertDictEqual(expected_issue_content, actual_issue_content) + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('sys.stdout', new=StringIO()) - def test_request_branch_all_releases(self, mock_grb, mock_request_post): + def test_request_branch_all_releases(self, mock_grb, mock_request_post, mock_request_get): """Tests request-branch with the '--all-releases' option """ mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + get_side_effect = [] + for i in range(1, 4): + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["main", "rawhide"]} + get_side_effect.append(mock_rv) + mock_request_post.side_effect = get_side_effect + post_side_effect = [] for i in range(1, 4): mock_rv = Mock() @@ -1268,6 +1357,8 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + self.assertEqual(3, mock_request_get.call_count) + for i in range(3): expected_issue_content = { 'action': 'new_branch', @@ -1431,13 +1522,20 @@ https://pagure.stg.example.com/releng/fedora-scm-requests/issue/3""" with six.assertRaisesRegex(self, rpkgError, expected_error): cli.request_branch() + @patch('requests.get') @patch('requests.post') @patch('fedpkg.cli.get_release_branches') @patch('sys.stdout', new=StringIO()) - def test_request_with_repo_option(self, mock_grb, mock_request_post): + def test_request_with_repo_option(self, mock_grb, mock_request_post, mock_request_get): """Test request branch with option --repo""" mock_grb.return_value = {'fedora': ['f25', 'f26', 'f27'], 'epel': ['el6', 'epel7']} + + mock_rv = Mock() + mock_rv.ok = True + mock_rv.json.return_value = {"branches": ["f25", "f26", "main", "rawhide"]} + mock_request_get.return_value = mock_rv + mock_rv = Mock() mock_rv.ok = True mock_rv.json.return_value = {'issue': {'id': 2}}