From ae122d7e55e88e8781ad915b4ee567b6dc649c56 Mon Sep 17 00:00:00 2001 From: Michal Konečný Date: Mar 23 2022 15:24:16 +0000 Subject: Add manual interference to scm_request_processor If manual interference is needed for SCM request, smc_request_processor first adds comment to ticket notifying the maintainers of fedora-scm-requests repository and then waits for subsequent comment that will validate the issue. Signed-off-by: Michal Konečný --- diff --git a/tests/plugins/test_scm_request_processor.py b/tests/plugins/test_scm_request_processor.py index 8fe7e13..f69f2ac 100644 --- a/tests/plugins/test_scm_request_processor.py +++ b/tests/plugins/test_scm_request_processor.py @@ -550,6 +550,167 @@ class TestCreateNewRepo: reason="Invalid", ) + def test_create_new_repo_exception_validated_by_invalid_user(self): + """ + Assert that processing will be interrupted if the ticket is validated by + wrong user. + """ + # Preparation + user = "zlopez" + invalid_user = "Tzeentch" + issue = { + "id": 100, + "user": {"name": user}, + "comments": [{ + "comment": "valid", + "user": { + "name": invalid_user + } + }] + } + + repo = "repo" + branch = "main" + namespace = "tests" + bug_id = "" + action = "new_repo" + sls = {branch: "2050-06-01"} + monitor = "monitor" + exception = False + json = { + "repo": repo, + "branch": branch, + "namespace": namespace, + "bug_id": bug_id, + "action": action, + "sls": sls, + "monitor": monitor, + "exception": exception, + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": { + "admin": [user], + "commit": [], + "ticket": [] + } + } + self.toddler.validation_comment = "valid" + + self.toddler.create_new_repo(issue, json) + + # asserts + self.toddler.pagure_io.get_project_contributors.assert_called_with( + scm_request_processor.PROJECT_NAMESPACE.split("/")[0], + scm_request_processor.PROJECT_NAMESPACE.split("/")[1] + ) + + def test_create_new_repo_exception_no_validation_comment(self): + """ + Assert that processing will be interrupted if the ticket validation comment + is not found. + """ + # Preparation + user = "zlopez" + issue = { + "id": 100, + "user": {"name": user}, + "comments": [{ + "comment": "comment", + "user": { + "name": user + } + }] + } + + repo = "repo" + branch = "main" + namespace = "tests" + bug_id = "" + action = "new_repo" + sls = {branch: "2050-06-01"} + monitor = "monitor" + exception = False + json = { + "repo": repo, + "branch": branch, + "namespace": namespace, + "bug_id": bug_id, + "action": action, + "sls": sls, + "monitor": monitor, + "exception": exception, + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": { + "admin": [user], + "commit": [], + "ticket": [] + } + } + self.toddler.validation_comment = "valid" + + self.toddler.create_new_repo(issue, json) + + # asserts + self.toddler.pagure_io.get_project_contributors.assert_called_with( + scm_request_processor.PROJECT_NAMESPACE.split("/")[0], + scm_request_processor.PROJECT_NAMESPACE.split("/")[1] + ) + + def test_create_new_repo_exception_notify_maintainers(self): + """ + Assert that comment will be added when the ticket needs manual + validation. + """ + # Preparation + user = "zlopez" + issue = { + "id": 100, + "user": {"name": user}, + "comments": [] + } + + repo = "repo" + branch = "main" + namespace = "tests" + bug_id = "" + action = "new_repo" + sls = {branch: "2050-06-01"} + monitor = "monitor" + exception = False + json = { + "repo": repo, + "branch": branch, + "namespace": namespace, + "bug_id": bug_id, + "action": action, + "sls": sls, + "monitor": monitor, + "exception": exception, + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": { + "admin": [user, "Tzeentch"], + "commit": [], + "ticket": [] + } + } + self.toddler.ping_comment = "Look at this comment {maintainers}" + self.toddler.create_new_repo(issue, json) + + # asserts + self.toddler.pagure_io.get_project_contributors.assert_called_with( + scm_request_processor.PROJECT_NAMESPACE.split("/")[0], + scm_request_processor.PROJECT_NAMESPACE.split("/")[1] + ) + + message = "Look at this comment @{} @Tzeentch".format(user) + + self.toddler.pagure_io.add_comment_to_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + comment=message + ) + def test_create_new_repo_missing_bug_id(self): """ Assert that ticket will be closed if Bugzilla bug id is not provided. @@ -612,7 +773,10 @@ class TestCreateNewRepo: @patch( "toddlers.plugins.scm_request_processor.SCMRequestProcessor.valid_epel_package" ) - def test_create_new_repo_invalid_epel(self, mock_valid_epel_package): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_invalid_epel(self, mock_validate_review_bug, mock_valid_epel_package): """ Assert that ticket will be closed if repo is invalid EPEL repo. """ @@ -625,7 +789,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -643,6 +807,9 @@ class TestCreateNewRepo: mock_valid_epel_package.assert_called_with(repo, branch) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.pagure_io.close_issue.assert_called_with( 100, namespace=scm_request_processor.PROJECT_NAMESPACE, @@ -650,7 +817,10 @@ class TestCreateNewRepo: reason="Invalid", ) - def test_create_new_repo_requester_not_in_dist_git(self): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_requester_not_in_dist_git(self, mock_validate_review_bug): """ Assert that ticket will be commented on when requester doesn't have a valid dist git account. @@ -664,7 +834,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -681,6 +851,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.user_exists.assert_called_with("zlopez") message = "@zlopez needs to login to {0} to sync accounts before we can proceed.".format( @@ -693,7 +866,10 @@ class TestCreateNewRepo: comment=message, ) - def test_create_new_repo_project_exists(self): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_project_exists(self, mock_validate_review_bug): """ Assert that ticket will be closed when repo already exists in dist git. """ @@ -706,7 +882,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -722,6 +898,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) self.toddler.pagure_io.close_issue.assert_called_with( @@ -731,7 +910,10 @@ class TestCreateNewRepo: reason="Invalid", ) - def test_create_new_repo_master_branch(self): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_master_branch(self, mock_validate_review_bug): """ Assert that ticket will be closed when branch is set to master branch. Master branch is no longer allowed. @@ -745,7 +927,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -760,6 +942,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) self.toddler.pagure_io.close_issue.assert_called_with( @@ -769,7 +954,10 @@ class TestCreateNewRepo: reason="Invalid", ) - def test_create_new_repo_unsupported_namespace(self): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_unsupported_namespace(self, mock_validate_review_bug): """ Assert that ticket will be closed when requested namespace is not recognized. """ @@ -782,7 +970,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -797,6 +985,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) error = ( @@ -813,7 +1004,10 @@ class TestCreateNewRepo: ) @patch("toddlers.utils.pdc.get_branch") - def test_create_new_repo_branch_exist(self, mock_pdc_get_branch): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_branch_exist(self, mock_validate_review_bug, mock_pdc_get_branch): """ Assert that ticket will be closed when branch already exist in PDC. """ @@ -826,7 +1020,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {"rawhide": "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -842,6 +1036,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) mock_pdc_get_branch.assert_called_with(repo, branch, namespace.rstrip("s")) @@ -866,7 +1063,18 @@ class TestCreateNewRepo: """ Assert that ticket will be processed when everything is in order and namespace is correct. """ - issue = {"id": 100, "user": {"name": "zlopez"}} + # Preparation + user = "zlopez" + issue = { + "id": 100, + "user": {"name": user}, + "comments": [{ + "comment": "valid", + "user": { + "name": user + } + }] + } repo = "repo" bug_id = "" @@ -887,8 +1095,17 @@ class TestCreateNewRepo: dist_git_url = "https://src.fp.o" self.toddler.dist_git.get_project.return_value = None self.toddler.dist_git._pagure_url = dist_git_url + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": { + "admin": [user], + "commit": [], + "ticket": [] + } + } + self.toddler.validation_comment = "valid" mock_pdc.get_branch.return_value = None + # Method to test self.toddler.create_new_repo(issue, json) # asserts @@ -929,7 +1146,18 @@ class TestCreateNewRepo: Assert that ticket will be processed when everything is in order and namespace is set to tests. """ - issue = {"id": 100, "user": {"name": "zlopez"}} + # Preparation + user = "zlopez" + issue = { + "id": 100, + "user": {"name": user}, + "comments": [{ + "comment": "valid", + "user": { + "name": user + } + }] + } repo = "repo" branch = "main" @@ -938,7 +1166,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {branch: "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -952,11 +1180,23 @@ class TestCreateNewRepo: dist_git_url = "https://src.fp.o" self.toddler.dist_git.get_project.return_value = None self.toddler.dist_git._pagure_url = dist_git_url + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": { + "admin": [user], + "commit": [], + "ticket": [] + } + } + self.toddler.validation_comment = "valid" mock_pdc.get_branch.return_value = None self.toddler.create_new_repo(issue, json) # asserts + self.toddler.pagure_io.get_project_contributors.assert_called_with( + scm_request_processor.PROJECT_NAMESPACE.split("/")[0], + scm_request_processor.PROJECT_NAMESPACE.split("/")[1] + ) self.toddler.dist_git.get_project.assert_called_with(namespace, repo) self.toddler.dist_git.new_project.assert_called_with( namespace, repo, "", "", branch, initial_commit=True, alias=True @@ -981,8 +1221,12 @@ class TestCreateNewRepo: reason="Processed", ) + @patch("toddlers.plugins.scm_request_processor.bugzilla_system") @patch("toddlers.plugins.scm_request_processor.pdc") - def test_create_new_repo_non_default_branch(self, mock_pdc): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_non_default_branch(self, mock_validate_review_bug, mock_pdc, mock_bz): """ Assert that ticket will be processed when everything is in order and requested branch is not default. @@ -992,11 +1236,11 @@ class TestCreateNewRepo: repo = "repo" branch = "f35" namespace = "rpms" - bug_id = "" + bug_id = "123" action = "new_repo" sls = {branch: "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -1017,6 +1261,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) # asserts + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) self.toddler.dist_git.new_project.assert_called_with( namespace, repo, "", "", "rawhide", initial_commit=True, alias=True @@ -1062,12 +1309,16 @@ class TestCreateNewRepo: message=message, reason="Processed", ) + mock_bz.comment_on_bug.assert_called_with(bug_id, message) @patch("toddlers.plugins.scm_request_processor.bugzilla_system") @patch("toddlers.plugins.scm_request_processor.pdc") - def test_create_new_repo_update_bug(self, mock_pdc, mock_bz): + @patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" + ) + def test_create_new_repo_default_brach(self, mock_validate_review_bug, mock_pdc, mock_bz): """ - Assert that bug will be updated if the bug_id is provided. + Assert that repo will be created with default branch when everything is in order. """ issue = {"id": 100, "user": {"name": "zlopez"}} @@ -1078,7 +1329,7 @@ class TestCreateNewRepo: action = "new_repo" sls = {branch: "2050-06-01"} monitor = "monitor" - exception = True + exception = False json = { "repo": repo, "branch": branch, @@ -1098,6 +1349,9 @@ class TestCreateNewRepo: self.toddler.create_new_repo(issue, json) # asserts + mock_validate_review_bug.assert_called_with( + bug_id, repo, branch, namespace=namespace, pagure_user="zlopez") + self.toddler.dist_git.get_project.assert_called_with(namespace, repo) self.toddler.dist_git.new_project.assert_called_with( namespace, repo, "", "", branch, initial_commit=True, alias=True diff --git a/toddlers.toml.example b/toddlers.toml.example index 804b8ab..f75abd1 100644 --- a/toddlers.toml.example +++ b/toddlers.toml.example @@ -230,6 +230,11 @@ pagure_url = "https:/pagure.io" pagure_api_key = "API token for pagure" # Monitoring choices for release-monitoring.org monitor_choices = ['no-monitoring', 'monitoring', 'monitoring-with-scratch'] +# What we should look for in validation comment +validation_comment = "valid" +# Text for the ping if the ticket needs to be manually verified +ping_comment = This request wants to skip bugzilla validation! {maintainers} could you check if this is correct? If yes, please respond to this ticket with 'valid' comment + # Pagure mapping to bugzilla [consumer_config.scm_request_processor.pagure_namespace_to_component] diff --git a/toddlers/plugins/scm_request_processor.py b/toddlers/plugins/scm_request_processor.py index 8a177b3..af794f3 100644 --- a/toddlers/plugins/scm_request_processor.py +++ b/toddlers/plugins/scm_request_processor.py @@ -82,6 +82,15 @@ class SCMRequestProcessor(ToddlerBase): # Requests session requests_session: requests.requests.Session + # Validation comment + # This is the comment we will be looking for in the tickets + # That need to be manually verified + validation_comment: str = "" + + # Ping comment + # Comment to sent when the ticket needs to be manually verified + ping_comment: str = "" + def accepts_topic(self, topic: str) -> bool: """Returns a boolean whether this toddler is interested in messages from this specific topic. @@ -136,6 +145,8 @@ class SCMRequestProcessor(ToddlerBase): self.pagure_namespace_to_product = config.get("pagure_namespace_to_product", {}) self.temp_dir = config.get("temp_dir", "") self.requests_session = requests.make_session() + self.validation_comment = config.get("validation_comment", "") + self.ping_comment = config.get("ping_comment", "") _log.info("Setting up PDC client") pdc.set_pdc(config) @@ -378,10 +389,39 @@ class SCMRequestProcessor(ToddlerBase): "tests", "container", ): - skip_msg = "- Skipping verification of RHBZ" - if bug_id: - skip_msg = "{0} #{1}".format(skip_msg, bug_id) - _log.info(skip_msg) + _log.info("- This ticket needs to be validated manually") + contributors = self.pagure_io.get_project_contributors( + PROJECT_NAMESPACE.split("/")[0], PROJECT_NAMESPACE.split("/")[1] + ) + # Get the list of maintainers of the scm_requests repository + maintainers = ( + set(contributors["users"]["admin"]) + | set(contributors["users"]["commit"]) + | set(contributors["users"]["ticket"]) + ) + # There is already a comment on this ticket, we should check if this is + # the validation ticket + if len(issue["comments"]) > 0: + valid = False + _log.info("- Check for validation comment") + for comment in issue["comments"]: + if comment["comment"].strip() == self.validation_comment: + if comment["user"]["name"] in maintainers: + _log.info("- Ticket is validated by {}, continue processing".format(comment["user"]["name"])) + valid = True + break + if not valid: + _log.info("- Ticket is not yet validated. Skipping...") + return + # No comment on the ticket yet, the maintainers should be pinged + else: + _log.info("- Notify the responsible users") + self.pagure_io.add_comment_to_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + comment=self.ping_comment.format(maintainers=" ".join(["@" + user for user in maintainers])) + ) + return else: if not bug_id: self.pagure_io.close_issue(