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:
http://copr.fedoraproject.org/coprs/vakwetu/dogtag/build/21936/
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__(
realm=realm,
subsystem="CA",
service_desc="certificate server",
dogtag_constants=dogtag_constants,
)
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)
--
Petr³
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
