<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 19.05.2016 16:59, Martin Babinsky wrote: </div> <blockquote cite="mid:28b45db2-8116-359c-d28f-90201fa16271@redhat.com" type="cite">Patch 0146 implements lower-lever infrastructure for querying server roles/attributes Patch 0147 are some basic tests slapped together for the `serverroles` backend to ensure that it works as expected. The new/modified CLI commands specified in the design page [1] will be coming soon. Also coming soon will be an interface to construct DNS records for each role requested by Petr^2 and Martin^2 which I will start implement when we agree on the backend implementation. <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/5181">https://fedorahosted.org/freeipa/ticket/5181</a> [1] <a class="moz-txt-link-freetext" href="https://www.freeipa.org/page/V4/Server_Roles#CLI">https://www.freeipa.org/page/V4/Server_Roles#CLI</a> <fieldset class="mimeAttachmentHeader"></fieldset> </blockquote> Hello, I have a few comments 0) I'm afraid that your docstrings contains docstest syntax and it may fail 1) IIRC the factory should return just one particular object, not dict of objects, should be these function called differently? +def role_factory(api_instance): + """ + return a dict of server role instances keyed by lower-cased role name + """ + + return {key: role_cls(api_instance) for key, role_cls in + role_registry.items()} + + +def attribute_factory(api_instance): + """ + return a dict of server attribute instances keyed by lower-cased attribute + name + """ + return {key: attr_cls(api_instance) for key, attr_cls in + attribute_registry.items()} 2) +class LDAPBasedProperty(object): .... + def __init__(self, api_instance): + self.api = api_instance + self.ldap_conn = self.api.Backend.ldap2 self.ldap_conn, this may bite us later, ldap2 may not exist when object is created an thus this may cause the AttributeErrors. IMO in method there should be used directly ldap = self.api.Backend.ldap2 ldap.search() Otherwise you will not be able to create objects without connected LDAP (just future thinking) we had simillar issue in DNS that in upgrade code it was failing 3) +class BaseServerAttribute(LDAPBasedProperty): ... + # pylint: disable=not-callable + self.role_instance = self.associated_role( + api_instance) + # pylint: enable=not-callable I don't like this, twice; don't disable pylint, do some check in constructor if associated_role was defined Something like if not isinstance(self.associated_role, <BaseServerRole>): raise TypeError(Define role) Or put as default ABCMetaclass (BaseServerRole), that should blow up 4) ipa_config_string_value = None IMO this should be empty list (in the same class) 5) <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;">associated_service_name = None IMO there should be check in constructor, that this variable is defined with something else than None, to avoid hard to not ot nice to debug errors 6) @property def providers(self): """ :returns: list of masters providing the attribute. In the case of singular attributes returns a list containing single object """ try: entries = self.search(self.search_base) except errors.EmptyResult: return set() I'm afraid this may break sometimes, because what exception is raised depends on implementation of search(), so IMO would be better to catch exception in search() and return set() from search method, or define new exception which should be used for this case and document it in search() method </pre> 7) <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;">class ServiceBasedRole(BaseServerRole): ... </pre> enabled = all( <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;"> value in entry.get(attribute, []) for (attribute, value) in self.enabled_attrs_value.items() ) Here ipaConfigString is case insensitive, so I'm afraid that this may not work in cases that there is value 'enabledservice' instead of 'enabledService' But if you are sure that this cannot happen in IPA, we can leave it as is, otherwise you have to distinguish case sensitive and case insensitive attrs IMO in this case generalization may cause more harm than gain. IMO should be better to create extra method (maybe abstract) which will return state if service is enabled or not in LDAP. Or create default implementation that compares just ipaConfigScript and subclasses should call it via super() and add it owns check by super 8) </pre> <meta http-equiv="content-type" content="text/html; charset=windows-1252"> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;">class ServiceBasedRole(BaseServerRole): ... <meta http-equiv="content-type" content="text/html; charset=windows-1252">try: entries = self.search(search_base) services = [self._get_service(e) for e in entries] self._validate_component_services(services) return (ENABLED if self._are_services_enabled(services) else CONFIGURED) except errors.EmptyResult: return ABSENT Same as 6). IMO you can put there else statement as part of first try-except block 9) class ServiceBasedRole(BaseServerRole): ... @property def providers(self): There is no except catching EmptyResults (not needed if you reimplement it as I suggested above) 10) Nitpick alert <meta http-equiv="content-type" content="text/html; charset=windows-1252">for master in services_by_master: services = services_by_master[master] for master, services in services_by_master.items() (or using six) is IMO better 11) providers = [] for master in services_by_master: services = services_by_master[master] self._validate_component_services(services) if all(s.enabled for s in services): providers.append(master) I was quite puzzled with this code until I realized that you are actually finding just subset of services that belongs to a role, not all services on particular master, maybe comment may help or variable naming also please use _are_services_enabled() instead of copy-paste 12) def obj_pkey_from_hostname This is used just once and it just return param as result without any change, do you have any future plans with it? If not then please remove it. 13) ************* Module ipaserver.plugins.serverroles ipaserver/plugins/serverroles.py:579: [W0223(abstract-method), ADTrustBasedRole] Method 'fqdn_from_entry' is abstract in class 'MembershipBasedRole' but is not overridden) IMO there is unneeded inheritance, keep number of levels as low as possible, IMO ADTrustAgent can inherit directly from MembershipBasedRole and implement group_dn property, because ADTrustBasedRole is used just once. 14) I'm not sure if self.group_dn express the right thing, because <meta http-equiv="content-type" content="text/html; charset=windows-1252">class ADTrustBasedRole(MembershipBasedRole): @property def group_dn(self): return DN( ('cn', 'adtrust agents'), ('cn', 'sysaccounts'), ('cn', 'etc'), self.api.env.basedn) It is account not group, I'm not sure what get_dn should express 15) I'm not a big fan of MembershipBasedRole class, if this circus is done just because ADTrustAgent service is nonstandard, I would rather have single implementation of that magic instead of having baseclass that is never used for anything else and wasting time with implementation of a general solution. I don't like that in current implementation you mixed show commands, direct ldap searches, for the same thing, and also using 'api.env.container_*' does not look safe for me as we do not enforce the exact format and it may break in future or with different objects. 16) <meta http-equiv="content-type" content="text/html; charset=windows-1252">class serverroles(Backend): Please don't do any lower() operation with options here, it should be just obj_type="role" <meta http-equiv="content-type" content="text/html; charset=windows-1252">def _get_obj_store(self, obj_type="Role"): if obj_type.lower() == "role": this is internal API so in case that somebody does not meet the lower case just raise exception violently The rest tomorrow :) Martin^2 <meta http-equiv="content-type" content="text/html; charset=windows-1252"><pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;"></pre><pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;"></pre> <pre style="background-color:#ffffff;color:#000000;font-family:'DejaVu Sans Mono';font-size:9.0pt;"></pre> </pre> </body></html>