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 ab6d1083ec - Show all commits

View File

@ -1,14 +1,12 @@
"""Transifex webhook handler """ """Transifex webhook handler """
import os
import base64 import base64

isort

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

no need logging

no need `logging`

no need for logging

no need for `logging`
from buildbot.process.properties import Properties from buildbot.process.properties import Properties
from buildbot.util import bytes2unicode, unicode2bytes from buildbot.util import bytes2unicode, unicode2bytes
@ -17,37 +15,36 @@ from buildbot.www.hooks.base import BaseHookHandler
from twisted.internet import defer from twisted.internet import defer
from twisted.python import log from twisted.python import log
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' _HEADER_URL_PATH = 'X-TX-Url'
HTTP_DATE = 'date' _HTTP_DATE = 'date'
_EVENT_KEY = 'event' _EVENT_KEY = 'event'
transifex_request_data = {} author = 'buildbot-transifex'

why?

why?

_HEADER_DATE

`_HEADER_DATE`
class TransifexHandler(BaseHookHandler): class TransifexHandler(BaseHookHandler):

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.
def __init__(self, master, secret, options, transifex_dict): def __init__(self, master, secret, transifex_to_github_map, options=None):

should be inside the map

should be inside the map

we don't need any of this

we don't need any of this
if not options: if not options:

should be arguments passed to constructor

should be arguments passed to constructor
options = {} options = {}
self.secret = secret self.secret = secret
self.master = master self.master = master

should be extracted from properties or buildbot runtime variables

should be extracted from properties or buildbot runtime variables
self.options = options self.options = options
self.transifex_dict = transifex_dict self.transifex_to_github_map = transifex_to_github_map

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

`options` should probably be the last and default to `None` or something
PeterSurda marked this conversation as resolved Outdated
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})
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
return [output, [('Content-type', 'application/json')]] return [output, [('Content-type', 'application/json')]]
def verifyTransifexSignature( def _verifyTransifexSignature(
self, request, content, signature, header_signature self, request, content, signature, header_signature
): ):
http_verb = 'POST' http_verb = 'POST'
PeterSurda marked this conversation as resolved Outdated

_verifyTransifexSignature

`_verifyTransifexSignature`
http_url_path = request.getHeader(_HEADER_URL_PATH) http_url_path = request.getHeader(_HEADER_URL_PATH)
http_gmt_date = request.getHeader(HTTP_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([
  • 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_verb, http_url_path, http_gmt_date, content_md5 http_verb, http_url_path, http_gmt_date, content_md5
@ -60,39 +57,25 @@ class TransifexHandler(BaseHookHandler):
).digest() ).digest()

here also

here also
) )
if tx_signature != header_signature: if tx_signature != header_signature:
return False raise ValueError("Tx Signature mismatch")
if signature != request.getHeader(_HEADER_SIGNATURE): if signature != request.getHeader(_HEADER_SIGNATURE):
return False raise ValueError("Signature mismatch")
return True
def process_translation_completed(self, payload, transifex_dict, event_type, codebase): def process_translation_completed(self, payload, codebase):

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

maybe just `raise ValueError("Signature mismatch")`, then it will propagate up
changes = [] changes = []

return True missing

