Merge lp:~canonical-losas/charms/trusty/squid-reverseproxy/venv-testing into lp:charms/trusty/squid-reverseproxy

Proposed by Paul Collins
Status: Merged
Merged at revision: 54
Proposed branch: lp:~canonical-losas/charms/trusty/squid-reverseproxy/venv-testing
Merge into: lp:charms/trusty/squid-reverseproxy
Diff against target: 39 lines (+11/-2)
2 files modified
Makefile (+10/-1)
hooks/install (+1/-1)
To merge this branch: bzr merge lp:~canonical-losas/charms/trusty/squid-reverseproxy/venv-testing
Reviewer Review Type Date Requested Status
Adam Israel (community) Needs Fixing
Review Queue (community) automated testing Needs Fixing
charmers Pending
Review via email: mp+250574@code.launchpad.net

This proposal supersedes a proposal from 2015-02-23.

Description of the change

Address bug#1401323 by creating the virtualenv using --system-site-packages so that apt_pkg can be accessed.

[Resubmit: Fix conflict vs. trusty.]

To post a comment you must log in.
Revision history for this message
Review Queue (review-queue) wrote :

This items has failed automated testing! Results available here http://reports.vapour.ws/charm-tests/charm-bundle-test-11055-results

review: Needs Fixing (automated testing)
Revision history for this message
Adam Israel (aisrael) wrote :

Hi Paul,

Thanks for your efforts to improve the testing story for this charm! I had the chance to review your merge proposal today. Adding virtualenv is a welcome addition here.

I ran into a few issues that I wanted to bring to your attention, though. While the venv is created, the Makefile continues to use the system python, installing packages globally. Once the venv is created, it should run commands from the `.venv/bin` directory, i.e., `.venv/bin/python`, `.venv/bin/pip`. With that, I also ran into version errors with pep8.

I reworked the Makefile to install and use the venv, and put the code here for reference:
https://code.launchpad.net/~aisrael/charms/trusty/squid-reverseproxy/rework_venv

With those issues above resolved, I see no problem getting these changes moved forward. Thanks again for your efforts! Just set this back to 'Needs Review' when you're ready for us to take another look.

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

Moving this to Work In Progress, when ready for another review please set to "Needs Review"

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile'
--- Makefile 2014-10-31 15:05:23 +0000
+++ Makefile 2015-02-23 02:12:45 +0000
@@ -21,7 +21,7 @@
21 @(charm proof $(PWD) || [ $$? -eq 100 ]) && echo OK21 @(charm proof $(PWD) || [ $$? -eq 100 ]) && echo OK
22 @test `cat revision` = 0 && rm revision22 @test `cat revision` = 0 && rm revision
2323
24test:24test: .venv
25 @echo Starting tests...25 @echo Starting tests...
26 @pip install nose testtools mock pyyaml26 @pip install nose testtools mock pyyaml
27 @CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) nosetests -s $(TEST_DIR)27 @CHARM_DIR=$(CHARM_DIR) $(TEST_PREFIX) nosetests -s $(TEST_DIR)
@@ -41,4 +41,13 @@
41 -d hooks/charmhelpers41 -d hooks/charmhelpers
42 @echo Do not forget to commit the updated files if any.42 @echo Do not forget to commit the updated files if any.
4343
44.venv:
45 sudo apt-get install -y python-pip python-coverage python-nose python-testtools \
46 python-mock make bzr python-virtualenv
47 virtualenv .venv --system-site-packages
48
49clean:
50 rm -rf .venv
51
52
44.PHONY: revision proof test lint sourcedeps charm-payload53.PHONY: revision proof test lint sourcedeps charm-payload
4554
=== modified file 'hooks/install'
--- hooks/install 2013-10-10 22:50:35 +0000
+++ hooks/install 2015-02-23 02:12:45 +0000
@@ -3,7 +3,7 @@
3set -eu3set -eu
44
5juju-log 'Invoking charm-pre-install hooks'5juju-log 'Invoking charm-pre-install hooks'
6[ -d exec.d ] && ( for f in exec.d/*/charm-pre-install; do [ -x $f ] && /bin/sh -c "$f"; done )6[ -d exec.d ] && ( for f in exec.d/*/charm-pre-install; do [ -x $f ] && /bin/sh -c "$f"; done ) || true
77
8juju-log 'Invoking python-based install hook'8juju-log 'Invoking python-based install hook'
9python hooks/hooks.py install9python hooks/hooks.py install

Subscribers

People subscribed via source and target branches