Hi Karen.
Thanks for reviewing.
On 02/28/12 10:39, Karen Tung wrote:
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?
Restored.
- 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."
OK.
- 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.
The summary screen reflects the state of the Support properties, as for
other groups. Unlike other property groups, property state depends on
more than just what is entered on the screen.
Users know about credentials from previous screens, where they were
entered. Users can backspace and fix their entries.
- 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.
OK.
usr/src/cmd/system-config/helpfiles/Makefile
- need to update copyright
Thanks.
usr/src/cmd/system-config/summary.py:
- Why is the CDDL section removed?
Restored.
- lines 204-206: Like my comment above, is it necessary for users
to know that a profile is generated?
A CU would find this message useful, as it will inform CU to install
those services in order to get a Support profile.
It's useful to leave it in here even though I took it out of the text
installer summary because sysconfig is more general purpose, including
generating profiles for other systems.
- lines 226-260: Same questions as above on exposing MOS credential
info to the users. Isn't that implementation detail?
Leaving in for same reasons as above.
- 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.
Yup, removed. Also removed some extraneous comments.
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?
They are used only in this file.
- 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".
Other modules, such as users.py, take their input into local variables,
call validate() on the local variable values, and then copy them to the
DOC object in on_change_screen(). I assume this was done for a reason.
The SupportCreds class serves the same purpose as the local variables do
in users.py. There are so many fields that I created a SupportCreds
class to store them all. Furthermore, those fields get manipulated and
changed by authentication. Better to commit them to the DOC together
once all of their handling is done. That's why SupportCreds was created
in the first place.
If I can violate that protocol (and please let me know if I can), then
I'm happy to get rid of SupportCreds.
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.
_has_auth_script or can_authenticate()
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.
I like the idea of having separate functionality grouped together in a
class, even if the class is not ever instantiated. I also see your
point about long argument lists.
Most of the long variable list are locals for input fields not in
SupportCreds: proxy and hub fields and encryption key, plus a few other
fields. Again, if I can violate the protocol of manipulating in local
variables, then please let me know. If I can do this, I can collapse
all into a single 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.
OK
- 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.
You're right but I'm going to leave it along. Comprehensions are more
efficient than a proper for loop, and the number of network devices is
usually very small.
- 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"?
OK. I'll also note that mkstemp is the secure way of writing files, and
so is well-suited for passwords and sensitive data.
- 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.
It is more object-oriented this way. If you feel strongly I'll change
it though.
- 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?
Moved to module scope.
Thanks,
Jack
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
