Added parent hook and some renderer #1

Merged
PeterSurda merged 7 commits from Ss_singh/buildbot_multibuild:hooks into master 2021-12-26 09:05:49 +01:00
Showing only changes of commit 9a55687ec6 - Show all commits

View File

@ -14,18 +14,19 @@ from os import walk
from os.path import exists, isfile, join, listdir from os.path import exists, isfile, join, listdir
import requests import requests
import re import re
from buildbot.plugins import steps, util
Ss_singh marked this conversation as resolved Outdated

The hostname part of the request_url should be dynamic. However since it doesn't depend on a specific job, it doesn't need to use interpolate.

The hostname part of the `request_url` should be dynamic. However since it doesn't depend on a specific job, it doesn't need to use interpolate.

What's the hostname part again?

What's the hostname part again?
https://git.bitmessage.org/Bitmessage/buildbot-config/src/branch/master/master.cfg#L1435

You said it should be dynamic, where should this come from then?

You said it should be dynamic, where should this come from then?

Something like

c['buildbotURL'] + '/change_hook/base'

the buildbotURL could be passed as an argument to trigger_child_hooks.

Something like ``` c['buildbotURL'] + '/change_hook/base' ``` the `buildbotURL` could be passed as an argument to `trigger_child_hooks`.
ty = '/change_hook/base' ty = "/change_hook/base"
request_headers = { request_headers = {
'Content-Type': 'application/x-www-form-urlencoded', "Content-Type": "application/x-www-form-urlencoded",
'Accept': 'text/plain' "Accept": "text/plain",
} }
Ss_singh marked this conversation as resolved Outdated

this should be extracted from the current properties.

this should be extracted from the current properties.

You mean it should come from util.Property("repository")? Do I need to save this repo url in some property?
Like, property="repo_url" in buildFactory step and use that later when executing the multibuild.py script?

You mean it should come from `util.Property("repository")`? Do I need to save this repo url in some property? Like, `property="repo_url"` in buildFactory step and use that later when executing the multibuild.py script?

You mean it should come from util.Property("repository")?

Yes.

Do I need to save this repo url in some property?

You should pass it as it is. Similar with "branch". I don't think others need passing.

Like, property="repo_url" in buildFactory step and use that later when executing the multibuild.py script?

If I understand you correctly, yes. These two properties you're basically just proxying from the parent to the child. But you're also adding more properties, mainly "jobname" and "dockerfile".

