See comments below:
On Wed, 2013-03-20 at 10:19 -0500, Endi Sukma Dewata wrote:
On 3/19/2013 3:54 PM, Ade Lee wrote:
> This is a pretty big change, but we want to get it into 10.0.2 so that
> we can eliminate our dependency on jython.
>
> So far, its been tested against a straight CA install. I plan to
> continue testing against other configurations, but as the code change is
> quite large, I want to start the review early.
>
> Please review,
> Ade
Some comments:
1. Right now the encoder.py has to import other PKI modules in order to
construct the TYPES and NOTYPES lists, so if new modules are added we'd
need to update the encoder.py. This is not ideal since encoder.py is a
common module. It might be better to let the modules register themselves
into those lists, for example in encoder.py we can do something like this:
import pki.encoder
class ConfigurationRequest:
...
encoder.TYPES['ConfigurationRequest'] = ConfigurationRequest
Fixed in 124.
2. The SystemCertData is now in NOTYPES list. What if we want to send
a
SystemCertData object in a future API? It will need to be in TYPES.
Maybe instead of using NOTYPES we should check if the object is an array
and the elements are in TYPES then we return an array of encoded objects.
Due to time constraints, we will not fix this in this patch. Please
open a ticket.
Same as above. Please open a ticket.
4. The ConfigurationResponse.getSystemCerts() should return a List
instead of a Collection.
Fixed in 124.
5. File access can be written with the 'with' keyword:
with open(...) as f:
data = f.read()
Fixed in 124 for the code added in this patch. There are, however, many
other occurences in the python code that should be addressed. Please
open a ticket to address these.
6. Sometimes OCSP or TKS installation failed:
pkispawn : INFO ....... constructing PKI configuration data.
pkispawn : INFO ....... configuring PKI configuration data.
pkispawn : ERROR ....... Exception from Java Configuration
Servlet: [Errno 111] Connection refused
Maybe Tomcat is too slow to start? Sometimes it works just fine.
Fixed in 123.