Update webhook and setup #1

Open
shekhar-cis wants to merge 5 commits from shekhar-cis/buildbot-transifex:transifex-webhook into master
Showing only changes of commit 89305ec602 - Show all commits

View File

@ -1,18 +1,14 @@
"""Transifex webhook handler """
import sys
import os import os

isort

isort
import base64 import base64
import time import time
import json import json
import re
import hmac import hmac
import pprint
import hashlib import hashlib
import requests
from subprocess import call from subprocess import call
from base64 import b64encode from base64 import b64encode

no need logging

no need `logging`

no need for logging

no need for `logging`
import requests
from buildbot.process.properties import Properties from buildbot.process.properties import Properties

we already imported base64

we already imported base64
from buildbot.util import bytes2unicode, unicode2bytes from buildbot.util import bytes2unicode, unicode2bytes
@ -25,152 +21,94 @@ from dateutil.parser import parse as dateparse
_HEADER_USER_AGENT = 'User-Agent' _HEADER_USER_AGENT = 'User-Agent'
_HEADER_SIGNATURE = 'X-TX-Signature' _HEADER_SIGNATURE = 'X-TX-Signature'
_HEADER_URL_PATH = 'X-TX-Url'
HTTP_DATE = 'date'
_EVENT_KEY = 'event' _EVENT_KEY = 'event'

why?

why?

_HEADER_DATE

`_HEADER_DATE`
transifexSecret = "" transifex_request_data = {}
transifexUsername = ""
transifexPassword = ""
transifex_dict = {}
secret = ""
master = ""

This is specific to a request so it shouldn't be a module variable. Wrong scope. At the very least, if there are multiple requests in parallel, this will cause collisions.

This is specific to a request so it shouldn't be a module variable. Wrong scope. At the very least, if there are multiple requests in parallel, this will cause collisions.
gitHubToken = os.environ('gitHubToken')

should be inside the map

should be inside the map

we don't need any of this

we don't need any of this
class TransifexHandler(BaseHookHandler): class TransifexHandler(BaseHookHandler):

should be arguments passed to constructor

should be arguments passed to constructor
def __init__(self, master, secret, transifex_dict): def __init__(self, master, secret, options, transifex_dict):
if not options:

should be extracted from properties or buildbot runtime variables

should be extracted from properties or buildbot runtime variables
options = {}
self.secret = secret self.secret = secret

options should probably be the last and default to None or something

`options` should probably be the last and default to `None` or something
self.master = master self.master = master
PeterSurda marked this conversation as resolved Outdated
self.options = options
self.transifex_dict = transifex_dict self.transifex_dict = transifex_dict
PeterSurda marked this conversation as resolved Outdated

something like transifex_to_github_map is a better name

something like `transifex_to_github_map` is a better name
def returnMessage(self, status = False, message = "Unimplemented"): def returnMessage(self, status = False, message = "Unimplemented"):
output = json.dumps({"status": "OK" if status else "FAIL", "message": message}) output = json.dumps({"status": "OK" if status else "FAIL", "message": message})
return [output, [('Content-type', 'text/plain'), return [output, [('Content-type', 'application/json')]]
('Content-Length', str(len(output)))
]]
def verifyTransifexSignature( def verifyTransifexSignature(
PeterSurda marked this conversation as resolved Outdated

_verifyTransifexSignature

`_verifyTransifexSignature`
self, request, content, rendered_secret, signature, header_signature self, request, content, signature, header_signature
): ):
http_verb = 'POST' http_verb = 'POST'
http_url_path = request.headers('X-TX-Url') http_url_path = request.getHeader(_HEADER_URL_PATH)
  • content-length is usually set by the web server automatically
  • content-type should maybe be application/json
- content-length is usually set by the web server automatically - content-type should maybe be `application/json`
http_gmt_date = request.headers('Date') http_gmt_date = request.getHeader(HTTP_DATE)
content_md5 = hashlib.md5(content).hexdigest() content_md5 = hashlib.md5(content).hexdigest()
msg = b'\n'.join([ msg = b'\n'.join([
http_verb, http_url_path, http_gmt_date, content_md5 http_verb, http_url_path, http_gmt_date, content_md5
PeterSurda marked this conversation as resolved Outdated

rendered_secret can be extracted directly, no need to pass it

rendered_secret can be extracted directly, no need to pass it
]) ])
tx_signature = base64.b64encode( tx_signature = base64.b64encode(
hmac.new( hmac.new(
PeterSurda marked this conversation as resolved
Review

should use constant

should use constant
key=rendered_secret, key=self.rendered_secret,

here also

here also
msg=msg, msg=msg,
digestmod=hashlib.sha256 digestmod=hashlib.sha256
).digest() ).digest()
) )
if tx_signature() != header_signature: if tx_signature != header_signature:
raise ValueError('Invalid secret')
try:
if signature != os.environ.get('HTTP_X_TX_SIGNATURE'):
return False
return True
except:
return False return False
def downloadTranslatedLanguage(self, ts, lang): if signature != request.getHeader(_HEADER_SIGNATURE):
headers = {"Authorization": "Basic " + b64encode(transifexUsername + ":" + transifexPassword)} return False

