From dcfdb9f532be3a05971982726d70d357bd2b7760 Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Mar 30 2020 02:18:02 +0000 Subject: Check missing config options more reliably User's config file is not always updated with freshly added config options in fedpkg release. The reason could be the user's own changes in the config file prior to the upgrade. It needs user's action and this fix provides a hint when this problem is detected. Resolves: rhbz#1813338 JIRA: COMPOSE-4228 Signed-off-by: Ondrej Nosek --- diff --git a/conf/etc/rpkg/fedpkg.conf b/conf/etc/rpkg/fedpkg.conf index 0809aef..e3089bc 100644 --- a/conf/etc/rpkg/fedpkg.conf +++ b/conf/etc/rpkg/fedpkg.conf @@ -69,6 +69,7 @@ url = https://bugzilla.redhat.com/ [fedpkg.pagure] url = https://pagure.io/ +token = [fedpkg.pdc] url = https://pdc.fedoraproject.org/ diff --git a/fedpkg/cli.py b/fedpkg/cli.py index c43fcff..452026b 100644 --- a/fedpkg/cli.py +++ b/fedpkg/cli.py @@ -30,11 +30,10 @@ from six.moves.urllib_parse import urlparse from fedpkg.bugzilla import BugzillaClient from fedpkg.utils import (assert_new_tests_repo, assert_valid_epel_package, - do_fork, expand_release, get_dist_git_url, - get_distgit_token, get_fedora_release_state, - get_pagure_token, get_release_branches, - get_stream_branches, is_epel, new_pagure_issue, - sl_list_to_dict, verify_sls) + config_get_safely, 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 @@ -256,6 +255,9 @@ class fedpkgClient(cliClient): def register_request_repo(self): help_msg = 'Request a new dist-git repository' + pagure_section = '{0}.pagure'.format(self.name) + pagure_url = config_get_safely(self.config, pagure_section, 'url') + pagure_url_parsed = urlparse(pagure_url).netloc description = textwrap.dedent(''' Request a new dist-git repository @@ -275,8 +277,7 @@ class fedpkgClient(cliClient): Another example to request a module foo: fedpkg request-repo --namespace modules foo - '''.format(self.name, urlparse(self.config.get( - '{0}.pagure'.format(self.name), 'url')).netloc)) + '''.format(self.name, pagure_url_parsed)) request_repo_parser = self.subparsers.add_parser( 'request-repo', @@ -322,8 +323,9 @@ class fedpkgClient(cliClient): 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 + pagure_section = '{0}.pagure'.format(self.name) + pagure_url = config_get_safely(self.config, pagure_section, 'url') + pagure_url_parsed = urlparse(pagure_url).netloc anongiturl = self.config.get( self.name, 'anongiturl', vars={'repo': 'any', 'module': 'any'} ) @@ -346,7 +348,7 @@ class fedpkgClient(cliClient): 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))) + '''.format(self.name, pagure_url_parsed, get_dist_git_url(anongiturl))) request_tests_repo_parser = self.subparsers.add_parser( 'request-tests-repo', @@ -441,6 +443,8 @@ class fedpkgClient(cliClient): def register_do_fork(self): help_msg = 'Create a new fork of the current repository' + distgit_section = '{0}.distgit'.format(self.name) + distgit_api_base_url = config_get_safely(self.config, distgit_section, "apibaseurl") description = textwrap.dedent(''' Create a new fork of the current repository @@ -460,8 +464,7 @@ class fedpkgClient(cliClient): username is taken. It could be overridden by reusing an argument: {0} --user FAS_ID fork - '''.format(self.name, urlparse(self.config.get( - '{0}.distgit'.format(self.name), 'apibaseurl')).netloc)) + '''.format(self.name, urlparse(distgit_api_base_url).netloc)) fork_parser = self.subparsers.add_parser( 'fork', @@ -905,8 +908,9 @@ class fedpkgClient(cliClient): ticket_body = '```\n{0}\n```'.format(ticket_body) ticket_title = 'New Repo for "{0}/{1}"'.format(ns, repo_name) - pagure_url = config.get('{0}.pagure'.format(name), 'url') - pagure_token = get_pagure_token(config, name) + pagure_section = '{0}.pagure'.format(name) + pagure_url = config_get_safely(config, pagure_section, 'url') + pagure_token = config_get_safely(config, pagure_section, 'token') print(new_pagure_issue( pagure_url, pagure_token, ticket_title, ticket_body, name)) @@ -1024,8 +1028,9 @@ class fedpkgClient(cliClient): sl_dict = sl_list_to_dict(service_levels) verify_sls(pdc_url, sl_dict) - pagure_url = config.get('{0}.pagure'.format(name), 'url') - pagure_token = get_pagure_token(config, name) + pagure_section = '{0}.pagure'.format(name) + pagure_url = config_get_safely(config, pagure_section, 'url') + pagure_token = config_get_safely(config, pagure_section, 'token') if all_releases: release_branches = list(itertools.chain( *list(get_release_branches(pdc_url).values()))) @@ -1097,13 +1102,14 @@ class fedpkgClient(cliClient): def do_distgit_fork(self): """create fork of the distgit repository""" - distgit_api_base_url = self.config.get('{0}.distgit'.format(self.name), "apibaseurl") + distgit_section = '{0}.distgit'.format(self.name) + distgit_api_base_url = config_get_safely(self.config, distgit_section, "apibaseurl") distgit_remote_base_url = self.config.get( '{0}'.format(self.name), "gitbaseurl", vars={'user': 'any', 'repo': 'any'}, ) - distgit_token = get_distgit_token(self.config, self.name) + distgit_token = config_get_safely(self.config, distgit_section, 'token') fork_url = do_fork( base_url=distgit_api_base_url, diff --git a/fedpkg/utils.py b/fedpkg/utils.py index 3a1c04d..b639c4f 100644 --- a/fedpkg/utils.py +++ b/fedpkg/utils.py @@ -292,38 +292,6 @@ def verify_sls(pdc_url, sl_dict): raise rpkgError('The SL "{0}" is not in PDC'.format(sl)) -def get_pagure_token(config, cli_name): - """ - Gets the Pagure token configured in the user's configuration file - :param config: ConfigParser object - :param cli_name: string of the CLI's name (e.g. fedpkg) - :return: string of the Pagure token - """ - conf_section = '{0}.pagure'.format(cli_name) - try: - return config.get(conf_section, 'token') - except (NoSectionError, NoOptionError): - raise rpkgError( - 'Missing a Pagure token. Refer to the help of the current command ' - '(-h/--help)" to set a token in your user configuration.') - - -def get_distgit_token(config, cli_name): - """ - Gets the distgit token configured in the user's configuration file - :param config: ConfigParser object - :param cli_name: string of the CLI's name (e.g. fedpkg) - :return: string of the distgit token - """ - conf_section = '{0}.distgit'.format(cli_name) - try: - return config.get(conf_section, 'token') - except (NoSectionError, NoOptionError): - raise rpkgError( - 'Missing a distgit token. Refer to the help of the current command ' - '(-h/--help)" to set a token in your user configuration.') - - def is_epel(branch): """ Determines if this is or will be an epel branch @@ -524,3 +492,43 @@ def get_fedora_release_state(config, cli_name, release): raise rpkgError(base_error_msg.format(rv.text)) return rv.json().get('state') + + +def config_get_safely(config, section, option): + """ + Returns option from the user's configuration file. In case of missing + section or option method throws an exception with a human-readable + warning and a possible hint. + The method should be used especially in situations when there are newly + added sections/options into the config. In this case, there is a risk that + the user's config wasn't properly upgraded. + + :param config: ConfigParser object + :param section: section name in the config + :param option: name of the option + :return: option value from the right section + :rtype: str + """ + + hint = ( + "First (if possible), refer to the help of the current command " + "(-h/--help).\n" + "There also might be a new version of the config after upgrade.\n" + "Hint: you can check if you have 'fedpkg.conf.rpmnew' or " + "'fedpkg.conf.rpmsave' in the config directory. If yes, try to merge " + "your changes to the config with the maintainer provided version " + "(or replace fedpkg.conf file with 'fedpkg.conf.rpmnew')." + ) + + try: + return config.get(section, option) + except NoSectionError: + msg = "Missing section '{0}' in the config file.".format(section) + raise rpkgError("{0}\n{1}".format(msg, hint)) + except NoOptionError: + msg = "Missing option '{0}' in the section '{1}' of the config file.".format( + option, section + ) + raise rpkgError("{0}\n{1}".format(msg, hint)) + except Exception: + raise diff --git a/test/test_utils.py b/test/test_utils.py index f7db2e4..904b3cb 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -177,13 +177,13 @@ class TestAssertNewTestsRepo(unittest.TestCase): class TestGetPagureToken(unittest.TestCase): - """Test get_pagure_token""" + """Test obtaining of Pagure token""" def test_return_token(self): config = Mock() config.get.return_value = '123456' - token = utils.get_pagure_token(config, 'fedpkg') + token = utils.config_get_safely(config, 'fedpkg.pagure', 'token') self.assertEqual('123456', token) config.get.assert_called_once_with('fedpkg.pagure', 'token') @@ -192,12 +192,15 @@ class TestGetPagureToken(unittest.TestCase): config = Mock() config.get.side_effect = NoOptionError('token', 'fedpkg.pagure') - six.assertRaisesRegex(self, rpkgError, 'Missing a Pagure token', - utils.get_pagure_token, config, 'fedpkg') + six.assertRaisesRegex(self, + rpkgError, "Missing option 'token' in the section 'fedpkg.pagure'", + utils.config_get_safely, + config, + 'fedpkg.pagure', 'token') config.get.side_effect = NoSectionError('fedpkg.pagure') - six.assertRaisesRegex(self, rpkgError, 'Missing a Pagure token', - utils.get_pagure_token, config, 'fedpkg') + six.assertRaisesRegex(self, rpkgError, "Missing section 'fedpkg.pagure'", + utils.config_get_safely, config, 'fedpkg.pagure', 'token') @patch('requests.get')