2:49 p.m.
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
7:09 p.m.
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.
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();
...
}
}
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.
4. It might be possible to convert some fields in ConfigurationData such
as dsPort or isClone into integer or boolean.
5. In InstallTokenRequest constructors the super() invocations aren't
necessary.
6. The SystemConfigurationResourceService contains a static variable
mRandom. The 'static' and the 'm' prefix don't seem to be necessary.
7. The auto-format doesn't seem to be working. There are some trailing
whitespaces too. Eclipse should have fixed it automatically.
--
Endi S. Dewata
10:57 a.m.
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 ..
4:24 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
4617
days inactive
4622
days old
3 comments
2 participants
participants (2)
-
Ade Lee -
Endi Sukma Dewata