Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-05-11 Thread Canonical IS Mergebot 
Change successfully merged at revision baf77967b42ecd7a846463cb7405c12cc860a18d
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/383235
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-05-11 Thread Tom Haddon 
Review: Approve

LGTM, although I don't think the comment is so useful

Diff comments:

> diff --git a/files/plugin_handler.py b/files/plugin_handler.py
> index 4feb8f2..eb9172a 100644
> --- a/files/plugin_handler.py
> +++ b/files/plugin_handler.py
> @@ -128,3 +134,20 @@ if __name__ == "__main__":
>  team_map = os.getenv("WP_PLUGIN_OPENID_TEAM_MAP")
>  if team_map:
>  enable_openid(team_map)
> +
> +# swift bits

I'm not sure this comment really adds much. Let's just remove it or change it 
for something a bit more useful.

> +swift_url = os.getenv("SWIFT_URL")
> +if swift_url:
> +swift_config = {}
> +swift_config['url'] = swift_url
> +swift_config['auth_url' = os.getenv("SWIFT_AUTH_URL")
> +swift_config['bucket'] = os.getenv("SWIFT_BUCKET")
> +swift_config['password'] = os.getenv("SWIFT_PASSWORD")
> +swift_config['prefix'] = os.getenv("SWIFT_PREFIX")
> +swift_config['region'] = os.getenv("SWIFT_REGION")
> +swift_config['tenant'] = os.getenv("SWIFT_TENANT")
> +swift_config['username'] = os.getenv("SWIFT_USERNAME")
> +swift_config['copy_to_swift'] = os.getenv("SWIFT_COPY_TO_SWIFT")
> +swift_config['serve_from_swift'] = 
> os.getenv("SWIFT_SERVE_FROM_SWIFT")
> +swift_config['remove_local_file'] = 
> os.getenv("SWIFT_REMOVE_LOCAL_FILE")
> +enable_swift(swift_config)


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/383235
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-05-11 Thread Canonical IS Mergebot 
This merge proposal is being monitored by mergebot. Change the status to 
Approved to merge.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/383235
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-04-30 Thread Barry Price 
For context:

https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/383120
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/383235
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-23 Thread Canonical IS Mergebot 
Change successfully merged at revision e1aedf5b4848c748e6731e5de3c42bdbc09fc2e7
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/380153
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-23 Thread Barry Price 
Addressed all comments, pushing fixed code now.

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__/

Fixed, thanks

> diff --git a/files/plugin_handler.py b/files/plugin_handler.py
> new file mode 100644
> index 000..e3c8f93
> --- /dev/null
> +++ b/files/plugin_handler.py
> @@ -0,0 +1,117 @@
> +#!/usr/bin/python3
> +
> +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
> +
> +
> +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

I've implemented this by touching a file on sucess to satisfy a readinessProbe 
- otherwise we time out after 10 mins.

> +
> +
> +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')

It is:

https://git.launchpad.net/~wordpress-charmers/charm-k8s-wordpress/+git/charm-k8s-wordpress/tree/config.yaml

> +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 000..82c0fec
> --- /dev/null
> +++ b/tests/unit/test_plugin_hander.py
> @@ -0,0 +1,32 @@
> +import os
> +import sys
> +import unittest
> +from unittest import mock
> +
>

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-10 Thread Tom Haddon 
Review: Approve

One minor comment, other than that looks good, thanks.

Diff comments:

> diff --git a/tests/unit/test_plugin_hander.py 
> b/tests/unit/test_plugin_hander.py
> new file mode 100644
> index 000..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__)
> +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"

This should be two lines, for consistency.

> +])
> +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 subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-10 Thread Stuart Bishop 
Review: Approve

Discussions indicate it doesn't matter if the script times out or not, as a 
failure will not escalate the pod to a failed state or inform Juju. Something 
else needs to happen to catch the case where wordpress comes up but the 
configuration script fails.
-- 
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-09 Thread Stuart Bishop 
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 000..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 =

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-08 Thread Barry Price 
Addressed most of these - pushing fixes

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 \

Thanks, done.

>  && 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 000..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__)

We could, but "sys.path.append(...) #NoQA: E402" is how operator itself does 
it, so I stuck with that for consistency. No strong feelings either way though.

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

Agreed, done.

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

Yup, done

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

