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

[WIP] brainstorming for /vote fast + prototype [WILL NOT BE UN-WIP-ED] #493

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions cron/poll_pull_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import os
import sys
from os.path import join, abspath, dirname
from lib.db.models import MeritocracyMentioned
from lib.db.models import MeritocracyMentioned, Issue

import settings
import github_api as gh
Expand Down Expand Up @@ -65,16 +65,41 @@ def poll_pull_requests(api):
# is our PR approved or rejected?
vote_total, variance = gh.voting.get_vote_sum(api, votes, contributors)
threshold = gh.voting.get_approval_threshold(api, settings.URN)
is_approved = vote_total >= threshold and meritocracy_satisfied
is_approved = vote_total >= threshold and meritocracy_satisfied > 0

seconds_since_updated = gh.prs.seconds_since_updated(api, pr)

half_window = False

# read the db to see if this PR is expedited by /vote fast...
try:
issue = Issue.get(issue_id=pr["id"])
half_window = Issue.expedited and \
meritocracy_satisfied >= settings.FAST_PR_MERITOCRATS

except Issue.DoesNotExist:
pass # not expedited

except:
log.exception("Failed to get expedited status")

voting_window = base_voting_window

if half_window:
voting_window /= 2

# Add the "expedited label"
gh.issues.label_issue(api, settings.URN, pr["number"], "expedited")

# the PR is mitigated or the threshold is not reached ?
if variance >= threshold or not is_approved:
voting_window = gh.voting.get_extended_voting_window(api, settings.URN)

if half_window:
voting_window /= 2

if (settings.IN_PRODUCTION and vote_total >= threshold / 2 and
seconds_since_updated > base_voting_window and not meritocracy_satisfied):
seconds_since_updated > base_voting_window and meritocracy_satisfied == 0):
# check if we need to mention the meritocracy
try:
commit = pr["head"]["sha"]
Expand All @@ -96,14 +121,14 @@ def poll_pull_requests(api):

gh.prs.post_accepted_status(
api, settings.URN, pr, seconds_since_updated, voting_window, votes, vote_total,
threshold, meritocracy_satisfied)
threshold, meritocracy_satisfied > 0)

if in_window:
__log.info("PR %d approved for merging!", pr_num)

try:
sha = gh.prs.merge_pr(api, settings.URN, pr, votes, vote_total,
threshold, meritocracy_satisfied)
threshold, meritocracy_satisfied > 0)
# some error, like suddenly there's a merge conflict, or some
# new commits were introduced between finding this ready pr and
# merging it
Expand All @@ -115,7 +140,7 @@ def poll_pull_requests(api):

gh.comments.leave_accept_comment(
api, settings.URN, pr_num, sha, votes, vote_total,
threshold, meritocracy_satisfied)
threshold, meritocracy_satisfied > 0)
gh.issues.label_issue(api, settings.URN, pr_num, ["accepted"])

# chaosbot rewards merge owners with a follow
Expand All @@ -130,21 +155,21 @@ def poll_pull_requests(api):
if in_window:
gh.prs.post_rejected_status(
api, settings.URN, pr, seconds_since_updated, voting_window, votes,
vote_total, threshold, meritocracy_satisfied)
vote_total, threshold, meritocracy_satisfied > 0)
__log.info("PR %d rejected, closing", pr_num)
gh.comments.leave_reject_comment(
api, settings.URN, pr_num, votes, vote_total, threshold,
meritocracy_satisfied)
meritocracy_satisfied > 0)
gh.issues.label_issue(api, settings.URN, pr_num, ["rejected"])
gh.prs.close_pr(api, settings.URN, pr)
elif vote_total < 0:
gh.prs.post_rejected_status(
api, settings.URN, pr, seconds_since_updated, voting_window, votes,
vote_total, threshold, meritocracy_satisfied)
vote_total, threshold, meritocracy_satisfied > 0)
else:
gh.prs.post_pending_status(
api, settings.URN, pr, seconds_since_updated, voting_window, votes,
vote_total, threshold, meritocracy_satisfied)
vote_total, threshold, meritocracy_satisfied > 0)

for user in votes:
if user in total_votes:
Expand Down
87 changes: 81 additions & 6 deletions cron/poll_read_issue_comments.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import logging
import arrow
import re
import json
from requests.exceptions import HTTPError

