Merge lp:~jtv/launchpad/bug-735319 into lp:launchpad

[Jeroen T. Vermeulen]

[r=mbp][bug=735319] Let scripts access feature flags.

= Summary =

Bug 735319: scripts can't read feature flags. They get None values instead, which are conventionally interpreted as "false" in the case of boolean flags.

This leads to surprising bugs.

== Proposed fix ==

Set up a feature controller in LaunchpadScript.

== Pre-implementation notes ==

According to lifeless, there is no particular plan behind the way various pieces of the code get or set the applicable feature controller by reading or assigning lp.services.features.per_thread.features, or using getattr/setattr on it, so I cleaned that up. It's strictly single-setter/single-getter now.

== Implementation details ==

ScopesFromRequest is mostly independent of the webapp. Most of its work goes into finding a scope handler that matches a rule's sope spec, and then feeding the scope spec to that handler. I abstracted that out into a base class MultiScopeHandler, with separate pieces of code for "find matching scope handler" and "look up scope in matching handlers."

ScopesFromRequest now inherits from that new class, and a new sibling ScopesForSCript does a similar job for scripts. These are really just factories for the base class now, but I couldn't be bothered to make the name change. A script is identified by its "name" property.

LaunchpadScript now sets up a feature handler, which means that all our scripts will magically be able to read feature flags now.

You also get a few free bonuses with this branch:

Cleanup. The applicable feature controller is set as lp.services.features.per_thread.features. Many different modules read, assign, getattr, and/or setattr this variable, even though we aren't even quite sure yet where it should live. So I concentrated all access in a single setter, plus a getter that we had already. The per_thread variable is no longer exposed.

Script scope. You can now write rules that set feature flags in the scope of a particular script, using "script:<script-name>."

Override switch. Every LaunchpadScript now has a "script_disabled" flag that stops the script from executing. This should be easier and faster to manage than the existing lazr configuration or messing with cron tabs. It also puts some control closer to the engineering team.

I hope somebody will feel inspired to add a "verbosity" feature flag for the scripts as well. It'd be nice to minimize the menial jobs we bother the LOSAs for.

== Tests ==

{{{
./bin/test -vvc lp.services.features
}}}

== Demo and Q/A ==

Feature flags in the web app should still work. But also, we'll be able to disable a script of our choice (cron or plain) and it will exit quickly (well, not counting the tedious ZCML processing on startup) with a message about it being disabled by feature flag.

= Launchpad lint =

I cleaned up a lot, and in fact that's probably the first thing you'll see in the diff.

I didn't also want to change all the section headings though: too much work, too much diff, and I'd probably use the wrong ReST underlines anyway.

Then there's the unused imports. I ought to have looked up if any of those could be eliminated, but the branch was big enough already.

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/bugs/browser/tests/test_bugattachment_file_access.py
  lib/canonical/launchpad/webapp/publication.py
  lib/lp/services/scripts/tests/test_feature_controller.py
  lib/lp/services/features/tests/test_flags.py
  lib/canonical/launchpad/doc/librarian.txt
  lib/canonical/launchpad/doc/timeout.txt
  lib/lp/testing/__init__.py
  lib/lp/services/features/webapp.py
  lib/lp/services/features/scopes.py
  lib/lp/services/features/testing.py
  lib/lp/services/features/tests/test_scopes.py
  lib/lp/registry/browser/tests/test_series_views.py

./lib/canonical/launchpad/doc/librarian.txt
       1: narrative uses a moin header.
      13: narrative uses a moin header.
      25: narrative uses a moin header.
      30: narrative uses a moin header.
     198: narrative uses a moin header.
     291: narrative uses a moin header.
     437: narrative uses a moin header.
     505: narrative uses a moin header.
     524: narrative uses a moin header.
     565: narrative uses a moin header.
     670: narrative uses a moin header.
     786: narrative uses a moin header.
     799: narrative uses a moin header.
     876: narrative uses a moin header.
./lib/canonical/launchpad/doc/timeout.txt
       1: narrative uses a moin header.
      81: narrative uses a moin header.
     117: narrative uses a moin header.
