launchpad-reviewers team mailing list archive

[Colin Watson <mp+431820@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-server-scopes into launchpad:master.

Commit message:
Remove server feature scopes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/431820

We don't seem to use these in practice, and I can't really think of many situations where it would be more convenient to do so rather than just setting appropriate feature rules on each instance.  The only somewhat relevant piece of configuration that we have right now is the `is_demo` flag, but that isn't implemented using server scopes anyway and it wouldn't be worth maintaining that code just for the sake of a single boolean.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-server-scopes into launchpad:master.

diff --git a/lib/lp/app/templates/base-layout.pt b/lib/lp/app/templates/base-layout.pt
index 63a8ca8..74b4f45 100644
--- a/lib/lp/app/templates/base-layout.pt
+++ b/lib/lp/app/templates/base-layout.pt
@@ -8,7 +8,6 @@
     devmode modules/lp.services.config/config/launchpad/devmode;
     rooturl modules/lp.services.webapp.vhosts/allvhosts/configs/mainsite/rooturl;
     is_demo modules/lp.services.config/config/launchpad/is_demo;
-    is_lpnet modules/lp.services.config/config/launchpad/is_lpnet;
     site_message modules/lp.services.config/config/launchpad/site_message;
     icingroot string:${rooturl}+icing/rev${revision};
     features request/features;
diff --git a/lib/lp/services/features/__init__.py b/lib/lp/services/features/__init__.py
index a672f29..181f7b4 100644
--- a/lib/lp/services/features/__init__.py
+++ b/lib/lp/services/features/__init__.py
@@ -25,10 +25,10 @@ that apply to that request, by finding the I{rule} with the highest
 I{priority}.
 
 Flags are defined by a I{name} that typically looks like a Python
-identifier, for example C{notification.global.text}.  A definition is
-given for a particular I{scope}, which also looks like a dotted identifier,
-for example C{user.beta} or C{server.lpnet}.  This is just a naming
-convention, and they do not need to correspond to Python modules.
+identifier, for example C{notification.global.text}.  A definition is given
+for a particular I{scope}, for example C{pageid:Person:+editemails} or
+C{team:admins}.  This is just a naming convention, and they do not need to
+correspond to Python modules.
 
 The value is stored in the database as just a Unicode string, and it might
 be interpreted as a boolean, number, human-readable string or whatever.
@@ -99,8 +99,9 @@ Templates can also check whether the request is in a particular scope, but
 before using this consider whether the code will always be bound to that
 scope or whether it would be more correct to define a new feature::
 
-  <p tal:condition="feature_scopes/server.staging">
-    Staging server: all data will be discarded daily!</p>
+  <p tal:condition="feature_scopes/team:admins">
+    You are a Launchpad administrator.  Be careful!
+  </p>
 
 Checking flags in code
 ======================
diff --git a/lib/lp/services/features/scopes.py b/lib/lp/services/features/scopes.py
index 4e114f5..cc848aa 100644
--- a/lib/lp/services/features/scopes.py
+++ b/lib/lp/services/features/scopes.py
@@ -4,7 +4,7 @@
 """Connect feature flags into scopes where they can be used.
 
 The most common is flags scoped by some attribute of a web request, such as
-the page ID or the server name.  But other types of scope can also match code
+the page ID or a team name.  But other types of scope can also match code
 run from cron scripts and potentially also other places.
 """
 
@@ -24,7 +24,6 @@ __all__ = [
 import re
 from itertools import zip_longest
 
-import lp.services.config
 from lp.registry.interfaces.person import IPerson
 from lp.services.propertycache import cachedproperty
 
@@ -182,25 +181,6 @@ class UserSliceScope(ScopeWithPerson):
         return (person_id % divisor) == modulus
 
 
-class ServerScope(BaseScope):
-    """Matches the current server.
-
-    For example, the scope server.lpnet is active when is_lpnet is set to True
-    in the Launchpad configuration.
-    """
-
-    pattern = r"server\."
-
-    def lookup(self, scope_name):
-        """Match the current server as a scope."""
-        server_name = scope_name.split(".", 1)[1]
-        try:
-            return lp.services.config.config["launchpad"]["is_" + server_name]
-        except KeyError:
-            pass
-        return False
-
-
 class ScriptScope(BaseScope):
     """Matches the name of the currently running script.
 
@@ -236,7 +216,7 @@ class FixedScope(BaseScope):
 # 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.
 # See BaseScope for hints as to what a scope handler should look like.
-HANDLERS = {DefaultScope, PageScope, TeamScope, ServerScope, ScriptScope}
+HANDLERS = {DefaultScope, PageScope, TeamScope, ScriptScope}
 
 
 class MultiScopeHandler:
@@ -296,7 +276,6 @@ class ScopesFromRequest(MultiScopeHandler):
         scopes.extend(
             [
                 PageScope(request),
-                ServerScope(),
                 TeamScope(person_from_request),
                 UserSliceScope(person_from_request),
             ]
diff --git a/lib/lp/services/features/tests/test_flags.py b/lib/lp/services/features/tests/test_flags.py
index 548c0d7..04a919b 100644
--- a/lib/lp/services/features/tests/test_flags.py
+++ b/lib/lp/services/features/tests/test_flags.py
@@ -186,7 +186,7 @@ class TestFeatureFlags(TestCase):
         self.assertEqual(dict(), f.usedScopes())
 
     def testScopeDict(self):
-        # can get scopes as a dict, for use by "feature_scopes/server.demo"
+        # can get scopes as a dict, for use by "feature_scopes/..."
         f, call_log = self.makeControllerInScopes(["beta_user"])
         self.assertEqual(True, f.scopes["beta_user"])
         self.assertEqual(False, f.scopes["alpha_user"])
diff --git a/lib/lp/services/features/tests/test_webapp.py b/lib/lp/services/features/tests/test_webapp.py
index c61504d..37de309 100644
--- a/lib/lp/services/features/tests/test_webapp.py
+++ b/lib/lp/services/features/tests/test_webapp.py
@@ -3,9 +3,6 @@
 
 """Tests for webapp glue."""
 
-from textwrap import dedent
-
-from lp.services.config import config
 from lp.services.features import getFeatureFlag, webapp
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.errorlog import globalErrorUtility
@@ -55,30 +52,6 @@ class TestScopesFromRequest(TestCase):
         scopes = webapp.ScopesFromRequest(request)
         self.assertTrue(scopes.lookup("default"))
 
-    def test_server(self):
-        request = LaunchpadTestRequest()
-        scopes = webapp.ScopesFromRequest(request)
-        self.assertFalse(scopes.lookup("server.lpnet"))
-        config.push(
-            "ensure_lpnet",
-            dedent(
-                """\
-            [launchpad]
-            is_lpnet: True
-            """
-            ),
-        )
-        try:
-            self.assertTrue(scopes.lookup("server.lpnet"))
-        finally:
-            config.pop("ensure_lpnet")
-
-    def test_server_missing_key(self):
-        request = LaunchpadTestRequest()
-        scopes = webapp.ScopesFromRequest(request)
-        # There is no such key in the config, so this returns False.
-        self.assertFalse(scopes.lookup("server.pink"))
-
     def test_unknown_scope(self):
         # Asking about an unknown scope is not an error.
         request = LaunchpadTestRequest()