On 07/16/2014 02:55 PM, Petr Viktorin wrote:
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.

Also, in e.g.
    root_logger.debug(
        'Unable to determine PIN for the Dogtag instance: %s' % str(e))
the "s" in "%s" means "convert to string", so explicit str() isn't necessary.

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,
     )
Same in DogtagInstance, DRMInstance.

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.


dogtaginstance.py, drminstance.py: Avoid star imports, especially in new code. AFAICS you're only using root_logger from ipapython.ipa_log_manager.
Also consider using an class-specific logger:
    class DogtagInstance(...):
        def __init__(...):
            ...
            self.log = ipa_log_manager.log_mgr.get_logger(self)

        later:
self.log.error("Unable to determine PIN for the Dogtag instance:", e)

About the "# noinspection PyBroadException" comments -- we already use pylint, I don't think we want instructions for other linters in the code.

In DogtagInstance.get_admin_cert:
    admin_cert = entry_attrs.get('usercertificate')[0]
This relies on the order of values returned by LDAP. If the choice of certificate doesn't matter here, leave a comment.

drminstance.py: Please remove the `if __name__ == '__main__':` section at the end. If this needs to be tested, provide a real test; if it's example usage put it in the docs/docstring; if it's a personal tool just keep it to yourself.

In DRMInstance.__init__, the docstring is wrong; __init__ doesn't return anything.

In dogtag.py, this change breaks the formatting:
-    |staus                    |string         |cert_request_status|unicode 
[1]_     |
+    |status                    |string         |cert_request_status|unicode 
[1]_     |

In drm.__init__, use `os.path.join(api.env.dot_ipa, 'alias')` instead of joining manually. There are also hardcoded paths, put them in the platform paths module.

--
PetrĀ³

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to