Merge ~jk0ne/charm-k8s-discourse/+git/charm-k8s-discourse:master into charm-k8s-discourse:master

[Jay Kuri]

Test rewrite per stub's feedback.

[Jay Kuri (jk0ne)]

Updated to finish core tests and test CMR routines. This gets us to 91% coverage... and I think this is ready for review, at least internally.

[Stuart Bishop (stub)]

This is all looking great. Some inline comments, nothing particularly important or that needs to block landing. If you agree, changes can be made on this branch before landing or in a subsequent cleanup branch.

review: Approve

[Tom Haddon (mthaddon)]

Just to add to stub's feedback, one comment about some test data.

Also, please add docstrings for all functions in src/charm.py and in the unit tests. Per review feedback that we've had on other charm changes:

Docstring's first sentence should be in one line, ending with a point (full stop). Then have a blank line and the rest of text you want.

Something like

"""Verify the config from Juju.

Specifically:

- check if all the required Juju config options are set

- check if all the Juju config options are properly formatted

:raises TelegrafK8sCharmJujuConfigError: if a required config is missing or wrongly formatted
"""

[Tom Haddon (mthaddon)]

Some minor comments inline.

Also, per the comment above please add docstrings to all functions/methods in src/charm.py and in the unit tests.

Thanks for the test data changes. Any objection to trying to find more descriptive names for the test data files than 'config_valid_{1,2,3}.yaml', for instance? Possibly something like 'config_valid_basic.yaml', 'config_valid_with_tls.yaml', etc.? I think this makes it a lot easier to understand for someone new to the code what's being tested.

[Tom Haddon (mthaddon)]

Some other comments from looking at the code outside of this MP.

In src/charm.py can we alpha-sort `from ops.model import MaintenanceStatus, BlockedStatus, ActiveStatus`?

Per a comment we got on the jenkins-agent charm recently, "Please use "store", not "state", for StoredState (with undercase if you want to make it private)." You'll obviously need to update a all the references to self.state as appropriate too.

In check_for_config_problems you have "if len(missing_fields):" but this could just be "if missing_fields:".

In check_config_is_valid you have "if len(errors) > 0:" but this could just be "if errors:".

Please alpha sort imports in the unit tests.

I'm not sure why you have 'dirname = os.path.dirname(__file__)' as a global variable in unit tests. Seems like you could just remove that and replace line 48 with:

self.configs = load_configs(os.path.join(os.path.dirname(__file__), 'fixtures'))

I don't think we need to import (or use) pprint in the unit tests any more? Looks like it was just for debugging of the load_configs function.

And lastly, just a suggestion, feel free to ignore if you like. In load_configs you have:

name = os.path.splitext(os.path.basename(filename))
configs[name[0]] = yaml.full_load(file)

This could be (which seems a little easier to follow to me, but YMMV):

name, _ = os.path.splitext(os.path.basename(filename))
configs[name] = yaml.full_load(file)

[Tom Haddon (mthaddon)]

Looks good, thanks for the follow ups

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision f1599379f9b7d3cac3c879dfe70e0ad6e3d7a87b

diff --git a/Makefile b/Makefile
index 00ca7aa..440712e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,4 @@
-DISCOURSE_VERSION ?= v2.5.1
+DISCOURSE_VERSION ?= v2.5.2
 IMAGE_VERSION ?= $(DISCOURSE_VERSION)
 IMAGE_NAME ?=discourse
 
diff --git a/image/Dockerfile b/image/Dockerfile
index 7baf5cb..96d49df 100644
--- a/image/Dockerfile
+++ b/image/Dockerfile
@@ -13,7 +13,7 @@ ARG CONTAINER_APP_GID
 
 # Copy any args we got into the environment.
 ENV CONTAINER_APP_NAME ${CONTAINER_APP_NAME:-discourse}
-ENV CONTAINER_APP_VERSION ${CONTAINER_APP_VERSION:-v2.4.3}
+ENV CONTAINER_APP_VERSION ${CONTAINER_APP_VERSION:-v2.5.2}
 ENV CONTAINER_APP_USERNAME ${CONTAINER_APP_USERNAME:-discourse}
 ENV CONTAINER_APP_UID ${CONTAINER_APP_UID:-200}
 ENV CONTAINER_APP_GROUP ${CONTAINER_APP_GROUP:-discourse}
