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

[Jan Cholasta]

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?
>
> 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).

Also please rename the class to "MigrateWinsync", for consistency.

>
> 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.
>


-- 
Jan Cholasta