Merge lp:~wgrant/launchpad/bug-708999-ff-docs into lp:launchpad

[William Grant]

[r=lifeless][bug=708999] Document code.branchmergequeue and code.incremental_diffs.enabled feature flags, and clean up the docs for the others.

This branch fixes bug #708999 by documenting the two undocumented feature flags.

It also cleans and reformats the value domains and descriptions of several other flags to be more consistent and informative.

[Martin Pool (mbp)]

Thanks for addressing this bug.

To me those explanations don't add a lot beyond the name of the variable.

For instance
  'shows branch merge queues if non-empty'
if the bmq is non-empty?

From looking at the usage of the flags it looks like this means "shows them if the flag is boolean true", and the values are not the strings 'empty' and 'nonempty' but rather you can use any string, which Python will treat as true?

I think that shows we badly need a standard way to express boolean flags.

I would document them as

  if true (a non-empty string), shows the branch merge queue user interface

  if true (a non-empty string), show incremental diffs on merge proposals

[William Grant (wgrant)]

After much discussion on IRC, I went with a more thorough revamp. I standardised the boolean type to empty/non-empty (which most of the flags already respected, despite their docs), and documented the types. I also expanded the descriptions, documented soyuz.derived-distros-ui.enabled, and added a link to further documentation on the dev wiki.

The new pages can be seen at <http://people.canonical.com/~wgrant/launchpad/feature-flag-docs/>.

Only one flag will require changes on production: "memcache pageid:ProjectGroup:+milestones 13 disabled" needs an s/disabled//.

[Martin Pool (mbp)]

<poolie> wgrant, also hard_timeout, contrary to its docs, seems to be
in milliseconds
 not seconds

[Aaron Bentley (abentley)]

I fully agree that we need a standard way of doing boolean FeatureFlags. I believe the lack of it indicates that Feature Flags are incomplete, since enabling/disabling features was described as key functionality.

But I think using empty/non-empty to indicate boolean is pretty gross.

[William Grant (wgrant)]

Sure, it is really gross, but all except two boolean flags already used that convention. It needs additional thought and work to make booleans work nicely.

[Martin Pool (mbp)]

On 17 February 2011 03:16, Aaron Bentley <email address hidden> wrote:
> I fully agree that we need a standard way of doing boolean FeatureFlags.  I believe the lack of it indicates that Feature Flags are incomplete, since enabling/disabling features was described as key functionality.
>
> But I think using empty/non-empty to indicate boolean is pretty gross.

I think it's gross too. I filed
<https://bugs.launchpad.net/launchpad/+bug/719182> for that. I think
documenting what currently exists has some value, and making things
consistent also. I'm not really happy about making them consistent
with the worst precedented approach, but doing it one step at a time
is good too.

[Robert Collins (lifeless)]

FWIW I really like the pithy nature of what we have at the moment.
Please don't lose that.

[Martin Pool (mbp)]

On 17 February 2011 11:31, Robert Collins <email address hidden> wrote:
> FWIW I really like the pithy nature of what we have at the moment.
> Please don't lose that.

So can we say it should make sense to people who didn't develop the
feature, but still one sentence?

[Robert Collins (lifeless)]

On Thu, Feb 17, 2011 at 1:49 PM, Martin Pool <email address hidden> wrote:
> On 17 February 2011 11:31, Robert Collins <email address hidden> wrote:
>> FWIW I really like the pithy nature of what we have at the moment.
>> Please don't lose that.
>
> So can we say it should make sense to people who didn't develop the
> feature, but still one sentence?

I don't understand.

[Martin Pool (mbp)]

On 17 February 2011 13:02, Robert Collins <email address hidden> wrote:
> On Thu, Feb 17, 2011 at 1:49 PM, Martin Pool <email address hidden> wrote:
>> On 17 February 2011 11:31, Robert Collins <email address hidden> wrote:
>>> FWIW I really like the pithy nature of what we have at the moment.
>>> Please don't lose that.
>>
>> So can we say it should make sense to people who didn't develop the
>> feature, but still one sentence?
>
> I don't understand.

Which aspect of pithiness were you talking about?

[Robert Collins (lifeless)]

On Thu, Feb 17, 2011 at 6:16 PM, Martin Pool <email address hidden> wrote:

> Which aspect of pithiness were you talking about?

the feature flags code itself and the code using it.

[Robert Collins (lifeless)]

The domain_value_info test is a little weird - is it really looking for the str() of the named tuples to be in the page?

The memcache flag seems a little hard to follow the implementation of now, but other than that this seems ok. I note that the memcache implementation has gotten substantially longer, and that seems undesirable to me. Lets talk.

[William Grant (wgrant)]

That test is similar to the others. It checks that each member of each namedtuple is displayed.

I agree with your memcache concern, but it was either that or add a default mechanism.

