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

On 01/30/2012 03:19 PM, Vincent Ladeuil wrote:
> > Review: Approve
> > 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.
>
> Indeed ! That's why I also change the code to query the option only if
> required ;)
You're (correctly) trying to retrieve the ca certs too if
ssl.ca_reqs=optional, but if the ca certs path doesn't exist, that
becomes a terminal error. It shouldn't be if ssl.ca_reqs=optional, only
if ssl.ca_reqs=required.
>
>
> > I realize you've asked for feedback from the packagers, but I
> > think we should raise ValueError or return None for now at least.
>
> That's what will happen if the option is needed, see
> ca_certs_from_store() and invalid='error' ! ;)
I don't see how that's the case. We'll be trying to retrieve the ca
certs in that case and it'll cause ConfigOptionValueError to be raised
(and bzr to be aborted), right?

Cheers,

Jelmer