From 56080f9d0e53602b65638401faa79840cfa9feb7 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Feb 02 2018 13:59:03 +0000 Subject: [PATCH 1/5] Refactor: parameterize the request_repo and request_branch functionality. Signed-off-by: Ralph Bean --- diff --git a/fedpkg/cli.py b/fedpkg/cli.py index 8f3540b..f0afc33 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -29,6 +29,8 @@ from fedpkg.utils import ( get_release_branches, sl_list_to_dict, verify_sls, new_pagure_issue, get_pagure_token, is_epel, assert_valid_epel_package) +RELEASE_BRANCH_REGEX = r'^(f\d+|el\d+|epel\d+)$' + class fedpkgClient(cliClient): def __init__(self, config, name=None): @@ -297,77 +299,107 @@ suggest_reboot=False os.unlink('clog') def request_repo(self): + self._request_repo( + module_name=self.cmd.module_name, + ns=self.cmd.ns, + branch='master', + summary=self.args.summary, + description=self.args.description, + upstreamurl=self.args.upstreamurl, + monitor=self.args.monitor, + bug=self.args.bug, + exception=self.args.exception, + name=self.name, + config=self.config, + ) + + @staticmethod + def _request_repo(module_name, ns, branch, summary, description, + upstreamurl, monitor, bug, exception, name, config): # bug is not a required parameter in the event the packager has an # exception, in which case, they may use the --exception flag - if not self.args.bug and not self.args.exception: + if not bug and not exception: raise rpkgError( 'A Bugzilla bug is required on new repository requests') repo_regex = r'^[a-zA-Z0-9_][a-zA-Z0-9-_.+]*$' - if not bool(re.match(repo_regex, self.cmd.module_name)): + if not bool(re.match(repo_regex, module_name)): raise rpkgError( 'The repository name "{0}" is invalid. It must be at least ' 'two characters long with only letters, numbers, hyphens, ' 'underscores, plus signs, and/or periods. Please note that ' 'the project cannot start with a period or a plus sign.' - .format(self.cmd.module_name)) + .format(module_name)) summary_from_bug = '' - if self.args.bug: - bz_url = self.config.get('{0}.bugzilla'.format(self.name), 'url') + if bug: + bz_url = config.get('{0}.bugzilla'.format(name), 'url') bz_client = BugzillaClient(bz_url) - bug = bz_client.get_review_bug( - self.args.bug, self.cmd.ns, self.cmd.module_name) - summary_from_bug = bug.summary.split(' - ', 1)[1].strip() + bug_obj = bz_client.get_review_bug(bug, ns, module_name) + summary_from_bug = bug_obj.summary.split(' - ', 1)[1].strip() ticket_body = { 'action': 'new_repo', - 'branch': 'master', - 'bug_id': self.args.bug or '', - 'description': self.args.description or '', - 'exception': self.args.exception, - 'monitor': self.args.monitor, - 'namespace': self.cmd.ns, - 'repo': self.cmd.module_name, - 'summary': self.args.summary or summary_from_bug, - 'upstreamurl': self.args.upstreamurl or '' + 'branch': branch, + 'bug_id': bug or '', + 'description': description or '', + 'exception': exception, + 'monitor': monitor, + 'namespace': ns, + 'repo': module_name, + 'summary': summary or summary_from_bug, + 'upstreamurl': upstreamurl or '' } ticket_body = json.dumps(ticket_body, indent=True) ticket_body = '```\n{0}\n```'.format(ticket_body) - ticket_title = 'New Repo for "{0}/{1}"'.format( - self.cmd.ns, self.cmd.module_name) + ticket_title = 'New Repo for "{0}/{1}"'.format(ns, module_name) - pagure_url = self.config.get('{0}.pagure'.format(self.name), 'url') - pagure_token = get_pagure_token(self.config, self.name) + pagure_url = config.get('{0}.pagure'.format(name), 'url') + pagure_token = get_pagure_token(config, name) print(new_pagure_issue( pagure_url, pagure_token, ticket_title, ticket_body)) def request_branch(self): - service_levels = self.args.sl - branch = None + try: + active_branch = self.cmd.repo.active_branch.name + except rpkgError: + active_branch = None + self._request_branch( + service_levels=self.args.sl, + all_releases=self.args.all_releases, + branch=self.args.branch, + active_branch=active_branch, + module_name=self.cmd.module_name, + ns=self.cmd.ns, + no_git_branch=self.args.no_git_branch, + name=self.name, + config=self.config, + ) - if self.args.all_releases: - if self.args.branch: + @staticmethod + def _request_branch(service_levels, all_releases, branch, active_branch, + module_name, ns, no_git_branch, + name, config): + if all_releases: + if branch: raise rpkgError('You cannot specify a branch with the ' '"--all-releases" option') elif service_levels: raise rpkgError('You cannot specify service levels with the ' '"--all-releases" option') - elif not self.args.branch: - try: - branch = self.cmd.repo.active_branch.name - except rpkgError: + elif not branch: + if active_branch: + branch = active_branch + else: raise rpkgError('You must specify a branch if you are not in ' 'a git repository') - else: - branch = self.args.branch - bodhi_url = self.config.get('{0}.bodhi'.format(self.name), 'url') + bodhi_url = config.get('{0}.bodhi'.format(name), 'url') if branch: if is_epel(branch): - assert_valid_epel_package(self.cmd.module_name, branch) + assert_valid_epel_package(module_name, branch) - if self.cmd.ns in ['modules', 'test-modules']: + if ns in ['modules', 'test-modules']: branch_valid = bool(re.match(r'^[a-zA-Z0-9.\-_+]+$', branch)) if not branch_valid: raise rpkgError( @@ -380,22 +412,22 @@ suggest_reboot=False raise rpkgError( 'You can\'t provide SLs for release branches') else: - if re.match(r'^(f\d+|el\d+|epel\d+)$', branch): + if re.match(RELEASE_BRANCH_REGEX, branch): raise rpkgError('{0} is not a current release branch' .format(branch)) elif not service_levels: raise rpkgError( - 'You must provide SLs for non-release branches') + 'You must provide SLs for non-release branches (%s)' % branch) # If service levels were provided, verify them if service_levels: - pdc_url = self.config.get('{0}.pdc'.format(self.name), 'url') + pdc_url = config.get('{0}.pdc'.format(name), 'url') sl_dict = sl_list_to_dict(service_levels) verify_sls(pdc_url, sl_dict) - pagure_url = self.config.get('{0}.pagure'.format(self.name), 'url') - pagure_token = get_pagure_token(self.config, self.name) - if self.args.all_releases: + pagure_url = config.get('{0}.pagure'.format(name), 'url') + pagure_token = get_pagure_token(config, name) + if all_releases: release_branches = get_release_branches(bodhi_url) branches = [b for b in release_branches if re.match(r'^(f\d+)$', b)] @@ -406,9 +438,9 @@ suggest_reboot=False ticket_body = { 'action': 'new_branch', 'branch': b, - 'namespace': self.cmd.ns, - 'repo': self.cmd.module_name, - 'create_git_branch': not self.args.no_git_branch + 'namespace': ns, + 'repo': module_name, + 'create_git_branch': not no_git_branch } if service_levels: ticket_body['sls'] = sl_dict @@ -416,7 +448,7 @@ suggest_reboot=False ticket_body = json.dumps(ticket_body, indent=True) ticket_body = '```\n{0}\n```'.format(ticket_body) ticket_title = 'New Branch "{0}" for "{1}/{2}"'.format( - b, self.cmd.ns, self.cmd.module_name) + b, ns, module_name) print(new_pagure_issue( pagure_url, pagure_token, ticket_title, ticket_body)) From 02fbcd947210f02808ea5cc726b37d59c2a696ed Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Feb 02 2018 13:59:03 +0000 Subject: [PATCH 2/5] Automatically request module for non-standard branches. This is half of fedrepo_req#129. Whenever a user requests a non-standard branch, we will also submit a request for a matching module with matching stream and matching SLs. In a subsequent changeset, we want to make the admin tool automatically populate new module repos with a useful modulemd starter template. Signed-off-by: Ralph Bean --- diff --git a/fedpkg/cli.py b/fedpkg/cli.py index f0afc33..963de82 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -166,6 +166,12 @@ and created: help='Don\'t create the branch in git but still create it in PDC' ) request_branch_parser.add_argument( + '--no-auto-module', default=False, action='store_true', + help='If requesting an rpm arbitrary branch, do not ' + 'also request a new matching module. See ' + 'https://pagure.io/fedrepo_req/issue/129' + ) + request_branch_parser.add_argument( '--all-releases', default=False, action='store_true', help='Make a new branch request for every active Fedora release' ) @@ -372,13 +378,14 @@ suggest_reboot=False module_name=self.cmd.module_name, ns=self.cmd.ns, no_git_branch=self.args.no_git_branch, + no_auto_module=self.args.no_auto_module, name=self.name, config=self.config, ) @staticmethod def _request_branch(service_levels, all_releases, branch, active_branch, - module_name, ns, no_git_branch, + module_name, ns, no_git_branch, no_auto_module, name, config): if all_releases: if branch: @@ -452,3 +459,39 @@ suggest_reboot=False print(new_pagure_issue( pagure_url, pagure_token, ticket_title, ticket_body)) + + # For non-standard rpm branch requests, also request a matching new + # module repo with a matching branch. + auto_module = ( + ns == 'rpms' + and not re.match(RELEASE_BRANCH_REGEX, b) + and not no_auto_module + ) + if auto_module: + summary = ('Automatically requested module for ' + 'rpms/%s:%s.' % (module_name, b)) + fedpkgClient._request_repo( + module_name=module_name, + ns='modules', + branch='master', + summary=summary, + description=summary, + upstreamurl=None, + monitor='no-monitoring', + bug=None, + exception=True, + name=name, + config=config, + ) + fedpkgClient._request_branch( + service_levels=service_levels, + all_releases=all_releases, + branch=b, + active_branch=active_branch, + module_name=module_name, + ns='modules', + no_git_branch=no_git_branch, + no_auto_module=True, # Avoid infinite recursion. + name=name, + config=config, + ) diff --git a/test/test_cli.py b/test/test_cli.py index c3e0a73..fa9a0d2 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -798,6 +798,21 @@ class TestRequestBranch(CliTestCase): cli = self.get_cli(cli_cmd) cli.request_branch() + # 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 + # module repo, and one for the matching module branch. + expected_output = ( + 'https://pagure.stg.example.com/releng/' + 'fedora-scm-requests/issue/2\n' + 'https://pagure.stg.example.com/releng/' + 'fedora-scm-requests/issue/2\n' + 'https://pagure.stg.example.com/releng/' + 'fedora-scm-requests/issue/2' + ) + self.assertMultiLineEqual(output, expected_output) + + # Check for rpm branch.. expected_issue_content = { 'action': 'new_branch', 'repo': 'nethack', @@ -809,15 +824,46 @@ class TestRequestBranch(CliTestCase): 'bug_fixes': '2030-12-01' } } - # Get the data that was submitted to Pagure post_data = mock_request_post.call_args_list[0][1]['data'] actual_issue_content = json.loads(json.loads( post_data)['issue_content'].strip('```')) - self.assertEqual(expected_issue_content, actual_issue_content) - output = sys.stdout.getvalue().strip() - expected_output = ('https://pagure.stg.example.com/releng/' - 'fedora-scm-requests/issue/2') - self.assertEqual(output, expected_output) + self.assertDictEqual(expected_issue_content, actual_issue_content) + + # Check for the module repo request.. + summary = u'Automatically requested module for rpms/nethack:9.' + expected_issue_content = { + u'action': u'new_repo', + u'branch': u'master', + u'bug_id': u'', + u'description': summary, + u'exception': True, + u'monitor': u'no-monitoring', + u'namespace': u'modules', + u'repo': u'nethack', + u'summary': summary, + u'upstreamurl': u'' + } + post_data = mock_request_post.call_args_list[1][1]['data'] + actual_issue_content = json.loads(json.loads( + post_data)['issue_content'].strip('```')) + self.assertDictEqual(expected_issue_content, actual_issue_content) + + # Check for module branch.. + expected_issue_content = { + 'action': 'new_branch', + 'repo': 'nethack', + 'namespace': 'modules', + 'branch': '9', + 'create_git_branch': True, + 'sls': { + 'security_fixes': '2030-12-01', + 'bug_fixes': '2030-12-01' + } + } + post_data = mock_request_post.call_args_list[2][1]['data'] + actual_issue_content = json.loads(json.loads( + post_data)['issue_content'].strip('```')) + self.assertDictEqual(expected_issue_content, actual_issue_content) @patch('requests.post') @patch('fedpkg.cli.get_release_branches') From c61cc3e7933d847a4f8d396f38c1bdb3b6722d24 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Feb 02 2018 15:38:41 +0000 Subject: [PATCH 3/5] Add docstrings. Signed-off-by: Ralph Bean --- diff --git a/fedpkg/cli.py b/fedpkg/cli.py index 963de82..2db11d6 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -322,6 +322,40 @@ suggest_reboot=False @staticmethod def _request_repo(module_name, ns, branch, summary, description, upstreamurl, monitor, bug, exception, name, config): + """ Implementation of `request_repo`. + + Submits a request for a new dist-git repo. + + :param module_name: The repository name string. Typically the + value of `self.cmd.module_name`. + :param ns: The repository namespace string, i.e. 'rpms' or 'modules'. + Typically takes the value of `self.cmd.ns`. + :param branch: The git branch string when requesting a repo. + Typically 'master'. + :param summary: A string, the summary of the new repo. Typically + takes the value of `self.args.summary`. + :param description: A string, the description of the new repo. + Typically takes the value of `self.args.description`. + :param upstreamurl: A string, the upstreamurl of the new repo. + Typically takes the value of `self.args.upstreamurl`. + :param monitor: A string, the monitoring flag of the new repo, i.e. + `'no-monitoring'`, `'monitoring'`, or `'monitoring-with-scratch'`. + Typically takes the value of `self.args.monitor`. + :param bug: An integer representing the bugzilla ID of a "package + review" associated with this new repo. Typically takes the + value of `self.args.bug`. + :param exception: An boolean specifying whether or not this request is + an exception to the packaging policy. Exceptional requests may be + granted the right to waive their package review at the discretion of + Release Engineering. Typically takes the value of + `self.args.exception`. + :param name: A string representing which section of the config should be + used. Typically the value of `self.name`. + :param config: A dict containing the configuration, loaded from file. + Typically the value of `self.config`. + :return: None + """ + # bug is not a required parameter in the event the packager has an # exception, in which case, they may use the --exception flag if not bug and not exception: @@ -387,6 +421,33 @@ suggest_reboot=False def _request_branch(service_levels, all_releases, branch, active_branch, module_name, ns, no_git_branch, no_auto_module, name, config): + """ Implementation of `request_branch`. + + Submits a request for a new branch of a given dist-git repo. + + :param service_levels: A list of service level strings. Typically the + value of `self.args.service_levels`. + :param all_releases: A boolean indicating if this request should be made + for all active Fedora branches. + :param branch: A string specifying the specific branch to be requested. + :param active_branch: A string (or None) specifying the active branch in + the current git repo (the branch that is currently checked out). + :param module_name: The repository name string. Typically the + value of `self.cmd.module_name`. + :param ns: The repository namespace string, i.e. 'rpms' or 'modules'. + Typically takes the value of `self.cmd.ns`. + :param no_git_branch: A boolean flag. If True, the SCM admins should + create the git branch in PDC, but not in pagure.io. + :param no_auto_module: A boolean flag. If True, requests for + non-standard branches should not automatically result in additional + requests for matching modules. + :param name: A string representing which section of the config should be + used. Typically the value of `self.name`. + :param config: A dict containing the configuration, loaded from file. + Typically the value of `self.config`. + :return: None + """ + if all_releases: if branch: raise rpkgError('You cannot specify a branch with the ' From 81e738854bef9fcb7b6efe2c07eb6ca1f9a48296 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Feb 02 2018 15:38:50 +0000 Subject: [PATCH 4/5] Typofix. Signed-off-by: Ralph Bean --- diff --git a/fedpkg/cli.py b/fedpkg/cli.py index 2db11d6..a203b91 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -481,7 +481,7 @@ suggest_reboot=False 'You can\'t provide SLs for release branches') else: if re.match(RELEASE_BRANCH_REGEX, branch): - raise rpkgError('{0} is not a current release branch' + raise rpkgError('{0} is a current release branch' .format(branch)) elif not service_levels: raise rpkgError( From 0b2b9656af4e058de598833023a6efa45a1c0957 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Feb 02 2018 15:38:56 +0000 Subject: [PATCH 5/5] Better mocking. Return different values for each new request. Signed-off-by: Ralph Bean --- diff --git a/test/test_cli.py b/test/test_cli.py index fa9a0d2..a23f8e1 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -787,10 +787,13 @@ class TestRequestBranch(CliTestCase): mock_request_post): """Tests request-branch with service levels""" mock_grb.return_value = set(['el6', 'epel7', 'f25', 'f26', 'f27']) - mock_rv_post = Mock() - mock_rv_post.ok = True - mock_rv_post.json.return_value = {'issue': {'id': 2}} - mock_request_post.return_value = mock_rv_post + responses = [] + for idx in range(2, 5): + mock_rv_post = Mock() + mock_rv_post.ok = True + mock_rv_post.json.return_value = {'issue': {'id': idx}} + responses.append(mock_rv_post) + mock_request_post.side_effect = responses cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path, '--module-name', 'nethack', 'request-branch', '9', '--sl', @@ -806,9 +809,9 @@ class TestRequestBranch(CliTestCase): 'https://pagure.stg.example.com/releng/' 'fedora-scm-requests/issue/2\n' 'https://pagure.stg.example.com/releng/' - 'fedora-scm-requests/issue/2\n' + 'fedora-scm-requests/issue/3\n' 'https://pagure.stg.example.com/releng/' - 'fedora-scm-requests/issue/2' + 'fedora-scm-requests/issue/4' ) self.assertMultiLineEqual(output, expected_output)