`return True` missing
transifex_response = self._transform_variables(payload, transifex_dict) translated_request = self._transform_variables(payload['project'], payload['resource'])
if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:
if 'translation_completed' in transifex_response['event']:
ts = int(time.time())
lang = transifex_response['language']
return
# if isinstance(self.options, dict):
# commits = commits[:1]
# # for commit in commits:
# # files = []
# # for kind in ('added', 'modified', 'removed'):
# # files.extend(commit.get(kind, []) or [])
change = { change = {

tx_signature isn't a function/method

`tx_signature` isn't a function/method
'author': 'buildbot-transifex, 'author': "buildbot-transifex",
PeterSurda marked this conversation as resolved Outdated

return False

`return False`
'resource': transifex_response['resource'], 'branch': translated_request["branch"],

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
'branch': transifex_dict['branch'], 'project': translated_request["project"],

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
'project': transifex_response['project'],
'event': event_type,
'properties': { 'properties': {
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
'branch': branch, "transifex_language": payload.get("language", "None"),

error handling missing here

error handling missing here
'revision': revision, "transifex_event": payload.get("event", "None"),
'language': lang, "transifex_project": payload.get("project", "None"),

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`
'resource': resource, "transifex_resource": payload.get("resource", "None"),

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

`language = payload.get('language', "None")`
'project': project "transifex_branch": "v0.6"
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
} }

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.
if codebase is not None: if codebase is not None:

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.
@ -100,18 +83,22 @@ class TransifexHandler(BaseHookHandler):
changes.insert(0, change) changes.insert(0, change)
return changes return changes
def _transform_variables(self, payload, transifex_dict): def _transform_variables(self, transifex_project, transifex_resource):

this is a sub-property

this is a sub-property
project = payload.get('project', 'None') if transifex_project is None:

from map

from map
transform_values = { raise ValueError("Unknown project %s from transifex".format(transifex_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
project: { key = "{}/{}".format(transifex_project, transifex_resource)
"repository": "https://github.com/Bitmessage/PyBitmessage", _map = self.map[key]
"branch": "v0.6" repository = _map["repository"]
project = re.sub(r'^.*/(.*?)(\.git)?$', r'\1', repository)
return{
'project': project,
'repository': repository,
'branch': _map["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)"```
} }
}
return transform_values

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

missing functionality, the project should be extracted from the `repository` variable
@defer.inlineCallbacks @defer.inlineCallbacks
def getChanges(self, request): def getChanges(self, request):
change = {}
self.secret = None self.secret = None

this shouldn't be here

this shouldn't be here
if isinstance(self.options, dict): if isinstance(self.options, dict):
self.secret = self.options.get('secret') self.secret = self.options.get('secret')
@ -126,36 +113,23 @@ class TransifexHandler(BaseHookHandler):
p = Properties() p = Properties()
p.master = self.master p.master = self.master
option = self.options option = self.options
self.rendered_secret = yield p.render(self.secret) rendered_secret = yield p.render(self.secret)
signature = hmac.new( signature = hmac.new(
unicode2bytes(self.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))

we can just use rendered_secret, not self.rendered_secret

we can just use `rendered_secret`, not `self.rendered_secret`
self.verifyTransifexSignature( self._verifyTransifexSignature(request, content, rendered_secret, signature, header_signature)
request, content, self.rendered_secret,
signature, header_signature
)
event_type = payload.get("event", "None") event_type = payload.get("event", "None")
language = payload.get("language", 'None')
project = payload.get("project", 'None')
resource = payload.get("resource", 'None')
transifex_request_data['branch'] = "v0.6" mapped_request = self._transform_variables(payload['project'], payload['resource'])

duplicate

duplicate
transifex_request_data['revision'] = ""
transifex_request_data["properties"] = "langugage"
transifex_request_data["properties"] = "resource"
transifex_request_data["properties"] = "project"
transifex_request_data["properties"] = { change["changes"] = {
"branch": branch, "author": author,

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
"revision": revision "repository": mapped_request["repository"],
} "project": mapped_request["project"],

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

this is incomplete, we shouldn't proceed if the verification fails
transifex_request_data["changes"] = { "branch": mapped_request["branch"]

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

we're not doing anything with the return value of the verification
"author": "buildbot-transifex",
"repository": project,
} }
log.msg("Received event '{}' from transifex".format(event_type)) log.msg("Received event '{}' from transifex".format(event_type))
@ -171,5 +145,4 @@ class TransifexHandler(BaseHookHandler):
return (changes, 'transifex') return (changes, 'transifex')

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?

not doing anything useful

not doing anything useful
transifex = TransifexHandler

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 = TransifexHandler(transifex_dict)