launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

The proposal to merge lp:~jtv/launchpad/bug-735319 into lp:launchpad has been updated.

Description changed to:

= 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

For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-735319/+merge/53578
-- 
https://code.launchpad.net/~jtv/launchpad/bug-735319/+merge/53578
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-735319 into lp:launchpad.