<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Pierre,<br>
<br>
Thanks for the reply.<br>
<br>
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).<br>
<br>
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.<br>
<br>
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.<br>
<br>
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:<br>
<br>
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. <br>
<br>
2. Add the checkConfig and reconfigService functions, but leave them
empty. This will at least ensure that I have those setup up
properly.<br>
<br>
3. Fill the functions.<br>
<br>
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.<br>
<br>
Neil Gilmore<br>
grammatech.com<br>
<br>
<div class="moz-cite-prefix">On 1/3/2017 2:42 AM, Pierre Tardy
wrote:<br>
</div>
<blockquote
cite="mid:CAJ+soVdPXKBNPgGZa8HG3edpaiaQTPPCKvPRnRPYL4dhCwV+sA@mail.gmail.com"
type="cite">
<div dir="ltr">Hi Neil,
<div><br>
<div>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@</div>
<div>[removing <a moz-do-not-send="true"
href="mailto:users@bb.net">users@bb.net</a> and switch to
<a moz-do-not-send="true" href="mailto:dev@bb.net">dev@bb.net</a>]</div>
<div><br>
</div>
<div><br>
</div>
<div>Indeed ForceScheduler is weird and is very different from
the other Schedulers even if it derives from BaseScheduler.</div>
<div>The properties needs to be a list in order to have a
defined order, so that the UI can be specified exactly.</div>
<div>This was designed at a time were OrderedDict was not
available in all the python we supported, and now it is
legacy. :-(</div>
<div>The properties from ForceScheduler are not really
properties, they need to be BaseParameters subclasses</div>
<div><br>
</div>
<div>I am not sure exactly how we should fix this. Would need
to look at the code (hint: submit your current version :) )</div>
<div><br>
</div>
<div>- Do no call super.checkConfig in ForceScheduler ?</div>
<div>- Change forcescheduler to use OrderedDict instead, and
make it translate the properties list to an OrderedDict if
it is a list (to support legacy) ?</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Pierre</div>
<br>
<div class="gmail_quote">
<div dir="ltr">Le lun. 2 janv. 2017 à 21:45, Neil Gilmore
<<a moz-do-not-send="true"
href="mailto:ngilmore@grammatech.com">ngilmore@grammatech.com</a>>
a écrit :<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Hi all,<br
class="gmail_msg">
<br class="gmail_msg">
As always, thanks for the advice.<br class="gmail_msg">
<br class="gmail_msg">
I think I've found an irregularity in ForceScheduler
(naturally, this<br class="gmail_msg">
would be netter in trac...). If nothing else, it renders
some of the<br class="gmail_msg">
documentation on Schedulers to be incorrect (I really
can't say which<br class="gmail_msg">
way this should fall). I've run into it while adding back<br
class="gmail_msg">
reconfiguration to the Force Scheduler.<br
class="gmail_msg">
<br class="gmail_msg">
It concerns the 'properties' argument. On the one hand,
BaseScheduler<br class="gmail_msg">
expects this to be a dictionary, and will in fact force it
to an empty<br class="gmail_msg">
dictionary if it isn't passed in. One the other,
ForceScheduler expects<br class="gmail_msg">
it to be a list, and will force it to a list if it isn't
passed in.<br class="gmail_msg">
Additionally, at least in 0.9.2, ForceScheduler.__init__()
does not pass<br class="gmail_msg">
its properties argument to BaseScheduler.__init__(). Now,
it certainly<br class="gmail_msg">
could be that ForceScheduler really doesn't implement the
common<br class="gmail_msg">
'properties', preferring its own instead.<br
class="gmail_msg">
<br class="gmail_msg">
But this leads to problems when we attempt to eliminate
the __init__()<br class="gmail_msg">
functions in favor of CheckConfig() and reconfigService().
We should<br class="gmail_msg">
preserve the ForceScheduler.__init__() properties (the
list kind) in<br class="gmail_msg">
order to check them in checkConfig() and assign them in<br
class="gmail_msg">
reconfigService(). But BaseScheduler already has
'properties' (the<br class="gmail_msg">
dictionary kind).<br class="gmail_msg">
<br class="gmail_msg">
I think for now, I'll be introducing something like
forceProperties in<br class="gmail_msg">
__init__() which will end up passed in kwargs. This won't
affect the<br class="gmail_msg">
current interface to ForceScheduler, and will preserve the
original<br class="gmail_msg">
properties (the list kind) to be used later. checkConfig
and<br class="gmail_msg">
reconfigService will use it.<br class="gmail_msg">
<br class="gmail_msg">
I'll also point out that ForceScheduler's check for name
being a unicode<br class="gmail_msg">
string is incorrect. If passed a unicode string, it fails.
The type<br class="gmail_msg">
checked against is 'str', which is not 'unicode'.<br
class="gmail_msg">
<br class="gmail_msg">
So far, I can start a buildbot with any of the variety of
schedulers. I<br class="gmail_msg">
do need to test reconfiguration to make sure everything
actually<br class="gmail_msg">
reconfigures.<br class="gmail_msg">
<br class="gmail_msg">
Then I'll need to submit it and hope the devs like it.<br
class="gmail_msg">
<br class="gmail_msg">
Neil Gilmore<br class="gmail_msg">
<a moz-do-not-send="true" href="http://grammatech.com"
rel="noreferrer" class="gmail_msg" target="_blank">grammatech.com</a><br
class="gmail_msg">
_______________________________________________<br
class="gmail_msg">
users mailing list<br class="gmail_msg">
<a moz-do-not-send="true" href="mailto:users@buildbot.net"
class="gmail_msg" target="_blank">users@buildbot.net</a><br
class="gmail_msg">
<a moz-do-not-send="true"
href="https://lists.buildbot.net/mailman/listinfo/users"
rel="noreferrer" class="gmail_msg" target="_blank">https://lists.buildbot.net/mailman/listinfo/users</a><br
class="gmail_msg">
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>