From 8f7b477fb8b9c23e4b250c69978febcb8cf1f78e Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Thu, 23 Jun 2016 23:49:38 -0500 Subject: [PATCH 01/16] start of gitlab code review/merge bot the intent of this thing is to scan configured projects' merge requests, and automatically assign them to designated code reviewers. if someone says "LGTM" in the merge request thread, that counts as a review and the bot either selects another reviewer or assigns the thing to a designated merge approver for the final approval and merge this is most of the way there, but not done yet. things still to do: 1) more strings than "LGTM", but we should be careful to avoid things that someone might actually say 2) i'm trying to avoid it but i probably need to track the 2 of 2 reviewer message separate from the message assigning the merge request to an approver. it's plausible that a reviewer is also an approver, and if the last reviewer is a candidate approver, the script does nothing, but we probably want it to still log the 2 of 2 part. i could track the "nagging" for 2 of 2 messages, to avoid the bot repeating itself, but that seems unfortunately annoying --- dr_botzo/dr_botzo/settings.py | 1 + dr_botzo/gitlab_bot/__init__.py | 0 dr_botzo/gitlab_bot/admin.py | 8 + dr_botzo/gitlab_bot/lib.py | 158 ++++++++++++++++++ dr_botzo/gitlab_bot/management/__init__.py | 0 .../management/commands/__init__.py | 0 .../management/commands/code_review_scan.py | 22 +++ .../gitlab_bot/migrations/0001_initial.py | 33 ++++ dr_botzo/gitlab_bot/migrations/__init__.py | 0 dr_botzo/gitlab_bot/models.py | 36 ++++ requirements.txt | 2 +- 11 files changed, 259 insertions(+), 1 deletion(-) create mode 100644 dr_botzo/gitlab_bot/__init__.py create mode 100644 dr_botzo/gitlab_bot/admin.py create mode 100644 dr_botzo/gitlab_bot/lib.py create mode 100644 dr_botzo/gitlab_bot/management/__init__.py create mode 100644 dr_botzo/gitlab_bot/management/commands/__init__.py create mode 100644 dr_botzo/gitlab_bot/management/commands/code_review_scan.py create mode 100644 dr_botzo/gitlab_bot/migrations/0001_initial.py create mode 100644 dr_botzo/gitlab_bot/migrations/__init__.py create mode 100644 dr_botzo/gitlab_bot/models.py diff --git a/dr_botzo/dr_botzo/settings.py b/dr_botzo/dr_botzo/settings.py index 5d3012b..43f8ad9 100644 --- a/dr_botzo/dr_botzo/settings.py +++ b/dr_botzo/dr_botzo/settings.py @@ -46,6 +46,7 @@ INSTALLED_APPS = ( 'countdown', 'dispatch', 'facts', + 'gitlab_bot', 'ircbot', 'karma', 'markov', diff --git a/dr_botzo/gitlab_bot/__init__.py b/dr_botzo/gitlab_bot/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/dr_botzo/gitlab_bot/admin.py b/dr_botzo/gitlab_bot/admin.py new file mode 100644 index 0000000..e160534 --- /dev/null +++ b/dr_botzo/gitlab_bot/admin.py @@ -0,0 +1,8 @@ +"""Admin stuff for GitLab bot models.""" + +from django.contrib import admin + +from gitlab_bot.models import GitlabConfig, GitlabProjectConfig + +admin.site.register(GitlabConfig) +admin.site.register(GitlabProjectConfig) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py new file mode 100644 index 0000000..cc13e2c --- /dev/null +++ b/dr_botzo/gitlab_bot/lib.py @@ -0,0 +1,158 @@ +"""Wrapped client for the configured GitLab bot.""" + +import logging +import random +import re + +import gitlab + +from gitlab_bot.models import GitlabConfig + +log = logging.getLogger(__name__) + + +class GitlabBot(object): + + """Bot for doing GitLab stuff. Might be used by the IRC bot, or daemons, or whatever.""" + + COUNTER_RESET_FORMAT = "Review counter reset." + COUNTER_START_FORMAT = "Please obtain {0:d} reviews." + NEW_REVIEWER_FORMAT = "{0:d} of {1:d} reviewed. Assigning to {2:s} to review." + REVIEWS_DONE_FORMAT = "Reviews conducted. Assigning to {0:s} to approve." + NOTE_COMMENT = [ + "The computer is your friend.", + "Thank you for complying.", + ] + + def __init__(self): + """Initialize the actual GitLab client.""" + config = GitlabConfig.objects.first() + self.client = gitlab.Gitlab(config.url, private_token=config.token) + self.client.auth() + + def random_counter_reset_message(self): + return "{0:s} {1:s} {2:s}".format(self.COUNTER_RESET_FORMAT, self.COUNTER_START_FORMAT, + random.choice(self.NOTE_COMMENT)) + + def random_new_reviewer_message(self): + return "{0:s} {1:s}".format(self.NEW_REVIEWER_FORMAT, random.choice(self.NOTE_COMMENT)) + + def random_reviews_done_message(self): + return "{0:s} {1:s}".format(self.REVIEWS_DONE_FORMAT, random.choice(self.NOTE_COMMENT)) + + def get_code_review_acceptance_count(self, merge_request): + """Walk the provided merge request and determine the number of code review signoffs in the comments.""" + if type(merge_request) != gitlab.objects.ProjectMergeRequest: + raise TypeError("merge_request must be a ProjectMergeRequest object") + + approve_count = 0 + approvers = list() + reset_found = False + send_reset = False + notes = sorted(self.client.project_mergerequest_notes.list(project_id=merge_request.project_id, + merge_request_id=merge_request.id, + all=True), + key=lambda x: x.id) + for note in notes: + if not note.system: + # note that we can't ignore note.system = True + merge_request.author, that might have been + # a new push or something, so we only ignore normal notes from the author + if note.author == merge_request.author: + log.debug("skipping note from the merge request author") + elif note.author.username == self.client.user.username: + log.debug("saw a message from myself, maybe i already nagged about the reset") + if note.body.find(self.COUNTER_RESET_FORMAT) >= 0: + log.debug("...yup") + send_reset = False + else: + log.debug("...nope") + else: + if note.body.find("LGTM") >= 0: + log.debug("merge request '%s', note '%s' has a LGTM", merge_request.title, note.id) + approve_count += 1 + if reset_found: + log.debug("ignoring the previous reset, since we have a counter") + reset_found = False + send_reset = False + approvers.append(note.author.username) + log.debug("approval count set to %d", approve_count) + else: + log.debug("merge request '%s', note '%s' does not have a LGTM", merge_request.title, note.id) + else: + log.debug("merge request '%s', note '%s' is a system message", merge_request.title, note.id) + if re.match(r'Added \d+ commit', note.body): + log.debug("setting the counter to 0, '%s' looks like a push!", note.body) + approve_count = 0 + approvers = list() + reset_found = True + send_reset = True + else: + log.debug("leaving the counter as it is, i don't think '%s' is a push", note.body) + + return approve_count, approvers, send_reset + + def scan_for_reviews(self, project, project_obj, merge_request): + log.debug("scanning merge request '%s'", merge_request.title) + + # check to see if the merge request needs a reviewer or a merge + approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) + if approve_count < project.code_reviews_necessary: + log.debug("%s needs a code review", merge_request.title) + if merge_request.assignee is not None: + log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) + excluded_candidates = approvers + [merge_request.author.username] + candidates = [x for x in project.code_reviewers.split(',') if x not in excluded_candidates] + + # if the current assignee is in candidates, then we don't need to do anything + if merge_request.assignee is None or merge_request.assignee.username not in candidates: + if len(candidates) > 0: + new_reviewer = random.choice(candidates) + log.debug("%s is the new reviewer", new_reviewer) + + # find the user object for the new reviewer + new_reviewer_obj = self.client.users.get_by_username(new_reviewer) + + # create note for the update + assign_msg = {'body': self.random_new_reviewer_message().format(approve_count, + project.code_reviews_necessary, + new_reviewer)} + self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + + # assign the merge request to the new reviewer + self.client.update(merge_request, assignee_id=new_reviewer_obj.id) + else: + log.warning("no reviewers left to review %s, doing nothing", merge_request.title) + else: + log.info("%s is already assigned to a candidate reviewer", merge_request.title) + + # regardless of our decision to assign or not, if we're supposed to send the reset + # message, we should + if send_reset: + log.info("adding reset message to %s", merge_request.title) + reset_msg = {'body': self.random_counter_reset_message().format(project.code_reviews_necessary)} + self.client.project_mergerequest_notes.create(reset_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + else: + log.debug("%s has enough code reviews, it needs someone to merge it", merge_request.title) + if merge_request.assignee is not None: + log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) + excluded = [merge_request.author.username] + candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded] + if merge_request.assignee is None or merge_request.assignee.username not in candidates: + if len(candidates) > 0: + new_approver = random.choice(candidates) + log.debug("%s is the new approver", new_approver) + + # find the user object for the new reviewer + new_approver_obj = self.client.users.get_by_username(new_approver) + + # create note for the update + assign_msg = {'body': self.random_reviews_done_message().format(new_approver)} + self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + + # assign the merge request to the new reviewer + self.client.update(merge_request, assignee_id=new_approver_obj.id) + else: + log.warning("no approvers left to approve %s, doing nothing", merge_request.title) diff --git a/dr_botzo/gitlab_bot/management/__init__.py b/dr_botzo/gitlab_bot/management/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/dr_botzo/gitlab_bot/management/commands/__init__.py b/dr_botzo/gitlab_bot/management/commands/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/dr_botzo/gitlab_bot/management/commands/code_review_scan.py b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py new file mode 100644 index 0000000..cb48931 --- /dev/null +++ b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py @@ -0,0 +1,22 @@ +"""Find merge requests that need code reviewers.""" + +import logging + +from django.core.management import BaseCommand + +from gitlab_bot.lib import GitlabBot +from gitlab_bot.models import GitlabProjectConfig + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = "Find merge requests needing code reviewers/assignees" + + def handle(self, *args, **options): + bot = GitlabBot() + projects = GitlabProjectConfig.objects.filter(manage_merge_request_code_reviews=True) + for project in projects: + project_obj = bot.client.projects.get(project.project_id) + for merge_request in project_obj.mergerequests.list(state='opened'): + bot.scan_for_reviews(project, project_obj, merge_request) diff --git a/dr_botzo/gitlab_bot/migrations/0001_initial.py b/dr_botzo/gitlab_bot/migrations/0001_initial.py new file mode 100644 index 0000000..4800d92 --- /dev/null +++ b/dr_botzo/gitlab_bot/migrations/0001_initial.py @@ -0,0 +1,33 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ] + + operations = [ + migrations.CreateModel( + name='GitlabConfig', + fields=[ + ('id', models.AutoField(auto_created=True, verbose_name='ID', primary_key=True, serialize=False)), + ('url', models.URLField()), + ('token', models.CharField(max_length=64)), + ], + ), + migrations.CreateModel( + name='GitlabProjectConfig', + fields=[ + ('id', models.AutoField(auto_created=True, verbose_name='ID', primary_key=True, serialize=False)), + ('project_id', models.CharField(max_length=64)), + ('manage_merge_request_code_reviews', models.BooleanField(default=False)), + ('code_reviews_necessary', models.PositiveSmallIntegerField(default=0)), + ('code_reviewers', models.TextField(blank=True, default='')), + ('code_review_final_merge_assignees', models.TextField(blank=True, default='')), + ('gitlab_config', models.ForeignKey(to='gitlab_bot.GitlabConfig', null=True)), + ], + ), + ] diff --git a/dr_botzo/gitlab_bot/migrations/__init__.py b/dr_botzo/gitlab_bot/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/dr_botzo/gitlab_bot/models.py b/dr_botzo/gitlab_bot/models.py new file mode 100644 index 0000000..78211e4 --- /dev/null +++ b/dr_botzo/gitlab_bot/models.py @@ -0,0 +1,36 @@ +"""Bot/daemons for doing stuff with GitLab.""" + +import logging + +from django.db import models + +log = logging.getLogger(__name__) + + +class GitlabConfig(models.Model): + + """Maintain bot-wide settings (URL, auth key, etc.).""" + + url = models.URLField() + token = models.CharField(max_length=64) + + def __str__(self): + """String representation.""" + return "bot @ {0:s}".format(self.url) + + +class GitlabProjectConfig(models.Model): + + """Maintain settings for a particular project in GitLab.""" + + gitlab_config = models.ForeignKey('GitlabConfig', null=True) + project_id = models.CharField(max_length=64) + + manage_merge_request_code_reviews = models.BooleanField(default=False) + code_reviews_necessary = models.PositiveSmallIntegerField(default=0) + code_reviewers = models.TextField(default='', blank=True) + code_review_final_merge_assignees = models.TextField(default='', blank=True) + + def __str__(self): + """String representation.""" + return "configuration for {0:s} @ {1:s}".format(self.project_id, self.gitlab_config.url) diff --git a/requirements.txt b/requirements.txt index 7db8cd6..bb05e39 100644 --- a/requirements.txt +++ b/requirements.txt @@ -43,6 +43,7 @@ pylint-common==0.2.2 pylint-django==0.7.1 pylint-plugin-utils==0.2.3 python-dateutil==2.4.2 +python-gitlab==0.13 pytz==2015.7 PyYAML==3.11 requests==2.9.1 @@ -52,7 +53,6 @@ setoptconf==0.2.0 six==1.10.0 tempora==1.4 twython==3.3.0 -wheel==0.26.0 wrapt==1.10.6 yg.lockfile==2.1 zc.lockfile==1.1.0 From e846502b48d156d590a9bcf727cae38a04868e4e Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 24 Jun 2016 13:50:09 -0500 Subject: [PATCH 02/16] gitlab bot: start refactoring code review methods the code review logic to this point is kind of convoluted, and still is for the moment, but this moves some of the stuff into a more coherent spot in the expectation of reuse. most of the refactoring is still to come --- dr_botzo/gitlab_bot/lib.py | 115 +++++++++--------- .../management/commands/code_review_scan.py | 4 +- 2 files changed, 61 insertions(+), 58 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index cc13e2c..5df85f1 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -91,68 +91,73 @@ class GitlabBot(object): return approve_count, approvers, send_reset - def scan_for_reviews(self, project, project_obj, merge_request): - log.debug("scanning merge request '%s'", merge_request.title) + def scan_project_for_reviews(self, project): + project_obj = self.client.projects.get(project.project_id) + if not project_obj: + return - # check to see if the merge request needs a reviewer or a merge - approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) - if approve_count < project.code_reviews_necessary: - log.debug("%s needs a code review", merge_request.title) - if merge_request.assignee is not None: - log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) - excluded_candidates = approvers + [merge_request.author.username] - candidates = [x for x in project.code_reviewers.split(',') if x not in excluded_candidates] + for merge_request in project_obj.mergerequests.list(state='opened'): + log.debug("scanning merge request '%s'", merge_request.title) - # if the current assignee is in candidates, then we don't need to do anything - if merge_request.assignee is None or merge_request.assignee.username not in candidates: - if len(candidates) > 0: - new_reviewer = random.choice(candidates) - log.debug("%s is the new reviewer", new_reviewer) + # check to see if the merge request needs a reviewer or a merge + approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) + if approve_count < project.code_reviews_necessary: + log.debug("%s needs a code review", merge_request.title) + if merge_request.assignee is not None: + log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) + excluded_candidates = approvers + [merge_request.author.username] + candidates = [x for x in project.code_reviewers.split(',') if x not in excluded_candidates] - # find the user object for the new reviewer - new_reviewer_obj = self.client.users.get_by_username(new_reviewer) + # if the current assignee is in candidates, then we don't need to do anything + if merge_request.assignee is None or merge_request.assignee.username not in candidates: + if len(candidates) > 0: + new_reviewer = random.choice(candidates) + log.debug("%s is the new reviewer", new_reviewer) - # create note for the update - assign_msg = {'body': self.random_new_reviewer_message().format(approve_count, - project.code_reviews_necessary, - new_reviewer)} - self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, - merge_request_id=merge_request.id) + # find the user object for the new reviewer + new_reviewer_obj = self.client.users.get_by_username(new_reviewer) - # assign the merge request to the new reviewer - self.client.update(merge_request, assignee_id=new_reviewer_obj.id) + # create note for the update + assign_msg = {'body': self.random_new_reviewer_message().format(approve_count, + project.code_reviews_necessary, + new_reviewer)} + self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + + # assign the merge request to the new reviewer + self.client.update(merge_request, assignee_id=new_reviewer_obj.id) + else: + log.warning("no reviewers left to review %s, doing nothing", merge_request.title) else: - log.warning("no reviewers left to review %s, doing nothing", merge_request.title) + log.info("%s is already assigned to a candidate reviewer", merge_request.title) + + # regardless of our decision to assign or not, if we're supposed to send the reset + # message, we should + if send_reset: + log.info("adding reset message to %s", merge_request.title) + reset_msg = {'body': self.random_counter_reset_message().format(project.code_reviews_necessary)} + self.client.project_mergerequest_notes.create(reset_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) else: - log.info("%s is already assigned to a candidate reviewer", merge_request.title) + log.debug("%s has enough code reviews, it needs someone to merge it", merge_request.title) + if merge_request.assignee is not None: + log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) + excluded = [merge_request.author.username] + candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded] + if merge_request.assignee is None or merge_request.assignee.username not in candidates: + if len(candidates) > 0: + new_approver = random.choice(candidates) + log.debug("%s is the new approver", new_approver) - # regardless of our decision to assign or not, if we're supposed to send the reset - # message, we should - if send_reset: - log.info("adding reset message to %s", merge_request.title) - reset_msg = {'body': self.random_counter_reset_message().format(project.code_reviews_necessary)} - self.client.project_mergerequest_notes.create(reset_msg, project_id=project_obj.id, - merge_request_id=merge_request.id) - else: - log.debug("%s has enough code reviews, it needs someone to merge it", merge_request.title) - if merge_request.assignee is not None: - log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) - excluded = [merge_request.author.username] - candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded] - if merge_request.assignee is None or merge_request.assignee.username not in candidates: - if len(candidates) > 0: - new_approver = random.choice(candidates) - log.debug("%s is the new approver", new_approver) + # find the user object for the new reviewer + new_approver_obj = self.client.users.get_by_username(new_approver) - # find the user object for the new reviewer - new_approver_obj = self.client.users.get_by_username(new_approver) + # create note for the update + assign_msg = {'body': self.random_reviews_done_message().format(new_approver)} + self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + merge_request_id=merge_request.id) - # create note for the update - assign_msg = {'body': self.random_reviews_done_message().format(new_approver)} - self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, - merge_request_id=merge_request.id) - - # assign the merge request to the new reviewer - self.client.update(merge_request, assignee_id=new_approver_obj.id) - else: - log.warning("no approvers left to approve %s, doing nothing", merge_request.title) + # assign the merge request to the new reviewer + self.client.update(merge_request, assignee_id=new_approver_obj.id) + else: + log.warning("no approvers left to approve %s, doing nothing", merge_request.title) diff --git a/dr_botzo/gitlab_bot/management/commands/code_review_scan.py b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py index cb48931..b4a66bf 100644 --- a/dr_botzo/gitlab_bot/management/commands/code_review_scan.py +++ b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py @@ -17,6 +17,4 @@ class Command(BaseCommand): bot = GitlabBot() projects = GitlabProjectConfig.objects.filter(manage_merge_request_code_reviews=True) for project in projects: - project_obj = bot.client.projects.get(project.project_id) - for merge_request in project_obj.mergerequests.list(state='opened'): - bot.scan_for_reviews(project, project_obj, merge_request) + bot.scan_project_for_reviews(project) From 1a428e7d2928729eae3cddf6173325556a372d97 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 24 Jun 2016 13:54:37 -0500 Subject: [PATCH 03/16] gitlab bot: make project_id unique=True --- .../migrations/0002_auto_20160624_1354.py | 19 +++++++++++++++++++ dr_botzo/gitlab_bot/models.py | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 dr_botzo/gitlab_bot/migrations/0002_auto_20160624_1354.py diff --git a/dr_botzo/gitlab_bot/migrations/0002_auto_20160624_1354.py b/dr_botzo/gitlab_bot/migrations/0002_auto_20160624_1354.py new file mode 100644 index 0000000..8e026bb --- /dev/null +++ b/dr_botzo/gitlab_bot/migrations/0002_auto_20160624_1354.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('gitlab_bot', '0001_initial'), + ] + + operations = [ + migrations.AlterField( + model_name='gitlabprojectconfig', + name='project_id', + field=models.CharField(unique=True, max_length=64), + ), + ] diff --git a/dr_botzo/gitlab_bot/models.py b/dr_botzo/gitlab_bot/models.py index 78211e4..b21c685 100644 --- a/dr_botzo/gitlab_bot/models.py +++ b/dr_botzo/gitlab_bot/models.py @@ -24,7 +24,7 @@ class GitlabProjectConfig(models.Model): """Maintain settings for a particular project in GitLab.""" gitlab_config = models.ForeignKey('GitlabConfig', null=True) - project_id = models.CharField(max_length=64) + project_id = models.CharField(max_length=64, unique=True) manage_merge_request_code_reviews = models.BooleanField(default=False) code_reviews_necessary = models.PositiveSmallIntegerField(default=0) From fca69b1994ca72f5c98eaa591d01b0dcdb0dc0ef Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 24 Jun 2016 14:32:59 -0500 Subject: [PATCH 04/16] gitlab bot: add a class to track review scanning the code to scan all merge request notes for reviews is getting messy, going to try moving most of the state into a class. still unsure this is final --- dr_botzo/gitlab_bot/lib.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 5df85f1..15fd9dd 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -161,3 +161,17 @@ class GitlabBot(object): self.client.update(merge_request, assignee_id=new_approver_obj.id) else: log.warning("no approvers left to approve %s, doing nothing", merge_request.title) + + +class _MergeRequestScanningState(object): + + """Track the state of a merge request as it is scanned and appropriate action identified.""" + + def __init__(self): + """Set default flags/values.""" + self.approver_list = [] + + self.unlogged_approval_reset = False + self.unlogged_approval = False + self.needs_reviewer = False + self.needs_accepter = False From a0f76e4c0fbe1424979c5b5a9f41b9259fd86523 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 24 Jun 2016 17:44:36 -0500 Subject: [PATCH 05/16] gitlab bot: major refactoring of code review scan this moves a bunch of code around and renames a bunch of things, so the diff for it is fairly huge, but it's the same general idea, just organized better. woo. --- dr_botzo/gitlab_bot/lib.py | 245 ++++++++++++++++++++++--------------- 1 file changed, 149 insertions(+), 96 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 15fd9dd..95f7e93 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -15,13 +15,16 @@ class GitlabBot(object): """Bot for doing GitLab stuff. Might be used by the IRC bot, or daemons, or whatever.""" - COUNTER_RESET_FORMAT = "Review counter reset." - COUNTER_START_FORMAT = "Please obtain {0:d} reviews." - NEW_REVIEWER_FORMAT = "{0:d} of {1:d} reviewed. Assigning to {2:s} to review." - REVIEWS_DONE_FORMAT = "Reviews conducted. Assigning to {0:s} to approve." + REVIEWS_START_FORMAT = "Starting review process." + REVIEWS_RESET_FORMAT = "Review process reset." + REVIEWS_REVIEW_REGISTERED_FORMAT = "Review identified." + REVIEWS_PROGRESS_FORMAT = "{0:d} reviews of {1:d} necessary to proceed." + REVIEWS_REVIEWS_COMPLETE = "Reviews complete." + NEW_REVIEWER_FORMAT = "Assigning to {0:s} to review merge request." + NEW_ACCEPTER_FORMAT = "Assigning to {0:s} to accept merge request." + NOTE_COMMENT = [ "The computer is your friend.", - "Thank you for complying.", ] def __init__(self): @@ -30,66 +33,26 @@ class GitlabBot(object): self.client = gitlab.Gitlab(config.url, private_token=config.token) self.client.auth() - def random_counter_reset_message(self): - return "{0:s} {1:s} {2:s}".format(self.COUNTER_RESET_FORMAT, self.COUNTER_START_FORMAT, + def random_reviews_start_message(self): + return "{0:s} {1:s} {2:s}".format(self.REVIEWS_START_FORMAT, self.REVIEWS_PROGRESS_FORMAT, random.choice(self.NOTE_COMMENT)) + def random_reviews_reset_message(self): + return "{0:s} {1:s} {2:s}".format(self.REVIEWS_RESET_FORMAT, self.REVIEWS_PROGRESS_FORMAT, + random.choice(self.NOTE_COMMENT)) + + def random_review_progress_message(self): + return "{0:s} {1:s} {2:s}".format(self.REVIEWS_REVIEW_REGISTERED_FORMAT, self.REVIEWS_PROGRESS_FORMAT, + random.choice(self.NOTE_COMMENT)) + + def random_reviews_complete_message(self): + return "{0:s} {1:s}".format(self.REVIEWS_REVIEWS_COMPLETE, random.choice(self.NOTE_COMMENT)) + def random_new_reviewer_message(self): return "{0:s} {1:s}".format(self.NEW_REVIEWER_FORMAT, random.choice(self.NOTE_COMMENT)) def random_reviews_done_message(self): - return "{0:s} {1:s}".format(self.REVIEWS_DONE_FORMAT, random.choice(self.NOTE_COMMENT)) - - def get_code_review_acceptance_count(self, merge_request): - """Walk the provided merge request and determine the number of code review signoffs in the comments.""" - if type(merge_request) != gitlab.objects.ProjectMergeRequest: - raise TypeError("merge_request must be a ProjectMergeRequest object") - - approve_count = 0 - approvers = list() - reset_found = False - send_reset = False - notes = sorted(self.client.project_mergerequest_notes.list(project_id=merge_request.project_id, - merge_request_id=merge_request.id, - all=True), - key=lambda x: x.id) - for note in notes: - if not note.system: - # note that we can't ignore note.system = True + merge_request.author, that might have been - # a new push or something, so we only ignore normal notes from the author - if note.author == merge_request.author: - log.debug("skipping note from the merge request author") - elif note.author.username == self.client.user.username: - log.debug("saw a message from myself, maybe i already nagged about the reset") - if note.body.find(self.COUNTER_RESET_FORMAT) >= 0: - log.debug("...yup") - send_reset = False - else: - log.debug("...nope") - else: - if note.body.find("LGTM") >= 0: - log.debug("merge request '%s', note '%s' has a LGTM", merge_request.title, note.id) - approve_count += 1 - if reset_found: - log.debug("ignoring the previous reset, since we have a counter") - reset_found = False - send_reset = False - approvers.append(note.author.username) - log.debug("approval count set to %d", approve_count) - else: - log.debug("merge request '%s', note '%s' does not have a LGTM", merge_request.title, note.id) - else: - log.debug("merge request '%s', note '%s' is a system message", merge_request.title, note.id) - if re.match(r'Added \d+ commit', note.body): - log.debug("setting the counter to 0, '%s' looks like a push!", note.body) - approve_count = 0 - approvers = list() - reset_found = True - send_reset = True - else: - log.debug("leaving the counter as it is, i don't think '%s' is a push", note.body) - - return approve_count, approvers, send_reset + return "{0:s} {1:s}".format(self.NEW_ACCEPTER_FORMAT, random.choice(self.NOTE_COMMENT)) def scan_project_for_reviews(self, project): project_obj = self.client.projects.get(project.project_id) @@ -99,29 +62,124 @@ class GitlabBot(object): for merge_request in project_obj.mergerequests.list(state='opened'): log.debug("scanning merge request '%s'", merge_request.title) - # check to see if the merge request needs a reviewer or a merge - approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) - if approve_count < project.code_reviews_necessary: + # moved this method into here + # # check to see if the merge request needs a reviewer or a merge + # approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) + + request_state = _MergeRequestScanningState() + notes = sorted(self.client.project_mergerequest_notes.list(project_id=merge_request.project_id, + merge_request_id=merge_request.id, + all=True), + key=lambda x: x.id) + for note in notes: + if not note.system: + log.debug("merge request '%s', note '%s' is a normal message", merge_request.title, note.id) + # note that we can't ignore note.system = True + merge_request.author, that might have been + # a new push or something, so we only ignore normal notes from the author + if note.author == merge_request.author: + log.debug("skipping note from the merge request author") + elif note.author.username == self.client.user.username: + log.debug("saw a message from myself, i might have already sent a notice i'm sitting on") + if note.body.find(self.REVIEWS_RESET_FORMAT) >= 0 and request_state.unlogged_approval_reset: + log.debug("saw a reset message, unsetting the flag") + request_state.unlogged_approval_reset = False + elif note.body.find(self.REVIEWS_START_FORMAT) >= 0 and request_state.unlogged_review_start: + log.debug("saw a start message, unsetting the flag") + request_state.unlogged_review_start = False + elif note.body.find(self.REVIEWS_REVIEW_REGISTERED_FORMAT) >= 0 and request_state.unlogged_approval: + log.debug("saw a review log message, unsetting the flag") + request_state.unlogged_approval = False + elif note.body.find(self.REVIEWS_REVIEWS_COMPLETE) >= 0 and request_state.unlogged_review_complete: + log.debug("saw a review complete message, unsetting the flag") + request_state.unlogged_review_complete = False + else: + log.debug("nothing in particular relevant in '%s'", note.body) + else: + if note.body.find("LGTM") >= 0: + log.debug("merge request '%s', note '%s' has a LGTM", merge_request.title, note.id) + request_state.unlogged_approval = True + request_state.approver_list.append(note.author.username) + log.debug("approvers: %s", request_state.approver_list) + if len(request_state.approver_list) < project.code_reviews_necessary: + log.debug("not enough code reviews yet, setting needs_reviewer") + request_state.needs_reviewer = True + request_state.needs_accepter = False + else: + log.debug("enough code reviews, setting needs_accepter") + request_state.needs_accepter = True + request_state.needs_reviewer = False + request_state.unlogged_review_complete = True + else: + log.debug("merge request '%s', note '%s' does not have a LGTM", merge_request.title, + note.id) + else: + log.debug("merge request '%s', note '%s' is a system message", merge_request.title, note.id) + if re.match(r'Added \d+ commit', note.body): + log.debug("resetting approval list, '%s' looks like a push!", note.body) + request_state.approver_list.clear() + request_state.unlogged_approval_reset = True + else: + log.debug("leaving the approval list as it is, i don't think '%s' is a push", note.body) + + # do some cleanup + excluded_review_candidates = request_state.approver_list + [merge_request.author.username] + review_candidates = [x for x in project.code_reviewers.split(',') if x not in excluded_review_candidates] + if merge_request.assignee: + if request_state.needs_reviewer and merge_request.assignee.username in review_candidates: + log.debug("unsetting the needs_reviewer flag, the request is already assigned to one") + request_state.needs_reviewer = False + + excluded_accept_candidates = [merge_request.author.username] + accept_candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded_accept_candidates] + if merge_request.assignee: + if request_state.needs_accepter and merge_request.assignee.username in accept_candidates: + log.debug("unsetting the needs_accepter flag, the request is already assigned to one") + request_state.needs_accepter = False + + log.debug("%s", request_state.__dict__) + + # status message stuff + if request_state.unlogged_review_start: + log.info("sending message for start of reviews") + msg = {'body': self.random_reviews_start_message().format(len(request_state.approver_list), + project.code_reviews_necessary)} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + if request_state.unlogged_approval_reset: + log.info("sending message for review reset") + msg = {'body': self.random_reviews_reset_message().format(len(request_state.approver_list), + project.code_reviews_necessary)} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + if request_state.unlogged_approval: + log.info("sending message for code review progress") + msg = {'body': self.random_review_progress_message().format(len(request_state.approver_list), + project.code_reviews_necessary)} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + if request_state.unlogged_review_complete: + log.info("sending message for code review complete") + msg = {'body': self.random_reviews_complete_message()} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, + merge_request_id=merge_request.id) + + # if there's a reviewer necessary, assign the merge request + if len(request_state.approver_list) < project.code_reviews_necessary and request_state.needs_reviewer: log.debug("%s needs a code review", merge_request.title) if merge_request.assignee is not None: log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) - excluded_candidates = approvers + [merge_request.author.username] - candidates = [x for x in project.code_reviewers.split(',') if x not in excluded_candidates] - # if the current assignee is in candidates, then we don't need to do anything - if merge_request.assignee is None or merge_request.assignee.username not in candidates: - if len(candidates) > 0: - new_reviewer = random.choice(candidates) + if merge_request.assignee is None or merge_request.assignee.username not in review_candidates: + if len(review_candidates) > 0: + new_reviewer = random.choice(review_candidates) log.debug("%s is the new reviewer", new_reviewer) - # find the user object for the new reviewer + # get the user object for the new reviewer new_reviewer_obj = self.client.users.get_by_username(new_reviewer) # create note for the update - assign_msg = {'body': self.random_new_reviewer_message().format(approve_count, - project.code_reviews_necessary, - new_reviewer)} - self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + msg = {'body': self.random_new_reviewer_message().format(new_reviewer)} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, merge_request_id=merge_request.id) # assign the merge request to the new reviewer @@ -129,38 +187,31 @@ class GitlabBot(object): else: log.warning("no reviewers left to review %s, doing nothing", merge_request.title) else: - log.info("%s is already assigned to a candidate reviewer", merge_request.title) + log.debug("needs_reviewer set but the request is assigned to a reviewer, doing nothing") - # regardless of our decision to assign or not, if we're supposed to send the reset - # message, we should - if send_reset: - log.info("adding reset message to %s", merge_request.title) - reset_msg = {'body': self.random_counter_reset_message().format(project.code_reviews_necessary)} - self.client.project_mergerequest_notes.create(reset_msg, project_id=project_obj.id, - merge_request_id=merge_request.id) - else: - log.debug("%s has enough code reviews, it needs someone to merge it", merge_request.title) + # if there's an accepter necessary, assign the merge request + if len(request_state.approver_list) >= project.code_reviews_necessary and request_state.needs_accepter: + log.debug("%s needs an accepter", merge_request.title) if merge_request.assignee is not None: log.debug("%s currently assigned to %s", merge_request.title, merge_request.assignee.username) - excluded = [merge_request.author.username] - candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded] - if merge_request.assignee is None or merge_request.assignee.username not in candidates: - if len(candidates) > 0: - new_approver = random.choice(candidates) - log.debug("%s is the new approver", new_approver) - # find the user object for the new reviewer - new_approver_obj = self.client.users.get_by_username(new_approver) + if merge_request.assignee is None or merge_request.assignee.username not in accept_candidates: + if len(accept_candidates) > 0: + new_accepter = random.choice(accept_candidates) + log.debug("%s is the new accepter", new_accepter) + + # get the user object for the new accepter + new_accepter_obj = self.client.users.get_by_username(new_accepter) # create note for the update - assign_msg = {'body': self.random_reviews_done_message().format(new_approver)} - self.client.project_mergerequest_notes.create(assign_msg, project_id=project_obj.id, + msg = {'body': self.random_reviews_done_message().format(new_accepter)} + self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, merge_request_id=merge_request.id) # assign the merge request to the new reviewer - self.client.update(merge_request, assignee_id=new_approver_obj.id) + self.client.update(merge_request, assignee_id=new_accepter_obj.id) else: - log.warning("no approvers left to approve %s, doing nothing", merge_request.title) + log.warning("no accepters left to accept %s, doing nothing", merge_request.title) class _MergeRequestScanningState(object): @@ -171,7 +222,9 @@ class _MergeRequestScanningState(object): """Set default flags/values.""" self.approver_list = [] + self.unlogged_review_start = True self.unlogged_approval_reset = False self.unlogged_approval = False - self.needs_reviewer = False + self.unlogged_review_complete = False + self.needs_reviewer = True self.needs_accepter = False From e2ed53d0313700ca6a8cb89ea7c1b2cb7adf8a02 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 10:23:42 -0500 Subject: [PATCH 06/16] gitlab bot: more silly messages --- dr_botzo/gitlab_bot/lib.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 95f7e93..9d07152 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -25,6 +25,9 @@ class GitlabBot(object): NOTE_COMMENT = [ "The computer is your friend.", + "Trust the computer.", + "Quality is mandatory.", + "Errors show your disloyalty to the computer.", ] def __init__(self): From 5fc2c0c7f6124770d19356dbac44fe7b92e340e3 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 10:24:40 -0500 Subject: [PATCH 07/16] gitlab bot: merge request IDs arg to scanner for use cases when we know the merge request(s) to scan, allow passing them as an optional argument --- dr_botzo/gitlab_bot/lib.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 9d07152..603b06d 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -57,12 +57,19 @@ class GitlabBot(object): def random_reviews_done_message(self): return "{0:s} {1:s}".format(self.NEW_ACCEPTER_FORMAT, random.choice(self.NOTE_COMMENT)) - def scan_project_for_reviews(self, project): + def scan_project_for_reviews(self, project, merge_request_ids=None): project_obj = self.client.projects.get(project.project_id) if not project_obj: return - for merge_request in project_obj.mergerequests.list(state='opened'): + if merge_request_ids: + merge_requests = [] + for merge_request_id in merge_request_ids: + merge_requests.append(project_obj.mergerequests.get(id=merge_request_id)) + else: + merge_requests = project_obj.mergerequests.list(state='opened') + + for merge_request in merge_requests: log.debug("scanning merge request '%s'", merge_request.title) # moved this method into here From 98bea5390665aff24e3e05c8173069d6d4a8c101 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 10:25:29 -0500 Subject: [PATCH 08/16] gitlab bot: don't bother scanning merged requests --- dr_botzo/gitlab_bot/lib.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 603b06d..c31f870 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -72,9 +72,9 @@ class GitlabBot(object): for merge_request in merge_requests: log.debug("scanning merge request '%s'", merge_request.title) - # moved this method into here - # # check to see if the merge request needs a reviewer or a merge - # approve_count, approvers, send_reset = self.get_code_review_acceptance_count(merge_request) + if merge_request.state == 'merged': + log.info("merge request '%s' is already merged, doing nothing", merge_request.title) + continue request_state = _MergeRequestScanningState() notes = sorted(self.client.project_mergerequest_notes.list(project_id=merge_request.project_id, From feb3944380ea3ef976e1e80dde553552e3a98b1d Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 10:25:50 -0500 Subject: [PATCH 09/16] gitlab bot: standardize on accept/approve language --- dr_botzo/gitlab_bot/management/commands/code_review_scan.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dr_botzo/gitlab_bot/management/commands/code_review_scan.py b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py index b4a66bf..63fd9a8 100644 --- a/dr_botzo/gitlab_bot/management/commands/code_review_scan.py +++ b/dr_botzo/gitlab_bot/management/commands/code_review_scan.py @@ -11,7 +11,7 @@ log = logging.getLogger(__name__) class Command(BaseCommand): - help = "Find merge requests needing code reviewers/assignees" + help = "Find merge requests needing code reviewers/accepters" def handle(self, *args, **options): bot = GitlabBot() From e48b7867feea251013725d77958901d2ab4ffaf0 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 10:31:53 -0500 Subject: [PATCH 10/16] gitlab bot: command for reviewing one specific MR --- .../commands/code_review_specific.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 dr_botzo/gitlab_bot/management/commands/code_review_specific.py diff --git a/dr_botzo/gitlab_bot/management/commands/code_review_specific.py b/dr_botzo/gitlab_bot/management/commands/code_review_specific.py new file mode 100644 index 0000000..447ed3f --- /dev/null +++ b/dr_botzo/gitlab_bot/management/commands/code_review_specific.py @@ -0,0 +1,25 @@ +"""Run the code review process on a specific merge request.""" + +import logging + +from django.core.management import BaseCommand + +from gitlab_bot.lib import GitlabBot +from gitlab_bot.models import GitlabProjectConfig + +log = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = "Assign code reviewers/accepters for a specific merge request" + + def add_arguments(self, parser): + parser.add_argument('project_id', type=int) + parser.add_argument('merge_request_id', type=int) + + def handle(self, *args, **options): + project = GitlabProjectConfig.objects.get(pk=options['project_id']) + merge_request_ids = [options['merge_request_id'], ] + + bot = GitlabBot() + bot.scan_project_for_reviews(project, merge_request_ids=merge_request_ids) From dd0fa00e8e7fceff74916f2307365d3d2e170113 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Mon, 27 Jun 2016 12:39:52 -0500 Subject: [PATCH 11/16] gitlab bot: code reviews: ignore closed MRs --- dr_botzo/gitlab_bot/lib.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index c31f870..8b731e8 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -72,8 +72,9 @@ class GitlabBot(object): for merge_request in merge_requests: log.debug("scanning merge request '%s'", merge_request.title) - if merge_request.state == 'merged': - log.info("merge request '%s' is already merged, doing nothing", merge_request.title) + if merge_request.state in ['merged', 'closed']: + log.info("merge request '%s' is already %s, doing nothing", merge_request.title, + merge_request.state) continue request_state = _MergeRequestScanningState() From 93c9224644af3de0aa883ce80722244be3c83739 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Wed, 29 Jun 2016 10:23:33 -0500 Subject: [PATCH 12/16] gitlab bot: no reset message on approvals 0/N bot was overzealous in logging the approval reset message when seeing pushes. this commit should keep it from logging the message if a push happened but there weren't actually any approvals yet (so there's nothing to reset) still waiting on testing, but this seems like a decent fix to me bss/dr.botzo#4 --- dr_botzo/gitlab_bot/lib.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 8b731e8..e7316a4 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -127,8 +127,11 @@ class GitlabBot(object): log.debug("merge request '%s', note '%s' is a system message", merge_request.title, note.id) if re.match(r'Added \d+ commit', note.body): log.debug("resetting approval list, '%s' looks like a push!", note.body) + + # only set the unlogged approval reset flag if there's some kind of progress + if len(request_state.approver_list) > 0: + request_state.unlogged_approval_reset = True request_state.approver_list.clear() - request_state.unlogged_approval_reset = True else: log.debug("leaving the approval list as it is, i don't think '%s' is a push", note.body) From 0b332f79c91cb863a88ee43f50aad4ce89a7d63e Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 1 Jul 2016 13:23:57 -0500 Subject: [PATCH 13/16] gitlab bot: use assignee name in comments when assigning a new user for review/acceptance, use their real name in the message, not their username (the reassign will include their username a split second later anyway) --- dr_botzo/gitlab_bot/lib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index e7316a4..d4caf40 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -192,7 +192,7 @@ class GitlabBot(object): new_reviewer_obj = self.client.users.get_by_username(new_reviewer) # create note for the update - msg = {'body': self.random_new_reviewer_message().format(new_reviewer)} + msg = {'body': self.random_new_reviewer_message().format(new_reviewer_obj.name)} self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, merge_request_id=merge_request.id) @@ -218,7 +218,7 @@ class GitlabBot(object): new_accepter_obj = self.client.users.get_by_username(new_accepter) # create note for the update - msg = {'body': self.random_reviews_done_message().format(new_accepter)} + msg = {'body': self.random_reviews_done_message().format(new_accepter_obj.name)} self.client.project_mergerequest_notes.create(msg, project_id=project_obj.id, merge_request_id=merge_request.id) From 2761e2315ea682b4dd82436e74b6daebfb4b7bbb Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Thu, 8 Sep 2016 17:23:03 -0500 Subject: [PATCH 14/16] gitlab: don't reassign MR if assigned to author --- dr_botzo/gitlab_bot/lib.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index d4caf40..edecdea 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -142,6 +142,10 @@ class GitlabBot(object): if request_state.needs_reviewer and merge_request.assignee.username in review_candidates: log.debug("unsetting the needs_reviewer flag, the request is already assigned to one") request_state.needs_reviewer = False + elif request_state.needs_reviewer and merge_request.assignee.username == merge_request.author.username: + log.info("unsetting the needs_reviewer flag, the request is assigned to the author") + log.info("in this case we are assuming that the author has work to do, and will re/unassign") + request_state.needs_reviewer = False excluded_accept_candidates = [merge_request.author.username] accept_candidates = [x for x in project.code_review_final_merge_assignees.split(',') if x not in excluded_accept_candidates] @@ -149,6 +153,10 @@ class GitlabBot(object): if request_state.needs_accepter and merge_request.assignee.username in accept_candidates: log.debug("unsetting the needs_accepter flag, the request is already assigned to one") request_state.needs_accepter = False + elif request_state.needs_accepter and merge_request.assignee.username == merge_request.author.username: + log.info("unsetting the needs_accepter flag, the request is assigned to the author") + log.info("in this case we are assuming that the author has work to do, and will re/unassign") + request_state.needs_accepter = False log.debug("%s", request_state.__dict__) From b248a47658a693757a8355cd8c2c93dd51f94d06 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Thu, 8 Sep 2016 17:36:11 -0500 Subject: [PATCH 15/16] gitlab: evenly distribute MRs among reviewers bss/dr.botzo#8 --- dr_botzo/gitlab_bot/lib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index edecdea..63209b9 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -193,7 +193,7 @@ class GitlabBot(object): if merge_request.assignee is None or merge_request.assignee.username not in review_candidates: if len(review_candidates) > 0: - new_reviewer = random.choice(review_candidates) + new_reviewer = review_candidates[merge_request.iid % len(review_candidates)] log.debug("%s is the new reviewer", new_reviewer) # get the user object for the new reviewer @@ -219,7 +219,7 @@ class GitlabBot(object): if merge_request.assignee is None or merge_request.assignee.username not in accept_candidates: if len(accept_candidates) > 0: - new_accepter = random.choice(accept_candidates) + new_accepter = accept_candidates[merge_request.iid % len(accept_candidates)] log.debug("%s is the new accepter", new_accepter) # get the user object for the new accepter From a944bf70fe0e740ef2c7829860dd3bb662c3b423 Mon Sep 17 00:00:00 2001 From: "Brian S. Stephan" Date: Fri, 11 Nov 2016 09:23:14 -0600 Subject: [PATCH 16/16] gitlab: reassign MR to reviewer when resetting --- dr_botzo/gitlab_bot/lib.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dr_botzo/gitlab_bot/lib.py b/dr_botzo/gitlab_bot/lib.py index 63209b9..c4e621b 100644 --- a/dr_botzo/gitlab_bot/lib.py +++ b/dr_botzo/gitlab_bot/lib.py @@ -131,6 +131,7 @@ class GitlabBot(object): # only set the unlogged approval reset flag if there's some kind of progress if len(request_state.approver_list) > 0: request_state.unlogged_approval_reset = True + request_state.needs_reviewer = True request_state.approver_list.clear() else: log.debug("leaving the approval list as it is, i don't think '%s' is a push", note.body)