Please review the patch with the changes suggested by Endi to refactor code inorder to maintain references to the global variables and the utility classes. A new class PKIDeployer is added to pkihelper.py, whose object is created in pkispawn/pkidestroy to hold the references and be passed to the PKIScriptlets. Declarations for pki_master_dict and pki_slots_dict have been moved to init method in pkiparser.py from pkiconfig. The compose methods are called in pkispawn/pkidestroy and then the references passed to the PKIDeployer object. This also fixes many issues of tpe E1120 reported by pylint. --Abhishek On Wed, 2013-06-05 at 21:38 -0500, Endi Sukma Dewata wrote:

On 5/31/2013 3:38 PM, Abhishek Koneru wrote:

...

Please review the patch which fixes a few errors reported by pylint in dogtag's python code.

Also find attached the remaining errors to be fixed. Will submit a detailed report in my next mail.

How i used pylint:

-- Installed pki packages. -- Executed the following command - cd /usr/lib/python2.7/sitepackages; pylint -E --include-ids=y pki/ pki/deployment/ pki/server/ `which pkispawn` `which pkidestroy` `which pki-upgrade` `which pki-server-upgrade`

--Abhishek

Looking at the pkihelper.py again, I'm not sure if this is the right approach. In the original code the classes are instantiated into global objects and we're calling the methods of these objects like this:

uid = identity.get_uid()

In the new code the methods are converted into static methods:

uid = Identity.get_uid()

The problem is many of these methods actually depend on other global objects such as 'master', so we haven't really removed the dependency on global variables.

I'm thinking we probably should encapsulate the global objects into an object that we can pass around, then we access them as local variables. For example, in pkispawn we generate the master object and put it in a Deployer object. Then we pass the deployer object to the scriptlets.

deployer = pki.server.Deployer(master)

instance = scriptlet.PkiScriptlet() instance.spawn(deployer)

The Deployer class will have methods that generate the old global objects and we pass the master into those objects:

def get_identity(self): return Identity(master)

In the scriptlets we can call the methods this way:

identity = deployer.get_identity() uid = identity.get_uid()

What do you think?

If we do this, I'd suggest we do the conversion one global object at a time so it's easier to review.

Addressed all the comments given. Pushed to master. On Tue, 2013-06-25 at 18:40 -0500, Endi Sukma Dewata wrote:

On 6/24/2013 4:29 PM, Abhishek Koneru wrote:

...

Please review the patch with the changes suggested by Endi to refactor code inorder to maintain references to the global variables and the utility classes.

A new class PKIDeployer is added to pkihelper.py, whose object is created in pkispawn/pkidestroy to hold the references and be passed to the PKIScriptlets.

Declarations for pki_master_dict and pki_slots_dict have been moved to init method in pkiparser.py from pkiconfig. The compose methods are called in pkispawn/pkidestroy and then the references passed to the PKIDeployer object.

This also fixes many issues of tpe E1120 reported by pylint.

--Abhishek

Some comments:

1. The helper class names begin with underscore (e.g. _Identity). I don't think this is necessary. These classes will be used by scriptlet writers, so they aren't meant to be private classes.

2. Some of the constructors are defined like this:

def __init__(self, pki_master_dict, identity = None): self.master_dict = pki_master_dict if identity is None: self.identity = _Identity(pki_master_dict) else: self.identity = identity

Since these classes are always used with the deployer object, we can simplify it like this:

def __init__(self, deployer): self.master_dict = deployer.pki_master_dict self.identity = deployer.identity

Using the deployer guarantees that the dicts and helper objects are already created, so there's no need to create the objects again.

3. There are some trailing whitespaces.

Everything else is fine, the code works. With the above issues fixed, ACK.

As future enhancement, it's possible to simplify the scriptlets further. For example:

deployer.file.copy_with_slot_substitution( deployer.master_dict['pki_source_catalina_properties'], deployer.master_dict['pki_target_catalina_properties'], overwrite_flag=True)

If the method is always used with values from the master_dict, the above line can be simplified as follows:

deployer.file.copy_with_slot_substitution( 'pki_source_catalina_properties', 'pki_target_catalina_properties', overwrite_flag=True)