From d8822dcfdcebab947c663fdd6c32038213f02ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 13 Jul 2021 11:23:25 +0100 Subject: [PATCH 1/4] Add descriptions to check_source tests --- tests/check_source_tests.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/check_source_tests.py b/tests/check_source_tests.py index 90a789f81..3b5f64748 100644 --- a/tests/check_source_tests.py +++ b/tests/check_source_tests.py @@ -43,6 +43,7 @@ def tearDown(self): del self.wf def test_no_devel_project(self): + """Declines the request when it does not come from a devel project""" req_id = self.wf.create_submit_request(SRC_PROJECT, self.randomString('package')).reqid self.assertReview(req_id, by_user=(self.bot_user, 'new')) @@ -54,6 +55,7 @@ def test_no_devel_project(self): self.assertIn('%s is not a devel project of %s' % (SRC_PROJECT, PROJECT), review.comment) def test_devel_project(self): + """Accepts a request coming from a devel project""" self._setup_devel_project() # Set up the reviewers team From 1e868aa7ee1418b4e448778d3ad6570c65483398 Mon Sep 17 00:00:00 2001 From: Ancor Gonzalez Sosa Date: Wed, 7 Jul 2021 17:17:52 +0200 Subject: [PATCH 2/4] Check for maintainers in the source project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * It introduces a new configuration parameter 'required-source-maintainer'. * If defined, it is expected to be a maintainer of the devel project. * If that's not the case, the request is declined and an add_role request is created. Co-authored-by: Ancor Gonzalez Sosa Co-authored-by: Knut Alejandro Anderssen González --- check_source.py | 38 ++++++++++++++++++++++++++++++++++++ osclib/conf.py | 1 + osclib/core.py | 15 ++++++++++++++ tests/check_source_tests.py | 39 +++++++++++++++++++++++++++++++++---- 4 files changed, 89 insertions(+), 4 deletions(-) diff --git a/check_source.py b/check_source.py index 297011552..605173b40 100755 --- a/check_source.py +++ b/check_source.py @@ -21,6 +21,8 @@ from osclib.core import package_kind from osclib.core import source_file_load from osclib.core import target_archs +from osclib.core import create_add_role_request +from osclib.core import maintainers_get from urllib.error import HTTPError import ReviewBot @@ -52,6 +54,7 @@ def target_project_config(self, project): self.mail_release_list = config.get('mail-release-list') self.staging_group = config.get('staging-group') self.repo_checker = config.get('repo-checker') + self.required_maintainer = config.get('required-source-maintainer', '') self.devel_whitelist = config.get('devel-whitelist', '').split() self.skip_add_reviews = False self.security_review_team = config.get('security-review-team', 'security-team') @@ -159,6 +162,25 @@ def check_source_submission(self, source_project, source_package, source_revisio if match: inair_renamed = target_package != match.group(1) + if not self.source_has_correct_maintainers(source_project): + declined_msg = ( + 'This request cannot be accepted unless %s is a maintainer of %s.' % + (self.required_maintainer, source_project) + ) + + try: + add_role_msg = 'Created automatically from request %s' % self.request.reqid + add_role_reqid = create_add_role_request(self.apiurl, source_project, self.required_maintainer, + 'maintainer', message=add_role_msg) + declined_msg += ' Created the add_role request %s for addressing this problem.' % add_role_reqid + except HTTPError as e: + self.logger.error( + 'Cannot create the corresponding add_role request for %s: %s' % (self.request.reqid, e) + ) + + self.review_messages['declined'] = declined_msg + return False + if not self.in_air_rename_allow and inair_renamed: self.review_messages['declined'] = 'Source and target package names must match' return False @@ -313,6 +335,22 @@ def is_devel_project(self, source_project, target_project): result = osc.core.search(self.apiurl, **search) return result['package'].attrib['matches'] != '0' + def source_has_correct_maintainers(self, source_project): + """Checks whether the source project has the required maintainer + + If a 'required-source-maintainer' is set, it checks whether it is a + maintainer for the source project. + + source_project - source project name + """ + self.logger.info( + 'Checking required maintainer from the source project (%s)' % self.required_maintainer + ) + if not self.required_maintainer: return True + + maintainers = maintainers_get(self.apiurl, source_project) + return self.required_maintainer in maintainers + @staticmethod def checkout_package(*args, **kwargs): _stdout = sys.stdout diff --git a/osclib/conf.py b/osclib/conf.py index 5c6c09d50..d64b1398f 100644 --- a/osclib/conf.py +++ b/osclib/conf.py @@ -47,6 +47,7 @@ 'mail-noreply': 'noreply@opensuse.org', 'mail-release-list': 'opensuse-releaseteam@opensuse.org', 'always_set_productversion_to': '', + 'required-source-maintainer': 'group:factory-maintainers', }, r'openSUSE:(?PFactory):ARM$': { 'product': 'openSUSE.product', diff --git a/osclib/core.py b/osclib/core.py index af66dbf06..36b553ee3 100644 --- a/osclib/core.py +++ b/osclib/core.py @@ -1098,6 +1098,21 @@ def create_change_devel_request(apiurl, source_project, source_package, tgt_project=target_project, tgt_package=target_package) return create_request(apiurl, action, message) +def create_add_role_request(apiurl, target_project, user, role, target_package=None, message=None): + """Create an add_role request + + user -- user or group name. If it is a group, it should start with 'group:'. + """ + + if user.startswith('group:'): + group = user.replace('group:', '') + kargs = dict(group_name=group, group_role=role) + else: + kargs = dict(person_name=user, person_role=role) + + action = Action('add_role', tgt_project=target_project, tgt_package=target_package, **kargs) + return create_request(apiurl, action, message) + def create_request(apiurl, action, message=None): """Create a request for the given action diff --git a/tests/check_source_tests.py b/tests/check_source_tests.py index 3b5f64748..de2effccb 100644 --- a/tests/check_source_tests.py +++ b/tests/check_source_tests.py @@ -3,11 +3,14 @@ from check_source import CheckSource import random import os +from osclib.core import request_action_list +from osc.core import get_request_list PROJECT = 'openSUSE:Factory' SRC_PROJECT = 'devel:Fishing' FIXTURES = os.path.join(os.getcwd(), 'tests/fixtures') REVIEW_TEAM = 'reviewers-team' +FACTORY_MAINTAINERS = 'group:factory-maintainers' # NOTE: Since there is no documentation explaining the good practices for creating tests for a # review bot, this test is created by mimicking parts of other existing tests. Most decisions are @@ -22,6 +25,14 @@ def setUp(self): # Using OBSLocal.StagingWorkflow makes it easier to setup testing scenarios self.wf = OBSLocal.StagingWorkflow(PROJECT) + + # Set up the reviewers team + self.wf.create_group(REVIEW_TEAM) + + self.wf.remote_config_set( + { 'required-source-maintainer': 'Admin', 'review-team': REVIEW_TEAM } + ) + self.bot_user = 'factory-auto' self.wf.create_user(self.bot_user) self.project = self.wf.create_project(PROJECT) @@ -58,10 +69,6 @@ def test_devel_project(self): """Accepts a request coming from a devel project""" self._setup_devel_project() - # Set up the reviewers team - self.wf.create_group(REVIEW_TEAM) - self.wf.remote_config_set({ 'review-team': REVIEW_TEAM }, replace_all=False) - req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish').reqid self.assertReview(req_id, by_user=(self.bot_user, 'new')) @@ -72,6 +79,30 @@ def test_devel_project(self): self.assertReview(req_id, by_user=(self.bot_user, 'accepted')) self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new')) + def test_source_maintainer(self): + """Declines the request when the 'required_maintainer' is not maintainer of the source project""" + self._setup_devel_project() + + # Change the required maintainer + self.wf.create_group(FACTORY_MAINTAINERS.replace('group:', '')) + self.wf.remote_config_set({ 'required-source-maintainer': FACTORY_MAINTAINERS }) + + req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish').reqid + + self.assertReview(req_id, by_user=(self.bot_user, 'new')) + + self.review_bot.set_request_ids([req_id]) + self.review_bot.check_requests() + + review = self.assertReview(req_id, by_user=(self.bot_user, 'declined')) + add_role_req = get_request_list(self.wf.apiurl, SRC_PROJECT, req_state=['new'], req_type='add_role')[0] + + self.assertIn('unless %s is a maintainer of %s' % (FACTORY_MAINTAINERS, SRC_PROJECT), review.comment) + self.assertIn('Created the add_role request %s' % add_role_req.reqid, review.comment) + + self.assertEqual(add_role_req.actions[0].tgt_project, SRC_PROJECT) + self.assertEqual('Created automatically from request %s' % req_id, add_role_req.description) + def _setup_devel_project(self): devel_project = self.wf.create_project(SRC_PROJECT) devel_package = OBSLocal.Package('blowfish', project=devel_project) From 632cbf92bc6fd7e73b8900e72a2edcb0f994234f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Tue, 13 Jul 2021 16:30:53 +0100 Subject: [PATCH 3/4] Updates from code review --- osclib/core.py | 2 +- tests/check_source_tests.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/osclib/core.py b/osclib/core.py index 36b553ee3..bf89bd5e1 100644 --- a/osclib/core.py +++ b/osclib/core.py @@ -1101,7 +1101,7 @@ def create_change_devel_request(apiurl, source_project, source_package, def create_add_role_request(apiurl, target_project, user, role, target_package=None, message=None): """Create an add_role request - user -- user or group name. If it is a group, it should start with 'group:'. + user -- user or group name. If it is a group, it has to start with 'group:'. """ if user.startswith('group:'): diff --git a/tests/check_source_tests.py b/tests/check_source_tests.py index de2effccb..853edf152 100644 --- a/tests/check_source_tests.py +++ b/tests/check_source_tests.py @@ -8,7 +8,7 @@ PROJECT = 'openSUSE:Factory' SRC_PROJECT = 'devel:Fishing' -FIXTURES = os.path.join(os.getcwd(), 'tests/fixtures') +FIXTURES = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'fixtures') REVIEW_TEAM = 'reviewers-team' FACTORY_MAINTAINERS = 'group:factory-maintainers' From 00ea25e609bf8f96e7d4fa96fede868f05f05395 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Wed, 14 Jul 2021 10:45:11 +0100 Subject: [PATCH 4/4] Properly handle required-source-maintainer when it is group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Knut Alejandro Anderssen González --- check_source.py | 9 +++++++-- tests/check_source_tests.py | 24 +++++++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/check_source.py b/check_source.py index 605173b40..157a0bcc4 100755 --- a/check_source.py +++ b/check_source.py @@ -11,6 +11,8 @@ except ImportError: import cElementTree as ET +from lxml import etree as ETL + import osc.conf import osc.core from osc.util.helper import decode_list @@ -22,7 +24,7 @@ from osclib.core import source_file_load from osclib.core import target_archs from osclib.core import create_add_role_request -from osclib.core import maintainers_get +from osc.core import show_project_meta from urllib.error import HTTPError import ReviewBot @@ -348,7 +350,10 @@ def source_has_correct_maintainers(self, source_project): ) if not self.required_maintainer: return True - maintainers = maintainers_get(self.apiurl, source_project) + meta = ETL.fromstringlist(show_project_meta(self.apiurl, source_project)) + maintainers = meta.xpath('//person[@role="maintainer"]/@userid') + maintainers += ['group:' + g for g in meta.xpath('//group[@role="maintainer"]/@groupid')] + return self.required_maintainer in maintainers @staticmethod diff --git a/tests/check_source_tests.py b/tests/check_source_tests.py index 853edf152..bd4919c4b 100644 --- a/tests/check_source_tests.py +++ b/tests/check_source_tests.py @@ -79,7 +79,7 @@ def test_devel_project(self): self.assertReview(req_id, by_user=(self.bot_user, 'accepted')) self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new')) - def test_source_maintainer(self): + def test_no_source_maintainer(self): """Declines the request when the 'required_maintainer' is not maintainer of the source project""" self._setup_devel_project() @@ -103,8 +103,26 @@ def test_source_maintainer(self): self.assertEqual(add_role_req.actions[0].tgt_project, SRC_PROJECT) self.assertEqual('Created automatically from request %s' % req_id, add_role_req.description) - def _setup_devel_project(self): - devel_project = self.wf.create_project(SRC_PROJECT) + def test_source_maintainer(self): + """Accepts the request when the 'required_maintainer' is a group and is a maintainer for the project""" + group_name = FACTORY_MAINTAINERS.replace('group:', '') + self.wf.create_group(group_name) + self.wf.remote_config_set({ 'required-source-maintainer': FACTORY_MAINTAINERS }) + + self._setup_devel_project(maintainer={'groups': [group_name]}) + + req_id = self.wf.create_submit_request(SRC_PROJECT, 'blowfish').reqid + + self.assertReview(req_id, by_user=(self.bot_user, 'new')) + + self.review_bot.set_request_ids([req_id]) + self.review_bot.check_requests() + + self.assertReview(req_id, by_user=(self.bot_user, 'accepted')) + self.assertReview(req_id, by_group=(REVIEW_TEAM, 'new')) + + def _setup_devel_project(self, maintainer={}): + devel_project = self.wf.create_project(SRC_PROJECT, maintainer=maintainer) devel_package = OBSLocal.Package('blowfish', project=devel_project) blowfish_spec = os.path.join(FIXTURES, 'packages', 'blowfish', 'blowfish.spec')