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

Thu May 7 11:17:39 UTC 2015

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.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0818-2-migrate-ds-log-migrated-group-members-only-on-debug-.patch
Type: text/x-patch
Size: 1397 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0817-2-migrate-ds-optimize-gid-checks-by-utilizing-dictiona.patch
Type: text/x-patch
Size: 1877 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0816-2-migrate-ds-remove-unused-def_group_gid-context-prope.patch
Type: text/x-patch
Size: 1338 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0815-2-migrate-ds-skip-default-group-option.patch
Type: text/x-patch
Size: 5201 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0814-2-migrate-ds-optimize-adding-users-to-default-group.patch
Type: text/x-patch
Size: 2988 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvoborni-0826-cli-differentiate-Flag-a-Bool-when-autofill-is-set.patch
Type: text/x-patch
Size: 1629 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150507/984f326b/attachment-0005.bin>