Thanks Sue and Jesse for the review.
Jesse,
Thanks for guiding me through changes for the CR's.
Regards
Nirmal
On 6/29/2012 9:08 PM, Jesse Butler wrote:
On 06/29/12 11:08 AM, Sue Sohn wrote:
Hi Nirmal,
See below.
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.
Please change "changes" to "updates"
Agreed.
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.
I have changed installadm_common.py to use svcprop instead of
getboolean_property() as it leads to
problems when property is not defined.
I have updated the webrev and tested the changes which works fine.
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/
This link didn't work for me, but the following one did (and is what
I reviewed):
https://cr.opensolaris.org/action/browse/caiman/nirmal27/dhcp-crs-diff/webrev-diff-1/
My other comments have been addressed.
Thanks,
Sue
Same here, looks fine to me.
/jb
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
