Merge lp:~thumper/launchpad/recipe-feature-flag into lp:launchpad

[Tim Penhey]

[r=lifeless,wallyworld][bug=720503] Use feature flags for recipe enablement and beta message showing

Use feature flags exclusively for controlling recipes.

Generally replaced the recipes_enabled method with a direct check
of the feature flag.

There is no point checking now in requestBuild.

lib/lp/code/browser/branch.py
 - the link is only shown on the template if the feature flag is set
   so no need to check it twice

[Martin Pool (mbp)]

Just one thing about this in the context of wgrant's work: the way you're using this with just 'if getFeatureFlag()' is just going to go off Python's cast of the value to boolean, ie any non-empty string counts as true. <https://bugs.launchpad.net/launchpad/+bug/719182> So I wouldn't describe them as true/false for now.

While you're changing it, would you like to the existing flag to code.recipes.enabled for consistency? You would have to ask the losas to duplicate the existing rules.

review: Needs Fixing

[Ian Booth (wallyworld)]

Nice cleanup work.

I am wondering if:

if not getFeatureFlag(RECIPE_ENABLED_FLAG):

is correct? Doesn't getFeatureFlag(RECIPE_ENABLED_FLAG) return "off" or "on" ie a string?

Also, why "code.recipes_enabled" and not "code.recipes.enabled" ?

review: Needs Information

[Ian Booth (wallyworld)]

> Nice cleanup work.
>
> I am wondering if:
>
> if not getFeatureFlag(RECIPE_ENABLED_FLAG):
>
> is correct? Doesn't getFeatureFlag(RECIPE_ENABLED_FLAG) return "off" or "on"
> ie a string?
>
> Also, why "code.recipes_enabled" and not "code.recipes.enabled" ?

Oh sorry, I had this open in my browser for a while and didn't see before I submitted that poolie had made similar comments.

[Tim Penhey (thumper)]

On Thu, 17 Feb 2011 17:29:53 you wrote:
> Review: Needs Fixing
> Just one thing about this in the context of wgrant's work: the way you're
> using this with just 'if getFeatureFlag()' is just going to go off
> Python's cast of the value to boolean, ie any non-empty string counts as
> true. <https://bugs.launchpad.net/launchpad/+bug/719182> So I wouldn't
> describe them as true/false for now.

having true/false is the same as on/off

They will operate exactly the same way. I had the following discussion with
lifeless about that bug (didn't realise the actual bug at the time).

<thumper> lifeless: what do you think about converting feature flag values of
"off" and "false" to False ?
<lifeless> thumper: i'm pretty unkeen
<lifeless> thumper: what issue are you trying to solve?
<thumper> lifeless: I'm adding a feature flag "code.recipe.beta" to control the
message and the beta icon
<thumper> lifeless: so we'll add a rule that says code.recipe.beta -> "on"
<thumper> lifeless: but if we set a rule to say code.recipe.beta -> 'off'
<lifeless> thumper: theres a rule for this already isn't there ?
<thumper> lifeless: I want it to work
<thumper> no
<thumper> we have code.recipe_enabled
<thumper> which is different
<thumper> I'd like to rename that to "code.recipe.enabled"
<thumper> enabled controls whether we barf on recipe actions
<lifeless> thumper: so this is to let you go to non-beta without tying it to a
deploy ?
<thumper> beta is just to show the messages
<thumper> yes
<thumper> exactly
<lifeless> and then you'll delete both rules entirely.
<thumper> aye
<wallyworld> thumper: can you tell me where our existing web service api tests
are? i've had a boy look but can't see them
<lifeless> I would just do the current thing - 'if <thing>: ... '
<thumper> yeah, I know I _can)
<lifeless> thumper: the default is None/'', and you can add rules of ''
<thumper> I just think it would make it more clear
<lifeless> I have think it makes it more awkward
<thumper> getFeatureFlag('some.feature') should be False if some.feature ==
"off" or "false"
<lifeless> Not at that layer
<lifeless> and I really don't see the argument for a higher layer at the
moment
<lifeless> it seems like cruft
<thumper> find
<thumper> fine
<thumper> we can agree to disagree and I'll not change it
<lifeless> I agree that there is some confusion

