Juju Charms Collection
haproxy package

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

  1. Precise Pangolin (12.04)
  2. trunk
  3. Merge into trunk
Proposed by Matt Bruzek
Status: Merged
Merged at revision: 76
Proposed branch: lp:~mbruzek/charms/precise/haproxy/trunk
Merge into: lp:charms/haproxy
Diff against target: 150 lines (+135/-0)
3 files modified
tests/00_setup.sh (+14/-0)
tests/10_deploy_test.py (+80/-0)
tests/default_apache.tmpl (+41/-0)
To merge this branch: bzr merge lp:~mbruzek/charms/precise/haproxy/trunk
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+202945@code.launchpad.net

Description of the change

Added amulet tests for haproxy charm.

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

To post a comment you must log in.
Revision history for this message
Matt Bruzek (mbruzek) wrote :

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

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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/

Revision history for this message
Matt Bruzek (mbruzek) wrote :

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 updated
76. By Matt Bruzek <email address hidden>

Fixing code review comments

Revision history for this message
Matt Bruzek (mbruzek) wrote :

Please take a look.

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

Revision history for this message
Matt Bruzek (mbruzek) wrote :

Please take a look.

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

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'tests'
2=== added file 'tests/00_setup.sh'
3--- tests/00_setup.sh 1970-01-01 00:00:00 +0000
4+++ tests/00_setup.sh 2014-01-24 16:58:22 +0000
5@@ -0,0 +1,14 @@
6+#!/bin/sh
7+
8+# The script installs amulet and other tools needed for the amulet tests.
9+
10+set -x
11+
12+# Get the status of the amulet package, this returns 0 of package is installed.
13+dpkg -s amulet
14+if [ $? -ne 0 ]; then
15+ # Install the Amulet testing harness.
16+ sudo add-apt-repository -y ppa:juju/stable
17+ sudo apt-get update
18+ sudo apt-get install -y amulet
19+fi
20
21=== added file 'tests/10_deploy_test.py'
22--- tests/10_deploy_test.py 1970-01-01 00:00:00 +0000
23+++ tests/10_deploy_test.py 2014-01-24 16:58:22 +0000
24@@ -0,0 +1,80 @@
25+#!/usr/bin/python3
26+
27+# This Amulet test deploys haproxy and related charms.
28+
29+import os
30+import amulet
31+import requests
32+import base64
33+
34+d = amulet.Deployment()
35+# Add the haproxy charm to the deployment.
36+d.add('haproxy')
37+d.add('apache2')
38+
39+# Get the directory this way to load the file when CWD is different.
40+path = os.path.abspath(os.path.dirname(__file__))
41+template_path = os.path.join(path, 'default_apache.tmpl')
42+# Read in the Apache2 default template file.
43+with open(template_path) as f:
44+ template = f.read()
45+ encodedTemplate = base64.b64encode(template.encode('utf-8'))
46+# Create a dictionary with configuration values for apache2.
47+configuration = {'vhost_https_template': encodedTemplate.decode('ascii')}
48+# Apache2 needs a base64 encoded template to configure the web site.
49+d.configure('apache2', configuration)
50+
51+# Relate the haproxy to apache2.
52+d.relate('haproxy:reverseproxy', 'apache2:website')
53+# Make the haproxy visible to the outside world.
54+d.expose('haproxy')
55+
56+# The number of seconds to wait for the environment to setup.
57+seconds = 900
58+try:
59+ # Execute the deployer with the current mapping.
60+ d.setup(timeout=seconds)
61+ # Wait for the relation to finish the transations.
62+ d.sentry.wait(seconds)
63+except amulet.helpers.TimeoutError:
64+ message = 'The environment did not setup in %d seconds.' % seconds
65+ # The SKIP status enables skip or fail the test based on configuration.
66+ amulet.raise_status(amulet.SKIP, msg=message)
67+except:
68+ raise
69+
70+# Test that haproxy is acting as the proxy for apache2.
71+
72+# Get the haproxy unit.
73+haproxy_unit = d.sentry.unit['haproxy/0']
74+haproxy_address = haproxy_unit.info['public-address']
75+page = requests.get('http://%s/index.html' % haproxy_address)
76+# Raise an error if the page does not load through haproxy.
77+page.raise_for_status()
78+print('Successfully got the Apache2 web page through haproxy IP address.')
79+
80+# Test that the apache2 relation data is saved on the haproxy server.
81+
82+# Get the sentry for apache and get the private IP address.
83+apache_unit = d.sentry.unit['apache2/0']
84+# Get the relation.
85+relation = apache_unit.relation('website', 'haproxy:reverseproxy')
86+# Get the private address from the relation.
87+apache_private = relation['private-address']
88+
89+print('Private address of the apache2 relation ', apache_private)
90+
91+# Grep the configuration file for the private address
92+output, code = haproxy_unit.run('grep %s /etc/haproxy/haproxy.cfg' %
93+ apache_private)
94+if code == 0:
95+ print('Found the relation IP address in the haproxy configuration file!')
96+ print(output)
97+else:
98+ print(output)
99+ message = 'Unable to find the Apache IP address %s in the haproxy ' \
100+ 'configuration file.' % apache_private
101+ amulet.raise_status(amulet.FAIL, msg=message)
102+
103+# Send a message that the tests are complete.
104+print('The haproxy tests are complete.')
105
106=== added file 'tests/default_apache.tmpl'
107--- tests/default_apache.tmpl 1970-01-01 00:00:00 +0000
108+++ tests/default_apache.tmpl 2014-01-24 16:58:22 +0000
109@@ -0,0 +1,41 @@
110+<VirtualHost *:80>
111+ ServerAdmin webmaster@localhost
112+
113+ DocumentRoot /var/www
114+ <Directory />
115+ Options FollowSymLinks
116+ AllowOverride None
117+ </Directory>
118+ <Directory /var/www/>
119+ Options Indexes FollowSymLinks MultiViews
120+ AllowOverride None
121+ Order allow,deny
122+ allow from all
123+ </Directory>
124+
125+ ScriptAlias /cgi-bin/ /usr/lib/cgi-bin/
126+ <Directory "/usr/lib/cgi-bin">
127+ AllowOverride None
128+ Options +ExecCGI -MultiViews +SymLinksIfOwnerMatch
129+ Order allow,deny
130+ Allow from all
131+ </Directory>
132+
133+ ErrorLog ${APACHE_LOG_DIR}/error.log
134+
135+ # Possible values include: debug, info, notice, warn, error, crit,
136+ # alert, emerg.
137+ LogLevel warn
138+
139+ CustomLog ${APACHE_LOG_DIR}/access.log combined
140+
141+ Alias /doc/ "/usr/share/doc/"
142+ <Directory "/usr/share/doc/">
143+ Options Indexes MultiViews FollowSymLinks
144+ AllowOverride None
145+ Order deny,allow
146+ Deny from all
147+ Allow from 127.0.0.0/255.0.0.0 ::1/128
148+ </Directory>
149+
150+</VirtualHost>

Subscribers

People subscribed via source and target branches