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)


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
67:  NIT - unneeded parentheses
70-75: Do you care about permissions when writing this file? mkstemp opens the file 0600. Is that ok?
78:  Rename class to OCMBridge to follow pep8 convention
104-114:  remove these?
134:  align indent
242:  change to just an else-clause
270:  Rename class to ASRBridge to follow pep8 convention
298-308:  remove these?
390-391, 436-437, 453-454:  remove?
462:  change to else-clause
607:  s/Err/Error
656:  remove ellipses

sysconfig/summary.py && text-install/summary.py
----------

Where'd the CDDL header go?


support.py
12:  Alphabetize imports
61, 63, 65, 69, 71, 73:   unnecessary parens
291, 292:  align with previous line
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.
331:  unnecessary outer parens
388, 846:  remove ellipses
566-568:  NIT move comment above the line
653:  align
656:  NIT move comment above line
670-672, 688-690:  see comment for line 319,320
681-682, 700-701, 833, 867:  align
910:  why the hex cast?
1037-1039, 1060-1062:  align
1214:  use:  if "://" not in value rather than value.find()

-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

Reply via email to