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 104 additions and 42 deletions
Showing only changes of commit f2587e071d - Show all commits

View File

@ -16,16 +16,87 @@ _HEADER_USER_AGENT = 'User-Agent'
_HEADER_SIGNATURE = 'X-TX-Signature' _HEADER_SIGNATURE = 'X-TX-Signature'
_EVENT_KEY = 'event' _EVENT_KEY = 'event'
class TransifexHandler(BaseHookHandler): class TransifexHandler(BaseHookHandler):
# def verifyGitHubSignature (environ, payload_body):
# signature = 'sha1=' + hmac.new(gitHubSecret, payload_body, sha1).hexdigest()
# try:
# if signature != environ.get('X-TX-Signature'):
# return False
# return True

why?

why?

_HEADER_DATE

`_HEADER_DATE`
# except:
# return False

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.

should be inside the map

should be inside the map

we don't need any of this

we don't need any of this
# def verifyTransifexSignature (environ, payload_body):

should be arguments passed to constructor

should be arguments passed to constructor
# signature = b64encode(hmac.new(transifexSecret, payload_body, sha1).digest())
# try:
# debug(signature)

should be extracted from properties or buildbot runtime variables

should be extracted from properties or buildbot runtime variables
# if signature != environ.get('HTTP_X_TX_SIGNATURE'):
# return False

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

`options` should probably be the last and default to `None` or something
# return True
PeterSurda marked this conversation as resolved Outdated
# except:
# return False
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(status = False, message = "Unimplemented"):
# output = json.dumps({"status": "OK" if status else "FAIL", "message": message})
# return [output, [('Content-type', 'text/plain'),
# ('Content-Length', str(len(output)))
# ]]
PeterSurda marked this conversation as resolved Outdated

_verifyTransifexSignature

`_verifyTransifexSignature`
# def application(environ, start_response):
# status = '200 OK'
# output = ''
# lockWait()
  • 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`
# length = int(environ.get('CONTENT_LENGTH', '0'))
# body = environ['wsgi.input'].read(length)
# if "Transifex" in environ.get("HTTP_USER_AGENT"):
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
# # debug(environ)
# # debug(body)
# if not verifyTransifexSignature(environ, body):
PeterSurda marked this conversation as resolved
Review

should use constant

should use constant
# debug ("Verify Transifex Signature fail, but fuck them")

here also

here also
# else:
# 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)

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

maybe just `raise ValueError("Signature mismatch")`, then it will propagate up
# # debug(payload)

return True missing

`return True` missing
# if 'pybitmessage' in payload['project'] and 'pybitmessage' in payload['resource']:
# if 'translated' in payload and '100' in payload['translated']:

tx_signature isn't a function/method

`tx_signature` isn't a function/method
# ts = int(time.time())
PeterSurda marked this conversation as resolved Outdated

return False

`return False`
# updateLocalTranslationDestination(ts, payload['language'][0].lower())

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
# downloadTranslatedLanguage(ts, payload['language'][0])

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
# response = commitTranslatedLanguage(ts, payload['language'][0].lower())
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
# if response.ok:

error handling missing here

error handling missing here
# output, responseHeaders = returnMessage(True, "Processed.")
# else:

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`
# output, responseHeaders = returnMessage(False, "Error: %i." % (response.status_code))

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

`language = payload.get('language', "None")`
# else:
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
# output, responseHeaders = returnMessage(False, "Nothing to do")
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
# else:

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.
# output, responseHeaders = returnMessage(False, "Nothing to do")

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.
# 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")

this is a sub-property

this is a sub-property
def __init__(self, transifex_dict=None):

from map

from map
super(TransifexHandler, self).__init__(*args, **kwargs)
  • 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 process_translation_completed(self, payload, event_type, codebase): def process_translation_completed(self, payload, event_type, codebase):
refname = payload["ref"] refname = payload["ref"]
changes = [] changes = []
# We only care about regular heads or tags # We only care about regular heads or tags
match = re.match(r"^refs/(heads|tags)/(.+)$", refname) match = re.match(r"^refs/(heads|tags)/(.+)$", refname)

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)"```
if event_type == 'translation_completed':
pass

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