./lib/lp/testing/__init__.py
     135: 'anonymous_logged_in' imported but unused
     135: 'with_anonymous_login' imported but unused
     154: 'launchpadlib_for' imported but unused
     154: 'launchpadlib_credentials_for' imported but unused
     135: 'with_person_logged_in' imported but unused
     135: 'person_logged_in' imported but unused
     154: 'oauth_access_token_for' imported but unused
     135: 'login_celebrity' imported but unused
     135: 'with_celebrity_logged_in' imported but unused
     153: 'test_tales' imported but unused
     135: 'celebrity_logged_in' imported but unused
     135: 'run_with_login' imported but unused
     135: 'is_logged_in' imported but unused
     135: 'login_team' imported but unused
     135: 'login_person' imported but unused
     135: 'login_as' imported but unused

Jeroen

[Robert Collins (lifeless)]

small notes - not a code review.

We already have a 'don't start script' feature that doesn't require
editing cron scripts - and *unlike* one based on feature flags, also
does not require db access (which is important for db deploys). Please
delete that part of your patch, because I am confident we'll have
unintended downtime if we have a db based duplicate facility.

Only LOSAs can set feature flags, so setting e.g. verbosity via a
feature flag is still a losa interrupt. It may be better than editing
cronscripts [or not - I haven't thought about it] : but losa
interruptedness is not a differentiating factor.

The rest sounds lovely.

Cheers,
Rob

[Martin Pool (mbp)]

That sounds good to me too. Please update
<https://dev.launchpad.net/FeatureFlags> too (maybe after it lands).

Martin

[Jeroen T. Vermeulen (jtv)]

Thanks for looking into this, and especially so soon.

We do have at least one person who can set feature flags: Stuart.

To me, setting a feature flag actually makes more sense than editing a config file, as does the regularity of being able to do it uniformly to any script by name. I do agree that the duplication is bad but given the ability to have either, maybe it's the config one that ought to go.

[Jeroen T. Vermeulen (jtv)]

For the record, we just talked about this on IRC and Robert made clear that:

(1) policy is for LOSAs to edit feature flags anyway, and
(2) we can't disable scripts by database during schema rollouts.

So I've taken out the disable switch for scripts.

[Martin Pool (mbp)]

I've read the code (a bit quickly because it's late) and it looks pretty good to me.

I wonder if we could/should actually just remove NullFeatureController.

review: Approve (code*)

=== modified file 'lib/canonical/launchpad/doc/librarian.txt'
--- lib/canonical/launchpad/doc/librarian.txt	2011-01-19 14:26:33 +0000
+++ lib/canonical/launchpad/doc/librarian.txt	2011-03-21 05:22:30 +0000
@@ -1,18 +1,19 @@
 = Librarian Access =
 
 The librarian is a file storage service for launchpad. Conceptually similar to
-other file storage API's like S3, it is used to store binary or large content -
-bug attachments, package builds, images and so on.
+other file storage API's like S3, it is used to store binary or large
+content - bug attachments, package builds, images and so on.
 
 Content in the librarian can be exposed at different urls. To expose some
-content use a LibraryFileAlias. Private content is supported as well - for that
-tokens are added to permit access for a limited time by a client - each time a
-client attempts to dereference a private LibraryFileAlias a token is emitted.
+content use a LibraryFileAlias. Private content is supported as well - for
+that tokens are added to permit access for a limited time by a client - each
+time a client attempts to dereference a private LibraryFileAlias a token is
+emitted.
 
 = Deployment notes =
 
-(These may seem a bit out of place - they are, but they need to be written down
-somewhere, and the deployment choices inform the implementation choices).
+(These may seem a bit out of place - they are, but they need to be written
+down somewhere, and the deployment choices inform the implementation choices).
 
 The basics are simple: The librarian talks to clients. However restricted file
 access makes things a little more complex. As the librarian itself doesn't do
@@ -23,13 +24,14 @@
 
 == setUp ==
 
