Merge lp:~ahasenack/txaws/no-local-dir-for-ca-lookup into lp:txaws

Proposed by Andreas Hasenack on 2012-10-29
Status: Merged
Approved by: Christopher Armstrong on 2012-10-30
Approved revision: 158
Merged at revision: 151
Proposed branch: lp:~ahasenack/txaws/no-local-dir-for-ca-lookup
Merge into: lp:txaws
Diff against target: 0 lines
To merge this branch: bzr merge lp:~ahasenack/txaws/no-local-dir-for-ca-lookup
Reviewer Review Type Date Requested Status
Christopher Armstrong Approve on 2012-10-30
Jamu Kakar 2012-10-29 Approve on 2012-10-29
Review via email:

Description of the change

This branch changes get_ca_certs() so that it does not accidentally include the current directory when looking for *.pem CA files to load.

To post a comment you must log in.
Jamu Kakar (jkakar) wrote :


The test is tainting the TXAWS_CERTS_PATH environment variable. You
could do something like the following. Also, I recommend you add a
simple sentence clearly describing the expected behaviour.

def test_get_ca_certs_no_current_dir(self):
    A L{CertsNotFoundError} exception is raised if no certificate paths are
    original_certs_path = os.environ.get("TXAWS_CERTS_PATH")
        os.environ["TXAWS_CERTS_PATH"] = "%s:" % self.no_certs_dir
        self.assertRaises(exception.CertsNotFoundError, ssl.get_ca_certs)
        if original_certs_path:
            os.environ["TXAWS_CERTS_PATH"] = original_certs_path
            del os.environ["TXAWS_CERTS_PATH"]

Nice work, +1! :)

review: Approve
Andreas Hasenack (ahasenack) wrote :

That docstring is not accurate. What I'm testing is that get_ca_certs() does not include the current directory if TXAWS_CERTS_PATH ends with a ":".

Good point about the variable remaining tainted, will fix, thanks.

Jamu Kakar (jkakar) wrote :

Hehe, that's a good reason to provide clear docstrings with tests.
They can be so easy to misunderstand, especially when you read the
code before the morning coffee has hit your bloodstream. ;b

Andreas Hasenack (ahasenack) wrote :

Can't I just put the os.chdir() that goes back to the original dir in that finally block too?

Andreas Hasenack (ahasenack) wrote :

NM, got some addCleanup() introduction from radix :)

Jamu Kakar (jkakar) wrote :

Yeah, addCleanup is your friend.

Andreas Hasenack (ahasenack) wrote :

Ok, how about this now.

Andreas Hasenack (ahasenack) wrote :

Having fun here with such a simple case :)

158. By Andreas Hasenack on 2012-10-30

Tidy up

Christopher Armstrong (radix) wrote :

Looks good. Thanks for moving the os.getcwd() call and fixing the other tests. +1

review: Approve

Preview Diff



People subscribed via source and target branches