Slightly decoupled ForceScheduler #385

Merged
merged 10 commits into from May 5, 2012

Projects

None yet

4 participants

@Jc2k
Collaborator
Jc2k commented Apr 20, 2012

The main aim of this change was to allow me to subclass ForceScheduler and run additional "smarts" before a build was queued. This accomplishes that and takes a step towards a more decoupled ForcedScheduler based on discussion in IRC.

For me, I can subclass and extend gatherPropertiesAndChanges or I can use a Parameter class (now that they can use deferreds).

For every one else, the scheduler no longer requires a request object. The IRC bot could now be refactored to do something like:

sched.force("Jc2k", "builder_name", someprop="hello", somepro="woo", ...)

And the defaults and validation set on the ForceScheduler would be used.

Ideally I would change the builder_name and owner params around - they are only that way round to match current implementation.

The logic for owner is wrong too - thoughts on that would be appreciated.

Tests pass, however.

@tomprince tomprince and 1 other commented on an outdated diff Apr 20, 2012
master/buildbot/schedulers/forcesched.py
- repository=repository,changeids=changeids)
- return sourcestampsetid
-
- d.addCallback(add_master_with_setid)
- def got_setid(sourcestampsetid):
- return self.addBuildsetForSourceStamp(builderNames=[builder_name],
- setid=sourcestampsetid, reason=r,
- properties=real_properties)
- d.addCallback(got_setid)
- return d
+ # everything is validated, we can create our source stamp, and buildrequest
+ res = yield self.schedule(builder_name, branch, revision, repository, project, changeids, properties, r)
+ defer.returnValue(res)
+
+ @defer.inlineCallbacks
+ def schedule(self, builder, branch, revision, repository, project, changeids, properties, reason):
@tomprince
tomprince Apr 20, 2012 Buildbot member

This doesn't look specific to the force scheduler at all. It seems like this could profitably be moved to the base class. Perhaps addBuildsetForSourceStampSetList or something? (That name seem unnecessarily long.

@tomprince
tomprince Apr 20, 2012 Buildbot member

@djmitche: Thoughts on the name?

@djmitche
djmitche Apr 21, 2012 Buildbot member

How is this different from the other addBuildsetFor.. methods? It's hard to tell if it's a wrapper around one of those, or more generally useful.

@tomprince
tomprince Apr 21, 2012 Buildbot member

All the current helpers take a ssetid. This takes a takes the date to create one.

@djmitche
djmitche Apr 26, 2012 Buildbot member

how about addBuildSetForSourceStampDetails?

@tomprince tomprince and 1 other commented on an outdated diff Apr 20, 2012
master/buildbot/schedulers/forcesched.py
- master.db.sourcestamps.addSourceStamp(
- sourcestampsetid = sourcestampsetid,
- branch=branch,
- revision=revision, project=project,
- repository=repository,changeids=changeids)
- return sourcestampsetid
-
- d.addCallback(add_master_with_setid)
- def got_setid(sourcestampsetid):
- return self.addBuildsetForSourceStamp(builderNames=[builder_name],
- setid=sourcestampsetid, reason=r,
- properties=real_properties)
- d.addCallback(got_setid)
- return d
+ # everything is validated, we can create our source stamp, and buildrequest
+ res = yield self.schedule(builder_name, branch, revision, repository, project, changeids, properties, r)
@tomprince
tomprince Apr 20, 2012 Buildbot member

This looks like the only deferred in this function, so it would probably make sense to just return it, rather than decorating the function with inlineCallbacks.

@Jc2k
Jc2k Apr 20, 2012 collaborator

There's a yield further up in the call.

@tomprince tomprince and 1 other commented on an outdated diff Apr 20, 2012
master/buildbot/schedulers/forcesched.py
def forceWithWebRequest(self, owner, builder_name, req):
"""Called by the web UI.
Authentication is already done, thus owner is passed as argument
+ """
+ args = {}
+ # damn html's ungeneric checkbox implementation...
@tomprince
tomprince Apr 20, 2012 Buildbot member

I'm very tempted to say that this function belongs in the web status. Particularly with this special case.

@Jc2k
Jc2k Apr 20, 2012 collaborator