-   >>> from canonical.database.sqlbase import session_store
-   >>> from canonical.launchpad.database.librarian import TimeLimitedToken
+    >>> from canonical.database.sqlbase import session_store
+    >>> from canonical.launchpad.database.librarian import TimeLimitedToken
 
 == High Level ==
 
     >>> from StringIO import StringIO
-    >>> from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
+    >>> from canonical.launchpad.interfaces.librarian import (
+    ...     ILibraryFileAliasSet)
     >>> data = 'This is some data'
 
 We can create LibraryFileAliases using the ILibraryFileAliasSet utility.
@@ -460,7 +462,8 @@
 
 Filenames with spaces in them work.
 
-    >>> aid = client.addFile('hot dog', len(data), StringIO(data), 'text/plain')
+    >>> aid = client.addFile(
+    ...     'hot dog', len(data), StringIO(data), 'text/plain')
     >>> transaction.commit()
     >>> f = client.getFileByAlias(aid)
     >>> f.read()
@@ -507,8 +510,8 @@
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> req = LaunchpadTestRequest()
     >>> alias = lfas.create(
-    ...     'text2.txt', len(data), StringIO(data), 'text/plain', NEVER_EXPIRES
-    ...     )
+    ...     'text2.txt', len(data), StringIO(data), 'text/plain',
+    ...     NEVER_EXPIRES)
     >>> transaction.commit()
     >>> lfa_view = getMultiAdapter((alias,req), name='+index')
     >>> lfa_view.initialize()
@@ -557,7 +560,7 @@
 
 Clear out existing tokens.
 
-   >>> _ = session_store().find(TimeLimitedToken).remove()
+    >>> _ = session_store().find(TimeLimitedToken).remove()
 
 == RedirectPerhapsWithTokenLibraryFileAliasView ==
 
@@ -566,8 +569,8 @@
 is interleaved: zope's registration system is too static to permit doing it
 cleanly with separate classes.
 