> While you're changing it, would you like to the existing flag to
> code.recipes.enabled for consistency? You would have to ask the losas to
> duplicate the existing rules.

I did seriously consider this. But we are likely to be removing both rules
within two weeks, so I didn't bother.

[Tim Penhey (thumper)]

On Thu, 17 Feb 2011 17:36:07 Ian Booth wrote:
> Review: Needs Information
> Nice cleanup work.
>
> I am wondering if:
>
> if not getFeatureFlag(RECIPE_ENABLED_FLAG):
>
> is correct? Doesn't getFeatureFlag(RECIPE_ENABLED_FLAG) return "off" or
> "on" ie a string?
>
> Also, why "code.recipes_enabled" and not "code.recipes.enabled" ?

Because "code.recipes_enabled" is what it is now, and has established rules.

I just decided to limit the changes (even though the old one doesn't follow
the suggested naming guidelines).

[Ian Booth (wallyworld)]

Sorry, I'm confused. If the value of the feature flag is "off", then:

if not getFeatureFlag(RECIPE_ENABLED_FLAG):
    do something

will not work as expected since both "off" and "on" evaluate to True in a conditional expression.
So shouldn't the above code be:

if getFeatureFlag(RECIPE_ENABLED_FLAG) == "off":
    do something

How does the code as it exists in the current mp correct?

review: Needs Information

[Tim Penhey (thumper)]

On Thu, 17 Feb 2011 18:13:52 Ian Booth wrote:
> Review: Needs Information
> Sorry, I'm confused. If the value of the feature flag is "off", then:
>
> if not getFeatureFlag(RECIPE_ENABLED_FLAG):
> do something
>
> will not work as expected since both "off" and "on" evaluate to True in a
> conditional expression. So shouldn't the above code be:

It will work as expected... as the only value the feature flag will have is
"on".

Then we delete all the checks, then delete the flag.

[Robert Collins (lifeless)]

As Martin notes, the doc for the new flag is inconsistent with wgrants cleanup, but this flag will have a very short lifespan so it really doesn't bother me.

review: Approve

[Ian Booth (wallyworld)]

> On Thu, 17 Feb 2011 18:13:52 Ian Booth wrote:
> > Review: Needs Information
> > Sorry, I'm confused. If the value of the feature flag is "off", then:
> >
> > if not getFeatureFlag(RECIPE_ENABLED_FLAG):
> > do something
> >
> > will not work as expected since both "off" and "on" evaluate to True in a
> > conditional expression. So shouldn't the above code be:
>
> It will work as expected... as the only value the feature flag will have is
> "on".
>
> Then we delete all the checks, then delete the flag.

Ok. I was going by the +feature-info doc which says the default value of the flag is "off". I'll give in to popular opinion but still am not 100% comfortable with the bigger picture implementation issue since it is not obvious or intuitive what the expected behaviour is and even though *this* flag will have a short life, the problem remains for other flags.

review: Approve (code*)

[Martin Pool (mbp)]

On 17 February 2011 18:56, Ian Booth <email address hidden> wrote:
> Ok. I was going by the +feature-info doc which says the default value of the flag is "off". I'll give in to popular opinion but still am not 100% comfortable with the bigger picture implementation issue since it is not obvious or intuitive what the expected behaviour is and even though *this* flag will have a short life, the problem remains for other flags.

You're quite right, I'm just suggesting we tackle that as
<https://bugs.launchpad.net/launchpad/+bug/719182>

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-02-02 15:43:31 +0000
+++ lib/lp/code/browser/branch.py	2011-02-18 08:58:49 +0000
@@ -143,7 +143,6 @@
 from lp.code.interfaces.branchnamespace import IBranchNamespacePolicy
 from lp.code.interfaces.branchtarget import IBranchTarget
 from lp.code.interfaces.codereviewvote import ICodeReviewVoteReference
