On Thu, 2011-07-14 at 23:05 +0000, JR Aquino wrote: > On Jul 14, 2011, at 11:55 AM, wrote: > > > https://fedorahosted.org/freeipa/ticket/1272 > > > > * Added new container in etc to hold the automembership configs. > > * Modified constants to point to the new container > > * Modified dsinstance to create the container > > * Modified hostgroup.py to add the new commands > > * Added xmlrpc test to verify functionality > > Minor adjustment: > Auto Membership Plugin isn't available until 1.2.9-0.2+ > > Modified freeipa.spec.in: > BuildRequires: 389-ds-base-devel >= 1.2.9-0.2
I have reviewed your patch. Basic functionality is OK but I have some concerns. 1) I am not sure with the command name, it is not really clear to me what this command does. But I know from my experience that inventing a cool name for something new may be the most difficult task at all :-) Maybe command name "hostgrouprule" or "hostgroupauto" would be more clear? 2) Overloading execute method in functions hostgroupclarity_add_condition and hostgroupclarity_remove_condition is an over-kill for me. I think we could just read current inclusive/exclusive regexes in pre_callback, modify them and let LDAPUpdate class do the standard LDAP operations. 3) I miss hostgroupclarity-mod module. What would I do if I want to update Description? 4) I didn't like this construct in the code, its error prone to potential future parameter changes. + if len(options) == 2: # 'all' and 'raw' are always sent + raise errors.EmptyModlist() I know it's in baseldap.py but I still wouldn't like to see this in plugins. 5) Test test_clarityrule_plugin.py: reference to inexistent python module: +Test the `ipalib/plugins/clarityrule.py` module. Then I did some real testing of the new command: 6) Invalid examples, fqdn is not supposed to be a part of regex $ ipa hostgroupclarity-add --inclusive-hostname-regex=fqdn=^www[1-9]+\.example\.com webservers Hostgroup Clarity Rule: webservers Inclusive Regex: fqdn=fqdn=^www[1-9]+.example.com 7) It does not make sense to have a rule with only an exclusive regex: $ ipa hostgroupclarity-add --exclusive-hostname-regex=^www5+\.example\.com webservers Hostgroup Clarity Rule: webservers $ ipa host-add --force foo.example.co $ ipa hostgroup-show webservers Host-group: webservers Description: Web Servers Member hosts: www1.example.com I think we should 1) hide exclusive regex option in hostgroupclarity-add and 2) check that there is at least one inclusive regex in the rule when running hostgroupclarity-add-condition and hostgroupclarity-remove-condition. 8) Plugin incorrectly handles a situation when both inclusive and exclusive regex-es are being added: $ ipa hostgroupclarity-add --inclusive-hostname-regex=^www[1-9]+\.example\.com webservers Hostgroup Clarity Rule: webservers Inclusive Regex: fqdn=^www[1-9]+.example.com $ ipa hostgroupclarity-add-condition --inclusive-hostname-regex=^web[1-9]+\.example\.com --exclusive-hostname-regex=www5\.example\.com webservers Inclusive Regex: fqdn=^web[1-9]+.example.com, fqdn=^www[1-9]+.example.com Exclusive Regex: www5.example.com Exclusive regex misses fqdn. 9) Removing multiple conditions also works incorrectly: $ ipa hostgroupclarity-show webservers Hostgroup Clarity Rule: webservers Inclusive Regex: fqdn=^www[1-9]+.example.com, fqdn=^web[1-9]+.example.com Exclusive Regex: fqdn=www5.example.com $ ipa hostgroupclarity-remove-condition webservers --inclusive-hostname-regex=^www[1-9]+\.example\.com --exclusive-hostname-regex=www5\.example\.com Inclusive Regex: fqdn=^web[1-9]+.example.com $ ipa hostgroupclarity-show webservers Hostgroup Clarity Rule: webservers Inclusive Regex: fqdn=^web[1-9]+.example.com Exclusive Regex: fqdn=www5.example.com 10) When removing nonexistent regex I would expect more explaining error message: $ ipa hostgroupclarity-show webservers Hostgroup Clarity Rule: webservers Inclusive Regex: fqdn=^web[1-9]+.example.com Exclusive Regex: fqdn=www5.example.com $ ipa hostgroupclarity-remove-condition webservers --inclusive-hostname-regex=foo ipa: ERROR: no modifications to be performed Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel