Merge ~woutervb/snap-store-proxy-charm:SN-491 into snap-store-proxy-charm:master
- Git
- lp:~woutervb/snap-store-proxy-charm
- SN-491
- Merge into master
Proposed by
Wouter van Bommel
Status: | Merged |
---|---|
Approved by: | Wouter van Bommel |
Approved revision: | 418d9fc63db729e56fba9719191318b75d613809 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~woutervb/snap-store-proxy-charm:SN-491 |
Merge into: | snap-store-proxy-charm:master |
Diff against target: |
811 lines (+100/-474) 7 files modified
README.md (+1/-8) config.yaml (+0/-17) src/charm.py (+11/-94) src/helpers.py (+1/-10) src/resource_helpers.py (+1/-21) tests/test_charm.py (+85/-272) tests/test_resource_helpers.py (+1/-52) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jonathan Hartley (community) | Approve | ||
Review via email: mp+415718@code.launchpad.net |
Commit message
Remove several charm configuration options
Removing username, password and domain options, as it is no longer possible to register a proxy managed by this charm that way. It is replaced by the registration_bundle option.
With this change the code to register a proxy has been removed as well, and some of the unittests have been rewritten
Description of the change
To post a comment you must log in.
Revision history for this message
Wouter van Bommel (woutervb) wrote : | # |
@Jonathan, thanks for the feedback. Added some responses and will double check the `return` question
Revision history for this message
Otto Co-Pilot (otto-copilot) wrote : | # |
Running landing tests failed
https:/
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/README.md b/README.md |
2 | index 0c0b870..9ca4d72 100644 |
3 | --- a/README.md |
4 | +++ b/README.md |
5 | @@ -6,7 +6,7 @@ This charm will allow the installation and management of the snapstore proxy cha |
6 | |
7 | # Usage |
8 | |
9 | -Minimal items that have to be set are the domain this proxy runs on, and a username / password, on ubuntu one that correspond to an account that does not have 2fa enabled, and has accepted the developer agreement on snapstore.io. |
10 | +For the minimal usage a registration bundle needs to be created with the snapstore-admin tool and provided to the charm as illustrated below. |
11 | |
12 | ## Example bundle |
13 | |
14 | @@ -47,13 +47,6 @@ There are some mandatory config options that have to be configured for the charm |
15 | |
16 | juju config snap-store-proxy registration_bundle=$(cat <path to file created with snapstore-client> | base64) |
17 | |
18 | - |
19 | -Or (deprecated) |
20 | - |
21 | -* **domain** - The domain on which the proxy will listen. This is needed for the registration, and the proxy itself needs to be able to resolve it. Ip addresses don't work. It has to be prepended with the protocol (http). |
22 | -* **username** - This is the username of the Ubuntu One account that will be used to register the proxy |
23 | -* **password** - The password of the above username |
24 | - |
25 | ## Juju resource usage |
26 | |
27 | The charm supports 2 modes to install the snapstore proxy code itself. This code is distributed as a snap from the actual snapstore, which means that the unit to which this charm is deployed, will need internet access, or at least access to the actual snapstore. If this is undesired, it is possible to deploy this charm in complete offline mode, which means that the snaps(\*) will need to be added as a resource. |
28 | diff --git a/config.yaml b/config.yaml |
29 | index 1c131a6..1a194a6 100644 |
30 | --- a/config.yaml |
31 | +++ b/config.yaml |
32 | @@ -11,21 +11,4 @@ options: |
33 | A bundle created via `snapstore-proxy_registration_bundle` added as a base64 |
34 | encoded string. ie "$(cat <file> | base64)" |
35 | type: string |
36 | - domain: |
37 | - default: http://localhost.localdomain |
38 | - description: Domain name for the Snap Store Proxy. |
39 | - type: string |
40 | - username: |
41 | - default: null |
42 | - description: | |
43 | - Ubuntu one username used to register this proxy, this account cannot |
44 | - have 2 factor authentication enabled, and needs to have accepted |
45 | - the developer agreement on the snapstore.io website. |
46 | - type: string |
47 | - password: |
48 | - default: null |
49 | - decription: | |
50 | - The passsword the belongs to the Ubuntu one username to register |
51 | - this proxy. |
52 | - type: string |
53 | |
54 | diff --git a/src/charm.py b/src/charm.py |
55 | index b75ae15..043114a 100755 |
56 | --- a/src/charm.py |
57 | +++ b/src/charm.py |
58 | @@ -26,7 +26,6 @@ from resource_helpers import ( |
59 | hash_from_resource, |
60 | install_from_resource, |
61 | install_from_the_charmstore, |
62 | - register_store, |
63 | snap_details, |
64 | ) |
65 | |
66 | @@ -73,9 +72,7 @@ class SnapstoreProxyCharm(CharmBase): |
67 | """Handle update on the configuration of the proxy. |
68 | |
69 | Here we handle changes to: |
70 | - * domain |
71 | - * username |
72 | - * password |
73 | + * registration bundle |
74 | """ |
75 | self.unit.status = MaintenanceStatus("Installing application snap") |
76 | self.handle_installation() |
77 | @@ -93,6 +90,13 @@ class SnapstoreProxyCharm(CharmBase): |
78 | self.evaluate_status() |
79 | event.defer() |
80 | return |
81 | + else: |
82 | + # With config options stored, we can try to configure the snap |
83 | + for option in all_config_options: |
84 | + if option in self.config: |
85 | + value = self.config[option] |
86 | + logger.info(f"Storing config item {option}") |
87 | + self._stored.__setattr__(option, value) |
88 | |
89 | self.handle_pgsql_dsn_change() |
90 | |
91 | @@ -103,30 +107,14 @@ class SnapstoreProxyCharm(CharmBase): |
92 | try: |
93 | self.validate_config_item() |
94 | except ConfigException as exc: |
95 | + logger.error(f"Value for {exc.message} is invalid") |
96 | self.errors.append(f"Could not parse the config option for {exc.message}") |
97 | self.evaluate_status() |
98 | return |
99 | - else: |
100 | - # With config options stored, we can try to configure the snap |
101 | - for option in all_config_options: |
102 | - |
103 | - if getattr(self._stored, "registered", False): |
104 | - # We are registered, so we hold any updates of the domain |
105 | - if option == "domain": |
106 | - continue |
107 | - |
108 | - if option in self.config: |
109 | - value = self.config[option] |
110 | - logger.debug(f"Storing {value} in {option}") |
111 | - self._stored.__setattr__(option, value) |
112 | |
113 | # Then configure it, and only then we can attempt to configure it |
114 | self.handle_config() |
115 | |
116 | - if not getattr(self._stored, "registered", False): |
117 | - self.unit.status = MaintenanceStatus("Registering proxy") |
118 | - self.handle_registration(event) |
119 | - |
120 | self.evaluate_status() |
121 | |
122 | def validate_config_item(self): |
123 | @@ -174,7 +162,7 @@ class SnapstoreProxyCharm(CharmBase): |
124 | self.errors.append("Failed to install the snap-store-proxy") |
125 | |
126 | def handle_config(self): |
127 | - |
128 | + logger.info("Testing to see if a config value has been updated") |
129 | # We will store dicts of item, potential new value in this list |
130 | to_update = [] |
131 | |
132 | @@ -194,13 +182,9 @@ class SnapstoreProxyCharm(CharmBase): |
133 | if current_md5 is None: |
134 | # A new entry, so we handle it |
135 | to_update.append({"name": item, "value": new_value, "md5": new_md5}) |
136 | - logger.info(f"Adding new item {item}") |
137 | elif current_md5 != new_md5: |
138 | # a different md5, so we update |
139 | to_update.append({"name": item, "value": new_value, "md5": new_md5}) |
140 | - logger.info(f"Updating value for {item}") |
141 | - else: |
142 | - logger.info(f"Skipping update for {item}, no new value") |
143 | |
144 | self.do_snap_updates(to_update) |
145 | |
146 | @@ -214,25 +198,7 @@ class SnapstoreProxyCharm(CharmBase): |
147 | for item in updates: |
148 | has_registration_bundle |= item["name"] == "registration_bundle" |
149 | |
150 | - # If the has_registration_bundle is true, we omit changes to the domain |
151 | - for item in updates: |
152 | - if item["name"] == "domain": |
153 | - logger.info("Configuring the domain") |
154 | - # Handle the storage of the domain |
155 | - # but we cannot actually store the url, so we need to break it |
156 | - # down to the hostname only |
157 | - hostname = urlparse(item["value"]).netloc.split(":")[0] |
158 | - configure_proxy("proxy.domain", hostname) |
159 | - else: |
160 | - logger.debug(f"Nothing todo for {item['name']}") |
161 | - |
162 | - # Ensure we record the values in the charm storage |
163 | - setattr(self._stored, item["name"], item["value"]) |
164 | - setattr(self._stored, f"{item['name']}_md5", item["value"]) |
165 | - |
166 | if has_registration_bundle: |
167 | - # If this config option is set, we basically ignore the username and |
168 | - # password and domain settings. And we take all from this field. |
169 | for item in updates: |
170 | if item["name"] == "registration_bundle": |
171 | break |
172 | @@ -297,39 +263,6 @@ class SnapstoreProxyCharm(CharmBase): |
173 | logger.debug("Stored the store id from the registration bundle") |
174 | configure_proxy(options, values, force=True) |
175 | |
176 | - def handle_registration(self, event): |
177 | - """Try to register the store. |
178 | - |
179 | - Depends on both a username and password provided for the registration. |
180 | - """ |
181 | - if getattr(self._stored, "registered", False): |
182 | - logger.info("This proxy is already registered") |
183 | - return |
184 | - |
185 | - if self._stored.db_uri is None: |
186 | - logger.info("The database relation is not yet setup") |
187 | - # defer, so we get retried in the future |
188 | - event.defer() |
189 | - return |
190 | - |
191 | - if ( |
192 | - getattr(self._stored, "username", None) is None |
193 | - or getattr(self._stored, "password", None) is None |
194 | - ): |
195 | - logger.info("Missing username or password, unable to register") |
196 | - return |
197 | - |
198 | - if getattr(self._stored, "domain", None) is None: |
199 | - logger.info("Missing domain needed to register") |
200 | - return |
201 | - |
202 | - registered = register_store(self._stored.username, self._stored.password) |
203 | - if registered: |
204 | - logger.info("Proxy is registered successfully") |
205 | - else: |
206 | - logger.info("Failed to register, check logging to find the reason") |
207 | - self._stored.registered = registered |
208 | - |
209 | def handle_pgsql_dsn_change(self): |
210 | """If the database is configured, then we try to configure the snap.""" |
211 | installation_source = getattr(self._stored, "snap_install_source", None) |
212 | @@ -352,25 +285,9 @@ class SnapstoreProxyCharm(CharmBase): |
213 | self.unit.status = BlockedStatus("Missing database relation") |
214 | return |
215 | |
216 | - if not hasattr(self._stored, "username") and ( |
217 | - not getattr(self._stored, "registered", False) |
218 | - ): |
219 | - self.unit.status = BlockedStatus( |
220 | - "Missing username, needed for registration" |
221 | - ) |
222 | - return |
223 | - |
224 | - if not hasattr(self._stored, "password") and ( |
225 | - not getattr(self._stored, "registered", False) |
226 | - ): |
227 | - self.unit.status = BlockedStatus( |
228 | - "Missing password, needed for registration" |
229 | - ) |
230 | - return |
231 | - |
232 | if not getattr(self._stored, "registered", False): |
233 | self.unit.status = BlockedStatus( |
234 | - "The unit is not registered, please supply username and password" |
235 | + "The unit is not registered, please supply a registration_bundle" |
236 | ) |
237 | return |
238 | |
239 | diff --git a/src/helpers.py b/src/helpers.py |
240 | index 1a53ad2..f4000ac 100644 |
241 | --- a/src/helpers.py |
242 | +++ b/src/helpers.py |
243 | @@ -46,17 +46,8 @@ def config_options(): |
244 | type_dict = {} |
245 | default_value = {} |
246 | for item in config["options"].keys(): |
247 | - default = config["options"][item]["default"] |
248 | - if default: |
249 | - default_value.update({item: default}) |
250 | - if item == "domain": |
251 | - type_dict.update({item: "url"}) |
252 | - elif item == "username": |
253 | - type_dict.update({item: "email"}) |
254 | - elif item == "registration_bundle": |
255 | + if item == "registration_bundle": |
256 | type_dict.update({item: "base64+json"}) |
257 | - else: |
258 | - type_dict.update({item: config["options"][item]["type"]}) |
259 | |
260 | return type_dict, default_value |
261 | |
262 | diff --git a/src/resource_helpers.py b/src/resource_helpers.py |
263 | index 007683b..b8628bb 100644 |
264 | --- a/src/resource_helpers.py |
265 | +++ b/src/resource_helpers.py |
266 | @@ -4,7 +4,7 @@ |
267 | |
268 | import hashlib |
269 | import logging |
270 | -from subprocess import PIPE, CalledProcessError, check_output, run |
271 | +from subprocess import PIPE, run |
272 | |
273 | logger = logging.getLogger(__name__) |
274 | |
275 | @@ -39,26 +39,6 @@ def snap_details(snap): |
276 | ) |
277 | |
278 | |
279 | -def register_store(username, password): |
280 | - """Use the provided username and password in an attempt to register. |
281 | - |
282 | - Returns a boolean indicating success or failure""" |
283 | - env = { |
284 | - "SNAPSTORE_EMAIL": username, |
285 | - "SNAPSTORE_PASSWORD": password, |
286 | - } |
287 | - try: |
288 | - check_output( |
289 | - ["/snap/bin/snap-store-proxy", "register", "--skip-questions"], env=env |
290 | - ) |
291 | - except CalledProcessError as error: |
292 | - logger.error(f"Registration of proxy failed, error; {error.output}") |
293 | - return False |
294 | - else: |
295 | - logger.info("Proxy registred sucessfully") |
296 | - return True |
297 | - |
298 | - |
299 | def create_database(uri): |
300 | """Let the proxy configure the database.""" |
301 | run(["/snap/bin/snap-store-proxy", "create-database", uri]) |
302 | diff --git a/tests/test_charm.py b/tests/test_charm.py |
303 | index 9fdac99..b92efce 100644 |
304 | --- a/tests/test_charm.py |
305 | +++ b/tests/test_charm.py |
306 | @@ -5,14 +5,14 @@ |
307 | # |
308 | |
309 | import base64 |
310 | -import logging |
311 | +import hashlib |
312 | import subprocess |
313 | from pathlib import Path |
314 | from unittest.mock import MagicMock, patch |
315 | |
316 | import pytest |
317 | import yaml |
318 | -from ops.model import ActiveStatus, BlockedStatus |
319 | +from ops.model import BlockedStatus |
320 | from ops.testing import Harness |
321 | |
322 | from charm import DATABASE_NAME, SnapstoreProxyCharm |
323 | @@ -49,18 +49,6 @@ class TestConfig(CharmTest): |
324 | # Accept the default value of None, for items that don't have a default value |
325 | assert getattr(self.harness.charm._stored, key, None) == value |
326 | |
327 | - def test_config_changed_option_update(self): |
328 | - # Our implicit requirement is that the database uri has been set. |
329 | - self.harness.charm._stored.db_uri = "postgresql://fake/database" |
330 | - self.harness.charm._stored.snap_install_source = "store" |
331 | - self.harness.charm._stored.database_created = True |
332 | - |
333 | - # Since valid values will be pushed on to the proxy, we need to stub that here |
334 | - with patch("charm.configure_proxy"): |
335 | - self.harness.update_config({"domain": "http://foo.com"}) |
336 | - |
337 | - assert self.harness.charm._stored.domain == "http://foo.com" |
338 | - |
339 | def test_on_config_changed(self): |
340 | # When there is an upodate, but no database set, we should end up with an |
341 | # Missing database relation blocked state |
342 | @@ -91,111 +79,106 @@ class TestConfig(CharmTest): |
343 | "Snap has not been installed" |
344 | ) |
345 | |
346 | - def test_field_is_updated(self): |
347 | - # Check that we have an update is the md5 matches |
348 | + def test_on_config_changed_invalid_config_item(self): |
349 | + self.harness.charm._stored.db_uri = "postgresql://fake/database" |
350 | + self.harness.charm.errors = [] |
351 | + |
352 | + with patch.object(self.harness.charm, "handle_pgsql_dsn_change"): |
353 | + self.harness.update_config({"registration_bundle": "not base 64"}) |
354 | + assert ( |
355 | + "Could not parse the config option for Charm option 'registration_bundle'" |
356 | + in self.harness.charm.errors[0] |
357 | + ) |
358 | + |
359 | + def test_handle_pgsql_dsn_change(self): |
360 | + self.harness.charm._stored.db_uri = "postgresql://fake/database" |
361 | + self.harness.charm._stored.snap_install_source = "store" |
362 | + |
363 | + with patch("charm.create_database") as patched_proxy: |
364 | + self.harness.charm.handle_pgsql_dsn_change() |
365 | + |
366 | + assert patched_proxy.called |
367 | + patched_proxy.assert_called_once_with("postgresql://fake/database") |
368 | |
369 | - self.harness.charm._stored.domain = "My new domain" |
370 | - self.harness.charm._stored.domain_md5 = "test" |
371 | + def test_new_config(self): |
372 | + self.harness.charm._stored.db_uri = "postgresql://fake/database" |
373 | + self.harness.charm._stored.snap_install_source = "store" |
374 | |
375 | + # Simulate an update |
376 | with patch.object( |
377 | self.harness.charm, "do_snap_updates" |
378 | - ) as patched_do_snap_updates: |
379 | - self.harness.charm.handle_config() |
380 | + ) as patched_do_snap_updates, patch.object( |
381 | + self.harness.charm, "handle_pgsql_dsn_change" |
382 | + ): |
383 | + self.harness.update_config( |
384 | + { |
385 | + "registration_bundle": "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJs" |
386 | + "aWNfa2V5IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
387 | + } |
388 | + ) |
389 | |
390 | - # This will be called with an array of dicts, containing the new |
391 | - # md5 value and the new resource value |
392 | + # If we update a config option with the same value it should be a noop |
393 | patched_do_snap_updates.assert_called_once_with( |
394 | [ |
395 | { |
396 | - "name": "domain", |
397 | - "value": "My new domain", |
398 | - "md5": "79911e6e27b92c112ea0034734bf9f14", |
399 | + "name": "registration_bundle", |
400 | + "value": ( |
401 | + "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJsaWNfa2V5" |
402 | + "IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
403 | + ), |
404 | + "md5": "8ebd54932fb33367154ca70953b86288", |
405 | } |
406 | ] |
407 | ) |
408 | |
409 | - def test_field_is_not_updated(self, caplog): |
410 | - caplog.clear() |
411 | - caplog.set_level(logging.INFO) |
412 | - |
413 | - self.harness.charm._stored.snap_install_source = "store" |
414 | - self.harness.charm._stored.domain = "My new domain" |
415 | - # We know the md5 |
416 | - self.harness.charm._stored.domain_md5 = "79911e6e27b92c112ea0034734bf9f14" |
417 | - |
418 | - with patch.object( |
419 | - self.harness.charm, "do_snap_updates" |
420 | - ) as patched_do_snap_updates: |
421 | - self.harness.charm.handle_config() |
422 | - |
423 | - # As there are no fields to update, we expect an empty array |
424 | - patched_do_snap_updates.assert_called_once_with([]) |
425 | - assert len(caplog.records) == 1 |
426 | - assert caplog.records[0].message == "Skipping update for domain, no new value" |
427 | - |
428 | - @pytest.mark.parametrize( |
429 | - "config,value", |
430 | - [ |
431 | - ("domain", "not.valid"), |
432 | - ("username", 1), |
433 | - ("password", 1), |
434 | - ], |
435 | - ) |
436 | - def test_config_validation_invalid(self, config, value): |
437 | - """ |
438 | - Here we test both that tests work and fail. |
439 | - |
440 | - Using 2 differen parameterize decorators, mean that they iterate over |
441 | - eachother, basically multiplying the number of tests executed. |
442 | - """ |
443 | - # Our implicit requirement is that the database uri has been set. |
444 | + def test_config_has_changed(self): |
445 | self.harness.charm._stored.db_uri = "postgresql://fake/database" |
446 | - self.harness.charm.errors = [] |
447 | + self.harness.charm._stored.registration_bundle = "test bundle" |
448 | + self.harness.charm._stored.registration_bundle_md5 = hashlib.md5( |
449 | + "test bundle".encode() |
450 | + ).hexdigest() |
451 | self.harness.charm._stored.snap_install_source = "store" |
452 | - self.harness.charm._stored.database_created = True |
453 | |
454 | - self.harness.update_config({config: value}) |
455 | + # Simulate an update |
456 | + with patch.object(self.harness.charm, "handle_pgsql_dsn_change"): |
457 | + self.harness.update_config( |
458 | + { |
459 | + "registration_bundle": "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJsaWNfa2V5" |
460 | + "IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
461 | + } |
462 | + ) |
463 | |
464 | - assert len(self.harness.charm.errors) == 1 |
465 | - assert ( |
466 | - f"Could not parse the config option for Charm option '{config}'" |
467 | - in self.harness.charm.errors[0] |
468 | + assert self.harness.charm._stored.registration_bundle == ( |
469 | + "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJsaWNfa2V5" |
470 | + "IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
471 | ) |
472 | |
473 | - @pytest.mark.parametrize( |
474 | - "config,value", |
475 | - [ |
476 | - ("domain", "http://valid.domain"), |
477 | - ("username", "user@domain.country"), |
478 | - ("password", "a valid string"), |
479 | - ], |
480 | - ) |
481 | - def test_config_validation_valid(self, config, value): |
482 | - """ |
483 | - Here we test both that tests work and fail. |
484 | - |
485 | - Using 2 differen parameterize decorators, mean that they iterate over |
486 | - eachother, basically multiplying the number of tests executed. |
487 | - """ |
488 | - # Our implicit requirement is that the database uri has been set. |
489 | - self.harness.charm._stored.db_uri = "postgresql://fake/database" |
490 | - self.harness.charm._stored.snap_install_source = "store" |
491 | - self.harness.charm._stored.database_created = True |
492 | - |
493 | - # Since valid values will be pushed on to the proxy, we need to stub that here |
494 | - with patch("charm.configure_proxy"): |
495 | - self.harness.update_config({config: value}) |
496 | - assert getattr(self.harness.charm._stored, config) == value |
497 | - |
498 | - def test_handle_pgsql_dsn_change(self): |
499 | + def test_config_has_not_changed(self): |
500 | self.harness.charm._stored.db_uri = "postgresql://fake/database" |
501 | + self.harness.charm._stored.registration_bundle = ( |
502 | + "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJsaWNfa2V5" |
503 | + "IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
504 | + ) |
505 | + self.harness.charm._stored.registration_bundle_md5 = ( |
506 | + "8ebd54932fb33367154ca70953b86288" |
507 | + ) |
508 | self.harness.charm._stored.snap_install_source = "store" |
509 | |
510 | - with patch("charm.create_database") as patched_proxy: |
511 | - self.harness.charm.handle_pgsql_dsn_change() |
512 | + # Simulate an update |
513 | + with patch.object( |
514 | + self.harness.charm, "do_snap_updates" |
515 | + ) as patched_do_snap_updates, patch.object( |
516 | + self.harness.charm, "handle_pgsql_dsn_change" |
517 | + ): |
518 | + self.harness.update_config( |
519 | + { |
520 | + "registration_bundle": "eyJkb21haW4iOiAic2Rmc2QiLCJwdWJs" |
521 | + "aWNfa2V5IjogIjEyIiwicHJpdmF0ZV9rZXkiOiAiMTIifQ==" |
522 | + } |
523 | + ) |
524 | |
525 | - assert patched_proxy.called |
526 | - patched_proxy.assert_called_once_with("postgresql://fake/database") |
527 | + # If we update a config option with the same value it should be a noop |
528 | + patched_do_snap_updates.assert_called_once_with([]) |
529 | |
530 | |
531 | class TestRegistrationBundle(CharmTest): |
532 | @@ -353,54 +336,29 @@ class TestStatus(CharmTest): |
533 | assert self.harness.charm.unit.status.message == "Missing database relation" |
534 | assert isinstance(self.harness.charm.unit.status, BlockedStatus) |
535 | |
536 | - def test_status_missing_username(self): |
537 | - self.harness.charm._stored.db_uri = "Filled, but useless" |
538 | - |
539 | - self.harness.charm.evaluate_status() |
540 | - |
541 | - assert ( |
542 | - self.harness.charm.unit.status.message |
543 | - == "Missing username, needed for registration" |
544 | - ) |
545 | - assert isinstance(self.harness.charm.unit.status, BlockedStatus) |
546 | - |
547 | - def test_status_missing_password(self): |
548 | - self.harness.charm._stored.db_uri = "Filled, but useless" |
549 | - self.harness.charm._stored.username = "minion" |
550 | - |
551 | - self.harness.charm.evaluate_status() |
552 | - |
553 | - assert ( |
554 | - self.harness.charm.unit.status.message |
555 | - == "Missing password, needed for registration" |
556 | - ) |
557 | - assert isinstance(self.harness.charm.unit.status, BlockedStatus) |
558 | - |
559 | def test_status_not_registered(self): |
560 | self.harness.charm._stored.db_uri = "Filled, but useless" |
561 | - self.harness.charm._stored.username = "minion" |
562 | - self.harness.charm._stored.password = "minions password" |
563 | self.harness.charm._stored.registered = None |
564 | |
565 | self.harness.charm.evaluate_status() |
566 | |
567 | assert ( |
568 | self.harness.charm.unit.status.message |
569 | - == "The unit is not registered, please supply username and password" |
570 | + == "The unit is not registered, please supply a registration_bundle" |
571 | ) |
572 | assert isinstance(self.harness.charm.unit.status, BlockedStatus) |
573 | |
574 | def test_unit_ready(self): |
575 | self.harness.charm._stored.db_uri = "Filled, but useless" |
576 | - self.harness.charm._stored.username = "minion" |
577 | - self.harness.charm._stored.password = "minions password" |
578 | self.harness.charm._stored.registered = True |
579 | - self.harness.charm._stored.domain = "dummy.domain" |
580 | + self.harness.charm._stored.domain = "https://dummy.domain" |
581 | |
582 | with patch("charm.open_port"): |
583 | self.harness.charm.evaluate_status() |
584 | |
585 | - assert self.harness.charm.unit.status.message == "Running on: dummy.domain" |
586 | + assert ( |
587 | + self.harness.charm.unit.status.message == "Running on: https://dummy.domain" |
588 | + ) |
589 | |
590 | |
591 | class TestEvents(CharmTest): |
592 | @@ -520,151 +478,6 @@ class TestEvents(CharmTest): |
593 | ) |
594 | |
595 | |
596 | -class TestRegistration(CharmTest): |
597 | - def test_handle_registration_no_database(self, caplog): |
598 | - caplog.clear() |
599 | - caplog.set_level(logging.INFO) |
600 | - event = MagicMock() |
601 | - |
602 | - self.harness.charm.handle_registration(event) |
603 | - |
604 | - assert len(caplog.records) == 1 |
605 | - assert caplog.records[0].message == "The database relation is not yet setup" |
606 | - |
607 | - def test_handle_registration_no_details(self, caplog): |
608 | - caplog.clear() |
609 | - caplog.set_level(logging.INFO) |
610 | - |
611 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
612 | - event = MagicMock() |
613 | - |
614 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
615 | - self.harness.charm.handle_registration(event) |
616 | - |
617 | - assert len(caplog.records) == 1 |
618 | - assert ( |
619 | - caplog.records[0].message |
620 | - == "Missing username or password, unable to register" |
621 | - ) |
622 | - |
623 | - def test_handle_registration_already_registered(self, caplog): |
624 | - caplog.clear() |
625 | - caplog.set_level(logging.INFO) |
626 | - |
627 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
628 | - self.harness.charm._stored.registered = True |
629 | - event = MagicMock() |
630 | - |
631 | - self.harness.charm.handle_registration(event) |
632 | - |
633 | - assert len(caplog.records) == 1 |
634 | - assert caplog.records[0].message == "This proxy is already registered" |
635 | - |
636 | - def test_handle_registration_missing_domain(self, caplog): |
637 | - caplog.clear() |
638 | - caplog.set_level(logging.INFO) |
639 | - |
640 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
641 | - self.harness.charm._stored.registered = False |
642 | - self.harness.charm._stored.username = "user@domain.country" |
643 | - self.harness.charm._stored.password = "password" |
644 | - event = MagicMock() |
645 | - |
646 | - self.harness.charm.handle_registration(event) |
647 | - |
648 | - assert len(caplog.records) == 1 |
649 | - assert caplog.records[0].message == "Missing domain needed to register" |
650 | - |
651 | - def test_handle_registration_register_succeeds(self, caplog): |
652 | - caplog.clear() |
653 | - caplog.set_level(logging.INFO) |
654 | - |
655 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
656 | - self.harness.charm._stored.registered = False |
657 | - self.harness.charm._stored.username = "user@domain.country" |
658 | - self.harness.charm._stored.password = "password" |
659 | - self.harness.charm._stored.domain = "http://localhost" |
660 | - event = MagicMock() |
661 | - |
662 | - with patch("charm.register_store") as patched_registration: |
663 | - patched_registration.return_value = True |
664 | - self.harness.charm.handle_registration(event) |
665 | - |
666 | - assert len(caplog.records) == 1 |
667 | - assert caplog.records[0].message == "Proxy is registered successfully" |
668 | - |
669 | - def test_handle_registration_register_fails(self, caplog): |
670 | - caplog.clear() |
671 | - caplog.set_level(logging.INFO) |
672 | - |
673 | - self.harness.charm._stored.db_uri = "postrgresql://fake/database" |
674 | - self.harness.charm._stored.registered = False |
675 | - self.harness.charm._stored.username = "user@domain.country" |
676 | - self.harness.charm._stored.password = "password" |
677 | - self.harness.charm._stored.domain = "localhost" |
678 | - event = MagicMock() |
679 | - |
680 | - with patch("charm.register_store") as patched_registration: |
681 | - patched_registration.return_value = False |
682 | - self.harness.charm.handle_registration(event) |
683 | - |
684 | - assert len(caplog.records) == 1 |
685 | - assert ( |
686 | - caplog.records[0].message |
687 | - == "Failed to register, check logging to find the reason" |
688 | - ) |
689 | - |
690 | - |
691 | -class TestSanity(CharmTest): |
692 | - def test_empty_username_after_registration_and_active_status(self): |
693 | - self.harness.charm._stored.snap_install_source = "store" |
694 | - self.harness.charm._stored.registered = True |
695 | - self.harness.charm._stored.database_created = True |
696 | - self.harness.charm._stored.username = "Some value" |
697 | - self.harness.charm._stored.db_uri = "postgresql://user@host/database" |
698 | - |
699 | - # Since valid values will be pushed on to the proxy, we need to stub that here |
700 | - with patch("charm.configure_proxy"), patch("charm.open_port"): |
701 | - # This should not result in a change, as none is not possible to be passed on |
702 | - self.harness.update_config({"username": None}) |
703 | - |
704 | - assert self.harness.charm._stored.username == "Some value" |
705 | - assert self.harness.charm.unit.status == ActiveStatus( |
706 | - "Running on: http://localhost.localdomain" |
707 | - ) |
708 | - |
709 | - def test_empty_password_after_registration_and_active_status(self): |
710 | - self.harness.charm._stored.registered = True |
711 | - self.harness.charm._stored.database_created = True |
712 | - self.harness.charm._stored.password = "Some value" |
713 | - self.harness.charm._stored.db_uri = "postgresql://user@host/database" |
714 | - |
715 | - # Since valid values will be pushed on to the proxy, we need to stub that here |
716 | - with patch("charm.configure_proxy"), patch("charm.open_port"): |
717 | - self.harness.update_config({"password": None}) |
718 | - |
719 | - assert self.harness.charm._stored.password == "Some value" |
720 | - assert self.harness.charm.unit.status == ActiveStatus( |
721 | - "Running on: http://localhost.localdomain" |
722 | - ) |
723 | - |
724 | - def test_update_domain_after_registration(self): |
725 | - self.harness.charm._stored.snap_install_source = "store" |
726 | - self.harness.charm._stored.registered = True |
727 | - self.harness.charm._stored.database_created = True |
728 | - self.harness.charm._stored.domain = "http://Some.value" |
729 | - self.harness.charm._stored.db_uri = "postgresql://user@host/database" |
730 | - |
731 | - # Since valid values will be pushed on to the proxy, we need to stub that here |
732 | - with patch("charm.configure_proxy"), patch("charm.open_port"): |
733 | - self.harness.update_config({"domain": "http://my.super.domain"}) |
734 | - |
735 | - assert self.harness.charm._stored.domain == "http://Some.value" |
736 | - assert self.harness.charm.unit.status == ActiveStatus( |
737 | - "Running on: http://Some.value" |
738 | - ) |
739 | - |
740 | - |
741 | class TestInstallation(CharmTest): |
742 | def test_install_from_resource(self): |
743 | self.harness.add_resource("core", "Dummy core") |
744 | diff --git a/tests/test_resource_helpers.py b/tests/test_resource_helpers.py |
745 | index 2827148..66e7ade 100644 |
746 | --- a/tests/test_resource_helpers.py |
747 | +++ b/tests/test_resource_helpers.py |
748 | @@ -1,5 +1,4 @@ |
749 | -import logging |
750 | -from subprocess import PIPE, CalledProcessError |
751 | +from subprocess import PIPE |
752 | from unittest.mock import MagicMock, call, mock_open, patch |
753 | |
754 | import resource_helpers |
755 | @@ -37,56 +36,6 @@ def test_snap_details(): |
756 | patched_run.assert_called_once_with(["snap", "list", "mysnap"], stdout=PIPE) |
757 | |
758 | |
759 | -def test_register_store_succeeds(caplog): |
760 | - username = "username@domain.ext" |
761 | - password = "super secret" |
762 | - env = { |
763 | - "SNAPSTORE_EMAIL": username, |
764 | - "SNAPSTORE_PASSWORD": password, |
765 | - } |
766 | - caplog.clear() |
767 | - caplog.set_level(logging.INFO) |
768 | - |
769 | - with patch("resource_helpers.check_output") as patched_check_output: |
770 | - result = resource_helpers.register_store(username, password) |
771 | - |
772 | - assert result |
773 | - patched_check_output.assert_called_once_with( |
774 | - ["/snap/bin/snap-store-proxy", "register", "--skip-questions"], env=env |
775 | - ) |
776 | - assert len(caplog.records) == 1 |
777 | - assert caplog.records[0].message == "Proxy registred sucessfully" |
778 | - |
779 | - |
780 | -def test_register_store_fails(caplog): |
781 | - username = "username@domain.ext" |
782 | - password = "super secret" |
783 | - env = { |
784 | - "SNAPSTORE_EMAIL": username, |
785 | - "SNAPSTORE_PASSWORD": password, |
786 | - } |
787 | - caplog.clear() |
788 | - caplog.set_level(logging.INFO) |
789 | - |
790 | - with patch("resource_helpers.check_output") as patched_check_output: |
791 | - patched_check_output.side_effect = CalledProcessError( |
792 | - 1, |
793 | - ["/snap/bin/snap-store-proxy", "register", "--skip-questions"], |
794 | - "my dummy error", |
795 | - ) |
796 | - result = resource_helpers.register_store(username, password) |
797 | - |
798 | - assert not result |
799 | - patched_check_output.assert_called_once_with( |
800 | - ["/snap/bin/snap-store-proxy", "register", "--skip-questions"], env=env |
801 | - ) |
802 | - assert len(caplog.records) == 1 |
803 | - assert ( |
804 | - caplog.records[0].message |
805 | - == "Registration of proxy failed, error; my dummy error" |
806 | - ) |
807 | - |
808 | - |
809 | def test_create_database(): |
810 | test_dsn = "postgresql://user:password@host/database" |
811 | with patch("resource_helpers.run") as patched_run: |
I don't know this repo, so won't be offended if you seek extra review, but the diffs look good to me. I have some very tiny comments in the diffs.