> You mean it should come from util.Property("repository")? Yes. > Do I need to save this repo url in some property? You should pass it as it is. Similar with "branch". I don't think others need passing. > Like, property="repo_url" in buildFactory step and use that later when executing the multibuild.py script? If I understand you correctly, yes. These two properties you're basically just proxying from the parent to the child. But you're also adding more properties, mainly "jobname" and "dockerfile".
request_data = { request_data = {
'project': 'testproject', "project": "testproject",
'comments': 'testcomment', "comments": "testcomment",
} }
dockerfile_extra_contents_focal = ''' dockerfile_extra_contents_focal = """
Ss_singh marked this conversation as resolved Outdated

this is fixed, derived from the base hook class.

this is fixed, derived from the base hook class.

Since we don't need type, I'd just remove it

Since we don't need `type`, I'd just remove it

We still need to construct the full URL, which consists of the URL of the buildbot server and the path for the base webhook. The latter part is statoc, I don't even think it's configurable in buildbot, it's derived from the hook class.

We still need to construct the full URL, which consists of the URL of the buildbot server and the path for the base webhook. The latter part is statoc, I don't even think it's configurable in buildbot, it's derived from the hook class.

So what this 'base webhook' should actually be?

So what this 'base webhook' should actually be?

I think it's fixed at /change_hook/base

I think it's fixed at `/change_hook/base`
Ss_singh marked this conversation as resolved Outdated

this can be extracted from global variables

this can be extracted from global variables

Similarly we don't need node here so it can be removed

Similarly we don't need `node` here so it can be removed
# Buildbot # Buildbot
RUN apt-get install -yq --no-install-suggests --no-install-recommends \ RUN apt-get install -yq --no-install-suggests --no-install-recommends \
@ -38,9 +39,9 @@ USER buildbot
ENTRYPOINT /entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS" ENTRYPOINT /entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS"

should be the same as for bionic (different path, missing wget)

Also I have a pending PR for adding multistage builds in buildbot: https://github.com/buildbot/buildbot/pull/6314

This way we can have all of this in just one Dockerfile stub, just in different stagesand use build arguments to pass the distro docker in realtime.

It would then look something like this:

FROM stage1 AS buildbot-ubuntu-bionic

RUN apt-get install -yq --no-install-suggests --no-install-recommends \
    buildbot-slave git subversion python3-dev libffi-dev python3-setuptools \
    python3-pip dumb-init curl openssh-client

FROM stage1 AS buildbot-ubuntu-focal

RUN apt-get install -yq --no-install-suggests --no-install-recommends \
    buildbot-worker git subversion python3-dev libffi-dev python3-setuptools \
    python3-pip dumb-init curl openssh-client wget

FROM buildbot-$DISTRO-$VARIANT AS buildbot-run

RUN wget -O /usr/local/bin/buildbot_entrypoint.sh https://git.bitmessage.org/Bitmessage/buildbot-scripts/raw/branch/master/docker/bionic/entrypoint.sh
RUN chmod +x /usr/local/bin/buildbot_entrypoint.sh

RUN echo 'buildbot ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers

USER buildbot

ENTRYPOINT /usr/local/bin/buildbot_entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS"
should be the same as for `bionic` (different path, missing wget) Also I have a pending PR for adding multistage builds in buildbot: https://github.com/buildbot/buildbot/pull/6314 This way we can have all of this in just one Dockerfile stub, just in different stagesand use build arguments to pass the distro docker in realtime. It would then look something like this: ``` FROM stage1 AS buildbot-ubuntu-bionic RUN apt-get install -yq --no-install-suggests --no-install-recommends \ buildbot-slave git subversion python3-dev libffi-dev python3-setuptools \ python3-pip dumb-init curl openssh-client FROM stage1 AS buildbot-ubuntu-focal RUN apt-get install -yq --no-install-suggests --no-install-recommends \ buildbot-worker git subversion python3-dev libffi-dev python3-setuptools \ python3-pip dumb-init curl openssh-client wget FROM buildbot-$DISTRO-$VARIANT AS buildbot-run RUN wget -O /usr/local/bin/buildbot_entrypoint.sh https://git.bitmessage.org/Bitmessage/buildbot-scripts/raw/branch/master/docker/bionic/entrypoint.sh RUN chmod +x /usr/local/bin/buildbot_entrypoint.sh RUN echo 'buildbot ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers USER buildbot ENTRYPOINT /usr/local/bin/buildbot_entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS" ```
''' """
dockerfile_extra_contents_bionic = ''' dockerfile_extra_contents_bionic = """
# Buildbot # Buildbot
RUN apt-get install -yq --no-install-suggests --no-install-recommends \ RUN apt-get install -yq --no-install-suggests --no-install-recommends \
@ -57,7 +58,8 @@ USER buildbot
ENTRYPOINT /usr/local/bin/buildbot_entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS" ENTRYPOINT /usr/local/bin/buildbot_entrypoint.sh "$BUILDMASTER" "$WORKERNAME" "$WORKERPASS"
''' """
def list_jobs(directory=".buildbot"): def list_jobs(directory=".buildbot"):
""" """
@ -65,10 +67,10 @@ def list_jobs(directory=".buildbot"):
""" """
results = [] results = []
Ss_singh marked this conversation as resolved Outdated

There should be additional code for modifying the dockerfile. At the very least, it needs to install buildbot-worker and its dependencies, setup the default worker and run it in entrypoint. At the moment we can assume "apt", but in the future it should be able to detect the distro and adjust commands based on that.

For security it should also filter the file to remove CMD or ENTRYFILE lines.

There should be additional code for modifying the dockerfile. At the very least, it needs to install buildbot-worker and its dependencies, setup the default worker and run it in entrypoint. At the moment we can assume "apt", but in the future it should be able to detect the distro and adjust commands based on that. For security it should also filter the file to remove CMD or ENTRYFILE lines.

Okay it's like take the contents of already available Dockerfile and append it with extra commands based on the distro.

Okay it's like take the contents of already available Dockerfile and append it with extra commands based on the distro.

Yes something like that, but it should be only the buildbot-specific stuff and not the whole existing Dockerfile

Yes something like that, but it should be only the buildbot-specific stuff and not the whole existing `Dockerfile`

I've pushed an update resolving this. Kindly have a look

I've pushed an update resolving this. Kindly have a look
for _ in next(walk(directory))[1]: for _ in next(walk(directory))[1]:
if exists(join(directory, _, "Dockerfile")) \ if exists(join(directory, _, "Dockerfile")) and (
and (exists(join(directory, _, "build.sh")) exists(join(directory, _, "build.sh"))
or exists(join(directory, _, "test.sh")) or exists(join(directory, _, "test.sh"))
): ):

I realised a problem. The code needs to be split between master and worker parts. Master code doesn't see the directories and files, and worker code doesn't have access to properties.

So my suggestion would be to add a BuildStep that constructs a JSON string (i.e. a renderer) and uploads it into the worker as a file. The next step will instruct the worker to run a script that will fetch the props from this JSON files, combines them with the per-job props and calls the child hooks.

I realised a problem. The code needs to be split between master and worker parts. Master code doesn't see the directories and files, and worker code doesn't have access to properties. So my suggestion would be to add a `BuildStep` that constructs a JSON string (i.e. a renderer) and uploads it into the worker as a file. The next step will instruct the worker to run a script that will fetch the props from this JSON files, combines them with the per-job props and calls the child hooks.
results.append(_) results.append(_)

node and type we don't need, that's for sysdeploy. we should put all the properties (build_script_available, test_script_available, dockerfile, ...) into this dict, there shouldn't be a sub-dict.

`node` and `type` we don't need, that's for sysdeploy. we should put all the properties (build_script_available, test_script_available, dockerfile, ...) into this dict, there shouldn't be a sub-dict.

I think it would look something like this (still may need fine tuning):

request_data = {
    'repository': Util.Property('repository'),
    'branch': Util.Property('branch'),
    'properties': {
                  'build_script_available': build_script_exists,
                  'test_script_available': test_script_exists,
                  'dockerfile': dockerfile
                  }
               }

requests.post(request_url, headers=request_headers, data=request_data)
I think it would look something like this (still may need fine tuning): ``` request_data = { 'repository': Util.Property('repository'), 'branch': Util.Property('branch'), 'properties': { 'build_script_available': build_script_exists, 'test_script_available': test_script_exists, 'dockerfile': dockerfile } } requests.post(request_url, headers=request_headers, data=request_data) ```

Can I access this Util in multibuid.py as well? I just passed the 'repository' and 'branch' as command line args while executing multibuild.py

Can I access this `Util` in multibuid.py as well? I just passed the 'repository' and 'branch' as command line args while executing multibuild.py

multibuild.py will have to be loaded from the buildbot config, just like now it's importing lib/wine.py, for example. But it can be made into a separate pip module, for example, so it can be developed and used independently.

`multibuild.py` will have to be loaded from the buildbot config, just like now it's importing `lib/wine.py`, for example. But it can be made into a separate pip module, for example, so it can be developed and used independently.
return results return results

we also need to add the repo information, i.e. the pre-existing properties.

we also need to add the repo information, i.e. the pre-existing properties.

And those are what? 'repository', 'branch', 'author', these things?

And those are what? 'repository', 'branch', 'author', these things?

yes. I think for an MVP we only need repository and branch, maybe revision. For later, probably owner and owners (the way I have it setup now, it would allow people to re-run jobs for their own PRs manually, without these two only admin can re-run job). I don't think author is needed, that's just something I borrowed from somewhere else.

yes. I think for an MVP we only need `repository` and `branch`, maybe `revision`. For later, probably `owner` and `owners` (the way I have it setup now, it would allow people to re-run jobs for their own PRs manually, without these two only admin can re-run job). I don't think `author` is needed, that's just something I borrowed from somewhere else.
@ -83,19 +85,21 @@ def find_artifacts(directory="out"):
continue continue
return join(directory, _) return join(directory, _)
def get_dockerfile_contents(path, os_codename): def get_dockerfile_contents(path, os_codename):
''' """
Read contents of a Dockerfile and add extra contents for the given os_codename Read contents of a Dockerfile and add extra contents for the given os_codename
''' """
with open(path, 'r') as file: with open(path, "r") as file:
contents = file.read() contents = file.read()
# remove any line containing CMD or ENTRYFILE keywords # remove any line containing CMD or ENTRYFILE keywords
contents = re.sub(r'(?m)^CMD.*$', '', contents) contents = re.sub(r"(?m)^CMD.*$", "", contents)
Ss_singh marked this conversation as resolved
Review