import settings
import github_api as gh

from lib.db.models import Comment, User, ActiveIssueCommands, Issue
from lib.db.models import RunTimes, InactiveIssueCommands
from lib.db.models import RunTimes, InactiveIssueCommands, MeritocracyMentioned

'''
Command Syntax
Expand All @@ -21,7 +22,7 @@

# If no subcommands, map cmd: None
COMMAND_LIST = {
"/vote": ("close", "reopen")
"/vote": ("close", "reopen", "fast")
}

__log = logging.getLogger("read_issue_comments")
Expand All @@ -35,13 +36,31 @@ def get_seconds_remaining(api, comment_id):
return seconds_remaining


def insert_or_update_issue(api, issue_id, number):
# get more info on the issue
gh_issue = gh.issue.open_issue(api, settings.URN, number)

# get user from db
user, _ = User.get_or_create(user_id=gh_issue["user"]["id"],
defaults={"login": gh_issue["user"]["login"]})

# db insert
issue, _ = Issue.get_or_create(issue_id=issue_id,
number=number,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does anyone know what the difference between these is? Can I drop one of them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend using the defaults param here for anything not in the primary key. We were having issues using this syntax before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better?

user=user,
created_at=gh_issue["created_at"],
expedited=False,
is_pr="pull_request" in gh_issue)

return issue


def insert_or_update(api, cmd_obj):
# Find the comment, or create it if it doesn't exit
comment_id = cmd_obj["global_comment_id"]
issue, _ = Issue.get_or_create(issue_id=cmd_obj["issue_id"])
user, _ = User.get_or_create(user_id=cmd_obj["user"]["id"],
defaults={"login": cmd_obj["user"]["login"]})

issue = insert_or_update_issue(api, cmd_obj["issue_id"], cmd_obj["number"])
Copy link
Contributor

Choose a reason for hiding this comment

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

where are you getting cmd_obj["number"] from? I don't think this exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it ought to be just returned from github, right: https://developer.github.com/v3/issues/#list-issues-for-a-repository?

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Read comments.py / get_all_issue_comments. It doesn't return the entire structure. You might need to change that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. Fixed

comment, _ = Comment.get_or_create(comment_id=comment_id,
defaults={
"user": user, "text": cmd_obj["comment_text"],
Expand Down Expand Up @@ -135,7 +154,21 @@ def get_command_votes(api, urn, comment_id):
return votes


def handle_vote_command(api, command, issue_id, comment_id, votes):
def get_meritocracy():
# FIXME: update when this is done by the db
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the meritocracy will ever be stored in the DB. Instead, put this code in the function.

meritocracy = {}
with open('server/meritocracy.json', 'r') as mfp:
fs = fp.read()
if fs:
meritocracy = json.loads(fs)

return meritocracy


def handle_vote_command(api, command, cmdmeta, votes):
issue_id = cmdmeta.issue.issue_id
comment_id = cmdmeta.comment.comment_id

orig_command = command[:]
# Check for correct command syntax, ie, subcommands
log_warning = False
Expand All @@ -145,6 +178,48 @@ def handle_vote_command(api, command, issue_id, comment_id, votes):
gh.issues.close_issue(api, settings.URN, issue_id)
elif sub_command == "reopen":
gh.issues.open_issue(api, settings.URN, issue_id)
elif sub_command == "fast":
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe pull this out into it's own function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

comment_updated_at = cmdmeta.comment.updated_at
comment_created_at = cmdmeta.comment.created_at
comment_poster = cmdmeta.comment.user
issue_poster = cmdmeta.issue.user
issue_created_at = cmdmeta.issue.created_at

# This must be a PR
if not cmdmeta.issue.is_pr:
return

# The post should not have been updated
if updated_at != created_at:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe leave some kind of response indicating why the command didn't run? Makes debugging easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... yes, I see your point... but would this be prone to inducing spam?

return

# The comment poster must be in the meritocracy
meritocracy = get_meritocracy()

if comment_poster not in meritocracy:
return

# The comment must posted within 5 min of the issue
if (arrow.get(comment_created) - arrow.get(issue_created_at)).total_seconds() > 5*60:
return

# Leave a note on the issue that it is expedited
Issue.update(expedited=True).where(Issue.issue_id == cmdmeta.issue.issue_id).execute()

# mention the meritocracy immediately
try:
pr = gh.prs.get_pr(api, settings.URN, issue.number)
commit = pr["head"]["sha"]

mm, created = MeritocracyMentioned.get_or_create(commit_hash=commit)
if created:
meritocracy_mentions = meritocracy - {pr["user"]["login"].lower(),
"chaosbot"}
gh.comments.leave_expedite_comment(api, settings.URN, pr["number"],
meritocracy_mentions)
except:
__log.exception("Failed to process meritocracy mention")

else:
# Implement other commands
pass
Expand Down Expand Up @@ -176,7 +251,7 @@ def handle_comment(api, cmd):
comment=comment_text))

if command == "/vote":
handle_vote_command(api, parsed_comment, issue_id, comment_id, votes)
handle_vote_command(api, parsed_comment, cmd, votes)

update_command_ran(api, comment_id, "Command Ran")

Expand Down
11 changes: 11 additions & 0 deletions github_api/comments.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,17 @@ def leave_meritocracy_comment(api, urn, pr, meritocracy):
return leave_comment(api, urn, pr, body)


def leave_expedite_comment(api, urn, pr, meritocracy):
meritocracy_str = " ".join(map(lambda user: "@" + user, meritocracy))
body = """
:warning: This PR is nominated to be **expedited**.
This requires **{num}** positive reviews from the meritocracy.

Please review: {meritocracy}
""".strip().format(meritocracy=meritocracy_str, num=settings.FAST_PR_MERITOCRATS)
return leave_comment(api, urn, pr, body)


def leave_deleted_comment(api, urn, pr):
body = """
:no_entry: The repository backing this PR has been deleted.
Expand Down
5 changes: 2 additions & 3 deletions github_api/voting.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import settings


def get_votes(api, urn, pr, meritocracy):
""" return a mapping of username => -1 or 1 for the votes on the current
state of a pr. we consider comments and reactions, but only from users who
Expand All @@ -18,7 +17,7 @@ def get_votes(api, urn, pr, meritocracy):
can't acquire approval votes, then change the pr """

votes = {}
meritocracy_satisfied = False
meritocracy_satisfied = 0
pr_owner = pr["user"]["login"]
pr_num = pr["number"]

Expand All @@ -33,7 +32,7 @@ def get_votes(api, urn, pr, meritocracy):
for vote_owner, (is_current, vote) in reviews.items():
if (vote > 0 and is_current and vote_owner != pr_owner
and vote_owner.lower() in meritocracy):
meritocracy_satisfied = True
meritocracy_satisfied += 1
break

# by virtue of creating the PR, the owner defaults to a vote of 1
Expand Down
5 changes: 5 additions & 0 deletions lib/db/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ class Comment(BaseModel):

class Issue(BaseModel):
issue_id = pw.IntegerField(primary_key=True)
number = pw.IntegerField()
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR number as oppose to PR id? The thing is when github returns a PR, it returns a value called "id" which is not the PR number we see on github, and it returns a "number" which is the number we see on github. TBH, I am really confused about which to use where. I think we want to use issue/PR numbers, rather than ids, but the db models appear to use the ids. Any help/auditing on this point would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The ids are used when making api calls. I think that's why we store them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... interesting. Most of the API I saw for issues and PRs take the issue/PR number, though?

created_at = pw.DateField()
user = pw.ForeignKeyField(User, related_name='poster')
is_pr = pw.BooleanField()
expedited = pw.BooleanField()


class ActiveIssueCommands(BaseModel):
Expand Down
6 changes: 5 additions & 1 deletion settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@
"mergeable": "dddddd",
"can't merge": "ededed",
"ci failed": "ff9800",
"crash report": "ff0000"
"crash report": "ff0000",
"expedited": "f49542",
}

# PRs that have merge conflicts and haven't been touched in this many hours
Expand Down Expand Up @@ -124,6 +125,9 @@
# Make sure usernames are lowercased
MERITOCRACY_VOTERS_BLACKLIST = {user.lower() for user in MERITOCRACY_VOTERS_BLACKLIST}

# Number of meritocrats needed to expedite a PR
FAST_PR_MERITOCRATS = 5

# Database settings
DB_ADAPTER = "sqlite"
DB_CONFIG = {
Expand Down