Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for maintainers in the source project #2601

Merged
merged 4 commits into from
Jul 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions check_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
except ImportError:
import cElementTree as ET

from lxml import etree as ETL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need two XML libraries in here now? we have xml.etree(or cElementTree) and lxml both imported (works, of course, but…)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually cElementTree is supposed to die, but we can do that refactoring later IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure - no problem in going with what we have now


import osc.conf
import osc.core
from osc.util.helper import decode_list
Expand All @@ -21,6 +23,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 osc.core import show_project_meta
from urllib.error import HTTPError

import ReviewBot
Expand Down Expand Up @@ -52,6 +56,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')
Expand Down Expand Up @@ -159,6 +164,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
Expand Down Expand Up @@ -313,6 +337,25 @@ 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

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
def checkout_package(*args, **kwargs):
_stdout = sys.stdout
Expand Down
1 change: 1 addition & 0 deletions osclib/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:(?P<project>Factory):ARM$': {
'product': 'openSUSE.product',
Expand Down
15 changes: 15 additions & 0 deletions osclib/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 has to 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

Expand Down
63 changes: 57 additions & 6 deletions tests/check_source_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
FIXTURES = os.path.join(os.path.dirname(os.path.abspath(__file__)), '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
Expand All @@ -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)
Expand All @@ -43,6 +54,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'))
Expand All @@ -54,11 +66,50 @@ 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
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'))

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 test_no_source_maintainer(self):
"""Declines the request when the 'required_maintainer' is not maintainer of the source project"""
imobachgs marked this conversation as resolved.
Show resolved Hide resolved
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 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

Expand All @@ -70,8 +121,8 @@ 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 _setup_devel_project(self):
devel_project = self.wf.create_project(SRC_PROJECT)
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')
Expand Down