On 4/19/2013 2:32 PM, Ade Lee wrote: > > Added servlet to return 501 for rest operations for d9 instances > > D9 instances run on tomcat6, which does not have support for the > autheticator and realm. We are not supporting the REST operations > on D9 style instances. They will need to be migrated. > > The migration framework has been modified to process d9 or d10 > style instances, and a migration script has been added to add the new > servlet to existing d9 instances. Some comments (some of which have been discussed over IRC): 1. To be consistent the pki_subsystem and pki_instance classes should be called PKISubsystem and PKIInstance.

2. In PKIUpgrader's constructor the instance/subsystem objects are only created for validation then discarded. It might be better to move the validation inside the instances() or subsystems() where the objects are actually created and stored.

3. There seems to be a bug in these lines: inst = pki.pki_instance(instance, instance_type) ... subs = pki.pki_instance(instance, subsystem, instance_type) The second line probably should have been calling pki.pki_subsystem() and the parameter should have been inst (the instance object, not the name).

4. To improve clarity, the subsystem/instance name variable should be called 'name' inside the corresponding class, and 'subsystemName' or 'instanceName' outside the class.

5. The PKISubsystem constructor shouldn't need to take the type parameter because it can be obtained from the instance.

6. The message in RESTServlet could be simplified into something like this: The REST services are not available because this server is a legacy Dogtag 9 server. To access the REST services this server must be migrated into a new Dogtag 10 server.

7. Instead of overriding doGet() and doPost() it's also possible to just override service().

8. The --instance-type parameter works differently from the --instance and --subsystem parameters. By default pki-upgrade will upgrade all instances and subsystem on the system. If the --instance or --subsystem is specified, it will narrow the scope to the specified instance/subsystem only. The --instance-type, on the other hand, works as an additional parameter to --instance and has a default value of 10. Suppose there is a mix of Dogtag 9 and Dogtag 10 instances and some may have the same names (e.g. D9 pki-ca and D10 pki-ca). The parameters will work like this: pki-upgrade ==> upgrade all instances pki-upgrade --instance pki-ca ==> upgrade D10 pki-ca only pki-upgrade --instance pki-ca --instance-type 9 ==> upgrade D9 pki-ca only To be consistent it should work like this: pki-upgrade --instance pki-ca ==> upgrade both pki-ca instances pki-upgrade --instance pki-ca --instance-type 10 ==> upgrade D10 pki-ca only pki-upgrade --instance-type 9 ==> upgrade all D9 instances

9. We discussed about putting the context XML in <instance>/conf/Catalina/localhost instead of META-INF. Is this going to be handled by a separate upgrade script?

10. Document root could be obtained using self.doc.getroot() instead of self.doc.find('.').

11. Element index could be obtained using self.root.index(mapping) instead of list(self.root).index(mapping).

12. There are some trailing whitespaces.

Modified with changes based on comments in #irc. 1. PKIException now takes instance and subsystem objects instead of names. 2. Changed __repr__ for PKIInstance 3. Fixed get_tracker 4. Redefine con_dir in terms of base_dir Tested with d9 and d10 instances. All still looks good. On Mon, 2013-04-22 at 09:54 -0400, Ade Lee wrote: > On Fri, 2013-04-19 at 23:20 -0500, Endi Sukma Dewata wrote: > > On 4/19/2013 2:32 PM, Ade Lee wrote: > > > > > > Added servlet to return 501 for rest operations for d9 instances > > > > > > D9 instances run on tomcat6, which does not have support for the > > > autheticator and realm. We are not supporting the REST operations > > > on D9 style instances. They will need to be migrated. > > > > > > The migration framework has been modified to process d9 or d10 > > > style instances, and a migration script has been added to add the new > > > servlet to existing d9 instances. > > > > Some comments (some of which have been discussed over IRC): > > > > 1. To be consistent the pki_subsystem and pki_instance classes should be > > called PKISubsystem and PKIInstance. > > done > > > > 2. In PKIUpgrader's constructor the instance/subsystem objects are only > > created for validation then discarded. It might be better to move the > > validation inside the instances() or subsystems() where the objects are > > actually created and stored. > > > done - we now have a validate() method which is called by the > constructor. > > > 3. There seems to be a bug in these lines: > > > > inst = pki.pki_instance(instance, instance_type) > > ... > > subs = pki.pki_instance(instance, subsystem, instance_type) > > > > The second line probably should have been calling pki.pki_subsystem() > > and the parameter should have been inst (the instance object, not the name). > > > > Done. This is validation code that has been removed. > > > 4. To improve clarity, the subsystem/instance name variable should be > > called 'name' inside the corresponding class, and 'subsystemName' or > > 'instanceName' outside the class. > > > Done > > > 5. The PKISubsystem constructor shouldn't need to take the type > > parameter because it can be obtained from the instance. > > > Done > > > 6. The message in RESTServlet could be simplified into something like this: > > > > The REST services are not available because this server is a legacy > > Dogtag 9 server. To access the REST services this server must be > > migrated into a new Dogtag 10 server. > > > Done > > 7. Instead of overriding doGet() and doPost() it's also possible to just > > override service(). > > > Done > > > 8. The --instance-type parameter works differently from the --instance > > and --subsystem parameters. By default pki-upgrade will upgrade all > > instances and subsystem on the system. If the --instance or --subsystem > > is specified, it will narrow the scope to the specified > > instance/subsystem only. The --instance-type, on the other hand, works > > as an additional parameter to --instance and has a default value of 10. > > > > Suppose there is a mix of Dogtag 9 and Dogtag 10 instances and some may > > have the same names (e.g. D9 pki-ca and D10 pki-ca). The parameters will > > work like this: > > > > pki-upgrade ==> upgrade all instances > > > > pki-upgrade --instance pki-ca ==> upgrade D10 pki-ca only > > > > pki-upgrade --instance pki-ca --instance-type 9 > > ==> upgrade D9 pki-ca only > > > > To be consistent it should work like this: > > > > pki-upgrade --instance pki-ca ==> upgrade both pki-ca instances > > > > pki-upgrade --instance pki-ca --instance-type 10 > > ==> upgrade D10 pki-ca only > > > > pki-upgrade --instance-type 9 ==> upgrade all D9 instances > > > Done > > > 9. We discussed about putting the context XML in > > <instance>/conf/Catalina/localhost instead of META-INF. Is this going to > > be handled by a separate upgrade script? > > > To be handled as a separate upgrade script when we do the 10.1 > customization work. > > > 10. Document root could be obtained using self.doc.getroot() instead of > > self.doc.find('.'). > > > Done > > 11. Element index could be obtained using self.root.index(mapping) > > instead of list(self.root).index(mapping). > > > Done > > > 12. There are some trailing whitespaces. > > > Fixed _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel