[Freeipa-devel] DHCP support - Request for review

[William Brown]

On 07/13/2012 08:39 PM, Petr Vobornik wrote:
> 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

I have never written an IPA plugin before, so most of this was new to
me. You did answer some of my questions about how you do things like
optional attributes etc. I have made most of these changes and will
complete the rest later (time allowing). Thanks for your advice.

-- 
Sincerely,

William Brown

pgp.mit.edu
http://pgp.mit.edu:11371/pks/lookup?op=vindex&search=0x3C0AC6DAB2F928A2



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 940 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120717/0b8fa3f7/attachment.sig>