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

[Martin Basti]

On 07/05/15 13:17, Petr Vobornik wrote:
> On 05/06/2015 04:49 PM, Martin Basti wrote:
>> 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.
>
> you could just wrote that I forgot to run ./makeapi ;)
>
>>
>> 2)
>> ipa: error: --use-default-group option does not take a value
>
> Attached new patch #826 which should fix the issue. Not sure if 
> Honza(CCd) will like it.
>
> "(default: true)" added to option description for better UX.
>
>>
>> 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)
>
> So that there will be a record in a default(not debug) log that 
> something happened. The reason is that it also affects users, without 
> a group, that are already present on the system.
>
As we personally talked with Honza, and he agreed with param 
modification, then
ACK for all patches.

-- 
Martin Basti