Update webhook and setup #1

Open
shekhar-cis wants to merge 5 commits from shekhar-cis/buildbot-transifex:transifex-webhook into master
2 changed files with 90 additions and 152 deletions

View File

@ -1,8 +1,13 @@
"""Transifex webhook handler """
import base64 import base64

isort

isort
import json
import re
import hmac
import hashlib import hashlib
import hmac
import json
import os
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
from buildbot.www.hooks.base import BaseHookHandler from buildbot.www.hooks.base import BaseHookHandler

we already imported base64

we already imported base64
@ -10,134 +15,95 @@ 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'
_HTTP_DATE = 'date'
_EVENT_KEY = 'event' _EVENT_KEY = 'event'
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 process_translation_completed(self, payload, event_type, codebase): 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
refname = payload["ref"] if not options:

should be arguments passed to constructor

should be arguments passed to constructor
options = {}
self.secret = secret
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.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"):
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')]]
def _verifyTransifexSignature(
self, request, content, signature, header_signature
):
http_verb = 'POST'
PeterSurda marked this conversation as resolved Outdated

_verifyTransifexSignature

`_verifyTransifexSignature`
http_url_path = request.getHeader(_HEADER_URL_PATH)
http_gmt_date = request.getHeader(_HTTP_DATE)
content_md5 = hashlib.md5(content).hexdigest()
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
])
tx_signature = base64.b64encode(
hmac.new(
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
key=self.rendered_secret,
msg=msg,
digestmod=hashlib.sha256
PeterSurda marked this conversation as resolved
Review

should use constant

should use constant
).digest()

here also

here also
)
if tx_signature != header_signature:
raise ValueError("Tx Signature mismatch")
if signature != request.getHeader(_HEADER_SIGNATURE):
raise ValueError("Signature mismatch")
return True
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
translated_request = self._transform_variables(payload['project'], payload['resource'])
# We only care about regular heads or tags ts = int(time.time())

tx_signature isn't a function/method

`tx_signature` isn't a function/method
match = re.match(r"^refs/(heads|tags)/(.+)$", refname)
if not match:
log.msg("Ignoring refname '{}': Not a branch or tag".format(refname))
return changes
branch = match.group(2)
repository = payload['repository']
repo_url = repository['ssh_url']
project = repository['full_name']
commits = payload['commits']
if isinstance(self.options, dict) and self.options.get('onlyIncludePushCommit', False):
commits = commits[:1]
for commit in commits:
files = []
for kind in ('added', 'modified', 'removed'):
files.extend(commit.get(kind, []) or [])
timestamp = dateparse(commit['timestamp'])
change = {
'author': '{} <{}>'.format(commit['author']['name'],
commit['author']['email']),
'files': files,
'comments': commit['message'],
'revision': commit['id'],
'when_timestamp': timestamp,
'branch': branch,
'revlink': commit['url'],
'repository': repo_url,
'project': project,
'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
def process_review_compoleted(self, payload, event_type, codebase):
action = payload['action']
# 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("Gitea Pull Request event '{}' ignored".format(action))
return []
pull_request = payload['pull_request']
if not pull_request['mergeable']:
log.msg("Gitea Pull Request ignored because it is not mergeable.")
return []
if pull_request['merged']:
log.msg("Gitea 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 = { change = {
PeterSurda marked this conversation as resolved Outdated

return False

`return False`
'author': '{} <{}>'.format(pull_request['user']['full_name'], 'author': author,

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
pull_request['user']['email']), 'branch': translated_request["branch"],

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
'comments': 'PR#{}: {}\n\n{}'.format( 'branch': translated_request["repository"],
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
pull_request['number'], 'project': translated_request["project"],

error handling missing here

error handling missing here
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': { 'properties': {
'event': event_type, "transifex_language": payload.get("language", "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`
'base_branch': base['ref'], "transifex_event": payload.get("event", "None"),

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

`language = payload.get('language', "None")`
'base_sha': base['sha'], "transifex_project": payload.get("project", "None"),
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
'base_repo_id': base['repo_id'], "transifex_resource": payload.get("resource", "None"),
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
'base_repository': base['repo']['clone_url'], "transifex_branch": "translate_" + payload['language'] + "_" + str(ts)

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.
'base_git_ssh_url': base['repo']['ssh_url'], }

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.
'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: if codebase is not None:
change['codebase'] = codebase change['codebase'] = codebase
return [change] changes.insert(0, change)
return changes

this is a sub-property

this is a sub-property

from map

from map
def _transform_variables(payload): def _transform_variables(self, 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
retval = { if transifex_project is None:
project: payload.get('project'), raise ValueError("Unknown project %s from transifex".format(transifex_project))
repository = [payload.get('resource')], key = transifex_project
branch = payload.get('language') _map = self.map[key]
repository = _map["repository"]
project = re.sub(r'^.*/(.*?)(\.git)?$', r'\1', repository)
return{
'project': project,

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)"```
'repository': repository,
'branch': _map["branch"],

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

missing functionality, the project should be extracted from the `repository` variable
} }
return retval
@defer.inlineCallbacks @defer.inlineCallbacks
def getChanges(self, request): def getChanges(self, request):

this shouldn't be here

this shouldn't be here
secret = None change = {}
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()
  • 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
content_text = bytes2unicode(content) content_text = bytes2unicode(content)
@ -145,36 +111,19 @@ 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))
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) option = self.options
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)

