On 06/27/2012 03:32 PM, William Brown wrote:
Hi,

I have been working on adding support for FreeIPA to support
configuration storage for ISC-DHCP 4.X servers. I have added the schema
which is included at installation, added the template / empty files that
will be filled in and used for the installation and created the ipalib
plugin for this. At this stage, this feature is still far from done. I
would appreciate a review of the work I have done to ensure I am on the
right track.

I would like to know if there is a better way to add ACLs than by
manually updating ldap by hand (IE, using the ACL libraries) (See
/install/share/dhcpd.ldif).



I just looked on the plugin part from Web UI (API) perspective. Proper plugin review should do somebody else.

What seems bad:
1) all entity params are required. When I'm looking at ldif, only couple of attributes are MUST.

Param is made optional by adding '?' after its name. Example from user.py:

    Str('displayname?',
        label=_('Display name'),
        default_from=lambda givenname, sn: '%s %s' % (givenname, sn),
        autofill=True,
    ),

However you can probably enforce some params to be required if it's required by DHCP server and not to blindly copy the LDAP schema.

2) You don't have to specify 'primary_key=False', it's default.

3) Don't use prefix for cli_name like 'dhcp_server_implementation' cli_name is used by IPA Client as an option name. Same for labels. For example for adding dhcp server proper command would probably be:

ipa dhcpserver-add $NAME --service-dn=$SERVICE_DN etc

not:

ipa dhcpserver-add $NAME --dhcp-server-service-dn=$SERVICE_DN etc

so the param def should be:

    Str('dhcpservicedn?',
        cli_name='service_dn',
        label=_('Service DN'),
    ),


5) All params are STR. I think some might be INT or something else.

6) You can fill 'default_attributes' list with more param names. What is mentioned in default attributes is displayed by `XXX-show $CN` command. What is not have to be displayed by `XXX-show $CN --all` command.

7) In labels you use 'Dhcp'. IMO it should be all uppercase.

Regards
--
Petr Vobornik


_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to