[Freeipa-devel] [PATCHES 0001-0005] Profile management commands

[Fraser Tweedale]

On Mon, May 04, 2015 at 06:35:45PM +0200, Martin Basti wrote:
> On 04/05/15 15:36, Fraser Tweedale wrote:
> >Hello,
> >
> >Please review the first cut of the 'certprofile' command and other
> >changes associated with the Certificate Profiles feature[1].
> >
> >Custom profiles can't be used yet because 'cert-request' has not
> >been updated, but you can manage the profiles (find, show, import,
> >modify, delete).  There's a bit more work to do on profile
> >management and a lot more to do for using profiles and sub-CAs.  I
> >am tracking my progress on etherpad[2] so if you are reviewing check
> >there for the TODO list and some commentary.
> >
> >If you want to test: for f21, please use Dogtag from my copr[2].
> >For f22 the required version is in updates-testing (or my copr).
> >
> >In summary: this is not the whole feature, just the first functional
> >part.  Since it is my first experience developing in the IPA
> >framework I want to get patches out so you can point out all the
> >things I did wrong or overlooked, and I can fix them.  Don't hold
> >back :)
> >
> >[1] http://www.freeipa.org/page/V4/Certificate_Profiles
> >[2] http://idm.etherpad.corp.redhat.com/rhel72-cert-mgmt-progress
> >[3] http://copr.fedoraproject.org/coprs/ftweedal/freeipa/
> >
> >
> Thank you for patches, I have no idea what kind of dogtag magic is happening
> there, but I have a few comments related to IPA:
> 
Thanks for reviewing, Martin.  Comments inline.

> Upgrade:
> 
> 1)
> 
> +        config.set("CA", "pki_profiles_in_ldap", "True")
> 
> IMO this will work only for new installations. For upgrade you may need to
> add this to ipa-upgradeconfig
> 
OK.

> 2)
> +dn: cn=certprofiles,cn=etc,$SUFFIX
> +changetype: add
> +objectClass: nsContainer
> +objectClass: top
> +cn: certprofiles
> 
> IMO this will work only for new installations. For upgrade you may need to
> add it into update file as well, with the 'default' keyword
> 
I don't understand about the 'default' keyword - can you expain this
some more?

> 3)
> Your patch 0004 will work on new installations only. You may need to add
> that new step into ipa-upgradeconfig.
> 
> Must be that step there during installation?
> If not you can create just one update file, which will be applied at the end
> of installation and during upgrade.
> 
This change must be made to the Dogtag directory (not IPA) - can an
update file be used to do that?  If not, is ipa-upgradeconfig the
best place to make this change?

> Other issues:
> 1)
> I do not see modifications in API.txt file
> 
> 2)
> We use new shorter license header
> #
> # Copyright (C) 2015  FreeIPA Contributors see COPYING for license
> #
> 
> 3)
> +from ipalib.plugins.baseldap import \
> +    LDAPObject, LDAPSearch, LDAPCreate, LDAPDelete, LDAPUpdate,
> LDAPRetrieve
> 
> please use 'import ( modules, .. )' instead of '\'
> 
> 4)
> +    if method == 'POST' \
> +            and 'content-type' not in (str(k).lower() for k in
> headers.viewkeys()):
> 
> again, please use if ( ... ): instead \
> 
> 5)
> +import  ipalib.errors as errors
> in dogtag.py
> 
> Can you use from ipalib import errors, instead?
> 
> 6)
> +    def __exit__(self, exc_type, exc_value, traceback):
> +        """Log out of the REST API"""
> +        # TODO logout
> +        self.cookie = None
> 
> This is forgotten, or will be this fixed later?
> 
> 7)
> +            if not explanation: print resp_body  # NOCOMMIT
> 
These are all fixed for the next patchset.

Thanks!
Fraser

> Martin^2
> 
> -- 
> Martin Basti
>