HI Drew.
Thanks for your review.
On 02/28/12 06:06, Drew Fisher wrote:
Jack,
I've looked at everything other than the GUI.
support_info.py
-------------
General comments:
- You should be using the run() partial object from
install_common/__init__.py. For the logger argument, simply pass
logger=LOGGER. For example, lines 55-62 become:
argslist = ['/usr/sbin/ipadm', 'show-addr', '-p', '-o',
'addrobj,state,current']
ipadm_out = run(argslist, logger=LOGGER)
Fixed.
The run() partial will take care of logging for you and for checking
the return code automatically.
20: NIT - alphabetize imports. Place above line 16
Fixed.
67: NIT - unneeded parentheses
Fixed.
70-75: Do you care about permissions when writing this file? mkstemp
opens the file 0600. Is that ok?
Yes. This is a temporary file usable only by the owner, and should not
be world readable.
78: Rename class to OCMBridge to follow pep8 convention
This module passes pep8.
104-114: remove these?
OK
134: align indent
Fixed
242: change to just an else-clause
Can't do that. The last thing in the "if" may set do_encrypt = True.
270: Rename class to ASRBridge to follow pep8 convention
This module passes pep8.
298-308: remove these?
Done.
390-391, 436-437, 453-454: remove?
Remove later, not yet, due to testing environment.
462: change to else-clause
Can't do that. The last thing in the "if" may set do_encrypt = True.
607: s/Err/Error
OK
656: remove ellipses
OK
sysconfig/summary.py && text-install/summary.py
----------
Where'd the CDDL header go?
Fixed for all four text py files.
support.py
12: Alphabetize imports
Fixed.
61, 63, 65, 69, 71, 73: unnecessary parens
Fixed.
291, 292: align with previous line
Fixed.
319, 320: align with previous line. If it won't fit due to line
wrap, move line 319 to a simple 4-space indent and join line 320 to it.
Fixed.
331: unnecessary outer parens
Fixed.
388, 846: remove ellipses
Fixed.
566-568: NIT move comment above the line
OK
653: align
Fixed.
656: NIT move comment above line
670-672, 688-690: see comment for line 319,320
Fixed.
681-682, 700-701, 833, 867: align
Fixed.
910: why the hex cast?
It explains why the upper bound is 65535.
Remember, there are 10 kinds of people in this world. Those who think
in binary and those who don't.
1037-1039, 1060-1062: align
Fixed.
1214: use: if "://" not in value rather than value.find()
Fixed.
Thanks again. I'll post another webrev after going through the other
comments.
Jack
-Drew
On 2/27/12 11:00 PM, Jack Schwartz wrote:
Hi everyone.
This is a webrev for the OCM/ASR configuration project.
OCM and ASR are services which send limited system configuration and
fault information to Oracle, to offer better customer support. This
project provides installer and sysconfig support for configuring the
OCM and ASR services.
There are two parts to this project:
text screens:
- usr/src/cmd/system-config/Makefile
- usr/src/cmd/system-config/__init__.py
- usr/src/cmd/system-config/helpfiles/Makefile
- usr/src/cmd/system-config/profile/Makefile
- usr/src/cmd/system-config/profile/__init__.py
- usr/src/cmd/system-config/profile/support_info.py
- usr/src/cmd/system-config/summary.py
- usr/src/cmd/system-config/support.py
- usr/src/cmd/text-install/summary.py
- usr/src/pkg/manifests/system-install-configuration.mf
GUI screens (courtesy of Dermot):
- usr/src/cmd/gui-install/aux/INSTALL_SUPPORT_PANEL.txt
- usr/src/cmd/gui-install/aux/Makefile
- usr/src/cmd/gui-install/src/Makefile
- usr/src/cmd/gui-install/src/base_screen.py
- usr/src/cmd/gui-install/src/gui_install_common.py
- usr/src/cmd/gui-install/src/help_dialog.py
- usr/src/cmd/gui-install/src/screen_manager.py
- usr/src/cmd/gui-install/src/support_screen.py
- usr/src/cmd/gui-install/xml/Makefile
- usr/src/cmd/gui-install/xml/gui-install.xml
- usr/src/cmd/gui-install/xml/support.xml
- usr/src/pkg/manifests/system-install-gui-install.mf
Please review. Dermot and I would like people familiar with
sysconfig and/or text installer to review the text screens, and
people familiar with GUIs to review the GUI screens. Each part is
about 2000 lines, so please pick a section to review.
Dermot and I would like this round of review to be completed by next
Monday 3/5 COB if possible.
Webrev:https://cr.opensolaris.org/action/browse/caiman/schwartz/7104766_1
Thanks,
Jack and Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
