From 3fb4891295af5b820901f8f1c4ba45d0c75aed09 Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 10:19:27 -0700 Subject: [PATCH 01/12] First pass at Swarm interface integration for inline comments --- inlineplz/interfaces/__init__.py | 4 +- inlineplz/interfaces/github.py | 19 ++++---- inlineplz/interfaces/swarm.py | 82 ++++++++++++++++++++++++++++++++ inlineplz/message.py | 4 +- setup.py | 3 +- 5 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 inlineplz/interfaces/swarm.py diff --git a/inlineplz/interfaces/__init__.py b/inlineplz/interfaces/__init__.py index 330d4352..48ca8be0 100644 --- a/inlineplz/interfaces/__init__.py +++ b/inlineplz/interfaces/__init__.py @@ -4,7 +4,9 @@ from __future__ import unicode_literals from inlineplz.interfaces.github import GitHubInterface +from inlineplz.interfaces.swarm import SwarmInterface INTERFACES = { - 'github': GitHubInterface + 'github': GitHubInterface, + 'swarm': SwarmInterface } diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index 2392273e..f6b78d4a 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -69,35 +69,36 @@ def post_messages(self, messages, max_comments): return messages_to_post if not msg.comments: continue - msg_position = self.position(msg) + msg_path = os.path.relpath(msg.path).replace('\\', '/').strip() + msg_position = self.position(msg, msg_path) if msg_position: messages_to_post += 1 - if not self.is_duplicate(msg, msg_position): + if not self.is_duplicate(msg, msg_path, msg_position): # skip this message if we already have too many comments on this file # max comments / 5 is an arbitrary number i totally made up. should maybe be configurable. - if paths.setdefault(msg.path, 0) > max_comments // 5: + if paths.setdefault(msg_path, 0) > max_comments // 5: continue try: print('Creating review comment: {0}'.format(msg)) self.pull_request.create_review_comment( self.format_message(msg), self.last_sha, - msg.path, + msg_path, msg_position ) except github3.GitHubError: pass - paths[msg.path] += 1 + paths[msg_path] += 1 messages_posted += 1 if max_comments >= 0 and messages_posted > max_comments: break print('{} messages posted to Github.'.format(messages_to_post)) return messages_to_post - def is_duplicate(self, message, position): + def is_duplicate(self, message, path, position): for comment in self.pull_request.review_comments(): if (comment.position == position and - comment.path == message.path and + comment.path == path and comment.body.strip() == self.format_message(message).strip()): return True return False @@ -114,14 +115,14 @@ def format_message(message): ) return '`{0}`'.format(list(message.comments)[0].strip()) - def position(self, message): + def position(self, message, path): """Calculate position within the PR, which is not the line number""" if not message.line_number: message.line_number = 1 patch = unidiff.PatchSet(self.diff.split('\n')) for patched_file in patch: target = patched_file.target_file.lstrip('b/') - if target == message.path: + if target == path: offset = 1 for hunk_no, hunk in enumerate(patched_file): for position, hunk_line in enumerate(hunk): diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py new file mode 100644 index 00000000..92be778e --- /dev/null +++ b/inlineplz/interfaces/swarm.py @@ -0,0 +1,82 @@ +# -*- coding: utf-8 -*- + +import requests +import random + +from inlineplz.interfaces.base import InterfaceBase + +class SwarmInterface(InterfaceBase): + def __init__(self, username, password, host, topic, version='v8'): + self.username = username + self.password = password + self.host = host + self.topic = topic + self.version = version + + def post_messages(self, messages, max_comments): + # randomize message order to more evenly distribute messages across different files + messages = list(messages) + random.shuffle(messages) + + messages_to_post = 0 + messages_posted = 0 + current_comments = self.get_comments(max_comments) + for msg in messages: + if not msg.comments: + continue + messages_to_post += 1 + body = self.format_message(msg) + if self.is_duplicate(current_comments, msg, body): + continue + # try to send swarm post comment + self.post_comment(msg) + messages_posted += 1 + if max_comments >= 0 and messages_posted > max_comments: + break + print('{} messages posted to Swarm.'.format(messages_to_post)) + return messages_to_post + + def post_comment(self, msg): + # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 + url = "https://{}/api/{}/comments".format(self.host, self.version) + payload = { + 'topic': self.topic, + 'body': self.format_message(msg), + 'context[file]': msg.path, + 'context[rightLine]': msg.line_number + } + r = requests.post(url, auth=(self.username, self.password), data=payload) + + def get_comments(self, max_comments=100): + # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 + parameters = "topic={}&max={}".format(self.topic, max_comments) + url = "https://{}/api/{}/comments".format(self.host, self.version) + r = requests.get(url, auth=(self.username, self.password)) + if (r.status_code != 200): + return {} + return r.json()["comments"] + + @staticmethod + def is_duplicate(comments, msg, body): + for comment in comments: + try: + if (comment["context"]["rightLine"] == msg.line_number): + if (comment["context"]["file"] == msg.path): + if (comment["body"].strip() == body.strip()): + return True + except (KeyError, TypeError) as e: + continue + return False + + @staticmethod + def format_message(message): + if not message.comments: + return '' + if len(message.comments) > 1: + return ( + '```\n' + + '\n'.join(sorted(list(message.comments))) + + '\n```' + ) + return '`{0}`'.format(list(message.comments)[0].strip()) + diff --git a/inlineplz/message.py b/inlineplz/message.py index 8a8c22d8..43577a3c 100644 --- a/inlineplz/message.py +++ b/inlineplz/message.py @@ -15,7 +15,7 @@ def __init__(self): self.messages = {} def add_message(self, path, line, message): - path = os.path.relpath(path).replace('\\', '/').strip() + path = path.replace('\\', '/').strip() # replace backticks with single quotes to avoid markdown escaping issues message = message.replace('`', '\'').strip() try: @@ -42,7 +42,7 @@ def get_messages(self): class Message(object): def __init__(self, path, line_number): - self.path = os.path.relpath(path).replace('\\', '/') + self.path = path.replace('\\', '/').strip() self.line_number = int(line_number) self.comments = set() diff --git a/setup.py b/setup.py index 5d0b152c..d8804d15 100644 --- a/setup.py +++ b/setup.py @@ -19,7 +19,8 @@ 'uritemplate.py', 'dirtyjson', 'python-dateutil', - 'git-url-parse' + 'git-url-parse', + 'requests' ] test_requirements = [ From 5bb35a4919a13e17660eebc25d4675eda41ff379 Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 11:09:49 -0700 Subject: [PATCH 02/12] Changed how args are passed to interfaces updates from PR comments --- inlineplz/interfaces/github.py | 27 ++++++++++++++++++++++++- inlineplz/interfaces/swarm.py | 20 +++++++++---------- inlineplz/main.py | 36 ++++++---------------------------- 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index f6b78d4a..a0d7173a 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -15,7 +15,32 @@ class GitHubInterface(InterfaceBase): - def __init__(self, owner, repo, pr, token, url=None): + def __init__(self, args): + url = args.url + if args.repo_slug: + owner = args.repo_slug.split('/')[0] + repo = args.repo_slug.split('/')[1] + else: + owner = args.owner + repo = args.repo + if args.url: + try: + url_to_parse = args.url + # giturlparse won't parse URLs that don't end in .git + if not url_to_parse.endswith('.git'): + url_to_parse += '.git' + parsed = giturlparse.parse(url_to_parse) + url = parsed.resource + if not url.startswith('https://'): + url = 'https://' + url + owner = parsed.owner + repo = parsed.name + except giturlparse.parser.ParserError: + pass + + pr = args.pull_request + token = args.token + self.github = None # TODO: support non-PR runs try: diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index 92be778e..175807e7 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -6,12 +6,12 @@ from inlineplz.interfaces.base import InterfaceBase class SwarmInterface(InterfaceBase): - def __init__(self, username, password, host, topic, version='v8'): - self.username = username - self.password = password - self.host = host - self.topic = topic - self.version = version + def __init__(self, args): + self.username = args.username + self.password = args.password + self.host = args.host + self.topic = args.topic + self.version = 'v8' def post_messages(self, messages, max_comments): # randomize message order to more evenly distribute messages across different files @@ -52,7 +52,7 @@ def get_comments(self, max_comments=100): parameters = "topic={}&max={}".format(self.topic, max_comments) url = "https://{}/api/{}/comments".format(self.host, self.version) r = requests.get(url, auth=(self.username, self.password)) - if (r.status_code != 200): + if (r.status_code != requests.code.ok): return {} return r.json()["comments"] @@ -60,9 +60,9 @@ def get_comments(self, max_comments=100): def is_duplicate(comments, msg, body): for comment in comments: try: - if (comment["context"]["rightLine"] == msg.line_number): - if (comment["context"]["file"] == msg.path): - if (comment["body"].strip() == body.strip()): + if (comment["context"]["rightLine"] == msg.line_number and + comment["context"]["file"] == msg.path and + comment["body"].strip() == body.strip()): return True except (KeyError, TypeError) as e: continue diff --git a/inlineplz/main.py b/inlineplz/main.py index 5cca7af9..f3766747 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -27,7 +27,11 @@ def main(): parser.add_argument('--repo-slug', type=str) parser.add_argument('--token', type=str) parser.add_argument('--interface', type=str, choices=interfaces.INTERFACES) - parser.add_argument('--url', type=str) + parser.add_argument('--url', type=str, default=None) + parser.add_argument('--username', type=str) + parser.add_argument('--password', type=str) + parser.add_argument('--host', type=str) + parser.add_argument('--topic', type=str) parser.add_argument('--enabled-linters', type=str, nargs='+') parser.add_argument('--disabled-linters', type=str, nargs='+') parser.add_argument('--dryrun', action='store_true') @@ -119,28 +123,6 @@ def inline(args): trusted = args.trusted args = load_config(args) - # TODO: consider moving this git parsing stuff into the github interface - url = args.url - if args.repo_slug: - owner = args.repo_slug.split('/')[0] - repo = args.repo_slug.split('/')[1] - else: - owner = args.owner - repo = args.repo - if args.url: - try: - url_to_parse = args.url - # giturlparse won't parse URLs that don't end in .git - if not url_to_parse.endswith('.git'): - url_to_parse += '.git' - parsed = giturlparse.parse(url_to_parse) - url = parsed.resource - if not url.startswith('https://'): - url = 'https://' + url - owner = parsed.owner - repo = parsed.name - except giturlparse.parser.ParserError: - pass if not args.dryrun and args.interface not in interfaces.INTERFACES: print('Valid inline-plz config not found') return 1 @@ -162,13 +144,7 @@ def inline(args): print_messages(messages) return 0 try: - my_interface = interfaces.INTERFACES[args.interface]( - owner, - repo, - args.pull_request, - args.token, - url - ) + my_interface = interfaces.INTERFACES[args.interface](args) if my_interface.post_messages(messages, args.max_comments) and not args.zero_exit: return 1 except KeyError: From 83bfc140cec182aa8496b602335a92a44265883a Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 13:50:51 -0700 Subject: [PATCH 03/12] Fixes after linter tests --- inlineplz/interfaces/swarm.py | 55 ++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index 175807e7..aa1e85ba 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -2,6 +2,7 @@ import requests import random +import subprocess from inlineplz.interfaces.base import InterfaceBase @@ -26,45 +27,56 @@ def post_messages(self, messages, max_comments): continue messages_to_post += 1 body = self.format_message(msg) - if self.is_duplicate(current_comments, msg, body): + proc = subprocess.Popen(["p4", "fstat", "-T", "depotFile", msg.path], stdout=subprocess.PIPE) + output = proc.stdout.read() + l = output.split() + if (len(l) != 3): + print "Can't find depotFile for '{}': {}".format(msg.path, output) + continue + path = output.split()[2] + if self.is_duplicate(current_comments, body, path, msg.line_number): + print "Duplicate for {}:{}".format(path, msg.line_number) continue # try to send swarm post comment - self.post_comment(msg) + self.post_comment(body, path, msg.line_number) messages_posted += 1 if max_comments >= 0 and messages_posted > max_comments: break print('{} messages posted to Swarm.'.format(messages_to_post)) return messages_to_post - def post_comment(self, msg): + def post_comment(self, body, path, line_number): # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 url = "https://{}/api/{}/comments".format(self.host, self.version) payload = { 'topic': self.topic, - 'body': self.format_message(msg), - 'context[file]': msg.path, - 'context[rightLine]': msg.line_number + 'body': body, + 'context[file]': path, + 'context[rightLine]': line_number } + print payload r = requests.post(url, auth=(self.username, self.password), data=payload) def get_comments(self, max_comments=100): # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 parameters = "topic={}&max={}".format(self.topic, max_comments) - url = "https://{}/api/{}/comments".format(self.host, self.version) - r = requests.get(url, auth=(self.username, self.password)) - if (r.status_code != requests.code.ok): + url = "https://{}/api/{}/comments?{}".format(self.host, self.version, parameters) + response = requests.get(url, auth=(self.username, self.password)) + if (response.status_code != requests.codes.ok): + print "Can't get comments, status code: {}".format(response.status_code) return {} - return r.json()["comments"] + return response.json()["comments"] @staticmethod - def is_duplicate(comments, msg, body): + def is_duplicate(comments, body, path, line_number): for comment in comments: try: - if (comment["context"]["rightLine"] == msg.line_number and - comment["context"]["file"] == msg.path and + if (comment["context"]["rightLine"] == line_number and + comment["context"]["file"] == path and comment["body"].strip() == body.strip()): - return True - except (KeyError, TypeError) as e: + print "Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"]) + return True + except (KeyError, TypeError) as exceptError: continue return False @@ -72,11 +84,8 @@ def is_duplicate(comments, msg, body): def format_message(message): if not message.comments: return '' - if len(message.comments) > 1: - return ( - '```\n' + - '\n'.join(sorted(list(message.comments))) + - '\n```' - ) - return '`{0}`'.format(list(message.comments)[0].strip()) - + return ( + '```\n' + + '\n'.join(sorted(list(message.comments))) + + '\n```' + ) From 44c25850d0ed9ae1c50783aa14f0145a1603108a Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 14:23:18 -0700 Subject: [PATCH 04/12] prints with parathensis for that python3 --- inlineplz/interfaces/swarm.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index 1c6cd6d1..ccef50bc 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -38,11 +38,11 @@ def post_messages(self, messages, max_comments): output = proc.stdout.read() l = output.split() if (len(l) != 3): - print "Can't find depotFile for '{}': {}".format(msg.path, output) + print("Can't find depotFile for '{}': {}".format(msg.path, output)) continue path = output.split()[2] if self.is_duplicate(current_comments, body, path, msg.line_number): - print "Duplicate for {}:{}".format(path, msg.line_number) + print("Duplicate for {}:{}".format(path, msg.line_number)) continue # try to send swarm post comment self.post_comment(body, path, msg.line_number) @@ -61,7 +61,7 @@ def post_comment(self, body, path, line_number): 'context[file]': path, 'context[rightLine]': line_number } - print payload + print("".format(payload)) r = requests.post(url, auth=(self.username, self.password), data=payload) def get_comments(self, max_comments=100): @@ -70,7 +70,7 @@ def get_comments(self, max_comments=100): url = "https://{}/api/{}/comments?{}".format(self.host, self.version, parameters) response = requests.get(url, auth=(self.username, self.password)) if (response.status_code != requests.codes.ok): - print "Can't get comments, status code: {}".format(response.status_code) + print("Can't get comments, status code: {}".format(response.status_code)) return {} return response.json()["comments"] @@ -81,7 +81,7 @@ def is_duplicate(comments, body, path, line_number): if (comment["context"]["rightLine"] == line_number and comment["context"]["file"] == path and comment["body"].strip() == body.strip()): - print "Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"]) + print("Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"])) return True except (KeyError, TypeError) as exceptError: continue From 136d13e933096f86261ea4c2e21daaed330a469b Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 14:41:47 -0700 Subject: [PATCH 05/12] fixes from failed CI and PR comments --- inlineplz/interfaces/github.py | 2 ++ inlineplz/interfaces/swarm.py | 9 ++++++--- inlineplz/main.py | 7 +++---- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index 52179e4e..9766a4f8 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -9,6 +9,7 @@ import github3 import unidiff +import giturlparse from inlineplz.interfaces.base import InterfaceBase from inlineplz.util import git, system @@ -52,6 +53,7 @@ def __init__(self, args): pr = args.pull_request token = args.token + branch = args.branch self.github = None if not url or url == 'https://github.com': diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index ccef50bc..cf76cd99 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -1,9 +1,10 @@ # -*- coding: utf-8 -*- -import requests import random import subprocess +import requests + from inlineplz.interfaces.base import InterfaceBase class SwarmInterface(InterfaceBase): @@ -19,6 +20,8 @@ def __init__(self, args): self.password = args.password self.host = args.host self.topic = args.topic + # current implementation uses version 8 of the implementation + # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Swarm_API%3FTocPath%3DSwarm%2520API%7C_____0 self.version = 'v8' def post_messages(self, messages, max_comments): @@ -62,7 +65,7 @@ def post_comment(self, body, path, line_number): 'context[rightLine]': line_number } print("".format(payload)) - r = requests.post(url, auth=(self.username, self.password), data=payload) + requests.post(url, auth=(self.username, self.password), data=payload) def get_comments(self, max_comments=100): # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 @@ -83,7 +86,7 @@ def is_duplicate(comments, body, path, line_number): comment["body"].strip() == body.strip()): print("Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"])) return True - except (KeyError, TypeError) as exceptError: + except (KeyError, TypeError): continue return False diff --git a/inlineplz/main.py b/inlineplz/main.py index 5d1a11c3..e0b0b494 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -10,7 +10,6 @@ import sys import time -import giturlparse import yaml from inlineplz import interfaces @@ -21,12 +20,12 @@ def main(): parser = argparse.ArgumentParser() - parser.add_argument('--pull-request', type=int) + parser.add_argument('--pull-request', type=int, default=None) parser.add_argument('--owner', type=str) parser.add_argument('--repo', type=str) parser.add_argument('--repo-slug', type=str) - parser.add_argument('--branch', type=str) - parser.add_argument('--token', type=str) + parser.add_argument('--branch', type=str, default=None) + parser.add_argument('--token', type=str, default=None) parser.add_argument('--interface', type=str, choices=interfaces.INTERFACES) parser.add_argument('--url', type=str, default=None) parser.add_argument('--username', type=str) From 67cf5776054a68668c99ac7ef686ea999ef21660 Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 15:02:39 -0700 Subject: [PATCH 06/12] integrate some PR comments --- inlineplz/interfaces/github.py | 4 ++-- inlineplz/interfaces/swarm.py | 10 ++++++++-- inlineplz/main.py | 14 ++++++-------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index 9766a4f8..caf5061d 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -51,8 +51,8 @@ def __init__(self, args): except giturlparse.parser.ParserError: pass - pr = args.pull_request - token = args.token + pr = args.review_id + token = args.credential branch = args.branch self.github = None diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index cf76cd99..f8b8462a 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -16,10 +16,16 @@ def __init__(self, args): host is the server (And any additional paths before the api) topic is the the review you are commenting on (for reviews, it will typically be "review/###" for some review number) """ + review_id = args.review_id + try: + review_id = int(review_id) + except (ValueError, TypeError): + print('{0} is not a valid review ID'.format(review_id)) + return self.username = args.username - self.password = args.password + self.password = args.credential self.host = args.host - self.topic = args.topic + self.topic = "review/{}".format(review_id) # current implementation uses version 8 of the implementation # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Swarm_API%3FTocPath%3DSwarm%2520API%7C_____0 self.version = 'v8' diff --git a/inlineplz/main.py b/inlineplz/main.py index e0b0b494..29b880fa 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -20,18 +20,16 @@ def main(): parser = argparse.ArgumentParser() - parser.add_argument('--pull-request', type=int, default=None) - parser.add_argument('--owner', type=str) - parser.add_argument('--repo', type=str) + parser.add_argument('--review-id', type=int, default=None) + parser.add_argument('--owner', type=str, help='the owner of the specified Git repo') + parser.add_argument('--repo', type=str, help='the repo to access through the Git interface') parser.add_argument('--repo-slug', type=str) parser.add_argument('--branch', type=str, default=None) - parser.add_argument('--token', type=str, default=None) + parser.add_argument('--password', type=str, default=None, help='credentials for accessing specified interface. This will be the token in Github or ticket/password for P4/Swarm.') parser.add_argument('--interface', type=str, choices=interfaces.INTERFACES) parser.add_argument('--url', type=str, default=None) - parser.add_argument('--username', type=str) - parser.add_argument('--password', type=str) - parser.add_argument('--host', type=str) - parser.add_argument('--topic', type=str) + parser.add_argument('--username', type=str, help='the username of the credentials used to access the specified interface') + parser.add_argument('--host', type=str, help='the hostname of the server that the interface will access. For Perforce, this is the base of the api url for swarm.') parser.add_argument('--enabled-linters', type=str, nargs='+') parser.add_argument('--disabled-linters', type=str, nargs='+') parser.add_argument('--dryrun', action='store_true') From 4c1a24481dfc027812cbef5b4792c3b7e272bf2a Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 16:01:01 -0700 Subject: [PATCH 07/12] fix breakage --- inlineplz/env/jenkins.py | 2 +- inlineplz/env/travis.py | 2 +- inlineplz/main.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/inlineplz/env/jenkins.py b/inlineplz/env/jenkins.py index 9ae2faee..862aee5f 100644 --- a/inlineplz/env/jenkins.py +++ b/inlineplz/env/jenkins.py @@ -23,7 +23,7 @@ def __init__(self): self.repo = os.environ.get('GITHUB_REPO_NAME') or os.environ.get('ghprbPullLink').split('/')[-3] self.commit = os.environ.get('ghprbActualCommit') self.interface = 'github' - self.token = os.environ.get('GITHUB_TOKEN') + self.credential = os.environ.get('GITHUB_TOKEN') spliturl = urlparse.urlsplit(os.environ.get('ghprbPullLink')) if spliturl.netloc != 'github.com': self.url = '{0}://{1}'.format(spliturl.scheme, spliturl.netloc) diff --git a/inlineplz/env/travis.py b/inlineplz/env/travis.py index 889814b1..434efea2 100644 --- a/inlineplz/env/travis.py +++ b/inlineplz/env/travis.py @@ -18,4 +18,4 @@ def __init__(self): self.commit = os.environ.get('TRAVIS_COMMIT') self.commit_range = os.environ.get('TRAVIS_COMMIT_RANGE') self.interface = 'github' - self.token = os.environ.get('GITHUB_TOKEN') + self.credential = os.environ.get('GITHUB_TOKEN') diff --git a/inlineplz/main.py b/inlineplz/main.py index 29b880fa..07511dc3 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -65,7 +65,7 @@ def main(): def update_from_config(args, config): blacklist = [ - 'trusted', 'token', 'interface', 'owner', 'repo', 'config_dir' + 'trusted', 'credential', 'interface', 'owner', 'repo', 'config_dir' 'repo_slug', 'pull_request', 'zero_exit', 'dryrun', 'url', 'branch' ] for key, value in config.items(): From 0ab9b87a8362019628c82859de871e4f2292d48f Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 16:57:35 -0700 Subject: [PATCH 08/12] fix password break --- inlineplz/env/jenkins.py | 2 +- inlineplz/env/travis.py | 2 +- inlineplz/interfaces/github.py | 2 +- inlineplz/interfaces/swarm.py | 2 +- inlineplz/main.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/inlineplz/env/jenkins.py b/inlineplz/env/jenkins.py index 862aee5f..1bee3a6a 100644 --- a/inlineplz/env/jenkins.py +++ b/inlineplz/env/jenkins.py @@ -23,7 +23,7 @@ def __init__(self): self.repo = os.environ.get('GITHUB_REPO_NAME') or os.environ.get('ghprbPullLink').split('/')[-3] self.commit = os.environ.get('ghprbActualCommit') self.interface = 'github' - self.credential = os.environ.get('GITHUB_TOKEN') + self.password = os.environ.get('GITHUB_TOKEN') spliturl = urlparse.urlsplit(os.environ.get('ghprbPullLink')) if spliturl.netloc != 'github.com': self.url = '{0}://{1}'.format(spliturl.scheme, spliturl.netloc) diff --git a/inlineplz/env/travis.py b/inlineplz/env/travis.py index 434efea2..dbac65da 100644 --- a/inlineplz/env/travis.py +++ b/inlineplz/env/travis.py @@ -18,4 +18,4 @@ def __init__(self): self.commit = os.environ.get('TRAVIS_COMMIT') self.commit_range = os.environ.get('TRAVIS_COMMIT_RANGE') self.interface = 'github' - self.credential = os.environ.get('GITHUB_TOKEN') + self.password = os.environ.get('GITHUB_TOKEN') diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index caf5061d..f581e163 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -52,7 +52,7 @@ def __init__(self, args): pass pr = args.review_id - token = args.credential + token = args.password branch = args.branch self.github = None diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index f8b8462a..eef4a365 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -23,7 +23,7 @@ def __init__(self, args): print('{0} is not a valid review ID'.format(review_id)) return self.username = args.username - self.password = args.credential + self.password = args.password self.host = args.host self.topic = "review/{}".format(review_id) # current implementation uses version 8 of the implementation diff --git a/inlineplz/main.py b/inlineplz/main.py index 07511dc3..951b32f8 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -65,7 +65,7 @@ def main(): def update_from_config(args, config): blacklist = [ - 'trusted', 'credential', 'interface', 'owner', 'repo', 'config_dir' + 'trusted', 'password', 'interface', 'owner', 'repo', 'config_dir' 'repo_slug', 'pull_request', 'zero_exit', 'dryrun', 'url', 'branch' ] for key, value in config.items(): From b58bbbe1fcd84fa97087e2a059faa28ae7082179 Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 20:14:57 -0700 Subject: [PATCH 09/12] minor adjustments and test print --- inlineplz/interfaces/github.py | 12 +++++++----- inlineplz/interfaces/swarm.py | 6 +++--- inlineplz/main.py | 5 +++-- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index f581e163..d13d6594 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -20,14 +20,15 @@ def __init__(self, args): """ GitHubInterface lets us post messages to GitHub. - owner and repo are the repository owner/organization and repo name respectively. + args.owner and args.repo are the repository owner/organization and repo name respectively. - pr is the ID number of the pull request. branch is the branch name. either pr OR branch - must be populated. + args.review_id is the ID number of the pull request. + args.branch is the branch name. + either args.review_id OR args.branch must be populated. - token is your GitHub API token. + args.password is your GitHub API token. - url is the base URL of your GitHub instance, such as https://github.com + args.url is the base URL of your GitHub instance, such as https://github.com """ url = args.url if args.repo_slug: @@ -63,6 +64,7 @@ def __init__(self, args): self.owner = owner self.repo = repo if branch and not pr: + print "{}, {}".format(owner, repo) github_repo = self.github.repository(self.owner, self.repo) for pull_request in github_repo.iter_pulls(): if pull_request.to_json()['head']['ref'] == branch: diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index eef4a365..4467460a 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -12,9 +12,9 @@ def __init__(self, args): """ SwarmInterface lets us post messages to Swarm (Helix). - username and password are the credentials used to access Swarm/Perforce. - host is the server (And any additional paths before the api) - topic is the the review you are commenting on (for reviews, it will typically be "review/###" for some review number) + args.username and args.password are the credentials used to access Swarm/Perforce. + args.host is the server (And any additional paths before the api) + args.review_id is the the review number you are commenting on """ review_id = args.review_id try: diff --git a/inlineplz/main.py b/inlineplz/main.py index 951b32f8..9af6fa57 100644 --- a/inlineplz/main.py +++ b/inlineplz/main.py @@ -67,6 +67,7 @@ def update_from_config(args, config): blacklist = [ 'trusted', 'password', 'interface', 'owner', 'repo', 'config_dir' 'repo_slug', 'pull_request', 'zero_exit', 'dryrun', 'url', 'branch' + 'username' ] for key, value in config.items(): if not key.startswith('_') and key not in blacklist: @@ -108,8 +109,8 @@ def inline(args): interface: How are we going to post comments? owner: Username of repo owner repo: Repository name - pr: Pull request ID - token: Authentication for repository + review_id: Pull request ID + password: Authentication for repository url: Root URL of repository (not your project) Default: https://github.com dryrun: Prints instead of posting comments. zero_exit: If true: always return a 0 exit code. From e0873a30430063d2d4780ad20fda1d949870eb2c Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 20:19:57 -0700 Subject: [PATCH 10/12] python3 --- inlineplz/interfaces/github.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index d13d6594..3cd58dc3 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -64,7 +64,7 @@ def __init__(self, args): self.owner = owner self.repo = repo if branch and not pr: - print "{}, {}".format(owner, repo) + print("{}, {}".format(owner, repo)) github_repo = self.github.repository(self.owner, self.repo) for pull_request in github_repo.iter_pulls(): if pull_request.to_json()['head']['ref'] == branch: From 51e87e192233c2f9d1f11c86c357ec61fe0f34d9 Mon Sep 17 00:00:00 2001 From: William Parks Date: Fri, 16 Mar 2018 21:21:18 -0700 Subject: [PATCH 11/12] change pull_request to review_id --- inlineplz/env/jenkins.py | 2 +- inlineplz/env/travis.py | 2 +- inlineplz/interfaces/github.py | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/inlineplz/env/jenkins.py b/inlineplz/env/jenkins.py index 1bee3a6a..707db852 100644 --- a/inlineplz/env/jenkins.py +++ b/inlineplz/env/jenkins.py @@ -18,7 +18,7 @@ class Jenkins(EnvBase): def __init__(self): if os.environ.get('ghprbPullId') or os.environ.get('ghprbActualCommit'): - self.pull_request = os.environ.get('ghprbPullId') + self.review_id = os.environ.get('ghprbPullId') self.owner = os.environ.get('GITHUB_REPO_OWNER') or os.environ.get('ghprbPullLink').split('/')[-4] self.repo = os.environ.get('GITHUB_REPO_NAME') or os.environ.get('ghprbPullLink').split('/')[-3] self.commit = os.environ.get('ghprbActualCommit') diff --git a/inlineplz/env/travis.py b/inlineplz/env/travis.py index dbac65da..ed6ade5e 100644 --- a/inlineplz/env/travis.py +++ b/inlineplz/env/travis.py @@ -12,7 +12,7 @@ class Travis(EnvBase): def __init__(self): - self.pull_request = os.environ.get('TRAVIS_PULL_REQUEST') + self.review_id = os.environ.get('TRAVIS_PULL_REQUEST') self.branch = os.environ.get('TRAVIS_BRANCH') self.repo_slug = os.environ.get('TRAVIS_REPO_SLUG') self.commit = os.environ.get('TRAVIS_COMMIT') diff --git a/inlineplz/interfaces/github.py b/inlineplz/interfaces/github.py index 3cd58dc3..a13f1ecd 100644 --- a/inlineplz/interfaces/github.py +++ b/inlineplz/interfaces/github.py @@ -64,7 +64,6 @@ def __init__(self, args): self.owner = owner self.repo = repo if branch and not pr: - print("{}, {}".format(owner, repo)) github_repo = self.github.repository(self.owner, self.repo) for pull_request in github_repo.iter_pulls(): if pull_request.to_json()['head']['ref'] == branch: From f67945d15fa98d8bc2d8074c25667c3817d8a684 Mon Sep 17 00:00:00 2001 From: William Parks Date: Mon, 19 Mar 2018 10:45:52 -0700 Subject: [PATCH 12/12] fixes based on comments --- README.rst | 10 ++++++---- inlineplz/interfaces/swarm.py | 20 ++++++++++++-------- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/README.rst b/README.rst index e1262298..8373f7fc 100644 --- a/README.rst +++ b/README.rst @@ -38,10 +38,12 @@ You probably want to run the above in a CI job, not in your regular development You'll also need to provide the following either in the command line or via environment variables: -* owner: the repo organization/owner -* repo: the repo name -* token: your auth token (encrypt this, don't put this in plaintext in any public configurations!) -* url: the url of your scm host +* owner: the repo organization/owner (for github) +* repo: the repo name (for github) +* password: your auth token for github (encrypt this, don't put this in plaintext in any public configurations!) or p4 ticket for swarm +* url: the url of your scm host (for github) +* username: username to acccess the interface (for swarm) +* host: the host part of the url for the interface api (for swarm) * interface: the type of scm host (such as github) Dependencies: diff --git a/inlineplz/interfaces/swarm.py b/inlineplz/interfaces/swarm.py index 4467460a..1fbd60ee 100644 --- a/inlineplz/interfaces/swarm.py +++ b/inlineplz/interfaces/swarm.py @@ -25,7 +25,7 @@ def __init__(self, args): self.username = args.username self.password = args.password self.host = args.host - self.topic = "review/{}".format(review_id) + self.topic = "reviews/{}".format(review_id) # current implementation uses version 8 of the implementation # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Swarm_API%3FTocPath%3DSwarm%2520API%7C_____0 self.version = 'v8' @@ -43,11 +43,14 @@ def post_messages(self, messages, max_comments): continue messages_to_post += 1 body = self.format_message(msg) - proc = subprocess.Popen(["p4", "fstat", "-T", "depotFile", msg.path], stdout=subprocess.PIPE) - output = proc.stdout.read() + try: + output = subprocess.check_output(["p4", "fstat", "-T", "depotFile", msg.path]) + except subprocess.CalledProcessError as procError: + print("Process call error: Can't find depotFile for '{}': {}".format(msg.path, procError.output)) + continue l = output.split() - if (len(l) != 3): - print("Can't find depotFile for '{}': {}".format(msg.path, output)) + if len(l) != 3: + print("Invalid output: Can't find depotFile for '{}': {}".format(msg.path, output)) continue path = output.split()[2] if self.is_duplicate(current_comments, body, path, msg.line_number): @@ -70,8 +73,10 @@ def post_comment(self, body, path, line_number): 'context[file]': path, 'context[rightLine]': line_number } - print("".format(payload)) - requests.post(url, auth=(self.username, self.password), data=payload) + #print("{}".format(payload)) + response = requests.post(url, auth=(self.username, self.password), data=payload) + if (response.status_code != requests.codes.ok): + print("Can't post comments, status code: {}".format(response.status_code)) def get_comments(self, max_comments=100): # https://www.perforce.com/perforce/doc.current/manuals/swarm/index.html#Swarm/swarm-apidoc.html#Comments___Swarm_Comments%3FTocPath%3DSwarm%2520API%7CAPI%2520Endpoints%7C_____3 @@ -90,7 +95,6 @@ def is_duplicate(comments, body, path, line_number): if (comment["context"]["rightLine"] == line_number and comment["context"]["file"] == path and comment["body"].strip() == body.strip()): - print("Dupe: {}:{}".format(comment["context"]["file"], comment["context"]["rightLine"])) return True except (KeyError, TypeError): continue