Merge lp:~rharding/charmworld/proof-config into lp:~juju-jitsu/charmworld/trunk
- proof-config
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 424 |
Proposed branch: | lp:~rharding/charmworld/proof-config |
Merge into: | lp:~juju-jitsu/charmworld/trunk |
Diff against target: |
533 lines (+412/-44) 4 files modified
charmworld/lib/proof.py (+76/-0) charmworld/lib/tests/test_proof.py (+151/-0) charmworld/views/api.py (+87/-39) charmworld/views/tests/test_api.py (+98/-5) |
To merge this branch: | bzr merge lp:~rharding/charmworld/proof-config |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju-Jitsu Hackers | Pending | ||
Review via email: mp+191689@code.launchpad.net |
Commit message
Description of the change
Add charm config proofing and update api response
- Move proof logic into a new lib/proof.py module
- Update the proof api call to look through the bundles and proof them
- Change errors to be catchable exceptions, the exception contains both a msg
and debug_info we expose via the api call
- Update the api call to provide a summary error_messages (for Marco's needs)
but keep the details in the actual list of errors with the messages and the
debug info we have that went into those errors.
Sample proof api call and response:
http://
Note: this is only adding the new proof checks of validating the
config/options from the charm found in the database to the options defined in
the bundle.
- Validate the option exists in the charm we found in the db
- Validate that the type is of the correct type.
- Validate that the charm we found has options at all.
QA
---
You have to ingest the charms so that we can validate them. You can then toss
the yaml file (as a single string) to the proof api endpoint as a POST'd key
deployer_file. Right now it only handles yaml.
- 432. By Richard Harding
-
Fix for the updated error_messages
- 433. By Richard Harding
-
add bundle proof tests
- 434. By Richard Harding
-
Add proof tests for the CharmProof
- 435. By Richard Harding
-
Add support for checking the updated proofing call
- 436. By Richard Harding
-
Update test to be a bit better demo of multiple errors
- 437. By Richard Harding
-
lint
Richard Harding (rharding) wrote : | # |
Richard Harding (rharding) wrote : | # |
Reviewer comments added.
https:/
File charmworld/
https:/
charmworld/
this is the list of actual errors. They can be global messages ('could
not parse the file') or objects from error checking a specific bundle in
the file.
https:/
charmworld/
this is a summary of all error messages provided as an aid to marco.
https:/
charmworld/
and a global level 'debug_info' to match the representation of errors of
other areas of the code where they all provide a message as well as
helpful debug information to assist in finding the way to correct the
error.
https:/
charmworld/
moved the old checks to the try:catch for the ProofError. I felt this
read a bit cleaner and allows for a function to return a value (the
parsed yaml file) normally, but have a complex object come back during a
failure.
https:/
File charmworld/
https:/
charmworld/
results[
this is due to the move to support multiple errors on the same service
within a bundle.
Gary Poster (gary) wrote : | # |
Hi Rick. Thank you for a nice branch. LGTM.
I give a ton of comments that could lead to a decent amount of work.
Please treat all of them as suggestions, though I do think that the code
in the BundleProof code ought to move back to the view, somehow, and I
hope I made at least a somewhat compelling case for it.
https:/
File charmworld/
https:/
charmworld/
On 2013/10/18 19:09:24, rharding wrote:
> I'm not a fan of importing models into a lib file, open to ideas of
ways to make
> this better other than passing in a resolver and a descriptor into the
proof
> calls from the view.
Similar alternative: You have to instantiate BundleProof with these
extra bits, to create something that the view can use? Don't know
charmworld, so I don't know where it would be stashed, but that broad
approach seems tried and true.
https:/
charmworld/
format='yaml'):
Ah, ye olde naming problem: a validation function also does work. :-)
If you really cared about the naming problem, I would suggest naming
this "parse_
tossing out any helpful information from the parser's error, btw :-/)
seems minor compared to the fact that this parses the data. <shrug>
(which means "do as you will")
https:/
charmworld/
Uh oh, this validation function is doing work too. :-/
Um.
This doesn't feel right. The Bundle proof bits here feel like they
belong in an integration layer, like a view, rather than in a library.
Do with that what you will.
https:/
charmworld/
On 2013/10/18 19:09:24, rharding wrote:
> this is only called if it's expecting to proof options. So not having
any is an
> error. (test_key and test_value are required inputs)
Is this an error of the bundle author or in the charmworld code? It
feels like the latter, in which case raising a ProofError seems wrong.
We ought to log something instead. If I'm wrong, nevermind.
https:/
File charmworld/
https:/
charmworld/
...This is JSON...
https:/
charmworld/
parseable as yaml."""
"...as YAML or JSON."?
https:/
File charmworld/
https:/
charmworld/views...
Preview Diff
1 | === added file 'charmworld/lib/proof.py' | |||
2 | --- charmworld/lib/proof.py 1970-01-01 00:00:00 +0000 | |||
3 | +++ charmworld/lib/proof.py 2013-10-18 18:57:29 +0000 | |||
4 | @@ -0,0 +1,76 @@ | |||
5 | 1 | """Helpers to proof charms and bundles.""" | ||
6 | 2 | import yaml | ||
7 | 3 | |||
8 | 4 | from charmworld.models import ( | ||
9 | 5 | BundledCharmDescription, | ||
10 | 6 | resolve_charm_from_description, | ||
11 | 7 | ) | ||
12 | 8 | |||
13 | 9 | |||
14 | 10 | class ProofError(Exception): | ||
15 | 11 | |||
16 | 12 | def __init__(self, debug_info, msg): | ||
17 | 13 | self.debug_info = debug_info | ||
18 | 14 | self.msg = msg | ||
19 | 15 | |||
20 | 16 | |||
21 | 17 | class BundleProof(object): | ||
22 | 18 | |||
23 | 19 | @staticmethod | ||
24 | 20 | def check_parseable_deployer_file(data, format='yaml'): | ||
25 | 21 | try: | ||
26 | 22 | bundle_data = yaml.safe_load(data) | ||
27 | 23 | except yaml.YAMLError: | ||
28 | 24 | raise ProofError( | ||
29 | 25 | data, | ||
30 | 26 | 'Could not parse the yaml provided.' | ||
31 | 27 | ) | ||
32 | 28 | return bundle_data | ||
33 | 29 | |||
34 | 30 | @staticmethod | ||
35 | 31 | def check_service_exists(db, name, service_data, bundle_config): | ||
36 | 32 | charm_description = BundledCharmDescription( | ||
37 | 33 | 'proof', service_data, bundle_config.get('series')) | ||
38 | 34 | charm_found = resolve_charm_from_description(db, charm_description) | ||
39 | 35 | if not charm_found: | ||
40 | 36 | raise ProofError( | ||
41 | 37 | charm_description.get_process(), | ||
42 | 38 | 'Could not find charm: ' + name, | ||
43 | 39 | ) | ||
44 | 40 | else: | ||
45 | 41 | return charm_found | ||
46 | 42 | |||
47 | 43 | |||
48 | 44 | class CharmProof(object): | ||
49 | 45 | |||
50 | 46 | @staticmethod | ||
51 | 47 | def check_config(charm, test_key, test_value): | ||
52 | 48 | type_map = { | ||
53 | 49 | 'boolean': ('bool',), | ||
54 | 50 | 'int': ('int',), | ||
55 | 51 | 'string': ('str', 'unicode'), | ||
56 | 52 | } | ||
57 | 53 | |||
58 | 54 | if not charm.options: | ||
59 | 55 | raise ProofError([ | ||
60 | 56 | 'Looking at charm id: ' + charm._id], | ||
61 | 57 | 'The charm has no options.' | ||
62 | 58 | ) | ||
63 | 59 | |||
64 | 60 | if test_key not in charm.options: | ||
65 | 61 | raise ProofError([ | ||
66 | 62 | 'Looking at charm id: ' + charm._id, | ||
67 | 63 | 'Found config keys: ' + str(charm.options.keys())], | ||
68 | 64 | 'The charm has no option for: ' + test_key | ||
69 | 65 | ) | ||
70 | 66 | |||
71 | 67 | # # The config key is a valid one for this charm. Check that the value | ||
72 | 68 | # # is of the right type. | ||
73 | 69 | specified_type = charm.options[test_key]['type'] | ||
74 | 70 | valid_types = type_map[specified_type] | ||
75 | 71 | msg = "%s is not of type %s." | ||
76 | 72 | if type(test_value) not in valid_types: | ||
77 | 73 | raise ProofError( | ||
78 | 74 | ['Looking at charm id: ' + charm._id], | ||
79 | 75 | msg % (test_key, specified_type) | ||
80 | 76 | ) | ||
81 | 0 | 77 | ||
82 | === added directory 'charmworld/lib/tests' | |||
83 | === added file 'charmworld/lib/tests/__init__.py' | |||
84 | === added file 'charmworld/lib/tests/test_proof.py' | |||
85 | --- charmworld/lib/tests/test_proof.py 1970-01-01 00:00:00 +0000 | |||
86 | +++ charmworld/lib/tests/test_proof.py 2013-10-18 18:57:29 +0000 | |||
87 | @@ -0,0 +1,151 @@ | |||
88 | 1 | from mock import ( | ||
89 | 2 | Mock, | ||
90 | 3 | patch, | ||
91 | 4 | ) | ||
92 | 5 | import yaml | ||
93 | 6 | |||
94 | 7 | from charmworld.lib.proof import ( | ||
95 | 8 | BundleProof, | ||
96 | 9 | CharmProof, | ||
97 | 10 | ProofError, | ||
98 | 11 | ) | ||
99 | 12 | from charmworld.models import Charm | ||
100 | 13 | from charmworld.testing import TestCase | ||
101 | 14 | |||
102 | 15 | |||
103 | 16 | class TestBundleProof(TestCase): | ||
104 | 17 | """Verify that a bundle can be proofed in charmworld.""" | ||
105 | 18 | |||
106 | 19 | sample_deployer_file = """ | ||
107 | 20 | { | ||
108 | 21 | "wiki": { | ||
109 | 22 | "services": { | ||
110 | 23 | "wiki": { | ||
111 | 24 | "charm": "mediawiki", | ||
112 | 25 | "num_units": 2, | ||
113 | 26 | "branch": "lp:charms/precise/mediawiki", | ||
114 | 27 | "constraints": "mem=2" | ||
115 | 28 | } | ||
116 | 29 | }, | ||
117 | 30 | "series": "precise" }}""" | ||
118 | 31 | |||
119 | 32 | def test_check_parseable_yaml(self): | ||
120 | 33 | """A deployer file should be parseable as yaml.""" | ||
121 | 34 | |||
122 | 35 | deployer_file = self.sample_deployer_file | ||
123 | 36 | result = BundleProof.check_parseable_deployer_file(deployer_file) | ||
124 | 37 | self.assertEqual(result['wiki']['services'].keys(), ['wiki']) | ||
125 | 38 | |||
126 | 39 | def test_failing_parsing_yaml_throws_proof_error(self): | ||
127 | 40 | """If the yaml is not parse the yaml thows a ProofError""" | ||
128 | 41 | deployer_file = '{' | ||
129 | 42 | with self.assertRaises(ProofError) as exc: | ||
130 | 43 | BundleProof.check_parseable_deployer_file(deployer_file) | ||
131 | 44 | |||
132 | 45 | self.assertEqual( | ||
133 | 46 | 'Could not parse the yaml provided.', | ||
134 | 47 | exc.exception.msg | ||
135 | 48 | ) | ||
136 | 49 | self.assertEqual( | ||
137 | 50 | deployer_file, | ||
138 | 51 | exc.exception.debug_info | ||
139 | 52 | ) | ||
140 | 53 | |||
141 | 54 | @patch('charmworld.lib.proof.resolve_charm_from_description') | ||
142 | 55 | def test_service_not_found_raises_proof_error(self, resolver): | ||
143 | 56 | """If we cannot find a service specified it's a ProofError""" | ||
144 | 57 | resolver.return_value = None | ||
145 | 58 | deployer = yaml.load(self.sample_deployer_file) | ||
146 | 59 | |||
147 | 60 | with self.assertRaises(ProofError) as exc: | ||
148 | 61 | BundleProof.check_service_exists( | ||
149 | 62 | None, | ||
150 | 63 | 'wiki', | ||
151 | 64 | deployer['wiki']['services']['wiki'], | ||
152 | 65 | deployer['wiki']) | ||
153 | 66 | |||
154 | 67 | self.assertEqual( | ||
155 | 68 | "Could not find charm: wiki", | ||
156 | 69 | exc.exception.msg | ||
157 | 70 | ) | ||
158 | 71 | # We should have an array of logic behind the decision to look up the | ||
159 | 72 | # charm. Just check we've got some logic. This number might get | ||
160 | 73 | # tweaked, but we're just checking it made it to the exception, not | ||
161 | 74 | # what the reasons are. | ||
162 | 75 | self.assertEqual( | ||
163 | 76 | 4, | ||
164 | 77 | len(exc.exception.debug_info) | ||
165 | 78 | ) | ||
166 | 79 | |||
167 | 80 | |||
168 | 81 | class TestCharmProof(TestCase): | ||
169 | 82 | |||
170 | 83 | def make_fake_charm_config(self, options): | ||
171 | 84 | charm = Charm({ | ||
172 | 85 | '_id': '/precise/id', | ||
173 | 86 | 'config': options | ||
174 | 87 | }) | ||
175 | 88 | |||
176 | 89 | return charm | ||
177 | 90 | |||
178 | 91 | def test_check_config_fails_when_no_options_defined(self): | ||
179 | 92 | """Bundles might attempt to set config values that are not valid""" | ||
180 | 93 | charm = Mock() | ||
181 | 94 | charm.options = {} | ||
182 | 95 | charm._id = 'precise/test' | ||
183 | 96 | |||
184 | 97 | with self.assertRaises(ProofError) as exc: | ||
185 | 98 | CharmProof.check_config(charm, 'name', 'test') | ||
186 | 99 | |||
187 | 100 | self.assertEqual( | ||
188 | 101 | 'The charm has no options.', | ||
189 | 102 | exc.exception.msg | ||
190 | 103 | ) | ||
191 | 104 | self.assertEqual( | ||
192 | 105 | ['Looking at charm id: precise/test'], | ||
193 | 106 | exc.exception.debug_info | ||
194 | 107 | ) | ||
195 | 108 | |||
196 | 109 | def test_check_config_bool(self): | ||
197 | 110 | """Bundles might attempt to set a boolean config value string/int""" | ||
198 | 111 | charm = Mock() | ||
199 | 112 | charm.options = { | ||
200 | 113 | 'name': { | ||
201 | 114 | 'type': 'boolean', | ||
202 | 115 | 'default': True, | ||
203 | 116 | } | ||
204 | 117 | } | ||
205 | 118 | charm._id = 'precise/test' | ||
206 | 119 | |||
207 | 120 | with self.assertRaises(ProofError) as exc: | ||
208 | 121 | CharmProof.check_config(charm, 'name', 'test') | ||
209 | 122 | |||
210 | 123 | self.assertEqual( | ||
211 | 124 | 'name is not of type boolean.', | ||
212 | 125 | exc.exception.msg | ||
213 | 126 | ) | ||
214 | 127 | self.assertEqual( | ||
215 | 128 | ['Looking at charm id: precise/test'], | ||
216 | 129 | exc.exception.debug_info | ||
217 | 130 | ) | ||
218 | 131 | |||
219 | 132 | def test_check_config_fails_when_option_does_not_exist(self): | ||
220 | 133 | """Bundles might attempt to set config values that are not valid""" | ||
221 | 134 | charm = Mock() | ||
222 | 135 | charm.options = {'foo': {}} | ||
223 | 136 | charm._id = 'precise/test' | ||
224 | 137 | |||
225 | 138 | with self.assertRaises(ProofError) as exc: | ||
226 | 139 | CharmProof.check_config(charm, 'name', 'test') | ||
227 | 140 | |||
228 | 141 | self.assertEqual( | ||
229 | 142 | 'The charm has no option for: name', | ||
230 | 143 | exc.exception.msg | ||
231 | 144 | ) | ||
232 | 145 | self.assertEqual( | ||
233 | 146 | [ | ||
234 | 147 | 'Looking at charm id: precise/test', | ||
235 | 148 | 'Found config keys: [\'foo\']' | ||
236 | 149 | ], | ||
237 | 150 | exc.exception.debug_info | ||
238 | 151 | ) | ||
239 | 0 | 152 | ||
240 | === modified file 'charmworld/views/api.py' | |||
241 | --- charmworld/views/api.py 2013-10-15 16:18:49 +0000 | |||
242 | +++ charmworld/views/api.py 2013-10-18 18:57:29 +0000 | |||
243 | @@ -12,7 +12,6 @@ | |||
244 | 12 | ) | 12 | ) |
245 | 13 | import re | 13 | import re |
246 | 14 | from urllib import unquote | 14 | from urllib import unquote |
247 | 15 | import yaml | ||
248 | 16 | 15 | ||
249 | 17 | from gridfs.errors import NoFile | 16 | from gridfs.errors import NoFile |
250 | 18 | import pymongo | 17 | import pymongo |
251 | @@ -23,6 +22,11 @@ | |||
252 | 23 | from pyramid.view import view_config | 22 | from pyramid.view import view_config |
253 | 24 | from webob import Response | 23 | from webob import Response |
254 | 25 | 24 | ||
255 | 25 | from charmworld.lib.proof import ( | ||
256 | 26 | BundleProof, | ||
257 | 27 | CharmProof, | ||
258 | 28 | ProofError, | ||
259 | 29 | ) | ||
260 | 26 | from charmworld.models import ( | 30 | from charmworld.models import ( |
261 | 27 | Bundle, | 31 | Bundle, |
262 | 28 | BundledCharmDescription, | 32 | BundledCharmDescription, |
263 | @@ -495,50 +499,94 @@ | |||
264 | 495 | # The top level is for general deployer file errors. | 499 | # The top level is for general deployer file errors. |
265 | 496 | response_data = { | 500 | response_data = { |
266 | 497 | 'errors': [], | 501 | 'errors': [], |
267 | 502 | 'error_messages': [], | ||
268 | 503 | 'debug_info': [], | ||
269 | 498 | } | 504 | } |
270 | 499 | bundle_string = request.params.get('deployer_file') | 505 | bundle_string = request.params.get('deployer_file') |
271 | 500 | bundle_format = request.params.get('bundle_format', 'yaml') | 506 | bundle_format = request.params.get('bundle_format', 'yaml') |
272 | 501 | bundle_data = None | 507 | bundle_data = None |
273 | 502 | 508 | ||
274 | 503 | if not bundle_string: | 509 | if not bundle_string: |
313 | 504 | response_data['errors'].append('No deployer file data received.') | 510 | response_data['error_messages'].append( |
314 | 505 | return json_response(400, response_data) | 511 | 'No deployer file data received.') |
315 | 506 | 512 | return json_response(400, response_data) | |
316 | 507 | if bundle_format == 'yaml': | 513 | |
317 | 508 | try: | 514 | try: |
318 | 509 | bundle_data = yaml.safe_load(bundle_string) | 515 | bundle_data = BundleProof.check_parseable_deployer_file( |
319 | 510 | except yaml.YAMLError: | 516 | bundle_string, |
320 | 511 | response_data['errors'].append( | 517 | format=bundle_format) |
321 | 512 | 'Could not parse the yaml provided.') | 518 | except ProofError, exc: |
322 | 513 | return json_response(400, response_data) | 519 | # If we cannot parse the config file there's nothing more to do. |
323 | 514 | 520 | # Return immediately. | |
324 | 515 | # Proof each bundle in the deployer file. | 521 | response_data['error_messages'].append(exc.msg) |
325 | 516 | for bundle_name, bundle_config in bundle_data.items(): | 522 | response_data['debug_info'].append(exc.debug_info) |
326 | 517 | has_bundle_error = False | 523 | return json_response(400, response_data) |
327 | 518 | bundle_info = { | 524 | |
328 | 519 | 'name': bundle_name, | 525 | # Proof each bundle in the deployer file. |
329 | 520 | 'services': {}, | 526 | for bundle_name, bundle_config in bundle_data.items(): |
330 | 521 | 'relations': {}, | 527 | bundle_info = { |
331 | 522 | } | 528 | 'name': bundle_name, |
332 | 523 | 529 | 'services': {}, | |
333 | 524 | for name, charm in bundle_config.get('services').items(): | 530 | 'relations': {}, |
334 | 525 | charm_description = BundledCharmDescription( | 531 | } |
335 | 526 | 'proof', charm, bundle_config.get('series')) | 532 | found_charms = {} |
336 | 527 | 533 | ||
337 | 528 | if not resolve_charm_from_description( | 534 | # Verify we can find the charms at all. |
338 | 529 | self.request.db, charm_description): | 535 | for name, charm in bundle_config.get('services').items(): |
339 | 530 | has_bundle_error = True | 536 | try: |
340 | 531 | # The error is keyed to the charm and includes a | 537 | charm_found = BundleProof.check_service_exists( |
341 | 532 | # general message + the description's thought process. | 538 | self.request.db, name, charm, bundle_config) |
342 | 533 | bundle_info['services'][name] = { | 539 | found_charms[name] = Charm(charm_found) |
343 | 534 | 'message': 'Could not find charm: ' + name, | 540 | except ProofError, exc: |
344 | 535 | 'process': charm_description.get_process() | 541 | # We could not find the charm. No other verification can |
345 | 536 | } | 542 | # happen. |
346 | 537 | 543 | bundle_info['services'][name] = [] | |
347 | 538 | if has_bundle_error: | 544 | bundle_info['services'][name].append({ |
348 | 539 | response_data['errors'].append(bundle_info) | 545 | 'message': exc.msg, |
349 | 540 | 546 | 'debug_info': exc.debug_info, | |
350 | 541 | return json_response(200, response_data) | 547 | }) |
351 | 548 | |||
352 | 549 | # For the charms we did find in the system, verify that their | ||
353 | 550 | # config is valid for the values allowed by the charm we found. | ||
354 | 551 | for name, charm in found_charms.iteritems(): | ||
355 | 552 | check_config = bundle_config['services'][name].get('options') | ||
356 | 553 | if not check_config: | ||
357 | 554 | # There's no config specified to validate. | ||
358 | 555 | continue | ||
359 | 556 | for test_key, test_value in check_config.iteritems(): | ||
360 | 557 | try: | ||
361 | 558 | CharmProof.check_config(charm, test_key, test_value) | ||
362 | 559 | except ProofError, exc: | ||
363 | 560 | if name not in bundle_info['services']: | ||
364 | 561 | bundle_info['services'][name] = [] | ||
365 | 562 | |||
366 | 563 | bundle_info['services'][name].append({ | ||
367 | 564 | 'message': exc.msg, | ||
368 | 565 | 'debug_info': exc.debug_info, | ||
369 | 566 | }) | ||
370 | 567 | |||
371 | 568 | # If there are errors in this bundles services, make sure we | ||
372 | 569 | # append the info to the response data that gets sent back to the | ||
373 | 570 | # user. | ||
374 | 571 | if bundle_info['services'].keys(): | ||
375 | 572 | response_data['errors'].append(bundle_info) | ||
376 | 573 | |||
377 | 574 | # Build up a root messages list of errors that can be used to help the | ||
378 | 575 | # client go through them all easily. | ||
379 | 576 | error_messages = [] | ||
380 | 577 | if response_data['errors']: | ||
381 | 578 | for bundle in response_data['errors']: | ||
382 | 579 | prefix = bundle['name'] | ||
383 | 580 | for service_name, service in bundle['services'].items(): | ||
384 | 581 | for err in service: | ||
385 | 582 | msg = '%s: %s' % (prefix, err['message']) | ||
386 | 583 | error_messages.append(msg) | ||
387 | 584 | if error_messages: | ||
388 | 585 | response_data['error_messages'].extend(error_messages) | ||
389 | 586 | |||
390 | 587 | # After proofing each deployer file return with any failure info | ||
391 | 588 | # found. | ||
392 | 589 | return json_response(200, response_data) | ||
393 | 542 | 590 | ||
394 | 543 | def charm(self, path=None): | 591 | def charm(self, path=None): |
395 | 544 | """Retrieve a charm according to its API ID (the path prefix).""" | 592 | """Retrieve a charm according to its API ID (the path prefix).""" |
396 | 545 | 593 | ||
397 | === modified file 'charmworld/views/tests/test_api.py' | |||
398 | --- charmworld/views/tests/test_api.py 2013-10-15 16:18:49 +0000 | |||
399 | +++ charmworld/views/tests/test_api.py 2013-10-18 18:57:29 +0000 | |||
400 | @@ -874,7 +874,7 @@ | |||
401 | 874 | self.assertEqual(400, response.status_code) | 874 | self.assertEqual(400, response.status_code) |
402 | 875 | self.assertEqual( | 875 | self.assertEqual( |
403 | 876 | 'No deployer file data received.', | 876 | 'No deployer file data received.', |
405 | 877 | response.json_body['errors'][0]) | 877 | response.json_body['error_messages'][0]) |
406 | 878 | 878 | ||
407 | 879 | def test_bundle_proof_invalid_deployer_file(self): | 879 | def test_bundle_proof_invalid_deployer_file(self): |
408 | 880 | request = self.make_request('bundle', remainder='/proof') | 880 | request = self.make_request('bundle', remainder='/proof') |
409 | @@ -885,7 +885,7 @@ | |||
410 | 885 | self.assertEqual(400, response.status_code) | 885 | self.assertEqual(400, response.status_code) |
411 | 886 | self.assertIn( | 886 | self.assertIn( |
412 | 887 | 'Could not parse', | 887 | 'Could not parse', |
414 | 888 | response.json_body['errors'][0]) | 888 | response.json_body['error_messages'][0]) |
415 | 889 | 889 | ||
416 | 890 | def test_bundle_proof_charm_exists(self): | 890 | def test_bundle_proof_charm_exists(self): |
417 | 891 | deployer_file = load_data_file('sample_bundle/bundles.yaml') | 891 | deployer_file = load_data_file('sample_bundle/bundles.yaml') |
418 | @@ -902,12 +902,12 @@ | |||
419 | 902 | self.assertEqual(4, len(results['errors'][0]['services'].keys())) | 902 | self.assertEqual(4, len(results['errors'][0]['services'].keys())) |
420 | 903 | self.assertIn( | 903 | self.assertIn( |
421 | 904 | 'Could not find ', | 904 | 'Could not find ', |
423 | 905 | results['errors'][0]['services']['db']['message']) | 905 | results['errors'][0]['services']['db'][0]['message']) |
424 | 906 | 906 | ||
425 | 907 | # And each failure has a list of 'process' information. | 907 | # And each failure has a list of 'process' information. |
426 | 908 | self.assertIn( | 908 | self.assertIn( |
429 | 909 | 'process', | 909 | 'debug_info', |
430 | 910 | results['errors'][0]['services']['db'].keys()) | 910 | results['errors'][0]['services']['db'][0].keys()) |
431 | 911 | 911 | ||
432 | 912 | def test_bundle_proof_valid(self): | 912 | def test_bundle_proof_valid(self): |
433 | 913 | # We have to create the charm so that we can verify it exists. | 913 | # We have to create the charm so that we can verify it exists. |
434 | @@ -939,6 +939,99 @@ | |||
435 | 939 | results = response.json_body | 939 | results = response.json_body |
436 | 940 | self.assertEqual(0, len(results['errors'])) | 940 | self.assertEqual(0, len(results['errors'])) |
437 | 941 | 941 | ||
438 | 942 | def test_bundle_proof_invalid_config_type(self): | ||
439 | 943 | # We have to create the charm so that we can verify it exists. | ||
440 | 944 | _id, charm = factory.makeCharm( | ||
441 | 945 | self.db, | ||
442 | 946 | description='' | ||
443 | 947 | ) | ||
444 | 948 | |||
445 | 949 | # and we'll cheat and just find it straight by the store url. | ||
446 | 950 | charm = Charm(charm) | ||
447 | 951 | store_url = charm.store_url | ||
448 | 952 | |||
449 | 953 | bundle_config = { | ||
450 | 954 | 'wiki': { | ||
451 | 955 | 'services': { | ||
452 | 956 | 'charm': { | ||
453 | 957 | 'charm_url': str(store_url), | ||
454 | 958 | 'options': { | ||
455 | 959 | 'script-interval': 'invalid' | ||
456 | 960 | } | ||
457 | 961 | } | ||
458 | 962 | } | ||
459 | 963 | } | ||
460 | 964 | } | ||
461 | 965 | |||
462 | 966 | request = self.make_request('bundle', remainder='/proof') | ||
463 | 967 | request.params = { | ||
464 | 968 | 'deployer_file': yaml.dump(bundle_config) | ||
465 | 969 | } | ||
466 | 970 | response = self.api_class(request)() | ||
467 | 971 | self.assertEqual(200, response.status_code) | ||
468 | 972 | results = response.json_body | ||
469 | 973 | self.assertEqual(1, len(results['errors'])) | ||
470 | 974 | self.assertEqual(1, len(results['error_messages'])) | ||
471 | 975 | |||
472 | 976 | # Note that the View will prefix the error message from the proof with | ||
473 | 977 | # the bundle name to help identify where the error message came from. | ||
474 | 978 | self.assertEqual( | ||
475 | 979 | 'wiki: script-interval is not of type int.', | ||
476 | 980 | results['error_messages'][0] | ||
477 | 981 | ) | ||
478 | 982 | |||
479 | 983 | def test_bundle_proof_supports_multiple_errors(self): | ||
480 | 984 | _id, charm = factory.makeCharm( | ||
481 | 985 | self.db, | ||
482 | 986 | description='' | ||
483 | 987 | ) | ||
484 | 988 | |||
485 | 989 | # and we'll cheat and just find it straight by the store url. | ||
486 | 990 | charm = Charm(charm) | ||
487 | 991 | store_url = charm.store_url | ||
488 | 992 | |||
489 | 993 | bundle_config = { | ||
490 | 994 | 'wiki': { | ||
491 | 995 | 'services': { | ||
492 | 996 | 'charm': { | ||
493 | 997 | 'charm_url': str(store_url), | ||
494 | 998 | 'options': { | ||
495 | 999 | 'script-interval': 'invalid', | ||
496 | 1000 | 'no-exist': 'hah invalid', | ||
497 | 1001 | } | ||
498 | 1002 | }, | ||
499 | 1003 | 'fail': { | ||
500 | 1004 | 'name': 'will-fail' | ||
501 | 1005 | } | ||
502 | 1006 | } | ||
503 | 1007 | } | ||
504 | 1008 | } | ||
505 | 1009 | |||
506 | 1010 | request = self.make_request('bundle', remainder='/proof') | ||
507 | 1011 | request.params = { | ||
508 | 1012 | 'deployer_file': yaml.dump(bundle_config) | ||
509 | 1013 | } | ||
510 | 1014 | response = self.api_class(request)() | ||
511 | 1015 | self.assertEqual(200, response.status_code) | ||
512 | 1016 | results = response.json_body | ||
513 | 1017 | self.assertEqual(1, len(results['errors'])) | ||
514 | 1018 | self.assertEqual(3, len(results['error_messages'])) | ||
515 | 1019 | |||
516 | 1020 | # One message from not finding a charm. | ||
517 | 1021 | self.assertEqual( | ||
518 | 1022 | 'wiki: Could not find charm: fail', | ||
519 | 1023 | results['error_messages'][0] | ||
520 | 1024 | ) | ||
521 | 1025 | # The other two from checking the config of the other charm. | ||
522 | 1026 | self.assertEqual( | ||
523 | 1027 | 'wiki: script-interval is not of type int.', | ||
524 | 1028 | results['error_messages'][1] | ||
525 | 1029 | ) | ||
526 | 1030 | self.assertEqual( | ||
527 | 1031 | 'wiki: The charm has no option for: no-exist', | ||
528 | 1032 | results['error_messages'][2] | ||
529 | 1033 | ) | ||
530 | 1034 | |||
531 | 942 | 1035 | ||
532 | 943 | class TestAPI3Bundles(TestAPIBundles, API3Mixin): | 1036 | class TestAPI3Bundles(TestAPIBundles, API3Mixin): |
533 | 944 | """Test API 3 bundle endpoint.""" | 1037 | """Test API 3 bundle endpoint.""" |
Reviewers: mp+191689_ code.launchpad. net,
Message:
*** Submitted:
Add charm config proofing and update api response
- Move proof logic into a new lib/proof.py module
- Update the proof api call to look through the bundles and proof them
- Change errors to be catchable exceptions, the exception contains both
a msg
and debug_info we expose via the api call
- Update the api call to provide a summary error_messages (for Marco's
needs)
but keep the details in the actual list of errors with the messages and
the
debug info we have that went into those errors.
Sample proof api call and response: paste.mitechie. com/show/ 1049/
http://
Note: this is only adding the new proof checks of validating the
config/options from the charm found in the database to the options
defined in
the bundle.
- Validate the option exists in the charm we found in the db
- Validate that the type is of the correct type.
- Validate that the charm we found has options at all.
QA
---
You have to ingest the charms so that we can validate them. You can then
toss
the yaml file (as a single string) to the proof api endpoint as a POST'd
key
deployer_file. Right now it only handles yaml.
R= /codereview. appspot. com/14789043
CC=
https:/
https:/ /codereview. appspot. com/14789043/ diff/3001/ charmworld/ lib/proof. py lib/proof. py (right):
File charmworld/
https:/ /codereview. appspot. com/14789043/ diff/3001/ charmworld/ lib/proof. py#newcode4 lib/proof. py:4: from charmworld.models import (
charmworld/
I'm not a fan of importing models into a lib file, open to ideas of ways
to make this better other than passing in a resolver and a descriptor
into the proof calls from the view.
https:/ /codereview. appspot. com/14789043/ diff/3001/ charmworld/ lib/proof. py#newcode54 lib/proof. py:54: if not charm.options:
charmworld/
this is only called if it's expecting to proof options. So not having
any is an error. (test_key and test_value are required inputs)
https:/ /codereview. appspot. com/14789043/ diff/3001/ charmworld/ lib/proof. py#newcode67 lib/proof. py:67: # # The config key is a valid one for this
charmworld/
charm. Check that the value
doh, will clean up.
Description:
Add charm config proofing and update api response
- Move proof logic into a new lib/proof.py module
- Update the proof api call to look through the bundles and proof them
- Change errors to be catchable exceptions, the exception contains both
a msg
and debug_info we expose via the api call
- Update the api call to provide a summary error_messages (for Marco's
needs)
but keep the details in the actual list of errors with the messages and
the
debug info we have that went into those errors.
Sample proof api call and response: paste.mitechie. com/show/ 1049/
http://
Note: this is only adding the new proof checks of validating the
config/options from the charm found in the database to the options
defined in
the bundle.
- Validate the option exists in the charm we found in the db
- Validate that the type is of the correct type.
- Validate that the charm we found has options at all.
QA
---
You have to ingest the charms so that we can validate them. You can then
toss
the yaml file (as a single string) to the proof api endpoint as a POST'd
key
deploye...