Done.

@tomprince tomprince and 1 other commented on an outdated diff Apr 20, 2012
master/buildbot/schedulers/forcesched.py
@@ -45,8 +41,8 @@ def __init__(self, name, label=None, regex=None, **kw):
# all other properties are generically passed via **kw
self.__dict__.update(kw)
- def update_from_post(self, master, properties, changes, req):
- args = req.args.get(self.name, [])
+ def get_from_post(self, kwargs):
@tomprince
tomprince Apr 20, 2012 Buildbot member

These functions should probably be changed to not refer to post any. (And changed to camelCase, since if they are changing anyway).

@Jc2k
Jc2k Apr 20, 2012 collaborator

Done.

@tomprince tomprince and 1 other commented on an outdated diff Apr 20, 2012
master/buildbot/test/unit/test_schedulers_forcesched.py
def do_ParameterTest(self, value, expect, klass, owner='user', req=None,
**kwargs):
sched = self.makeScheduler(properties=[klass(name="p1",**kwargs)])
if not req:
req = self.makeRequest(p1=value,reason='because')
try:
- d = sched.forceWithWebRequest(owner, 'a', req)
+ bsid, brids = yield sched.forceWithWebRequest(owner, 'a', req)
@tomprince
tomprince Apr 20, 2012 Buildbot member

This should probably be changed to test .force instead.

@Jc2k
Jc2k Apr 20, 2012 collaborator

Done.

