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

Define comment commands and provide for ReviewBot review overrides and repo-checker skip-cycle. #1427

Merged
merged 6 commits into from Mar 7, 2018

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Mar 5, 2018

  • 68b93e1:
    repo_checker: provide comment command to skip-cycle for group.

    For example (on staging project):

    @repo-checker skip-cycle
    
  • 4f4e6da:
    repo_checker: utilize ReviewBot.comment_api instead of new instance.

  • e775d44:
    ReviewBot: provide comment command override.

    For example:

    @leaper override accept
    @repo-checker override decline
    
  • 3674191:
    osclib/core: maintainers_get(): utilize new group_members() function.

  • e2efbf5:
    osclib/core: provide group_members() function.

  • eb19b82:
    osclib/comments: provide command_find() for comment commands.

Fixes #1400.
Fixes #1365.

Should partial work towards #1181, but would likely need to scan declined reviews to see if re-open or change workflow for bots like factory-auto.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 5, 2018

This allows members of staging-group or tool-maintainer-group, but perhaps the later is note desired

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 5, 2018

The comment for skip-cycle is to be placed on staging project just like repo-checker comment. The individual request overrides on the requests themselves (for any bot).

Disable for legal-auto unless we want to keep since presumably everything is to be handled in the legaldb.

@jberry-suse jberry-suse force-pushed the jberry-suse:ReviewBot-comment-override branch from d5c7273 to 7161a06 Mar 5, 2018
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 5, 2018

Resolved test failures, by disabling override_allow during test.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 5, 2018

The comment can be placed within a larger comment as long as the command is on it's own line.

Example:

Leaper is confused by XYZ and we should really do this differently, but accept for now.

$ osrt leaper override accept
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 5, 2018

Coverage Status

Coverage decreased (-0.01%) to 30.636% when pulling 7161a06 on jberry-suse:ReviewBot-comment-override into bac6644 on openSUSE:master.

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Mar 6, 2018

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Mar 6, 2018

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 6, 2018

What's the use case of the latter?

Having a technical group familiar with bot issues able to override was the idea.

Would it make sense to be able to define a group per bot instead? Ie
leaper overrides are policy decisions (release management) whereas
repochecker is purely technical (release engineering).

Perhaps, although I imagine we have less than half a dozen potential unique members of all groups. I am not sure if the ideal scenario is for SLE differs or the same setup. Presumably they do not have the tools group at all currently.

What about using "@leaper override accept" instead? The shell style
is geeky for sure, if we were talking to a human one would use the @
style to address the other person though. So I think that would be
more natural and requires less syntax to remember.

I went back and forth on a number of styles. The original thinking was having a generic syntax that wasn't tide to bots, but obviously that is our first usage. Currently the code uses the bot_name which is based off the class to determine if commands for that bot. This works in the case where the bot is run as a different user (like locally) although that may or may not always be desired. I suppose at run-time the bot does know which user it is running as and could look for references to itself.

The idea behind the non-bot targeted commands was thinking about staging instructions or similar that should effect anyone running osc staging commands, but perhaps that should be done differently if ever.

@jberry-suse jberry-suse force-pushed the jberry-suse:ReviewBot-comment-override branch from 7161a06 to 68b93e1 Mar 6, 2018
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 6, 2018

Reworked:

  • provide <botname>-override-group config key in addition to the staging-group. For example leaper-override-group is set to leap-reviewers.
  • changed command syntax to @user ...
  • dropped release-tools-maintainers group from overrides
diff --git a/ReviewBot.py b/ReviewBot.py
index 3e6be99..c515557 100644
--- a/ReviewBot.py
+++ b/ReviewBot.py
@@ -86,6 +86,7 @@ class ReviewBot(object):
         self.request_default_return = None
         self.comment_handler = False
         self.override_allow = True
+        self.override_group_key = '{}-override-group'.format(self.bot_name.lower())
 
         self.load_config()
 
@@ -170,9 +171,10 @@ class ReviewBot(object):
 
         users = group_members(self.apiurl, config['staging-group'])
 
-        tool_group = config.get('tool-maintainer-group')
-        if tool_group:
-            users += group_members(self.apiurl, tool_group, maintainers=True)
+        if self.override_group_key:
+            override_group = config.get(self.override_group_key)
+            if override_group:
+                users += group_members(self.apiurl, override_group)
 
         return users
 
@@ -184,13 +186,15 @@ class ReviewBot(object):
         comments = self.comment_api.get_comments(request_id=request.reqid)
         users = self.request_override_check_users(request.actions[0].tgt_project)
         for args, who in self.comment_api.command_find(
-            comments, self.bot_name.lower(), 'override', users):
-            if args[2] == 'accept':
-                self.review_messages['accepted'] = 'overridden by {}'.format(who)
+            comments, self.review_user, 'override', users):
+            message = 'overridden by {}'.format(who)
+            override = args[1] or None
+            if override == 'accept':
+                self.review_messages['accepted'] = message
                 return True
 
