Update webhook and setup #1
|
@ -1,8 +1,13 @@
|
|||
"""Transifex webhook handler """
|
||||
import base64
|
||||
|
||||
import json
|
||||
import re
|
||||
import hmac
|
||||
import hashlib
|
||||
import hmac
|
||||
import json
|
||||
import os
|
||||
import requests
|
||||
import re
|
||||
import time
|
||||
|
||||
PeterSurda
commented
no need no need `logging`
PeterSurda
commented
no need for no need for `logging`
|
||||
from buildbot.process.properties import Properties
|
||||
from buildbot.util import bytes2unicode, unicode2bytes
|
||||
from buildbot.www.hooks.base import BaseHookHandler
|
||||
PeterSurda
commented
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.python import log
|
||||
|
||||
from dateutil.parser import parse as dateparse
|
||||
|
||||
_HEADER_USER_AGENT = 'User-Agent'
|
||||
_HEADER_SIGNATURE = 'X-TX-Signature'
|
||||
_HEADER_URL_PATH = 'X-TX-Url'
|
||||
_HTTP_DATE = 'date'
|
||||
_EVENT_KEY = 'event'
|
||||
author = 'buildbot-transifex'
|
||||
|
||||
|
||||
PeterSurda
commented
why? why?
PeterSurda
commented
`_HEADER_DATE`
|
||||
class TransifexHandler(BaseHookHandler):
|
||||
|
||||
PeterSurda
commented
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):
|
||||
refname = payload["ref"]
|
||||
def __init__(self, master, secret, transifex_to_github_map, options=None):
|
||||
PeterSurda
commented
should be inside the map should be inside the map
PeterSurda
commented
we don't need any of this we don't need any of this
|
||||
if not options:
|
||||
PeterSurda
commented
should be arguments passed to constructor should be arguments passed to constructor
|
||||
options = {}
|
||||
self.secret = secret
|
||||
self.master = master
|
||||
PeterSurda
commented
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
|
||||
PeterSurda
commented
`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
PeterSurda
commented
something like 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
PeterSurda
commented
`_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([
|
||||
PeterSurda
commented
- 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
PeterSurda
commented
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
|
||||
).digest()
|
||||
)
|
||||
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):
|
||||
PeterSurda
commented
maybe just maybe just `raise ValueError("Signature mismatch")`, then it will propagate up
|
||||
changes = []
|
||||
PeterSurda
commented
`return True` missing
|
||||
|
||||
# We only care about regular heads or tags
|
||||
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'])
|
||||
translated_request = self._transform_variables(payload['project'], payload['resource'])
|
||||
ts = int(time.time())
|
||||
PeterSurda
commented
`tx_signature` isn't a function/method
|
||||
change = {
|
||||
PeterSurda marked this conversation as resolved
Outdated
|
||||
'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,
|
||||
'author': author,
|
||||
PeterSurda
commented
don't have weird constants here, rather process is through the map. don't have weird constants here, rather process is through the map.
PeterSurda
commented
`repository` missing
|
||||
'branch': translated_request["branch"],
|
||||
PeterSurda
commented
`transifex_to_github_map` is an object attribute, no need to pass it around
|
||||
'branch': translated_request["repository"],
|
||||
PeterSurda marked this conversation as resolved
Outdated
PeterSurda
commented
- use constant
- use a default value, then no need for try/except
|
||||
'project': translated_request["project"],
|
||||
PeterSurda
commented
error handling missing here error handling missing here
|
||||
'properties': {
|
||||
'event': event_type,
|
||||
'repository_name': repository['name'],
|
||||
'owner': repository["owner"]["username"]
|
||||
},
|
||||
"transifex_language": payload.get("language", "None"),
|
||||
PeterSurda
commented
if you use if you use `.get` with a default value, you can avoid `try`/`except`
|
||||
"transifex_event": payload.get("event", "None"),
|
||||
PeterSurda
commented
`language = payload.get('language', "None")`
|
||||
"transifex_project": payload.get("project", "None"),
|
||||
PeterSurda
commented
this should be the randomly generated one, the branch of the PR this should be the randomly generated one, the branch of the PR
|
||||
"transifex_resource": payload.get("resource", "None"),
|
||||
PeterSurda marked this conversation as resolved
Outdated
PeterSurda
commented
This should be in the buildbot job, not here. The webhook only transforms data structures and puts them into a database.
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
PeterSurda
commented
this line ( this line (`'resource': ...`) should be remove
|
||||
"transifex_branch": "translate_" + payload['language'] + "_" + str(ts)
|
||||
PeterSurda
commented
something like something like `translated_request` or `mapped_request` would be more understandable. There is not response coming from transifex.
|
||||
}
|
||||
PeterSurda
commented
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.
|
||||
}
|
||||
if codebase is not None:
|
||||
change['codebase'] = codebase
|
||||
changes.insert(0, change)
|
||||
return changes
|
||||
PeterSurda
commented
this is a sub-property this is a sub-property
|
||||
|
||||
PeterSurda
commented
from map from map
|
||||
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 = {
|
||||
'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"],
|
||||
},
|
||||
def _transform_variables(self, transifex_project):
|
||||
PeterSurda
commented
- no need super
- needs `master`
- doesn't save `transifex_dict`
- `secret` also missing
PeterSurda
commented
from map from map
|
||||
if transifex_project is None:
|
||||
raise ValueError("Unknown project %s from transifex".format(transifex_project))
|
||||
key = transifex_project
|
||||
_map = self.map[key]
|
||||
repository = _map["repository"]
|
||||
project = re.sub(r'^.*/(.*?)(\.git)?$', r'\1', repository)
|
||||
return{
|
||||
'project': project,
|
||||
PeterSurda
commented
maybe add error handling, e.g. maybe add error handling, e.g.
```raise ValueError("Unknown project %s from transifex".format(transifex_project)"```
|
||||
'repository': repository,
|
||||
'branch': _map["branch"],
|
||||
PeterSurda
commented
missing functionality, the project should be extracted from the missing functionality, the project should be extracted from the `repository` variable
|
||||
}
|
||||
if codebase is not None:
|
||||
change['codebase'] = codebase
|
||||
return [change]
|
||||
|
||||
def _transform_variables(payload):
|
||||
retval = {
|
||||
project: payload.get('project'),
|
||||
repository = [payload.get('resource')],
|
||||
branch = payload.get('language')
|
||||
}
|
||||
return retval
|
||||
|
||||
@defer.inlineCallbacks
|
||||
def getChanges(self, request):
|
||||
PeterSurda
commented
this shouldn't be here this shouldn't be here
|
||||
secret = None
|
||||
change = {}
|
||||
self.secret = None
|
||||
if isinstance(self.options, dict):
|
||||
secret = self.options.get('secret')
|
||||
self.secret = self.options.get('secret')
|
||||
try:
|
||||
content = request.content.read()
|
||||
PeterSurda
commented
- 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)
|
||||
|
@ -145,36 +111,19 @@ class TransifexHandler(BaseHookHandler):
|
|||
except Exception as exception:
|
||||
raise ValueError('Error loading JSON: ' + str(exception))
|
||||
|
||||
|
||||
if secret is not None:
|
||||
if self.secret is not None:
|
||||
p = Properties()
|
||||
p.master = self.master
|
||||
rendered_secret = yield p.render(secret)
|
||||
option = self.options
|
||||
rendered_secret = yield p.render(self.secret)
|
||||
signature = hmac.new(
|
||||
unicode2bytes(rendered_secret),
|
||||
unicode2bytes(content_text.strip()),
|
||||
digestmod=hashlib.sha256)
|
||||
PeterSurda
commented
we can just use we can just use `rendered_secret`, not `self.rendered_secret`
|
||||
header_signature = bytes2unicode(
|
||||
request.getHeader(_HEADER_SIGNATURE))
|
||||
|
||||
http_verb = 'POST'
|
||||
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")
|
||||
self._verifyTransifexSignature(request, content, rendered_secret, signature, header_signature)
|
||||
event_type = payload.get("event", "None")
|
||||
PeterSurda
commented
duplicate duplicate
|
||||
log.msg("Received event '{}' from transifex".format(event_type))
|
||||
|
||||
codebase = ""
|
||||
PeterSurda
commented
since we're raising inside the since we're raising inside the `_verifyTransifexSignature`, no need for `if` here anymore
|
||||
|
@ -188,6 +137,4 @@ class TransifexHandler(BaseHookHandler):
|
|||
|
||||
PeterSurda
commented
duplicate duplicate
|
||||
return (changes, 'transifex')
|
||||
|
||||
|
||||
# Plugin name
|
||||
transifex = TransifexHandler
|
||||
|
|
27
setup.py
|
@ -6,32 +6,23 @@ from setuptools import setup
|
|||
with open("README.md", "r") as fh:
|
||||
long_description = fh.read()
|
||||
|
||||
VERSION = "1.7.2"
|
||||
VERSION = "0.1"
|
||||
PeterSurda
commented
change version, say 0.1 change version, say 0.1
|
||||
|
||||
setup(name='buildbot-gitea',
|
||||
setup(name='buildbot-transifex',
|
||||
version=VERSION,
|
||||
description='buildbot plugin for integration with Gitea.',
|
||||
author='Marvin Pohl',
|
||||
author_email='hello@lab132.com',
|
||||
url='https://github.com/lab132/buildbot-gitea',
|
||||
long_description=long_description,
|
||||
description='buildbot plugin for integration with transifex.',
|
||||
author='',
|
||||
PeterSurda
commented
should fill out should fill out
PeterSurda
commented
`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='',
|
||||
url='',
|
||||
long_description='Transifex webhook',
|
||||
long_description_content_type="text/markdown",
|
||||
packages=['buildbot_gitea'],
|
||||
packages=['buildbot_transifex'],
|
||||
install_requires=[
|
||||
"buildbot>=3.0.0"
|
||||
],
|
||||
entry_points={
|
||||
"buildbot.webhooks": [
|
||||
"gitea = buildbot_gitea.webhook:gitea"
|
||||
],
|
||||
"buildbot.steps": [
|
||||
"Gitea = buildbot_gitea.step_source:Gitea"
|
||||
],
|
||||
"buildbot.reporters": [
|
||||
"GiteaStatusPush = buildbot_gitea.reporter:GiteaStatusPush"
|
||||
],
|
||||
"buildbot.util": [
|
||||
"GiteaAuth = buildbot_gitea.auth:GiteaAuth"
|
||||
"transifex = buildbot_transifex.webhook:transifex"
|
||||
]
|
||||
},
|
||||
classifiers=[
|
||||
|
|
isort