Also done. Thanks!



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

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-06 Thread Tom Haddon 
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 000..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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-06 Thread Canonical IS Mergebot 
This merge proposal is being monitored by mergebot. Change the status to 
Approved to merge.
-- 
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-03-04 Thread Tom Haddon 
Some inline comments.

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> index 24b6b0b..7abcf33 100644
> --- a/Dockerfile
> +++ b/Dockerfile
> @@ -50,10 +50,22 @@ COPY --chown=www-data:www-data ./files/themes/ 
> /var/www/html/wp-content/themes/
>  COPY ./files/wp-info.php /var/www/html/
>  COPY ./files/wp-config.php /var/www/html/
>  
> +# Copy our helper scripts into their own directory
> +COPY ./files/_add_option.php /srv/wordpress-helpers/
> +COPY ./files/_enable_option.php /srv/wordpress-helpers/
> +COPY ./files/_get_option.php /srv/wordpress-helpers/

If you had all these in the same directory you could just have one COPY line 
(same above for the ones you're putting in /var/www/html). Is there a reason 
not to do that, as it seems cleaner?

> +
> +# And their wrapper:
> +COPY ./files/plugin_handler.py /srv/wordpress-helpers/
> +RUN chmod 0755 /srv/wordpress-helpers/plugin_handler.py
> +
> +# And its service definition
> +COPY ./files/wordpress-plugin-handler.service /usr/lib/systemd/system/
> +
>  # entrypoint script will configure Wordpress based on env variables
>  COPY ./files/docker-entrypoint.sh /usr/local/bin/
> -
>  RUN chmod 0755 /usr/local/bin/docker-entrypoint.sh
> +
>  # Port 80 only, TLS will terminate elsewhere
>  EXPOSE 80
>  
> diff --git a/files/plugin_handler.py b/files/plugin_handler.py
> new file mode 100644
> index 000..1b23891
> --- /dev/null
> +++ b/files/plugin_handler.py
> @@ -0,0 +1,123 @@
> +#!/usr/bin/python3
> +
> +# get the env vars, if neither are set then nothing to do
> +# if either/both are set:
> +# # wait for localhost:80
> +# # set the first one
> +# # set the second one
> +# # delete ourselves and go away forever
> +
> +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'
> +
> +logging.basicConfig(filename='/var/log/wordpress-plugin-handler.log', 
> level=logging.DEBUG)
> +
> +
> +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
> +
> +
> +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

This really needs a unit test I think, if nothing else as a way of documenting 
the format we're expecting to see.

> +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('0:8:"stdClass":4:{')
> +team_map_lines.append('s:2:"id";')
> +team_map_lines.append('i:1;')
> +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:

I think it'd be worth having an upper limit on this (could be an arg to the 
function).

> +try:
> +response =

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-29 Thread Canonical IS Mergebot 
Change successfully merged at revision d926292f1df9ee1c686a69370a4c1bcede7421b6
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-29 Thread Tom Haddon 
Review: Approve

Based on discussions on IRC I'm approving this. I'd missed the fact we were 
still doing an autoremove, and it seems there's no functional difference to 
adding --no-install-recommends, so that must be being set in the docker 
images's apt preferences already.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-29 Thread Tom Haddon 
Two comments inline

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> index d9d9d67..ae922d5 100644
> --- a/Dockerfile
> +++ b/Dockerfile
> @@ -8,21 +8,16 @@ ARG BUILD_DATE
>  
>  LABEL org.label-schema.build-date=$BUILD_DATE
>  
> +ENV APACHE_CONFDIR=/etc/apache2
> +ENV APACHE_ENVVARS=/etc/apache2/envvars
> +
>  # Avoid interactive prompts
>  RUN echo 'debconf debconf/frontend select Noninteractive' | 
> debconf-set-selections
>  
> -# Update all packages, remove cruft
> +# Update all packages, remove cruft, install required packages, configure 
> apache

The "remove cruft" comment now doesn't quite make sense. I think it's worth 
updating it to say that docker does clean up for us.

