[devel at bb.net] [users at bb.net] Another anecdote from 0.9.2 scheduler modification.

Tue Jan 3 15:57:15 UTC 2017

Hi Pierre,

Thanks for the reply.

The current status of my code is that all the schedulers now support 
checkConfig and reconfigService. I can also (after some wrangling) start 
buildbot using any of the schedulers. I have yet to test 
reconfiguration, and I have not run any of the current buildbot unit 
tests. Nor have I done any of the github stuff. I did subscribe to the 
dev list (it was really just a matter of time).

My current code (which appears to work) adds another argument for 
ForceSchedulers 'properties'. The __init__() function uses 'properties', 
but then calls BaseScheduler.__init__() using the new argument, which 
ends up in kwargs for most of the trip around to 
ForceScheduler.checkConfig(). The code cribbed from the old __init__() 
is adjusted to use the new name. Thus, the current API and behavior are 
preserved.

I don't think not calling super.checkConfig is a good idea. We still 
need to check the rest of the config against the base classes (in this 
case BaseScheduler and BuildbotService). Incidentally, my current code 
only uses super if the code in __init__() used it. Most of the classes 
used explicit bases, so I left that. It's a decent candidate for a later 
change.

The current code changes are pretty extensive, yes, because they affect 
every scheduler class. I think it might be good to stage the PRs this way:

1. Regularize compare_attrs for schedulers. In 0.9.2, several schedulers 
appear to have a problem where they do not include their base classes' 
compare_attrs. This wasn't as important when there was no effective 
reconfiguration.

2. Add the checkConfig and reconfigService functions, but leave them 
empty. This will at least ensure that I have those setup up properly.

3. Fill the functions.

My current task is to exercise actual reconfigurability. This will 
involve learning how to write a buildbot test, which will probably be 
predicated on learning how to run the current tests.

Neil Gilmore
grammatech.com

On 1/3/2017 2:42 AM, Pierre Tardy wrote:
> Hi Neil,
>
> Such discussion are imho easiest to be in the PR or emails rather in 
> trac (we will be getting rid of trac soon anyway), but this is more 
> suitable in devel@
> [removing users at bb.net <mailto:users at bb.net> and switch to dev at bb.net 
> <mailto:dev at bb.net>]
>
>
> Indeed ForceScheduler is weird and is very different from the other 
> Schedulers even if it derives from BaseScheduler.
> The properties needs to be a list in order to have a defined order, so 
> that the UI can be specified exactly.
> This was designed at a time were OrderedDict was not available in all 
> the python we supported, and now it is legacy. :-(
> The properties from ForceScheduler are not really properties, they 
> need to be BaseParameters subclasses
>
> I am not sure exactly how we should fix this. Would need to look at 
> the code (hint: submit your current version :) )
>
> - Do no call super.checkConfig in ForceScheduler ?
> - Change forcescheduler to use OrderedDict instead, and make it 
> translate the properties list to an OrderedDict if it is a list (to 
> support legacy) ?
>
> I think later is the way to go eventually, but we might want to split 
> this task for later as I dont think this is a good idea to make your 
> PR too big.
>
> Pierre
>
> Le lun. 2 janv. 2017 à 21:45, Neil Gilmore <ngilmore at grammatech.com 
> <mailto:ngilmore at grammatech.com>> a écrit :
>
>     Hi all,
>
>     As always, thanks for the advice.
>
>     I think I've found an irregularity in ForceScheduler (naturally, this
>     would be netter in trac...). If nothing else, it renders some of the
>     documentation on Schedulers to be incorrect (I really can't say which
>     way this should fall). I've run into it while adding back
>     reconfiguration to the Force Scheduler.
>
>     It concerns the 'properties' argument. On the one hand, BaseScheduler
>     expects this to be a dictionary, and will in fact force it to an empty
>     dictionary if it isn't passed in. One the other, ForceScheduler
>     expects
>     it to be a list, and will force it to a list if it isn't passed in.
>     Additionally, at least in 0.9.2, ForceScheduler.__init__() does
>     not pass
>     its properties argument to BaseScheduler.__init__(). Now, it certainly
>     could be that ForceScheduler really doesn't implement the common
>     'properties', preferring its own instead.
>
>     But this leads to problems when we attempt to eliminate the __init__()
>     functions in favor of CheckConfig() and reconfigService(). We should
>     preserve the ForceScheduler.__init__() properties (the list kind) in
>     order to check them in checkConfig() and assign them in
>     reconfigService(). But BaseScheduler already has 'properties' (the
>     dictionary kind).
>
>     I think for now, I'll be introducing something like forceProperties in
>     __init__() which will end up passed in kwargs. This won't affect the
>     current interface to ForceScheduler, and will preserve the original
>     properties (the list kind) to be used later. checkConfig and
>     reconfigService will use it.
>
>     I'll also point out that ForceScheduler's check for name being a
>     unicode
>     string is incorrect. If passed a unicode string, it fails. The type
>     checked against is 'str', which is not 'unicode'.
>
>     So far, I can start a buildbot with any of the variety of
>     schedulers. I
>     do need to test reconfiguration to make sure everything actually
>     reconfigures.
>
>     Then I'll need to submit it and hope the devs like it.
>
>     Neil Gilmore
>     grammatech.com <http://grammatech.com>
>     _______________________________________________
>     users mailing list
>     users at buildbot.net <mailto:users at buildbot.net>
>     https://lists.buildbot.net/mailman/listinfo/users
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildbot.net/pipermail/devel/attachments/20170103/24c6dfd4/attachment.html>