Code review comment for lp:~vila/bzr/920455-ssl-defaults

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

32 def test_specified_doesnt_exist(self):
33 path = os.path.join(self.test_dir, "nonexisting.pem")
34 stack = self.get_stack("ssl.ca_certs = %s\n" % path)
35 - self.warnings = []
36 - def warning(*args):
37 - self.warnings.append(args[0] % args[1:])
38 - self.overrideAttr(trace, 'warning', warning)
39 - self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
40 - stack.get('ssl.ca_certs'))
41 - self.assertLength(1, self.warnings)
42 - self.assertContainsRe(self.warnings[0],
43 - "is not valid for \"ssl.ca_certs\"")
44 + self.assertRaises(ConfigOptionValueError, stack.get, 'ssl.ca_certs')
Should it really be an error if the ssl.ca_certs path doesn't exist? What if e.g. "ssl.ca_reqs = optional", it doesn't seem like it should be a problem if the ca_certs are missing.

default_ca_cert() seems to return "/etc/ssl/..." on Windows. This seems wrong in any case. I realize you've asked for feedback from the packagers, but I think we should raise ValueError or return None for now at least.

review: Approve

« Back to merge proposal