PR#188: Fixes for unretire packages plugin - fedora-infra/toddlers

fedora-infra / toddlers

#188 Fixes for unretire packages plugin

Merged a year ago by zlopez. Opened a year ago by amedvede.

fedora-infra/ amedvede/toddlers unretire_package_bug_fixes into main

pdc_unretire_packages: fixes of plugin and tests to cover changed code

amedvede • a year ago

075d83d

tests/plugins/test_pdc_unretire_packages.py

file modified

+62 -27

		`@@ -102,7 +102,7 @@`
		`caplog.records[-1].message == "The issue 100 is not open. Skipping message."`
		`)`

		`- def test_process_keyword_in_title(self, caplog, toddler):`
		`+ def test_process_keyword_not_in_title(self, caplog, toddler):`
		`"""`
		`Assert that message without keyword in title will be skipped.`
		`"""`
		`@@ -128,18 +128,49 @@`

		`assert (`
		`caplog.records[-1].message`
		`- == "The issue doesn't contain keyword 'unretire' in the title 'unrere popa'"`
		`+ == "The issue doesn't contain keyword 'unretire' in the title 'unrere popa', or it's "`
		`+ "not a first word in title"`
		`+ )`
		`+`
		`+ def test_process_keyword_not_on_first_position_in_title(self, caplog, toddler):`
		`+ """`
		`+ Assert that message without keyword in title will be skipped.`
		`+ """`
		`+ caplog.set_level(logging.INFO)`
		`+`
		`+ body = {`
		`+ "project": {"fullname": pdc_unretire_packages.PROJECT_NAMESPACE},`
		`+ "issue": {`
		`+ "id": 100,`
		`+ "status": "Open",`
		`+ "title": "Pac unretire",`
		`+ "user": {"name": "zlopez"},`
		`+ },`
		`+ }`
		`+ msg = IssueNewV1(body=body)`
		`+`
		`+ with patch(`
		`+ "toddlers.plugins.pdc_unretire_packages.PDCUnretirePackages.process_ticket"`
		`+ ) as mock_process_ticket:`
		`+ toddler.process({}, msg)`
		`+`
		`+ mock_process_ticket.assert_not_called()`
		`+`
		`+ assert (`
		`+ caplog.records[-1].message`
		`+ == "The issue doesn't contain keyword 'unretire' in the title 'Pac unretire', or it's "`
		`+ "not a first word in title"`
		`)`

		`@patch("toddlers.utils.pdc.pdc_client_for_config")`
		`@patch("toddlers.utils.pagure.set_pagure")`
		`@patch("toddlers.utils.bugzilla_system.set_bz")`
		`@patch("koji.ClientSession")`
		`- def test_process_base_exception_during_ticket_processing(`
		`- self, mock_koji, mock_bugzilla, mock_pagure, mock_pdc, toddler`
		`+ def test_process_base_exception_appeared(`
		`+ self, mock_koji, mock_bz, mock_pagure, mock_pdc, caplog, toddler`
		`):`
		`"""`
		`- Assert that ticket will be close if BaseException will appear during ticket process`
		`+ Assert that if base exception appeared, it will be processed correctly.`
		`"""`
		`issue = {`
		`"id": 100,`
		`@@ -151,11 +182,12 @@`
		`"project": {"fullname": pdc_unretire_packages.PROJECT_NAMESPACE},`
		`"issue": issue,`
		`}`
		`- msg = IssueNewV1(body=body)`
		`config = {`
		`"dist_git_url": "https://src.fedoraproject.org",`
		`"dist_git_token": "Private API Key",`
		`+ "temp_folder": "/var/tmp",`
		`}`
		`+ msg = IssueNewV1(body=body)`

		`mock_pagure_io = Mock()`
		`mock_pagure.return_value = mock_pagure_io`
		`@@ -163,12 +195,10 @@`
		`with patch(`
		`"toddlers.plugins.pdc_unretire_packages.PDCUnretirePackages.process_ticket"`
		`) as mock_process_ticket:`
		`- mock_process_ticket.side_effect = Exception("Exception")`
		`+ mock_process_ticket.side_effect = Exception("test")`
		`toddler.process(config, msg)`
		`mock_process_ticket.assert_called_with(issue)`

		`- mock_pdc.assert_called_with(config)`
		`- mock_bugzilla.assert_called_with(config)`
		`mock_pagure_io.add_comment_to_issue.assert_called_once()`

		`@patch("toddlers.utils.pdc.pdc_client_for_config")`
		`@@ -229,13 +259,16 @@`
		`Initialize toddler`
		`"""`
		`self.toddler = pdc_unretire_packages.PDCUnretirePackages()`
		`+ self.toddler.temp_dir = Mock()`
		`self.toddler.git_repo = MagicMock()`
		`self.toddler.pagure_io = Mock()`
		`self.toddler.koji_session = MagicMock()`

		`- def test_process_ticket_package_name_doesnt_have_required_prefix(self, caplog):`
		`+ def test_process_ticket_issue_title_should_not_contain_more_than_two_words(`
		`+ self, caplog`
		`+ ):`
		`"""`
		`- Assert that ticket will be closed if title doesn't contain required prefix.`
		`+ Assert that toddler will close issue if title is more than two words.`
		`"""`
		`caplog.set_level(logging.INFO)`
		`issue = {`
		`@@ -248,10 +281,9 @@`
		`"id": 55,`
		`}`
		`close_msg = (`
		- "Unretire can only apply to rpm packages, please add `{0}`"
		`- " prefix before the package name"`
		`- ).format(pdc_unretire_packages.RPM_PREFIX)`
		`-`
		`+ "The unretirement can be requested just for one package in time, title should not "`
		`+ "contain more than two words."`
		`+ )`
		`self.toddler.process_ticket(issue)`
		`assert caplog.records[-1].message == close_msg`
		`self.toddler.pagure_io.close_issue.assert_called_with(`
		`@@ -261,13 +293,13 @@`
		`reason="Invalid",`
		`)`

		`- def test_process_ticket_two_packages_requested(self, caplog):`
		`+ def test_process_ticket_package_name_doesnt_have_required_prefix(self, caplog):`
		`"""`
		`- Assert that ticket will be closed if in single title two packages were requested.`
		`+ Assert that ticket will be closed if title doesn't contain required prefix.`
		`"""`
		`caplog.set_level(logging.INFO)`
		`issue = {`
		`- "title": "Unretire rpms/rubygem-logging lala rpms/pakca",`
		`+ "title": "Unretire rubygem-logging",`
		`"content": "",`
		`"full_url": "",`
		`"user": {`
		`@@ -276,9 +308,9 @@`
		`"id": 55,`
		`}`
		`close_msg = (`
		`- "Requester trying to request more than one package in single ticket "`
		`- "or made a mistake in namespace."`
		`- )`
		+ "Unretire can only apply to rpm packages, please add `{0}`"
		`+ " prefix before the package name"`
		`+ ).format(pdc_unretire_packages.RPM_PREFIX)`

		`self.toddler.process_ticket(issue)`
		`assert caplog.records[-1].message == close_msg`
		`@@ -352,7 +384,10 @@`
		`"toddlers.plugins.pdc_unretire_packages.PDCUnretirePackages._is_url_exist",`
		`return_value=True,`
		`)`
		`- def test_process_ticket_clone_repo_error(self, mock_is_url_exist, caplog):`
		`+ @patch("tempfile.TemporaryDirectory")`
		`+ def test_process_ticket_clone_repo_error(`
		`+ self, mock_temp_dir, mock_is_url_exist, caplog`
		`+ ):`
		`"""`
		`Assert that remote repo doesn't exist or user don't have a permission.`
		`"""`
		`@@ -366,10 +401,7 @@`
		`},`
		`"id": 55,`
		`}`
		`- close_msg = (`
		`- "Remote repository doesn't exist or you do not have the appropriate "`
		`- "permissions to access it."`
		`- )`
		`+ close_msg = "Something went wrong during cloning git repository."`
		`with patch(`
		`"toddlers.plugins.pdc_unretire_packages.git.clone_repo",`
		`side_effect=git.GitCommandError("some_command"),`
		`@@ -392,8 +424,9 @@`
		`"toddlers.plugins.pdc_unretire_packages.PDCUnretirePackages._is_url_exist",`
		`return_value=True,`
		`)`
		`+ @patch("tempfile.TemporaryDirectory")`
		`def test_process_ticket_package_not_ready_for_unretirement(`
		`- self, mock_is_url_exist, mock_get_tags, mock_git`
		`+ self, mock_temp_dir, mock_is_url_exist, mock_get_tags, mock_git`
		`):`
		`"""`
		`Assert that processing will not continue if package will not be verified.`
		`@@ -436,8 +469,10 @@`
		`"toddlers.plugins.pdc_unretire_packages.PDCUnretirePackages._is_url_exist",`
		`return_value=True,`
		`)`
		`+ @patch("tempfile.TemporaryDirectory")`
		`def test_process_ticket(`
		`self,`
		`+ mock_temp_dir,`
		`mock_is_url_exist,`
		`is_package_ready_mock,`
		`get_tags_mock,`

