Merge lp:~mbruzek/charms/precise/haproxy/trunk into lp:charms/haproxy

[Matt Bruzek]

Added amulet tests for haproxy charm.

https://codereview.appspot.com/56140043/

[Matt Bruzek (mbruzek)]

Reviewers: mp+202945_code.launchpad.net,

Message:
Please take a look.

Description:
Added amulet tests for haproxy charm.

https://code.launchpad.net/~mbruzek/charms/precise/haproxy/trunk/+merge/202945

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/56140043/

Affected files (+137, -0 lines):
   A [revision details]
   A tests/00_setup.sh
   A tests/10_deploy_test.py
   A tests/default_apache.tmpl

[Benjamin Saller (bcsaller)]

LGTM with minors

some suggestions below. Thanks for this.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py
File tests/10_deploy_test.py (right):

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode19
tests/10_deploy_test.py:19: #d.add('apache2', units=units)
comment and commented out code can be removed.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode24
tests/10_deploy_test.py:24: template_path = os.path.join(path,
'default_apache.tmpl')
I think the preferred way of doing this is with pkg_resources:

import pkg_resources
my_data = pkg_resources.resource_string(__name__, "default_apache.tmp")

which would return the contents of the package relative file as a
string.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode29
tests/10_deploy_test.py:29: configuration = {'vhost_https_template' :
encodedTemplate.decode('ascii')}
Were you really seeing issues that forced you down the path of acsii
encode/decodes?

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode65
tests/10_deploy_test.py:65: print('Private address of the apache2
relation ', relation['private-address'])
I think we can remove the print here, unless you think its worth
keeping.

https://codereview.appspot.com/56140043/

[Matt Bruzek (mbruzek)]

Thanks for the review Benjamin.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py
File tests/10_deploy_test.py (right):

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode19
tests/10_deploy_test.py:19: #d.add('apache2', units=units)
Thanks for the comment, removed the comments.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode24
tests/10_deploy_test.py:24: template_path = os.path.join(path,
'default_apache.tmpl')
I was not aware of pkg_resources. I get an error when I try that code in
python3.

Python 3.3.3 (default, Jan 11 2014, 05:48:01)
[GCC 4.8.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkg_resources
>>> template = pkg_resources.resource_string(__name__,
"default_apache.tmpl")
Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
   File "/usr/lib/python3/dist-packages/pkg_resources.py", line 891, in
resource_string
     self, resource_name
   File "/usr/lib/python3/dist-packages/pkg_resources.py", line 1292, in
get_resource_string
     return self._get(self._fn(self.module_path, resource_name))
   File "/usr/lib/python3/dist-packages/pkg_resources.py", line 1370, in
_get
     "Can't perform this operation for loaders without 'get_data()'"
NotImplementedError: Can't perform this operation for loaders without
'get_data()'

Seems to work fine in python2.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode29
tests/10_deploy_test.py:29: configuration = {'vhost_https_template' :
encodedTemplate.decode('ascii')}
You are right I can convert (encode) the template with utf-8 but all the
character set of base65 can be represented in ascii, so I will keep that
on the decode.

https://codereview.appspot.com/56140043/diff/1/tests/10_deploy_test.py#newcode65
tests/10_deploy_test.py:65: print('Private address of the apache2
relation ', relation['private-address'])
I really like having the print there for manual testing. It does not
create too much extra logging I would prefer to keep it.

https://codereview.appspot.com/56140043/

[lp:~mbruzek/charms/precise/haproxy/trunk]

76. By Matt Bruzek <email address hidden> on 2014-01-24

Fixing code review comments

[Matt Bruzek (mbruzek)]

Please take a look.

https://codereview.appspot.com/56140043/

[Matt Bruzek (mbruzek)]

Please take a look.

https://codereview.appspot.com/56140043/

[Marco Ceppi (marcoceppi)]

These LGTM, there's a suggestion as to a more...bashonic? bashinista?
Anyways these will be merged shortly

https://codereview.appspot.com/56140043/diff/60001/tests/00_setup.sh
File tests/00_setup.sh (right):

https://codereview.appspot.com/56140043/diff/60001/tests/00_setup.sh#newcode9
tests/00_setup.sh:9: if [ $? -ne 0 ]; then
This is really just a knitpick, but you can combine these two lines to
just

if ! dpkg -s amulet 2>/dev/null; then

If you drop the [] and just run a command the if statement will be based
on the exit status. so 0 is true otherwise false. The 2>/dev/null is
just to redirect the stderr (really optional and not a big deal).

https://codereview.appspot.com/56140043/

=== added directory 'tests'
=== added file 'tests/00_setup.sh'
--- tests/00_setup.sh	1970-01-01 00:00:00 +0000
+++ tests/00_setup.sh	2014-01-24 16:58:22 +0000
@@ -0,0 +1,14 @@
+#!/bin/sh
+
+# The script installs amulet and other tools needed for the amulet tests.
+
+set -x 
+
+# Get the status of the amulet package, this returns 0 of package is installed.
+dpkg -s amulet
+if [ $? -ne 0 ]; then
+  # Install the Amulet testing harness.
+  sudo add-apt-repository -y ppa:juju/stable
+  sudo apt-get update 
+  sudo apt-get install -y amulet
+fi

=== added file 'tests/10_deploy_test.py'
--- tests/10_deploy_test.py	1970-01-01 00:00:00 +0000
+++ tests/10_deploy_test.py	2014-01-24 16:58:22 +0000
@@ -0,0 +1,80 @@
+#!/usr/bin/python3
+
+# This Amulet test deploys haproxy and related charms.
+
+import os
+import amulet
+import requests
+import base64
+
+d = amulet.Deployment()
+# Add the haproxy charm to the deployment.
+d.add('haproxy')
+d.add('apache2')
+
+# Get the directory this way to load the file when CWD is different.
+path = os.path.abspath(os.path.dirname(__file__))
+template_path = os.path.join(path, 'default_apache.tmpl')
+# Read in the Apache2 default template file.
+with open(template_path) as f:
+    template = f.read()
+    encodedTemplate = base64.b64encode(template.encode('utf-8'))
+# Create a dictionary with configuration values for apache2.
+configuration = {'vhost_https_template': encodedTemplate.decode('ascii')}
+# Apache2 needs a base64 encoded template to configure the web site.
+d.configure('apache2', configuration)
+
+# Relate the haproxy to apache2.
+d.relate('haproxy:reverseproxy', 'apache2:website')
+# Make the haproxy visible to the outside world.
+d.expose('haproxy')
+
+# The number of seconds to wait for the environment to setup.
+seconds = 900
+try:
+    # Execute the deployer with the current mapping.
+    d.setup(timeout=seconds)
+    # Wait for the relation to finish the transations.
+    d.sentry.wait(seconds)
+except amulet.helpers.TimeoutError:
+    message = 'The environment did not setup in %d seconds.' % seconds
+    # The SKIP status enables skip or fail the test based on configuration.
+    amulet.raise_status(amulet.SKIP, msg=message)
+except:
+    raise
+
+# Test that haproxy is acting as the proxy for apache2.
+
+# Get the haproxy unit.
+haproxy_unit = d.sentry.unit['haproxy/0']
+haproxy_address = haproxy_unit.info['public-address']
+page = requests.get('http://%s/index.html' % haproxy_address)
+# Raise an error if the page does not load through haproxy.
+page.raise_for_status()
+print('Successfully got the Apache2 web page through haproxy IP address.')
+
+# Test that the apache2 relation data is saved on the haproxy server.
+
+# Get the sentry for apache and get the private IP address.
+apache_unit = d.sentry.unit['apache2/0']
+# Get the relation.
+relation = apache_unit.relation('website', 'haproxy:reverseproxy')
+# Get the private address from the relation.
+apache_private = relation['private-address']
+
+print('Private address of the apache2 relation ', apache_private)
+
+# Grep the configuration file for the private address
+output, code = haproxy_unit.run('grep %s /etc/haproxy/haproxy.cfg' %
+                                apache_private)
+if code == 0:
+    print('Found the relation IP address in the haproxy configuration file!')
+    print(output)
+else:
+    print(output)
+    message = 'Unable to find the Apache IP address %s in the haproxy ' \
+              'configuration file.' % apache_private
+    amulet.raise_status(amulet.FAIL, msg=message)
+
+# Send a message that the tests are complete.
+print('The haproxy tests are complete.')

=== added file 'tests/default_apache.tmpl'
--- tests/default_apache.tmpl	1970-01-01 00:00:00 +0000
+++ tests/default_apache.tmpl	2014-01-24 16:58:22 +0000
@@ -0,0 +1,41 @@
+<VirtualHost *:80>
+	ServerAdmin webmaster@localhost
+
+	DocumentRoot /var/www
+	<Directory />
+		Options FollowSymLinks
+		AllowOverride None
+	</Directory>
+	<Directory /var/www/>
+		Options Indexes FollowSymLinks MultiViews
+		AllowOverride None
+		Order allow,deny
+		allow from all
+	</Directory>
+
+	ScriptAlias /cgi-bin/ /usr/lib/cgi-bin/
+	<Directory "/usr/lib/cgi-bin">
+		AllowOverride None
+		Options +ExecCGI -MultiViews +SymLinksIfOwnerMatch
+		Order allow,deny
+		Allow from all
+	</Directory>
+
+	ErrorLog ${APACHE_LOG_DIR}/error.log
+
+	# Possible values include: debug, info, notice, warn, error, crit,
+	# alert, emerg.
+	LogLevel warn
+
+	CustomLog ${APACHE_LOG_DIR}/access.log combined
+
+    Alias /doc/ "/usr/share/doc/"
+    <Directory "/usr/share/doc/">
+        Options Indexes MultiViews FollowSymLinks
+        AllowOverride None
+        Order deny,allow
+        Deny from all
+        Allow from 127.0.0.0/255.0.0.0 ::1/128
+    </Directory>
+
+</VirtualHost>