Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-739986 into
lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #739986 in Launchpad itself: "LaunchpadScript fails to install feature
controller"
https://bugs.launchpad.net/launchpad/+bug/739986
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-739986/+merge/54299
= Summary =
The script feature controller wasn't being set up due to a last-minute API
change.
make_script_feature_controller installed the feature controller it created,
rather than returning it (it returned None). But the script code tried to
install the feature controller that make_script_feature_controller returned, so
it ended up installing None.
== Proposed fix ==
Return a feature controller from make_script_feature_controller without
installing it. This makes the existing usage in LaunchpadCronScript.run
correct.
== Pre-implementation notes ==
I wholeheartedly apologize for the pain this caused StevenK.
== Implementation details ==
The way LaunchpadScript installs its feature controller had changed a bit at
the last moment. It used to do that in the constructor, but because some
scripts' "name" property isn't callable yet at that point, I've made it
internal to "run." Which probably makes more sense anyway, except that I
neglected to call "run" in the relevant test.
In the script test I added a check that the observed feature controller really
is a FeatureController. That's not necessary to detect the actual bug (the
test will fail before then, once we remember to call run()) but it would have
helped spot the mistake of not calling run.
== Tests ==
{{{
./bin/test -vvc lp.services.scripts.tests.test_feature_controller
}}}
== Demo and Q/A ==
Steve will be able to observe the difference in his ongoing work.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/services/scripts/base.py
lib/lp/services/features/__init__.py
lib/lp/services/scripts/tests/test_feature_controller.py
--
https://code.launchpad.net/~jtv/launchpad/bug-739986/+merge/54299
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~jtv/launchpad/bug-739986 into lp:launchpad.
=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py 2011-03-16 07:56:02 +0000
+++ lib/lp/services/features/__init__.py 2011-03-22 06:47:10 +0000
@@ -211,13 +211,15 @@
def make_script_feature_controller(script_name):
- """Create and install a `FeatureController` for the named script."""
+ """Create a `FeatureController` for the named script.
+
+ You can then install this feature controller using
+ `install_feature_controller`.
+ """
# Avoid circular import.
from lp.services.features.flags import FeatureController
from lp.services.features.rulesource import StormFeatureRuleSource
from lp.services.features.scopes import ScopesForScript
- install_feature_controller(
- FeatureController(
- ScopesForScript(script_name).lookup,
- StormFeatureRuleSource()))
+ return FeatureController(
+ ScopesForScript(script_name).lookup, StormFeatureRuleSource())
=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py 2011-03-21 04:50:12 +0000
+++ lib/lp/services/scripts/base.py 2011-03-22 06:47:10 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -40,7 +40,6 @@
)
from canonical.lp import initZopeless
from lp.services.features import (
- getFeatureFlag,
get_relevant_feature_controller,
install_feature_controller,
make_script_feature_controller,
=== modified file 'lib/lp/services/scripts/tests/test_feature_controller.py'
--- lib/lp/services/scripts/tests/test_feature_controller.py 2011-03-16 09:14:41 +0000
+++ lib/lp/services/scripts/tests/test_feature_controller.py 2011-03-22 06:47:10 +0000
@@ -10,8 +10,10 @@
get_relevant_feature_controller,
install_feature_controller,
)
-from lp.services.features.flags import NullFeatureController
-from lp.services.features.testing import FeatureFixture
+from lp.services.features.flags import (
+ FeatureController,
+ NullFeatureController,
+ )
from lp.services.scripts.base import LaunchpadScript
from lp.testing import TestCase
from lp.testing.fakemethod import FakeMethod
@@ -48,10 +50,12 @@
def test_script_installs_script_feature_controller(self):
script = FakeScript(name="bongo")
- script_feature_controller = get_relevant_feature_controller()
+ script.run()
self.assertNotEqual(
self.original_controller, script.observed_feature_controller)
self.assertNotEqual(None, script.observed_feature_controller)
+ self.assertIsInstance(
+ script.observed_feature_controller, FeatureController)
def test_script_restores_feature_controller(self):
previous_controller = NullFeatureController()
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