toddlers/plugins/pdc_unretire_packages.py

file modified

+31 -36

		`@@ -126,21 +126,23 @@`
		`return`

		`issue_title = issue["title"]`
		`-`
		`- if UNRETIRE_KEYWORD not in issue_title.lower():`
		`+ words_in_issue_title = issue_title.split()`
		`+ if UNRETIRE_KEYWORD != words_in_issue_title[0].lower():`
		`_log.info(`
		`- "The issue doesn't contain keyword '{0}' in the title '{1}'".format(`
		`- UNRETIRE_KEYWORD, issue_title`
		`- )`
		`+ "The issue doesn't contain keyword '{0}' in the title '{1}', or it's not a first "`
		`+ "word in title".format(UNRETIRE_KEYWORD, issue_title)`
		`)`
		`return`

		`+ print("12")`
		`+ _log.info("Getting temp_folder name from config.")`
		`+ self.temp_dir = config.get("temp_folder", "")`
		`+`
		`_log.info("Getting dist-git url from config.")`
		`self.dist_git_base = config.get("dist_git_url")`

		`_log.info("Setting up connection to PDC")`
		`self.pdc_client = pdc.pdc_client_for_config(config)`
		`-`
		`_log.info("Setting up connection to Pagure")`
		`self.pagure_io = pagure.set_pagure(config)`

		`@@ -175,33 +177,32 @@`
		`issue_opener = issue["user"]["name"]`
		`issue_id = issue["id"]`

		- _log.debug("Verifying that issue title has `{0}` prefix".format(RPM_PREFIX))
		`- if RPM_PREFIX not in issue_title.lower():`
		`+ _log.debug(`
		`+ "Verifying that issue title contain just 'unretire' keyword and package name."`
		`+ )`
		`+ words_in_issue_title = issue_title.split()`
		`+ if len(words_in_issue_title) > 2:`
		`msg = (`
		- "Unretire can only apply to rpm packages, please add `{0}`"
		`- " prefix before the package name"`
		`- ).format(RPM_PREFIX)`
		`+ "The unretirement can be requested just for one package in time, title should not "`
		`+ "contain more than two words."`
		`+ )`
		`_log.info(msg)`
		`self.pagure_io.close_issue(`
		`- issue_id,`
		`- namespace=PROJECT_NAMESPACE,`
		`- message=msg,`
		`- reason="Invalid",`
		`+ issue_id, namespace=PROJECT_NAMESPACE, message=msg, reason="Invalid"`
		`)`
		`return`

		`- _log.debug("Verifying that just single package is requested in one ticket.")`
		`- words_in_issue_title = issue_title.split()`
		`- words_with_prefix = [`
		`- word.lower()`
		`- for word in words_in_issue_title`
		`- if word.lower().startswith(RPM_PREFIX)`
		`- ]`
		`- if len(words_with_prefix) != 1:`
		`+ pkg_name_with_ns = words_in_issue_title[1].lower()`
		`+`
		`+ _log.debug(`
		`+ "Verifying that package name in issue title"`
		+ " starts with `{0}` prefix".format(RPM_PREFIX)
		`+ )`
		`+ if not pkg_name_with_ns.startswith(RPM_PREFIX):`
		`msg = (`
		`- "Requester trying to request more than one package in single ticket "`
		`- "or made a mistake in namespace."`
		`- )`
		+ "Unretire can only apply to rpm packages, please add `{0}`"
		`+ " prefix before the package name"`
		`+ ).format(RPM_PREFIX)`
		`_log.info(msg)`
		`self.pagure_io.close_issue(`
		`issue_id,`
		`@@ -211,7 +212,6 @@`
		`)`
		`return`

		`- pkg_name_with_ns = words_with_prefix[0]`
		`pkg_name_and_ns = [word for word in pkg_name_with_ns.split("/") if word != ""]`

		`_log.debug("Verifying that package name with namespace are correct.")`
		`@@ -247,24 +247,19 @@`
		`)`
		`return`

		`- folder_name_for_clonned_repo = "{0}_folder".format(package_name)`
		`+ folder_name_for_clonned_repo = "{0}_repository_folder".format(package_name)`

		`_log.info("Creating temporary directory")`
		`- with tempfile.TemporaryDirectory():`
		`+ with tempfile.TemporaryDirectory(dir=self.temp_dir) as tmp_dir:`
		`_log.info(`
		`"Cloning repo into dir with name '{0}'".format(`
		`folder_name_for_clonned_repo`
		`)`
		`)`
		`try:`
		`- self.git_repo = git.clone_repo(`
		`- package_url, folder_name_for_clonned_repo`
		`- )`
		`+ self.git_repo = git.clone_repo(package_url, tmp_dir)`
		`except GitCommandError:`
		`- message = (`
		`- "Remote repository doesn't exist or you do not have the appropriate "`
		`- "permissions to access it."`
		`- )`
		`+ message = "Something went wrong during cloning git repository."`
		`_log.info(message)`
		`self.pagure_io.close_issue(`
		`issue_id,`