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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to