Hi Jan and Keith,
Here are my comments for files in sections E and F.
General comment:
-------------------
3 comments on the following line in all the files for the text installer:
self.install_profile = doc.get_descendants(name="install_profile")[0]
- Instead of hard coding the string "install_profile" here, I think you
should
use the label string defined in the install_profile object.
- I think it would be useful to check and make sure the
value returned from doc.get_descendants is not None instead of just assuming
that it is valid.
- Would it be useful to define a helper function for getting the
install profile obj so we don't have the exact same code all over the place?
I understand that it is just 2 lines, but it would still be useful
to just make 1 change instead of 10+ changes, if something needs to change.
Specific comments:
---------------------------
> usr/src/cmd/text-install/osol_install/text_install/__init__.py
- line 35: why are you doing "del gettext" here? It would be useful
to put a comment here explaining the reason.
> usr/src/cmd/text-install/osol_install/text_install/disk_selection.py
- line 99: instead of hard coding the full path to the text installer
help file
here, I think it would be better for the future if the path to the text
installer help file is defined in a common place, and then, just append
the specific help text file for the screen here.
> usr/src/cmd/text-install/osol_install/text_install/fdisk_partitions.py
- lines 70-77: same comment about the help file paths.
> usr/src/cmd/text-install/osol_install/text_install/install_status.py
- line 81: "install_succeeded" is not defined in the install_profile object.
I understand that you can just add variables to an object in Python.
But I think it would be useful for debugging to initialize it
to False in the __init__ of the object, and also to print the
value in the __str__ method.
>
usr/src/cmd/text-install/osol_install/text_install/partition_edit_screen.py
- lines 82-90: same comment regarding hard coding the full help file path.
> usr/src/cmd/text-install/osol_install/text_install/summary.py
- lines 55-56: same comment regarding hard coding the full help file path.
- lines 151-152: I think it would be better to define somewhere, perhaps
somewhere in the sysconfig module,
constants for indexes in the array for the root and primary user, so, we
don't
need to remember that 0 means root and 1 means primary user.
> usr/src/cmd/text-install/osol_install/text_install/text_install.py
- line 149: why is the loglevel hard coded to 0, instead of the
value in options.log_level?
- lines 155-156: "no_install_mode" and "is_x86" are not defined in the
install_profile object. Should they be. As with the previous comment,
it would be useful to define them and to print them out in __str__ for
debugging.
- line 160: I know there's no harm to register the checkpoint here.
But would it make the code easier to read to register it before you need
to run it?
- line 254: The way you are getting a reference to the install profile here
is different than all the other places in the text installer, where you
are searching for name="install_profile". Please be consistent.
> usr/src/cmd/text-install/osol_install/text_install/ti_install.py
- lines 69-73: these lines are no longer needed.
- 386-402: I believe all these lines are no longer needed since the
ti_install.py file isn't involve in calling any functions directly
to create the users. Please also fix the function definition and the
calling of
the run_ICTs() function, since most of those arguments are no longer used.
- line 491: since this is temporary, perhaps move the registration
of the checkpoint to execute here? That way, we have all the checkpoint
execution
stuff that need to be "relocated" in one place. By the way, is the name
"apply-sc-profile" defined somewhere in the sys
config code? If so, it will be better to use that constant than hard coding
the string here.
- line 580-581: since the previous C ICT "ict_set_host_node_name" is split
into 2 separate ICTs. You are just calling the ict_set_hosts here,
Do you also need to call the ict_set_nodename ICT?
> usr/src/cmd/text-install/osol_install/text_install/welcome.py
- line 56: same comment about full path for help file.
> usr/src/lib/libict/ict_test.c
- line 114, line 125: I know you are just following the convention in
this file.
Is there a reason why a hard coded number is used as the 3rd argument to
the strncmp() function instead of doing strlen(SET_NODENAME) and
strlen(SET_HOSTS)? Using strlen() seems more maintainable.
Thanks,
--Karen
On 12/14/10 07:00 AM, Jan Damborsky wrote:
Hello Caimaniacs,
we (me & Keith) would like to ask you for a feedback for the first
version
of System Configuration Interactive tool.
Please see below for details.
Thank you very much,
Jan
Overview
--------
We decided to deliver SCI tool in steps. There are couple of reasons
for that
decision - we consider following to be the most important ones:
* There is a significant amount of external dependences. Instead of
waiting
for all of them to be satisfied, we believe it makes sense to deliver
in steps taking into account time frame in which particular
dependences get satisfied.
* There are bunch of ongoing 'CUD efforts' which are assumed to share
following
pieces delivered by SCI tool project:
* DOC data structures holding system configuration information
* mechanism for generating SC profile
* changes in ICT world
* initial changes for text installer->CUD transition
We assume that making those common pieces available as soon as
possible will
help related CUD project with making smoother progress.
That first phase intends to deliver SCI tool and modified text installer
with following characteristics:
[1] SCI tool
* system configuration information stored in DOC
* SCI tool compliant with CUD
* limited set of screens available - in particular those
which are currently available in text installer plus
support IPv4 manual network configuration & DNS
[2] text installer
* SCI tool embedded in text installer
* initial steps taken to transition text installer into CUD
* generated SC profile used for configuration of installed system
* ICT tasks removed/modified as a result of text installer configuring
most of the things in generated SC profile
Things which will be addressed in next steps (e.g. when related
dependences
are met and screens are designed):
* configuration of naming services
* configuration of locales
* support for IPv6 network configuration
* zone awareness
Code review information
-----------------------
webrev:
http://cr.opensolaris.org/~kemitche/sci.1/
Unit tests are not part of this review - they currently being developed
by Keith and we assume to include them for 2nd (final) round of review.
design documents:
System Configuration Interactive Tool design spec:
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/sci%2Dtool%2Dlatest.pdf
System Configuration SMF Service design spec:
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/scsmfdesignlatest.pdf
Since couple of thousand of lines in slim_source were affected, we
divided changes
into several groups - please see below.
Please let us know off line if you plan to review any of those
changes, so that we can assure we have at least one reviewer for
each group of changes.
We are aware that most of the people will be unavailable at least for
some
period during Christmas time, so we will be collecting the feedback
for the
rest of this year until 12/31.
That said, if you would like to review those changes and need more time,
just let us know, we will account for that.
Testing done:
* text install media built with modified bits
* different install scenarios tested using built text install media
* SCI tool run as a standalone application
- different scenarios tested and generated System Configuration
profiles inspected
Groups
------
[a] sysconfig module - UI part
usr/src/cmd/system-config/date_time.py
usr/src/cmd/system-config/helpfiles/Makefile
usr/src/cmd/system-config/helpfiles/date_time.txt
usr/src/cmd/system-config/helpfiles/network.txt
usr/src/cmd/system-config/helpfiles/network_manual.txt
usr/src/cmd/system-config/helpfiles/summary.txt
usr/src/cmd/system-config/helpfiles/timezone.txt
usr/src/cmd/system-config/helpfiles/users.txt
usr/src/cmd/system-config/helpfiles/welcome.txt
usr/src/cmd/system-config/network_nic_configure.py
usr/src/cmd/system-config/network_nic_select.py
usr/src/cmd/system-config/network_type.py
usr/src/cmd/system-config/summary.py
usr/src/cmd/system-config/timezone.py
usr/src/cmd/system-config/users.py
usr/src/cmd/system-config/welcome.py
usr/src/cmd/system-config/sysconfig.py
usr/src/cmd/system-config/test/test_users.py
usr/src/pkg/manifests/system-install-configuration.mf
usr/src/cmd/system-config/__init__.py
usr/src/cmd/system-config/Makefile
[b] sysconfig module - mapping data in DOC & smf profile generator
usr/src/cmd/system-config/profile/Makefile
usr/src/cmd/system-config/profile/__init__.py
usr/src/cmd/system-config/profile/ip_address.py
usr/src/cmd/system-config/profile/network_info.py
usr/src/cmd/system-config/profile/system_info.py
usr/src/cmd/system-config/profile/user_info.py
usr/src/cmd/system-config/xslt/Makefile
usr/src/cmd/system-config/xslt/doc2sc_profile.xslt
[c] svc:/system/install/config smf(5) service -
changes needed in order to support configuration
of user/root account in text installer
usr/src/cmd/system-config/svc/svc-system-config
usr/src/cmd/system-config/svc/system-config.xml
[d] terminalui Python module
usr/src/lib/Makefile.targ
usr/src/lib/terminalui/Makefile
usr/src/lib/terminalui/__init__.py
usr/src/lib/terminalui/action.py
usr/src/lib/terminalui/base_screen.py
usr/src/lib/terminalui/color_theme.py
usr/src/lib/terminalui/edit_field.py
usr/src/lib/terminalui/error_window.py
usr/src/lib/terminalui/help_screen.py
usr/src/lib/terminalui/i18n.py
usr/src/lib/terminalui/inner_window.py
usr/src/lib/terminalui/list_item.py
usr/src/lib/terminalui/main_window.py
usr/src/lib/terminalui/screen_list.py
usr/src/lib/terminalui/scroll_window.py
usr/src/lib/terminalui/test/test_edit_field.py
usr/src/lib/terminalui/test/test_i18n.py
usr/src/lib/terminalui/test/test_scroll_window.py
usr/src/lib/terminalui/test/test_window_area.py
usr/src/lib/terminalui/window_area.py
[e] text installer - UI part
usr/src/cmd/text-install/helpfiles/Makefile
usr/src/cmd/text-install/osol_install/profile/Makefile
usr/src/cmd/text-install/osol_install/profile/install_profile.py
usr/src/cmd/text-install/osol_install/text_install/Makefile
usr/src/cmd/text-install/osol_install/text_install/__init__.py
usr/src/cmd/text-install/osol_install/text_install/disk_selection.py
usr/src/cmd/text-install/osol_install/text_install/disk_window.py
usr/src/cmd/text-install/osol_install/text_install/fdisk_partitions.py
usr/src/cmd/text-install/osol_install/text_install/install_progress.py
usr/src/cmd/text-install/osol_install/text_install/install_status.py
usr/src/cmd/text-install/osol_install/text_install/log_viewer.py
usr/src/cmd/text-install/osol_install/text_install/partition_edit_screen.py
usr/src/cmd/text-install/osol_install/text_install/summary.py
usr/src/cmd/text-install/osol_install/text_install/test/test_disk_select.py
usr/src/cmd/text-install/osol_install/text_install/test/test_disk_window.py
usr/src/cmd/text-install/osol_install/text_install/text_install.py
usr/src/cmd/text-install/osol_install/text_install/ti_install.py
usr/src/cmd/text-install/osol_install/text_install/welcome.py
usr/src/pkg/manifests/system-install-text-install.mf
[f] text installer - ICT
usr/src/lib/libict/ict.c
usr/src/lib/libict/ict_api.h
usr/src/lib/libict/ict_test.c
usr/src/lib/libict_pymod/ict.py
usr/src/lib/libict_pymod/sc_template.xml
usr/src/lib/liborchestrator/perform_slim_install.c
usr/src/cmd/slim-install/finish/install-finish
[g] rest
.hgignore
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile.targ
usr/src/lib/install_logging_pymod/logger.py
usr/src/lib/install_manifest/writer/writer.py
usr/src/pkg/manifests/system-install.mf
usr/src/tools/tests/config.nose
usr/src/tools/tests/tests.nose
_______________________________________________
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