maybe just raise ValueError("Signature mismatch"), then it will propagate up

maybe just `raise ValueError("Signature mismatch")`, then it will propagate up
fname = "bitmessage_" + lang.lower() + ".po"
with open("src/translations/" + fname, "wt") as handle:
response = requests.get("https://www.transifex.com/api/2/project/pybitmessage/resource/pybitmessage/translation/" + lang + "/",
headers=headers)
if response.ok:
content = json.loads(response.content)["content"]
handle.write(content.encode("utf-8"))
return response
def commitTranslatedLanguage(self, ts, lang):
call(["kivy", "src/translations/messages.pro"])
call(["git", "add", "src/translations/bitmessage_" + lang + ".ts", "src/translations/bitmessage_" + lang + ".qm"])
call(["git", "commit", "-q", "-S", "-m", "Auto-updated language %s from transifex" % (lang)])
newbranch = "translate_" + lang + "_" + str(ts)
call(["git", "push", "-q", "translations", newbranch + ":" + newbranch])
branch = transifex_dict['branch']
request = {
"title": "Translation update " + lang,
"body": "Auto-updated from transifex",
"head": "PyBitmessageTranslations:" + newbranch,
"base": branch
}
headers = {"Authorization": "token " + gitHubToken}
response = requests.post("https://api.github.com/repos/Bitmessage/PyBitmessage/pulls",
headers=headers, data=json.dumps(request))
return response

return True missing

`return True` missing
def process_translation_completed(self, payload, transifex_dict, event_type, codebase): def process_translation_completed(self, payload, transifex_dict, event_type, codebase):
changes = [] changes = []

tx_signature isn't a function/method

`tx_signature` isn't a function/method
transifex_response = self._transform_variables(payload, transifex_dict) transifex_response = self._transform_variables(payload, transifex_dict)
PeterSurda marked this conversation as resolved Outdated

return False

`return False`
if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']: if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:

don't have weird constants here, rather process is through the map.

don't have weird constants here, rather process is through the map.

repository missing

`repository` missing
if 'translation_completed' in transifex_response['event'] and 100 in transifex_response['translated']: if 'translation_completed' in transifex_response['event']:

transifex_to_github_map is an object attribute, no need to pass it around

`transifex_to_github_map` is an object attribute, no need to pass it around
ts = int(time.time()) ts = int(time.time())
PeterSurda marked this conversation as resolved Outdated
  • use constant
  • use a default value, then no need for try/except
- use constant - use a default value, then no need for try/except
lang = transifex_response['language'] lang = transifex_response['language']

error handling missing here

error handling missing here
branch = transifex_dict['branch'] return
self.downloadTranslatedLanguage(ts, lang.lower())
response = self.commitTranslatedLanguage(ts, lang.lower())
if response.ok:
output, responseHeaders = self.returnMessage(True, "Processed.")
else:
output, responseHeaders = self.returnMessage(False, "Error: %i." % (response.status_code))
else:
output, responseHeaders = self.returnMessage(False, "Nothing to do")
else:
output, responseHeaders = self.returnMessage(False, "Nothing to do")

if you use .get with a default value, you can avoid try/except

if you use `.get` with a default value, you can avoid `try`/`except`
# if isinstance(self.options, dict) and self.options.get('onlyIncludePushCommit', False): # if isinstance(self.options, dict):

language = payload.get('language', "None")

`language = payload.get('language', "None")`
# commits = commits[:1] # commits = commits[:1]
Review

this should be the randomly generated one, the branch of the PR

this should be the randomly generated one, the branch of the PR
PeterSurda marked this conversation as resolved Outdated

