Re: [caiman-discuss] CR request for dhcp cr's

Thu, 28 Jun 2012 11:45:06 -0700 

Hi Sue,

Thanks for the review. Please find the reply inline.

On 6/28/2012 12:35 AM, Sue Sohn wrote:

On 06/26/12 02:02 PM, Nirmal Agarwal wrote:

Hi all

Can I please get code review for below dhcp related CR's :
7130777 installadm should allow Administrators to disable modification of the DHCP configuration file 
7173043 dhcp smf service goes into maintenance after delete-client
7151276 installadm should try to preserve existing dhcp entry data where possible 

Webrev Location :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/dhcp-crs/webrev/ 

Testing :
Pep8 clean

Slim test : Pass

Manual Testing :
/net/indiana-build.us.oracle.com/export/home/na210770/ai/dhcp/Manual-Testing.txt 

Thanks
Nirmal
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss


Hi Nirmal,

General comment:
You are adding new strings beyond the G11n message freeze (b19).

client_control.py
174 To improve readability, can you change the name get_manage_dhcp()
    to be something more intuitive so we'd have "if com.managing_dhcp()"
    or "if com.dhcp_being_managed()" or something along those lines?

create_service.py
216 The error message says: "DHCP server configuration is disabled."
    which made me think that DHCP was disabled. Perhaps change to more
    explicitly say that installadm (or AI) updates to DHCP server
    configuration are disabled.
216 suggested reword:
     Value of all_services/manage_dhcp property of
     install/server SMF should be True to manage DHCP server
    ->
     Set the value of the install/server SMF property, %s,
     to True to have AI manage the DHCP server changes.
216 Also, please use cli_wrap (cw) for this output

dhcp.py
1065 "we modify host stanza as per service"
      Can you expand on this comment? Not sure what that means.


Updated the webrev.

installadm_common.py
987 see name change suggestion under client_control above
989 Please modify comment so it is clear you are referring
     to the AI SMF service and not a regular AI service.
993 What do you want the behavior to be if the property
    is not part of the service?
As per our discussion on IM, since the property is defined as required=true, it will be part of service. 
server.xml
109-113 please indent to line up with the other all_services properties above 
194-206 same comment, indent to line up with properties above
201-203 suggested reword:
Property which defines whether AI related DHCP server configuration is 
         managed by AI (true) or by administrator (false).


Done.

Questions on testing:
1) Have you tried create-client for a u1 service if that same client currently existed for an S11 service (and vice versa) with both manage_dhcp = True and False? Did you ensure that the contents of the dhcpd conf file (or the output) were correct for both cases, switching from 
supporting uefi to not as appropriate?

Tested and worked as expected.

2) Did you test update-service?


Tested and updated the test results file with the results.

Please find the updated webrev :

webrev rev 1 :

https://cr.opensolaris.org/action/browse/caiman/nirmal27/dhcp-crs-rev1/webrev/

Diff webrev :
https://cr.opensolaris.org/action/browse/caiman/nirmal27/dhcp-crs-diff/webrev-diff/

Thanks,
Nirmal


Sue


_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss

Reply via email to