>  RUN apt-get update && apt-get -y dist-upgrade \
>  && apt-get --purge autoremove -y \
> -&& apt-get clean \
> -&& rm -rf /var/lib/apt/lists/*
> -
> -# install and configure apache2 (--no-install-recommends as we don't need 
> ssl-cert)
> -ENV APACHE_CONFDIR=/etc/apache2
> -ENV APACHE_ENVVARS=/etc/apache2/envvars
> -RUN apt-get update \
> -&& apt-get install -y --no-install-recommends apache2 \
> -&& rm -rf /var/lib/apt/lists/* \
> +&& apt-get install -y apache2 php libapache2-mod-php php-mysql php-gd 
> curl ssl-cert pwgen \

Do we know for sure that we can't use --no-install-recommends?

>  && 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 \


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-24 Thread Barry Price 
Deploy confirmed working too. Ready for re-review, I think.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-24 Thread Barry Price 
Build is still working, at least:

https://jenkins.canonical.com/is/job/wordpress-k8s-image-builder/24/console
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-23 Thread Benjamin Allot 
A general comment on the dockerfile.

All metadata related steps, LABEL, ENTRYPOINT, CMD, need to be carefully placed 
in the Dockerfile to maximize cache usage.

I would move ENTRYPOINT and CMD right under the ARG part, even if it's minor.

As for the RUN however, the rule of thumb is to avoid their number as much as 
possible.
Doing one big RUN with a seccession of "&&" or say, 2, one for the install of 
package, one for the configuration items would be preferred IMO.

Also, I would the curl of the latest wordpress tarball at the top of the chain, 
so if we have a conenctivity issue and this fails, we fail early.

Last, but not least, ubuntu docker image already have a cleaning automated 
after all apt commands

$ docker run --rm -ti ubuntu:bionic cat /etc/apt/apt.conf.d/docker-clean
DPkg::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb 
/var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };
APT::Update::Post-Invoke { "rm -f /var/cache/apt/archives/*.deb 
/var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true"; };
Dir::Cache::pkgcache ""; Dir::Cache::srcpkgcache "";
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-23 Thread Canonical IS Mergebot 
This merge proposal is being monitored by mergebot. Change the status to 
Approved to merge.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-23 Thread Barry Price 
Confirmed working with a local (juju/microk8s) deploy.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-23 Thread Barry Price 
And another, post-fix:

https://jenkins.canonical.com/is/job/wordpress-k8s-image-builder/22/console
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-23 Thread Barry Price 
Passing run:

https://jenkins.canonical.com/is/job/wordpress-k8s-image-builder/21/console

Still need to confirm the image actually runs as expected...
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377920
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-21 Thread Canonical IS Mergebot 
Change successfully merged at revision 625248ee904c67b6fcaa2640476d54d7088c5bc9
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377851
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-21 Thread Tom Haddon 
Review: Approve

Approved with one comment about adding a comment

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> index a6bed91..f667d10 100644
> --- a/Dockerfile
> +++ b/Dockerfile
> @@ -1,7 +1,11 @@
>  FROM ubuntu:bionic
>  
> -ARG http_proxy
> -ARG https_proxy
> +LABEL maintainer="wordpress-charmers@lists.launchpad.net"
> +
> +ARG HTTPS_PROXY

Might be worth a comment as to what this is used for as it's not referenced 
directly anywhere else in the Dockerfile

> +ARG BUILD_DATE
> +
> +LABEL org.label-schema.build-date=$BUILD_DATE
>  
>  # Avoid interactive prompts
>  RUN echo 'debconf debconf/frontend select Noninteractive' | 
> debconf-set-selections


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377851
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-20 Thread Barry Price 
Successful Jenkins run of my branch:

https://jenkins.canonical.com/is/job/wordpress-k8s-image-builder/19/console
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377851
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Canonical IS Mergebot 
Change successfully merged at revision 27fa70ac52af813fc0bd35a68a8ea2d2e49ff9bf
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377649
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Tom Haddon 
Review: Approve

LGTM, thx
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377649
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Tom Haddon 
One comment inline

Diff comments:

> diff --git a/fetcher.py b/fetcher.py
> index 9f93073..eb6c0b5 100755
> --- a/fetcher.py
> +++ b/fetcher.py
> @@ -110,7 +110,7 @@ def get_themes(branch_themes):
>  current_branch = 0
>  for branch_theme in branch_themes:
>  current_branch = current_branch + 1
> -print('Downloading branched theme {} of 
> {}...'.format(current_branch, total_branches))
> +print('Downloading branched theme {} of {}: {} 
> ...'.format(current_branch, total_branches, branch_theme))

The format I was using was:

print('Downloading {} of {} branched plugins: {} ...'.format(current_branch, 
total_branches, branch_plugin)) 

Would be good to be consistent I think

>  url = branch_themes[branch_theme].get('url')
>  basename = os.path.basename(url)
>  if basename.startswith('lp:'):


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377649
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Canonical IS Mergebot 
This merge proposal is being monitored by mergebot. Change the status to 
Approved to merge.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377649
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Canonical IS Mergebot 
Change successfully merged at revision de3ad0c4a3b51b6750c1d06f28bd7f2fbc3899f8
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377499
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-15 Thread Tom Haddon 
Review: Approve

LGTM, thx
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377499
Your team Wordpress Charmers is subscribed to branch 
~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-13 Thread Barry Price 
Thanks, addressed 2/3, will push a fresh branch and we can discuss the other 
later today.

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> new file mode 100644
> index 000..38866f4
> --- /dev/null
> +++ b/Dockerfile
> @@ -0,0 +1,55 @@
> +FROM ubuntu:bionic
> +
> +RUN echo 'debconf debconf/frontend select Noninteractive' | 
> debconf-set-selections
> +
> +RUN apt-get update && apt-get -y dist-upgrade \
> +&& apt-get --purge autoremove -y \
> +&& apt-get clean \
> +&& rm -rf /var/lib/apt/lists/*
> +
> +ENV APACHE_CONFDIR=/etc/apache2
> +ENV APACHE_ENVVARS=/etc/apache2/envvars
> +
> +RUN apt-get update \
> +&& apt-get install -y --no-install-recommends apache2 \

This particular case just stops the ssl-cert package from being installed 
(we're not terminating TLS here). I'll add a comment.

> +&& rm -rf /var/lib/apt/lists/* \
> +&& 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 \
> +&& ln -sfT /dev/stderr "$APACHE_LOG_DIR/error.log" \
> +&& ln -sfT /dev/stdout "$APACHE_LOG_DIR/access.log" \
> +&& ln -sfT /dev/stdout "$APACHE_LOG_DIR/other_vhosts_access.log" \
> +&& chown -R --no-dereference "$APACHE_RUN_USER:$APACHE_RUN_GROUP" 
> "$APACHE_LOG_DIR"
> +
> +RUN echo '' > 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tSetHandler application/x-httpd-php' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo >> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo 'DirectoryIndex disabled' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo 'DirectoryIndex index.php index.html' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo >> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tOptions -Indexes' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tAllowOverride All' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \

Good idea, thanks. Will do.

> +&& a2enconf docker-php
> +
> +RUN apt-get update && apt-get install -y curl php libapache2-mod-php 
> php-mysql php-gd \
> +&& apt-get clean \
> +&& rm -rf /var/lib/apt/lists/*
> +
> +RUN a2dismod mpm_event \
> +&& a2enmod mpm_prefork
> +
> +RUN curl -o wordpress.tar.gz -fSL "https://wordpress.org/latest.tar.gz; \
> +&& tar -xzf wordpress.tar.gz -C /usr/src/ \
> +&& rm wordpress.tar.gz \
> +&& chown -R www-data:www-data /usr/src/wordpress \
> +&& rm -rf /var/www/html \
> +&& mv /usr/src/wordpress /var/www/html
> +
> +COPY --chown=www-data:www-data ./plugins/ /var/www/html/wp-content/plugins/
> +COPY --chown=www-data:www-data ./themes/ /var/www/html/wp-content/themes/
> +
> +EXPOSE 80
> +CMD apachectl -D FOREGROUND
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 000..92c0edb
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,30 @@
> +build: lint deps
> + @echo "Fetching plugins and themes."
> + @tox -e fetch
> + @echo "Building the image."
> + @docker build . -t wordpress:latest
> + @echo "Pushing to the prod-is-external registry."
> + @docker tag wordpress:latest 
> prod-is-external.docker-registry.canonical.com/wordpress:latest
> + @docker push 
> prod-is-external.docker-registry.canonical.com/wordpress:latest
> +
> +deps:
> + @echo "Checking dependencies are present"
> + which bzr || sudo apt-get install -y bzr
> + which git || sudo apt-get install -y git

There is precedent in a few charm Makefiles, but we can discuss on a call. 
Leaving in place pending discussion.

> +
> +lint: clean
> + @echo "Normalising python layout with black."
> + @tox -e black
> + @echo "Running flake8"
> + @tox -e lint
> +
> +clean:
> + @echo "Cleaning files"
> + @rm -rf ./.tox
> + @rm -rf ./.pytest_cache
> + @rm -rf ./plugins/*
> + @rm -rf ./themes/*
> + @mkdir -p plugins
> + @mkdir -p themes
> +
> +.PHONY: build lint clean


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377499
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-13 Thread Tom Haddon 
Some comments inline

Diff comments:

> diff --git a/Dockerfile b/Dockerfile
> new file mode 100644
> index 000..38866f4
> --- /dev/null
> +++ b/Dockerfile
> @@ -0,0 +1,55 @@
> +FROM ubuntu:bionic
> +
> +RUN echo 'debconf debconf/frontend select Noninteractive' | 
> debconf-set-selections
> +
> +RUN apt-get update && apt-get -y dist-upgrade \
> +&& apt-get --purge autoremove -y \
> +&& apt-get clean \
> +&& rm -rf /var/lib/apt/lists/*
> +
> +ENV APACHE_CONFDIR=/etc/apache2
> +ENV APACHE_ENVVARS=/etc/apache2/envvars
> +
> +RUN apt-get update \
> +&& apt-get install -y --no-install-recommends apache2 \

We should confirm if we want --no-install-recommends for all apt installs. If 
not, let's comment as to why.

> +&& rm -rf /var/lib/apt/lists/* \
> +&& 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 \
> +&& ln -sfT /dev/stderr "$APACHE_LOG_DIR/error.log" \
> +&& ln -sfT /dev/stdout "$APACHE_LOG_DIR/access.log" \
> +&& ln -sfT /dev/stdout "$APACHE_LOG_DIR/other_vhosts_access.log" \
> +&& chown -R --no-dereference "$APACHE_RUN_USER:$APACHE_RUN_GROUP" 
> "$APACHE_LOG_DIR"
> +
> +RUN echo '' > 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tSetHandler application/x-httpd-php' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo >> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo 'DirectoryIndex disabled' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo 'DirectoryIndex index.php index.html' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo >> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tOptions -Indexes' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '\tAllowOverride All' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \
> +&& echo '' >> 
> "$APACHE_CONFDIR/conf-available/docker-php.conf" \

Let's switch to a file copy here.

> +&& a2enconf docker-php
> +
> +RUN apt-get update && apt-get install -y curl php libapache2-mod-php 
> php-mysql php-gd \
> +&& apt-get clean \
> +&& rm -rf /var/lib/apt/lists/*
> +
> +RUN a2dismod mpm_event \
> +&& a2enmod mpm_prefork
> +
> +RUN curl -o wordpress.tar.gz -fSL "https://wordpress.org/latest.tar.gz; \
> +&& tar -xzf wordpress.tar.gz -C /usr/src/ \
> +&& rm wordpress.tar.gz \
> +&& chown -R www-data:www-data /usr/src/wordpress \
> +&& rm -rf /var/www/html \
> +&& mv /usr/src/wordpress /var/www/html
> +
> +COPY --chown=www-data:www-data ./plugins/ /var/www/html/wp-content/plugins/
> +COPY --chown=www-data:www-data ./themes/ /var/www/html/wp-content/themes/
> +
> +EXPOSE 80
> +CMD apachectl -D FOREGROUND
> diff --git a/Makefile b/Makefile
> new file mode 100644
> index 000..92c0edb
> --- /dev/null
> +++ b/Makefile
> @@ -0,0 +1,30 @@
> +build: lint deps
> + @echo "Fetching plugins and themes."
> + @tox -e fetch
> + @echo "Building the image."
> + @docker build . -t wordpress:latest
> + @echo "Pushing to the prod-is-external registry."
> + @docker tag wordpress:latest 
> prod-is-external.docker-registry.canonical.com/wordpress:latest
> + @docker push 
> prod-is-external.docker-registry.canonical.com/wordpress:latest
> +
> +deps:
> + @echo "Checking dependencies are present"
> + which bzr || sudo apt-get install -y bzr
> + which git || sudo apt-get install -y git

My preference would be to warn on missing packages rather than install, but 
would be interested if others feel the same.

> +
> +lint: clean
> + @echo "Normalising python layout with black."
> + @tox -e black
> + @echo "Running flake8"
> + @tox -e lint
> +
> +clean:
> + @echo "Cleaning files"
> + @rm -rf ./.tox
> + @rm -rf ./.pytest_cache
> + @rm -rf ./plugins/*
> + @rm -rf ./themes/*
> + @mkdir -p plugins
> + @mkdir -p themes
> +
> +.PHONY: build lint clean


-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377499
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp

Re: [Wordpress-charmers] [Merge] ~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master into ~wordpress-charmers/charm-k8s-wordpress/+git/wordpress-k8s-image-builder:master

2020-01-13 Thread Canonical IS Mergebot 
This merge proposal is being monitored by mergebot. Change the status to 
Approved to merge.
-- 
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/wordpress-k8s-image-builder/+merge/377499
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.

-- 
Mailing list: https://launchpad.net/~wordpress-charmers
Post to : wordpress-charmers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~wordpress-charmers
More help   : https://help.launchpad.net/ListHelp