launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05047
[Merge] lp:~mwhudson/launchpad/feature-flag-xmlrpc-2 into lp:launchpad
Michael Hudson-Doyle has proposed merging lp:~mwhudson/launchpad/feature-flag-xmlrpc-2 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #855609 in Launchpad itself: "feature flag scopes are broken"
https://bugs.launchpad.net/launchpad/+bug/855609
For more details, see:
https://code.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc-2/+merge/76493
This relands my branch https://code.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc/+merge/75673 which broke because feature controllers are installed before authentication happens, so looking at request.principal at that time doesn't work.
The change that fixes the problem is here: http://bazaar.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc-2/revision/14013
I haven't added an integration level test, because it would be a lot of work I think (you could register a view with a template that accesses a feature flag and then use testbrowser to load it) and I'm not sure it would have caught this problem anyway (are you really sure that the way testbrowser does authentication is that similar to how it happens in a real appserver? maybe it is, but I don't know). I can add one if the reviewer thinks it's a good idea though.
--
https://code.launchpad.net/~mwhudson/launchpad/feature-flag-xmlrpc-2/+merge/76493
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mwhudson/launchpad/feature-flag-xmlrpc-2 into lp:launchpad.
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf 2011-09-21 14:38:47 +0000
+++ configs/development/launchpad-lazr.conf 2011-09-21 22:26:28 +0000
@@ -158,6 +158,7 @@
geonames_identity: lpdev
storm_cache: generational
storm_cache_size: 100
+feature_flags_endpoint: http://xmlrpc-private.launchpad.dev:8087/featureflags/
[launchpad_session]
cookie: launchpad_dev
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-09-21 14:38:47 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-09-21 22:26:28 +0000
@@ -1211,6 +1211,12 @@
# The number of items to display:
homepage_recent_posts_count: 6
+# The URL of the XML-RPC endpoint that allows querying of feature
+# flags. This should implement IFeatureFlagApplication.
+#
+# datatype: string
+feature_flags_endpoint:
+
[launchpad_session]
# The database connection string.
# datatype: pgconnection
=== modified file 'lib/canonical/launchpad/interfaces/launchpad.py'
--- lib/canonical/launchpad/interfaces/launchpad.py 2011-09-21 14:38:47 +0000
+++ lib/canonical/launchpad/interfaces/launchpad.py 2011-09-21 22:26:28 +0000
@@ -152,6 +152,8 @@
softwarecenteragent = Attribute(
"""Software center agent XML-RPC end point.""")
+ featureflags = Attribute("""Feature flag information endpoint""")
+
class IAuthServerApplication(ILaunchpadApplication):
"""Launchpad legacy AuthServer application root."""
=== modified file 'lib/canonical/launchpad/xmlrpc/application.py'
--- lib/canonical/launchpad/xmlrpc/application.py 2011-09-21 14:38:47 +0000
+++ lib/canonical/launchpad/xmlrpc/application.py 2011-09-21 22:26:28 +0000
@@ -36,6 +36,7 @@
)
from lp.registry.interfaces.mailinglist import IMailingListApplication
from lp.registry.interfaces.person import ISoftwareCenterAgentApplication
+from lp.services.features.xmlrpc import IFeatureFlagApplication
# NOTE: If you add a traversal here, you should update
@@ -73,6 +74,11 @@
"""See `IPrivateApplication`."""
return getUtility(ISoftwareCenterAgentApplication)
+ @property
+ def featureflags(self):
+ """See `IPrivateApplication`."""
+ return getUtility(IFeatureFlagApplication)
+
class ISelfTest(Interface):
"""XMLRPC external interface for testing the XMLRPC external interface."""
@@ -127,4 +133,3 @@
def run_test(self):
return "OK"
-
=== modified file 'lib/lp/services/features/__init__.py'
--- lib/lp/services/features/__init__.py 2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/__init__.py 2011-09-21 22:26:28 +0000
@@ -125,6 +125,18 @@
if value:
print value
+Checking flags without access to the database
+=============================================
+
+Feature flags can also be checked without access to the database by making use
+of the 'getFeatureFlag' XML-RPC method.
+
+ server_proxy = xmlrpclib.ServerProxy(
+ config.launchpad.feature_flags_endpoint, allow_none=True)
+ if server_proxy.getFeatureFlag(
+ 'codehosting.use_forking_server', ['user:' + user_name]):
+ pass
+
Debugging feature usage
=======================
=== modified file 'lib/lp/services/features/configure.zcml'
--- lib/lp/services/features/configure.zcml 2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/configure.zcml 2011-09-21 22:26:28 +0000
@@ -19,4 +19,11 @@
handler="lp.services.features.webapp.end_request"
/>
+ <securedutility
+ class="lp.services.features.xmlrpc.FeatureFlagApplication"
+ provides="lp.services.features.xmlrpc.IFeatureFlagApplication">
+ <allow
+ interface="lp.services.features.xmlrpc.IFeatureFlagApplication"/>
+ </securedutility>
+
</configure>
=== modified file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py 2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/scopes.py 2011-09-21 22:26:28 +0000
@@ -9,9 +9,14 @@
"""
__all__ = [
+ 'DefaultScope',
+ 'default_scopes',
+ 'FixedScope',
'HANDLERS',
+ 'MultiScopeHandler',
'ScopesForScript',
'ScopesFromRequest',
+ 'TeamScope',
'undocumented_scopes',
]
@@ -19,9 +24,7 @@
import re
-from zope.component import getUtility
-
-from canonical.launchpad.webapp.interfaces import ILaunchBag
+from lp.registry.interfaces.person import IPerson
from lp.services.propertycache import cachedproperty
import canonical.config
@@ -60,14 +63,7 @@
return True
-class BaseWebRequestScope(BaseScope):
- """Base class for scopes that key off web request attributes."""
-
- def __init__(self, request):
- self.request = request
-
-
-class PageScope(BaseWebRequestScope):
+class PageScope(BaseScope):
"""The current page ID.
Pageid scopes are written as 'pageid:' + the pageid to match. Pageids
@@ -81,6 +77,9 @@
pattern = r'pageid:'
+ def __init__(self, pageid):
+ self._pageid = pageid
+
def lookup(self, scope_name):
"""Is the given scope match the current pageid?"""
pageid_scope = scope_name[len('pageid:'):]
@@ -104,21 +103,32 @@
@cachedproperty
def _request_pageid_namespace(self):
- return tuple(self._pageid_to_namespace(
- self.request._orig_env.get('launchpad.pageid', '')))
+ return tuple(self._pageid_to_namespace(self._pageid))
class TeamScope(BaseScope):
- """The current user's team memberships.
+ """A user's team memberships.
Team ID scopes are written as 'team:' + the team name to match.
The scope 'team:launchpad-beta-users' will match members of the team
'launchpad-beta-users'.
+
+ The constructor takes a callable that returns the currently logged in
+ person because Scopes are set up very early in the request publication
+ process -- in particular, before authentication has happened.
"""
pattern = r'team:'
+ def __init__(self, get_person):
+ self._get_person = get_person
+ self._person = None
+
+ @cachedproperty
+ def person(self):
+ return self._get_person()
+
def lookup(self, scope_name):
"""Is the given scope a team membership?
@@ -126,11 +136,9 @@
team based scopes in use to a small number. (Person.inTeam could be
fixed to reduce this to one query).
"""
- team_name = scope_name[len('team:'):]
- person = getUtility(ILaunchBag).user
- if person is None:
- return False
- return person.inTeam(team_name)
+ if self.person is not None:
+ team_name = scope_name[len('team:'):]
+ return self.person.inTeam(team_name)
class ServerScope(BaseScope):
@@ -169,6 +177,20 @@
return scope_name == self.script_scope
+class FixedScope(BaseScope):
+ """A scope that matches an exact value.
+
+ Functionally `ScriptScope` and `DefaultScope` are equivalent to instances
+ of this class, but their docstings are used on the +feature-info page.
+ """
+
+ def __init__(self, scope):
+ self.pattern = re.escape(scope) + '$'
+
+ def lookup(self, scope_name):
+ return True
+
+
# These are the handlers for all of the allowable scopes, listed here so that
# we can for example show all of them in an admin page. Any new scope will
# need a scope handler and that scope handler has to be added to this list.
@@ -213,21 +235,28 @@
undocumented_scopes.add(scope_name)
+default_scopes = (DefaultScope(),)
+
+
class ScopesFromRequest(MultiScopeHandler):
"""Identify feature scopes based on request state."""
def __init__(self, request):
- super(ScopesFromRequest, self).__init__([
- DefaultScope(),
- PageScope(request),
- TeamScope(),
- ServerScope()])
+ def person_from_request():
+ return IPerson(request.principal, None)
+ scopes = list(default_scopes)
+ scopes.extend([
+ PageScope(request._orig_env.get('launchpad.pageid', '')),
+ ServerScope(),
+ TeamScope(person_from_request)
+ ])
+ super(ScopesFromRequest, self).__init__(scopes)
class ScopesForScript(MultiScopeHandler):
"""Identify feature scopes for a given script."""
def __init__(self, script_name):
- super(ScopesForScript, self).__init__([
- DefaultScope(),
- ScriptScope(script_name)])
+ scopes = list(default_scopes)
+ scopes.append(ScriptScope(script_name))
+ super(ScopesForScript, self).__init__(scopes)
=== modified file 'lib/lp/services/features/tests/test_scopes.py'
--- lib/lp/services/features/tests/test_scopes.py 2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/tests/test_scopes.py 2011-09-21 22:26:28 +0000
@@ -9,18 +9,20 @@
from lp.testing import TestCaseWithFactory
from lp.services.features.scopes import (
BaseScope,
- BaseWebRequestScope,
MultiScopeHandler,
ScopesForScript,
ScriptScope,
)
-class FakeScope(BaseWebRequestScope):
+class FakeScope(BaseScope):
pattern = r'fake:'
+ def __init__(self, name):
+ self.name = name
+
def lookup(self, scope_name):
- return scope_name == (self.pattern + self.request)
+ return scope_name == (self.pattern + self.name)
class TestScopes(TestCaseWithFactory):
=== modified file 'lib/lp/services/features/tests/test_webapp.py'
--- lib/lp/services/features/tests/test_webapp.py 2011-09-21 14:38:47 +0000
+++ lib/lp/services/features/tests/test_webapp.py 2011-09-21 22:26:28 +0000
@@ -87,7 +87,7 @@
def test_team_scope_outside_team(self):
request = LaunchpadTestRequest()
scopes = webapp.ScopesFromRequest(request)
- self.factory.loginAsAnyone()
+ self.factory.loginAsAnyone(request)
self.assertFalse(scopes.lookup('team:nonexistent'))
def test_team_scope_in_team(self):
@@ -95,5 +95,5 @@
scopes = webapp.ScopesFromRequest(request)
member = self.factory.makePerson()
team = self.factory.makeTeam(members=[member])
- login_as(member)
+ login_as(member, request)
self.assertTrue(scopes.lookup('team:%s' % team.name))
=== added file 'lib/lp/services/features/tests/test_xmlrpc.py'
--- lib/lp/services/features/tests/test_xmlrpc.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/tests/test_xmlrpc.py 2011-09-21 22:26:28 +0000
@@ -0,0 +1,99 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for FeatureFlagApplication."""
+
+__metaclass__ = type
+
+import xmlrpclib
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.config import config
+from lp.services import features
+from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.services.features.scopes import (
+ DefaultScope,
+ FixedScope,
+ MultiScopeHandler,
+ )
+from lp.services.features.xmlrpc import FeatureFlagApplication
+from lp.testing import (
+ feature_flags,
+ set_feature_flag,
+ TestCaseWithFactory,
+ )
+from lp.testing.xmlrpc import XMLRPCTestTransport
+
+
+class TestGetFeatureFlag(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ TestCaseWithFactory.setUp(self)
+ self.endpoint = FeatureFlagApplication()
+
+ def installFeatureController(self, feature_controller):
+ old_features = features.get_relevant_feature_controller()
+ features.install_feature_controller(feature_controller)
+ self.addCleanup(
+ features.install_feature_controller, old_features)
+
+ def test_getFeatureFlag_returns_None_by_default(self):
+ self.assertIs(None, self.endpoint.getFeatureFlag(u'unknown'))
+
+ def test_getFeatureFlag_returns_true_for_set_flag(self):
+ flag_name = u'flag'
+ with feature_flags():
+ set_feature_flag(flag_name, u'1')
+ self.assertEqual(u'1', self.endpoint.getFeatureFlag(flag_name))
+
+ def test_getFeatureFlag_ignores_relevant_feature_controller(self):
+ # getFeatureFlag should only consider the scopes it is asked to
+ # consider, not any that happen to be active due to the XML-RPC
+ # request itself.
+ flag_name = u'flag'
+ scope_name = u'scope'
+ self.installFeatureController(
+ FeatureController(
+ MultiScopeHandler(
+ [DefaultScope(), FixedScope(scope_name)]).lookup,
+ StormFeatureRuleSource()))
+ set_feature_flag(flag_name, u'1', scope_name)
+ self.assertEqual(None, self.endpoint.getFeatureFlag(flag_name))
+
+ def test_getFeatureFlag_considers_supplied_scope(self):
+ flag_name = u'flag'
+ scope_name = u'scope'
+ with feature_flags():
+ set_feature_flag(flag_name, u'value', scope_name)
+ self.assertEqual(
+ u'value',
+ self.endpoint.getFeatureFlag(flag_name, [scope_name]))
+
+ def test_getFeatureFlag_turns_user_into_team_scope(self):
+ flag_name = u'flag'
+ person = self.factory.makePerson()
+ team = self.factory.makeTeam(members=[person])
+ with feature_flags():
+ set_feature_flag(flag_name, u'value', u'team:' + team.name)
+ self.assertEqual(
+ u'value',
+ self.endpoint.getFeatureFlag(
+ flag_name, ['user:' + person.name]))
+
+ def test_xmlrpc_interface_unset(self):
+ sp = xmlrpclib.ServerProxy(
+ config.launchpad.feature_flags_endpoint,
+ transport=XMLRPCTestTransport(), allow_none=True)
+ self.assertEqual(None, sp.getFeatureFlag(u'flag'))
+
+ def test_xmlrpc_interface_set(self):
+ sp = xmlrpclib.ServerProxy(
+ config.launchpad.feature_flags_endpoint,
+ transport=XMLRPCTestTransport(), allow_none=True)
+ flag_name = u'flag'
+ with feature_flags():
+ set_feature_flag(flag_name, u'1')
+ self.assertEqual(u'1', sp.getFeatureFlag(flag_name))
=== added file 'lib/lp/services/features/xmlrpc.py'
--- lib/lp/services/features/xmlrpc.py 1970-01-01 00:00:00 +0000
+++ lib/lp/services/features/xmlrpc.py 2011-09-21 22:26:28 +0000
@@ -0,0 +1,59 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""FeatureFlagApplication allows access to information about feature flags."""
+
+__metaclass__ = type
+__all__ = [
+ 'IFeatureFlagApplication',
+ 'FeatureFlagApplication',
+ ]
+
+from zope.component import getUtility
+from zope.interface import implements
+
+from canonical.launchpad.webapp.interfaces import ILaunchpadApplication
+from lp.registry.interfaces.person import IPersonSet
+from lp.services.features.flags import FeatureController
+from lp.services.features.rulesource import StormFeatureRuleSource
+from lp.services.features.scopes import (
+ default_scopes,
+ FixedScope,
+ MultiScopeHandler,
+ TeamScope,
+ )
+
+
+class IFeatureFlagApplication(ILaunchpadApplication):
+ """Mailing lists application root."""
+
+ def getFeatureFlag(flag_name, username=None, scopes=()):
+ """Return the value of the given feature flag.
+
+ :param flag_name: The name of the flag to query.
+ :param username: If supplied, the name of a Person to use in
+ evaluating the 'team:' scope.
+ :param scopes: A list of scopes to consider active. The 'default'
+ scope is always considered to be active, and does not need to be
+ included here.
+ """
+
+
+class FeatureFlagApplication:
+
+ implements(IFeatureFlagApplication)
+
+ def getFeatureFlag(self, flag_name, active_scopes=()):
+ scopes = list(default_scopes)
+ for scope_name in active_scopes:
+ if scope_name.startswith('user:'):
+ person = getUtility(IPersonSet).getByName(
+ scope_name[len('user:'):])
+ if person is not None:
+ scopes.append(TeamScope(lambda: person))
+ else:
+ scopes.append(FixedScope(scope_name))
+ flag_name = unicode(flag_name)
+ controller = FeatureController(
+ MultiScopeHandler(scopes).lookup, StormFeatureRuleSource())
+ return controller.getFlag(flag_name)
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-09-21 19:10:33 +0000
+++ lib/lp/testing/factory.py 2011-09-21 22:26:28 +0000
@@ -505,7 +505,7 @@
for any other required objects.
"""
- def loginAsAnyone(self):
+ def loginAsAnyone(self, participation=None):
"""Log in as an arbitrary person.
If you want to log in as a celebrity, including admins, see
@@ -513,7 +513,7 @@
"""
login(ANONYMOUS)
person = self.makePerson()
- login_as(person)
+ login_as(person, participation)
return person
@with_celebrity_logged_in('admin')
=== modified file 'utilities/page-performance-report.ini'
--- utilities/page-performance-report.ini 2011-09-21 14:38:47 +0000
+++ utilities/page-performance-report.ini 2011-09-21 22:26:28 +0000
@@ -11,7 +11,8 @@
[^/]+($|/
(?!\+haproxy|\+opstats|\+access-token
|((authserver|bugs|bazaar|codehosting|
- codeimportscheduler|mailinglists|softwarecenteragent)/\w+$)))
+ codeimportscheduler|mailinglists|softwarecenteragent|
+ featureflags)/\w+$)))
Other=^/
Launchpad Frontpage=^https?://launchpad\.[^/]+(/index\.html)?$
@@ -54,7 +55,7 @@
Private XML-RPC=^https://(launchpad|xmlrpc)[^/]+/
(authserver|bugs|codehosting|
codeimportscheduler|mailinglists|
- softwarecenteragent)/\w+$
+ softwarecenteragent|featureflags)/\w+$
[metrics]
ppr_all=All Launchpad except operational pages
