Merge lp://staging/~julian-edwards/launchpad/hanging-builder-edit-bug-583905 into lp://staging/launchpad
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Merged at revision: | 10930 | ||||
| Proposed branch: | lp://staging/~julian-edwards/launchpad/hanging-builder-edit-bug-583905 | ||||
| Merge into: | lp://staging/launchpad | ||||
| Diff against target: |
76 lines (+60/-1) 2 files modified
lib/lp/soyuz/browser/builder.py (+11/-1) lib/lp/soyuz/browser/tests/test_builder_views.py (+49/-0) |
||||
| To merge this branch: | bzr merge lp://staging/~julian-edwards/launchpad/hanging-builder-edit-bug-583905 | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Michael Nelson (community) | code | Approve | |
|
Review via email:
|
|||
Description of the change
= Summary =
Stop timeouts on Builder:+edit
== Pre-implementation notes ==
It turns out that when the edit form is saved, the Snapshot stuff is kicked
off to grab the old object state to provide in event notifications. The
problem is that one of the properties it queries (is_available) is actually
some code that tries to send an xmlrpc request to the build slave itself.
Because this request is coming from a webapp, it hits a fairly solid firewall
until the connection times out.
== Implementation details ==
I've added notify_
stops it from taking snapshots of the object state. The comment in the code
explains the rationale, I've repeated it here:
# notify_modified is set False here because it uses
# lazr.lifecycle.
# before and after modification. This is dangerous for the
# builder model class because it causes some properties to be
# queried that try and communicate with the slave, which cannot
# be done from the webapp (it's generally firewalled). We could
# prevent snapshots for individual properties by defining the
# interface properties with doNotSnapshot() but this doesn't
# guard against future properties being created.
== Tests ==
I've added a new unit test for builder views. It stubs out the call to a
method slaveStatusSent
slave_called to True so we can assert whether it was called or not.
== Demo and Q/A ==
There's no way of QA-ing this since it's an artefact of the production
firewall set-up. The dogfood builders are not firewalled in the same way.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files:
lib/lp/
lib/lp/
16:40 < noodles775> bigjools: why do you remove the proxy on the test case's instance var, rather than just when you need to set something that you normally wouldn't be able to?
16:45 < noodles775> bigjools: also, you could use FakeMethod there if you wanted to... it would mean that you don't need to set slave_called on the builder, which would also mean you don't need removeSecurityProxy right?
16:46 < bigjools> noodles775: proxy is removed because...ummm I can't remember, let me try without it again since I've changed the code a bit since I added it (for good reason at the time)
16:47 < noodles775> I'm guessing it's because you're setting builder.
16:47 < bigjools> indeed
16:47 < bigjools> ah, it's because I can't override slaveStatusSentence otherwise
16:48 < bigjools> well, I suppose I could name it the same
16:49 < noodles775> Could you do the following in setUp: removeSecurityP
16:50 < noodles775> Then your test could just check self.builder.
16:50 < bigjools> yeah, let me try it, I wasn't aware of FakeMethod
16:51 < noodles775> OK, with that r=me