wordpress-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Needs Information

Code looks fine.

Thanks for keeping what you can Python. I can't really review the PHP chunks, which seem to be the least worst solution.

I agree with other commentators that it may be best for wait_for_wordpress to timeout, as I expect it is better for the pod spin up to fail (and be noticed by k8s) than for it to hang.

Some other inline comments, opinions only.

Diff comments:

> diff --git a/.gitignore b/.gitignore
> index e0f7f96..d8aaacd 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,3 +1,6 @@
>  files/plugins/
>  files/themes/
> +.coverage
>  .tox/
> +files/__pycache__/
> +tests/unit/__pycache__/

I think you can just gitignore '__pycache__' and have it apply to all levels (and files named __pycache__, which we don't need to care about).

> diff --git a/Makefile b/Makefile
> index 36c7f00..a3cddc9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -24,12 +24,18 @@ lint: clean
>  	@echo "Running flake8"
>  	@tox -e lint
>  
> +test: lint
> +	@echo "Running unit tests"
> +	@tox -e unit
> +
>  clean:
>  	@echo "Cleaning files"
>  	@rm -rf ./.tox
>  	@rm -rf ./.pytest_cache
>  	@rm -rf ./files/plugins/*
>  	@rm -rf ./files/themes/*
> +	@rm -rf ./files/__pycache__
> +	@rm -rf ./tests/unit/__pycache__
>  	@mkdir -p ./files/plugins
>  	@mkdir -p ./files/themes

I use 'git clean -fXd' to remove everything gitignored and untracked, although many would argue your explicit approach is better than the magic one.

>  
> diff --git a/files/docker-entrypoint.sh b/files/docker-entrypoint.sh
> index 5f67d9d..136a0d5 100644
> --- a/files/docker-entrypoint.sh
> +++ b/files/docker-entrypoint.sh
> @@ -11,4 +11,6 @@ do
>      sed -i -e "s/%%%${key}%%%/$(pwgen 64 1)/" /var/www/html/wp-info.php
>  done
>  
> +nohup bash -c "/srv/wordpress-helpers/plugin_handler.py &"

This seems cleaner than installing cron or systemd into the container.

> +
>  exec "$@"
> diff --git a/files/plugin_handler.py b/files/plugin_handler.py
> new file mode 100644
> index 0000000..e3c8f93
> --- /dev/null
> +++ b/files/plugin_handler.py
> @@ -0,0 +1,117 @@
> +#!/usr/bin/python3

If we were upstream, this would likely be a php script. But we aren't, so it isn't. And I can review it.

> +
> +import logging
> +import os
> +import subprocess
> +import urllib.request
> +from time import sleep
> +from yaml import safe_load
> +
> +helpers_path = '/srv/wordpress-helpers'
> +install_path = '/var/www/html'
> +
> +
> +def call_php_helper(helper, stdin='', *args):
> +    path = os.path.join(helpers_path, helper)
> +    cmd = ['php', path]
> +    cmd.extend([str(arg) for arg in args])
> +    logging.info(cmd)
> +    process = subprocess.Popen(
> +        cmd,
> +        cwd=install_path,
> +        stdin=subprocess.PIPE,
> +        stdout=subprocess.PIPE,
> +        stderr=subprocess.STDOUT,
> +    )
> +    return process.communicate(stdin)[0]  # spit back stdout+stderr combined

Use of subprocess.run has been considered, but I find subprocess.Popen fine to.

> +
> +
> +def enable_plugin(*plugins):
> +    logging.info("Enabling plugins: {}".format(plugins))
> +    logging.info(call_php_helper('_enable_plugin.php', '', *plugins))
> +
> +
> +def get_option(key):
> +    value = call_php_helper('_get_option.php', '', key)
> +    return safe_load(value)
> +
> +
> +def add_option(key, value):
> +    # Ensure we don't overwrite settings
> +    if not get_option(key):
> +        logging.info('Adding option: {}'.format(key))
> +        call_php_helper('_add_option.php', '', key, value)
> +    else:
> +        logging.info('Option "{}" already in place, skipping.'.format(key))
> +
> +
> +def encode_team_map(team_map):
> +    # example: site-sysadmins=administrator,site-editors=editor,site-executives=editor
> +    team_map_lines = []
> +    i = 0
> +    team_map_lines.append("a:{}:{{".format(len(team_map.split(','))))
> +    for mapping in team_map.split(','):
> +        i = i + 1
> +        team, role = mapping.split('=', 2)
> +        team_map_lines.append('i:{};'.format(i))
> +        team_map_lines.append('O:8:"stdClass":4:{')
> +        team_map_lines.append('s:2:"id";')
> +        team_map_lines.append('i:{};'.format(i))
> +        team_map_lines.append('s:4:"team";')
> +        team_map_lines.append('s:{}:"{}";'.format(len(team), team))
> +        team_map_lines.append('s:4:"role";')
> +        team_map_lines.append('s:{}:"{}";'.format(len(role), role))
> +        team_map_lines.append('s:6:"server";')
> +        team_map_lines.append('s:1:"0";')
> +        team_map_lines.append('}')
> +    team_map_lines.append('}')
> +
> +    return ''.join(team_map_lines)
> +
> +
> +def enable_akismet(key):
> +    enable_plugin('akismet/akismet.php')
> +    add_option('akismet_strictness', '0')
> +    add_option('akismet_show_user_comments_approved', '0')
> +    add_option('wordpress_api_key', key)
> +
> +
> +def enable_openid(team_map):
> +    encoded_team_map = encode_team_map(team_map)
> +    enable_plugin('openid/openid.php')
> +    add_option('openid_required_for_registration', '1')
> +    add_option('openid_teams_trust_list', encoded_team_map)
> +
> +
> +def wait_for_wordpress():
> +    url = 'http://localhost'
> +    sleep_time = 10
> +    success = False
> +    while success is not True:
> +        try:
> +            response = urllib.request.urlopen(url, timeout=sleep_time)
> +        except Exception:
> +            logging.info('Waiting for Wordpress to accept connections')
> +            sleep(sleep_time)
> +        else:
> +            if response.status == 200:
> +                success = True
> +            else:
> +                logging.info('Waiting for Wordpress to return HTTP 200 (got {})'.format(response.status))
> +                sleep(sleep_time)
> +    return True

Can we have this timeout with an exception, causing the pod to go into an error state we will notice? And maybe have k8s retry per its configuration? This seems preferable to other approaches to notice a hung deployment.

> +
> +
> +if __name__ == '__main__':
> +    logger = logging.getLogger(__name__)
> +    logging.basicConfig(filename='/var/log/wordpress-plugin-handler.log', level=logging.DEBUG)
> +
> +    wait_for_wordpress()
> +
> +    key = os.getenv('WP_PLUGIN_AKISMET_KEY')
> +    if key:
> +        enable_akismet(key)
> +
> +    team_map = os.getenv('WP_PLUGIN_OPENID_TEAM_MAP')

The format of WP_PLUGIN_OPENID_TEAM_MAP only seems to be documented in a comment in the encode_team_map helper (not even a docstring). Assuming the source is charm config in config.yaml, can you confirm the format is documented in the option's description?

> +    if team_map:
> +        enable_openid(team_map)
> diff --git a/tests/unit/test_plugin_hander.py b/tests/unit/test_plugin_hander.py
> new file mode 100644
> index 0000000..82c0fec
> --- /dev/null
> +++ b/tests/unit/test_plugin_hander.py
> @@ -0,0 +1,32 @@
> +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__)))))

I'd normally spell this os.path.join('..','..', os.path.dirname(__file__)), or os.path.abspath(os.path.join(...)) if you need it absolute.

> +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 = ",".join([
> +            "canonical-sysadmins=administrator",
> +            "canonical-website-editors=editor",
> +            "canonical-website-admins=administrator,launchpad=editor"
> +        ])
> +        want = "".join([
> +            """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";}}"""
> +        ])
> +        got = plugin_handler.encode_team_map(given)
> +        self.assertEqual(got, want)


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