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.
| return step.getProperty("save_packages") and fnmatch_any( | ||
| step.getProperty("branch"), savedBranches | ||
| step.getProperty("branch"), savedPackageBranches |
There was a problem hiding this comment.
@cvicentiu Please be aware that master-galera has a different value for savedPackageBranches and won't be taken into account.
The savedPackageBranches from constants take precedence.
| for b in builders_galera_mtr: | ||
| if builderName in b: | ||
| if builder_name in b: |
There was a problem hiding this comment.
builder_name in b is not ok, can cause an evaluation to true is some special cases.
Let's say you put in builders_galera_mtr this entry: amd64-ubuntu-2004-debug
It will evaluate true for amd64-ubuntu-2004 also.
Instead let's use == .
| return ( | ||
| not fnmatch_any(branch, releaseBranches), | ||
| not fnmatch_any(branch, savedPackageBranches), | ||
| request.getSubmitTime(), |
There was a problem hiding this comment.
I need to check in BB code, maybe you already did that, but are you sure that the requests object doesn't have the oldest build request at [0] ? If yes, then sorting is not needed, FIFO being satisfied by design.
There was a problem hiding this comment.
More on that, we should first parse requests,
and return the most recent request for a match on releaseBranches.
For example, let's say we have the following case:
-> request 1 - branch bb-10.5-release - tarball 1
-> request 2 - branch bb-10.5-release - tarball 2
It's obvious that we're interested in tarball 2 to finish first because it potentially contains a fix that makes it the candidate for release.
Maybe that's a story for a separate MDBF and we stick just to code refactoring without changing the logic.
This PR introduces two changes:
The branch prioritisation logic should be double checked.