[Robert Collins (lifeless)]

review: Approve

=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py	2011-01-20 23:55:37 +0000
+++ lib/canonical/launchpad/browser/librarian.py	2011-02-18 08:19:14 +0000
@@ -83,8 +83,8 @@
 
     If the file is public, it will redirect to the files http url.
 
-    Otherwise if the feature flag publicrestrictedlibrarian is set to 'on'
-    this will allocate a token and redirect to the aliases private url.
+    Otherwise if the feature flag publicrestrictedlibrarian is true this will
+    allocate a token and redirect to the aliases private url.
 
     Otherwise it will proxy the file in the appserver.
 
@@ -179,7 +179,7 @@
         if not self.context.restricted:
             # Public file, just point the client at the right place.
             return RedirectionView(self.context.http_url, self.request), ()
-        if getFeatureFlag(u'publicrestrictedlibrarian') != 'on':
+        if not getFeatureFlag(u'publicrestrictedlibrarian'):
             # Restricted file and we have not enabled the public
             # restricted librarian yet :- deliver inline.
             self._when_streaming()

=== modified file 'lib/lp/services/features/browser/info.py'
--- lib/lp/services/features/browser/info.py	2011-01-26 22:07:08 +0000
+++ lib/lp/services/features/browser/info.py	2011-02-18 08:19:14 +0000
@@ -15,6 +15,7 @@
 from lp.services.features.flags import (
     flag_info,
     undocumented_flags,
+    value_domain_info,
     )
 from lp.services.features.scopes import (
     HANDLERS,
@@ -25,6 +26,7 @@
 
 # Named tuples to use when passing flag and scope data to the template.
 Flag = namedtuple('Flag', ('name', 'domain', 'description', 'default'))
+ValueDomain = namedtuple('ValueDomain', ('name', 'description'))
 Scope = namedtuple('Scope', ('regex', 'description'))
 
 
@@ -45,6 +47,11 @@
         return ', '.join(undocumented_flags)
 
     @property
+    def value_domain_info(self):
+        """A list of flags as named tuples, ready to be rendered."""
+        return map(ValueDomain._make, value_domain_info)
+
+    @property
     def undocumented_scopes(self):
         """Scope names referenced during process lifetime but not documented.
         """

=== modified file 'lib/lp/services/features/browser/tests/test_feature_info.py'
--- lib/lp/services/features/browser/tests/test_feature_info.py	2011-01-26 22:07:08 +0000
+++ lib/lp/services/features/browser/tests/test_feature_info.py	2011-02-18 08:19:14 +0000
@@ -19,6 +19,7 @@
     flag_info,
     NullFeatureController,
     undocumented_flags,
+    value_domain_info,
     )
 from lp.services.features.scopes import (
     HANDLERS,
@@ -66,6 +67,14 @@
             for item in record:
                 self.assertThat(browser.contents, Contains(item))
 
+    def test_value_domain_documentation_displayed(self):
+        """The value domain documentation is displayed on the page."""
+        browser = self.getUserBrowserAsAdmin()
+        browser.open(self.getFeatureInfoUrl())
+        for record in value_domain_info:
+            for item in record:
+                self.assertThat(browser.contents, Contains(item))
+
     def test_scope_documentation_displayed(self):
         """The scope documentation is displayed on the page."""
         browser = self.getUserBrowserAsAdmin()

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-02-14 18:21:01 +0000
+++ lib/lp/services/features/flags.py	2011-02-18 08:19:14 +0000
@@ -6,6 +6,7 @@
     'flag_info',
     'NullFeatureController',
     'undocumented_flags',
+    'value_domain_info',
     ]
 
 
@@ -17,36 +18,56 @@
 
 __metaclass__ = type
 
