[Pki-devel] PATCH 0029 - RESTful servlet to configure system in a single servlet

[Ade Lee]

pushed to master.  Thanks!

On Tue, 2012-05-01 at 11:57 -0400, Ade Lee wrote:
> 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 ..
> 
> 
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel