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 1b29a5639c - Show all commits

View File

@ -1,8 +1,19 @@
import sys

isort

isort
import os
import base64 import base64
import time
import json import json
import re import re
import hmac import hmac

no need logging

no need `logging`

no need for logging

no need for `logging`
import pprint
import hashlib import hashlib
import requests

we already imported base64

we already imported base64
from subprocess import call
from base64 import b64encode
from buildbot.process.properties import Properties from buildbot.process.properties import Properties
from buildbot.util import bytes2unicode, unicode2bytes from buildbot.util import bytes2unicode, unicode2bytes
from buildbot.www.hooks.base import BaseHookHandler from buildbot.www.hooks.base import BaseHookHandler
@ -15,201 +26,157 @@ from dateutil.parser import parse as dateparse
_HEADER_USER_AGENT = 'User-Agent' _HEADER_USER_AGENT = 'User-Agent'

why?

why?

_HEADER_DATE

`_HEADER_DATE`
_HEADER_SIGNATURE = 'X-TX-Signature' _HEADER_SIGNATURE = 'X-TX-Signature'
_EVENT_KEY = 'event' _EVENT_KEY = 'event'

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.
transifexSecret = ""

should be inside the map

should be inside the map

we don't need any of this

we don't need any of this
transifexUsername = ""

should be arguments passed to constructor

should be arguments passed to constructor
transifexPassword = ""
transifex_dict = {}
secret = ""

should be extracted from properties or buildbot runtime variables

should be extracted from properties or buildbot runtime variables
master = ""

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

`options` should probably be the last and default to `None` or something
gitHubToken = os.environ('gitHubToken')
PeterSurda marked this conversation as resolved Outdated
class TransifexHandler(BaseHookHandler): class TransifexHandler(BaseHookHandler):
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 verifyGitHubSignature (environ, payload_body): def __init__(self, master, secret, transifex_dict):
# signature = 'sha1=' + hmac.new(gitHubSecret, payload_body, sha1).hexdigest() self.secret = secret
# try: self.master = master
# if signature != environ.get('X-TX-Signature'): self.transifex_dict = transifex_dict
# return False
# return True
# except:
# return False
# def verifyTransifexSignature (environ, payload_body):
# signature = b64encode(hmac.new(transifexSecret, payload_body, sha1).digest())
# try:
# debug(signature)
# if signature != environ.get('HTTP_X_TX_SIGNATURE'):
# return False
# return True
# except:
# return False
PeterSurda marked this conversation as resolved Outdated

_verifyTransifexSignature

`_verifyTransifexSignature`
# def returnMessage(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', 'text/plain'),
# ('Content-Length', str(len(output))) ('Content-Length', str(len(output)))
  • 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`
# ]] ]]
# def application(environ, start_response): def verifyTransifexSignature(
# status = '200 OK' self, request, content, rendered_secret, signature, header_signature
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
# output = '' ):
# lockWait() http_verb = 'POST'
# length = int(environ.get('CONTENT_LENGTH', '0')) http_url_path = request.headers('X-TX-Url')
PeterSurda marked this conversation as resolved
Review

should use constant

should use constant
# body = environ['wsgi.input'].read(length) http_gmt_date = request.headers('Date')

here also

here also
content_md5 = hashlib.md5(content).hexdigest()
msg = b'\n'.join([
http_verb, http_url_path, http_gmt_date, content_md5
])
tx_signature = base64.b64encode(
hmac.new(
key=rendered_secret,
msg=msg,
digestmod=hashlib.sha256

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

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

return True missing

`return True` missing
)
if tx_signature() != header_signature:

tx_signature isn't a function/method

`tx_signature` isn't a function/method
raise ValueError('Invalid secret')
PeterSurda marked this conversation as resolved Outdated

return False

`return False`

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 "Transifex" in environ.get("HTTP_USER_AGENT"): try:

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
# # debug(environ) if signature != os.environ.get('HTTP_X_TX_SIGNATURE'):
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
# # debug(body) return False

error handling missing here

error handling missing here
# if not verifyTransifexSignature(environ, body): return True
# debug ("Verify Transifex Signature fail, but fuck them") except:

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`
# else: return False

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

