Update webhook and setup #1

Open
shekhar-cis wants to merge 5 commits from shekhar-cis/buildbot-transifex:transifex-webhook into master
First-time contributor

Remove gitea and add transifex, update setup and webhook.py

Remove gitea and add transifex, update setup and webhook.py
shekhar-cis added 1 commit 2023-01-02 15:02:46 +00:00
shekhar-cis force-pushed transifex-webhook from 4f412af6d9 to f2587e071d 2023-01-04 06:39:20 +00:00 Compare
PeterSurda requested changes 2023-01-04 07:12:41 +00:00
@ -22,0 +85,4 @@
# 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)
Owner
  • 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
@ -74,3 +145,4 @@
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']:
Owner

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
@ -89,2 +159,4 @@
# base = pull_request['base']
# head = pull_request['head']
repository = payload['repository']
change = {
Owner
  • 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
setup.py Outdated
@ -8,30 +8,21 @@ with open("README.md", "r") as fh:
VERSION = "1.7.2"
Owner

change version, say 0.1

change version, say 0.1
setup.py Outdated
@ -16,2 +13,2 @@
url='https://github.com/lab132/buildbot-gitea',
long_description=long_description,
description='buildbot plugin for integration with transifex.',
author='',
Owner

should fill out

should fill out
shekhar-cis added 1 commit 2023-01-04 15:21:22 +00:00
shekhar-cis force-pushed transifex-webhook from f0f715b342 to 4b38bcc252 2023-01-05 06:26:25 +00:00 Compare
PeterSurda requested changes 2023-01-05 06:40:50 +00:00
@ -13,3 +24,3 @@
from dateutil.parser import parse as dateparse
_HEADER_USER_AGENT = 'User-Agent'
HTTP_USER_AGENT = 'User-Agent'
Owner

why?

why?
@ -16,3 +27,3 @@
_HEADER_SIGNATURE = 'X-TX-Signature'
_EVENT_KEY = 'event'
branch = 'v0.6'
Owner

should be inside the map

should be inside the map
@ -17,2 +28,3 @@
_EVENT_KEY = 'event'
branch = 'v0.6'
transifexSecret = ""
Owner

should be arguments passed to constructor

should be arguments passed to constructor
@ -19,0 +30,4 @@
transifexSecret = ""
transifexUsername = ""
transifexPassword = ""
transifex_webhook_url = 'https://buildbot.bitmessage.org/change_hook/transifex'
Owner

should be extracted from properties or buildbot runtime variables

should be extracted from properties or buildbot runtime variables
@ -30,2 +107,2 @@
log.msg("Ignoring refname '{}': Not a branch or tag".format(refname))
return changes
# match = re.match(r"^refs/(heads|tags)/(.+)$", refname)
if "Transifex" in os.environ.get("HTTP_USER_AGENT"):
Owner
  • 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
shekhar-cis force-pushed transifex-webhook from 4b38bcc252 to 1e92388b78 2023-01-05 15:17:45 +00:00 Compare
shekhar-cis force-pushed transifex-webhook from 1e92388b78 to 1b29a5639c 2023-01-05 16:55:13 +00:00 Compare
PeterSurda requested changes 2023-01-06 07:06:26 +00:00
@ -1,8 +1,19 @@
import sys
Owner

isort

isort
@ -15,129 +26,157 @@ from dateutil.parser import parse as dateparse
_HEADER_USER_AGENT = 'User-Agent'
_HEADER_SIGNATURE = 'X-TX-Signature'
_EVENT_KEY = 'event'
transifexSecret = ""
Owner

we don't need any of this

we don't need any of this
@ -18,1 +33,4 @@
secret = ""
master = ""
gitHubToken = os.environ('gitHubToken')
Owner

no

no
PeterSurda marked this conversation as resolved
@ -24,0 +46,4 @@
def returnMessage(self, 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)))
Owner
  • 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`
@ -24,0 +50,4 @@
]]
def verifyTransifexSignature(
self, request, content, rendered_secret, signature, header_signature
Owner

rendered_secret can be extracted directly, no need to pass it

rendered_secret can be extracted directly, no need to pass it
PeterSurda marked this conversation as resolved
@ -24,0 +53,4 @@
self, request, content, rendered_secret, signature, header_signature
):
http_verb = 'POST'
http_url_path = request.headers('X-TX-Url')
Owner

should use constant

should use constant
PeterSurda marked this conversation as resolved
@ -24,0 +54,4 @@
):
http_verb = 'POST'
http_url_path = request.headers('X-TX-Url')
http_gmt_date = request.headers('Date')
Owner

here also

here also
@ -24,0 +66,4 @@
digestmod=hashlib.sha256
).digest()
)
if tx_signature() != header_signature:
Owner

tx_signature isn't a function/method

`tx_signature` isn't a function/method
@ -24,0 +67,4 @@
).digest()
)
if tx_signature() != header_signature:
raise ValueError('Invalid secret')
Owner

return False

`return False`
PeterSurda marked this conversation as resolved
@ -24,0 +70,4 @@
raise ValueError('Invalid secret')
try:
if signature != os.environ.get('HTTP_X_TX_SIGNATURE'):
Owner
  • use constant
  • use a default value, then no need for try/except
- use constant - use a default value, then no need for try/except
PeterSurda marked this conversation as resolved
@ -24,0 +76,4 @@
except:
return False
def downloadTranslatedLanguage(self, ts, lang):
Owner

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
PeterSurda marked this conversation as resolved
@ -133,0 +162,4 @@
def _transform_variables(self, payload, transifex_dict):
transifex_variables = {
Owner
  • 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" } ```
