[pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates
- From: Erik Trauschke <
- To: Yiteng Zhang <
- Subject: [pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates
- Date: Fri, 13 Dec 2013 17:13:44 -0800
On 12/13/13 04:38 PM, Yiteng Zhang wrote:
Thank you for your detailed suggestions. In my first version, I supposed
that I have to start another Apache server instead of the one that
HTTPSTestClass uses because I was creating a new certificate that
effects the existing server ca cert file "combined_cas.pem" and caused
that Apache server fail. But I am wrong because I created a new CA
instead of using the existing CA and its private key to create the new
So basically I changed the code as what you described, reuse the code
and put them in the right place. It looks more clean and consistent now.
Please see the updated one and let me know if it is on the right track.
Yay, this is like a hundred times better :D And you noticed yourself how
much cleaner this now looks and feels.
There are still a bunch of things we can do better, though:
In t_https.py (lines 269++) you are setting up files which need to be
set up every time someone wants to create certificates. So this should
be moved into the cert generator as some sort of initialization routine.
The current handling of the working directory is kinda ugly. For
instance, line 107 of pkg5unittest.py is only relevant for the
generate-certs.py script. Lines 110 should probably move into the class
But even after this you still have the problem of where the certs are
actually put into the filesystem. It might make sense to pass an
argument called base_dir or something to the functions. In any case,
your test case shouldn't need to create any directories by itself.
Same problem for the configuration file, openssl.cnf. You could just
assume that the cnf file needs to be in your base_dir or you can add
another argument for its location. You have to play around with this a
little to see what actually looks the best. It might turn out that
having the generator functions being static is not working out well and
it would be better if you set all the directories and cnf files up in
the init function of the class. But as I said you need to do a bit of
The goal here is to make creating certs in a test case or the script as
easy and straight forward as possible, even if the generator gets more
complicated. It's unlikely that the generator needs much work in the
future but there might be more test cases which need to generate their
own certs. So you want to make that easy.
The test case itself looks good. However, I don't see the need for
copying the ta around. Can you not just specify the location of the
original ta as parent_loc for make_cs_cert()?
Once you implement the suggestions above, you'll be surprised how short
your test case is gonna get. And that is what we want.