Hi Jack,
I reviewed everything but the GUI installer changes. Here are my comments:
/usr/src/cmd/text-install/summary.py:
- Why is the CDDL section removed?
- lines 236-238: I think this sentence is giving too much implementation
detail for the text installer's summary screen. Users do not need
to know that a support profile is generated or not.
I think it's enough to say that "OCM and ASR services are not installed."
- lines 258-292: Are "MOS credentials" just implementation details? Do the
users need to know about them? If they are not validated,
there's nothing the can do to cause it to be validated, right?
I wonder whether it's necessary to display that info on the summary screen.
- lines 283-292: Since this is the text installer's summary screen, I don't
think these lines are lines appropriate? Only lines 279-282 are appropriate
for text installer summary screen.
usr/src/cmd/system-config/helpfiles/Makefile
- need to update copyright
usr/src/cmd/system-config/summary.py:
- Why is the CDDL section removed?
- lines 204-206: Like my comment above, is it necessary for users
to know that a profile is generated?
- lines 226-260: Same questions as above on exposing MOS credential
info to the users. Isn't that implementation detail?
- lines 247-250: These few lines are not needed, because if you are
at the sys config tool summary screen, you are not running from
a live env.
usr/src/cmd/system-config/profile/support_info.py:
- Gneral question. Why _is_live_env(), _current_net_exists() and
_write_password_file() functions start with "_" for the name?
- General comment about this file:
There are 4 classes defined in this file, OCM_Bridge, ASR_Bridge
SupportCreds and SupportInfo. I do not see the need for these
4 separate classes. I belive it's better for their functionalities
to be combined into 1 single class.
From what I can see, SupportCreds and SupportInfo class contains
the same information. The only thing is the SupportInfo class
is a DOC object. Because they contain the same information, you
need to define SupportCreds.save() and SupportCreds.get() functions
to "transfer" the information between them. Also, in the support.py
UI screens, you need to keep track of whether the information
got saved or not. Why do we need the separate SupportCreds class?
I think having the separate classes are very error prone, because
people might forget to "save".
Furthermore, the OCM_Bridge and ASR_Bridge just provides the
functionality of calling the OCM or ASR command and parse the output.
Those 2 classes doesn't really have any unique attributes. It needs
all the information it needs to work with from the SupportInfo class.
Therefore, it needs a very long list of arguments and a very long
list of return functions. Why not put all the functionalities in
the SupportInfo class itself, since it is utializing all the data there.
Also, instead of passing back 5 return values, those values
can directly be set on the private variables of the SupportInfo class.
IMO, we just need 1 class for SupportInfo(), and all the manipulations
we need on attributes in that class should just be defined in the
class itself instead of being defined in other classes.
- lines 32-49: That's way too complicated to determine whether
you are running from the install env. You can simply check for
the existence of /.textinstall or /.livecd or /.autoinstall file.
- lines 65-67: I think this "for" loop will go through all
the output of the ipadm command, right? I don't think that's
necessary. You can just find the first "success" and conclude
that network exists.
- line 70: since this function is just for writing a string to a temp
file and it is not doing anything special to treat that string as
a password , isn't it better to call it "write_tempfile"?
- line 94 and 99-102: if you don't make _has_auth_script a private
variable, you don't even need the "can_authenticate()" function.
I don't see a reason why _has_auth_script need to be a private variable.
- line 288, and 293-296: Just like above, I don't think _has_auth_script
needs to be a private variable and we don't need the can_authenticate()
function.
- lines 613-616: It is kinda odd to have definitions for class variables
in the middle of the class. Why are they here?
Thanks,
--Karen
On 02/27/12 22:00, 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