@ -145,34 +184,20 @@ class TransifexHandler(BaseHookHandler):
except Exception as exception:
raise ValueError('Error loading JSON: ' + str(exception))
Owner
  • 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")
@ -174,4 +201,2 @@
if tx_signature() != header_signature:
raise ValueError('Invalid secret')
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None")
Owner
  • 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")
@ -175,4 +201,3 @@
raise ValueError('Invalid secret')
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None")
log.msg("Received event '{}' from transifex".format(event_type))
Owner
- 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`
shekhar-cis added 1 commit 2023-01-06 15:09:12 +00:00
shekhar-cis force-pushed transifex-webhook from 43d59d537b to 89305ec602 2023-01-06 15:19:46 +00:00 Compare
PeterSurda requested changes 2023-01-09 06:56:58 +00:00
@ -25,0 +42,4 @@
output = json.dumps({"status": "OK" if status else "FAIL", "message": message})
return [output, [('Content-type', 'application/json')]]
def verifyTransifexSignature(
Owner

_verifyTransifexSignature

`_verifyTransifexSignature`
PeterSurda marked this conversation as resolved
@ -25,0 +64,4 @@
if signature != request.getHeader(_HEADER_SIGNATURE):
return False
Owner

return True missing

`return True` missing
@ -88,2 +70,2 @@
head = pull_request['head']
repository = payload['repository']
transifex_response = self._transform_variables(payload, transifex_dict)
if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:
Owner

don't have weird constants here, rather process is through the map.

don't have weird constants here, rather process is through the map.
@ -90,0 +71,4 @@
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']
Owner

error handling missing here

error handling missing here
@ -102,2 +85,2 @@
'project': repository['full_name'],
'category': event_type,
'author': 'buildbot-transifex,
'resource': transifex_response['resource'],
Owner

this is a sub-property

this is a sub-property
@ -103,1 +85,3 @@
'category': event_type,
'author': 'buildbot-transifex,
'resource': transifex_response['resource'],
'branch': transifex_dict['branch'],
Owner

from map

from map
@ -104,0 +85,4 @@
'author': 'buildbot-transifex,
'resource': transifex_response['resource'],
'branch': transifex_dict['branch'],
'project': transifex_response['project'],
Owner

from map

from map
@ -178,0 +143,4 @@
project = payload.get("project", 'None')
resource = payload.get("resource", 'None')
transifex_request_data['branch'] = "v0.6"
Owner

from the map

from the map
@ -178,0 +145,4 @@
transifex_request_data['branch'] = "v0.6"
transifex_request_data['revision'] = ""
transifex_request_data["properties"] = "langugage"
Owner
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 } ```
@ -189,5 +214,4 @@ class TransifexHandler(BaseHookHandler):
return (changes, 'transifex')
Owner

