On 07/14/2014 11:45 AM, Ade Lee wrote:
Hi all,

I have rebased all the previous patches against master, and have squashed them 
all into a single patch.
Its a large patch, but as many folks have already reviewed the constituent 
precursor patches, most if it
should be familiar and easier to review.

I think it would be nice to have the fixes that aren't related to DRM (e.g. style issues) separate, so the patch can be reverted easily. But, what's done is done. Thanks for the cleanup!

The main difference with what was specified before is that the DRM database is 
installed as a subtree
to o=ipaca.  This means that no new replication agreements will be needed to 
replicate DRM data.
Replication agreements set up for the Dogtag CA will automatically replicate 
DRM data.

In order for this patch to work, a new 10.2 build of Dogtag 10.2 is needed - 
with specific changes to
allow the ability to install a database as a subtree of an existing tree.  At 
this time, these
changes have not yet been checked into the dogtag source.   You can obtain such 
a build from:


Please review,

I've started reading the code; here are my thoughts on the first part:

In ipa-ca-install and ipa-replica-install, you only ever set "REPLICA_INFO_TOP_DIR" to None. In Python, "global" has module scope, it's not for the entire process. (And it's bad practice anyway.)
You'll need to pass it around explicitly, perhaps store it in ReplicaConfig.

It would be nice to use the admintool framework for the new script, instead of copying the old boilerplate code again. See e.g. ipa-server-certinstall for an example.

Logging functions interpolate their arguments, so you should use e.g.
root_logger.critical("failed to configure %s instance %s", subsystem, e)
instead of using the % operator.

installutils.create_replica_config: Any time you call sys.exit(), please log a message so we know why the log file ends.

In ipa-drm-install, why do you read /etc/ipa/default.conf manually? The information is available in api.env once api is finalized.

In ipa-server-install, do we want to display the message about backing up /root/drmcert.p12 even if DRM is not installed?

Please use path constants from ipaplatform.base.paths, or add new ones if necessary, instead of DogtagInstance.{AGENT_P12_PATH,AGENT_P12_PATH}. In ipa-upgradeconfig, you hardcoded '/etc/named.keytab'. Please change that back to paths.NAMED_KEYTAB.
Same in CAInstance.uninstall with "/usr/bin/pkiremove".

In ipa-upgradeconfig.find_subject_base, the docstring should mention to DsInstance.find_subject_base's docstring rather than being a copy; we dont' want them getting out of sync.

In DsInstance.find_subject_base, don't use a bare `except:`, at least do `except Exception:`, so you don't e.g. disable Ctrl+C.

In CADSInstance.uninstall, you don't need the _running and _user_exists variables at all, just call the functions.
Same in CAInstance.__create_ra_agent_db for _stdout, _stderr, _returncode.
Same in CAInstance.uninstall for _user_exists.

In DogtagInstance, stop_tracking_certificates was moved here from cainstance, but it no longer stops tracking ipaCert in HTTPD_ALIAS_DIR. Is that intended?

In CAInstance.__init__, when calling DogtagInstance.__init__, consider using super, and naming the arguments for clarity:
    super(CAInstance, self).__init__(
        service_desc="certificate server",

Since CAInstance inherits from DogtagInstance, the `enable`, `start_instance`, `stop_instance`, `http_proxy` methods that just call the superclass are unnecessary. IMO the comment from CAInstance.__enable was important, it should be in DogtagInstance.enable.

In DogtagInstance.spawn_instance, it's not nice to modify lists the caller pased in. Take a copy of the nolog first. Ideally combine that with the conversion to tuple, and allow any sequence to be passed in: nolog_list = tuple(nolog_list) + (self.admin_password, self.dm_password)

In CAInstance.__create_ca_user, it's better to use functions instead of staticmethods, unless you'll want to override them in subclasses (which you're explicitly discouraging here, by using the double underscore).
Anyway I don't get why you want these to be staticmethods.

(Note to self: I'm at line 1866 of the patch)


Freeipa-devel mailing list

Reply via email to