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 > -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code