From f8a8c2b68a0e844f8d53a7a6d90dfa4dfb686f87 Mon Sep 17 00:00:00 2001 From: Michal Konecny Date: Wed, 2 Apr 2025 16:07:50 +0200 Subject: [PATCH] [scm_request_processor] Make bot more interactive This commits adds separate processing for comments in scm_request_processor and adds following commands the bot will react on. help - prints this help message retry - retries the failed request processing valid - can be used by authorized user to mark the request as valid --- docs/scm_request_processor.md | 10 + tests/plugins/test_scm_request_processor.py | 578 +++++++++----------- toddlers.toml.example | 2 - toddlers/plugins/scm_request_processor.py | 273 ++++++--- 4 files changed, 465 insertions(+), 398 deletions(-) diff --git a/docs/scm_request_processor.md b/docs/scm_request_processor.md index 47b24c3..7e72448 100644 --- a/docs/scm_request_processor.md +++ b/docs/scm_request_processor.md @@ -10,6 +10,16 @@ The SCM Request processor accepts few topics emitted by pagure. These topics all * org.fedoraproject.*.pagure.issue.edit - Issue on pagure is edited * org.fedoraporject.*.pagure.issue.comment.added - New comment is added to issue +## Supported commands +The SCM Request processor now supports few commands that could be invoked by sending comment +addressed to bot user (releng-bot) in issue opened on https://pagure.io/releng/fedora-scm-requests/ + +Supported commands: + +* help - prints help for commands +* retry - retries the failed request +* valid - validate the request as authorized user + ## Flowchart diagram [Flowchart diagram](./scm_request_processor.png) illustrates the code flow of the SCM Request Processor, what is being validated and what reaction is done if the validation fails. diff --git a/tests/plugins/test_scm_request_processor.py b/tests/plugins/test_scm_request_processor.py index 4721d6d..eed5c62 100644 --- a/tests/plugins/test_scm_request_processor.py +++ b/tests/plugins/test_scm_request_processor.py @@ -122,11 +122,11 @@ class TestProcess: msg = IssueCommentAddedV1(body=body) with patch( - "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket" - ) as mock_process_ticket: + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_comment" + ) as mock_process_comment: toddler.process(config, msg) - mock_process_ticket.assert_not_called() + mock_process_comment.assert_not_called() assert caplog.records[ -1 @@ -231,6 +231,39 @@ class TestProcess: mock_fasjson.assert_called_with(config) mock_bugzilla.assert_called_with(config) + @patch("toddlers.utils.pagure.set_pagure") + @patch("toddlers.utils.fedora_account.set_fasjson") + @patch("toddlers.utils.bugzilla_system.set_bz") + def test_process_comment(self, mock_bugzilla, mock_fasjson, mock_pagure, toddler): + """ + Assert that toddler will handle comments correctly. + """ + pagure_user = "pagure_user" + config = { + "pagure_user": pagure_user, + "branch_slas": {}, + "monitoring_choices": [], + "pagure_namespace_to_component": {}, + "pagure_namespace_to_product": {}, + "temp_dir": "", + "dist_git_url": "https://src.fedoraproject.org", + "dist_git_token": "Private API Key", + } + issue = {"status": "Open", "user": {"name": "zlopez"}} + body = { + "project": {"fullname": scm_request_processor.PROJECT_NAMESPACE}, + "issue": {"status": "Open", "user": {"name": "zlopez"}}, + "agent": "zlopez", + } + msg = IssueCommentAddedV1(body=body) + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_comment" + ) as mock_process_comment: + toddler.process(config, msg) + + mock_process_comment.assert_called_with(issue) + class TestProcessTicket: """ @@ -416,7 +449,7 @@ class TestProcessTicket: self.toddler.branch_slas = {"branch": "SLA"} with patch( - "toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_repo" + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_new_repo" ) as mock_new_repo: self.toddler.process_ticket(issue) @@ -563,9 +596,194 @@ class TestVerifySLAs: self.toddler.verify_slas("", sla) -class TestCreateNewRepo: +class TestProcessComment: """ - Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_repo` + Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_comment`. + """ + + def setup_method(self): + """ + Initialize toddler. + """ + self.toddler = scm_request_processor.SCMRequestProcessor() + self.toddler.pagure_io = Mock() + self.toddler.pagure_user = "pagure_user" + + def test_process_comment_notification(self, caplog): + """ + Assert that notification will be ignored. + """ + issue = { + "comments": [{"notification": True}], + "full_url": "https://blacklibrary.wh40k", + } + caplog.set_level(logging.INFO) + + self.toddler.process_comment(issue) + + assert caplog.records[-1].message == "Comment is notification. Ignoring." + + def test_process_comment_missing_bot_mention(self, caplog): + """ + Assert that comment will be ignored if it doesn't mention bot. + """ + issue = { + "comments": [{"notification": False, "comment": ""}], + "full_url": "https://blacklibrary.wh40k", + } + caplog.set_level(logging.INFO) + + self.toddler.process_comment(issue) + + assert caplog.records[ + -1 + ].message == "Comment is not for '{}'. Ignoring.".format( + self.toddler.pagure_user + ) + + def test_process_comment_help_command(self): + """ + Assert that help command will be processed correctly. + """ + issue = { + "id": 100, + "comments": [ + { + "notification": False, + "comment": "@{0} help".format(self.toddler.pagure_user), + } + ], + "full_url": "https://blacklibrary.wh40k", + } + + self.toddler.process_comment(issue) + + self.toddler.pagure_io.add_comment_to_issue.assert_called_with( + issue["id"], + namespace=scm_request_processor.PROJECT_NAMESPACE, + comment=( + "{0} recognizes following commands on {1}.\n" + "help - prints this help message\n" + "retry - retries the failed request processing\n" + "valid - can be used by authorized user to mark the request as valid\n" + ).format(self.toddler.pagure_user, scm_request_processor.PROJECT_NAMESPACE), + ) + + def test_process_comment_retry_command(self): + """ + Assert that retry command will be processed correctly. + """ + issue = { + "id": 100, + "comments": [ + { + "notification": False, + "comment": "@{0} retry".format(self.toddler.pagure_user), + } + ], + "full_url": "https://blacklibrary.wh40k", + } + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket" + ) as mock_process_ticket: + self.toddler.process_comment(issue) + + mock_process_ticket.assert_called_with(issue) + + def test_process_comment_valid_command(self): + """ + Assert that valid command from authorized user + will resume repo creation. + """ + user = "zlopez" + issue = { + "id": 100, + "comments": [ + { + "user": {"name": user}, + "notification": False, + "comment": "@{0} valid".format(self.toddler.pagure_user), + } + ], + "full_url": "https://blacklibrary.wh40k", + "content": '{"action": "new_repo"}', + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": {"admin": [user], "commit": [], "ticket": []} + } + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor._create_new_repo" + ) as mock_create_new_repo: + self.toddler.process_comment(issue) + + mock_create_new_repo.assert_called_with(issue, {"action": "new_repo"}) + + def test_process_comment_valid_command_wrong_action(self): + """ + Assert that valid command will fail for invalid action. + """ + user = "zlopez" + issue = { + "id": 100, + "comments": [ + { + "user": {"name": user}, + "notification": False, + "comment": "@{0} valid".format(self.toddler.pagure_user), + } + ], + "full_url": "https://blacklibrary.wh40k", + "content": '{"action": "new_branch"}', + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": {"admin": [user], "commit": [], "ticket": []} + } + + self.toddler.process_comment(issue) + + self.toddler.pagure_io.add_comment_to_issue.assert_called_with( + issue["id"], + namespace=scm_request_processor.PROJECT_NAMESPACE, + comment="Validation can be done for 'new_repo' requests only.", + ) + + def test_process_comment_valid_command_unauthorized_user(self): + """ + Assert that valid command will fail for unathorized user. + """ + user = "zlopez" + issue = { + "id": 100, + "comments": [ + { + "user": {"name": user}, + "notification": False, + "comment": "@{0} valid".format(self.toddler.pagure_user), + } + ], + "full_url": "https://blacklibrary.wh40k", + "content": '{"action": "new_repo"}', + } + self.toddler.pagure_io.get_project_contributors.return_value = { + "users": {"admin": ["Lucius"], "commit": [], "ticket": []} + } + + self.toddler.process_comment(issue) + + self.toddler.pagure_io.add_comment_to_issue.assert_called_with( + issue["id"], + namespace=scm_request_processor.PROJECT_NAMESPACE, + comment=( + "User {0} is not a maintainer of {1}." "Can't validate this request." + ).format(user, scm_request_processor.PROJECT_NAMESPACE), + ) + + +class TestProcessNewRepo: + """ + Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_new_repo` method. """ @@ -576,15 +794,16 @@ class TestCreateNewRepo: self.toddler = scm_request_processor.SCMRequestProcessor() self.toddler.pagure_io = Mock() self.toddler.dist_git = Mock() + self.toddler.ping_comment = "{maintainers}" - def test_create_new_repo_missing_required_key(self): + def test_process_new_repo_missing_required_key(self): """ Assert that ticket will be closed if required key is missing in request. """ issue = { "id": 100, } - self.toddler.create_new_repo(issue, {}) + self.toddler.process_new_repo(issue, {}) self.toddler.pagure_io.close_issue.assert_called_with( 100, @@ -593,7 +812,7 @@ class TestCreateNewRepo: reason="Invalid", ) - def test_create_new_repo_invalid_repo_name(self): + def test_process_new_repo_invalid_repo_name(self): """ Assert that ticket will be closed if provided repository name is invalid. """ @@ -609,7 +828,7 @@ class TestCreateNewRepo: "monitor": "monitor", } - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) error = ( "The repository name is invalid. It must be at least two " @@ -625,196 +844,7 @@ 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}, - "notification": False, - } - ], - } - - 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}, "notification": False} - ], - } - - 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_exception_not_valid_notification_comment_present(self): - """ - Assert that comment will not be added if the toddler already commented on the - ticket. - """ - # Preparation - user = "zlopez" - issue = { - "id": 100, - "user": {"name": user}, - "comments": [ - {"comment": "comment", "user": {"name": user}, "notification": False} - ], - } - - 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": ["Tzeentch"], "commit": [], "ticket": []} - } - self.toddler.pagure_user = user - 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.pagure_io.add_comment_to_issue.assert_not_called() - - def test_create_new_repo_missing_bug_id(self): + def test_process_new_repo_missing_bug_id(self): """ Assert that ticket will be closed if Bugzilla bug id is not provided. """ @@ -830,7 +860,8 @@ class TestCreateNewRepo: "monitor": "monitor", } - self.toddler.create_new_repo(issue, json) + self.toddler.dist_git.get_project.return_value = None + self.toddler.process_new_repo(issue, json) self.toddler.pagure_io.close_issue.assert_called_with( 100, @@ -842,7 +873,7 @@ class TestCreateNewRepo: @patch( "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" ) - def test_create_new_repo_invalid_review_bug(self, mock_validate_review_bug): + def test_process_new_repo_invalid_review_bug(self, mock_validate_review_bug): """ Assert that ticket will be closed if Bugzilla bug is not valid. """ @@ -858,9 +889,10 @@ class TestCreateNewRepo: "monitor": "monitor", } + self.toddler.dist_git.get_project.return_value = None mock_validate_review_bug.side_effect = ValidationError("error") - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) mock_validate_review_bug.assert_called_with( "123", "repo", "rawhide", namespace="rpms", pagure_user="zlopez" @@ -876,12 +908,7 @@ class TestCreateNewRepo: @patch( "toddlers.plugins.scm_request_processor.SCMRequestProcessor.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 - ): + def test_process_new_repo_invalid_epel(self, mock_valid_epel_package): """ Assert that ticket will be closed if repo is invalid EPEL repo. """ @@ -908,14 +935,10 @@ class TestCreateNewRepo: mock_valid_epel_package.return_value = False - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) 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, @@ -923,10 +946,7 @@ class TestCreateNewRepo: reason="Invalid", ) - @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): + def test_process_new_repo_requester_not_in_dist_git(self): """ Assert that ticket will be commented on when requester doesn't have a valid dist git account. @@ -955,11 +975,7 @@ class TestCreateNewRepo: self.toddler.dist_git.user_exists.return_value = False self.toddler.dist_git._pagure_url = "https://src.fedoraproject.org" - 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.process_new_repo(issue, json) self.toddler.dist_git.user_exists.assert_called_with("zlopez") @@ -973,10 +989,7 @@ class TestCreateNewRepo: comment=message, ) - @patch( - "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" - ) - def test_create_new_repo_project_exists(self, mock_validate_review_bug): + def test_process_new_repo_project_exists(self): """ Assert that ticket will be closed when repo already exists in dist git. """ @@ -1003,11 +1016,7 @@ class TestCreateNewRepo: self.toddler.dist_git.get_project.return_value = "project" - 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.process_new_repo(issue, json) self.toddler.dist_git.get_project.assert_called_with(namespace, repo) @@ -1018,10 +1027,7 @@ class TestCreateNewRepo: reason="Invalid", ) - @patch( - "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" - ) - def test_create_new_repo_master_branch(self, mock_validate_review_bug): + def test_process_new_repo_master_branch(self): """ Assert that ticket will be closed when branch is set to master branch. Master branch is no longer allowed. @@ -1048,11 +1054,7 @@ class TestCreateNewRepo: } self.toddler.dist_git.get_project.return_value = None - 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.process_new_repo(issue, json) self.toddler.dist_git.get_project.assert_called_with(namespace, repo) @@ -1063,7 +1065,7 @@ class TestCreateNewRepo: reason="Invalid", ) - def test_create_new_repo_unsupported_namespace(self): + def test_process_new_repo_unsupported_namespace(self): """ Assert that ticket will be closed when requested namespace is not recognized. """ @@ -1087,7 +1089,7 @@ class TestCreateNewRepo: "monitor": monitor, "exception": exception, } - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) error = ( "The requested namespace '{0}' is not recognized. " @@ -1105,14 +1107,15 @@ class TestCreateNewRepo: @pytest.mark.parametrize( "namespace, branch", [ - ("rpms", "rawhide"), + ("tests", "main"), ("container", "rawhide"), ("flatpaks", "stable"), ], ) - def test_create_new_repo_namespaces(self, namespace, branch): + def test_process_new_repo_namespaces(self, namespace, branch): """ - Assert that ticket will be processed when everything is in order and namespace is correct. + Assert that maintainers will be notified when repo is created in + non rpms namespace. """ # Preparation user = "zlopez" @@ -1120,7 +1123,8 @@ class TestCreateNewRepo: "id": 100, "user": {"name": user}, "comments": [ - {"comment": "valid", "user": {"name": user}, "notification": False} + {"notification": False, "comment": "First"}, + {"notification": True}, ], } @@ -1146,57 +1150,43 @@ class TestCreateNewRepo: self.toddler.pagure_io.get_project_contributors.return_value = { "users": {"admin": [user], "commit": [], "ticket": []} } - self.toddler.validation_comment = "valid" # Method to test - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) # asserts - 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 - ) - self.toddler.dist_git.set_monitoring_status.assert_called_with( - namespace, repo, monitor - ) - self.toddler.dist_git.change_project_main_admin.assert_called_with( - namespace, repo, "zlopez" - ) - - message = "The Pagure repository was created at {0}/{1}/{2}".format( - dist_git_url, namespace, repo - ) - - self.toddler.pagure_io.close_issue.assert_called_with( - 100, + self.toddler.pagure_io.add_comment_to_issue.assert_called_with( + issue["id"], namespace=scm_request_processor.PROJECT_NAMESPACE, - message=message, - reason="Processed", + comment="@" + user, ) - def test_create_new_repo_tests_namespace(self): + def test_process_new_repo_ping_comment_exists(self, caplog): """ - Assert that ticket will be processed when everything is in order and namespace - is set to tests. + Assert that maintainers are not notified multiple times. """ # Preparation + caplog.set_level(logging.INFO) user = "zlopez" issue = { "id": 100, "user": {"name": user}, "comments": [ - {"comment": "valid", "user": {"name": user}, "notification": False} + { + "comment": "@" + user, + "notification": False, + } ], } repo = "repo" - branch = "main" - namespace = "tests" + namespace = "rpms" + branch = "rawhide" bug_id = "" action = "new_repo" sls = {branch: "2050-06-01"} monitor = "monitor" - exception = False + exception = True json = { "repo": repo, "branch": branch, @@ -1213,42 +1203,18 @@ class TestCreateNewRepo: 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) + # Method to test + self.toddler.process_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 - ) - self.toddler.dist_git.set_monitoring_status.assert_called_with( - namespace, repo, monitor - ) - self.toddler.dist_git.change_project_main_admin.assert_called_with( - namespace, repo, "zlopez" - ) - - message = "The Pagure repository was created at {0}/{1}/{2}".format( - dist_git_url, namespace, repo - ) - - self.toddler.pagure_io.close_issue.assert_called_with( - 100, - namespace=scm_request_processor.PROJECT_NAMESPACE, - message=message, - reason="Processed", - ) + assert caplog.records[-1].message == "- Ping comment found." @patch("toddlers.plugins.scm_request_processor.bugzilla_system") @patch( "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" ) - def test_create_new_repo_non_default_branch( + def test_process_new_repo_non_default_branch( self, mock_validate_review_bug, mock_bz ): """ @@ -1281,7 +1247,7 @@ class TestCreateNewRepo: self.toddler.dist_git.get_project.return_value = None self.toddler.dist_git._pagure_url = dist_git_url - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) # asserts mock_validate_review_bug.assert_called_with( @@ -1317,7 +1283,7 @@ class TestCreateNewRepo: @patch( "toddlers.plugins.scm_request_processor.SCMRequestProcessor.validate_review_bug" ) - def test_create_new_repo_default_brach(self, mock_validate_review_bug, mock_bz): + def test_process_new_repo_default_brach(self, mock_validate_review_bug, mock_bz): """ Assert that repo will be created with default branch when everything is in order. """ @@ -1346,7 +1312,7 @@ class TestCreateNewRepo: self.toddler.dist_git.get_project.return_value = None self.toddler.dist_git._pagure_url = dist_git_url - self.toddler.create_new_repo(issue, json) + self.toddler.process_new_repo(issue, json) # asserts mock_validate_review_bug.assert_called_with( diff --git a/toddlers.toml.example b/toddlers.toml.example index b604bd3..9d930ca 100644 --- a/toddlers.toml.example +++ b/toddlers.toml.example @@ -249,8 +249,6 @@ 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" diff --git a/toddlers/plugins/scm_request_processor.py b/toddlers/plugins/scm_request_processor.py index d82be43..08929e4 100644 --- a/toddlers/plugins/scm_request_processor.py +++ b/toddlers/plugins/scm_request_processor.py @@ -186,7 +186,10 @@ class SCMRequestProcessor(ToddlerBase): bugzilla_system.set_bz(config) try: - self.process_ticket(issue) + if message.topic.endswith("pagure.issue.comment.added"): + self.process_comment(issue) + else: + self.process_ticket(issue) except BaseException: self.pagure_io.add_comment_to_issue( issue["id"], @@ -196,6 +199,80 @@ class SCMRequestProcessor(ToddlerBase): ).format(traceback.format_exc()), ) + def process_comment(self, issue: dict): + """ + Process comment on the issue. + + Params: + issue: A dictionary containing the issue. + """ + _log.info("Processing comment on pagure ticket '{0}'".format(issue["full_url"])) + + last_comment = issue["comments"][-1] + + # Comment is notification + if last_comment["notification"]: + _log.info("Comment is notification. Ignoring.") + return + # Check if the comment is addressed to bot user + if "@" + self.pagure_user not in last_comment["comment"]: + _log.info("Comment is not for '{0}'. Ignoring.".format(self.pagure_user)) + return + + # Extract command from comment @releng-bot command + command = last_comment["comment"].split()[1] + + if command == "help": + self.pagure_io.add_comment_to_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + comment=( + "{0} recognizes following commands on {1}.\n" + "help - prints this help message\n" + "retry - retries the failed request processing\n" + "valid - can be used by authorized user to mark the request as valid\n" + ).format(self.pagure_user, PROJECT_NAMESPACE), + ) + elif command == "retry": + self.process_ticket(issue) + elif command == "valid": + # Get full list of contributors + 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"]) + ) + if last_comment["user"]["name"] in maintainers: + _log.info( + "Ticket is validated by {}, continue processing".format( + last_comment["user"]["name"] + ) + ) + issue_body_json = json.loads(issue["content"].strip("`").strip("\n")) + if issue_body_json.get("action") == "new_repo": + self._create_new_repo(issue, issue_body_json) + else: + self.pagure_io.add_comment_to_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + comment=( + "Validation can be done for 'new_repo' requests only." + ), + ) + else: + self.pagure_io.add_comment_to_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + comment=( + "User {0} is not a maintainer of {1}." + "Can't validate this request." + ).format(last_comment["user"]["name"], PROJECT_NAMESPACE), + ) + def process_ticket(self, issue: dict): """ Process a single ticket @@ -294,7 +371,7 @@ class SCMRequestProcessor(ToddlerBase): return if issue_body.get("action") == "new_repo": - self.create_new_repo( + self.process_new_repo( issue, issue_body, initial_commit=issue_body.get("initial_commit", True) ) elif issue_body.get("action") == "new_branch": @@ -350,11 +427,11 @@ class SCMRequestProcessor(ToddlerBase): 'The SL "{0}" must expire on June 1st or December 1st'.format(eol) ) - def create_new_repo( + def process_new_repo( self, issue: dict, issue_body_json: dict, initial_commit: bool = True ): """ - A helper function that process request for new repo. + Process request for new repo. Params: issue: the dictionary representing the issue @@ -382,6 +459,23 @@ class SCMRequestProcessor(ToddlerBase): ) return + # Validate the request first + if self._validate_new_repo_request(issue, issue_body_json): + _log.info("Ticket passed all validations. Creating repository.") + self._create_new_repo(issue, issue_body_json, initial_commit) + + def _validate_new_repo_request(self, issue: dict, issue_body_json: dict) -> bool: + """ + Validate request for new repo. + + Params: + issue: the dictionary representing the issue + issue_body_json: a partially validated dictionary of the JSON in the + issue body + + Returns: + True if the request is valid, false otherwise. + """ requester = issue["user"]["name"] namespace = issue_body_json["namespace"].strip() repo = issue_body_json["repo"].strip() @@ -403,7 +497,7 @@ class SCMRequestProcessor(ToddlerBase): message=message, reason="Invalid", ) - return + return False if not re.match(PROJECT_NAME_REGEX, repo): error = ( @@ -418,7 +512,57 @@ class SCMRequestProcessor(ToddlerBase): message=error, reason="Invalid", ) - return + return False + + # This should never trigger because if the user requested an EPEL branch + # here, it'd have to be accompanied by a Bugzilla bug for the "Fedora EPEL" + # product. So this will only trigger if the reviewer made a mistake. + if re.match(EPEL_REGEX, branch_name) and not self.valid_epel_package( + repo, branch_name + ): + self.pagure_io.close_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + message=INVALID_EPEL_ERROR, + reason="Invalid", + ) + return False + + _log.info( + "- Checking if user {0} has an account in dist-git.".format(requester) + ) + if not self.dist_git.user_exists(requester): + sync_comment = ( + "@{0} needs to login to {1} to sync accounts " + "before we can proceed.".format(requester, self.dist_git._pagure_url) + ) + self.pagure_io.add_comment_to_issue( + issue["id"], namespace=PROJECT_NAMESPACE, comment=sync_comment + ) + return False + + _log.info( + "- Checking if {0}/{1} already exists in dist-git.".format(namespace, repo) + ) + project = self.dist_git.get_project(namespace, repo) + if project: + self.pagure_io.close_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + message="The Pagure project already exists", + reason="Invalid", + ) + return False + + # Close the ticket if the requested default branch is master + if branch_name == "master": + self.pagure_io.close_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + message="Branch `master` cannot be created, please request the right branch.", + reason="Invalid", + ) + return False if exception is True or namespace in ( "flatpaks", @@ -436,44 +580,24 @@ class SCMRequestProcessor(ToddlerBase): | set(contributors["users"]["ticket"]) ) # Ping the maintainers if toddler didn't comment on the ticket yet - ping_comment = True - # This is set to true if the validation comment is set by maintainer - valid = False - # There is already a comment on this ticket, we should check if this is - # the validation ticket and if we already responded to the ticket + ping_comment = self.ping_comment.format( + maintainers=" ".join(["@" + user for user in sorted(maintainers)]) + ) if len(issue["comments"]) > 0: - _log.info("- Check for validation comment") + _log.info("- Check for ping comment") for comment in issue["comments"]: # Skip the notification comments if not comment["notification"]: - 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 - # Toddler already commented on the ticket - if comment["user"]["name"] == self.pagure_user: - ping_comment = False - if not valid and not ping_comment: - _log.info("- Ticket is not yet validated. Skipping...") - return - + # We already pinged the maintainers + if comment["comment"].strip() == ping_comment: + _log.info("- Ping comment found.") + return False # We didn't commented on the ticket yet, ping the maintainers - if ping_comment and not valid: - _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 sorted(maintainers)] - ) - ), - ) - return + _log.info("- Notify the responsible users") + self.pagure_io.add_comment_to_issue( + issue["id"], namespace=PROJECT_NAMESPACE, comment=ping_comment + ) + return False else: if not bug_id: self.pagure_io.close_issue( @@ -482,7 +606,7 @@ class SCMRequestProcessor(ToddlerBase): message="An invalid Bugzilla bug was provided", reason="Invalid", ) - return + return False try: _log.info("- Checking that #{0} is a valid RHBZ.".format(bug_id)) self.validate_review_bug( @@ -499,59 +623,30 @@ class SCMRequestProcessor(ToddlerBase): message=str(error), reason="Invalid", ) - return + return False - # This should never trigger because if the user requested an EPEL branch - # here, it'd have to be accompanied by a Bugzilla bug for the "Fedora EPEL" - # product. So this will only trigger if the reviewer made a mistake. - if re.match(EPEL_REGEX, branch_name) and not self.valid_epel_package( - repo, branch_name - ): - self.pagure_io.close_issue( - issue["id"], - namespace=PROJECT_NAMESPACE, - message=INVALID_EPEL_ERROR, - reason="Invalid", - ) - return + return True - _log.info( - "- Checking if user {0} has an account in dist-git.".format(requester) - ) - if not self.dist_git.user_exists(requester): - sync_comment = ( - "@{0} needs to login to {1} to sync accounts " - "before we can proceed.".format(requester, self.dist_git._pagure_url) - ) - self.pagure_io.add_comment_to_issue( - issue["id"], namespace=PROJECT_NAMESPACE, comment=sync_comment - ) - return - - _log.info( - "- Checking if {0}/{1} already exists in dist-git.".format(namespace, repo) - ) - project = self.dist_git.get_project(namespace, repo) - if project: - self.pagure_io.close_issue( - issue["id"], - namespace=PROJECT_NAMESPACE, - message="The Pagure project already exists", - reason="Invalid", - ) - return + def _create_new_repo( + self, issue: dict, issue_body_json: dict, initial_commit: bool = True + ): + """ + Create new repository. + Params: + issue: the dictionary representing the issue + issue_body_json: a partially validated dictionary of the JSON in the + issue body + initial_commit: indicate whether to create an initial commit. + """ + requester = issue["user"]["name"] + namespace = issue_body_json["namespace"].strip() + repo = issue_body_json["repo"].strip() + bug_id = str(issue_body_json["bug_id"]).strip() + branch_name = issue_body_json["branch"].strip() description = issue_body_json.get("description", "").strip() upstreamurl = issue_body_json.get("upstreamurl", "").strip() - # Close the ticket if the requested default branch is master - if branch_name == "master": - self.pagure_io.close_issue( - issue["id"], - namespace=PROJECT_NAMESPACE, - message="Branch `master` cannot be created, please request the right branch.", - reason="Invalid", - ) - return + if namespace in ["rpms", "container"]: default_branch = "rawhide" elif namespace in ["flatpaks"]: @@ -559,8 +654,6 @@ class SCMRequestProcessor(ToddlerBase): elif namespace in ["tests"]: default_branch = "main" - _log.info("Ticket passed all validations. Creating repository.") - # Create the Pagure repo dist_git_url = "{0}/{1}/{2}".format( self.dist_git._pagure_url.rstrip("/"), namespace, repo