Added parent hook and some renderer #1
24
lib/renderers.py
Normal file
|
@ -0,0 +1,24 @@
|
|||
from buildbot.plugins import util
|
||||
import re
|
||||
|
||||
|
||||
@util.renderer
|
||||
def is_build_script_available(props):
|
||||
# Actual check will got here
|
||||
return props.getProperty("build_available", default=False)
|
||||
|
||||
|
||||
|
||||
@util.renderer
|
||||
def isnt_build_script_available(props):
|
||||
return not is_build_script_available(props)
|
||||
|
||||
|
||||
@util.renderer
|
||||
def is_test_script_available(props):
|
||||
# Actual check will got here
|
||||
return props.getProperty("test_available", default=False)
|
||||
PeterSurda
commented
should also check the correct property should also check the correct property
|
||||
|
||||
|
||||
@util.renderer
|
||||
def isnt_test_script_available(props):
|
||||
return not is_test_script_available(props)
|
179
multibuild.py
|
@ -12,6 +12,59 @@ Requires docker
|
|||
|
||||
from os import walk
|
||||
from os.path import exists, isfile, join, listdir
|
||||
import requests
|
||||
import re
|
||||
from buildbot.plugins import steps, util
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
The hostname part of the 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.
Ss_singh
commented
What's the hostname part again? What's the hostname part again?
PeterSurda
commented
https://git.bitmessage.org/Bitmessage/buildbot-config/src/branch/master/master.cfg#L1435
Ss_singh
commented
You said it should be dynamic, where should this come from then? You said it should be dynamic, where should this come from then?
PeterSurda
commented
Something like
the Something like
```
c['buildbotURL'] + '/change_hook/base'
```
the `buildbotURL` could be passed as an argument to `trigger_child_hooks`.
|
||||
|
||||
from .lib.renderers import *
|
||||
|
||||
ty = "/change_hook/base"
|
||||
request_headers = {
|
||||
"Content-Type": "application/x-www-form-urlencoded",
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
this should be extracted from the current properties. this should be extracted from the current properties.
Ss_singh
commented
You mean it should come from 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?
PeterSurda
commented
Yes.
You should pass it as it is. Similar with "branch". I don't think others need passing.
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".
|
||||
"Accept": "text/plain",
|
||||
}
|
||||
request_data = {
|
||||
"project": "testproject",
|
||||
"comments": "testcomment",
|
||||
}
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
this is fixed, derived from the base hook class. this is fixed, derived from the base hook class.
Ss_singh
commented
Since we don't need Since we don't need `type`, I'd just remove it
PeterSurda
commented
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.
Ss_singh
commented
So what this 'base webhook' should actually be? So what this 'base webhook' should actually be?
PeterSurda
commented
I think it's fixed at I think it's fixed at `/change_hook/base`
|
||||
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
this can be extracted from global variables this can be extracted from global variables
Ss_singh
commented
Similarly we don't need Similarly we don't need `node` here so it can be removed
|
||||
dockerfile_extra_contents_focal = """
|
||||
|
||||
# Buildbot
|
||||
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
|
||||
|
||||
# buildbot entrypoint
|
||||
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
|
||||
PeterSurda
commented
should be the same as for 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:
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"
```
|
||||
|
||||
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 = """
|
||||
|
||||
# Buildbot
|
||||
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 wget
|
||||
|
||||
# buildbot entrypoint
|
||||
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"
|
||||
|
||||
"""
|
||||
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
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.
Ss_singh
commented
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.
PeterSurda
commented
Yes something like that, but it should be only the buildbot-specific stuff and not the whole existing Yes something like that, but it should be only the buildbot-specific stuff and not the whole existing `Dockerfile`
Ss_singh
commented
I've pushed an update resolving this. Kindly have a look I've pushed an update resolving this. Kindly have a look
|
||||
|
||||
def list_jobs(directory=".buildbot"):
|
||||
|
@ -20,10 +73,10 @@ def list_jobs(directory=".buildbot"):
|
|||
"""
|
||||
PeterSurda
commented
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 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 = []
|
||||
for _ in next(walk(directory))[1]:
|
||||
PeterSurda
commented
`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.
PeterSurda
commented
I think it would look something like this (still may need fine tuning):
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)
```
Ss_singh
commented
Can I access this 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
PeterSurda
commented
`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.
|
||||
if exists(join(directory, _, "Dockerfile")) \
|
||||
and (exists(join(directory, _, "build.sh"))
|
||||
or exists(join(directory, _, "test.sh"))
|
||||
):
|
||||
if exists(join(directory, _, "Dockerfile")) and (
|
||||
PeterSurda
commented
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.
Ss_singh
commented
And those are what? 'repository', 'branch', 'author', these things? And those are what? 'repository', 'branch', 'author', these things?
PeterSurda
commented
yes. I think for an MVP we only need 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.
|
||||
exists(join(directory, _, "build.sh"))
|
||||
or exists(join(directory, _, "test.sh"))
|
||||
):
|
||||
results.append(_)
|
||||
|
||||
return results
|
||||
|
@ -37,3 +90,121 @@ def find_artifacts(directory="out"):
|
|||
if not isfile(join(directory, _)):
|
||||
continue
|
||||
return join(directory, _)
|
||||
|
||||
|
||||
def get_dockerfile_contents(path, os_codename):
|
||||
"""
|
||||
Ss_singh marked this conversation as resolved
PeterSurda
commented
How about just
How about just
```
contents = re.sub(r"(?m)^(CMD|ENTRYFILE).*$", "", contents)
```
|
||||
Read contents of a Dockerfile and add extra contents for the given os_codename
|
||||
"""
|
||||
with open(path, "r") as file:
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
No No `eval`, it's evil. Maybe use a dict, or just try the multistage.
|
||||
contents = file.read()
|
||||
# remove any line containing CMD or ENTRYFILE keywords
|
||||
re.sub(r"(?m)^(CMD|ENTRYFILE).*$", "", contents)
|
||||
|
||||
return contents + {
|
||||
"focal": dockerfile_extra_contents_focal,
|
||||
"bionic": dockerfile_extra_contents_bionic,
|
||||
}[os_codename]
|
||||
|
||||
|
||||
def trigger_child_hooks(buildbotUrl: str, os_codename: str, repository, branch, jobname, directory=".buildbot"):
|
||||
request_url = buildbotUrl + ty
|
||||
|
||||
# List all jobs in the directory
|
||||
jobs = list_jobs(directory)
|
||||
|
||||
# Check if build.sh or test.sh exists in each of the jobs
|
||||
for job in jobs:
|
||||
build_script_exists = False
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
I think it should be
I think it should be
```
request_data['properties']['dockerfile'] = ...
```
|
||||
test_script_exists = False
|
||||
if exists(join(directory, job, "build.sh")):
|
||||
build_script_exists = True
|
||||
if exists(join(directory, job, "test.sh")):
|
||||
test_script_exists = True
|
||||
Ss_singh marked this conversation as resolved
PeterSurda
commented
proxied properties are missing proxied properties are missing
PeterSurda
commented
I think they are still missing. Like I think they are still missing. Like `repository` and `branch`.
Ss_singh
commented
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.
|
||||
|
||||
# make a post request
|
||||
request_data["properties"] = {
|
||||
"dockerfile": get_dockerfile_contents(
|
||||
join(directory, job, "Dockerfile"), os_codename
|
||||
),
|
||||
"build_script_available": is_build_script_available(build_script_exists),
|
||||
"test_script_available": is_test_script_available(test_script_exists),
|
||||
"repository": repository,
|
||||
PeterSurda
commented
I think I think `repository` and `branch` are a level higher.
|
||||
"branch": branch,
|
||||
"jobname": jobname,
|
||||
}
|
||||
requests.post(request_url, headers=request_headers, data=request_data)
|
||||
|
||||
|
||||
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",
|
||||
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,
|
||||
Ss_singh marked this conversation as resolved
PeterSurda
commented
should also have conditionals for should also have conditionals for `build_script_exists`. If it doesn't, this step is skipped and hidden.
|
||||
"https://buildbot.bitmessage.org",
|
||||
util.Interpolate("%(prop:os_codename)s"),
|
||||
Ss_singh marked this conversation as resolved
PeterSurda
commented
I wouldn't change the directory, but just run it from it, i.e.
I wouldn't change the directory, but just run it from it, i.e.
```
command=["bash", "-c", join(directory, job, "build.sh")],
|
||||
],
|
||||
PeterSurda
commented
For now it's probably ok. Over longer term however the 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_build_sh_step(build_factory, job, directory=".buildbot"):
|
||||
PeterSurda
commented
I'm not 100% sure this will pass properties. A safer solution would be to create a child of 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(
|
||||
Ss_singh marked this conversation as resolved
Outdated
PeterSurda
commented
analogous changes as with analogous changes as with `build.sh`
|
||||
steps.ShellCommand(
|
||||
name="build_" + job,
|
||||
command=["bash", "-c", join(directory, job, "build.sh")],
|
||||
doStepIf=is_build_script_available,
|
||||
hideStepIf=isnt_build_script_available,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
def add_child_test_sh_step(build_factory, job, directory=".buildbot"):
|
||||
"""
|
||||
Add a step to the build factory
|
||||
"""
|
||||
build_factory.addStep(
|
||||
steps.ShellCommand(
|
||||
name="test_" + job,
|
||||
command=["bash", "-c", join(directory, job, "test.sh")],
|
||||
doStepIf=is_test_script_available,
|
||||
hideStepIf=isnt_test_script_available,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
# expect jobname, repository, branch, buildbotUrl, os_codename from command line args
|
||||
import sys
|
||||
|
||||
if len(sys.argv) == 6:
|
||||
jobname = sys.argv[1]
|
||||
repository = sys.argv[2]
|
||||
branch = sys.argv[3]
|
||||
buildbotUrl = sys.argv[4]
|
||||
os_codename = sys.argv[5]
|
||||
|
||||
trigger_child_hooks(buildbotUrl, os_codename, repository, branch, jobname)
|
||||
else:
|
||||
print(
|
||||
"Usage: python3 multibuild.py <jobname> <repository> <branch> <buildbotUrl> <os_codename>"
|
||||
)
|
||||
|
|
should check for the correct property