Merge lp:~jtv/launchpad/bug-735319 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Jeroen T. Vermeulen | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12632 | ||||
Proposed branch: | lp:~jtv/launchpad/bug-735319 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
728 lines (+297/-82) 14 files modified
lib/canonical/launchpad/doc/librarian.txt (+26/-23) lib/canonical/launchpad/doc/timeout.txt (+8/-7) lib/canonical/launchpad/webapp/publication.py (+1/-1) lib/lp/bugs/browser/tests/test_bugattachment_file_access.py (+6/-6) lib/lp/registry/browser/tests/test_series_views.py (+4/-7) lib/lp/services/features/__init__.py (+21/-2) lib/lp/services/features/scopes.py (+65/-17) lib/lp/services/features/testing.py (+8/-5) lib/lp/services/features/tests/test_flags.py (+5/-4) lib/lp/services/features/tests/test_scopes.py (+67/-0) lib/lp/services/features/webapp.py (+6/-4) lib/lp/services/scripts/base.py (+12/-0) lib/lp/services/scripts/tests/test_feature_controller.py (+61/-0) lib/lp/testing/__init__.py (+7/-6) |
||||
To merge this branch: | bzr merge lp:~jtv/launchpad/bug-735319 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool (community) | code* | Approve | |
Review via email: mp+53578@code.launchpad.net |
Commit message
[r=mbp][bug=735319] Let scripts access feature flags.
Description of the change
= 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.
== 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.
Script scope. You can now write rules that set feature flags in the scope of a particular script, using "script:
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.
}}}
== 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/
lib/lp/
lib/lp/
lib/canonical
lib/lp/
lib/lp/
lib/canonical
lib/canonical
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
./lib/canonical
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
1: narrative uses a moin header.
81: narrative uses a moin header.
117: narrative uses a moin header.
./lib/lp/
135: 'anonymous_
135: 'with_anonymous
154: 'launchpadlib_for' imported but unused
154: 'launchpadlib_
135: 'with_person_
135: 'person_logged_in' imported but unused
154: 'oauth_
135: 'login_celebrity' imported but unused
135: 'with_celebrity
153: 'test_tales' imported but unused
135: 'celebrity_
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
small notes - not a code review.
We already have a 'don't start script' feature that doesn't require
editing cron scripts - and *unlike* one based on feature flags, also
does not require db access (which is important for db deploys). Please
delete that part of your patch, because I am confident we'll have
unintended downtime if we have a db based duplicate facility.
Only LOSAs can set feature flags, so setting e.g. verbosity via a
feature flag is still a losa interrupt. It may be better than editing
cronscripts [or not - I haven't thought about it] : but losa
interruptedness is not a differentiating factor.
The rest sounds lovely.
Cheers,
Rob