+
+value_domain_info = sorted([
+    ('boolean',
+     'Any non-empty value is true; an empty value is false.'),
+    ('float',
+     'The flag value is set to the given floating point number.'),
+    ])
+
 # This table of flag name, value domain, and prose documentation is used to
 # generate the web-visible feature flag documentation.
 flag_info = sorted([
+    ('code.branchmergequeue',
+     'boolean',
+     'Enables merge queue pages and lists them on branch pages.',
+     ''),
+    ('code.incremental_diffs.enabled',
+     'boolean',
+     'Shows incremental diffs on merge proposals.',
+     ''),
     ('code.recipes_enabled',
-     '[on|off]',
-     'enable recipes',
-     'off'),
+     'boolean',
+     'Enables source package recipes in the API and UI.',
+     ''),
     ('hard_timeout',
      'float',
-     'sets the hard timeout in seconds',
+     'Sets the hard request timeout in milliseconds.',
      ''),
     ('malone.advanced-subscriptions.enabled',
-     '[on|off]',
-     'enables advanced subscriptions features',
-     'off'),
+     'boolean',
+     'Enables advanced bug subscription features.',
+     ''),
     ('malone.disable_targetnamesearch',
-     '[empty|nonempty]',
-     'If nonempty targetnames are not consulted during text search.',
+     'boolean',
+     'If true, disables consultation of target names during bug text search.',
      ''),
     ('memcache',
-     '[enabled|disabled]',
-     'enables/disables memcache',
+     'boolean',
+     'Enables use of memcached where it is supported.',
      'enabled'),
     ('publicrestrictedlibrarian',
-     '[on|off]',
-     'redirect to private URLs instead of proxying',
-     'off'),
+     'boolean',
+     'Redirects to private librarian files instead of proxying them.',
+     ''),
+    ('soyuz.derived-series-ui.enabled',
+     'boolean',
+     'Enables derivative distributions pages.',
+     ''),
     ('visible_render_time',
-     'empty|nonempty',
-     'enables showing the page render overheads in the login widget',
+     'boolean',
+     'Shows the server-side page render time in the login widget.',
      ''),
     ])
 

=== modified file 'lib/lp/services/features/templates/feature-info.pt'
--- lib/lp/services/features/templates/feature-info.pt	2011-01-20 18:33:25 +0000
+++ lib/lp/services/features/templates/feature-info.pt	2011-02-18 08:19:14 +0000
@@ -29,7 +29,7 @@
     </tbody>
   </table>
 
-  <p>
+  <p/>
   <h2>Undocumented flags</h2>
   These flags were referenced during this process' lifetime but are not
   documented:
@@ -38,7 +38,24 @@
   </strong>
   <strong tal:content="view/undocumented_flags">list of flags</strong>
 
-  <p>
+  <p/>
+  <h2>Value domains</h2>
+  <table class="listing">
+    <thead>
+      <tr>
+        <th>Name</th>
+        <th>Description</th>
+      </tr>
+    </thead>
+    <tbody>
+      <tr tal:repeat="info view/value_domain_info">
+        <td tal:content="info/name">name here</td>
+        <td tal:content="info/description">description here</td>
+      </tr>
+    </tbody>
+  </table>
+
+  <p/>
   <h2>Scopes</h2>
 
   The table below describes the currently available scopes.  The first column
@@ -46,7 +63,7 @@
   "pageid:foo" scopes match the regex "pageid:") and the second gives a
   description of when the scope is active.
 
-  <p>
+  <p/>
   <table class="listing">
     <thead>
       <tr>
@@ -62,7 +79,7 @@
     </tbody>
   </table>
 
-  <p>
+  <p/>
   <h2>Undocumented scopes</h2>
   These scopes were referenced during this process' lifetime but are not
   documented:

=== modified file 'lib/lp/services/features/templates/feature-rules.pt'
--- lib/lp/services/features/templates/feature-rules.pt	2011-01-26 22:07:08 +0000
+++ lib/lp/services/features/templates/feature-rules.pt	2011-02-18 08:19:14 +0000
@@ -18,8 +18,12 @@
         </p>
         <tal:diff replace="structure view/diff"/>
       </div>
+      <p>
       For more information about the available feature flags and scopes see
-      the <a href="+feature-info">feature flag info</a>.
+      the <a href="+feature-info">feature flag info</a>. For other
+      documentation on feature flags, see the
+      <a href="https://dev.launchpad.net/FeatureFlags">development wiki</a>.
+      </p>
     </div>
   </div>
 </div>

=== modified file 'lib/lp/services/memcache/client.py'
--- lib/lp/services/memcache/client.py	2010-09-12 05:52:41 +0000
+++ lib/lp/services/memcache/client.py	2011-02-18 08:19:14 +0000
@@ -36,7 +36,11 @@
 
     @property
     def _enabled(self):
-        return features.getFeatureFlag('memcache') != 'disabled'
+        configured_value = features.getFeatureFlag('memcache')
+        if configured_value is None:
+            return True
+        else:
+            return configured_value
 
     def get(self, key):
         if not self._enabled:

=== modified file 'lib/lp/services/memcache/doc/tales-cache.txt'
--- lib/lp/services/memcache/doc/tales-cache.txt	2010-11-02 17:47:05 +0000
+++ lib/lp/services/memcache/doc/tales-cache.txt	2011-02-18 08:19:14 +0000
@@ -394,10 +394,10 @@
 ---------
 
 Memcache in templates can be disabled entirely by setting the memcache flag to
-'disabled'.
+the empty string.
 
     >>> from lp.services.features.testing import FeatureFixture
-    >>> fixture = FeatureFixture({'memcache': 'disabled'})
+    >>> fixture = FeatureFixture({'memcache': ''})
     >>> fixture.setUp()
 
 And now what cached before will not cache.