[Freeipa-devel] [PATCHES 306-316] Automated migration tool from Winsync

Martin Babinsky mbabinsk at redhat.com
Wed Jul 1 17:32:51 UTC 2015


On 06/30/2015 05:55 PM, Tomas Babej wrote:
>
>
> On 06/16/2015 01:01 PM, Jan Cholasta wrote:
>> Dne 16.6.2015 v 10:14 Martin Babinsky napsal(a):
>>> On 05/06/2015 10:12 AM, Tomas Babej wrote:
>>>>
>>>>
>>>> On 05/05/2015 02:02 PM, Tomas Babej wrote:
>>>>>
>>>>>
>>>>> On 04/29/2015 12:28 PM, Tomas Babej wrote:
>>>>>>
>>>>>>
>>>>>> On 03/11/2015 04:20 PM, Jan Cholasta wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Dne 10.3.2015 v 16:35 Tomas Babej napsal(a):
>>>>>>>>
>>>>>>>> On 03/09/2015 12:26 PM, Tomas Babej wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> this couple of patches provides a initial implementation of the
>>>>>>>>> winsync migration tool:
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4524
>>>>>>>>>
>>>>>>>>> Some parts could use some polishing, but this is a sound
>>>>>>>>> foundation.
>>>>>>>>>
>>>>>>>>> Tomas
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Attaching one more patch to the bundle. This one should make the
>>>>>>>> winsync
>>>>>>>> tool readily available after install.
>>>>>>>>
>>>>>>>> Tomas
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> Nitpicks:
>>>>>>>
>>>>>>> The winsync_migrate module should be in ipaserver.install. Also I
>>>>>>> don't see why it has to be a package when there is just one short
>>>>>>> file in it.
>>>>>>>
>>>>>>> By convention, the AdminTool subclass should be named
>>>>>>> WinsyncMigrate, or the tool should be named ipa-migrate-winsync.
>>>>>>>
>>>>>>> Honza
>>>>>>>
>>>>>>
>>>>>> Updated patches attached.
>>>>>>
>>>>>> Tomas
>>>>>
>>>>> Rebased patches with cleaned membership bits.
>>>>>
>>>>> Tomas
>>>>
>>>> I did some self-review, updated patches attached.
>>>>
>>>>
>>> Hi Tomas,
>>>
>>> patches look good and seem to work as expected. I have some comments:
>>>
>>> 1.) When running the tool I get a number of warnings about users not
>>> found (https://paste.fedoraproject.org/232251/43884831/), but in the end
>>> everything seems to be fine and users are migrated in the external
>>> groups just fine. Is this behavior normal?
>>>
>
> In that case, yes. What happened here is that SSSD in POSIX trust will
> not resolve users that do not have POSIX attributes set. Winsync
> synchornizes all the users, hence the discrepancy.
>
>
>>> 2.) Since both "--realm" and "--server" options are mandatory, I was
>>> thinking if it would be better to use positional arguments, since you
>>> always have to specify them. What are your thought on this?
>>
>> I would rather stay consistent with ipa-server-install and friends and
>> keep them as options.
>>
>>>
>>> 3.) Patches 317-318 seem to just just rename/move things and could be
>>> squashed in the previous ones. But that is just a minor thing and I
>>> leave that to your discretion.
>>>
>>> 4.) After all the renaming and moving around the WinsyncMigrate class
>>> (see previous point) there is an unused file
>>> "ipaserver/winsync_migrate/__init__.py" left. You should remove it in
>>> some patch (e.g. in patch 318 if you decide to keep it).
>
> I removed the file and squashed the change into 318.
>
>>
>> Also please rename the class to "MigrateWinsync", for consistency.
>>
>
> Naming is consistent, the tool is called ipa-winsync-migrate, class is
> called WinsyncMigrate. This is consistent with other IPA tools.
>
>
>>>
>>> 5.) Option "--log-file" seems to be broken. When specified on CLI the
>>> log is created but empty, the program prints out nothing and then exits
>>> without doing anything. However, I suspect that this is AdminTool's
>>> problem, not yours.
>>>
>
> Yep. Please, file a ticket for this more generic issue.
>

Will do.

Otherwise ACK.

-- 
Martin^3 Babinsky




More information about the Freeipa-devel mailing list