`language = payload.get('language', "None")`
# debug ("Verify Transifex Signature ok")
# # output, responseHeaders = returnMessage(False, "Checksum bad")
# # start_response(status, responseHeaders)
# # unlock()
# # return [output]
# try:
# # debug(body)
# payload = parse_qs(body)
# # debug(payload)
# if 'pybitmessage' in payload['project'] and 'pybitmessage' in payload['resource']:
# if 'translated' in payload and '100' in payload['translated']:
# ts = int(time.time())
# updateLocalTranslationDestination(ts, payload['language'][0].lower())
# downloadTranslatedLanguage(ts, payload['language'][0])
# response = commitTranslatedLanguage(ts, payload['language'][0].lower())
# if response.ok:
# output, responseHeaders = returnMessage(True, "Processed.")
# else:
# output, responseHeaders = returnMessage(False, "Error: %i." % (response.status_code))
# else:
# output, responseHeaders = returnMessage(False, "Nothing to do")
# else:
# output, responseHeaders = returnMessage(False, "Nothing to do")
# except:
# output, responseHeaders = returnMessage(True, "Not processing")
# else:
# debug("Unknown command %s" % (environ.get("HTTP_X_GITHUB_EVENT")))
# output, responseHeaders = returnMessage(True, "Unknown command, ignoring")
def __init__(self, transifex_dict=None):
super(TransifexHandler, self).__init__(*args, **kwargs)
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
def process_translation_completed(self, payload, event_type, codebase): def downloadTranslatedLanguage(self, ts, lang):
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
refname = payload["ref"] headers = {"Authorization": "Basic " + b64encode(transifexUsername + ":" + transifexPassword)}

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.
fname = "bitmessage_" + lang.lower() + ".po"

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.
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"]

this is a sub-property

this is a sub-property
handle.write(content.encode("utf-8"))

from map

from map
return response
  • 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
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']

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)"```
request = {

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

missing functionality, the project should be extracted from the `repository` variable
"title": "Translation update " + lang,
"body": "Auto-updated from transifex",
"head": "PyBitmessageTranslations:" + newbranch,
"base": branch

this shouldn't be here

this shouldn't be here
}
headers = {"Authorization": "token " + gitHubToken}
response = requests.post("https://api.github.com/repos/Bitmessage/PyBitmessage/pulls",
headers=headers, data=json.dumps(request))
return response
  • 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
def process_translation_completed(self, payload, transifex_dict, event_type, codebase):
changes = [] changes = []
transifex_response = self._transform_variables(payload, transifex_dict)
# We only care about regular heads or tags if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:
match = re.match(r"^refs/(heads|tags)/(.+)$", refname) if 'translation_completed' in transifex_response['event'] and 100 in transifex_response['translated']:
if event_type == 'translation_completed': ts = int(time.time())
pass lang = transifex_response['language']
branch = transifex_dict['branch']
self.downloadTranslatedLanguage(ts, lang.lower())
response = self.commitTranslatedLanguage(ts, lang.lower())
if response.ok:
output, responseHeaders = self.returnMessage(True, "Processed.")
else:

we can just use rendered_secret, not self.rendered_secret

we can just use `rendered_secret`, not `self.rendered_secret`
output, responseHeaders = self.returnMessage(False, "Error: %i." % (response.status_code))
else:
output, responseHeaders = self.returnMessage(False, "Nothing to do")
else:

duplicate

duplicate
output, responseHeaders = self.returnMessage(False, "Nothing to do")
if not match: # if isinstance(self.options, dict) and self.options.get('onlyIncludePushCommit', False):

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
log.msg("Ignoring refname '{}': Not a branch or tag".format(refname)) # commits = commits[:1]
return changes

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