-            if args[2] == 'decline':
-                self.review_messages['declined'] = 'overridden by {}'.format(who)
+            if override == 'decline':
+                self.review_messages['declined'] = message
                 return False
 
     def _set_review(self, req, state):
diff --git a/osclib/comments.py b/osclib/comments.py
index 0cede21..2d787c9 100644
--- a/osclib/comments.py
+++ b/osclib/comments.py
@@ -23,8 +23,6 @@ from osc.core import http_GET
 from osc.core import http_POST
 from osc.core import makeurl
 
-COMMAND_PREFIX = 'osrt'
-
 
 class CommentAPI(object):
     COMMENT_MARKER_REGEX = re.compile(r'<!-- (?P<bot>[^ ]+)(?P<info>(?: [^= ]+=[^ ]+)*) -->')
@@ -114,16 +112,17 @@ class CommentAPI(object):
                 return c, info
         return None, None
 
-    def command_find(self, comments, bot=None, command=None, who_allowed=None):
+    def command_find(self, comments, user, command=None, who_allowed=None):
         """
         Find comment commands with the optional conditions.
 
         Usage (in comment):
-            $ osrt <bot> <command> <args...>
+            @<user> <command> [args...]
         """
-        command_re = re.compile(r'^\$\s*{}(?P<args>.*)$'.format(COMMAND_PREFIX))
+        command_re = re.compile(r'^@{} (?P<args>.*)$'.format(user))
 
-        for comment in comments.values():
+        # Search for commands in the order the comment was created.
+        for comment in sorted(comments.values(), key=lambda c: c['when']):
             if who_allowed and comment['who'] not in who_allowed:
                 continue
 
@@ -132,10 +131,7 @@ class CommentAPI(object):
                 continue
 
             args = match.group('args').strip().split(' ')
-            if bot and bot != args[0].replace('-', ''):
-                continue
-
-            if command and command != args[1]:
+            if command and command != (args[0] or None):
                 continue
 
             yield args, comment['who']
diff --git a/osclib/conf.py b/osclib/conf.py
index 10b84b3..015bdf8 100644
--- a/osclib/conf.py
+++ b/osclib/conf.py
@@ -45,7 +45,6 @@ DEFAULT = {
         'openqa': 'https://openqa.opensuse.org',
         'lock': 'openSUSE:%(project)s:Staging',
         'lock-ns': 'openSUSE',
-        'tool-maintainer-group': 'release-tools-maintainers',
         'delreq-review': 'factory-maintainers',
         'main-repo': 'standard',
         'download-baseurl': 'http://download.opensuse.org/tumbleweed/',
@@ -68,7 +67,7 @@ DEFAULT = {
         'openqa': 'https://openqa.opensuse.org',
         'lock': 'openSUSE:%(project)s:Staging',
         'lock-ns': 'openSUSE',
-        'tool-maintainer-group': 'release-tools-maintainers',
+        'leaper-override-group': 'leap-reviewers',
         'delreq-review': None,
         'main-repo': 'standard',
         'download-baseurl': 'http://download.opensuse.org/distribution/leap/%(version)s/',
diff --git a/repo_checker.py b/repo_checker.py
index 00e341b..5137d01 100755
--- a/repo_checker.py
+++ b/repo_checker.py
@@ -190,7 +190,7 @@ class RepoChecker(ReviewBot.ReviewBot):
                 # Look for skip-cycle comment command.
                 users = self.request_override_check_users(request.actions[0].tgt_project)
                 for _, who in self.comment_api.command_find(
-                    comments, self.bot_name.lower(), 'skip-cycle', users):
+                    comments, self.review_user, 'skip-cycle', users):
                     self.logger.debug('comment command: skip-cycle by {}'.format(who))
                     self.groups_skip_cycle.append(group)
                     break
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 6, 2018

In order to pickup override comments locally one must set --user appropriately or .oscrc.

@jberry-suse jberry-suse force-pushed the jberry-suse:ReviewBot-comment-override branch from 68b93e1 to cdf70be Mar 6, 2018
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 6, 2018

Tweaked regex in relation to user.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Mar 7, 2018

To be clear this is somewhat inconsistent although it is likely more straight-forward from a usage standpoint. Comments (reports) are discovered based on bot_name which is derived from class (ie per tool) and comment commands are discovered based on user running the tool. This means depending on invocation details one can get rather different results.

Changing the comment discovery to user would be consistent, but would break things like osc staging select comments which should be discovered regardless of user running the command.

Overall, we can keep it this way, but it may come bite us at some point.

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Mar 7, 2018

so maybe comments need to be seen as group responsibility whereas command address individuals. In any case cool. Let's bring it live :)

@jberry-suse jberry-suse merged commit d30db7e into openSUSE:master Mar 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jberry-suse jberry-suse deleted the jberry-suse:ReviewBot-comment-override branch Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.