missing functionality, the project should be extracted from the `repository` variable
if not match: if not match:
log.msg("Ignoring refname '{}': Not a branch or tag".format(refname)) log.msg("Ignoring refname '{}': Not a branch or tag".format(refname))
return changes return changes

this shouldn't be here

this shouldn't be here
@ -68,36 +139,37 @@ class TransifexHandler(BaseHookHandler):
changes.insert(0, change) changes.insert(0, change)
return changes return changes
def process_review_compoleted(self, payload, event_type, codebase): def process_review_completed(self, payload, event_type, codebase):

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

we shouldn't be modifying `request_data`, that's input
action = payload['action'] action = payload['action']

maybe mapped_request

maybe `mapped_request`
if event_type == 'review_completed':
pass
# Only handle potential new stuff, ignore close/. # Only handle potential new stuff, ignore close/.

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?
# Merge itself is handled by the regular branch push message # Merge itself is handled by the regular branch push message

not doing anything useful

not doing anything useful
if action not in ['opened', 'synchronized', 'edited', 'reopened']: if action not in ['opened', 'synchronized', 'edited', 'reopened']:

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 } ```
log.msg("Gitea Pull Request event '{}' ignored".format(action)) log.msg("Transifex Pull Request event '{}' ignored".format(action))
return [] return []
pull_request = payload['pull_request'] # pull_request = payload['pull_request']
if not pull_request['mergeable']: # if not pull_request['mergeable']:
log.msg("Gitea Pull Request ignored because it is not mergeable.") # log.msg("Transifex Pull Request ignored because it is not mergeable.")
return [] # return []
if pull_request['merged']: # if pull_request['merged']:
log.msg("Gitea Pull Request ignored because it is already merged.") # log.msg("Transifex Pull Request ignored because it is already merged.")
return [] # return []
timestamp = dateparse(pull_request['updated_at']) # timestamp = dateparse(pull_request['updated_at'])
base = pull_request['base'] # base = pull_request['base']
head = pull_request['head'] # head = pull_request['head']
repository = payload['repository'] repository = payload['repository']
change = { 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
'author': '{} <{}>'.format(pull_request['user']['full_name'], 'author': '{} <{}>'.format(pull_request['user']['full_name'],
pull_request['user']['email']), pull_request['user']['email']),
'comments': 'PR#{}: {}\n\n{}'.format( # 'comments': 'PR#{}: {}\n\n{}'.format(
  • 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" } ```
pull_request['number'], # pull_request['number'],
pull_request['title'], # pull_request['title'],
pull_request['body']), # pull_request['body']),
'revision': base['sha'], 'revision': base['sha'],
'when_timestamp': timestamp, 'when_timestamp': timestamp,
'branch': base['ref'], 'branch': base['ref'],
'revlink': pull_request['html_url'], # 'revlink': pull_request['html_url'],
'repository': base['repo']['ssh_url'], 'repository': base['repo']['ssh_url'],
'project': repository['full_name'], 'project': repository['full_name'],
'category': event_type, 'category': event_type,
@ -115,8 +187,8 @@ class TransifexHandler(BaseHookHandler):
'head_git_ssh_url': head['repo']['ssh_url'], 'head_git_ssh_url': head['repo']['ssh_url'],
'head_owner': head['repo']['owner']['username'], 'head_owner': head['repo']['owner']['username'],
'head_reponame': head['repo']['name'], 'head_reponame': head['repo']['name'],
'pr_id': pull_request['id'], # 'pr_id': pull_request['id'],
'pr_number': pull_request['number'], # 'pr_number': pull_request['number'],
'repository_name': repository['name'], 'repository_name': repository['name'],
'owner': repository["owner"]["username"], 'owner': repository["owner"]["username"],
}, },
@ -189,5 +261,4 @@ class TransifexHandler(BaseHookHandler):
return (changes, 'transifex') return (changes, 'transifex')
# Plugin name transifex = TransifexHandler(transifex_dict)
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=[