we can just use rendered_secret, not self.rendered_secret

we can just use `rendered_secret`, not `self.rendered_secret`
header_signature = bytes2unicode( header_signature = bytes2unicode(
request.getHeader(_HEADER_SIGNATURE)) request.getHeader(_HEADER_SIGNATURE))
self._verifyTransifexSignature(request, content, rendered_secret, signature, header_signature)
http_verb = 'POST' event_type = payload.get("event", "None")

duplicate

duplicate
http_url_path = request.headers('X-TX-Url')
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")
log.msg("Received event '{}' from transifex".format(event_type)) log.msg("Received event '{}' from transifex".format(event_type))
codebase = "" codebase = ""

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
@ -188,6 +137,4 @@ class TransifexHandler(BaseHookHandler):

duplicate

duplicate
return (changes, 'transifex') return (changes, 'transifex')
# Plugin name
transifex = TransifexHandler transifex = TransifexHandler

View File

@ -6,32 +6,23 @@ from setuptools import setup
with open("README.md", "r") as fh: with open("README.md", "r") as fh:
long_description = fh.read() long_description = fh.read()
VERSION = "1.7.2" VERSION = "0.1"

change version, say 0.1

change version, say 0.1
setup(name='buildbot-gitea', setup(name='buildbot-transifex',
version=VERSION, version=VERSION,
description='buildbot plugin for integration with Gitea.', description='buildbot plugin for integration with transifex.',
author='Marvin Pohl', author='',

should fill out

should fill out
Review

author and author-email should represent someone who can be in theory contacted for more information.
url is where the plugin can be downloaded from

`author` and `author-email` should represent someone who can be in theory contacted for more information. `url` is where the plugin can be downloaded from
author_email='hello@lab132.com', author_email='',
url='https://github.com/lab132/buildbot-gitea', url='',
long_description=long_description, long_description='Transifex webhook',
long_description_content_type="text/markdown", long_description_content_type="text/markdown",
packages=['buildbot_gitea'], packages=['buildbot_transifex'],
install_requires=[ install_requires=[
"buildbot>=3.0.0" "buildbot>=3.0.0"
], ],
entry_points={ entry_points={
"buildbot.webhooks": [ "buildbot.webhooks": [
"gitea = buildbot_gitea.webhook:gitea" "transifex = buildbot_transifex.webhook:transifex"
],
"buildbot.steps": [
"Gitea = buildbot_gitea.step_source:Gitea"
],
"buildbot.reporters": [
"GiteaStatusPush = buildbot_gitea.reporter:GiteaStatusPush"
],
"buildbot.util": [
"GiteaAuth = buildbot_gitea.auth:GiteaAuth"
] ]
}, },
classifiers=[ classifiers=[