-To enable the behaviour we want to test while its controlled by a feature flag,
-we must enable a feature flag for it:
+To enable the behaviour we want to test while its controlled by a feature
+flag, we must enable a feature flag for it:
 
     >>> from lp.services.features.model import FeatureFlag, getFeatureStore
     >>> ignore = getFeatureStore().add(FeatureFlag(
@@ -589,11 +592,11 @@
 
 XXX bug=631884 we have to establish the flags object manually for testing.
 
-    >>> from lp.services.features import per_thread
+    >>> from lp.services.features import install_feature_controller
     >>> from lp.services.features.flags import FeatureController
     >>> from lp.services.features.webapp import ScopesFromRequest
-    >>> per_thread.features = FeatureController(
-    ...     ScopesFromRequest(empty_request).lookup)
+    >>> install_feature_controller(FeatureController(
+    ...     ScopesFromRequest(empty_request).lookup))
 
     >>> restricted_view = RedirectPerhapsWithTokenLibraryFileAliasView(
     ...     restricted_file, empty_request)
@@ -616,7 +619,7 @@
     >>> view.target.endswith(session_store().find(TimeLimitedToken,
     ...     path=private_path).any().token)
     True
-    
+
 The domain for the target starts with the id of the LibraryFileAlias.
 
     >>> view.target.startswith('https://i%d.restricted.' % restricted_file.id)
@@ -657,12 +660,12 @@
     ...     FeatureFlag.flag==u'publicrestrictedlibrarian').remove()
     >>> transaction.commit()
 
-Because the flags code aggressively caches, we have to do a small dance to 
+Because the flags code aggressively caches, we have to do a small dance to
 convince it a new request has started too.
 
     >>> empty_request = LaunchpadTestRequest()
-    >>> per_thread.features = FeatureController(
-    ...     ScopesFromRequest(empty_request).lookup)
+    >>> install_feature_controller(FeatureController(
+    ...     ScopesFromRequest(empty_request).lookup))
 
 == StreamOrRedirectLibraryFileAliasView ==
 
 
=== modified file 'lib/canonical/launchpad/doc/timeout.txt'
--- lib/canonical/launchpad/doc/timeout.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/timeout.txt	2011-03-21 05:22:30 +0000
@@ -81,14 +81,15 @@
 = Overriding hard timeouts via FeatureFlags =
 
 It's possible to use FeatureFlags to control the hard timeout. This is used to
-deal with pages that suddenly start performing badly, which are being optimised
-but should not hold back the overall timeout decrease, or for which there are
-only a few specific users and we are willing to have them run for longer
-periods. For more information on feature flags see lp.services.features.
+deal with pages that suddenly start performing badly, which are being
+optimised but should not hold back the overall timeout decrease, or for which
+there are only a few specific users and we are willing to have them run for
+longer periods. For more information on feature flags see
+lp.services.features.
 
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> from lp.services.features.model import FeatureFlag, getFeatureStore
-    >>> from lp.services.features import per_thread
+    >>> from lp.services.features import install_feature_controller
     >>> from lp.services.features.flags import FeatureController
     >>> from lp.services.features.webapp import ScopesFromRequest
 
@@ -99,8 +100,8 @@
     ... db_statement_timeout = 10000'''))
 
     >>> empty_request = LaunchpadTestRequest()
-    >>> per_thread.features = FeatureController(
-    ...     ScopesFromRequest(empty_request).lookup)
+    >>> install_feature_controller(FeatureController(
+    ...     ScopesFromRequest(empty_request).lookup))
     >>> ignore = getFeatureStore().add(FeatureFlag(
     ...     scope=u'default', flag=u'hard_timeout', value=u'20000',
     ...     priority=1))
 
=== modified file 'lib/canonical/launchpad/webapp/publication.py'
--- lib/canonical/launchpad/webapp/publication.py	2011-02-25 18:06:16 +0000
+++ lib/canonical/launchpad/webapp/publication.py	2011-03-21 05:22:30 +0000
@@ -464,7 +464,7 @@
             'RootObject:OpStats', 'RootObject:+opstats',
             'RootObject:+haproxy'):
             request.features = NullFeatureController()
-            features.per_thread.features = request.features
+            features.install_feature_controller(request.features)
 
         # Calculate the hard timeout: needed because featureflags can be used
         # to control the hard timeout, and they trigger DB access, but our
 
=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2011-03-15 11:18:01 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2011-03-21 05:22:30 +0000
@@ -108,9 +108,9 @@
         view = navigation.publishTraverse(request, '+files')
         # XXX Ensure the feature will be off - everything is off with
         # NullFeatureController. bug=631884
-        lp.services.features.per_thread.features = NullFeatureController()
-        self.addCleanup(
-            setattr, lp.services.features.per_thread, 'features', None)
+        lp.services.features.install_feature_controller(
+            NullFeatureController())
+        self.addCleanup(lp.services.features.install_feature_controller, None)
         next_view, traversal_path = view.browserDefault(request)
         self.assertEqual(view, next_view)
         file_ = next_view()
@@ -152,9 +152,9 @@
         view = navigation.publishTraverse(request, '+files')
         # XXX Ensure the feature will be off - everything is off with
         # NullFeatureController. bug=631884
-        lp.services.features.per_thread.features = NullFeatureController()
-        self.addCleanup(
-            setattr, lp.services.features.per_thread, 'features', None)
+        lp.services.features.install_feature_controller(
+            NullFeatureController())
+        self.addCleanup(lp.services.features.install_feature_controller, None)
         next_view, traversal_path = view.browserDefault(request)
         self.assertIsInstance(
             next_view, SafeStreamOrRedirectLibraryFileAliasView)
 
=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py	2011-03-07 19:53:13 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py	2011-03-21 05:22:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -31,7 +31,7 @@
     )
 from lp.services.features import (
     getFeatureFlag,
-    per_thread,
+    install_feature_controller,
     )
 from lp.testing import (
     TestCaseWithFactory,
@@ -81,11 +81,8 @@
     # features.
     def in_scope(value):
         return True
-    per_thread.features = FeatureController(in_scope)
-
-    def reset_per_thread_features():
-        per_thread.features = None
-    test_case.addCleanup(reset_per_thread_features)
+    install_feature_controller(FeatureController(in_scope))
+    test_case.addCleanup(install_feature_controller, None)
 
 
 class DistroSeriesLocalPackageDiffsPageTestCase(TestCaseWithFactory):
 
=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py	2010-11-02 17:47:05 +0000
+++ lib/lp/services/features/__init__.py	2011-03-21 05:22:30 +0000
@@ -173,7 +173,8 @@
 __all__ = [
     'get_relevant_feature_controller',
     'getFeatureFlag',
-    'per_thread',
+    'install_feature_controller',
+    'make_script_feature_controller',
     ]
 
 
@@ -185,8 +186,13 @@
 """
 
 
+def install_feature_controller(controller):
+    """Install a `FeatureController` on this thread."""
+    per_thread.features = controller
+
+
 def get_relevant_feature_controller():
-    """Get a FeatureController for this thread."""
+    """Get a `FeatureController` for this thread."""
 
     # The noncommittal name "relevant" is because this function may change to
     # look things up from the current request or some other mechanism in
@@ -202,3 +208,16 @@
     if features is None:
         return None
     return features.getFlag(flag)
+
+
+def make_script_feature_controller(script_name):
+    """Create and install a `FeatureController` for the named script."""
+    # 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()))
 
=== modified file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	2011-01-26 22:07:08 +0000
+++ lib/lp/services/features/scopes.py	2011-03-21 05:22:30 +0000
@@ -5,6 +5,7 @@
 
 __all__ = [
     'HANDLERS',
+    'ScopesForScript',
     'ScopesFromRequest',
     'undocumented_scopes',
     ]
@@ -142,18 +143,51 @@
         return False
 
 
+class ScriptScope(BaseScope):
+    """Matches the name of the currently running script.
+
+    For example, the scope script:embroider is active in a script called
+    "embroider."
+    """
+
+    pattern = r'script:'
+
+    def __init__(self, script_name):
+        super(ScriptScope, self).__init__(None)
+        self.script_scope = self.pattern + script_name
+
+    def lookup(self, scope_name):
+        """Match the running script as a scope."""
+        return scope_name == self.script_scope
+
+
+# Handlers for the scopes that may occur in the webapp.
+WEBAPP_SCOPE_HANDLERS = [DefaultScope, PageScope, TeamScope, ServerScope]
+
+
+# Handlers for the scopes that may occur in scripts.
+SCRIPT_SCOPE_HANDLERS = [DefaultScope, ScriptScope]
+
+
 # These are the handlers for all of the allowable scopes.  Any new scope will
 # need a scope handler and that scope handler has to be added to this list.
 # See BaseScope for hints as to what a scope handler should look like.
-HANDLERS = [DefaultScope, PageScope, TeamScope, ServerScope]
-
-
-class ScopesFromRequest():
-    """Identify feature scopes based on request state."""
-
-    def __init__(self, request):
+HANDLERS = set(WEBAPP_SCOPE_HANDLERS + SCRIPT_SCOPE_HANDLERS)
+
+
+class MultiScopeHandler():
+    """A scope handler that combines multiple `BaseScope`s."""
+
+    def __init__(self, request, scopes):
         self.request = request
-        self.handlers = [f(request) for f in HANDLERS]
+        self.handlers = scopes
+
+    def _findMatchingHandlers(self, scope_name):
+        """Find any handlers that match `scope_name`."""
+        return [
+            handler
+            for handler in self.handlers
+                if handler.compiled_pattern.match(scope_name)]
 
     def lookup(self, scope_name):
         """Determine if scope_name applies to this request.
@@ -163,15 +197,29 @@
         the current request or the handlers are exhuasted, in which case the
         scope name is not a match.
         """
-        found_a_handler = False
-        for handler in self.handlers:
-            if handler.compiled_pattern.match(scope_name):
-                found_a_handler = True
-                if handler.lookup(scope_name):
-                    return True
+        matching_handlers = self._findMatchingHandlers(scope_name)
+        for handler in matching_handlers:
+            if handler.lookup(scope_name):
+                return True
 
-        # If we didn't find at least one matching handler, then the requested
-        # scope is unknown and we want to record the scope for the +flag-info page to display.
-        if not found_a_handler:
+        # If we didn't find at least one matching handler, then the
+        # requested scope is unknown and we want to record the scope for
+        # the +flag-info page to display.
+        if len(matching_handlers) == 0:
             undocumented_scopes.add(scope_name)
 
+
+class ScopesFromRequest(MultiScopeHandler):
+    """Identify feature scopes based on request state."""
+
+    def __init__(self, request):
+        super(ScopesFromRequest, self).__init__(
+            request, [f(request) for f in WEBAPP_SCOPE_HANDLERS])
+
+
+class ScopesForScript(MultiScopeHandler):
+    """Identify feature scopes for a given script."""
+
+    def __init__(self, script_name):
+        super(ScopesForScript, self).__init__(
+            None, [DefaultScope(None), ScriptScope(script_name)])
 
=== modified file 'lib/lp/services/features/testing.py'
--- lib/lp/services/features/testing.py	2010-12-02 16:13:51 +0000
+++ lib/lp/services/features/testing.py	2011-03-21 05:22:30 +0000
@@ -9,7 +9,10 @@
 
 from fixtures import Fixture
 
-from lp.services.features import per_thread
+from lp.services.features import (
+    get_relevant_feature_controller,
+    install_feature_controller,
+    )
 from lp.services.features.flags import FeatureController
 from lp.services.features.rulesource import (
     Rule,
@@ -51,10 +54,10 @@
             rule_source.setAllRules, rule_source.getAllRulesAsTuples())
         rule_source.setAllRules(self.makeNewRules())
 
-        original_controller = getattr(per_thread, 'features', None)
-        controller = FeatureController(lambda _: True, rule_source)
-        per_thread.features = controller
-        self.addCleanup(setattr, per_thread, 'features', original_controller)
+        original_controller = get_relevant_feature_controller()
+        install_feature_controller(
+            FeatureController(lambda _: True, rule_source))
+        self.addCleanup(install_feature_controller, original_controller)
 
     def makeNewRules(self):
         """Make a set of new feature flag rules."""
 
=== modified file 'lib/lp/services/features/tests/test_flags.py'
--- lib/lp/services/features/tests/test_flags.py	2011-01-04 15:20:04 +0000
+++ lib/lp/services/features/tests/test_flags.py	2011-03-21 05:22:30 +0000
@@ -11,7 +11,7 @@
 from canonical.testing import layers
 from lp.services.features import (
     getFeatureFlag,
-    per_thread,
+    install_feature_controller,
     )
 from lp.services.features.flags import FeatureController
 from lp.services.features.rulesource import StormFeatureRuleSource
@@ -115,19 +115,20 @@
     def test_threadGetFlag(self):
         self.populateStore()
         # the start-of-request handler will do something like this:
-        per_thread.features, call_log = self.makeControllerInScopes(
+        controller, call_log = self.makeControllerInScopes(
             ['default', 'beta_user'])
+        install_feature_controller(controller)
         try:
             # then application code can simply ask without needing a context
             # object
             self.assertEqual(u'4.0', getFeatureFlag('ui.icing'))
         finally:
-            per_thread.features = None
+            install_feature_controller(None)
 
     def test_threadGetFlagNoContext(self):
         # If there is no context, please don't crash. workaround for the root
         # cause in bug 631884.
-        per_thread.features = None
+        install_feature_controller(None)
         self.assertEqual(None, getFeatureFlag('ui.icing'))
 
     def testLazyScopeLookup(self):
 
=== added file 'lib/lp/services/features/tests/test_scopes.py'
--- lib/lp/services/features/tests/test_scopes.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/tests/test_scopes.py	2011-03-21 05:22:30 +0000
@@ -0,0 +1,67 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test feature-flag scopes."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import TestCaseWithFactory
+from lp.services.features.scopes import (
+    BaseScope,
+    MultiScopeHandler,
+    ScopesForScript,
+    ScriptScope,
+    )
+
+
+class FakeScope(BaseScope):
+    pattern = r'fake:'
+
+    def lookup(self, scope_name):
+        return scope_name == (self.pattern + self.request)
+
+
+class TestScopes(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_ScriptScope_lookup_matches_script_scope(self):
+        script_name = self.factory.getUniqueString()
+        scope = ScriptScope(script_name)
+        self.assertTrue(scope.lookup("script:" + script_name))
+
+    def test_ScriptScope_lookup_does_not_match_other_script_scope(self):
+        script_name = self.factory.getUniqueString()
+        scope = ScriptScope(script_name)
+        self.assertFalse(scope.lookup("script:other"))
+
+    def test_MultiScopeHandler_lookup_ignores_unmatched_scope(self):
+        scope_name = self.factory.getUniqueString()
+        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        self.assertFalse(handler.lookup("other:other"))
+
+    def test_MultiScopeHandler_lookup_ignores_inapplicable_scope(self):
+        scope_name = self.factory.getUniqueString()
+        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        self.assertFalse(handler.lookup("fake:other"))
+
+    def test_MultiScopeHandler_lookup_finds_matching_scope(self):
+        scope_name = self.factory.getUniqueString()
+        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        self.assertTrue(handler.lookup("fake:" + scope_name))
+
+    def test_ScopesForScript_includes_default_scope(self):
+        script_name = self.factory.getUniqueString()
+        scopes = ScopesForScript(script_name)
+        self.assertTrue(scopes.lookup("default"))
+
+    def test_ScopesForScript_lookup_finds_script(self):
+        script_name = self.factory.getUniqueString()
+        scopes = ScopesForScript(script_name)
+        self.assertTrue(scopes.lookup("script:" + script_name))
+
+    def test_ScopesForScript_lookup_does_not_find_other_script(self):
+        script_name = self.factory.getUniqueString()
+        scopes = ScopesForScript(script_name)
+        self.assertFalse(scopes.lookup("script:other"))
 
=== modified file 'lib/lp/services/features/webapp.py'
--- lib/lp/services/features/webapp.py	2011-01-14 21:48:57 +0000
+++ lib/lp/services/features/webapp.py	2011-03-21 05:22:30 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Connect Feature flags into webapp requests."""
@@ -7,7 +7,7 @@
 
 __metaclass__ = type
 
-from lp.services.features import per_thread
+from lp.services.features import install_feature_controller
 from lp.services.features.flags import FeatureController
 from lp.services.features.rulesource import StormFeatureRuleSource
 from lp.services.features.scopes import ScopesFromRequest
@@ -15,11 +15,13 @@
 
 def start_request(event):
     """Register FeatureController."""
-    event.request.features = per_thread.features = FeatureController(
+    event.request.features = FeatureController(
         ScopesFromRequest(event.request).lookup,
         StormFeatureRuleSource())
+    install_feature_controller(event.request.features)
 
 
 def end_request(event):
     """Done with this FeatureController."""
-    event.request.features = per_thread.features = None
+    install_feature_controller(None)
+    event.request.features = None
 
=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py	2011-02-21 00:06:55 +0000
+++ lib/lp/services/scripts/base.py	2011-03-21 05:22:30 +0000
@@ -39,6 +39,12 @@
     setupInteractionByEmail,
     )
 from canonical.lp import initZopeless
+from lp.services.features import (
+    getFeatureFlag,
+    get_relevant_feature_controller,
+    install_feature_controller,
+    make_script_feature_controller,
+    )
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 
 
@@ -294,6 +300,7 @@
     @log_unhandled_exception_and_exit
     def run(self, use_web_security=False, isolation=None):
         """Actually run the script, executing zcml and initZopeless."""
+
         if isolation is None:
             isolation = ISOLATION_LEVEL_DEFAULT
         self._init_zca(use_web_security=use_web_security)
@@ -303,6 +310,9 @@
         profiler = None
         if self.options.profile:
             profiler = Profile()
+
+        original_feature_controller = get_relevant_feature_controller()
+        install_feature_controller(make_script_feature_controller(self.name))
         try:
             if profiler:
                 profiler.runcall(self.main)
@@ -316,6 +326,8 @@
         else:
             date_completed = datetime.datetime.now(UTC)
             self.record_activity(date_started, date_completed)
+        finally:
+            install_feature_controller(original_feature_controller)
         if profiler:
             profiler.dump_stats(self.options.profile)
 
 
=== added file 'lib/lp/services/scripts/tests/test_feature_controller.py'
--- lib/lp/services/scripts/tests/test_feature_controller.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/scripts/tests/test_feature_controller.py	2011-03-21 05:22:30 +0000
@@ -0,0 +1,61 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test feature controller in scripts."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.services.features import (
+    get_relevant_feature_controller,
+    install_feature_controller,
+    )
+from lp.services.features.flags import NullFeatureController
+from lp.services.features.testing import FeatureFixture
+from lp.services.scripts.base import LaunchpadScript
+from lp.testing import TestCase
+from lp.testing.fakemethod import FakeMethod
+
+
+class FakeScript(LaunchpadScript):
+    """A dummy script that only records which feature controller is active."""
+
+    observed_feature_controller = object()
+
+    def __init__(self, name):
+        super(FakeScript, self).__init__(name=name, test_args=[])
+
+    def main(self):
+        self.observed_feature_controller = get_relevant_feature_controller()
+
+    # Shortcut some underpinnings of LaunchpadScript.run that we can't
+    # afford to have happen in tests.
+    _init_zca = FakeMethod()
+    _init_db = FakeMethod()
+    record_activity = FakeMethod()
+
+
+class TestScriptFeatureController(TestCase):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestScriptFeatureController, self).setUp()
+        self.original_controller = get_relevant_feature_controller()
+
+    def tearDown(self):
+        install_feature_controller(self.original_controller)
+        super(TestScriptFeatureController, self).tearDown()
+
+    def test_script_installs_script_feature_controller(self):
+        script = FakeScript(name="bongo")
+        script_feature_controller = get_relevant_feature_controller()
+        self.assertNotEqual(
+            self.original_controller, script.observed_feature_controller)
+        self.assertNotEqual(None, script.observed_feature_controller)
+
+    def test_script_restores_feature_controller(self):
+        previous_controller = NullFeatureController()
+        install_feature_controller(previous_controller)
+        FakeScript(name="mongo").run()
+        self.assertEqual(
+            previous_controller, get_relevant_feature_controller())
 
=== modified file 'lib/lp/testing/__init__.py'
--- lib/lp/testing/__init__.py	2011-03-09 18:48:09 +0000
+++ lib/lp/testing/__init__.py	2011-03-21 05:22:30 +0000
@@ -968,6 +968,7 @@
     This prevents the events from propagating to their normal subscribers.
     The recorded events can be accessed via the 'events' list.
     """
+
     def __init__(self):
         self.events = []
         self.old_subscribers = None
@@ -987,13 +988,13 @@
 def feature_flags():
     """Provide a context in which feature flags work."""
     empty_request = LaunchpadTestRequest()
-    old_features = getattr(features.per_thread, 'features', None)
-    features.per_thread.features = FeatureController(
-        ScopesFromRequest(empty_request).lookup)
+    old_features = features.get_relevant_feature_controller()
+    features.install_feature_controller(FeatureController(
+        ScopesFromRequest(empty_request).lookup))
     try:
         yield
     finally:
-        features.per_thread.features = old_features
+        features.install_feature_controller(old_features)
 
 
 def get_lsb_information():
@@ -1102,12 +1103,12 @@
     """Set a feature flag to the specified value.
 
     In order to access the flag, use the feature_flags context manager or
-    populate features.per_thread.features some other way.
+    set the feature controller in some other way.
     :param name: The name of the flag.
     :param value: The value of the flag.
     :param scope: The scope in which the specified value applies.
     """
-    assert getattr(features.per_thread, 'features', None) is not None
+    assert features.get_relevant_feature_controller() is not None
     flag = FeatureFlag(
         scope=scope, flag=name, value=value, priority=priority)
     store = getFeatureStore()