[Freeipa-devel] [PATCH 0135] upgrade: unconditional import of certificate profiles into LDAP

[Fraser Tweedale]

On Tue, Feb 23, 2016 at 07:32:31AM +0100, Jan Cholasta wrote:
> On 23.2.2016 06:40, Fraser Tweedale wrote:
> >On Mon, Feb 22, 2016 at 02:03:49PM +0100, Martin Babinsky wrote:
> >>https://fedorahosted.org/freeipa/ticket/5682
> >>
> >>--
> >>Martin^3 Babinsky
> >>
> >Thanks for the patch.  Conditional ACK.
> >
> >Patch is tested and works, but I am wary about checking for
> >substring match against RemoteRetrieveError reason string (see hunk
> >below).  It would be better to carry the status code as an attribute
> >of RemoteRetrieveError and check whether it is 409.
> >
> >>+        except errors.RemoteRetrieveError as e:
> >>+            if "Profile already exists" in e.reason:
> >>+                root_logger.debug("Profile '{}' already present".format(
> >>+                    profile_id))
> >>+            else:
> >>+                root_logger.error("Error migrating profile '{}': {}".format(
> >>+                    profile_id, e))
> >>+                return
> >
> >If you agree, we can file a ticket and I am happy for these patches
> >to be merged as-is.  The scope of changing RemoteRetrieveError is
> >larger than #5682 so it makes sense to do it separately, and just
> >for master branch.
> 
> I don't think this is the right approach. create_profile() should raise
> DuplicateEntry rather than RemoteRetrieveError if the profile already
> exists, which can then be properly handled in _create_dogtag_profile().
> 
I am happy with that.