Merge pull request #1660 from jberry-suse/maintbot-supersede-by-ReviewBot

ReviewBot: add devel_project_review_*() methods adapted from maintbot (and port users)

ReviewBot.py

@@ -29,7 +29,9 @@ from collections import OrderedDict
 from osclib.cache import Cache
 from osclib.comments import CommentAPI
 from osclib.conf import Config
+from osclib.core import devel_project_fallback
 from osclib.core import group_members
+from osclib.core import maintainers_get
 from osclib.memoize import memoize
 from osclib.memoize import memoize_session_reset
 from osclib.stagingapi import StagingAPI
@@ -307,6 +309,36 @@ class ReviewBot(object):
         if code != 'ok':
             raise Exception('non-ok return code: {}'.format(code))
 
+    def devel_project_review_add(self, request, project, package, message='adding devel project review'):
+        devel_project, devel_package = devel_project_fallback(self.apiurl, project, package)
+        if not devel_project:
+            self.logger.warning('no devel project found for {}/{}'.format(project, package))
+            return False
+
+        self.add_review(request, by_project=devel_project, by_package=devel_package, msg=message)
+        return True
+
+    def devel_project_review_ensure(self, request, project, package, message='submitter not devel maintainer'):
+        if not self.devel_project_review_needed(request, project, package):
+            self.logger.debug('devel project review not needed')
+            return True
+
+        return self.devel_project_review_add(request, project, package, message)
+
+    def devel_project_review_needed(self, request, project, package):
+        author = request.get_creator()
+        maintainers = set(maintainers_get(self.apiurl, project, package))
+
+        if author in maintainers:
+            return False
+
+        # Carried over from maintbot, but seems haphazard.
+        for review in request.reviews:
+            if review.by_user in maintainers:
+                return False
+
+        return True
+
     def check_one_request(self, req):
         """
         check all actions in one request.

check_source.py

@@ -18,7 +18,6 @@ from osclib.core import devel_project_get
 from osclib.core import devel_project_fallback
 import urllib2
 import ReviewBot
-from check_maintenance_incidents import MaintenanceChecker
 from osclib.conf import str2bool
 
 class CheckSource(ReviewBot.ReviewBot):
@@ -31,8 +30,6 @@ class CheckSource(ReviewBot.ReviewBot):
         # ReviewBot options.
         self.request_default_return = True
 
-        self.maintbot = MaintenanceChecker(*args, **kwargs)
-
         self.skip_add_reviews = False
 
     def target_project_config(self, project):
@@ -286,8 +283,7 @@ class CheckSource(ReviewBot.ReviewBot):
         links = root.findall('sourceinfo/linked')
         if links is None or len(links) == 0:
             if not self.ignore_devel:
-                # Utilize maintbot to add devel project review if necessary.
+                self.devel_project_review_ensure(request, action.tgt_project, action.tgt_package)
-                self.maintbot.check_one_request(request)
 
             if not self.skip_add_reviews and self.repo_checker is not None:
                 self.add_review(self.request, by_user=self.repo_checker, msg='Is this delete request safe?')

dist/package/openSUSE-release-tools.spec

@@ -133,8 +133,6 @@ Group:          Development/Tools/Other
 BuildArch:      noarch
 Requires:       %{name} = %{version}
 Requires:       osclib = %{version}
-# osrt-leaper-review needs check_maintenance_incidents.py
-Requires:       %{name}-maintenance = %{version}
 Requires(pre):  shadow
 
 %description leaper

leaper.py

@@ -39,7 +39,6 @@ from osclib.core import devel_project_get
 import urllib2
 import yaml
 import ReviewBot
-from check_maintenance_incidents import MaintenanceChecker
 from check_source_in_factory import FactorySourceChecker
 
 class Leaper(ReviewBot.ReviewBot):
@@ -53,7 +52,6 @@ class Leaper(ReviewBot.ReviewBot):
 
         self.do_comments = True
 
-        self.maintbot = MaintenanceChecker(*args, **kwargs)
         # for FactorySourceChecker
         self.factory = FactorySourceChecker(*args, **kwargs)
 
@@ -125,6 +123,26 @@ class Leaper(ReviewBot.ReviewBot):
         return project.startswith(origin)
 
     def check_source_submission(self, src_project, src_package, src_rev, target_project, target_package):
+        ret = self.check_source_submission_inner(src_project, src_package, src_rev, target_project, target_package)
+
+        # The layout of this code is just plain wrong and awkward. What is
+        # really desired a "post check source submission" not
+        # check_one_request() which applies to all action types and all actions
+        # at once. The follow-ups need the same context that the main method
+        # determining to flip the switch had. For maintenance incidents the
+        # ReviewBot code does a fair bit of mangling to the package and project
+        # values passed in which is not present during check_one_request().
+        # Currently the only feature used by maintenance is
+        # do_check_maintainer_review and the rest of the checks apply in the
+        # single action workflow so this should work fine, but really all the
+        # processing should be done per-action instead of per-request.
+
+        if self.do_check_maintainer_review:
+            self.devel_project_review_ensure(self.request, target_project, target_package)
+
+        return ret
+
+    def check_source_submission_inner(self, src_project, src_package, src_rev, target_project, target_package):
         super(Leaper, self).check_source_submission(src_project, src_package, src_rev, target_project, target_package)
         self.automatic_submission = False
 
@@ -228,6 +246,9 @@ class Leaper(ReviewBot.ReviewBot):
             self.logger.info("expected origin is '%s' (%s)", origin,
                              "unchanged" if origin_same else "changed")
 
+            # Only when not from current product should request require maintainer review.
+            self.do_check_maintainer_review = False
+
             if origin_same:
                 return True
 
@@ -240,6 +261,8 @@ class Leaper(ReviewBot.ReviewBot):
                 self.logger.info('found pending submission against origin ({})'.format(origin))
                 return good
 
+            self.do_check_maintainer_review = True
+
             return None
         elif self.action.type == 'maintenance_incident':
             self.logger.debug('unhandled incident pattern (targetting non :Update project)')
@@ -478,9 +501,6 @@ class Leaper(ReviewBot.ReviewBot):
         self.packages = {}
 
         request_ok = ReviewBot.ReviewBot.check_one_request(self, req)
-        if self.do_check_maintainer_review:
-            has_correct_maintainer = self.maintbot.check_one_request(req)
-            self.logger.debug("has_correct_maintainer: %s", has_correct_maintainer)
 
         self.logger.debug("review result: %s", request_ok)
         if self.pending_factory_submission: