PR#189: Add requests-tests-repo command - fedpkg

fedpkg

#189 Add requests-tests-repo command

Merged 6 years ago by cqi. Opened 6 years ago by mvadkert.

mvadkert/fedpkg issue/176 into master

Add requests-tests-repo command

Miroslav Vadkerti • 6 years ago

34071ac

fedpkg/cli.py

file modified

+85 -23

		`@@ -27,7 +27,8 @@`
		`from fedpkg.bugzilla import BugzillaClient`
		`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)`
		`+ get_pagure_token, is_epel, assert_valid_epel_package,`
		`+ assert_new_tests_repo, get_dist_git_url)`

		`RELEASE_BRANCH_REGEX = r'^(f\d+\|el\d+\|epel\d+)$'`

		`@@ -56,6 +57,7 @@`
		`self.register_retire()`
		`self.register_update()`
		`self.register_request_repo()`
		`+ self.register_request_tests_repo()`
		`self.register_request_branch()`

		`# Target registry goes here`
		`@@ -128,6 +130,42 @@`
		`'process (specifically, it does not require a Bugzilla bug)')`
		`request_repo_parser.set_defaults(command=self.request_repo)`

		`+ def register_request_tests_repo(self):`
		`+ help_msg = 'Request a new tests dist-git repository'`
		`+ pagure_url = urlparse(self.config.get(`
		`+ '{0}.pagure'.format(self.name), 'url')).netloc`
		`+ anongiturl = self.config.get(self.name, 'anongiturl', vars={'module': 'any'})`
		`+ description = '''Request a new dist-git repository in tests shared namespace`
		`+`
		`+ {2}/projects/tests/*`
		`+`
		`+ For more information about tests shared namespace see`
		`+`
		`+ https://fedoraproject.org/wiki/CI/Share_Test_Code`
		`+`
		`+ Please refer to the request-repo command to see what has to be done before`
		`+ requesting a repository in the tests namespace.`
		`+`
		`+ Below is a basic example of the command to request a dist-git repository for`
		`+ the space tests/foo:`
		`+`
		`+ fedpkg --module-name foo request-tests-repo "Description of the repository"`
		`+`
		`+ Note that the space name needs to reflect the intent of the tests and will`
		`+ undergo a manual review.`
		`+`
		`+ '''.format(self.name, pagure_url, get_dist_git_url(anongiturl))`
		`+`
		`+ request_tests_repo_parser = self.subparsers.add_parser(`
		`+ 'request-tests-repo',`
		`+ formatter_class=argparse.RawDescriptionHelpFormatter,`
		`+ help=help_msg,`
		`+ description=description)`
		`+ request_tests_repo_parser.add_argument(`
		`+ 'description',`
		`+ help='Description of the tests repository')`
		`+ request_tests_repo_parser.set_defaults(command=self.request_tests_repo)`
		`+`
		`def register_request_branch(self):`
		`help_msg = 'Request a new dist-git branch'`
		`description = '''Request a new dist-git branch`
		`@@ -319,9 +357,20 @@`
		`config=self.config,`
		`)`

		`+ def request_tests_repo(self):`
		`+ self._request_repo(`
		`+ module_name=self.cmd.module_name,`
		`+ ns='tests',`
		`+ description=self.args.description,`
		`+ name=self.name,`
		`+ config=self.config,`
		`+ anongiturl=self.cmd.anongiturl`
		`+ )`
		`+`
		`@staticmethod`
		`- def _request_repo(module_name, ns, branch, summary, description,`
		`- upstreamurl, monitor, bug, exception, name, config):`
		`+ def _request_repo(module_name, ns, description, name, config, branch=None,`
		`+ summary=None, upstreamurl=None, monitor=None, bug=None,`
		`+ exception=None, anongiturl=None):`
		""" Implementation of `request_repo`.

		`Submits a request for a new dist-git repo.`
		`@@ -330,12 +379,16 @@`
		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 description: A string, the description of the new repo.`
		+ Typically takes the value of `self.args.description`.
		`+ :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`.
		`: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.`
		`@@ -349,16 +402,14 @@`
		`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`.
		`+ :param anongiturl: A string with the name of the anonymous git url.`
		+ Typically the value of `self.cmd.anongiturl`.
		`: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:`
		`+ if not bug and not exception and ns != 'tests':`
		`raise rpkgError(`
		`'A Bugzilla bug is required on new repository requests')`
		`repo_regex = r'^[a-zA-Z0-9_][a-zA-Z0-9-_.+]*$'`
		`@@ -371,24 +422,35 @@`
		`.format(module_name))`

		`summary_from_bug = ''`
		`- if bug:`
		`+ if bug and ns != 'tests':`
		`bz_url = config.get('{0}.bugzilla'.format(name), 'url')`
		`bz_client = BugzillaClient(bz_url)`
		`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': 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 ''`
		`- }`
		`+ if ns == 'tests':`
		`+ # check if tests repository does not exist already`
		`+ assert_new_tests_repo(module_name, get_dist_git_url(anongiturl))`
		`+`
		`+ ticket_body = {`
		`+ 'action': 'new_repo',`
		`+ 'namespace': 'tests',`
		`+ 'repo': module_name,`
		`+ 'description': description,`
		`+ }`
		`+ else:`
		`+ ticket_body = {`
		`+ 'action': 'new_repo',`
		`+ '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)

fedpkg/utils.py

file modified

+36 -1

		`@@ -14,7 +14,7 @@`
		`import json`
		`from datetime import datetime`

		`- from six.moves.urllib.parse import urlencode`
		`+ from six.moves.urllib.parse import urlencode, urlparse`
		`from six.moves.configparser import NoSectionError, NoOptionError`
		`import requests`
		`from requests.exceptions import ConnectionError`
		`@@ -243,3 +243,38 @@`
		`pkg_arches = set(pkg_info['arch'])`
		`if pkg_arches == set(['noarch']) or not (all_arches - pkg_arches):`
		`raise rpkgError(error_msg_two)`
		`+`
		`+`
		`+ def assert_new_tests_repo(name, dist_git_url):`
		`+ """`
		`+ Asserts that the tests repository name is new. Note that the repository name`
		`+ can be any arbitrary string, so just check if the repository already exists.`
		`+`
		`+ :param name: a string with the package name`
		`+ :return: None or rpkgError`
		`+ """`
		`+`
		`+ url = '{0}/tests/{1}'.format(dist_git_url, name)`
		`+ error_msg = (`
		`+ 'The connection to dist-git failed'`
		`+ 'trying to determine if this is a valid new tests '`
		`+ ' repository name.')`
		`+ try:`
		`+ rv = requests.get(url, timeout=60)`
		`+ except ConnectionError as error:`
		`+ error_msg += ' The error was: {0}'.format(str(error))`
		`+ raise rpkgError(error_msg)`
		`+`
		`+ if rv.ok:`
		`+ raise rpkgError("Repository {0} already exists".format(url))`
		`+`
		`+`
		`+ def get_dist_git_url(anongiturl):`
		`+ """`
		`+ Extracts dist-git url from the anongiturl configuration option.`
		+ :param anongiturl: The `anongiturl` configuration option value. Typically
		+ takes the argument of `self.cmd.anongiturl`
		`+ :return: dist-git url string or rpkgError`
		`+ """`
		`+ parsed_url = urlparse(anongiturl)`
		`+ return '{0}://{1}'.format(parsed_url.scheme, parsed_url.netloc)`

test/test_cli.py

file modified

+79

		`@@ -1032,3 +1032,82 @@`
		`'Engineering team.')`
		`with six.assertRaisesRegex(self, rpkgError, expected_error):`
		`cli.request_branch()`
		`+`
		`+`
		`+ class TestRequestTestsRepo(CliTestCase):`
		`+ """Test the request-tests-repo command"""`
		`+`
		`+ def setUp(self):`
		`+ super(TestRequestTestsRepo, self).setUp()`
		`+`
		`+ def tearDown(self):`
		`+ super(TestRequestTestsRepo, self).tearDown()`
		`+`
		`+ def get_cli(self, cli_cmd, name='fedpkg-stage', cfg='fedpkg-stage.conf',`
		`+ user_cfg='fedpkg-user-stage.conf'):`
		`+ with patch('sys.argv', new=cli_cmd):`
		`+ return self.new_cli(name=name, cfg=cfg, user_cfg=user_cfg)`
		`+`
		`+ @patch('requests.post')`
		`+ @patch('requests.get')`
		`+ @patch('sys.stdout', new=StringIO())`
		`+ def test_request_tests_repo(self, mock_request_get, mock_request_post):`
		`+ """Tests request-tests-repo"""`
		`+`
		`+ # mock the request.get from assert_new_tests_repo`
		`+ mock_get_rv = Mock()`
		`+ mock_get_rv.ok = False`
		`+ mock_request_get.return_value = mock_get_rv`
		`+`
		`+ # mock the request.post to Pagure`
		`+ mock_post_rv = Mock()`
		`+ mock_post_rv.ok = True`
		`+ mock_post_rv.json.return_value = {'issue': {'id': 1}}`
		`+ mock_request_post.return_value = mock_post_rv`
		`+`
		`+ cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path,`
		`+ '--module-name', 'foo', 'request-tests-repo',`
		`+ 'Some description']`
		`+ cli = self.get_cli(cli_cmd)`
		`+ cli.request_tests_repo()`
		`+`
		`+ expected_issue_content = {`
		`+ 'action': 'new_repo',`
		`+ 'repo': 'foo',`
		`+ 'namespace': 'tests',`
		`+ 'description': 'Some description'`
		`+ }`
		`+`
		`+ # 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('```'))
cqi commented 6 years ago I don't think leading and trailing '```' should be removed in this test. Instead, the actual issue content, which is sent to Pagure.io, should be asserted.
		`+ 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/1')`
		`+ self.assertEqual(output, expected_output)`
		`+`
		`+ @patch('requests.get')`
		`+ def test_request_tests_repo_exists(self, mock_request_get):`
		`+ """Tests request-tests-repo exception if repo already exists"""`
		`+`
		`+ # mock the request.get from assert_new_tests_repo`
		`+ mock_get_rv = Mock()`
		`+ mock_get_rv.ok = True`
		`+ mock_request_get.return_value = mock_get_rv`
		`+`
		`+ cli_cmd = ['fedpkg-stage', '--path', self.cloned_repo_path,`
		`+ '--module-name', 'foo', 'request-tests-repo',`
		`+ 'Some description']`
		`+ cli = self.get_cli(cli_cmd)`
		`+`
		`+ expected_error = (`
		`+ 'Repository git://pkgs.stg.example.com/tests/foo already exists')`
		`+`
		`+ try:`
		`+ cli.request_tests_repo()`
		`+ assert False, 'rpkgError not raised'`
		`+ except rpkgError as error:`
		`+ self.assertEqual(str(error), expected_error)`

mvadkert commented 6 years ago

This patch adds a command for adding new repository in the tests
namespace. The only required parameter is the module name and the
description of the repository.

An example of issue filed by the new command:

https://stg.pagure.io/releng/fedora-scm-requests/issue/47

Resolves:

https://pagure.io/fedpkg/issue/176

Note:

I did not try to extend the current request-repo command, as the
required parameters for this request are not needed and the request
does not need an bugzilla.

Signed-off-by: Miroslav Vadkerti mvadkert@redhat.com

cqi commented on line 27 of fedpkg/cli.py 6 years ago

There is fedpkg-stage that interacts with stg dist-git. src.fedoraproject.org should be the stg domain name when fedpkg-stage request-tests-repo -h.

https://src.fedoraproject.org/projects/rpms/fedpkg redirects to https://src.fedoraproject.org/rpms/fedpkg. I think the part projects/ is not necessary. But, I'm not sure if this also applies to tests/ repositories.

cqi commented on line 72 of fedpkg/cli.py 6 years ago

I think we can reuse existing _request_repo. Part of the arguments may not be necessary for requesting a tests repository. It should make sense to make them optional so that _request_repo is usable to request either normal or tests repository.

cqi commented on line 97 of fedpkg/cli.py 6 years ago

This should be a good idea. However, instead of requesting from https://src.fedoraproject.org/tests/{0}, is PDC a better source to confirm if the requested repository exists?

cqi commented on line 15 of fedpkg/cli.py 6 years ago

I'm thinking we can also reuse command request-repo with a new command line option for the request of a tests repo. What do you think?

cqi commented on line 59 of fedpkg/cli.py 6 years ago

Should this argument be a bug ID or an arbitrary text for a description?

astepano commented on line 28 of fedpkg/cli.py 6 years ago

Interesting

rebased onto c66ed50e6911885eb6009c7c3dc5dd0b60acf347

6 years ago

mvadkert commented 6 years ago

@cqi I tried to address all your comments. I now user the _request_repo function, to mitigate code duplication. I also try to extract the repository URL from the anongiturl config variable.

cqi commented on line 46 of fedpkg/utils.py 6 years ago

s/rhpkgError/rpkgError/

cqi commented on line 33 of fedpkg/utils.py 6 years ago

'{}' is not supported in Python 2.6. As we have to build and release EL6 package, fedpkg must be able to run with Python 2.6.

cqi commented on line 53 of fedpkg/utils.py 6 years ago

Although it would be good to write such defense code in case something wrong with anongiturl in configuration, self.cmd.anongiturl should be enough. The load_cmd handles it already. We can keep simple here.

cqi commented on line 41 of fedpkg/cli.py 6 years ago

These lines repeats what request-repo mentions. How about just give a message to reference request-repo's help message just like the message of request-branch? This can avoid maintaining same content in two places.

cqi commented on line 60 of fedpkg/cli.py 6 years ago

This still confuses me. Bug ID is not required for requesting a tests repo and from the test below it is just a text not a bug. It looks this help is not correct. Am I right?

cqi commented on line 54 of test/test_cli.py 6 years ago

I don't think leading and trailing '```' should be removed in this test. Instead, the actual issue content, which is sent to Pagure.io, should be asserted.

cqi commented 6 years ago

@mvadkert Thanks. It's much better to reuse _request_repo. Still have some comments.

cqi commented 6 years ago

In addition, there are also some issues detected by Jenkins job.

mvadkert commented 6 years ago

s/rhpkgError/rpkgError/

Fixed. thanks

'{}' is not supported in Python 2.6. As we have to build and release EL6 package, fedpkg must be able to run with Python 2.6.

Fixed on all places

Although it would be good to write such defense code in case something wrong with anongiturl in configuration, self.cmd.anongiturl should be enough. The load_cmd handles it already. We can keep simple here.

Why I did it this way, i.e. reading anongiturl from config, was that I needed it for help of the requests-tests-repo command, and in that time self.cmd.anongiturl is available. Anyway, I changed
the code to use self.cmd.anongiturl in one case, when it is

These lines repeats what request-repo mentions. How about just give a message to reference request-repo's help message just like the message of request-branch? This can avoid maintaining same content in two places.

Right, added :)

I don't think leading and trailing '```' should be removed in this test. Instead, the actual issue content, which is sent to Pagure.io, should be asserted.

This is already done the same way in the tests above actually. I would keep it for consistency if possible.