Juju Charms Collection
haproxy package

Merge lp://staging/~jseutter/charms/precise/haproxy/trunk into lp://staging/charms/haproxy

Precise Pangolin (12.04)
trunk
Merge into trunk

Proposed by Jerry Seutter on 2013-12-11

Status:	Merged
Merged at revision:	72
Proposed branch:	lp://staging/~jseutter/charms/precise/haproxy/trunk
Merge into:	lp://staging/charms/haproxy
Diff against target:	183 lines (+99/-4) 3 files modified hooks/hooks.py (+27/-3) hooks/tests/test_helpers.py (+37/-0) hooks/tests/test_peer_hooks.py (+35/-1)
To merge this branch:	bzr merge lp://staging/~jseutter/charms/precise/haproxy/trunk
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Marco Ceppi (community)	2013-12-11	Approve on 2014-01-03
Jerry Seutter (community)		Abstain on 2013-12-13
Chris Glass (community)		Approve on 2013-12-13
Adam Collard (community)	2013-12-12	Approve on 2013-12-12
Review via email: mp+198646@code.staging.launchpad.net

Description of the change

This adds support for the backend service to specify errorfiles in the service configuration. If errorfiles are supplied, the haproxy charm will write them to /var/lib/haproxy/<service_name>/<http status>.html and configure haproxy to use them.

Currently only the Landscape charm supplies errorfiles.

Revision history for this message

Adam Collard (adam-collard) wrote on 2013-12-12:

8 +import base64

Move this up to the first import line so the imports stay in alphabetical order

57 + path = "%s/service_%s" % (default_haproxy_lib_dir, service_name)
58 + if not os.path.exists(path):
59 + os.makedirs(path)
60 + full_path = "%s/%s.html" % (path, errorfile["http_status"])

Use os.path.join() for the path constructions.

121 +

One too many blank lines?

+ self.assertTrue(create_listen_stanza.called)
171 +

move that outside of the with-block - you don't need the patching in place for this

review: Approve

lp://staging/~jseutter/charms/precise/haproxy/trunk updated on 2013-12-13

72. By Jerry Seutter on 2013-12-12: Integrate review feedback.
73. By Jerry Seutter on 2013-12-13: Build path using os.path.join()

Revision history for this message

Jerry Seutter (jseutter) wrote on 2013-12-13:

> 8 +import base64
>
> Move this up to the first import line so the imports stay in alphabetical
> order

Fixed.

>
>
> 57 + path = "%s/service_%s" % (default_haproxy_lib_dir, service_name)
> 58 + if not os.path.exists(path):
> 59 + os.makedirs(path)
> 60 + full_path = "%s/%s.html" % (path, errorfile["http_status"])
>
> Use os.path.join() for the path constructions.
>

Fixed

> 121 +
>
> One too many blank lines?
>

Fixed

> + self.assertTrue(create_listen_stanza.called)
> 171 +
>
> move that outside of the with-block - you don't need the patching in place for
> this

Fixed. Thanks!

Revision history for this message

Chris Glass (tribaal) wrote on 2013-12-13:

Great! +1

The following points are different facets of the same thing:

[0]
+ "%s.html" % errorfile["http_status"])

The HAProxy configuration suggests to call the files *.http instead, since their content is printed to the socket verbatim (it's really a serialized http response, not an HTML file).

[1]
+ errorfiles = [{'http_status': 403, 'path': '/path/403.html',
+ 'content': base64.b64encode('<html></html>')}]

Similar comment, it would probably be good to use complete values to serve as an example, like: http://paste.ubuntu.com/6567575/

[2]
+# content: base 64 content to serve as doc

Maybe rephrase to "base 64 content for HAproxy to print to socket"?

review: Approve

Revision history for this message

Jerry Seutter (jseutter) on 2013-12-13:

review: Abstain

lp://staging/~jseutter/charms/precise/haproxy/trunk updated on 2013-12-13

74. By Jerry Seutter on 2013-12-13: Error file extension is now .http
The code now reflects the fact that errorfiles is the raw HTTP response,
not just an html document.

Revision history for this message

Jerry Seutter (jseutter) wrote on 2013-12-13:

> Great! +1
>
> The following points are different facets of the same thing:
>
> [0]
> + "%s.html" % errorfile["http_status"])
>
> The HAProxy configuration suggests to call the files *.http instead, since
> their content is printed to the socket verbatim (it's really a serialized http
> response, not an HTML file).

Ah, I had read that once but it didn't stick in my brain. Fixed now.

> [1]
> + errorfiles = [{'http_status': 403, 'path': '/path/403.html',
> + 'content': base64.b64encode('<html></html>')}]
>
> Similar comment, it would probably be good to use complete values to serve as
> an example, like: http://paste.ubuntu.com/6567575/

Fixed.

> [2]
> +# content: base 64 content to serve as doc
>
> Maybe rephrase to "base 64 content for HAproxy to print to socket"?

Fixed. Thanks, tribaal!

lp://staging/~jseutter/charms/precise/haproxy/trunk updated on 2013-12-17