Code review comment for lp:~jacekn/charm-helpers/charm-helpers-ssl

Revision history for this message
Matthew Wedgwood (mew) wrote :

This looks quite helpful. Thanks.

I see a few issues here.

Running this as generate_selfsigned("mykey", "mycert") fails:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "charmhelpers/contrib/ssl/__init__.py", line 19, in generate_selfsigned
    subprocess.check_call(cmd)
UnboundLocalError: local variable 'cmd' referenced before assignment

There should be a test that catches this.

Furthermore, the "subject" argument is a little opaque. I somewhat expected to be able to pass it a hostname, but that's not at all what's needed. I'd recommend any combination of:

1. Adding a docstring with a little explanation
2. Exposing the subject components as function args (rather than as one dict)

As a bonus, it might be nice to allow users to specify the number of bits the for the cert as 1024 will inevitably fall out of vogue.
3. Building a simple "CertificateSubject" class to help users construct a valid subject
4. Providing reasonable defaults.

review: Needs Fixing

« Back to merge proposal