This should be in the buildbot job, not here. The webhook only transforms data structures and puts them into a database.

  • "tx pull -l language {}".format(options["language"). or something like that
This should be in the buildbot job, not here. The webhook only transforms data structures and puts them into a database. - `"tx pull -l language {}".format(options["language").` or something like that

this line ('resource': ...) should be remove

this line (`'resource': ...`) should be remove
# for commit in commits: # # for commit in commits:

something like translated_request or mapped_request would be more understandable. There is not response coming from transifex.

something like `translated_request` or `mapped_request` would be more understandable. There is not response coming from transifex.
# files = [] # # files = []

not translating anything. There is nothing here that translates transifex payload information into github information.

not translating anything. There is nothing here that translates transifex payload information into github information.
# for kind in ('added', 'modified', 'removed'): # # for kind in ('added', 'modified', 'removed'):
# files.extend(commit.get(kind, []) or []) # # files.extend(commit.get(kind, []) or [])
# timestamp = dateparse(commit['timestamp']) change = {
# change = { 'author': 'buildbot-transifex,
# 'author': '{} <{}>'.format(commit['author']['name'], 'resource': transifex_response['resource'],

this is a sub-property

this is a sub-property
# commit['author']['email']), 'branch': transifex_dict['branch'],

from map

from map
# 'files': files, 'project': transifex_response['project'],
  • no need super
  • needs master
  • doesn't save transifex_dict
  • secret also missing
- no need super - needs `master` - doesn't save `transifex_dict` - `secret` also missing

from map

from map
# 'comments': commit['message'], 'event': event_type,
# 'revision': commit['id'], 'properties': {
# 'when_timestamp': timestamp, 'branch': branch,
# 'branch': branch, 'revision': revision,
# 'revlink': commit['url'], 'language': lang,
# 'repository': repo_url, 'resource': resource,
# 'project': project, 'project': project
# 'category': event_type, }

maybe add error handling, e.g.
raise ValueError("Unknown project %s from transifex".format(transifex_project)"

maybe add error handling, e.g. ```raise ValueError("Unknown project %s from transifex".format(transifex_project)"```
# 'properties': { }
# 'event': event_type, if codebase is not None:

missing functionality, the project should be extracted from the repository variable

missing functionality, the project should be extracted from the `repository` variable
# 'repository_name': repository['name'], change['codebase'] = codebase
# 'owner': repository["owner"]["username"] changes.insert(0, change)
# },
# }
# if codebase is not None:
# change['codebase'] = codebase
# changes.insert(0, change)
return changes return changes

this shouldn't be here

this shouldn't be here
def process_review_completed(self, payload, transifex_data):
pass
def _transform_variables(self, payload, transifex_dict): def _transform_variables(self, payload, transifex_dict):
transifex_variables = { project = payload.get('project', 'None')
'project': payload['project'], transform_values = {
"translated": payload['translated'], project: {
"resource": payload['resource'], "repository": "https://github.com/Bitmessage/PyBitmessage",
"event": payload['event'], "branch": "v0.6"
  • either don't verify user agent at all, or verify it inside verifyTransifexSignature
  • the verification should be called before the factory pattern
- either don't verify user agent at all, or verify it inside verifyTransifexSignature - the verification should be called before the factory pattern
"language": payload['language']
} }
}
return transifex_variables return transform_values
@defer.inlineCallbacks @defer.inlineCallbacks
def getChanges(self, request): def getChanges(self, request):
@ -187,19 +125,39 @@ class TransifexHandler(BaseHookHandler):
if self.secret is not None: if self.secret is not None:
p = Properties() p = Properties()

duplicate

duplicate
p.master = self.master p.master = self.master
rendered_secret = yield p.render(self.secret) option = self.options
self.rendered_secret = yield p.render(self.secret)

since we're raising inside the _verifyTransifexSignature, no need for if here anymore

since we're raising inside the `_verifyTransifexSignature`, no need for `if` here anymore
signature = hmac.new( signature = hmac.new(
unicode2bytes(rendered_secret), unicode2bytes(self.rendered_secret),

this is incomplete, we shouldn't proceed if the verification fails

this is incomplete, we shouldn't proceed if the verification fails
unicode2bytes(content_text.strip()), unicode2bytes(content_text.strip()),

we're not doing anything with the return value of the verification

we're not doing anything with the return value of the verification
digestmod=hashlib.sha256) digestmod=hashlib.sha256)
header_signature = bytes2unicode( header_signature = bytes2unicode(
request.getHeader(_HEADER_SIGNATURE)) request.getHeader(_HEADER_SIGNATURE))
self.verifyTransifexSignature( self.verifyTransifexSignature(
request, content, rendered_secret, request, content, self.rendered_secret,

duplicate

duplicate
signature, header_signature signature, header_signature
) )
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None") event_type = payload.get("event", "None")
language = payload.get("language", 'None')

we shouldn't be modifying request_data, that's input

we shouldn't be modifying `request_data`, that's input
project = payload.get("project", 'None')

maybe mapped_request

maybe `mapped_request`
resource = payload.get("resource", 'None')
transifex_request_data['branch'] = "v0.6"

from the map

from the map

maybe make "buildbot-transifex" a class variable or a module variable?

maybe make `"buildbot-transifex`" a class variable or a module variable?
transifex_request_data['revision'] = ""

not doing anything useful

not doing anything useful
transifex_request_data["properties"] = "langugage"

this is github syntax, needs to be translated into transifex syntax, maybe even ignored as transifex may not have this kind of variable

this is github syntax, needs to be translated into transifex syntax, maybe even ignored as transifex may not have this kind of variable
transifex_request_data["properties"] = {
   "transifex_language": language,
   "transifex_resource": resource,
   "transifex_project": project
}
``` transifex_request_data["properties"] = { "transifex_language": language, "transifex_resource": resource, "transifex_project": project } ```
transifex_request_data["properties"] = "resource"
transifex_request_data["properties"] = "project"
transifex_request_data["properties"] = {
"branch": branch,
"revision": revision
}
transifex_request_data["changes"] = {
"author": "buildbot-transifex",
"repository": project,
}
log.msg("Received event '{}' from transifex".format(event_type)) log.msg("Received event '{}' from transifex".format(event_type))
  • here you need to translate from transifex syntax using the map to github syntax
  • also add additional transifex variables as custom properties
- here you need to translate from transifex syntax using the map to github syntax - also add additional transifex variables as custom properties
codebase = "" codebase = ""