On Mon, 2012-04-30 at 19:09 -0500, Endi Sukma Dewata wrote:
On 4/26/2012 2:49 PM, Ade Lee wrote:
Please review:
New RESTful servlet that does system configuration in a single servlet.
Installation code common to the panels and the installation servlet are extracted to a ConfigurationUtils file. The panel code will be cleaned up to use the code in this class in a later commit.
Contains restful client and test driver code. The test driver code should be modified and placed in a junit/system test framework. Installation has been tested to work with the following installations: master CA, clone CA, KRA, OCSP, TKS, subordinate CA, CA signed by external CA (parts 1 and 2).
Ticket #155
Thanks, Ade
Here's some initial feedback:
1. The ConfigurationErrorInterceptor is basically identical to the existing DRMErrorInterceptor. To avoid duplicating the code for each test components we can move the DRMErrorInterceptor from the base/kra/functional into the base/test and call it CMSErrorInterceptor (or something like that) which then can be used by all test components. Or we can put it into the common package so it can be used by any clients including the test tools. Ideally it should be put into a separate jar file that contains the client libraries only.
I agree with this completely. We will need to extract these interfaces and model classes and put them in a common package for use by both servers and clients. This includes putting all the common code (the interceptors and the common jss connection code). I suggest we check this code in - as well as Jack's code for the CA, and then create a task to do this consolidation. I'll create the task.
2. Classes such as ConfigurationTest and ConfigurationUtils are defined as containers of static functions. It might be possible to convert these static functions into object methods and the parameters can be passed as attributes, for example:
public class ConfigurationTest {
String host;
public void parse(...) { host = cmd.getOptionValue("h"); }
public static void main(String args[]) { ConfigurationTest test = new ConfigurationTest(); test.parse(args); ConfigurationData data = test.construct(); ... } }
We could certainly do that for ConfigurationTest - although I think we may want to consider changing ConfigurationTest to some sort of junit test. The purpose of ConfigurationTest was just to get something quick and dirty up to test the new servlet and provide a model for the pkicreate configure script. ConfigurationUtils is supposed to be used as a common set of functions for both the new servlet and the installation panels. In fact, basically what this patch does is extract (and clean up a bit) all the old logic in the installation panels and put them in a common place (ConfigurationUtils). My next set of patches will replace the logic in the existing panels to calls in ConfigurationUtils. You can't really keep objects around from panel to panel - so these really are static functions. And I'm not sure what that would buy you ..
3. The ConfigurationUtils.getDomainXML() returns an XML string, but before it's used it's always parsed into DOM. It might be better to modify this method to return DOM directly. Is the XML string equivalent to DomainInfo class? If that's the case the method can also return a DomainInfo directly.
In the panels, we will want to return the string for storage in CS.cfg. See next patch.
4. It might be possible to convert some fields in ConfigurationData such as dsPort or isClone into integer or boolean.
Yeah, I agree. I figured you would pick up on that. Let me open that as a separate task.
5. In InstallTokenRequest constructors the super() invocations aren't necessary.
Will remove.
6. The SystemConfigurationResourceService contains a static variable mRandom. The 'static' and the 'm' prefix don't seem to be necessary.
agreed - will fix.
7. The auto-format doesn't seem to be working. There are some trailing whitespaces too. Eclipse should have fixed it automatically.
Weird .. maybe the trailing whitespaces are in web.xml and other config files? I'll check it out ..