Additional properties needed:

  • language
  • resource
  • transifex_branch (for PR)
Additional properties needed: - `language` - `resource` - `transifex_branch` (for PR)
@ -16,2 +13,2 @@
url='https://github.com/lab132/buildbot-gitea',
long_description=long_description,
description='buildbot plugin for integration with transifex.',
author='',
Owner

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
shekhar-cis added 1 commit 2023-01-09 16:27:49 +00:00
PeterSurda requested changes 2023-01-10 07:10:41 +00:00
@ -15,2 +23,4 @@
_HEADER_USER_AGENT = 'User-Agent'
_HEADER_SIGNATURE = 'X-TX-Signature'
_HEADER_URL_PATH = 'X-TX-Url'
HTTP_DATE = 'date'
Owner

_HEADER_DATE

`_HEADER_DATE`
@ -17,1 +25,4 @@
_HEADER_URL_PATH = 'X-TX-Url'
HTTP_DATE = 'date'
_EVENT_KEY = 'event'
transifex_request_data = {}
Owner

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.
@ -25,0 +36,4 @@
self.secret = secret
self.master = master
self.options = options
self.transifex_dict = transifex_dict
Owner

something like transifex_to_github_map is a better name

something like `transifex_to_github_map` is a better name
PeterSurda marked this conversation as resolved
@ -26,0 +73,4 @@
# if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:
# if 'translation_completed' in transifex_response['event']:
try:
lang = transifex_response['language']
Owner

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`
@ -133,0 +99,4 @@
def _transform_variables(self, payload, transifex_dict):
project = payload.get('project', 'None')
transform_values = {
project: {
Owner

this shouldn't be here

this shouldn't be here
@ -170,3 +132,1 @@
msg=msg,
digestmod=hashlib.sha256
).digest()
self._verifyTransifexSignature(
Owner

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

we're not doing anything with the return value of the verification
@ -178,0 +139,4 @@
project = payload.get("project", 'None')
resource = payload.get("resource", 'None')
transifex_request_data['branch'] = transifex_dict['branch']
Owner

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

we shouldn't be modifying `request_data`, that's input
shekhar-cis force-pushed transifex-webhook from dbf00e0ed5 to 1a54f3b81d 2023-01-10 12:16:35 +00:00 Compare
PeterSurda requested changes 2023-01-11 06:44:21 +00:00
@ -23,2 +33,3 @@
refname = payload["ref"]
class TransifexHandler(BaseHookHandler):
def __init__(self, master, secret, options, transifex_to_github_map):
Owner

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

`options` should probably be the last and default to `None` or something
@ -25,0 +69,4 @@
return False
return True
def process_translation_completed(self, payload, transifex_to_github_map, event_type, codebase):
Owner

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
@ -26,0 +74,4 @@
transifex_response = self._transform_variables(payload, transifex_to_github_map)
# if 'pybitmessage-test' in transifex_response['project'] and 'messagespot' in transifex_response['resource']:
# if 'translation_completed' in transifex_response['event']:
language = transifex_response.get['language']
Owner

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

`language = payload.get('language', "None")`
@ -102,2 +80,2 @@
'project': repository['full_name'],
'category': event_type,
'author': "buildbot-transifex",
'resource': transifex_to_github_map["resource"],
Owner

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.
@ -175,3 +131,1 @@
raise ValueError('Invalid secret')
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None")
if not self._verifyTransifexSignature(request, content, self.rendered_secret, signature, header_signature):
Owner

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

this is incomplete, we shouldn't proceed if the verification fails
@ -178,0 +144,4 @@
change["changes"] = {
"author": "buildbot-transifex",
"repository": transifex_to_github_map['repository'],
Owner

not doing anything useful

not doing anything useful
shekhar-cis force-pushed transifex-webhook from 1a54f3b81d to 23931cf91f 2023-01-12 06:33:30 +00:00 Compare
PeterSurda requested changes 2023-01-12 06:50:27 +00:00
@ -6,0 +7,4 @@
import requests
import time
import logging
Owner

no need logging

no need `logging`
Owner

no need for logging

no need for `logging`
@ -6,0 +10,4 @@
import logging
from subprocess import call
from base64 import b64encode
Owner

we already imported base64

we already imported base64
@ -25,0 +63,4 @@
).digest()
)
if tx_signature != header_signature:
return False
Owner

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

maybe just `raise ValueError("Signature mismatch")`, then it will propagate up
@ -102,2 +78,2 @@
'project': repository['full_name'],
'category': event_type,
'author': "buildbot-transifex",
'resource': transifex_response[resource],
Owner

this line ('resource': ...) should be remove

this line (`'resource': ...`) should be remove
@ -103,1 +78,3 @@
'category': event_type,
'author': "buildbot-transifex",
'resource': transifex_response[resource],
'branch': transifex_response["branch"],
Owner

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.
@ -132,1 +94,3 @@
branch = payload.get('language')
def _transform_variables(self, transifex_project, transifex_resource):
key = "{}/{}".format(transifex_project, transifex_resource)
_map = self.map[key]
Owner

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)"```
@ -133,0 +95,4 @@
key = "{}/{}".format(transifex_project, transifex_resource)
_map = self.map[key]
repository = _map["repository"]
project = transifex_project
Owner

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

