Merge lp://staging/~dpb/charm-tools/fix-test-env-1271745 into lp://staging/charm-tools/1.2

Proposed by David Britton
Status: Merged
Merged at revision: 311
Proposed branch: lp://staging/~dpb/charm-tools/fix-test-env-1271745
Merge into: lp://staging/charm-tools/1.2
Diff against target: 50 lines (+22/-0)
2 files modified
charmtools/test.py (+4/-0)
tests/test_juju_test.py (+18/-0)
To merge this branch: bzr merge lp://staging/~dpb/charm-tools/fix-test-env-1271745
Reviewer Review Type Date Requested Status
Marco Ceppi (community) Approve
Review via email: mp+202880@code.staging.launchpad.net

Description of the change

Simple fix to import current environment into the test conductor. Without doing this, at least the following things break for me:

- I lose connection to the ssh agent
- I don't get a working path so things like "juju ssh service/0" stop working

I included a test case that breaks if my code change is not in place.

Sorry for the bug number in the branch name, it's a different bug with a similar title, ignore it. This is to fix bug #1271803

To post a comment you must log in.
Revision history for this message
Marco Ceppi (marcoceppi) wrote :

I don't like that this sets /all/ the environment variables. I'd rather there be a whitelist of environment variables to pass to the tests as the testing environment will have very limited scope of env variables.

review: Needs Fixing
310. By David Britton

limit to whitelist of env variables.

Revision history for this message
David Britton (dpb) wrote :

On Fri, Jan 24, 2014 at 3:47 PM, Marco Ceppi <email address hidden> wrote:

> Review: Needs Fixing
>
> I don't like that this sets /all/ the environment variables. I'd rather
> there be a whitelist of environment variables to pass to the tests as the
> testing environment will have very limited scope of env variables.
>

Fixed with corresponding test cases.

--
David Britton <email address hidden>

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

LGTM +1 thank you

review: Approve
311. By David Britton

Merging up

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: