wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Added some comments inline.

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> index f1a2252..5d9f4e9 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -85,6 +86,10 @@ def gather_wordpress_secrets():
>      return rv
>  
>  
> +def split_additional_hostnames(hostnames):
> +    return hostnames.split(" ")

I'm not sure about this one. We're adding a new function because we're using it in two places, but it doesn't save us any lines of code from just `split_hostnames = config["additional_hostnames"].split(" ")`. If we do think there's a good case for keeping this function we'll want a docstring and test for it.

> +
> +
>  class WordpressInitialiseEvent(EventBase):
>      """Custom event for signalling Wordpress initialisation.
>  
> @@ -274,6 +279,20 @@ class WordpressCharm(CharmBase):
>              },
>          }
>  
> +        if self.model.config["additional_hostnames"]:
> +            additional_hostnames = split_additional_hostnames(self.model.config["additional_hostnames"])

Lower down you call this variable `split_hostnames`, I think it'd be good to be consistent here.

> +            rules = resources["kubernetesResources"]["ingressResources"][0]["spec"]["rules"]
> +            for hostname in additional_hostnames:
> +                rule = {
> +                    "host": hostname,
> +                    "http": {
> +                        "paths": [
> +                            {"path": "/", "backend": {"serviceName": self.app.name, "servicePort": 80}}
> +                        ]
> +                    },
> +                }
> +                rules.append(rule)
> +
>          ingress = resources["kubernetesResources"]["ingressResources"][0]
>          if self.model.config["tls_secret_name"]:
>              ingress["spec"]["tls"] = [
> @@ -332,7 +351,7 @@ class WordpressCharm(CharmBase):
>  
>          return spec
>  
> -    def is_valid_config(self):
> +    def is_valid_config(self):  # If this grows anymore consider breaking up into smaller functions.

I'm not sure we should have a comment like this. It's not going to be clear from looking at the code when this was added, and I think it's just something we should catch in review as a matter of course.

>          is_valid = True
>          config = self.model.config
>  
> diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
> index acf406c..6a9c1c2 100644
> --- a/tests/unit/test_charm.py
> +++ b/tests/unit/test_charm.py
> @@ -65,6 +65,15 @@ class TestWordpressCharm(unittest.TestCase):
>          self.assertEqual(self.harness.charm.unit.status.message, expected_msg)
>          self.assertLogs(expected_msg, level="INFO")
>  
> +        # Test for invalid additional hostnames.

I think it'd be good to split this into a separate test.

> +        invalid_additional_hostnames = "forgot-my-tld invalid+character.com"
> +        expected_msg = "Invalid additional hostnames supplied: {}".format(invalid_additional_hostnames)
> +        self.harness.update_config({"additional_hostnames": invalid_additional_hostnames})
> +        self.harness.charm.is_valid_config()
> +        self.assertIsInstance(self.harness.charm.unit.status, BlockedStatus)
> +        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):
> diff --git a/tests/unit/test_wordpress.py b/tests/unit/test_wordpress.py
> index 92712e7..54982b1 100644
> --- a/tests/unit/test_wordpress.py
> +++ b/tests/unit/test_wordpress.py
> @@ -17,6 +17,7 @@ TEST_MODEL_CONFIG = {
>      "db_name": "wordpress",
>      "db_user": "admin",
>      "db_password": "letmein123",
> +    "additional_hostnames": "cool-newsite.org blog.test.com",

I think we should be testing with additional_hostnames specified, and with it set to an empty value as well.

>      "wp_plugin_openid_team_map": True,
>      "wp_plugin_akismet_key": "somerandomstring",
>      "container_config": "test-key: test",


-- 
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress-1/+merge/395661
Your team Wordpress Charmers is requested to review the proposed merge of ~tcuthbert/charm-k8s-wordpress:additional_hostnames into charm-k8s-wordpress:master.