missing functionality, the project should be extracted from the `repository` variable
@ -151,2 +120,3 @@
p.master = self.master
rendered_secret = yield p.render(secret)
option = self.options
self.rendered_secret = yield p.render(self.secret)
Owner

we can just use rendered_secret, not self.rendered_secret

we can just use `rendered_secret`, not `self.rendered_secret`
@ -175,3 +129,1 @@
raise ValueError('Invalid secret')
event_type = bytes2unicode(payload.get(_EVENT_KEY), "None")
if not self._verifyTransifexSignature(request, content, self.rendered_secret, signature, header_signature):
Owner

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
@ -178,0 +134,4 @@
event_type = payload.get("event", "None")
change["properties"] = {
Owner

duplicate

duplicate
@ -178,0 +140,4 @@
"transifex_project": payload.get("project", "None"),
"transifex_resource": payload.get("resource", "None")
}
transiform_map = self._transform_variables(payload['project'], payload['resource'])
Owner

maybe mapped_request

maybe `mapped_request`
@ -178,0 +143,4 @@
transiform_map = self._transform_variables(payload['project'], payload['resource'])
change["changes"] = {
"author": "buildbot-transifex",
Owner

maybe make "buildbot-transifex" a class variable or a module variable?

maybe make `"buildbot-transifex`" a class variable or a module variable?
PeterSurda requested changes 2023-01-12 07:09:41 +00:00
PeterSurda left a comment
Owner

transifex_branch missing

`transifex_branch` missing
shekhar-cis force-pushed transifex-webhook from 23931cf91f to 4ecb3b9cb9 2023-01-12 10:23:39 +00:00 Compare
shekhar-cis force-pushed transifex-webhook from 4ecb3b9cb9 to ab6d1083ec 2023-01-12 10:36:13 +00:00 Compare
PeterSurda reviewed 2023-01-13 06:33:07 +00:00
@ -102,2 +70,2 @@
'project': repository['full_name'],
'category': event_type,
'author': "buildbot-transifex",
'branch': translated_request["branch"],
Owner

repository missing

`repository` missing
PeterSurda reviewed 2023-01-13 06:34:04 +00:00
@ -123,0 +75,4 @@
"transifex_event": payload.get("event", "None"),
"transifex_project": payload.get("project", "None"),
"transifex_resource": payload.get("resource", "None"),
"transifex_branch": "v0.6"
Owner

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 reviewed 2023-01-13 06:36:00 +00:00
@ -178,0 +123,4 @@
self._verifyTransifexSignature(request, content, rendered_secret, signature, header_signature)
event_type = payload.get("event", "None")
mapped_request = self._transform_variables(payload['project'], payload['resource'])
Owner

duplicate

duplicate
PeterSurda requested changes 2023-01-13 06:36:24 +00:00
PeterSurda left a comment
Owner

now only minor changes left

now only minor changes left
shekhar-cis added 1 commit 2023-01-13 07:00:51 +00:00
This pull request can be merged automatically.
You are not authorized to merge this pull request.
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b shekhar-cis-transifex-webhook master
git pull transifex-webhook

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff shekhar-cis-transifex-webhook
git push origin master
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/buildbot-transifex#1
No description provided.