worker and master splited #9

Merged
PeterSurda merged 1 commits from cis-muzahid/buildbot_multibuild:kivy-test into master 2022-03-09 12:34:02 +00:00
Member

Split master and worker file,

New PR behalf of #8 which is deleted bymistake.

Split master and worker file, New PR behalf of https://git.bitmessage.org/Bitmessage/buildbot_multibuild/pulls/8 which is deleted bymistake.
cis-muzahid added 3 commits 2022-02-28 14:31:04 +00:00
cis-muzahid force-pushed kivy-test from 66a413f4d5 to 6fb2fdfa34 2022-02-28 14:35:58 +00:00 Compare
cis-muzahid added 1 commit 2022-02-28 14:53:08 +00:00
cis-muzahid force-pushed kivy-test from 5aaad5b9b6 to 5a9465a02d 2022-02-28 15:06:37 +00:00 Compare
Owner

--force-directories doesn't do what you think it does.

`--force-directories` doesn't do what you think it does.
PeterSurda reviewed 2022-03-01 07:23:55 +00:00
@ -0,0 +109,4 @@
# make a post request
request_data["properties"] = {
"dockerfile": _get_dockerfile_contents(
join(directory, job, "Dockerfile"), os_codename
Owner

remove os_codename from arguments

remove `os_codename` from arguments
PeterSurda marked this conversation as resolved
PeterSurda requested changes 2022-03-01 07:25:39 +00:00
@ -0,0 +69,4 @@
return results
def _get_dockerfile_contents(jobname, os_codename='bionic'):
Owner

remove os_codename, make it a variable, not an argument

remove `os_codename`, make it a variable, not an argument
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -151,0 +36,4 @@
build_factory.addStep(steps.ShellCommand(
name="download worker",
command=["wget", "--force-directories", "-O", "https://git.bitmessage.org/Bitmessage/buildbot_multibuild/raw/branch/master/lib/worker_multibuild.py", join(getenv['HOME'], '.local/bin/worker_multibuild.py')]
Owner

add mkdir -p

add `mkdir -p`
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -204,7 +90,7 @@ if __name__ == "__main__":
buildbotUrl = sys.argv[4]
os_codename = sys.argv[5]
Owner

no os_codename

no `os_codename`
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -205,3 +91,3 @@
os_codename = sys.argv[5]
trigger_child_hooks(buildbotUrl, os_codename, repository, branch, jobname)
trigger_child_hooks(buildbotUrl, os_codename, repository, branch, jobname, ".buildbot", is_build_script_available, is_test_script_available)
Owner

also here

also here
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-01 09:17:34 +00:00
cis-muzahid requested review from PeterSurda 2022-03-01 09:44:39 +00:00
cis-muzahid force-pushed kivy-test from 01e2caa8f5 to f98a503e2b 2022-03-01 09:46:17 +00:00 Compare
PeterSurda requested changes 2022-03-01 11:09:05 +00:00
@ -0,0 +59,4 @@
list jobs found in a directory
"""
results = []
for _ in next(walk(directory))[1]:
Owner

I wouldn't use walk, we only need one level. Also, we need to add a check that Dockerfile, build.sh and test.sh are regular files and not symlinks (probably two checks needed per file so another function should be written for that)

I wouldn't use walk, we only need one level. Also, we need to add a check that `Dockerfile`, `build.sh` and `test.sh` are regular files and not symlinks (probably two checks needed per file so another function should be written for that)
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -151,0 +36,4 @@
build_factory.addStep(steps.ShellCommand(
name="download worker",
command=["mkdir", "-p", join(getenv['HOME'], '.local/bin'), "&&", "wget", "-O", "https://git.bitmessage.org/Bitmessage/buildbot_multibuild/raw/branch/master/lib/worker_multibuild.py", join(getenv['HOME'], '.local/bin/worker_multibuild.py')]
Owner

should be two separate commands.

should be two separate commands.
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-01 15:02:48 +00:00
PeterSurda requested changes 2022-03-02 06:21:22 +00:00
@ -0,0 +60,4 @@
"""
results = []
files = ["Dockerfile", "build.sh", "test.sh"]
for file in files:
Owner

I think I didn't explain it well. It should look for subdirectories, but only at one level, and should ignore symlinked directories

I think I didn't explain it well. It should look for subdirectories, but only at one level, and should ignore symlinked directories
Author
Member

results = []
files = ["Dockerfile", "build.sh", "test.sh"]
for item in listdir(directory):
print(item)
if isdir(join(directory, item)) and not islink(join(directory, item)):
for file in listdir(join(directory, item)):
if file in files and isfile(join(directory, item, file)) and not islink(join(directory, item, file)):
results.append(file)
return results

Is this logic fine? I haven't remove file name from files while it found in path

results = [] files = ["Dockerfile", "build.sh", "test.sh"] for item in listdir(directory): print(item) if isdir(join(directory, item)) and not islink(join(directory, item)): for file in listdir(join(directory, item)): if file in files and isfile(join(directory, item, file)) and not islink(join(directory, item, file)): results.append(file) return results ```Is this logic fine? I haven't remove file name from files while it found in path```
PeterSurda marked this conversation as resolved
@ -0,0 +63,4 @@
for file in files:
print(join(directory, file))
print(exists(join(directory, file)))
if exists(join(directory, file)) and not islink(join(directory, file)):
Owner

this is incomplete (doesn't filter directories, sockets, devices, etc). Better would be isfile and not islink

this is incomplete (doesn't filter directories, sockets, devices, etc). Better would be `isfile and not islink`
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -154,3 +49,3 @@
name="Execute multibuild script",
name="Execute worker script",
command=[
"python",
Owner

python3

`python3`
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-03 15:31:33 +00:00
cis-muzahid added 1 commit 2022-03-03 15:34:46 +00:00
PeterSurda requested changes 2022-03-04 06:40:24 +00:00
@ -0,0 +62,4 @@
files = ["Dockerfile", "build.sh", "test.sh"]
for item in listdir(directory):
if isdir(join(directory, item)) and not islink(join(directory, item)):
for file in listdir(join(directory, item)):
Owner
for file in files:
    filepath = join(directory, item, file)
    if exists and bad:
       break 2ND LEVEL `for` loop
# here the directory passed check 1
if exists(Dockerfile) and (exists(build.sh) or exists(test.sh)):
    add `item` (DIRECTORY, NOT FILE) to return value
``` for file in files: filepath = join(directory, item, file) if exists and bad: break 2ND LEVEL `for` loop # here the directory passed check 1 if exists(Dockerfile) and (exists(build.sh) or exists(test.sh)): add `item` (DIRECTORY, NOT FILE) to return value ```
Author
Member

With the following condition
if exists(Dockerfile) and (exists(build.sh) or exists(test.sh)):
If we have Dockerfile as a normal file and builds.sh as symlink so its skips Dockerfile as it does not include the directory.
So its clear we have to keep both Dockerfile and builds.sh at same placein the directory

With the following condition ```if exists(Dockerfile) and (exists(build.sh) or exists(test.sh)):``` If we have ```Dockerfile``` as a normal file and ```builds.sh``` as symlink so its skips ```Dockerfile``` as it does not include the directory. So its clear we have to keep both ```Dockerfile and builds.sh ``` at same placein the directory
Owner

These files are all per directory. And only if all the files in the directory pass checks, will the directory be permitted as a job.

These files are all per directory. And only if all the files in the directory pass checks, will the directory be permitted as a job.
PeterSurda marked this conversation as resolved
@ -0,0 +64,4 @@
if isdir(join(directory, item)) and not islink(join(directory, item)):
for file in listdir(join(directory, item)):
if file in files and isfile(join(directory, item, file)) and not islink(join(directory, item, file)):
results.append(file)
Owner

we should add item, not file to the return array (should be fixed as described in the pseudocode above)

we should add `item`, not `file` to the return array (should be fixed as described in the pseudocode above)
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-04 14:34:02 +00:00
PeterSurda requested changes 2022-03-07 07:25:43 +00:00
@ -0,0 +63,4 @@
for item in listdir(directory):
for file in files:
filepath = join(directory, item, file)
if islink(filepath) and not exists(filepath):
Owner
if islink(filepath) or not isfile(filepath):
``` if islink(filepath) or not isfile(filepath): ```
PeterSurda marked this conversation as resolved
@ -0,0 +65,4 @@
filepath = join(directory, item, file)
if islink(filepath) and not exists(filepath):
continue
if exists(join(directory, item, 'Dockerfile')) or exists(join(directory, item, 'build.sh')) or exists(join(directory, item, 'test.sh')):
Owner

indent one level up

indent one level up
Owner

also first or shoud be and and missing brackets

also first `or` shoud be `and` and missing brackets
PeterSurda marked this conversation as resolved
@ -0,0 +82,4 @@
res = ""
inside_allowed_command = False
for line in contents:
if re.match(r"(?m)^(FROM|RUN).*$", line):
Owner

ENV also is allowed

`ENV` also is allowed
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -157,2 +51,3 @@
"multibuild.py",
"python3",
join(getenv['HOME'], '.local/bin/worker_multibuild.py'),
util.Interpolate("%(prop:jobname)s"),
Owner

no jobname in parent. Parent inserts the jobnames into buildbot, doesn't read them from buildbot. Child contains a jobname as a property then.

no `jobname` in parent. Parent inserts the jobnames into buildbot, doesn't read them from buildbot. Child contains a jobname as a property then.
Owner

same for os_codename. Parent inserts os_codename into buildbot.

same for `os_codename`. Parent inserts os_codename into buildbot.
Owner

perhaps insteaad of URL, but it may not be possible at least without workarounds

getURLForBuild(master, builderid, build_number)

maybe also

util.Property('url')
perhaps insteaad of URL, but it may not be possible at least without workarounds ``` getURLForBuild(master, builderid, build_number) ``` maybe also ``` util.Property('url') ```
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-07 16:37:15 +00:00
PeterSurda requested changes 2022-03-08 06:56:44 +00:00
@ -0,0 +97,4 @@
return res + dockerfile_extra_contents[os_codename]
def trigger_child_hooks(buildbotUrl: str, repository, branch, jobname, directory=".buildbot", is_build_script_available, is_test_script_available):
Owner

no jobname
no is_build_script_available
no is_test_script_available

These are all per-jobs variables, extracted from the filesystem in each loop.

no `jobname` no `is_build_script_available` no `is_test_script_available` These are all per-jobs variables, extracted from the filesystem in each loop.
PeterSurda marked this conversation as resolved
@ -0,0 +121,4 @@
"test_script_available": is_test_script_available(test_script_exists),
"repository": repository,
"branch": branch,
"jobname": jobname,
Owner

"jobname": job,

` "jobname": job, `
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -160,3 +57,2 @@
util.Property('branch'),
"https://buildbot.bitmessage.org",
util.Interpolate("%(prop:os_codename)s"),
util.getURLForBuild(util, util.Property("builderid"), util.Property("buildnumber")),
Owner

try:
util.Property('url')

try: ` util.Property('url') `
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -205,3 +98,2 @@
os_codename = sys.argv[5]
trigger_child_hooks(buildbotUrl, os_codename, repository, branch, jobname)
trigger_child_hooks(buildbotUrl, repository, branch, jobname, ".buildbot", is_build_script_available, is_test_script_available)
Owner

again remove jobname, is_build_script_available, is_test_script_available

again remove `jobname`, `is_build_script_available`, `is_test_script_available`
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-08 15:28:21 +00:00
PeterSurda requested changes 2022-03-09 06:44:35 +00:00
multibuild.py Outdated
@ -81,3 +22,1 @@
results.append(_)
return results
os_codename='bionic'
Owner

can be removed

can be removed
PeterSurda marked this conversation as resolved
@ -202,10 +94,9 @@ if __name__ == "__main__":
repository = sys.argv[2]
branch = sys.argv[3]
buildbotUrl = sys.argv[4]
Owner

remove buildbotUrl

remove `buildbotUrl`
Author
Member

Sorry, I think buildbotUrl need to be here

Sorry, I think `buildbotUrl` need to be here
Owner

I think you're right.

I think you're right.
PeterSurda marked this conversation as resolved
multibuild.py Outdated
@ -205,3 +97,2 @@
os_codename = sys.argv[5]
trigger_child_hooks(buildbotUrl, os_codename, repository, branch, jobname)
trigger_child_hooks(buildbotUrl, repository, branch, ".buildbot")
Owner

remove ".buildbot"

remove `".buildbot"`
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-09 08:38:40 +00:00
cis-muzahid force-pushed kivy-test from d501ce26a0 to 2b3963b9ae 2022-03-09 08:41:34 +00:00 Compare
PeterSurda requested changes 2022-03-09 11:48:24 +00:00
multibuild.py Outdated
@ -208,3 +96,3 @@
else:
print(
"Usage: python3 multibuild.py <jobname> <repository> <branch> <buildbotUrl> <os_codename>"
"Usage: python3 multibuild.py <jobname> <repository> <branch> <buildbotUrl> "
Owner

This help message needs to be updated to reflect the arguments correctly.

This help message needs to be updated to reflect the arguments correctly.
PeterSurda marked this conversation as resolved
cis-muzahid added 1 commit 2022-03-09 12:03:40 +00:00
cis-muzahid force-pushed kivy-test from 08999a7a91 to d847415a41 2022-03-09 12:05:10 +00:00 Compare
PeterSurda approved these changes 2022-03-09 12:23:45 +00:00
PeterSurda merged commit d847415a41 into master 2022-03-09 12:34:02 +00:00
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_multibuild#9
No description provided.