PR#52: Make fixed-architecture job scheduling possible -

taskotron / taskotron-trigger

#52 Make fixed-architecture job scheduling possible

Closed Apr 27, 2018 by jskladan. Opened Apr 25, 2018 by jskladan.

feature/arch_specific_tasks into develop

jskladan commented Apr 25, 2018

Adds arch_dependent key to the task section of trigger config file. if set to True, the jobrunner will instruct Buildbot to select a builder tagged with the specified architecture. 'noarch' is used otherwise.

From what I gather based on the current Buildbot configuration ( https://pagure.io/fedora-qa/qa-ansible/blob/qa-master/f/roles/taskotron/buildmaster-configure/templates/taskotron.master.cfg.j2#_78 and following lines), this is already "accidentally enabled", but I'd like to have a second opinion on that.
I also made a guesstimate on which key is used to tell buildbot which Builder to use - see lines 59/60 of the runners.py diff

kparal commented Apr 27, 2018

I'm proposing to use host_arch instead of run_arch, so perhaps it would make sense to be adjusted here as well.

kparal commented Apr 27, 2018

Instead of arch_dependent, would match_host_arch be clearer? (IIRC that as a name you proposed, and I like it).

kparal commented Apr 27, 2018

I'm not sure why you renamed trigger_tasks to trigger_jobs, but since you did, would it make sense to use job instead of task here as well?

kparal commented Apr 27, 2018

I don't understand the FIXME. It is configurable now (as default_run_arch), isn't it?

kparal commented Apr 27, 2018

Perhaps (run on %s host) to make it clearer?

kparal commented Apr 27, 2018

I'm confused here. It the keyval is present, why don't you use the value and instead hardcode True? What if there's arch_dependent: False in the trigger rules config?

kparal commented Apr 27, 2018

host_arch instead of run_arch could be more obvious

kparal commented Apr 27, 2018

See the few nitpicks, otherwise looks reasonable to me. This hasn't been tested, right? We can make a scratch build before committing this and test it on taskotron-dev, if you prefer.

jskladan commented Apr 27, 2018

Not really. In my mind, at least. A "job" is a combination of task, arch, item, type, .... I changed the name to better match what the method does - it takes a list of tasks and parameters, and makes sure the jobs are scheduled (we have jobrunners not taskrunners for example).

jskladan commented Apr 27, 2018

forgot to remove it, good catch

jskladan commented Apr 27, 2018

I could not care less, why not :)

jskladan commented Apr 27, 2018

because it is a boolean value, and I like True/False more than 'no'/'' or {1:2}/[].
If there's arch_dependent: False in the config, then the condition on line 43 would be false, and the default value (False) from line 35 is kept.
I give it to you, that the condition could have been simplified to if item.get('arch_depentent')

jskladan commented Apr 27, 2018

One could also argue, that the whole thing could have been written as task['arch_dependent'] = item.get('arch_dependent', False) but for some reason I just like the explicit True/False a bit more, even if ... = bool(item.get(...)) is possible.
Anyway, I don't see a functional issue here, it is all the nearly the same to me, but I just prefer explicit True/False for some weird reason.