[Freeipa-devel] DHCP support - Request for review

[Petr Vobornik]

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