diff --git a/src/charm.py b/src/charm.py
index b7e5e2b..bb33602 100755
--- a/src/charm.py
+++ b/src/charm.py
@@ -7,13 +7,14 @@ from ops.charm import CharmBase
 from ops.main import main
 from ops.framework import StoredState
 
-from ops.model import MaintenanceStatus, BlockedStatus, ActiveStatus
+from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus
 
 
 pgsql = ops.lib.use("pgsql", 1, "[e-mail]")
 
 
 def create_discourse_pod_config(config):
+    """Create the pod environment config from the juju config."""
     pod_config = {
         'DISCOURSE_POSTGRES_USERNAME': config['db_user'],
         'DISCOURSE_POSTGRES_PASSWORD': config['db_password'],
@@ -36,6 +37,7 @@ def create_discourse_pod_config(config):
 
 
 def create_ingress_config(app_name, config):
+    """Create the ingress config form the juju config."""
     annotations = {}
     ingressResource = {
         "name": app_name + "-ingress",
@@ -61,6 +63,13 @@ def create_ingress_config(app_name, config):
 
 
 def get_pod_spec(app_name, config):
+    """Get the entire pod spec using the juju config.
+
+    - uses create_discourse_pod_config() to generate pod envConfig.
+
+    - uses create_ingress_config() to generate pod ingressResources.
+
+    """
     pod_spec = {
         "version": 3,
         "containers": [
@@ -92,10 +101,16 @@ def get_pod_spec(app_name, config):
 
 
 def check_for_config_problems(config):
+    """Check if there are issues with the juju config.
+
+    - Primarily looks for missing config options using check_for_missing_config_fields()
+
+    - Returns a list of errors if any were found.
+    """
     errors = []
     missing_fields = check_for_missing_config_fields(config)
 
-    if len(missing_fields):
+    if missing_fields:
         errors.append('Required configuration missing: {}'.format(" ".join(missing_fields)))
 
     if "db_host" not in config or config["db_host"] is None:
@@ -105,6 +120,11 @@ def check_for_config_problems(config):
 
 
 def check_for_missing_config_fields(config):
+    """Check for missing fields in juju config.
+
+    - Returns a list of required fields that are either not present
+      or are empty.
+    """
     missing_fields = []
 
     needed_fields = [
@@ -125,13 +145,18 @@ def check_for_missing_config_fields(config):
 
 
 class DiscourseCharm(CharmBase):
-    state = StoredState()
+    stored = StoredState()
 
     def __init__(self, *args):
+        """Initialization.
+
+        - Primarily sets up defaults and event handlers.
+
+        """
         super().__init__(*args)
 
         # TODO: is_started is unused. Remove?
-        self.state.set_default(is_started=False, db_user=None, db_password=None, db_host=None)
+        self.stored.set_default(is_started=False, db_user=None, db_password=None, db_host=None)
         self.framework.observe(self.on.leader_elected, self.configure_pod)
         self.framework.observe(self.on.config_changed, self.configure_pod)
         self.framework.observe(self.on.upgrade_charm, self.configure_pod)
@@ -141,11 +166,17 @@ class DiscourseCharm(CharmBase):
         self.framework.observe(self.db.on.master_changed, self.on_database_changed)
 
     def check_config_is_valid(self, config):
+        """Check that the provided config is valid.
+
+        - Returns True if config is valid, False otherwise.
+
+        - Sets model status as appropriate.
+        """
         valid_config = True
         errors = check_for_config_problems(config)
 
-        # set status if we have a bad config
-        if len(errors) > 0:
+        # Set status if we have a bad config.
+        if errors:
             self.model.unit.status = BlockedStatus(", ".join(errors))
             valid_config = False
         else:
@@ -153,10 +184,13 @@ class DiscourseCharm(CharmBase):
 
         return valid_config
 
-    def get_pod_spec(self, config):
-        return get_pod_spec(self.framework.model.app.name, config)
-
     def configure_pod(self, event=None):
+        """Configure pod.
+
+        - Verifies config is valid and unit is leader.
+
+        - Configures pod using pod_spec generated from config.
+        """
         # Set our status while we get configured.
         self.model.unit.status = MaintenanceStatus('Configuring pod')
 
@@ -168,43 +202,49 @@ class DiscourseCharm(CharmBase):
             config = dict(self.model.config)
             if not config["db_name"]:
                 config["db_name"] = self.app.name
-            config["db_user"] = self.state.db_user
-            config["db_password"] = self.state.db_password
-            config["db_host"] = self.state.db_host
+            config["db_user"] = self.stored.db_user
+            config["db_password"] = self.stored.db_password
+            config["db_host"] = self.stored.db_host
             # Get our spec definition.
             if self.check_config_is_valid(config):
                 # Get pod spec using our app name and config
-                pod_spec = self.get_pod_spec(config)
+                pod_spec = get_pod_spec(self.framework.model.app.name, config)
                 # Set our pod spec.
                 self.model.pod.set_spec(pod_spec)
-                self.state.is_started = True
+                self.stored.is_started = True
                 self.model.unit.status = ActiveStatus()
         else:
-            self.state.is_started = True
+            self.stored.is_started = True
             self.model.unit.status = ActiveStatus()
 
-    # TODO: This is unused? Remove?
-    def on_new_client(self, event):
-        if not self.state.is_started:
-            return event.defer()
-        event.client.serve(hosts=[event.client.ingress_address], port=self.model.config['http_port'])
-
     def on_database_relation_joined(self, event):
+        """Event handler for a newly joined database relation.
+
+        - Sets the event.database field on the database joined event.
+
+        - Required because setting the database name is only possible
+          from inside the event handler per https://github.com/canonical/ops-lib-pgsql/issues/2
+        """
         # Per https://github.com/canonical/ops-lib-pgsql/issues/2,
         # changing the setting in the config will not take effect,
         # unless the relation is dropped and recreated.
         event.database = self.model.config["db_name"]
 
     def on_database_changed(self, event):
+        """Event handler for database relation change.
+
+        - Sets our database parameters based on what was provided
+          in the relation event.
+        """
         if event.master is None:
-            self.state.db_user = None
-            self.state.db_password = None
-            self.state.db_host = None
+            self.stored.db_user = None
+            self.stored.db_password = None
+            self.stored.db_host = None
             return
 
-        self.state.db_user = event.master.user
-        self.state.db_password = event.master.password
-        self.state.db_host = event.master.host
+        self.stored.db_user = event.master.user
+        self.stored.db_password = event.master.password
+        self.stored.db_host = event.master.host
 
         self.configure_pod()
 
diff --git a/tests/unit/fixtures/config_invalid_2.yaml b/tests/unit/fixtures/config_invalid_missing_cors.yaml
similarity index 76%
rename from tests/unit/fixtures/config_invalid_2.yaml
rename to tests/unit/fixtures/config_invalid_missing_cors.yaml
index 57ec729..da4a5c8 100644
--- a/tests/unit/fixtures/config_invalid_2.yaml
+++ b/tests/unit/fixtures/config_invalid_missing_cors.yaml
@@ -2,7 +2,7 @@ config:
     cors_origin: ''
     db_name: discourse
     db_host: 10.25.244.12
-    developer_emails: [e-mail]
+    developer_emails: [e-mail]
     discourse_image: discourse-k8s:1.0.7f
     enable_cors: true
     external_hostname: discourse.local
@@ -11,9 +11,10 @@ config:
     redis_host: 10.9.89.197
     smtp_address: 167.89.123.58
     smtp_authentication: login
-    smtp_domain: canonical.com
+    smtp_domain: example.com
     smtp_openssl_verify_mode: none
-    smtp_password: T6DY2MzWV0AJk
+    smtp_password: OBV10USLYF4K3
     smtp_port: 587
     smtp_username: apikey
-expected_error_status: 'Required configuration missing: cors_origin'
+missing_fields: 
+    - 'cors_origin'
diff --git a/tests/unit/fixtures/config_invalid_1.yaml b/tests/unit/fixtures/config_invalid_missing_db_name.yaml
similarity index 72%
rename from tests/unit/fixtures/config_invalid_1.yaml
rename to tests/unit/fixtures/config_invalid_missing_db_name.yaml
index e5df777..0e2ea39 100644
--- a/tests/unit/fixtures/config_invalid_1.yaml
+++ b/tests/unit/fixtures/config_invalid_missing_db_name.yaml
@@ -1,7 +1,6 @@
 config:
     cors_origin: '*'
-    db_name: discourse
-    developer_emails: [e-mail]
+    developer_emails: [e-mail]
     discourse_image: discourse-k8s:1.0.7f
     enable_cors: true
     external_hostname: discourse.local
@@ -10,9 +9,10 @@ config:
     redis_host: 10.9.89.197
     smtp_address: 167.89.123.58
     smtp_authentication: login
-    smtp_domain: canonical.com
+    smtp_domain: example.com
     smtp_openssl_verify_mode: none
-    smtp_password: T6DY2MzWV0AJk
+    smtp_password: OBV10USLYF4K3
     smtp_port: 587
     smtp_username: apikey
-expected_error_status: 'db relation is required'
+missing_fields: 
+    - 'db_name'
diff --git a/tests/unit/fixtures/config_valid_1.yaml b/tests/unit/fixtures/config_valid_complete.yaml
similarity index 57%
rename from tests/unit/fixtures/config_valid_1.yaml
rename to tests/unit/fixtures/config_valid_complete.yaml
index ae6aa35..28e7367 100644
--- a/tests/unit/fixtures/config_valid_1.yaml
+++ b/tests/unit/fixtures/config_valid_complete.yaml
@@ -4,7 +4,7 @@ config:
     db_name: discourse
     db_password: a_real_password
     db_user: discourse_m
-    developer_emails: [e-mail]
+    developer_emails: [e-mail]
     discourse_image: discourse-k8s:1.0.7f
     enable_cors: true
     external_hostname: discourse.local
@@ -13,54 +13,26 @@ config:
     redis_host: 10.9.89.197
     smtp_address: 167.89.123.58
     smtp_authentication: login
-    smtp_domain: canonical.com
+    smtp_domain: example.com
     smtp_openssl_verify_mode: none
-    smtp_password: T6DY2MzWV0AJk
+    smtp_password: OBV10USLYF4K3
     smtp_port: 587
     smtp_username: apikey
-spec:
-    containers:
-    - envConfig:
-        DISCOURSE_CORS_ORIGIN: '*'
-        DISCOURSE_DEVELOPER_EMAILS: [e-mail]
-        DISCOURSE_ENABLE_CORS: true
-        DISCOURSE_HOSTNAME: discourse.local
-        DISCOURSE_POSTGRES_HOST: 10.9.89.237
-        DISCOURSE_POSTGRES_NAME: discourse
-        DISCOURSE_POSTGRES_PASSWORD: a_real_password
-        DISCOURSE_POSTGRES_USERNAME: discourse_m
-        DISCOURSE_REDIS_HOST: 10.9.89.197
-        DISCOURSE_SMTP_ADDRESS: 167.89.123.58
-        DISCOURSE_SMTP_AUTHENTICATION: login
-        DISCOURSE_SMTP_DOMAIN: canonical.com
-        DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
-        DISCOURSE_SMTP_PASSWORD: T6DY2MzWV0AJk
-        DISCOURSE_SMTP_PORT: 587
-        DISCOURSE_SMTP_USER_NAME: apikey
-      imageDetails:
-        imagePath: discourse-k8s:1.0.7f
-      imagePullPolicy: IfNotPresent
-      kubernetes:
-        readinessProbe:
-          httpGet:
-            path: /srv/status
-            port: 3000
-      name: discourse
-      ports:
-      - containerPort: 3000
-        protocol: TCP
-    kubernetesResources:
-      ingressResources:
-      - name: discourse-ingress
-        annotations:
-          nginx.ingress.kubernetes.io/ssl-redirect: 'false'
-        spec:
-          rules:
-          - host: discourse.local
-            http:
-              paths:
-              - backend:
-                  serviceName: discourse
-                  servicePort: 3000
-                path: /
-    version: 3
+    tls_secret_name: discourse_local
+pod_config:
+    DISCOURSE_CORS_ORIGIN: '*'
+    DISCOURSE_DEVELOPER_EMAILS: [e-mail]
+    DISCOURSE_ENABLE_CORS: true
+    DISCOURSE_HOSTNAME: discourse.local
+    DISCOURSE_POSTGRES_HOST: 10.9.89.237
+    DISCOURSE_POSTGRES_NAME: discourse
+    DISCOURSE_POSTGRES_PASSWORD: a_real_password
+    DISCOURSE_POSTGRES_USERNAME: discourse_m
+    DISCOURSE_REDIS_HOST: 10.9.89.197
+    DISCOURSE_SMTP_ADDRESS: 167.89.123.58
+    DISCOURSE_SMTP_AUTHENTICATION: login
+    DISCOURSE_SMTP_DOMAIN: example.com
+    DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
+    DISCOURSE_SMTP_PASSWORD: OBV10USLYF4K3
+    DISCOURSE_SMTP_PORT: 587
+    DISCOURSE_SMTP_USER_NAME: apikey
diff --git a/tests/unit/fixtures/config_valid_3.yaml b/tests/unit/fixtures/config_valid_with_tls.yaml
similarity index 57%
rename from tests/unit/fixtures/config_valid_3.yaml
rename to tests/unit/fixtures/config_valid_with_tls.yaml
index b568c5a..fa19ce1 100644
--- a/tests/unit/fixtures/config_valid_3.yaml
+++ b/tests/unit/fixtures/config_valid_with_tls.yaml
@@ -4,7 +4,7 @@ config:
     db_name: discourse
     db_password: a_real_password
     db_user: discourse_m
-    developer_emails: [e-mail]
+    developer_emails: [e-mail]
     discourse_image: discourse-k8s:1.0.7f
     enable_cors: true
     external_hostname: discourse.local
@@ -13,58 +13,26 @@ config:
     redis_host: 10.9.89.197
     smtp_address: 167.89.123.58
     smtp_authentication: login
-    smtp_domain: canonical.com
+    smtp_domain: example.com
     smtp_openssl_verify_mode: none
-    smtp_password: T6DY2MzWV0AJk
+    smtp_password: OBV10USLYF4K3
     smtp_port: 587
     smtp_username: apikey
     tls_secret_name: discourse-local
-spec:
-    containers:
-    - envConfig:
-        DISCOURSE_CORS_ORIGIN: '*'
-        DISCOURSE_DEVELOPER_EMAILS: [e-mail]
-        DISCOURSE_ENABLE_CORS: true
-        DISCOURSE_HOSTNAME: discourse.local
-        DISCOURSE_POSTGRES_HOST: 10.9.89.237
-        DISCOURSE_POSTGRES_NAME: discourse
-        DISCOURSE_POSTGRES_PASSWORD: a_real_password
-        DISCOURSE_POSTGRES_USERNAME: discourse_m
-        DISCOURSE_REDIS_HOST: 10.9.89.197
-        DISCOURSE_SMTP_ADDRESS: 167.89.123.58
-        DISCOURSE_SMTP_AUTHENTICATION: login
-        DISCOURSE_SMTP_DOMAIN: canonical.com
-        DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
-        DISCOURSE_SMTP_PASSWORD: T6DY2MzWV0AJk
-        DISCOURSE_SMTP_PORT: 587
-        DISCOURSE_SMTP_USER_NAME: apikey
-      imageDetails:
-        imagePath: discourse-k8s:1.0.7f
-        username: discourse_role
-        password: discourse123
-      imagePullPolicy: IfNotPresent
-      kubernetes:
-        readinessProbe:
-          httpGet:
-            path: /srv/status
-            port: 3000
-      name: discourse
-      ports:
-      - containerPort: 3000
-        protocol: TCP
-    kubernetesResources:
-      ingressResources:
-      - name: discourse-ingress
-        spec:
-          tls:
-          - hosts: discourse.local
-            secretName: discourse-local
-          rules:
-          - host: discourse.local
-            http:
-              paths:
-              - backend:
-                  serviceName: discourse
-                  servicePort: 3000
-                path: /
-    version: 3
+pod_config:
+    DISCOURSE_CORS_ORIGIN: '*'
+    DISCOURSE_DEVELOPER_EMAILS: [e-mail]
+    DISCOURSE_ENABLE_CORS: true
+    DISCOURSE_HOSTNAME: discourse.local
+    DISCOURSE_POSTGRES_HOST: 10.9.89.237
+    DISCOURSE_POSTGRES_NAME: discourse
+    DISCOURSE_POSTGRES_PASSWORD: a_real_password
+    DISCOURSE_POSTGRES_USERNAME: discourse_m
+    DISCOURSE_REDIS_HOST: 10.9.89.197
+    DISCOURSE_SMTP_ADDRESS: 167.89.123.58
+    DISCOURSE_SMTP_AUTHENTICATION: login
+    DISCOURSE_SMTP_DOMAIN: example.com
+    DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
+    DISCOURSE_SMTP_PASSWORD: OBV10USLYF4K3
+    DISCOURSE_SMTP_PORT: 587
+    DISCOURSE_SMTP_USER_NAME: apikey
diff --git a/tests/unit/fixtures/config_valid_2.yaml b/tests/unit/fixtures/config_valid_without_tls.yaml
similarity index 62%
rename from tests/unit/fixtures/config_valid_2.yaml
rename to tests/unit/fixtures/config_valid_without_tls.yaml
index 9bd94ae..8c440b8 100644
--- a/tests/unit/fixtures/config_valid_2.yaml
+++ b/tests/unit/fixtures/config_valid_without_tls.yaml
@@ -4,7 +4,7 @@ config:
     db_name: discourse
     db_password: a_real_password
     db_user: discourse_m
-    developer_emails: [e-mail]
+    developer_emails: [e-mail]
     discourse_image: discourse-k8s:1.0.8
     enable_cors: true
     external_hostname: discourse.example.com
@@ -13,55 +13,25 @@ config:
     redis_host: 10.25.242.12
     smtp_address: smtp.internal
     smtp_authentication: login
-    smtp_domain: canonical.com
+    smtp_domain: example.com
     smtp_openssl_verify_mode: none
     smtp_password:
     smtp_port: 587
     smtp_username: apikey
-spec:
-    containers:
-    - envConfig:
-        DISCOURSE_CORS_ORIGIN: '*'
-        DISCOURSE_DEVELOPER_EMAILS: [e-mail]
-        DISCOURSE_ENABLE_CORS: true
-        DISCOURSE_HOSTNAME: discourse.example.com
-        DISCOURSE_POSTGRES_HOST: 10.9.89.237
-        DISCOURSE_POSTGRES_NAME: discourse
-        DISCOURSE_POSTGRES_PASSWORD: a_real_password
-        DISCOURSE_POSTGRES_USERNAME: discourse_m
-        DISCOURSE_REDIS_HOST: 10.25.242.12
-        DISCOURSE_SMTP_ADDRESS: smtp.internal
-        DISCOURSE_SMTP_AUTHENTICATION: login
-        DISCOURSE_SMTP_DOMAIN: canonical.com
-        DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
-        DISCOURSE_SMTP_PASSWORD: null
-        DISCOURSE_SMTP_PORT: 587
-        DISCOURSE_SMTP_USER_NAME: apikey
-      imageDetails:
-        imagePath: discourse-k8s:1.0.8
-      imagePullPolicy: IfNotPresent
-      kubernetes:
-        readinessProbe:
-          httpGet:
-            path: /srv/status
-            port: 3000
-      name: discourse
-      ports:
-      - containerPort: 3000
-        protocol: TCP
-    kubernetesResources:
-      ingressResources:
-      - name: discourse-ingress
-        annotations:
-          nginx.ingress.kubernetes.io/ssl-redirect: 'false'
-        spec:
-          rules:
-          - host: discourse.example.com
-            http:
-              paths:
-              - backend:
-                  serviceName: discourse
-                  servicePort: 3000
-                path: /
-    version: 3
-
+pod_config:
+    DISCOURSE_CORS_ORIGIN: '*'
+    DISCOURSE_DEVELOPER_EMAILS: [e-mail]
+    DISCOURSE_ENABLE_CORS: true
+    DISCOURSE_HOSTNAME: discourse.example.com
+    DISCOURSE_POSTGRES_HOST: 10.9.89.237
+    DISCOURSE_POSTGRES_NAME: discourse
+    DISCOURSE_POSTGRES_PASSWORD: a_real_password
+    DISCOURSE_POSTGRES_USERNAME: discourse_m
+    DISCOURSE_REDIS_HOST: 10.25.242.12
+    DISCOURSE_SMTP_ADDRESS: smtp.internal
+    DISCOURSE_SMTP_AUTHENTICATION: login
+    DISCOURSE_SMTP_DOMAIN: example.com
+    DISCOURSE_SMTP_OPENSSL_VERIFY_MODE: none
+    DISCOURSE_SMTP_PASSWORD: null
+    DISCOURSE_SMTP_PORT: 587
+    DISCOURSE_SMTP_USER_NAME: apikey
diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
index 4fd7324..214bfa1 100644
--- a/tests/unit/test_charm.py
+++ b/tests/unit/test_charm.py
@@ -3,40 +3,41 @@
 # Copyright 2020 Canonical Ltd.
 # See LICENSE file for licensing details.
 
-import os
+import copy
 import glob
+import mock
+import os
 import unittest
 import yaml
-import mock
-from pprint import pprint
 
-from charm import DiscourseCharm, BlockedStatus
+from types import SimpleNamespace
+
+from charm import (
+    check_for_missing_config_fields,
+    create_discourse_pod_config,
+    get_pod_spec,
+    DiscourseCharm,
+)
 
 from ops import testing
 
-dirname = os.path.dirname(__file__)
-
-
-# Valid and Invalid configs are present in the fixtures
-# directory. The files contain the juju config, along with
-# the spec that that config should produce in the case of
-# valid configs. In the case of invalid configs, they contain
-# the juju config and the error that should be triggered by
-# the config. These scenarios are tested below. Additional
-# config variations can be created by creating an appropriately
-# named config file in the fixtures directory. Valid configs
-# should be named: config_valid_###.yaml and invalid configs
-# should be named: config_invalid_###.yaml.
-#
-# This function loads the configs for use by the tests.
+
 def load_configs(directory):
+    """Load configs for use by tests.
+
+    Valid and invalid configs are present in the fixtures directory. The files
+    contain the juju config, along with the spec that that config should
+    produce in the case of valid configs. In the case of invalid configs, they
+    contain the juju config and the error that should be triggered by the
+    config. These scenarios are tested below. Additional config variations can
+    be created by creating an appropriately named config file in the fixtures
+    directory. Valid configs should be named: config_valid_###.yaml and invalid
+    configs should be named: config_invalid_###.yaml."""
     configs = {}
     for filename in glob.glob(os.path.join(directory, 'config*.yaml')):
-        pprint(filename)
         with open(filename) as file:
-            name = os.path.splitext(os.path.basename(filename))
-            pprint(name[0])
-            configs[name[0]] = yaml.full_load(file)
+            name, _ = os.path.splitext(os.path.basename(filename))
+            configs[name] = yaml.full_load(file)
     return configs
 
 
@@ -46,81 +47,70 @@ class TestDiscourseK8sCharmHooksDisabled(unittest.TestCase):
         self.harness.disable_hooks()
         self.harness.set_leader(True)
         self.harness.begin()
-        self.configs = load_configs(os.path.join(dirname, 'fixtures'))
+        self.configs = load_configs(os.path.join(os.path.dirname(__file__), 'fixtures'))
 
     def test_valid_configs_are_ok(self):
         """Test that a valid config is considered valid."""
         for config_key in self.configs:
             if config_key.startswith('config_valid_'):
-                self.harness.update_config(self.configs[config_key]['config'])
-                valid_config = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
-                self.assertEqual(valid_config, True, 'Valid config {} is not recognized as valid.'.format(config_key))
-
-    def test_charm_identifies_bad_configs(self):
-        """Test that bad configs are identified by the charm."""
-        for config_key in self.configs:
-            if config_key.startswith('config_invalid_'):
-                self.harness.update_config(self.configs[config_key]['config'])
-                valid_config = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
-                self.assertEqual(valid_config, False, 'Bad Config {} is recognized.'.format(config_key))
+                config_valid = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
+                pod_config = create_discourse_pod_config(self.configs[config_key]['config'])
+                self.assertEqual(config_valid, True, 'Valid config is not recognized as valid')
                 self.assertEqual(
-                    self.harness.charm.model.unit.status,
-                    BlockedStatus(self.configs[config_key]['expected_error_status']),
-                    'Invalid config {} does not produce correct status'.format(config_key),
+                    pod_config,
+                    self.configs[config_key]['pod_config'],
+                    'Valid config {} is does not produce expected config options for pod.'.format(config_key),
                 )
 
-    def test_charm_creates_valid_ingress_config(self):
-        """Test that a valid config creates a valid ingress spec."""
-        for config_key in self.configs:
-            if config_key.startswith('config_valid_'):
-                self.harness.update_config(self.configs[config_key]['config'])
-                spec = self.harness.charm.get_pod_spec(self.configs[config_key]['config'])
-                self.assertEqual(
-                    spec['kubernetesResources']['ingressResources'],
-                    self.configs[config_key]['spec']['kubernetesResources']['ingressResources'],
-                    'Valid config {} does not produce expected ingress config.'.format(config_key),
-                )
-
-    def test_valid_pod_spec(self):
-        """A valid config results in a valid pod spec."""
+    def test_invalid_configs_are_recognized(self):
+        """Test that bad configs are identified by the charm."""
         for config_key in self.configs:
-            if config_key.startswith('config_valid_'):
-                self.harness.update_config(self.configs[config_key]['config'])
-                spec = self.harness.charm.get_pod_spec(self.configs[config_key]['config'])
+            if config_key.startswith('config_invalid_'):
+                config_valid = self.harness.charm.check_config_is_valid(self.configs[config_key]['config'])
+                missing_fields = check_for_missing_config_fields(self.configs[config_key]['config'])
+                self.assertEqual(config_valid, False, 'Invalid config is not recognized as invalid')
                 self.assertEqual(
-                    spec,
-                    self.configs[config_key]['spec'],
-                    'Valid config {} does not produce expected pod spec.'.format(config_key),
+                    missing_fields,
+                    self.configs[config_key]['missing_fields'],
+                    'Invalid config {} does not fail as expected.'.format(config_key),
                 )
 
     def test_charm_config_process(self):
-        action_event = mock.Mock()
-        self.harness.update_config(self.configs['config_valid_1']['config'])
-        self.harness.charm.configure_pod(action_event)
-        (configured_spec, k8s_resources) = self.harness.get_pod_spec()
-        self.assertEqual(
-            configured_spec,
-            self.configs['config_valid_1']['spec'],
-            'Valid config does not cause charm to set expected pod spec.',
-        )
-
-    def test_charm_config_process_invalid_config(self):
-        action_event = mock.Mock()
-        self.harness.update_config(self.configs['config_invalid_1']['config'])
-        self.harness.charm.configure_pod(action_event)
-        result = self.harness.get_pod_spec()
-        pprint(result)
-        self.assertEqual(result, None, 'Invalid config does not get set as pod spec.')
+        """Test that the entire config process for the charm works."""
+        test_config = copy.deepcopy(self.configs['config_valid_complete']['config'])
+        test_config['db_user'] = 'discourse_m'
+        test_config['db_password'] = 'a_real_password'
+        test_config['db_host'] = '10.9.89.237'
+        db_event = SimpleNamespace()
+        db_event.master = SimpleNamespace()
+        db_event.master.user = 'discourse_m'
+        db_event.master.password = 'a_real_password'
+        db_event.master.host = '10.9.89.237'
+        expected_spec = (get_pod_spec(self.harness.charm.framework.model.app.name, test_config), None)
+        self.harness.update_config(self.configs['config_valid_complete']['config'])
+        self.harness.charm.on_database_relation_joined(db_event)
+        self.harness.charm.on_database_changed(db_event)
+        configured_spec = self.harness.get_pod_spec()
+        self.assertEqual(configured_spec, expected_spec, 'Configured spec does not match expected pod spec.')
 
     def test_charm_config_process_not_leader(self):
+        """Test that the config process on a non-leader does not trigger a pod_spec change."""
         non_leader_harness = testing.Harness(DiscourseCharm)
         non_leader_harness.disable_hooks()
         non_leader_harness.set_leader(False)
         non_leader_harness.begin()
 
         action_event = mock.Mock()
-        non_leader_harness.update_config(self.configs['config_valid_1']['config'])
+        non_leader_harness.update_config(self.configs['config_valid_complete']['config'])
         non_leader_harness.charm.configure_pod(action_event)
         result = non_leader_harness.get_pod_spec()
-        pprint(result)
         self.assertEqual(result, None, 'Non-leader does not set pod spec.')
+
+    def test_lost_db_relation(self):
+        """Test that losing our DB relation triggers a drop of pod config."""
+        self.harness.update_config(self.configs['config_valid_complete']['config'])
+        db_event = SimpleNamespace()
+        db_event.master = None
+        self.harness.charm.on_database_changed(db_event)
+        configured_spec = self.harness.get_pod_spec()
+        self.assertEqual(configured_spec, None, 'Lost DB relation does not result in empty spec as expected')