Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1407)

Issue 56140043: Added amulet tests for haproxy charm.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by mbruzek
Modified:
5 years, 6 months ago
Reviewers:
mp+202945, benjamin.saller, Marco Ceppi
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 8

Patch Set 2 : Added amulet tests for haproxy charm. #

Patch Set 3 : Added amulet tests for haproxy charm. #

Patch Set 4 : Added amulet tests for haproxy charm. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+137 lines, -0 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
A tests/00_setup.sh View 1 chunk +14 lines, -0 lines 1 comment Download
A tests/10_deploy_test.py View 1 1 chunk +80 lines, -0 lines 0 comments Download
A tests/default_apache.tmpl View 1 chunk +41 lines, -0 lines 0 comments Download

Messages

Total messages: 6
mbruzek
Please take a look.
5 years, 7 months ago (2014-01-23 20:30:11 UTC) #1
benjamin.saller
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: ...
5 years, 7 months ago (2014-01-23 20:54:52 UTC) #2
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 ...
5 years, 7 months ago (2014-01-24 15:32:13 UTC) #3
mbruzek
Please take a look.
5 years, 7 months ago (2014-01-24 16:58:51 UTC) #4
mbruzek
Please take a look.
5 years, 7 months ago (2014-01-24 16:59:48 UTC) #5
Marco Ceppi
5 years, 6 months ago (2014-02-07 12:28:27 UTC) #6
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).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b