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

[Tomas Babej]

On 07/01/2015 07:32 PM, Martin Babinsky wrote:
> 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.
> 

Pushed to master: 8d30feb5391026a42a2f8da5df8d539311963b86