How about just

contents = re.sub(r"(?m)^(CMD|ENTRYFILE).*$", "", contents)
How about just ``` contents = re.sub(r"(?m)^(CMD|ENTRYFILE).*$", "", contents) ```
contents = re.sub(r'(?m)^ENTRYFILE.*$', '', contents) contents = re.sub(r"(?m)^ENTRYFILE.*$", "", contents)
return contents + eval("dockerfile_extra_contents_" + os_codename) return contents + eval("dockerfile_extra_contents_" + os_codename)
Ss_singh marked this conversation as resolved Outdated

No eval, it's evil. Maybe use a dict, or just try the multistage.

No `eval`, it's evil. Maybe use a dict, or just try the multistage.
def trigger_child_hooks(buildbotUrl:str, os_codename:str, directory=".buildbot"):
def trigger_child_hooks(buildbotUrl: str, os_codename: str, directory=".buildbot"):
request_url = buildbotUrl + ty request_url = buildbotUrl + ty
# List all jobs in the directory # List all jobs in the directory
@ -111,14 +115,70 @@ def trigger_child_hooks(buildbotUrl:str, os_codename:str, directory=".buildbot")
test_script_exists = True test_script_exists = True
# make a post request # make a post request
request_data['dockerfile'] = get_dockerfile_contents(join(directory, job, "Dockerfile"), os_codename) request_data["dockerfile"] = get_dockerfile_contents(
Ss_singh marked this conversation as resolved Outdated

I think it should be

request_data['properties']['dockerfile'] = ...
I think it should be ``` request_data['properties']['dockerfile'] = ... ```
request_data['build_script_available'] = str(build_script_exists) join(directory, job, "Dockerfile"), os_codename
request_data['test_script_available'] = str(test_script_exists) )
request_data["build_script_available"] = str(build_script_exists)
request_data["test_script_available"] = str(test_script_exists)
requests.post(request_url, headers=request_headers, data=request_data) requests.post(request_url, headers=request_headers, data=request_data)
Ss_singh marked this conversation as resolved
Review