-from lp.code.interfaces.sourcepackagerecipe import recipes_enabled
 from lp.registry.interfaces.person import (
     IPerson,
     IPersonSet,
@@ -385,10 +384,8 @@
             '+upgrade', 'Upgrade this branch', icon='edit', enabled=enabled)
 
     def create_recipe(self):
-        if not self.context.private and recipes_enabled():
-            enabled = True
-        else:
-            enabled = False
+        # You can't create a recipe for a private branch.
+        enabled = not self.context.private
         text = 'Create packaging recipe'
         return Link('+new-recipe', text, enabled=enabled, icon='add')
 

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2011-02-17 22:49:24 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2011-02-18 08:58:49 +0000
@@ -91,8 +91,10 @@
     ISourcePackageRecipe,
     ISourcePackageRecipeSource,
     MINIMAL_RECIPE_TEXT,
+    RECIPE_BETA_FLAG,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
 from lp.soyuz.model.archive import Archive
 
@@ -185,10 +187,9 @@
     """Default view of a SourcePackageRecipe."""
 
     def initialize(self):
-        # XXX: rockstar: This should be removed when source package recipes
-        # are put into production. spec=sourcepackagerecipes
         super(SourcePackageRecipeView, self).initialize()
-        self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
+        if getFeatureFlag(RECIPE_BETA_FLAG):
+            self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
         recipe = self.context
         if recipe.build_daily and recipe.daily_build_archive is None:
             self.request.response.addWarningNotification(
@@ -524,9 +525,8 @@
 
     def initialize(self):
         super(SourcePackageRecipeAddView, self).initialize()
-        # XXX: rockstar: This should be removed when source package recipes
-        # are put into production. spec=sourcepackagerecipes
-        self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
+        if getFeatureFlag(RECIPE_BETA_FLAG):
+           self.request.response.addWarningNotification(RECIPE_BETA_MESSAGE)
         widget = self.widgets['use_ppa']
         current_value = widget._getFormValue()
         self.use_ppa_existing = render_radio_widget_part(

=== modified file 'lib/lp/code/browser/sourcepackagerecipelisting.py'
--- lib/lp/code/browser/sourcepackagerecipelisting.py	2011-01-20 23:48:36 +0000
+++ lib/lp/code/browser/sourcepackagerecipelisting.py	2011-02-18 08:58:49 +0000
@@ -19,7 +19,8 @@
     LaunchpadView,
     Link,
     )
-from lp.code.interfaces.sourcepackagerecipe import recipes_enabled
+from lp.code.interfaces.sourcepackagerecipe import RECIPE_ENABLED_FLAG
+from lp.services.features import getFeatureFlag
 
 
 class HasRecipesMenuMixin:
@@ -30,7 +31,7 @@
         enabled = False
         if self.context.getRecipes().count():
             enabled = True
-        if not recipes_enabled():
+        if not getFeatureFlag(RECIPE_ENABLED_FLAG):
             enabled = False
         return Link(
             '+recipes', text, icon='info', enabled=enabled, site='code')

=== modified file 'lib/lp/code/browser/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-17 10:10:29 +0000
+++ lib/lp/code/browser/tests/test_sourcepackagerecipe.py	2011-02-18 08:58:49 +0000
@@ -43,13 +43,18 @@
 from lp.code.browser.sourcepackagerecipebuild import (
     SourcePackageRecipeBuildView,
     )
-from lp.code.interfaces.sourcepackagerecipe import MINIMAL_RECIPE_TEXT
+from lp.code.interfaces.sourcepackagerecipe import (
+    MINIMAL_RECIPE_TEXT,
+    RECIPE_BETA_FLAG,
+    RECIPE_ENABLED_FLAG,
+    )
 from lp.code.tests.helpers import recipe_parser_newest_version
 from lp.registry.interfaces.person import (
     IPersonSet,
     TeamSubscriptionPolicy,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.features.testing import FeatureFixture
 from lp.services.propertycache import clear_property_cache
 from lp.soyuz.model.processor import ProcessorFamily
 from lp.testing import (
@@ -194,6 +199,13 @@
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestSourcePackageRecipeAddView, self).setUp()
+        self.useFixture(
+            FeatureFixture(
+                {RECIPE_ENABLED_FLAG: 'on',
+                 RECIPE_BETA_FLAG: 'true'}))
+
     def makeBranch(self):
         product = self.factory.makeProduct(
             name='ratatouille', displayname='Ratatouille')

=== modified file 'lib/lp/code/interfaces/sourcepackagerecipe.py'
--- lib/lp/code/interfaces/sourcepackagerecipe.py	2011-02-17 02:23:25 +0000
+++ lib/lp/code/interfaces/sourcepackagerecipe.py	2011-02-18 08:58:49 +0000
@@ -14,7 +14,8 @@
     'ISourcePackageRecipeData',
     'ISourcePackageRecipeSource',
     'MINIMAL_RECIPE_TEXT',
-    'recipes_enabled',
+    'RECIPE_BETA_FLAG',
+    'RECIPE_ENABLED_FLAG',
     ]
 
 
@@ -57,7 +58,6 @@
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.role import IHasOwner
-from lp.services import features
 from lp.services.fields import (
     Description,
     PersonChoice,
@@ -66,6 +66,9 @@
 from lp.soyuz.interfaces.archive import IArchive
 
 
+RECIPE_ENABLED_FLAG = u'code.recipes_enabled'
+RECIPE_BETA_FLAG = u'code.recipes.beta'
+
 MINIMAL_RECIPE_TEXT = dedent(u'''\
     # bzr-builder format 0.3 deb-version {debupstream}-0~{revno}
     %s
@@ -221,13 +224,3 @@
 
     def exists(owner, name):
         """Check to see if a recipe by the same name and owner exists."""
-
-
-def recipes_enabled():
-    """Return True if recipes are enabled."""
-    # Features win:
-    if features.getFeatureFlag(u'code.recipes_enabled'):
-        return True
-    if not config.build_from_branch.enabled:
-        return False
-    return True

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2011-02-17 22:45:31 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2011-02-18 08:58:49 +0000
@@ -62,7 +62,6 @@
     ISourcePackageRecipe,
     ISourcePackageRecipeData,
     ISourcePackageRecipeSource,
-    recipes_enabled,
     )
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildSource,
@@ -248,8 +247,6 @@
     def requestBuild(self, archive, requester, distroseries, pocket,
                      manual=False):
         """See `ISourcePackageRecipe`."""
-        if not recipes_enabled():
-            raise ValueError('Source package recipe builds disabled.')
         if not archive.is_ppa:
             raise NonPPABuildRequest
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-02-17 22:45:31 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2011-02-18 08:58:49 +0000
@@ -39,7 +39,6 @@
     ISourcePackageRecipe,
     ISourcePackageRecipeSource,
     MINIMAL_RECIPE_TEXT,
-    recipes_enabled,
     )
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuild,
@@ -72,7 +71,6 @@
     login,
     login_person,
     person_logged_in,
-    set_feature_flag,
     TestCaseWithFactory,
     ws_object,
     )
@@ -376,24 +374,6 @@
         queue_record.score()
         self.assertEqual(2705, queue_record.lastscore)
 
-    def test_requestBuildHonoursConfig(self):
-        recipe = self.factory.makeSourcePackageRecipe()
-        (distroseries,) = list(recipe.distroseries)
-        ppa = self.factory.makeArchive()
-        self.pushConfig('build_from_branch', enabled=False)
-        self.assertRaises(
-            ValueError, recipe.requestBuild, ppa, ppa.owner, distroseries,
-            PackagePublishingPocket.RELEASE)
-
-    def test_recipes_enabled_config(self):
-        self.pushConfig('build_from_branch', enabled=False)
-        self.assertFalse(recipes_enabled())
-
-    def test_recipes_enabled_flag(self):
-        self.pushConfig('build_from_branch', enabled=False)
-        set_feature_flag(u'code.recipes_enabled', u'on')
-        self.assertTrue(recipes_enabled())
-
     def test_requestBuildRejectsOverQuota(self):
         """Build requests that exceed quota raise an exception."""
         requester = self.factory.makePerson(name='requester')

=== modified file 'lib/lp/code/stories/sourcepackagerecipes/xx-recipe-listings.txt'
--- lib/lp/code/stories/sourcepackagerecipes/xx-recipe-listings.txt	2010-08-23 04:48:17 +0000
+++ lib/lp/code/stories/sourcepackagerecipes/xx-recipe-listings.txt	2011-02-18 08:58:49 +0000
@@ -5,7 +5,10 @@
 Pages that want to display lists of recipes use the recipe-listing
 page template, and views derived from RecipeListingView.
 
-
+    >>> from lp.services.features.testing import FeatureFixture
+    >>> fixture = FeatureFixture(
+    ...     {'code.recipes_enabled': 'on', 'code.recipes.beta': 'true'})
+    >>> fixture.setUp()
     >>> def print_recipe_listing_head(browser):
     ...     table = find_tag_by_id(browser.contents, 'recipetable')
     ...     for row in table.thead.fetch('tr'):
@@ -135,3 +138,5 @@
     generic-string... lp://dev/... ...
     generic-string... lp://dev/... ...
     generic-string... lp://dev/... ...
+
+    >>> fixture.cleanUp()

=== modified file 'lib/lp/code/templates/branch-index.pt'
--- lib/lp/code/templates/branch-index.pt	2010-11-08 14:11:57 +0000
+++ lib/lp/code/templates/branch-index.pt	2011-02-18 08:58:49 +0000
@@ -111,7 +111,7 @@
            replace="structure context/@@++branch-pending-merges" />
       <tal:branch-recipes
            replace="structure context/@@++branch-recipes"
-           condition="modules/lp.code.interfaces.sourcepackagerecipe/recipes_enabled"
+           condition="request/features/code.recipes_enabled"
       />
       <tal:related-bugs-specs
            replace="structure context/@@++branch-related-bugs-specs" />

=== modified file 'lib/lp/code/templates/branch-recipes.pt'
--- lib/lp/code/templates/branch-recipes.pt	2010-10-31 22:27:36 +0000
+++ lib/lp/code/templates/branch-recipes.pt	2011-02-18 08:58:49 +0000
@@ -25,7 +25,8 @@
       tal:condition="link/enabled"
       tal:replace="structure link/render"
       />
-    <img src="/@@/beta" alt="beta" title="This feature is in beta" />
+    <img src="/@@/beta" alt="beta" title="This feature is in beta"
+         tal:condition="request/features/code.recipes.beta"/>
   </div>
 
 </div>

=== modified file 'lib/lp/code/templates/branchmergequeue-listing.pt'
--- lib/lp/code/templates/branchmergequeue-listing.pt	2010-11-01 12:35:07 +0000
+++ lib/lp/code/templates/branchmergequeue-listing.pt	2011-02-18 08:58:49 +0000
@@ -10,13 +10,13 @@
 
   <div metal:fill-slot="main">
 
-    <div tal:condition="not: features/code.branchmergequeue">
+    <div tal:condition="not: request/features/code.branchmergequeue">
         <em>
          No merge queues
         </em>
     </div>
 
-    <div tal:condition="features/code.branchmergequeue">
+    <div tal:condition="request/features/code.branchmergequeue">
 
         <tal:has-queues condition="view/mergequeue_count">
 

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2011-02-15 08:04:21 +0000
+++ lib/lp/services/features/flags.py	2011-02-18 08:58:49 +0000
@@ -41,6 +41,10 @@
      'boolean',
      'Enables source package recipes in the API and UI.',
      ''),
+    ('code.recipes.beta',
+     'boolean',
+     'True if recipes are still in beta',
+     ''),
     ('hard_timeout',
      'float',
      'Sets the hard request timeout in milliseconds.',