Merge pull request #2126 from jberry-suse/origin-manager-maintenance

origin-manager: handle maintenance workflow at multiple levels

ReviewBot.py

@@ -10,9 +10,11 @@ from collections import OrderedDict
 from osclib.cache import Cache
 from osclib.comments import CommentAPI
 from osclib.conf import Config
+from osclib.core import action_is_patchinfo
 from osclib.core import devel_project_fallback
 from osclib.core import group_members
 from osclib.core import maintainers_get
+from osclib.core import request_action_key
 from osclib.memoize import memoize
 from osclib.memoize import memoize_session_reset
 from osclib.stagingapi import StagingAPI
@@ -303,6 +305,10 @@ class ReviewBot(object):
             self.logger.info('POST %s' % u)
             return
 
+        if self.multiple_actions:
+            key = request_action_key(self.action)
+            msg = yaml.dump({key: msg}, default_flow_style=False)
+
         r = osc.core.http_POST(u, data=msg)
         code = ET.parse(r).getroot().attrib['code']
         if code != 'ok':
@@ -355,18 +361,29 @@ class ReviewBot(object):
         return None if nothing to do, True to accept, False to reject
         """
 
-        # Copy original values to revert changes made to them.
+        if len(req.actions) > 1:
-        self.review_messages = self.DEFAULT_REVIEW_MESSAGES.copy()
+            if self.only_one_action:
+                self.review_messages['declined'] = 'Only one action per request supported'
+                return False
 
-        if self.only_one_action and len(req.actions) != 1:
+            # Will cause added reviews and overall review message to include
-            self.review_messages['declined'] = 'Only one action per request supported'
+            # each actions message prefixed by an action key.
-            return False
+            self.multiple_actions = True
+            review_messages_multi = {}
+        else:
+            self.multiple_actions = False
+
+            # Copy original values to revert changes made to them.
+            self.review_messages = self.DEFAULT_REVIEW_MESSAGES.copy()
 
         if self.comment_handler is not False:
             self.comment_handler_add()
 
         overall = True
         for a in req.actions:
+            if self.multiple_actions:
+                self.review_messages = self.DEFAULT_REVIEW_MESSAGES.copy()
+
             # Store in-case sub-classes need direct access to original values.
             self.action = a
 
@@ -380,6 +397,19 @@ class ReviewBot(object):
                     (overall is None and ret is False)):
                     overall = ret
 
+            if self.multiple_actions and ret is not None:
+                key = request_action_key(a)
+                message_key = self.review_message_key(ret)
+                review_messages_multi[key] = self.review_messages[message_key]
+
+        message_key = self.review_message_key(overall)
+        if self.multiple_actions:
+            message_combined = yaml.dump(review_messages_multi, default_flow_style=False)
+            self.review_messages[message_key] = message_combined
+        elif type(self.review_messages[message_key]) is dict:
+            self.review_messages[message_key] = yaml.dump(
+                self.review_messages[message_key], default_flow_style=False)
+
         return overall
 
     def action_method(self, action):
@@ -406,12 +436,11 @@ class ReviewBot(object):
         method_type = '_default'
         return '_'.join([method_prefix, method_type])
 
-    @staticmethod
+    def review_message_key(self, result):
-    def _is_patchinfo(pkgname):
+        return 'accepted' if result else 'declined'
-        return pkgname == 'patchinfo' or pkgname.startswith('patchinfo.')
 
     def check_action_maintenance_incident(self, req, a):
-        if self._is_patchinfo(a.src_package):
+        if action_is_patchinfo(a):
             self.logger.debug('ignoring patchinfo action')
             return True
 
@@ -434,7 +463,7 @@ class ReviewBot(object):
 
     def check_action_maintenance_release(self, req, a):
         pkgname = a.src_package
-        if self._is_patchinfo(pkgname):
+        if action_is_patchinfo(a):
             self.logger.debug('ignoring patchinfo action')
             return True
 

check_maintenance_incidents.py

@@ -17,6 +17,7 @@ from urllib.error import HTTPError, URLError
 import yaml
 
 from osclib.memoize import memoize
+from osclib.core import action_is_patchinfo
 from osclib.core import owner_fallback
 from osclib.core import maintainers_get
 
@@ -33,7 +34,7 @@ class MaintenanceChecker(ReviewBot.ReviewBot):
     def add_devel_project_review(self, req, package):
         """ add devel project/package as reviewer """
         a = req.actions[0]
-        if self._is_patchinfo(a.src_package):
+        if action_is_patchinfo(a):
             a = req.actions[1]
         project = a.tgt_releaseproject if a.type == 'maintenance_incident' else req.actions[0].tgt_project
         root = owner_fallback(self.apiurl, project, package)
@@ -71,7 +72,7 @@ class MaintenanceChecker(ReviewBot.ReviewBot):
             (linkprj, linkpkg) = self._get_linktarget(a.src_project, pkgname)
             if linkpkg is not None:
                 pkgname = linkpkg
-            if self._is_patchinfo(a.src_package):
+            if action_is_patchinfo(a):
                 return None
 
             project = a.tgt_releaseproject

origin-manager.py

@@ -18,6 +18,10 @@ class OriginManager(ReviewBot.ReviewBot):
         self.override_allow = False
 
     def check_action_delete_package(self, request, action):
+        advance, result = self.config_validate(action.tgt_project)
+        if not advance:
+            return result
+
         origin_info_old = origin_find(self.apiurl, action.tgt_project, action.tgt_package)
 
         reviews = {'fallback': 'Delete requests require fallback review.'}
@@ -27,8 +31,9 @@ class OriginManager(ReviewBot.ReviewBot):
         return True
 
     def check_source_submission(self, src_project, src_package, src_rev, tgt_project, tgt_package):
-        if not self.config_validate(tgt_project):
+        advance, result = self.config_validate(tgt_project)
-            return False
+        if not advance:
+            return result
 
         source_hash_new = package_source_hash(self.apiurl, src_project, src_package, src_rev)
         origin_info_new = origin_find(self.apiurl, tgt_project, tgt_package, source_hash_new)
@@ -44,17 +49,22 @@ class OriginManager(ReviewBot.ReviewBot):
     def config_validate(self, target_project):
         config = config_load(self.apiurl, target_project)
         if not config:
-            self.review_messages['declined'] = 'OSRT:OriginConfig attribute missing'
+            # No perfect solution for lack of a config. For normal projects a
-            return False
+            # decline seems best, but in the event of failure to return proper
+            # config no good behavior. For maintenance the situation is further
+            # complicated since multiple actions some of which are not intended
+            # to be reviewed, but not always guaranteed to see multiple actions.
+            self.review_messages['accepted'] = 'skipping since no OSRT:OriginConfig'
+            return False, True
         if not config.get('fallback-group'):
             self.review_messages['declined'] = 'OSRT:OriginConfig.fallback-group missing'
-            return False
+            return False, False
         if not self.dryrun and config['review-user'] != self.review_user:
             self.logger.warning(
                 'OSRT:OriginConfig.review-user ({}) does not match ReviewBot.review_user ({})'.format(
                     config['review-user'], self.review_user))
 
-        return True
+        return True, True
 
     def policy_result_handle(self, project, package, origin_info_new, origin_info_old, result):
         self.policy_result_reviews_add(project, package, result.reviews, origin_info_new, origin_info_old)
@@ -66,11 +76,12 @@ class OriginManager(ReviewBot.ReviewBot):
             override = self.request_override_check(self.request, True)
             if override:
                 self.review_messages['accepted'] = origin_annotation_dump(
-                    origin_info_new, origin_info_old, self.review_messages['accepted'])
+                    origin_info_new, origin_info_old, self.review_messages['accepted'], raw=True)
                 return override
         else:
             if result.accept:
-                self.review_messages['accepted'] = origin_annotation_dump(origin_info_new, origin_info_old)
+                self.review_messages['accepted'] = origin_annotation_dump(
+                    origin_info_new, origin_info_old, raw=True)
             return result.accept
 
         return None

osclib/core.py

@@ -16,16 +16,17 @@ except ImportError:
 from osc.core import get_binarylist
 from osc.core import get_commitlog
 from osc.core import get_dependson
+from osc.core import get_request_list
 from osc.core import http_GET
 from osc.core import http_POST
 from osc.core import http_PUT
 from osc.core import makeurl
 from osc.core import owner
 from osc.core import Request
-from osc.core import search
 from osc.core import show_package_meta
 from osc.core import show_project_meta
 from osc.core import show_results_meta
+from osc.core import xpath_join
 from osc.util.helper import decode_it
 from osclib.conf import Config
 from osclib.memoize import memoize
@@ -338,9 +339,12 @@ def attribute_value_load(apiurl, project, name, namespace='OSRT'):
 
         raise e
 
-    value = root.xpath(
+    xpath_base = './attribute[@namespace="{}" and @name="{}"]'.format(namespace, name)
-        './attribute[@namespace="{}" and @name="{}"]/value/text()'.format(namespace, name))
+    value = root.xpath('{}/value/text()'.format(xpath_base))
     if not len(value):
+        if root.xpath(xpath_base):
+            # Handle boolean attributes that are present, but have no value.
+            return True
         return None
 
     return str(value[0])
@@ -653,7 +657,7 @@ def project_attribute_list(apiurl, attribute, value=None):
     if value is not None:
         xpath += '="{}"'.format(value)
 
-    root = search(apiurl, project=xpath)['project']
+    root = search(apiurl, 'project', xpath)
     for project in root.findall('project'):
         yield project.get('name')
 
@@ -661,7 +665,7 @@ def project_attribute_list(apiurl, attribute, value=None):
 def project_remote_list(apiurl):
     remotes = {}
 
-    root = search(apiurl, project='starts-with(remoteurl, "http")')['project']
+    root = search(apiurl, 'project', 'starts-with(remoteurl, "http")')
     for project in root.findall('project'):
         # Strip ending /public as the only use-cases for manually checking
         # remote projects is to query them directly to use an API that does not
@@ -763,3 +767,156 @@ def duplicated_binaries_in_repo(apiurl, project, repository):
             duplicates[arch][name] = list(duplicates[arch][name])
 
     return duplicates
+
+# osc.core.search() is over-complicated and does not return lxml element.
+def search(apiurl, path, xpath, query={}):
+    query['match'] = xpath
+    url = makeurl(apiurl, ['search', path], query)
+    return ETL.parse(http_GET(url)).getroot()
+
+def action_is_patchinfo(action):
+    return (action.type == 'maintenance_incident' and (
+        action.src_package == 'patchinfo' or action.src_package.startswith('patchinfo.')))
+
+def request_action_key(action):
+    identifier = []
+
+    if action.type in ['add_role', 'change_devel', 'maintenance_release', 'submit']:
+        identifier.append(action.tgt_project)
+        identifier.append(action.tgt_package)
+
+        if action.type in ['add_role', 'set_bugowner']:
+            if action.person_name is not None:
+                identifier.append(action.person_name)
+                if action.type == 'add_role':
+                    identifier.append(action.person_role)
+            else:
+                identifier.append(action.group_name)
+                if action.type == 'add_role':
+                    identifier.append(action.group_role)
+    elif action.type == 'delete':
+        identifier.append(action.tgt_project)
+        if action.tgt_package is not None:
+            identifier.append(action.tgt_package)
+        elif action.tgt_repository is not None:
+            identifier.append(action.tgt_repository)
+    elif action.type == 'maintenance_incident':
+        if not action_is_patchinfo(action):
+            identifier.append(action.tgt_releaseproject)
+        identifier.append(action.src_package)
+
+    return '::'.join(['/'.join(identifier), action.type])
+
+def request_action_list_maintenance_incident(apiurl, project, package, states=['new', 'review']):
+    # The maintenance workflow seems to be designed to be as difficult to find
+    # requests as possible. As such, in order to find incidents for a given
+    # target project one must search for the requests in two states: before and
+    # after being assigned to an incident project. Additionally, one must search
+    # the "maintenance projects" denoted by an attribute instead of the actual
+    # target project. To make matters worse the actual target project of the
+    # request is not accessible via search (ie. action/target/releaseproject)
+    # so it must be checked client side. Lastly, since multiple actions are also
+    # designed completely wrong one must loop over the actions and recheck the
+    # search parameters to figure out which action caused the request to be
+    # included in the search results. Overall, another prime example of design
+    # done completely and utterly wrong.
+
+    package_repository = '{}.{}'.format(package, project.replace(':', '_'))
+
+    # Loop over all maintenance projects and create selectors for the two
+    # request states for the given project.
+    xpath = ''
+    for maintenance_project in project_attribute_list(apiurl, 'OBS:MaintenanceProject'):
+        xpath_project = ''
+
+        # Before being assigned to an incident.
+        xpath_project = xpath_join(xpath_project, 'action/target/@project="{}"'.format(
+            maintenance_project))
+        xpath_project = xpath_join(xpath_project, 'action/source/@package="{}"'.format(package), op='and', inner=True)
+
+        xpath = xpath_join(xpath, xpath_project, op='or', nexpr_parentheses=True)
+        xpath_project = ''
+
+        # After being assigned to an incident.
+        xpath_project = xpath_join(xpath_project, 'starts-with(action/target/@project,"{}:")'.format(
+            maintenance_project))
+        xpath_project = xpath_join(xpath_project, 'action/target/@package="{}"'.format(
+            package_repository), op='and', inner=True)
+
+        xpath = xpath_join(xpath, xpath_project, op='or', nexpr_parentheses=True)
+
+    xpath = '({})'.format(xpath)
+
+    if not 'all' in states:
+        xpath_states = ''
+        for state in states:
+            xpath_states = xpath_join(xpath_states, 'state/@name="{}"'.format(state), inner=True)
+        xpath = xpath_join(xpath, xpath_states, op='and', nexpr_parentheses=True)
+
+    xpath = xpath_join(xpath, 'action/@type="maintenance_incident"', op='and')
+
+    root = search(apiurl, 'request', xpath)
+    for request_element in root.findall('request'):
+        request = Request()
+        request.read(request_element)
+
+        for action in request.actions:
+            if action.type == 'maintenance_incident' and action.tgt_releaseproject == project and (
+                (action.tgt_package is None and action.src_package == package) or
+                (action.tgt_package == package_repository)):
+                yield request, action
+                break
+
+def request_action_list_maintenance_release(apiurl, project, package, states=['new', 'review']):
+    package_repository = '{}.{}'.format(package, project.replace(':', '_'))
+
+    xpath = 'action/target/@project="{}"'.format(project)
+    xpath = xpath_join(xpath, 'action/source/@package="{}"'.format(package_repository), op='and', inner=True)
+    xpath = '({})'.format(xpath)
+
+    if not 'all' in states:
+        xpath_states = ''
+        for state in states:
+            xpath_states = xpath_join(xpath_states, 'state/@name="{}"'.format(state), inner=True)
+        xpath = xpath_join(xpath, xpath_states, op='and', nexpr_parentheses=True)
+
+    xpath = xpath_join(xpath, 'action/@type="maintenance_release"', op='and')
+
+    root = search(apiurl, 'request', xpath)
+    for request_element in root.findall('request'):
+        request = Request()
+        request.read(request_element)
+
+        for action in request.actions:
+            if (action.type == 'maintenance_release' and
+                action.tgt_project == project and action.src_package == package_repository):
+                yield request, action
+                break
+
+def request_action_single_list(apiurl, project, package, states, request_type):
+    # TODO To be consistent this should not include request source from project.
+    for request in get_request_list(apiurl, project, package, None, states, request_type):
+        if len(request.actions) > 1:
+            raise Exception('request {} has more than one action'.format(request.reqid))
+
+        yield request, request.actions[0]
+
+def request_action_list(apiurl, project, package, states=['new', 'review'], types=['submit']):
+    for request_type in types:
+        if request_type == 'maintenance_incident':
+            yield from request_action_list_maintenance_incident(apiurl, project, package, states)
+        if request_type == 'maintenance_release':
+            yield from request_action_list_maintenance_release(apiurl, project, package, states)
+        else:
+            yield from request_action_single_list(apiurl, project, package, states, request_type)
+
+def request_action_list_source(apiurl, project, package, states=['new', 'review'], include_release=False):
+    types = []
+    if attribute_value_load(apiurl, project, 'Maintained', 'OBS'):
+        types.append('maintenance_incident')
+        if include_release:
+            types.append('maintenance_release')
+    else:
+        types.append('submit')
+
+    yield from request_action_list(apiurl, project, package, states, types)

osclib/origin.py

@@ -10,9 +10,11 @@ from osclib.core import package_source_hash
 from osclib.core import package_source_hash_history
 from osclib.core import package_version
 from osclib.core import project_remote_apiurl
+from osclib.core import request_action_key
+from osclib.core import request_action_list_source
+from osclib.core import request_remote_identifier
 from osclib.core import review_find_last
 from osclib.core import reviews_remaining
-from osclib.core import request_remote_identifier
 from osclib.memoize import memoize
 from osclib.util import project_list_family
 from osclib.util import project_list_family_prior_pattern
@@ -245,17 +247,17 @@ def project_source_contain(apiurl, project, package, source_hash):
 
 def project_source_pending(apiurl, project, package, source_hash):
     apiurl_remote, project_remote = project_remote_apiurl(apiurl, project)
-    requests = get_request_list(apiurl_remote, project_remote, package, None, ['new', 'review'], 'submit')
+    request_actions = request_action_list_source(apiurl_remote, project_remote, package,
-    for request in requests:
+                                                 states=['new', 'review'], include_release=True)
-        for action in request.actions:
+    for request, action in request_actions:
-            source_hash_consider = package_source_hash(
+        source_hash_consider = package_source_hash(
-                apiurl_remote, action.src_project, action.src_package, action.src_rev)
+            apiurl_remote, action.src_project, action.src_package, action.src_rev)
 
-            project_source_log('pending', project, source_hash_consider, source_hash)
+        project_source_log('pending', project, source_hash_consider, source_hash)
-            if source_hash_consider == source_hash:
+        if source_hash_consider == source_hash:
-                return PendingRequestInfo(
+            return PendingRequestInfo(
-                    request_remote_identifier(apiurl, apiurl_remote, request.reqid),
+                request_remote_identifier(apiurl, apiurl_remote, request.reqid),
-                    reviews_remaining(request))
+                reviews_remaining(request))
 
     return False
 
@@ -267,13 +269,12 @@ def project_source_log(key, project, source_hash_consider, source_hash):
 def origin_find_fallback(apiurl, target_project, package, source_hash, user):
     # Search accepted requests (newest to oldest), find the last review made by
     # the specified user, load comment as annotation, and extract origin.
-    requests = get_request_list(apiurl, target_project, package, None, ['accepted'], 'submit')
+    request_actions = request_action_list_source(apiurl, target_project, package, states=['accepted'])
-    for request in sorted(requests, key=lambda r: r.reqid, reverse=True):
+    for request, action in sorted(request_actions, key=lambda i: i[0].reqid, reverse=True):
-        review = review_find_last(request, user)
+        annotation = origin_annotation_load(request, action, user)
-        if not review:
+        if not annotation:
             continue
 
-        annotation = origin_annotation_load(review.comment)
         return OriginInfo(annotation.get('origin'), False)
 
     # Fallback to searching workaround project.
@@ -296,7 +297,7 @@ def origin_find_fallback(apiurl, target_project, package, source_hash, user):
 
     return None
 
-def origin_annotation_dump(origin_info_new, origin_info_old, override=False):
+def origin_annotation_dump(origin_info_new, origin_info_old, override=False, raw=False):
     data = {'origin': str(origin_info_new.project)}
     if origin_info_old and origin_info_new.project != origin_info_old.project:
         data['origin_old'] = str(origin_info_old.project)
@@ -305,12 +306,36 @@ def origin_annotation_dump(origin_info_new, origin_info_old, override=False):
         data['origin'] = origin_workaround_ensure(data['origin'])
         data['comment'] = override
 
+    if raw:
+        return data
+
     return yaml.dump(data, default_flow_style=False)
 
-def origin_annotation_load(annotation):
+def origin_annotation_load(request, action, user):
-    # For some reason OBS insists on indenting every subsequent line which
+    review = review_find_last(request, user)
-    # screws up yaml parsing since indentation has meaning.
+    if not review:
-    return yaml.safe_load(re.sub(r'^\s+', '', annotation, flags=re.MULTILINE))
+        return False
+
+    try:
+        annotation = yaml.safe_load(review.comment)
+    except yaml.scanner.ScannerError as e:
+        # OBS used to prefix subsequent review lines with two spaces. At some
+        # point it was changed to no longer indent, but still need to be able
+        # to load older annotations.
+        comment_stripped = re.sub(r'^  ', '', review.comment, flags=re.MULTILINE)
+        annotation = yaml.safe_load(comment_stripped)
+
+    if not annotation:
+        return None
+
+    if len(request.actions) > 1:
+        action_key = request_action_key(action)
+        if action_key not in annotation:
+            return False
+
+        return annotation[action_key]
+
+    return annotation
 
 def origin_find_highest(apiurl, project, package):
     config = config_load(apiurl, project)
@@ -528,13 +553,9 @@ def origin_potentials(apiurl, target_project, package):
 def origin_history(apiurl, target_project, package, user):
     history = []
 
-    requests = get_request_list(apiurl, target_project, package, None, ['all'], 'submit')
+    request_actions = request_action_list_source(apiurl, target_project, package, states=['all'])
-    for request in sorted(requests, key=lambda r: r.reqid, reverse=True):
+    for request, action in sorted(request_actions, key=lambda i: i[0].reqid, reverse=True):
-        review = review_find_last(request, user)
+        annotation = origin_annotation_load(request, action, user)
-        if not review:
-            continue
-
-        annotation = origin_annotation_load(review.comment)
         if not annotation:
             continue