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

--
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