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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
