On Wed, 2015-05-06 at 10:23 -0400, Ralph Bean wrote:
On Wed, May 06, 2015 at 01:53:12PM +0200, Mathieu Bridon wrote:
> + if self.client_cert is not None:
> + if os.path.exists(self.client_cert):
> + c.setopt(pycurl.SSLCERT, self.client_cert)
> + else:
> + self.log.warn("Missing certificate: %s"
> + % self.client_cert)
> +
> + if self.ca_cert is not None:
> + if os.path.exists(self.ca_cert):
> + c.setopt(pycurl.CAINFO, self.ca_cert)
> + else:
> + self.log.warn("Missing certificate: %s" %
self.ca_cert)
Hm, I think that these two ``log.warn`` statements should probably be
raised exceptions.
If I'm reading this right, in the previous commit you provide an
interface for users of the library to override properties to define
their own client certs and ca certs. If they use that and specify
those paths explicitly, but they don't exist on disk, that warrants
more than just a warning (which might get swallowed up by someone's
incorrectly configured logging somewhere). An outright exception that
stops execution is due.
I'd agree with you in absolute, but the patch series is already pretty
big, so I preferred avoiding to sneak in some behavioural changes like
this.
fedpkg currently only logs a warning, so I kept the same here.
We could change this of course, but I thought it would be better to
change it in a followup patch, rather than in the middle of this big
series.
If there's consensus on the list to raise exceptions here, I'll send a
patch 22/21. :)
--
Mathieu