Merge ~ack/maas:1949963-api-description-stable-sorting into maas:master
- Git
- lp:~ack/maas
- 1949963-api-description-stable-sorting
- Merge into master
Proposed by
Alberto Donato
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Alberto Donato | ||||
Approved revision: | 6b5ab5590e107aa65deaada1e0e63fb050a6ce4b | ||||
Merge reported by: | MAAS Lander | ||||
Merged at revision: | not available | ||||
Proposed branch: | ~ack/maas:1949963-api-description-stable-sorting | ||||
Merge into: | maas:master | ||||
Diff against target: |
333 lines (+87/-58) 5 files modified
src/maasserver/api/doc.py (+43/-33) src/maasserver/api/doc_handler.py (+3/-2) src/maasserver/forms/settings.py (+14/-17) src/maasserver/forms/tests/test_settings.py (+26/-5) src/maasserver/urls_api.py (+1/-1) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Björn Tillenius | Approve | ||
MAAS Lander | Approve | ||
Review via email: mp+411447@code.launchpad.net |
Commit message
LP:1949963 ensure form choices are formatted in order in doc text
Description of the change
To post a comment you must log in.
Revision history for this message
Alberto Donato (ack) : | # |
Revision history for this message
MAAS Lander (maas-lander) wrote : | # |
UNIT TESTS
-b 1949963-
STATUS: SUCCESS
COMMIT: 6b5ab5590e107aa
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/src/maasserver/api/doc.py b/src/maasserver/api/doc.py | |||
2 | index b22251f..3b777aa 100644 | |||
3 | --- a/src/maasserver/api/doc.py | |||
4 | +++ b/src/maasserver/api/doc.py | |||
5 | @@ -19,6 +19,7 @@ from inspect import getdoc | |||
6 | 19 | from io import StringIO | 19 | from io import StringIO |
7 | 20 | from itertools import zip_longest | 20 | from itertools import zip_longest |
8 | 21 | import json | 21 | import json |
9 | 22 | from operator import itemgetter | ||
10 | 22 | from threading import RLock | 23 | from threading import RLock |
11 | 23 | 24 | ||
12 | 24 | from django.urls import get_resolver | 25 | from django.urls import get_resolver |
13 | @@ -44,25 +45,23 @@ def accumulate_api_resources(resolver, accumulator): | |||
14 | 44 | :rtype: Generator, yielding handlers. | 45 | :rtype: Generator, yielding handlers. |
15 | 45 | """ | 46 | """ |
16 | 46 | 47 | ||
22 | 47 | def p_has_resource_uri(resource): | 48 | def visible(handler): |
23 | 48 | return getattr(resource.handler, "resource_uri", None) is not None | 49 | return bool( |
24 | 49 | 50 | getattr(handler, "resource_uri", None) | |
25 | 50 | def p_is_not_hidden(resource): | 51 | and not getattr(handler, "hidden", False) |
26 | 51 | return getattr(resource.handler, "hidden", False) | 52 | ) |
27 | 52 | 53 | ||
28 | 53 | for pattern in resolver.url_patterns: | 54 | for pattern in resolver.url_patterns: |
29 | 54 | if isinstance(pattern, URLResolver): | 55 | if isinstance(pattern, URLResolver): |
30 | 55 | accumulate_api_resources(pattern, accumulator) | 56 | accumulate_api_resources(pattern, accumulator) |
31 | 56 | elif isinstance(pattern, URLPattern): | 57 | elif isinstance(pattern, URLPattern): |
38 | 57 | if isinstance(pattern.callback, Resource): | 58 | if isinstance(pattern.callback, Resource) and visible( |
39 | 58 | resource = pattern.callback | 59 | pattern.callback.handler |
40 | 59 | if p_has_resource_uri(resource) and not p_is_not_hidden( | 60 | ): |
41 | 60 | resource | 61 | accumulator.add(pattern.callback) |
36 | 61 | ): | ||
37 | 62 | accumulator.add(resource) | ||
42 | 63 | else: | 62 | else: |
43 | 64 | raise AssertionError( | 63 | raise AssertionError( |
45 | 65 | "Not a recognised pattern or resolver: %r" % (pattern,) | 64 | f"Not a recognised pattern or resolver: {pattern!r}" |
46 | 66 | ) | 65 | ) |
47 | 67 | 66 | ||
48 | 68 | 67 | ||
49 | @@ -228,10 +227,18 @@ def describe_actions(handler): | |||
50 | 228 | restful: Indicates if this is a CRUD/ReSTful action. | 227 | restful: Indicates if this is a CRUD/ReSTful action. |
51 | 229 | 228 | ||
52 | 230 | """ | 229 | """ |
54 | 231 | from maasserver.api import support # Circular import. | 230 | from maasserver.api import support |
55 | 232 | 231 | ||
56 | 233 | getname = support.OperationsResource.crudmap.get | 232 | getname = support.OperationsResource.crudmap.get |
58 | 234 | for signature, function in handler.exports.items(): | 233 | |
59 | 234 | actions = [] | ||
60 | 235 | |||
61 | 236 | # ensure stable sorting, accounting for operation being None | ||
62 | 237 | exports = sorted( | ||
63 | 238 | handler.exports.items(), | ||
64 | 239 | key=lambda item: (item[0][0], item[0][1] or ""), | ||
65 | 240 | ) | ||
66 | 241 | for signature, function in exports: | ||
67 | 235 | http_method, operation = signature | 242 | http_method, operation = signature |
68 | 236 | name = getname(http_method) if operation is None else operation | 243 | name = getname(http_method) if operation is None else operation |
69 | 237 | 244 | ||
70 | @@ -276,13 +283,16 @@ def describe_actions(handler): | |||
71 | 276 | ) | 283 | ) |
72 | 277 | doc += ":type %s: %s\n\n " % (pname, param["type"]) | 284 | doc += ":type %s: %s\n\n " % (pname, param["type"]) |
73 | 278 | 285 | ||
80 | 279 | yield dict( | 286 | actions.append( |
81 | 280 | method=http_method, | 287 | { |
82 | 281 | name=name, | 288 | "method": http_method, |
83 | 282 | doc=doc, | 289 | "name": name, |
84 | 283 | op=operation, | 290 | "doc": doc, |
85 | 284 | restful=(operation is None), | 291 | "op": operation, |
86 | 292 | "restful": operation is None, | ||
87 | 293 | } | ||
88 | 285 | ) | 294 | ) |
89 | 295 | return sorted(actions, key=itemgetter("name")) | ||
90 | 286 | 296 | ||
91 | 287 | 297 | ||
92 | 288 | def describe_handler(handler): | 298 | def describe_handler(handler): |
93 | @@ -306,7 +316,7 @@ def describe_handler(handler): | |||
94 | 306 | ) | 316 | ) |
95 | 307 | 317 | ||
96 | 308 | return { | 318 | return { |
98 | 309 | "actions": list(describe_actions(handler)), | 319 | "actions": describe_actions(handler), |
99 | 310 | "doc": getdoc(handler), | 320 | "doc": getdoc(handler), |
100 | 311 | "name": handler.__name__, | 321 | "name": handler.__name__, |
101 | 312 | "params": tuple(uri_params), | 322 | "params": tuple(uri_params), |
102 | @@ -345,26 +355,30 @@ def describe_api(): | |||
103 | 345 | """ | 355 | """ |
104 | 346 | from maasserver import urls_api as urlconf | 356 | from maasserver import urls_api as urlconf |
105 | 347 | 357 | ||
106 | 358 | resources = sorted( | ||
107 | 359 | ( | ||
108 | 360 | describe_resource(resource) | ||
109 | 361 | for resource in find_api_resources(urlconf) | ||
110 | 362 | ), | ||
111 | 363 | key=itemgetter("name"), | ||
112 | 364 | ) | ||
113 | 365 | |||
114 | 348 | # This is the core of it: | 366 | # This is the core of it: |
115 | 349 | description = { | 367 | description = { |
116 | 350 | "doc": "MAAS API", | 368 | "doc": "MAAS API", |
121 | 351 | "resources": [ | 369 | "resources": resources, |
118 | 352 | describe_resource(resource) | ||
119 | 353 | for resource in find_api_resources(urlconf) | ||
120 | 354 | ], | ||
122 | 355 | } | 370 | } |
123 | 356 | |||
124 | 357 | # However, for backward compatibility, add "handlers" as an alias for all | 371 | # However, for backward compatibility, add "handlers" as an alias for all |
125 | 358 | # not-None anon and auth handlers in "resources". | 372 | # not-None anon and auth handlers in "resources". |
126 | 359 | description["handlers"] = [] | 373 | description["handlers"] = [] |
127 | 360 | description["handlers"].extend( | 374 | description["handlers"].extend( |
128 | 361 | resource["anon"] | 375 | resource["anon"] |
130 | 362 | for resource in description["resources"] | 376 | for resource in resources |
131 | 363 | if resource["anon"] is not None | 377 | if resource["anon"] is not None |
132 | 364 | ) | 378 | ) |
133 | 365 | description["handlers"].extend( | 379 | description["handlers"].extend( |
134 | 366 | resource["auth"] | 380 | resource["auth"] |
136 | 367 | for resource in description["resources"] | 381 | for resource in resources |
137 | 368 | if resource["auth"] is not None | 382 | if resource["auth"] is not None |
138 | 369 | ) | 383 | ) |
139 | 370 | 384 | ||
140 | @@ -537,11 +551,7 @@ def hash_canonical(description): | |||
141 | 537 | into a new `hashlib.sha1` object. | 551 | into a new `hashlib.sha1` object. |
142 | 538 | """ | 552 | """ |
143 | 539 | description = describe_canonical(description) | 553 | description = describe_canonical(description) |
149 | 540 | description_as_json = json.dumps(description) | 554 | description_as_json = json.dumps(description).encode("ascii") |
145 | 541 | # Python 3's json.dumps returns a `str`, so encode if necessary. | ||
146 | 542 | if not isinstance(description_as_json, bytes): | ||
147 | 543 | description_as_json = description_as_json.encode("ascii") | ||
148 | 544 | # We /could/ instead pass a hashing object in and call .update()... | ||
150 | 545 | return hashlib.sha1(description_as_json) | 555 | return hashlib.sha1(description_as_json) |
151 | 546 | 556 | ||
152 | 547 | 557 | ||
153 | diff --git a/src/maasserver/api/doc_handler.py b/src/maasserver/api/doc_handler.py | |||
154 | index 9b332c7..e9e1f4a 100644 | |||
155 | --- a/src/maasserver/api/doc_handler.py | |||
156 | +++ b/src/maasserver/api/doc_handler.py | |||
157 | @@ -235,11 +235,12 @@ def describe(request): | |||
158 | 235 | # The handler URIs returned by describe_resource() are relative paths. | 235 | # The handler URIs returned by describe_resource() are relative paths. |
159 | 236 | absolute = partial(build_absolute_uri, request) | 236 | absolute = partial(build_absolute_uri, request) |
160 | 237 | for resource in description["resources"]: | 237 | for resource in description["resources"]: |
162 | 238 | for handler_type in "anon", "auth": | 238 | for handler_type in ("anon", "auth"): |
163 | 239 | handler = resource[handler_type] | 239 | handler = resource[handler_type] |
164 | 240 | if handler is not None: | 240 | if handler is not None: |
165 | 241 | handler["uri"] = absolute(handler["path"]) | 241 | handler["uri"] = absolute(handler["path"]) |
166 | 242 | # Return as a JSON document. | 242 | # Return as a JSON document. |
167 | 243 | return HttpResponse( | 243 | return HttpResponse( |
169 | 244 | json.dumps(description), content_type="application/json" | 244 | json.dumps(description, sort_keys=True), |
170 | 245 | content_type="application/json", | ||
171 | 245 | ) | 246 | ) |
172 | diff --git a/src/maasserver/forms/settings.py b/src/maasserver/forms/settings.py | |||
173 | index 10ab368..5a662a2 100644 | |||
174 | --- a/src/maasserver/forms/settings.py | |||
175 | +++ b/src/maasserver/forms/settings.py | |||
176 | @@ -75,7 +75,7 @@ def make_default_osystem_field(*args, **kwargs): | |||
177 | 75 | "osystem", os_choices | 75 | "osystem", os_choices |
178 | 76 | ) | 76 | ) |
179 | 77 | }, | 77 | }, |
181 | 78 | **kwargs | 78 | **kwargs, |
182 | 79 | ) | 79 | ) |
183 | 80 | return field | 80 | return field |
184 | 81 | 81 | ||
185 | @@ -154,7 +154,7 @@ def make_default_distro_series_field(*args, **kwargs): | |||
186 | 154 | "release", release_choices | 154 | "release", release_choices |
187 | 155 | ) | 155 | ) |
188 | 156 | }, | 156 | }, |
190 | 157 | **kwargs | 157 | **kwargs, |
191 | 158 | ) | 158 | ) |
192 | 159 | return field | 159 | return field |
193 | 160 | 160 | ||
194 | @@ -186,7 +186,7 @@ def make_default_min_hwe_kernel_field(*args, **kwargs): | |||
195 | 186 | "default_min_hwe_kernel", kernel_choices | 186 | "default_min_hwe_kernel", kernel_choices |
196 | 187 | ) | 187 | ) |
197 | 188 | }, | 188 | }, |
199 | 189 | **kwargs | 189 | **kwargs, |
200 | 190 | ) | 190 | ) |
201 | 191 | return field | 191 | return field |
202 | 192 | 192 | ||
203 | @@ -206,7 +206,7 @@ def make_commissioning_distro_series_field(*args, **kwargs): | |||
204 | 206 | "commissioning_distro_series", commissioning_choices | 206 | "commissioning_distro_series", commissioning_choices |
205 | 207 | ) | 207 | ) |
206 | 208 | }, | 208 | }, |
208 | 209 | **kwargs | 209 | **kwargs, |
209 | 210 | ) | 210 | ) |
210 | 211 | return field | 211 | return field |
211 | 212 | 212 | ||
212 | @@ -221,7 +221,7 @@ def make_dnssec_validation_field(*args, **kwargs): | |||
213 | 221 | "dnssec_validation", DNSSEC_VALIDATION_CHOICES | 221 | "dnssec_validation", DNSSEC_VALIDATION_CHOICES |
214 | 222 | ) | 222 | ) |
215 | 223 | }, | 223 | }, |
217 | 224 | **kwargs | 224 | **kwargs, |
218 | 225 | ) | 225 | ) |
219 | 226 | return field | 226 | return field |
220 | 227 | 227 | ||
221 | @@ -236,7 +236,7 @@ def make_network_discovery_field(*args, **kwargs): | |||
222 | 236 | "network_discovery", NETWORK_DISCOVERY_CHOICES | 236 | "network_discovery", NETWORK_DISCOVERY_CHOICES |
223 | 237 | ) | 237 | ) |
224 | 238 | }, | 238 | }, |
226 | 239 | **kwargs | 239 | **kwargs, |
227 | 240 | ) | 240 | ) |
228 | 241 | return field | 241 | return field |
229 | 242 | 242 | ||
230 | @@ -251,7 +251,7 @@ def make_active_discovery_interval_field(*args, **kwargs): | |||
231 | 251 | "active_discovery_interval", ACTIVE_DISCOVERY_INTERVAL_CHOICES | 251 | "active_discovery_interval", ACTIVE_DISCOVERY_INTERVAL_CHOICES |
232 | 252 | ) | 252 | ) |
233 | 253 | }, | 253 | }, |
235 | 254 | **kwargs | 254 | **kwargs, |
236 | 255 | ) | 255 | ) |
237 | 256 | return field | 256 | return field |
238 | 257 | 257 | ||
239 | @@ -998,17 +998,12 @@ def get_config_form(config_name, data=None): | |||
240 | 998 | return form | 998 | return form |
241 | 999 | 999 | ||
242 | 1000 | 1000 | ||
251 | 1001 | def describe_choices(choices): | 1001 | def get_config_doc(config_items=None, indentation=0): |
244 | 1002 | """Describe the items in an enumeration of Django form choices.""" | ||
245 | 1003 | return ", ".join( | ||
246 | 1004 | "'%s' (%s)" % (value, meaning) for value, meaning in choices | ||
247 | 1005 | ) | ||
248 | 1006 | |||
249 | 1007 | |||
250 | 1008 | def get_config_doc(indentation=0): | ||
252 | 1009 | """Return the documentation about the available configuration settings.""" | 1002 | """Return the documentation about the available configuration settings.""" |
253 | 1003 | if config_items is None: | ||
254 | 1004 | config_items = CONFIG_ITEMS | ||
255 | 1010 | doc = ["Available configuration items:\n\n"] | 1005 | doc = ["Available configuration items:\n\n"] |
257 | 1011 | for config_name, config_details in sorted(CONFIG_ITEMS.items()): | 1006 | for config_name, config_details in sorted(config_items.items()): |
258 | 1012 | form_details = config_details["form_kwargs"] | 1007 | form_details = config_details["form_kwargs"] |
259 | 1013 | doc.append(":" + config_name + ": " + form_details["label"] + ". ") | 1008 | doc.append(":" + config_name + ": " + form_details["label"] + ". ") |
260 | 1014 | # Append help text if present. | 1009 | # Append help text if present. |
261 | @@ -1018,7 +1013,9 @@ def get_config_doc(indentation=0): | |||
262 | 1018 | # Append list of possible choices if present. | 1013 | # Append list of possible choices if present. |
263 | 1019 | choices = form_details.get("choices") | 1014 | choices = form_details.get("choices") |
264 | 1020 | if choices is not None: | 1015 | if choices is not None: |
266 | 1021 | choice_descr = describe_choices(choices) | 1016 | choice_descr = ", ".join( |
267 | 1017 | f"'{value}' ({meaning})" for value, meaning in sorted(choices) | ||
268 | 1018 | ) | ||
269 | 1022 | doc.append("Available choices are: %s." % choice_descr) | 1019 | doc.append("Available choices are: %s." % choice_descr) |
270 | 1023 | doc.append("\n") | 1020 | doc.append("\n") |
271 | 1024 | full_text = (" " * indentation).join(doc) | 1021 | full_text = (" " * indentation).join(doc) |
272 | diff --git a/src/maasserver/forms/tests/test_settings.py b/src/maasserver/forms/tests/test_settings.py | |||
273 | index a1dcc23..f0cd075 100644 | |||
274 | --- a/src/maasserver/forms/tests/test_settings.py | |||
275 | +++ b/src/maasserver/forms/tests/test_settings.py | |||
276 | @@ -1,8 +1,7 @@ | |||
277 | 1 | # Copyright 2013-2016 Canonical Ltd. This software is licensed under the | 1 | # Copyright 2013-2016 Canonical Ltd. This software is licensed under the |
278 | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). | 2 | # GNU Affero General Public License version 3 (see the file LICENSE). |
279 | 3 | 3 | ||
282 | 4 | """Test forms settings.""" | 4 | from textwrap import dedent |
281 | 5 | |||
283 | 6 | 5 | ||
284 | 7 | from django import forms | 6 | from django import forms |
285 | 8 | from django.core.exceptions import ValidationError | 7 | from django.core.exceptions import ValidationError |
286 | @@ -41,9 +40,31 @@ class TestGetConfigForm(MAASServerTestCase): | |||
287 | 41 | 40 | ||
288 | 42 | class TestGetConfigDoc(MAASServerTestCase): | 41 | class TestGetConfigDoc(MAASServerTestCase): |
289 | 43 | def test_get_config_doc(self): | 42 | def test_get_config_doc(self): |
293 | 44 | doc = get_config_doc() | 43 | config_items = { |
294 | 45 | # Just make sure that the doc looks okay. | 44 | "testitem": { |
295 | 46 | self.assertIn("maas_name", doc) | 45 | "default": "foo", |
296 | 46 | "form": forms.CharField, | ||
297 | 47 | "form_kwargs": { | ||
298 | 48 | "label": "bar", | ||
299 | 49 | "choices": [ | ||
300 | 50 | ("b", "B"), | ||
301 | 51 | ("a", "A"), | ||
302 | 52 | ], | ||
303 | 53 | }, | ||
304 | 54 | }, | ||
305 | 55 | } | ||
306 | 56 | doc = get_config_doc(config_items=config_items) | ||
307 | 57 | # choices are returned in the correct order | ||
308 | 58 | self.assertEqual( | ||
309 | 59 | doc, | ||
310 | 60 | dedent( | ||
311 | 61 | """\ | ||
312 | 62 | Available configuration items: | ||
313 | 63 | |||
314 | 64 | :testitem: bar. Available choices are: 'a' (A), 'b' (B). | ||
315 | 65 | """ | ||
316 | 66 | ), | ||
317 | 67 | ) | ||
318 | 47 | 68 | ||
319 | 48 | 69 | ||
320 | 49 | class TestSpecificConfigSettings(MAASServerTestCase): | 70 | class TestSpecificConfigSettings(MAASServerTestCase): |
321 | diff --git a/src/maasserver/urls_api.py b/src/maasserver/urls_api.py | |||
322 | index ff6bf95..bbf76a1 100644 | |||
323 | --- a/src/maasserver/urls_api.py | |||
324 | +++ b/src/maasserver/urls_api.py | |||
325 | @@ -566,7 +566,7 @@ urlpatterns += [ | |||
326 | 566 | ), | 566 | ), |
327 | 567 | url(r"^vlans/(?P<vlan_id>[^/]+)/$", vlan_handler, name="vlanid_handler"), | 567 | url(r"^vlans/(?P<vlan_id>[^/]+)/$", vlan_handler, name="vlanid_handler"), |
328 | 568 | url( | 568 | url( |
330 | 569 | r"fabrics/(?P<fabric_id>[^/]+)/vlans/(?P<vid>[^/]+)/$", | 569 | r"^fabrics/(?P<fabric_id>[^/]+)/vlans/(?P<vid>[^/]+)/$", |
331 | 570 | vlan_handler, | 570 | vlan_handler, |
332 | 571 | name="vlan_handler", | 571 | name="vlan_handler", |
333 | 572 | ), | 572 | ), |
UNIT TESTS api-description -stable- sorting lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas
-b 1949963-
STATUS: FAILED maas-ci. internal: 8080/job/ maas/job/ branch- tester/ 11469/console 431d8e254293dbd 8b4951d876
LOG: http://
COMMIT: 84aaeaa007c67b1