proxied properties are missing

proxied properties are missing
Review

I think they are still missing. Like repository and branch.

I think they are still missing. Like `repository` and `branch`.
Review

Well, they were already being added before the hook function was getting triggered but anyways, now I've them placed altogether.

Well, they were already being added before the hook function was getting triggered but anyways, now I've them placed altogether.
def add_parent_step(build_factory, jobname, repository, branch):
"""
Add a step to the parent build factory that will trigger the child hooks
"""
build_factory.addStep(
steps.SetPropertyFromCommand(
name="Get OS codename",
Review

I think repository and branch are a level higher.

I think `repository` and `branch` are a level higher.
command="grep ^VERSION_CODENAME= /etc/os-release | cut -d= -f2",
property="os_codename",
)
)
build_factory.addStep(
steps.ShellCommand(
name="Execute multibuild script",
command=[
"python",
"multibuild.py",
jobname,
repository,
branch,
"https://buildbot.bitmessage.org",
util.Interpolate("%(prop:os_codename)s"),
],
)
)
def add_child_build_sh_step(build_factory, job, directory=".buildbot"):
"""
Add a step to the build factory
"""
build_factory.addStep(
steps.ShellCommand(
Ss_singh marked this conversation as resolved
Review

should also have conditionals for build_script_exists. If it doesn't, this step is skipped and hidden.

should also have conditionals for `build_script_exists`. If it doesn't, this step is skipped and hidden.
name="build_" + job,
command=["bash", "-c", "cd " + join(directory, job) + " && ./build.sh"],
Ss_singh marked this conversation as resolved
Review

I wouldn't change the directory, but just run it from it, i.e.

command=["bash", "-c", join(directory, job, "build.sh")],
I wouldn't change the directory, but just run it from it, i.e. ``` command=["bash", "-c", join(directory, job, "build.sh")],
)
Review

For now it's probably ok. Over longer term however the os_codename isn't extracted from the parent (parent may be running something else than the child should), but from the child Dockerfile. The Dockerfile will begin with something like FROM ubuntu:bionic AS ..., so that determines the OS.

For now it's probably ok. Over longer term however the `os_codename` isn't extracted from the parent (parent may be running something else than the child should), but from the child `Dockerfile`. The `Dockerfile` will begin with something like `FROM ubuntu:bionic AS ...`, so that determines the OS.
)
def add_child_test_sh_step(build_factory, job, directory=".buildbot"):
"""
Review

I'm not 100% sure this will pass properties. A safer solution would be to create a child of ShellCommand and call super with a constructed command.

I'm not 100% sure this will pass properties. A safer solution would be to create a child of `ShellCommand` and call `super` with a constructed `command`.
Add a step to the build factory
"""
build_factory.addStep(
steps.ShellCommand(
Ss_singh marked this conversation as resolved Outdated

analogous changes as with build.sh

analogous changes as with `build.sh`
name="test_" + job,
command=["bash", "-c", "cd " + join(directory, job) + " && ./test.sh"],
)
)
if __name__ == "__main__": if __name__ == "__main__":
# expect jobname, repository, branch, buildbotUrl, os_codename from command line args # expect jobname, repository, branch, buildbotUrl, os_codename from command line args
import sys import sys
if len(sys.argv) == 6: if len(sys.argv) == 6:
jobname = sys.argv[1] jobname = sys.argv[1]
repository = sys.argv[2] repository = sys.argv[2]
@ -127,10 +187,12 @@ if __name__ == "__main__":
os_codename = sys.argv[5] os_codename = sys.argv[5]
# add these into the request_data # add these into the request_data
request_data['jobname'] = jobname request_data["jobname"] = jobname
request_data['repository'] = repository request_data["repository"] = repository
request_data['branch'] = branch request_data["branch"] = branch
trigger_child_hooks(buildbotUrl, os_codename) trigger_child_hooks(buildbotUrl, os_codename)
else: else:
print("Usage: python3 multibuild.py <jobname> <repository> <branch> <buildbotUrl> <os_codename>") print(
"Usage: python3 multibuild.py <jobname> <repository> <branch> <buildbotUrl> <os_codename>"
)