PR#310: Show hint when Pagure token expires - fedpkg

fedpkg

#310 Show hint when Pagure token expires

Merged 5 years ago by onosek. Opened 5 years ago by onosek.

onosek/fedpkg hint_pagure_token_expires into master

Show hint when Pagure token expires

Ondrej Nosek • 5 years ago

a66bbaf

fedpkg/cli.py

file modified

+2 -2

		`@@ -855,7 +855,7 @@`
		`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))`
		`+ pagure_url, pagure_token, ticket_title, ticket_body, name))`

		`def request_branch(self):`
		`if self.args.repo_name_for_branch:`
		`@@ -982,7 +982,7 @@`
		`b, ns, repo_name)`

		`print(new_pagure_issue(`
		`- pagure_url, pagure_token, ticket_title, ticket_body))`
		`+ pagure_url, pagure_token, ticket_title, ticket_body, name))`

		`# For non-standard rpm branch requests, also request a matching new`
		`# module repo with a matching branch.`

fedpkg/utils.py

file modified

+6 -1

		`@@ -87,7 +87,7 @@`
		`return None`


		`- def new_pagure_issue(url, token, title, body):`
		`+ def new_pagure_issue(url, token, title, body, cli_name):`
		`"""`
		`Posts a new Pagure issue`
		`:param url: a string of the URL to Pagure`
		`@@ -127,6 +127,11 @@`
		`rv_error = rv.json().get('error')`
		`except ValueError:`
		`rv_error = rv.text`
		`+ # show hint for expired token`
		`+ if re.search(r"Invalid or expired token", rv_error, re.IGNORECASE):`
		`+ base_error_msg += '\nFor invalid or expired token refer to ' \`
		`+ '"{0} request-repo -h" to set a token in your user ' \`
		`+ 'configuration.'.format(cli_name)`
		`raise rpkgError(base_error_msg.format(rv_error))`

		`return '{0}/releng/fedora-scm-requests/issue/{1}'.format(`

test/test_utils.py

file modified

+19 -3

		`@@ -357,7 +357,7 @@`
		`six.assertRaisesRegex(`
		`self, rpkgError, 'The connection to Pagure failed',`
		`utils.new_pagure_issue,`
		`- 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'})`
		`+ 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')`

		`def test_responses_not_ok_and_response_body_is_not_json(self, post):`
		`rv = Mock(ok=False, text='error')`
		`@@ -368,7 +368,22 @@`
		`self, rpkgError,`
		`'The following error occurred while creating a new issue',`
		`utils.new_pagure_issue,`
		`- 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'})`
		`+ 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')`
		`+`
		`+ def test_responses_not_ok_when_token_is_expired(self, post):`
		`+ rv = Mock(`
		`+ ok=False,`
		`+ text='Invalid or expired token. Please visit '`
		`+ 'https://pagure.io/settings#api-keys to get or renew your API token.')`
		`+ rv.json.side_effect = ValueError`
		`+ post.return_value = rv`
		`+`
		`+ six.assertRaisesRegex(`
		`+ self, rpkgError,`
		`+ 'For invalid or expired token refer to "fedpkg request-repo -h" to set '`
		`+ 'a token in your user configuration.',`
		`+ utils.new_pagure_issue,`
		`+ 'http://distgit/', '123456', 'new package', {'repo': 'pkg1'}, 'fedpkg')`

		`def test_create_pagure_issue(self, post):`
		`rv = Mock(ok=True)`
		`@@ -381,7 +396,8 @@`
		`issue_url = utils.new_pagure_issue(pagure_api_url,`
		`'123456',`
		`'new package',`
		`- issue_ticket_body)`
		`+ issue_ticket_body,`
		`+ 'fedpkg',)`

		`expected_issue_url = (`
		`'{0}/releng/fedora-scm-requests/issue/1'`

onosek commented 5 years ago

Command fedpkg request-repo --help already contains this information, so adding hint (to execute this command) when this happens.

Fixes: #285

Signed-off-by: Ondrej Nosek onosek@redhat.com

cqi commented on line 15 of fedpkg/utils.py 5 years ago

Would this be too general catch the expected error message? In test, a possible response error message contains content "Invalid or expired token", if Pagure.io actually responses this content when a token is invalid or expired, it would be good just to match this content as much as possible, in case '\btoken\b' matches any other potential unrelated messages that just contains a word "token".

rebased onto a66bbaf

5 years ago

onosek commented 5 years ago

Thanks for the comment. Actually, it was my original purpose - to catch all error messages related to tokens.
Of course, there could be other different messages, that refer to different errors than invalid or expired token (missing token is covered elsewhere), but the Pagure message is still shown, and as a hint, there is added one sentence with context "For invalid or expired token refer to ...".
OK, I did the search pattern more specific as per your comment, but there is danger in case that Pagure will change the error wording, the hint will silently disappear. And that is, in my opinion, better than showing this hint when it is not 100% relevant.