PR#388: Check missing config options more reliably - fedpkg

fedpkg

#388 Check missing config options more reliably

Merged 4 years ago by onosek. Opened 4 years ago by onosek.

onosek/fedpkg config_warning into master

Check missing config options more reliably

Ondrej Nosek • 4 years ago

dcfdb9f

conf/etc/rpkg/fedpkg.conf

file modified

		`@@ -69,6 +69,7 @@`

		`[fedpkg.pagure]`
		`url = https://pagure.io/`
		`+ token =`

		`[fedpkg.pdc]`
		`url = https://pdc.fedoraproject.org/`

fedpkg/cli.py

file modified

+24 -18

		`@@ -30,11 +30,10 @@`

		`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 @@`

		`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 @@`
		`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 @@`

		`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 @@`

		`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 @@`

		`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 @@`
		`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 @@`
		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 @@`
		`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 @@`

		`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,`

fedpkg/utils.py

file modified

+40 -32

		`@@ -292,38 +292,6 @@`
		`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 @@`
		`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`

test/test_utils.py

file modified

+9 -6

		`@@ -177,13 +177,13 @@`


		`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 @@`
		`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')`

onosek commented 4 years ago

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