wordpress-charmers team mailing list archive

[Thomas Cuthbert <thomas.cuthbert@xxxxxxxxxxxxx>]

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'd agree the name of the function is a bit terse but I disagree that it's ever a bad thing to encapsulate code that gets run more than once. It improves code readability by accurately describing what we are doing with the Juju setting, it's simple enough that it isn't prematurely optimised, and means if we ever wanted to do more with parsing Juju setting strings, we can.

> +
> +
>  class WordpressInitialiseEvent(EventBase):
>      """Custom event for signalling Wordpress initialisation.
>  
> 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.

Happy to do so but food for thought: Each unit test targets a particular method of the charm. The validation for additional hostnames occurs within the `is_config_valid` method. Having one of the validators separate from the others may lead to confusion. Like my previous inline comment mentioned before, if our validation logic ends up becoming more complex and requires splitting up then I'd agree we should split each separate validator into its own 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):


-- 
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.