Refactor utils.py #637
Refactor utils.py #637
Conversation
cvicentiu
force-pushed
the
refactor-3
branch
2 times, most recently
from
04671c9
to
1c53f57
Compare
cvicentiu
force-pushed
the
refactor-3
branch
from
1c53f57
to
d78def1
Compare
1. Introduce typehints 2. Include what you use.
| @@ -19,6 +19,7 @@ sys.setrecursionlimit(10000) | |||
| sys.path.insert(0, "/srv/buildbot/master") | |||
|
|
|||
| from utils import * | |||
There was a problem hiding this comment.
Should we take that refactor opportunity to remove the wildcard import?
There was a problem hiding this comment.
master.cfg will come next, in a separate PR :)
| @@ -123,7 +120,7 @@ def createWorker( | |||
| docker_host=private_config["private"]["docker_workers"][worker_name], | |||
| image=image_str, | |||
| dockerfile=dockerfile_str, | |||
| tls=tls, | |||
| tls=None, | |||
There was a problem hiding this comment.
It was always passed as none. See the deletion from above. I just made it obvious.
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok for most of the worker, this is fine since we use wireguard, no reason to use extra layer of security. But we have still a bunch of workers that communicate on the public IP, @RazvanLiviuVarzaru now that you have a better overview could you please double check that point and if that applies here?
There was a problem hiding this comment.
Indeed it was always None as default on every function call.
Shouldn't affect existing workers configured via createWorker function.
I propose to set tls with default value None as a function argument, for future scenarios where wireguard is not an option.
Having it as an argument is clear enough for the caller also.
This PR introduces two changes:
The branch prioritisation logic should be double checked.