[Freeipa-devel] [PATCH] 814-818 migrate-ds: optimize adding users to default group

[Martin Basti]

On 10/04/15 12:55, Petr Vobornik wrote:
> The essential patch is 814.
>
> 815 a proposal for new option.
>
> 816 and 818 are cleanup patches.
>
> 817 little optimization.
>
> == [PATCH] 814 migrate-ds: optimize adding users to default group ==
> Migrate-ds searches for user without a group and adds them to default 
> group. There is no point in checking if the user's selected by 
> previous query are not member of default group because they are not 
> member of any group.
>
> The operation is also speeded up by not fetching the default group. 
> Users are added right away.
>
> https://fedorahosted.org/freeipa/ticket/4950
>
NACK

Users haven't been added into ipa default group after migration.

Just nitpick

1) too many parentheses
             api.log.error(('Adding new members to default group failed: 
%s \n'
                            'members: %s') % (e, (','.join(member_dns))))
You can use this instead:
             api.log.error('Adding new members to default group failed: 
%s \n'
                           'members: %s', e, ','.join(member_dns))

== [PATCH] 815 migrate-ds: skip default group options ==
> New option --use-default-group=False could be used to disable adding of
> migrated users into default group.
>
> By default, the default group is no longer POSIX therefore it doesn't
> fulfill the original idea of providing GID and therefore it could be
> skipped during migration.
LGTM
>
> == [PATCH] 816 migrate-ds: remove unused def_group_gid context 
> property ==
> it's no longer used anywhere
>
1)
You can remove the unused variable 'g_attrs'
> == [PATCH] migrate-ds: optimize gid checks by utilizing dictionary 
> nature of set ==
>
LGTM
> == [PATCH] migrate-ds: log migrated group members only on debug level ==
> It pollutes error_log.
>
1)
you do not need % formatting in logger
api.log.debug('migrating %s group %s' , member_attr, m)
>
>

Martin^2

-- 
Martin Basti

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150430/caca153d/attachment.htm>