launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

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()