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

[Martin Basti]

On 05/05/15 10:59, Petr Vobornik wrote:
> On 04/30/2015 03:53 PM, Martin Basti wrote:
>> 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.
>
> Fixed in patch 815.
>
>>
>> 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))
>
> Fixed.
>
>>
>> == [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
>
> the option got so the default would be applied.
> +            autofill=True,
>
>
>>>
>>> == [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'
>
> removed
>
>>> == [PATCH] 817 migrate-ds: optimize gid checks by utilizing dictionary
>>> nature of set ==
>>>
>> LGTM
>>> == [PATCH] 818 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)
>
> fixed and also changed "migrating %s user %s'" line to debug, which 
> was the actual reason for this patch.
>
>>>
>>>
>>
>> Martin^2
>>
>
>
Thanks.

1)
Please create new API file, probably missing autofill in API.txt:

Option 'use_def_group?' in command 'migrate_ds' in API file not found
Options count in migrate_ds of 18 doesn't match expected: 19
Option use_def_group? of command migrate_ds in ipalib, not in API file:
Bool('use_def_group?', autofill=True, cli_name='use_default_group', 
default=True)

There are one or more changes to the API.
Either undo the API changes or update API.txt and increment the major 
version in VERSION.

2)
ipa: error: --use-default-group option does not take a value

and a nitpick:

patch 814
1)
Why this change?

-        api.log.debug('Adding %d users to group%s duration %s',
-                len(new_members), mode, d)
+        api.log.info('Adding %d users to group%s duration %s',
+                      len(member_dns), mode, d)

-- 
Martin Basti