wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

Some comments inline.

I do think some kind of timeout for retrying wait_for_wordpress makes sense, even if it's something like an hour or two. Will ask for input from some others on this.

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> index 24b6b0b..c866c98 100644
> --- a/Dockerfile
> +++ b/Dockerfile
> @@ -19,7 +19,8 @@ RUN echo 'debconf debconf/frontend select Noninteractive' | debconf-set-selectio
>  # Update all packages, remove cruft, install required packages, configure apache
>  RUN apt-get update && apt-get -y dist-upgrade \
>      && apt-get --purge autoremove -y \
> -    && apt-get install -y apache2 php libapache2-mod-php php-mysql php-gd curl ssl-cert pwgen \
> +    && apt-get install -y apache2 php libapache2-mod-php php-mysql php-gd \
> +       curl ssl-cert pwgen python3 python3-yaml php-symfony-yaml \

Let's group the php ones together. Alpha sorting all of these might be good while we're at it.

>      && sed -ri 's/^export ([^=]+)=(.*)$/: ${\1:=\2}\nexport \1/' "$APACHE_ENVVARS" \
>      && . "$APACHE_ENVVARS" \
>      && for dir in "$APACHE_LOCK_DIR" "$APACHE_RUN_DIR" "$APACHE_LOG_DIR"; do rm -rvf "$dir"; mkdir -p "$dir"; chown "$APACHE_RUN_USER:$APACHE_RUN_GROUP" "$dir"; chmod 777 "$dir";  done \
> diff --git a/tests/unit/test_plugin_hander.py b/tests/unit/test_plugin_hander.py
> new file mode 100644
> index 0000000..d72b865
> --- /dev/null
> +++ b/tests/unit/test_plugin_hander.py
> @@ -0,0 +1,19 @@
> +import os
> +import sys
> +import unittest
> +from unittest import mock
> +
> +sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))

Could we not avoid this by putting it in a particular directory (e.g. ./files/plugin_hander/) and then including that in our PYTHONPATH in tox.ini?

> +from files import plugin_handler  # NOQA: E402
> +
> +
> +class testWrapper(unittest.TestCase):
> +    def setUp(self):
> +        self.maxDiff = None
> +
> +    @mock.patch("files.plugin_handler.logging")
> +    def test_team_mapper(self, foo):
> +        given = "canonical-sysadmins=administrator,canonical-website-editors=editor,canonical-website-admins=administrator,launchpad=editor"  # NOQA: E501

Would be more readable if you split this onto multiple lines, or did ",".join(["canonical-sysadmins=administrator", "canonical-website-editors=editor", "canonical-website-admins=administrator", "launchpad=editor"] which could be very easy to line split, visually.

> +        want = """a:4:{i:1;O:8:"stdClass":4:{s:2:"id";i:1;s:4:"team";s:19:"canonical-sysadmins";s:4:"role";s:13:"administrator";s:6:"server";s:1:"0";}i:2;O:8:"stdClass":4:{s:2:"id";i:2;s:4:"team";s:25:"canonical-website-editors";s:4:"role";s:6:"editor";s:6:"server";s:1:"0";}i:3;O:8:"stdClass":4:{s:2:"id";i:3;s:4:"team";s:24:"canonical-website-admins";s:4:"role";s:13:"administrator";s:6:"server";s:1:"0";}i:4;O:8:"stdClass":4:{s:2:"id";i:4;s:4:"team";s:9:"launchpad";s:4:"role";s:6:"editor";s:6:"server";s:1:"0";}}"""  # NOQA: E501

This could also be made a lot more readable by displaying in some other way (and then collapsing into this string when needed for comparison.

> +        got = plugin_handler.encode_team_map(given)
> +        self.assertTrue(got == want)

self.assertEqual would be more straightforward here.



-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/380153
Your team Wordpress Charmers is requested to review the proposed merge of ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.