@tomprince tomprince commented on an outdated diff Apr 20, 2012
master/buildbot/test/unit/test_schedulers_forcesched.py
- ('owner', ('user', 'Force Build Form')),
- ('p1', (expect, 'Force Build Form')),
- ('reason', ('because', 'Force Build Form')),
- ('scheduler', ('testsched', 'Scheduler')),
- ],
- sourcestampsetid=100),
- {"":
- dict(branch="", revision="", repository="", codebase='',
- project="", sourcestampsetid=100)
- })
- d.addCallback(check)
- return d
+ defer.returnValue(None) # success
+ self.db.buildsets.assertBuildset\
+ (bsid,
+ dict(reason="The web-page 'force build' button was pressed "
@tomprince
tomprince Apr 20, 2012 Buildbot member

The reason should perhaps be updated to not refer to the web page (or changed so that that is specified from the web code.

@tomprince
Member

Thanks for working on this.

It looks like the first two commits could be squashed or refactored, so that the first isn't adding dead code.

@Jc2k
Collaborator
Jc2k commented Apr 20, 2012

Have squished as requested.

@Jc2k
Collaborator
Jc2k commented Apr 20, 2012

Squish went wrong - fixed.

Pick a name for the schedule function and i'll move it :)

The reason property: I would dearly like to kill the "User press a button on a form" reason and just use whatever the user specified directly.

@djmitche djmitche commented on an outdated diff Apr 22, 2012
master/buildbot/schedulers/forcesched.py
@@ -16,16 +16,15 @@
import traceback
import re
from twisted.internet import defer
-try:
- import email.utils as email_utils
- email_utils = email_utils
-except ImportError:
- # Python-2.4 capitalization
- import email.Utils as email_utils
+import email.utils as email_utils
+email_utils = email_utils
@djmitche
djmitche Apr 22, 2012 Buildbot member

This line shouldn't be necessary anymore - it just keeps pyflakes happy when using the try .. except ImportError form.

@Jc2k
Collaborator
Jc2k commented Apr 25, 2012

Rebased as conflicting changes had landed in master.

Fixed most things.

Reason text is now not web specific. I tried to break it out into the parameter but there are actually 2 reasons, one reason string might be "force build", the other would be "The force build button was pressed by freddy: force build".

I have not renamed the helper function as a suitable name has not been suggested.

@djmitche
Member
djmitche commented May 1, 2012

How's addBuildSetForSourceStampDetails sound? Is the rename of that function (and move to base.py) the last step here?

@jaredgrubb jaredgrubb commented on the diff May 5, 2012
master/buildbot/schedulers/forcesched.py
@@ -236,69 +241,79 @@ def startService(self):
def stopService(self):
pass
- def forceWithWebRequest(self, owner, builder_name, req):
- """Called by the web UI.
- Authentication is already done, thus owner is passed as argument
+ @defer.inlineCallbacks
+ def gatherPropertiesAndChanges(self, **kwargs):
+ properties = {}
+ changeids = []
+
+ for param in self.forcedProperties:
+ yield defer.maybeDeferred(param.updateFromKwargs, self.master, properties, changeids, kwargs)
+
+ changeids = map(lambda a: type(a)==int and a or a.number, changeids)
@jaredgrubb
jaredgrubb May 5, 2012 Buildbot member

isinstance(a,int) is better than type(a)==int

@jaredgrubb jaredgrubb commented on the diff May 5, 2012
master/buildbot/schedulers/forcesched.py
We check the parameters, and launch the build, if everything is correct
"""
if not builder_name in self.builderNames:
# in the case of buildAll, this method will be called several times
# for all the builders
# we just do nothing on a builder that is not in our builderNames
- return defer.succeed(None)
- master = self.master
- properties = {}
- changeids = []
+ defer.returnValue(None)
+
@jaredgrubb
jaredgrubb May 5, 2012 Buildbot member

Sylistically, I think I remember Dustin&Tom saying to always add "return" on a line after "defer.returnValue" just for cliarty that the function is over ... its a little more readable to people not 100% down with inlineCallbacks rules

@Jc2k
Jc2k May 5, 2012 collaborator

@djmitche / @tomprince is this still in effect?

I think it's more confusing to people not 100% down with inlineCallbacks - they might think that defer.returnValue is just setting a global and that return still does the return!

@djmitche
djmitche May 5, 2012 Buildbot member

The difference is in how you read "returnValue" - is that "set the return value" (no) or "return this value now" (yes). I actually got this wrong in the docs (http://buildbot.net/buildbot/docs/latest/developer/style.html#inlinecallbacks, although I'll fix that now).

At any rate, those docs do say to use it, and IMHO over-communication is better, so please add the return statements.

@djmitche djmitche added a commit that referenced this pull request May 5, 2012
@djmitche djmitche fix docs about as mentioned in pull #385 6473eb7
@djmitche djmitche merged commit 50fa054 into buildbot:master May 5, 2012
@djmitche djmitche added a commit to djmitche/buildbot that referenced this pull request May 6, 2012
@djmitche djmitche Merge branch 'master' into nine
* master:
  fix pyflakes
  clean up returnValue calls
  fix docs about  as mentioned in pull #385
  fix busted coverage generation
  Move method to base scheduler
  Update docs and tests based on feedback
  Make sure build steps can raise BuildStepFailed
  Should use username not owner
  Change the reason text to not be web specific
  Move webstatus specific code to b.s.w.builder
  Use a specific error message instead of ValueError
  get_from_post -> getFromKwargs
  Remove Python2.4ism
  Refactor so that ForceScheduler isn't as tightly coupled to HTML
  Use a dict() for r.args (the real one is a dict anyway)
  Break out property gathering and scheduling into seperate functions
34279c4
@tomprince tomprince added a commit to tomprince/buildbot that referenced this pull request Jan 25, 2013
@tomprince tomprince svnpoller: Don't fail when revisions contain changes outside our prefix.
When getting the logs, revisions that don't affect the SVNPoller.svnurl path
are already being filtered out by svn.  Since the poller doesn't even consider
them, I would expect it not to consider unrelated paths that come with the logs
and just filter them out.

Fixes #385.
1732adc
@thinkski thinkski pushed a commit to kuna-systems/buildbot that referenced this pull request Sep 10, 2014
@djmitche djmitche fix docs about as mentioned in pull #385 d690be2
@thinkski thinkski pushed a commit to kuna-systems/buildbot that referenced this pull request Sep 10, 2014
@tomprince tomprince svnpoller: Don't fail when revisions contain changes outside our prefix.
When getting the logs, revisions that don't affect the SVNPoller.svnurl path
are already being filtered out by svn.  Since the poller doesn't even consider
them, I would expect it not to consider unrelated paths that come with the logs
and just filter them out.

Fixes #385.
e788243
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment