Merge lp:~wgrant/launchpad/bug-708999-ff-docs into lp:launchpad
- bug-708999-ff-docs
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Robert Collins |
Approved revision: | no longer in the source branch. |
Merged at revision: | 12406 |
Proposed branch: | lp:~wgrant/launchpad/bug-708999-ff-docs |
Merge into: | lp:launchpad |
Diff against target: |
276 lines (+89/-27) 8 files modified
lib/canonical/launchpad/browser/librarian.py (+3/-3) lib/lp/services/features/browser/info.py (+7/-0) lib/lp/services/features/browser/tests/test_feature_info.py (+9/-0) lib/lp/services/features/flags.py (+37/-16) lib/lp/services/features/templates/feature-info.pt (+21/-4) lib/lp/services/features/templates/feature-rules.pt (+5/-1) lib/lp/services/memcache/client.py (+5/-1) lib/lp/services/memcache/doc/tales-cache.txt (+2/-2) |
To merge this branch: | bzr merge lp:~wgrant/launchpad/bug-708999-ff-docs |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Approve | ||
Review via email: mp+49758@code.launchpad.net |
Commit message
[r=lifeless][bug=708999] Document code.branchmerg
Description of the change
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) wrote : | # |
William Grant (wgrant) wrote : | # |
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-
The new pages can be seen at <http://
Only one flag will require changes on production: "memcache pageid:
Martin Pool (mbp) wrote : | # |
<poolie> wgrant, also hard_timeout, contrary to its docs, seems to be
in milliseconds
not seconds
Aaron Bentley (abentley) 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.
William Grant (wgrant) wrote : | # |
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) wrote : | # |
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:/
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) wrote : | # |
FWIW I really like the pithy nature of what we have at the moment.
Please don't lose that.
Martin Pool (mbp) 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?
Robert Collins (lifeless) 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.
Martin Pool (mbp) wrote : | # |
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) wrote : | # |
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) wrote : | # |
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) wrote : | # |
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) : | # |
Preview Diff
1 | === modified file 'lib/canonical/launchpad/browser/librarian.py' | |||
2 | --- lib/canonical/launchpad/browser/librarian.py 2011-01-20 23:55:37 +0000 | |||
3 | +++ lib/canonical/launchpad/browser/librarian.py 2011-02-18 08:19:14 +0000 | |||
4 | @@ -83,8 +83,8 @@ | |||
5 | 83 | 83 | ||
6 | 84 | If the file is public, it will redirect to the files http url. | 84 | If the file is public, it will redirect to the files http url. |
7 | 85 | 85 | ||
10 | 86 | Otherwise if the feature flag publicrestrictedlibrarian is set to 'on' | 86 | Otherwise if the feature flag publicrestrictedlibrarian is true this will |
11 | 87 | this will allocate a token and redirect to the aliases private url. | 87 | allocate a token and redirect to the aliases private url. |
12 | 88 | 88 | ||
13 | 89 | Otherwise it will proxy the file in the appserver. | 89 | Otherwise it will proxy the file in the appserver. |
14 | 90 | 90 | ||
15 | @@ -179,7 +179,7 @@ | |||
16 | 179 | if not self.context.restricted: | 179 | if not self.context.restricted: |
17 | 180 | # Public file, just point the client at the right place. | 180 | # Public file, just point the client at the right place. |
18 | 181 | return RedirectionView(self.context.http_url, self.request), () | 181 | return RedirectionView(self.context.http_url, self.request), () |
20 | 182 | if getFeatureFlag(u'publicrestrictedlibrarian') != 'on': | 182 | if not getFeatureFlag(u'publicrestrictedlibrarian'): |
21 | 183 | # Restricted file and we have not enabled the public | 183 | # Restricted file and we have not enabled the public |
22 | 184 | # restricted librarian yet :- deliver inline. | 184 | # restricted librarian yet :- deliver inline. |
23 | 185 | self._when_streaming() | 185 | self._when_streaming() |
24 | 186 | 186 | ||
25 | === modified file 'lib/lp/services/features/browser/info.py' | |||
26 | --- lib/lp/services/features/browser/info.py 2011-01-26 22:07:08 +0000 | |||
27 | +++ lib/lp/services/features/browser/info.py 2011-02-18 08:19:14 +0000 | |||
28 | @@ -15,6 +15,7 @@ | |||
29 | 15 | from lp.services.features.flags import ( | 15 | from lp.services.features.flags import ( |
30 | 16 | flag_info, | 16 | flag_info, |
31 | 17 | undocumented_flags, | 17 | undocumented_flags, |
32 | 18 | value_domain_info, | ||
33 | 18 | ) | 19 | ) |
34 | 19 | from lp.services.features.scopes import ( | 20 | from lp.services.features.scopes import ( |
35 | 20 | HANDLERS, | 21 | HANDLERS, |
36 | @@ -25,6 +26,7 @@ | |||
37 | 25 | 26 | ||
38 | 26 | # Named tuples to use when passing flag and scope data to the template. | 27 | # Named tuples to use when passing flag and scope data to the template. |
39 | 27 | Flag = namedtuple('Flag', ('name', 'domain', 'description', 'default')) | 28 | Flag = namedtuple('Flag', ('name', 'domain', 'description', 'default')) |
40 | 29 | ValueDomain = namedtuple('ValueDomain', ('name', 'description')) | ||
41 | 28 | Scope = namedtuple('Scope', ('regex', 'description')) | 30 | Scope = namedtuple('Scope', ('regex', 'description')) |
42 | 29 | 31 | ||
43 | 30 | 32 | ||
44 | @@ -45,6 +47,11 @@ | |||
45 | 45 | return ', '.join(undocumented_flags) | 47 | return ', '.join(undocumented_flags) |
46 | 46 | 48 | ||
47 | 47 | @property | 49 | @property |
48 | 50 | def value_domain_info(self): | ||
49 | 51 | """A list of flags as named tuples, ready to be rendered.""" | ||
50 | 52 | return map(ValueDomain._make, value_domain_info) | ||
51 | 53 | |||
52 | 54 | @property | ||
53 | 48 | def undocumented_scopes(self): | 55 | def undocumented_scopes(self): |
54 | 49 | """Scope names referenced during process lifetime but not documented. | 56 | """Scope names referenced during process lifetime but not documented. |
55 | 50 | """ | 57 | """ |
56 | 51 | 58 | ||
57 | === modified file 'lib/lp/services/features/browser/tests/test_feature_info.py' | |||
58 | --- lib/lp/services/features/browser/tests/test_feature_info.py 2011-01-26 22:07:08 +0000 | |||
59 | +++ lib/lp/services/features/browser/tests/test_feature_info.py 2011-02-18 08:19:14 +0000 | |||
60 | @@ -19,6 +19,7 @@ | |||
61 | 19 | flag_info, | 19 | flag_info, |
62 | 20 | NullFeatureController, | 20 | NullFeatureController, |
63 | 21 | undocumented_flags, | 21 | undocumented_flags, |
64 | 22 | value_domain_info, | ||
65 | 22 | ) | 23 | ) |
66 | 23 | from lp.services.features.scopes import ( | 24 | from lp.services.features.scopes import ( |
67 | 24 | HANDLERS, | 25 | HANDLERS, |
68 | @@ -66,6 +67,14 @@ | |||
69 | 66 | for item in record: | 67 | for item in record: |
70 | 67 | self.assertThat(browser.contents, Contains(item)) | 68 | self.assertThat(browser.contents, Contains(item)) |
71 | 68 | 69 | ||
72 | 70 | def test_value_domain_documentation_displayed(self): | ||
73 | 71 | """The value domain documentation is displayed on the page.""" | ||
74 | 72 | browser = self.getUserBrowserAsAdmin() | ||
75 | 73 | browser.open(self.getFeatureInfoUrl()) | ||
76 | 74 | for record in value_domain_info: | ||
77 | 75 | for item in record: | ||
78 | 76 | self.assertThat(browser.contents, Contains(item)) | ||
79 | 77 | |||
80 | 69 | def test_scope_documentation_displayed(self): | 78 | def test_scope_documentation_displayed(self): |
81 | 70 | """The scope documentation is displayed on the page.""" | 79 | """The scope documentation is displayed on the page.""" |
82 | 71 | browser = self.getUserBrowserAsAdmin() | 80 | browser = self.getUserBrowserAsAdmin() |
83 | 72 | 81 | ||
84 | === modified file 'lib/lp/services/features/flags.py' | |||
85 | --- lib/lp/services/features/flags.py 2011-02-14 18:21:01 +0000 | |||
86 | +++ lib/lp/services/features/flags.py 2011-02-18 08:19:14 +0000 | |||
87 | @@ -6,6 +6,7 @@ | |||
88 | 6 | 'flag_info', | 6 | 'flag_info', |
89 | 7 | 'NullFeatureController', | 7 | 'NullFeatureController', |
90 | 8 | 'undocumented_flags', | 8 | 'undocumented_flags', |
91 | 9 | 'value_domain_info', | ||
92 | 9 | ] | 10 | ] |
93 | 10 | 11 | ||
94 | 11 | 12 | ||
95 | @@ -17,36 +18,56 @@ | |||
96 | 17 | 18 | ||
97 | 18 | __metaclass__ = type | 19 | __metaclass__ = type |
98 | 19 | 20 | ||
99 | 21 | |||
100 | 22 | value_domain_info = sorted([ | ||
101 | 23 | ('boolean', | ||
102 | 24 | 'Any non-empty value is true; an empty value is false.'), | ||
103 | 25 | ('float', | ||
104 | 26 | 'The flag value is set to the given floating point number.'), | ||
105 | 27 | ]) | ||
106 | 28 | |||
107 | 20 | # This table of flag name, value domain, and prose documentation is used to | 29 | # This table of flag name, value domain, and prose documentation is used to |
108 | 21 | # generate the web-visible feature flag documentation. | 30 | # generate the web-visible feature flag documentation. |
109 | 22 | flag_info = sorted([ | 31 | flag_info = sorted([ |
110 | 32 | ('code.branchmergequeue', | ||
111 | 33 | 'boolean', | ||
112 | 34 | 'Enables merge queue pages and lists them on branch pages.', | ||
113 | 35 | ''), | ||
114 | 36 | ('code.incremental_diffs.enabled', | ||
115 | 37 | 'boolean', | ||
116 | 38 | 'Shows incremental diffs on merge proposals.', | ||
117 | 39 | ''), | ||
118 | 23 | ('code.recipes_enabled', | 40 | ('code.recipes_enabled', |
122 | 24 | '[on|off]', | 41 | 'boolean', |
123 | 25 | 'enable recipes', | 42 | 'Enables source package recipes in the API and UI.', |
124 | 26 | 'off'), | 43 | ''), |
125 | 27 | ('hard_timeout', | 44 | ('hard_timeout', |
126 | 28 | 'float', | 45 | 'float', |
128 | 29 | 'sets the hard timeout in seconds', | 46 | 'Sets the hard request timeout in milliseconds.', |
129 | 30 | ''), | 47 | ''), |
130 | 31 | ('malone.advanced-subscriptions.enabled', | 48 | ('malone.advanced-subscriptions.enabled', |
134 | 32 | '[on|off]', | 49 | 'boolean', |
135 | 33 | 'enables advanced subscriptions features', | 50 | 'Enables advanced bug subscription features.', |
136 | 34 | 'off'), | 51 | ''), |
137 | 35 | ('malone.disable_targetnamesearch', | 52 | ('malone.disable_targetnamesearch', |
140 | 36 | '[empty|nonempty]', | 53 | 'boolean', |
141 | 37 | 'If nonempty targetnames are not consulted during text search.', | 54 | 'If true, disables consultation of target names during bug text search.', |
142 | 38 | ''), | 55 | ''), |
143 | 39 | ('memcache', | 56 | ('memcache', |
146 | 40 | '[enabled|disabled]', | 57 | 'boolean', |
147 | 41 | 'enables/disables memcache', | 58 | 'Enables use of memcached where it is supported.', |
148 | 42 | 'enabled'), | 59 | 'enabled'), |
149 | 43 | ('publicrestrictedlibrarian', | 60 | ('publicrestrictedlibrarian', |
153 | 44 | '[on|off]', | 61 | 'boolean', |
154 | 45 | 'redirect to private URLs instead of proxying', | 62 | 'Redirects to private librarian files instead of proxying them.', |
155 | 46 | 'off'), | 63 | ''), |
156 | 64 | ('soyuz.derived-series-ui.enabled', | ||
157 | 65 | 'boolean', | ||
158 | 66 | 'Enables derivative distributions pages.', | ||
159 | 67 | ''), | ||
160 | 47 | ('visible_render_time', | 68 | ('visible_render_time', |
163 | 48 | 'empty|nonempty', | 69 | 'boolean', |
164 | 49 | 'enables showing the page render overheads in the login widget', | 70 | 'Shows the server-side page render time in the login widget.', |
165 | 50 | ''), | 71 | ''), |
166 | 51 | ]) | 72 | ]) |
167 | 52 | 73 | ||
168 | 53 | 74 | ||
169 | === modified file 'lib/lp/services/features/templates/feature-info.pt' | |||
170 | --- lib/lp/services/features/templates/feature-info.pt 2011-01-20 18:33:25 +0000 | |||
171 | +++ lib/lp/services/features/templates/feature-info.pt 2011-02-18 08:19:14 +0000 | |||
172 | @@ -29,7 +29,7 @@ | |||
173 | 29 | </tbody> | 29 | </tbody> |
174 | 30 | </table> | 30 | </table> |
175 | 31 | 31 | ||
177 | 32 | <p> | 32 | <p/> |
178 | 33 | <h2>Undocumented flags</h2> | 33 | <h2>Undocumented flags</h2> |
179 | 34 | These flags were referenced during this process' lifetime but are not | 34 | These flags were referenced during this process' lifetime but are not |
180 | 35 | documented: | 35 | documented: |
181 | @@ -38,7 +38,24 @@ | |||
182 | 38 | </strong> | 38 | </strong> |
183 | 39 | <strong tal:content="view/undocumented_flags">list of flags</strong> | 39 | <strong tal:content="view/undocumented_flags">list of flags</strong> |
184 | 40 | 40 | ||
186 | 41 | <p> | 41 | <p/> |
187 | 42 | <h2>Value domains</h2> | ||
188 | 43 | <table class="listing"> | ||
189 | 44 | <thead> | ||
190 | 45 | <tr> | ||
191 | 46 | <th>Name</th> | ||
192 | 47 | <th>Description</th> | ||
193 | 48 | </tr> | ||
194 | 49 | </thead> | ||
195 | 50 | <tbody> | ||
196 | 51 | <tr tal:repeat="info view/value_domain_info"> | ||
197 | 52 | <td tal:content="info/name">name here</td> | ||
198 | 53 | <td tal:content="info/description">description here</td> | ||
199 | 54 | </tr> | ||
200 | 55 | </tbody> | ||
201 | 56 | </table> | ||
202 | 57 | |||
203 | 58 | <p/> | ||
204 | 42 | <h2>Scopes</h2> | 59 | <h2>Scopes</h2> |
205 | 43 | 60 | ||
206 | 44 | The table below describes the currently available scopes. The first column | 61 | The table below describes the currently available scopes. The first column |
207 | @@ -46,7 +63,7 @@ | |||
208 | 46 | "pageid:foo" scopes match the regex "pageid:") and the second gives a | 63 | "pageid:foo" scopes match the regex "pageid:") and the second gives a |
209 | 47 | description of when the scope is active. | 64 | description of when the scope is active. |
210 | 48 | 65 | ||
212 | 49 | <p> | 66 | <p/> |
213 | 50 | <table class="listing"> | 67 | <table class="listing"> |
214 | 51 | <thead> | 68 | <thead> |
215 | 52 | <tr> | 69 | <tr> |
216 | @@ -62,7 +79,7 @@ | |||
217 | 62 | </tbody> | 79 | </tbody> |
218 | 63 | </table> | 80 | </table> |
219 | 64 | 81 | ||
221 | 65 | <p> | 82 | <p/> |
222 | 66 | <h2>Undocumented scopes</h2> | 83 | <h2>Undocumented scopes</h2> |
223 | 67 | These scopes were referenced during this process' lifetime but are not | 84 | These scopes were referenced during this process' lifetime but are not |
224 | 68 | documented: | 85 | documented: |
225 | 69 | 86 | ||
226 | === modified file 'lib/lp/services/features/templates/feature-rules.pt' | |||
227 | --- lib/lp/services/features/templates/feature-rules.pt 2011-01-26 22:07:08 +0000 | |||
228 | +++ lib/lp/services/features/templates/feature-rules.pt 2011-02-18 08:19:14 +0000 | |||
229 | @@ -18,8 +18,12 @@ | |||
230 | 18 | </p> | 18 | </p> |
231 | 19 | <tal:diff replace="structure view/diff"/> | 19 | <tal:diff replace="structure view/diff"/> |
232 | 20 | </div> | 20 | </div> |
233 | 21 | <p> | ||
234 | 21 | For more information about the available feature flags and scopes see | 22 | For more information about the available feature flags and scopes see |
236 | 22 | the <a href="+feature-info">feature flag info</a>. | 23 | the <a href="+feature-info">feature flag info</a>. For other |
237 | 24 | documentation on feature flags, see the | ||
238 | 25 | <a href="https://dev.launchpad.net/FeatureFlags">development wiki</a>. | ||
239 | 26 | </p> | ||
240 | 23 | </div> | 27 | </div> |
241 | 24 | </div> | 28 | </div> |
242 | 25 | </div> | 29 | </div> |
243 | 26 | 30 | ||
244 | === modified file 'lib/lp/services/memcache/client.py' | |||
245 | --- lib/lp/services/memcache/client.py 2010-09-12 05:52:41 +0000 | |||
246 | +++ lib/lp/services/memcache/client.py 2011-02-18 08:19:14 +0000 | |||
247 | @@ -36,7 +36,11 @@ | |||
248 | 36 | 36 | ||
249 | 37 | @property | 37 | @property |
250 | 38 | def _enabled(self): | 38 | def _enabled(self): |
252 | 39 | return features.getFeatureFlag('memcache') != 'disabled' | 39 | configured_value = features.getFeatureFlag('memcache') |
253 | 40 | if configured_value is None: | ||
254 | 41 | return True | ||
255 | 42 | else: | ||
256 | 43 | return configured_value | ||
257 | 40 | 44 | ||
258 | 41 | def get(self, key): | 45 | def get(self, key): |
259 | 42 | if not self._enabled: | 46 | if not self._enabled: |
260 | 43 | 47 | ||
261 | === modified file 'lib/lp/services/memcache/doc/tales-cache.txt' | |||
262 | --- lib/lp/services/memcache/doc/tales-cache.txt 2010-11-02 17:47:05 +0000 | |||
263 | +++ lib/lp/services/memcache/doc/tales-cache.txt 2011-02-18 08:19:14 +0000 | |||
264 | @@ -394,10 +394,10 @@ | |||
265 | 394 | --------- | 394 | --------- |
266 | 395 | 395 | ||
267 | 396 | Memcache in templates can be disabled entirely by setting the memcache flag to | 396 | Memcache in templates can be disabled entirely by setting the memcache flag to |
269 | 397 | 'disabled'. | 397 | the empty string. |
270 | 398 | 398 | ||
271 | 399 | >>> from lp.services.features.testing import FeatureFixture | 399 | >>> from lp.services.features.testing import FeatureFixture |
273 | 400 | >>> fixture = FeatureFixture({'memcache': 'disabled'}) | 400 | >>> fixture = FeatureFixture({'memcache': ''}) |
274 | 401 | >>> fixture.setUp() | 401 | >>> fixture.setUp() |
275 | 402 | 402 | ||
276 | 403 | And now what cached before will not cache. | 403 | And now what cached before will not cache. |
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