On Thu, 04 Oct 2012, Sumit Bose wrote:
On Thu, Oct 04, 2012 at 12:13:57PM +0300, Alexander Bokovoy wrote:
On Thu, 04 Oct 2012, Sumit Bose wrote:
>Hi,
>
>this patch tries to avoid the ldapmodiy error messages during
>ipa-adtrust-install by checking if the related object already exists.
>Fixes https://fedorahosted.org/freeipa/ticket/3012 .
In general -- ACK for the approach. However, I wonder if you could create
a helper method that would accept:
  - ldif file name,
  - cn component
  - name of the plugin for the "already configured" message

Then every __add_* method would call simply the helper with appropriate
arguments.

yes, I was thinking about the same, but I preferred this solution for
now instead of refactoring the current code. Additionally it would be
nice if the helper method would read the DN from the ldif file to make
the code more robust. Would you mind to open a trac ticket to refactor
this part of the code?
I would disagree here. Reading DN from ldif file adds complexity for no
real gain.

Refactoring this code is really simple -- your patch did already
introduced bigger change than that refactoring would make.

If you aren't comfort with this suggestion, I can make the change
myself. Having these repeated patterns is hurting my eyes :)

--
/ Alexander Bokovoy

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

Reply via email to