wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Tom Haddon has proposed merging ~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:use-leadership-interface into charm-k8s-wordpress:master.

Commit message:
Use leadership interface for WORDPRESS_SECRETS rather than custom functions

Requested reviews:
  Wordpress Charmers (wordpress-charmers)

For more details, see:
https://code.launchpad.net/~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396194

Use leadership interface for WORDPRESS_SECRETS rather than custom functions
-- 
Your team Wordpress Charmers is requested to review the proposed merge of ~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:use-leadership-interface into charm-k8s-wordpress:master.

diff --git a/src/charm.py b/src/charm.py
index 3dcb33e..1bb2749 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -2,7 +2,6 @@
 
 import io
 import logging
-import subprocess
 from pprint import pprint
 from yaml import safe_load
 
@@ -64,29 +63,6 @@ def generate_pod_config(config, secured=True):
     return pod_config
 
 
-def _leader_get(attribute):
-    cmd = ['leader-get', '--format=yaml', attribute]
-    return safe_load(subprocess.check_output(cmd).decode('UTF-8'))
-
-
-def _leader_set(settings):
-    cmd = ['leader-set'] + ['{}={}'.format(k, v or '') for k, v in settings.items()]
-    subprocess.check_call(cmd)
-
-
-def create_wordpress_secrets():
-    for secret in WORDPRESS_SECRETS:
-        if not _leader_get(secret):
-            _leader_set({secret: password_generator(64)})
-
-
-def gather_wordpress_secrets():
-    rv = {}
-    for secret in WORDPRESS_SECRETS:
-        rv[secret] = _leader_get(secret)
-    return rv
-
-
 class WordpressInitialiseEvent(EventBase):
     """Custom event for signalling Wordpress initialisation.
 
@@ -233,7 +209,6 @@ class WordpressCharm(CharmBase):
     def configure_pod(self):
         # Only the leader can set_spec().
         if self.model.unit.is_leader():
-            create_wordpress_secrets()
             resources = self.make_pod_resources()
             spec = self.make_pod_spec()
             spec.update(resources)
@@ -306,7 +281,7 @@ class WordpressCharm(CharmBase):
         config["db_password"] = self.state.db_password
 
         full_pod_config = generate_pod_config(config, secured=False)
-        full_pod_config.update(gather_wordpress_secrets())
+        full_pod_config.update(self._get_wordpress_secrets())
         secure_pod_config = generate_pod_config(config, secured=True)
 
         ports = [
@@ -373,6 +348,23 @@ class WordpressCharm(CharmBase):
         except Exception:
             logger.info("We don't have any ingress addresses yet")
 
+    def _get_wordpress_secrets(self):
+        """Get secrets, creating them if they don't exist.
+
+        These are part of the pod spec, and so this function can only be run
+        on the leader. We can therefore safely generate them if they don't
+        already exist."""
+        wp_secrets = {}
+        for secret in WORDPRESS_SECRETS:
+            # `self.leader_data` itself will never return a KeyError, but
+            # checking for the presence of an item before setting it will make
+            # it easier to test, as we can simply set `self.leader_data` to
+            # be a dictionary.
+            if secret not in self.leader_data or not self.leader_data[secret]:
+                self.leader_data[secret] = password_generator(64)
+            wp_secrets[secret] = self.leader_data[secret]
+        return wp_secrets
+
     def is_service_up(self):
         """Check to see if the HTTP service is up"""
         service_ip = self.get_service_ip()
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 7f6dc54..0d010a7 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -6,7 +6,7 @@ import unittest
 
 from unittest.mock import Mock
 
-from charm import WordpressCharm, create_wordpress_secrets, gather_wordpress_secrets
+from charm import WordpressCharm
 from wordpress import WORDPRESS_SECRETS
 from ops import testing
 from ops.model import (
@@ -18,16 +18,6 @@ from ops.model import (
 from test_wordpress import TEST_MODEL_CONFIG
 
 
-class TestLeadershipData:
-    data = {}
-
-    def _leader_set(self, d):
-        self.data.update(d)
-
-    def _leader_get(self, k):
-        return self.data.get(k)
-
-
 class TestWordpressCharm(unittest.TestCase):
 
     test_model_config = TEST_MODEL_CONFIG
@@ -99,24 +89,11 @@ class TestWordpressCharm(unittest.TestCase):
         self.assertEqual(self.harness.charm.unit.status.message, expected_msg)
         self.assertLogs(expected_msg, level="INFO")
 
-    @mock.patch("charm._leader_set")
-    @mock.patch("charm._leader_get")
-    def test_create_wordpress_secrets(self, _leader_get_func, _leader_set_func):
-        leadership_data = TestLeadershipData()
-        _leader_set_func.side_effect = leadership_data._leader_set
-        _leader_get_func.side_effect = leadership_data._leader_get
-        create_wordpress_secrets()
-
-        self.assertEqual(sorted(list(leadership_data.data.keys())), sorted(WORDPRESS_SECRETS))
-
-    @mock.patch("charm._leader_set")
-    @mock.patch("charm._leader_get")
-    def test_gather_wordpress_secrets(self, _leader_get_func, _leader_set_func):
-        leadership_data = TestLeadershipData()
-        _leader_set_func.side_effect = leadership_data._leader_set
-        _leader_get_func.side_effect = leadership_data._leader_get
-        create_wordpress_secrets()
-        wp_secrets = gather_wordpress_secrets()
+    def test_get_wordpress_secrets(self):
+        # Set leader_data to an empty dict to avoid subsequent calls to
+        # `leader-get` and `leader-set` in this test.
+        self.harness.charm.leader_data = {}
+        wp_secrets = self.harness.charm._get_wordpress_secrets()
         for key in WORDPRESS_SECRETS:
             self.assertIsInstance(wp_secrets[key], str)
             self.assertEqual(len(wp_secrets[key]), 64)
@@ -207,13 +184,10 @@ class TestWordpressCharm(unittest.TestCase):
             self.harness.charm._on_get_initial_password_action(action_event)
             self.assertEqual(action_event.set_results.call_args, mock.call({"password": "passwd"}))
 
-    @mock.patch("charm._leader_set")
-    @mock.patch("charm._leader_get")
-    def test_configure_pod(self, _leader_get_func, _leader_set_func):
-        leadership_data = TestLeadershipData()
-        _leader_set_func.side_effect = leadership_data._leader_set
-        _leader_get_func.side_effect = leadership_data._leader_get
-
+    def test_configure_pod(self):
+        # Set leader_data to an empty dict to avoid subsequent calls to
+        # `leader-get` and `leader-set` in this test.
+        self.harness.charm.leader_data = {}
         # First of all, test with leader set, but not initialised.
         self.harness.set_leader(True)
         self.assertEqual(self.harness.charm.state.initialised, False)