this is incomplete, we shouldn't proceed if the verification fails
branch = match.group(2) # for commit in commits:

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

we're not doing anything with the return value of the verification
# files = []
repository = payload['repository'] # for kind in ('added', 'modified', 'removed'):
repo_url = repository['ssh_url'] # files.extend(commit.get(kind, []) or [])
project = repository['full_name'] # timestamp = dateparse(commit['timestamp'])
# change = {

duplicate

duplicate
commits = payload['commits'] # 'author': '{} <{}>'.format(commit['author']['name'],
if isinstance(self.options, dict) and self.options.get('onlyIncludePushCommit', False): # commit['author']['email']),
commits = commits[:1] # 'files': files,
# 'comments': commit['message'],
for commit in commits: # 'revision': commit['id'],

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

we shouldn't be modifying `request_data`, that's input
files = [] # 'when_timestamp': timestamp,

maybe mapped_request

maybe `mapped_request`
for kind in ('added', 'modified', 'removed'): # 'branch': branch,
files.extend(commit.get(kind, []) or []) # 'revlink': commit['url'],
timestamp = dateparse(commit['timestamp']) # 'repository': repo_url,

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?
change = { # 'project': project,

not doing anything useful

not doing anything useful
'author': '{} <{}>'.format(commit['author']['name'], # 'category': event_type,

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 } ```
commit['author']['email']), # 'properties': {
'files': files, # 'event': event_type,
'comments': commit['message'], # 'repository_name': repository['name'],
'revision': commit['id'], # 'owner': repository["owner"]["username"]
'when_timestamp': timestamp, # },
'branch': branch, # }
'revlink': commit['url'], # if codebase is not None:
'repository': repo_url, # change['codebase'] = codebase
'project': project, # changes.insert(0, change)
'category': event_type,
'properties': {
'event': event_type,
'repository_name': repository['name'],
'owner': repository["owner"]["username"]
},
}
if codebase is not None:
change['codebase'] = codebase
changes.insert(0, change)
return changes return changes
def process_review_completed(self, payload, event_type, codebase): def process_review_completed(self, payload, transifex_data):
action = payload['action'] pass
if event_type == 'review_completed':
pass
# Only handle potential new stuff, ignore close/.
# Merge itself is handled by the regular branch push message
if action not in ['opened', 'synchronized', 'edited', 'reopened']:
log.msg("Transifex Pull Request event '{}' ignored".format(action))
return []
# pull_request = payload['pull_request']
# if not pull_request['mergeable']:
# log.msg("Transifex Pull Request ignored because it is not mergeable.")
# return []
# if pull_request['merged']:
# log.msg("Transifex Pull Request ignored because it is already merged.")
# return []
# timestamp = dateparse(pull_request['updated_at'])
# base = pull_request['base']
# head = pull_request['head']
repository = payload['repository']
change = {
'author': '{} <{}>'.format(pull_request['user']['full_name'],
pull_request['user']['email']),
# 'comments': 'PR#{}: {}\n\n{}'.format(
# pull_request['number'],
# pull_request['title'],
# pull_request['body']),
'revision': base['sha'],
'when_timestamp': timestamp,
'branch': base['ref'],
# 'revlink': pull_request['html_url'],
'repository': base['repo']['ssh_url'],
'project': repository['full_name'],
'category': event_type,
'properties': {
'event': event_type,
'base_branch': base['ref'],
'base_sha': base['sha'],
'base_repo_id': base['repo_id'],
'base_repository': base['repo']['clone_url'],
'base_git_ssh_url': base['repo']['ssh_url'],
'head_branch': head['ref'],
'head_sha': head['sha'],
'head_repo_id': head['repo_id'],
'head_repository': head['repo']['clone_url'],
'head_git_ssh_url': head['repo']['ssh_url'],
'head_owner': head['repo']['owner']['username'],
'head_reponame': head['repo']['name'],
# 'pr_id': pull_request['id'],
# 'pr_number': pull_request['number'],
'repository_name': repository['name'],
'owner': repository["owner"]["username"],
},
}
if codebase is not None:
change['codebase'] = codebase
return [change]
  • 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
def _transform_variables(payload):
retval = { def _transform_variables(self, payload, transifex_dict):
project: payload.get('project'), transifex_variables = {
  • we need to translate from transifex structure to github/gitea structure
  • the key should be constructed from transifex data, and the value should be a dictionary of key/values referencing a github/gitea project

Probably something like:

key = payload.get("project", "")
value = {
    "repository": "https://github.com/Bitmessage/PyBitmessage",
    "branch": "v0.6"
}
- we need to translate from transifex structure to github/gitea structure - the key should be constructed from transifex data, and the value should be a dictionary of key/values referencing a github/gitea project Probably something like: ``` key = payload.get("project", "") value = { "repository": "https://github.com/Bitmessage/PyBitmessage", "branch": "v0.6" } ```
repository = [payload.get('resource')], 'project': payload['project'],
branch = payload.get('language') "translated": payload['translated'],
"resource": payload['resource'],
"event": payload['event'],
"language": payload['language']
} }
return retval
return transifex_variables
@defer.inlineCallbacks @defer.inlineCallbacks
def getChanges(self, request): def getChanges(self, request):
secret = None self.secret = None
if isinstance(self.options, dict): if isinstance(self.options, dict):
secret = self.options.get('secret') self.secret = self.options.get('secret')
try: try:
content = request.content.read() content = request.content.read()
content_text = bytes2unicode(content) content_text = bytes2unicode(content)
@ -217,34 +184,20 @@ class TransifexHandler(BaseHookHandler):
except Exception as exception: except Exception as exception:
raise ValueError('Error loading JSON: ' + str(exception)) raise ValueError('Error loading JSON: ' + str(exception))
  • add a section to verify all mandatory options are present (e.g. "language", "project", "resource")
- add a section to verify all mandatory options are present (e.g. "language", "project", "resource")
if self.secret is not None:
if secret is not None:
p = Properties() p = Properties()
p.master = self.master p.master = self.master
rendered_secret = yield p.render(secret) rendered_secret = yield p.render(self.secret)
signature = hmac.new( signature = hmac.new(
unicode2bytes(rendered_secret), unicode2bytes(rendered_secret),
unicode2bytes(content_text.strip()), unicode2bytes(content_text.strip()),
digestmod=hashlib.sha256) digestmod=hashlib.sha256)
header_signature = bytes2unicode( header_signature = bytes2unicode(
request.getHeader(_HEADER_SIGNATURE)) request.getHeader(_HEADER_SIGNATURE))
self.verifyTransifexSignature(
http_verb = 'POST' request, content, rendered_secret,
http_url_path = request.headers('X-TX-Url') signature, header_signature
http_gmt_date = request.headers('Date')
content_md5 = hashlib.md5(content).hexdigest()
msg = b'\n'.join([
http_verb, http_url_path, http_gmt_date, content_md5
])
tx_signature = base64.b64encode(
hmac.new(
key=rendered_secret,
msg=msg,
digestmod=hashlib.sha256
).digest()
) )
if tx_signature() != header_signature:
raise ValueError('Invalid secret')
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None") event_type = bytes2unicode(payload.get(_EVENT_KEY), "None")
  • maybe we don't need bytes2unicode? (leftover from gitea hook)
  • we probably need `event_type = payload.get("event", "none")
- ~~maybe we don't need `bytes2unicode`?~~ (leftover from gitea hook) - we probably need `event_type = payload.get("event", "none")
log.msg("Received event '{}' from transifex".format(event_type)) log.msg("Received event '{}' from transifex".format(event_type))
- check out https://git.bitmessage.org/Bitmessage/buildbot_multibuild/src/branch/master/lib/worker_multibuild.py to see the mandatory components - `project` can be extracted from `repository` (is a substring) - `revision` let's ignore it for now - `branch` is in the map, just like `repository`