Hi Karen.
Thanks for taking time to review.
On 03/23/11 01:00 AM, Karen Tung wrote:
Hi Jack,
Here are my comments for all the files in group D. If a file is not
mentioned,
that means I have no comment on it.
-----------------------
usr/src/Makefile.master
------------------------
Line 109-110: I don't see the "derived_manifest_test" directory in the
list of files for code review. Is it supposed to be there?
Makefile.master sets up directories in the proto area, not in source files.
Most functionality have the "tests" directory as a subdirectory
in the area where the code is. So, I am surprised to see a special
directory
for tests.
Some library modules have tests in the proto area, for example
./solaris_install/engine/test
./solaris_install/engine/test/engine_test_utils.py
./solaris_install/engine/test/engine_test_utils.pyc
./solaris_install/target/test_td.py
./solaris_install/target/test_td.pyc
There doesn't seem to be a standard way of naming though (in a test
subdir, not in a test subdir,...)
"derived_manifest_tests" seems a bit long too. I'll rename
"derived_manifest_tests" (which is under the AI checkpoints directory)
to be "test" . This will also be more generic, so if others want to add
tests for their checkpoints they can put them there.
Like test_utils, I'll just store the tests in the proto area and not add
them to a test package, unless someone says they should go in the
install-tests package.
I will also store other unit tests in the proto area as well.
----------------------
usr/src/Targetdirs
----------------------
line 69: the "derviced_manifest_test" directory is listed here again.
Changed to "tests" here as in Makefile.master.
-------------------------------
usr/src/cmd/aimanifest/aimanifest.py
-------------------------------
- alphabetize all the import statements
Done.
- line 48-80: is there a reason for not using InstallLogger(), which
should
be used for all install code.
Yes, there is. aimanifest is intended for use mainly by the derived
manifests module (DMM). The DMM takes output from the scripts it runs
(and thus the commands those scripts run) and logs them. To do so again
in aimanifest would result in double-logging.
However, the DM spec calls for aimanifest to log to a logfile of its
own, as specified by the environment variable AIM_LOGFILE. This is what
I set up.
As I scrutinize this module, however, I see that there are several
places where error logging (as opposed to just error printing) is
missing. This will be changed before code review round 2.
- line 98-121: instead of having a function dedicated to creating
the usage string, the fix part of the string can be
defined as a constant, and all you had to do is to input the
program name as you have them.
Currently the usage shows the command name in several places, so I'd
like to leave the implementation as is.
- For all the exceptions, you have a function called handle_error() to
deal with the exception. I guess you do that because you don't want
the user to see the full trace back. However, there's useful information
in the traceback that should be captured.
handle_error() is called for specific exceptions due to expected kinds
of errors, such as file not found or mim_errors. Unexpected exceptions
will go through the normal bubble-up-to-the-commandline-with-traceback
handling. In this case, tracebacks will be output to stderr, and the
DMM will capture and log the errors properly.
The most appropriate place
is the log file, I think. Instead of logging just the error string
from the traceback to the log file in the handle_error() function, it
would be useful to just call AimLog.exception() inside the "except".
That way, the full exception will be captured. Then, the handle_error()
function can still be used to hide the exception from the user.
- line 200-202, 217-218, 235-238, : looks like debugging info to me,
do you really want the log level "info"?
Changed to debug level.
- line 255: do you really mean to return -1 here? -1 doesn't sound
like a
typical return code.
Changed to errno.EINVAL
---------------------------------
usr/src/cmd/auto-install/config/get_manifest
---------------------------------
- Why are these changes needed?
May not be needed. I'll build a gate without these changes and verify
that it works. I'll let you know why I need this file if I have to put
it back.
---------------------------
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/shadow/Makefile
---------------------------
- These 2 files don't look like they belong to this project.
Are they included in the review by mistake?
I fixed some spacing that apparently doesn't show up in the webrev.
----------------------------------
usr/src/pkg/manifests/system-install-auto-install.mf
----------------------------------
- line 32-33, in addition to these 2 directories, I think you also
need to list out all the directories leading to these 2 directories,
Yup, thanks.
-----------------------------------
usr/src/cmd/rbac/prof_attr.system%2Finstall%2Fauto-install
-----------------------------------
- line 24: Do you also need to specify a "help=xxxxx.html" field
for this entry? I see entries delivered by other ON packages have
that field.
It's optional. The prof_attr file for the DC package doesn't have one.
In ON, most fields in usr/src/lib/libsecdb/prof_attr.txt have it but not
all of them. The manpage says its optional as well.
----------------------------
usr/src/cmd/rbac/user_attr.system%2Finstall%2Fauto-install
----------------------------
- line 24-25: I am confused. Normally, root is a role. Line 25 seem
to suggest that root will be assigned the role "aiuser". If I install
the auto-install package into my system where my root is a role, what
will happen to the root role?
This is the AI client image, where root is not a role.
Line 25 is redundant, as root can do anything it wants, including su to
aiuser. Originally I included it more for documentation purposes but
maybe that was not a good idea. I'll remove that line and verify things
still work, and leave the line out if they do.
Thanks again for reviewing.
Thanks,
Jack
Thanks,
--Karen
On 03/11/11 07:36 PM, Jack Schwartz wrote:
Hi everyone.
Here is the first of two code review wads for the Derived Manifests
project.
This wad includes the AI client side support for running a script to
derive manifests. A subsequent (much smaller) one coming within a
few days will be for improved AI server support of default manifest
management.
The focus of this code review is on the following:
- the Derived Manifest Module checkpoint
- the aimanifest command
- the Manifest Input Module (which supports aimanifest)
http://cr.opensolaris.org/~schwartz/110311.1/webrev/index.html
Please take a group of files and review by Weds 3/23 COB. Please let
me know ASAP what you are able to review.
I'll be setting up an info session for Tuesday morning from 9-10 PST
to help *jumpstart* the process. (Sorry, couldn't resist) Logistic
info to follow.
I suggest the following groups:
(A) The Derived Manifest Module
usr/src/cmd/auto-install/checkpoints/dmm/Makefile
usr/src/cmd/auto-install/checkpoints/dmm/__init__.py
usr/src/cmd/auto-install/checkpoints/dmm/dmm.py
usr/src/cmd/auto-install/checkpoints/dmm/dmm_errors.py
usr/src/cmd/auto-install/checkpoints/dmm/test/Makefile
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_build_test.py
usr/src/cmd/auto-install/checkpoints/dmm/test/dmm_env_test.py
(B) The Manifest Input Module, focus on overlay
usr/src/lib/install_manifest_input/Makefile
usr/src/lib/install_manifest_input/__init__.py
usr/src/lib/install_manifest_input/process_dtd.py
usr/src/lib/install_manifest_input/test/test_manifest_input_overlay.py
usr/src/lib/install_manifest_input/test/test_process_dtd.py
(C) The Manifest Input Module, the rest
usr/src/lib/install_manifest_input/mim.py
usr/src/lib/install_manifest_input/mim_utils.py
usr/src/lib/install_manifest_input/mim_errors.py
usr/src/lib/install_manifest_input/test/test_manifest_input_pathing.py
usr/src/lib/install_manifest_input/test/test_manifest_input_set_get_add.py
usr/src/lib/install_manifest_input/test/test_manifest_input_validate_commit.py
(D) aimanifest command, AI environment prep, packaging and other files:
usr/src/cmd/aimanifest/Makefile
usr/src/cmd/aimanifest/aimanifest.py
usr/src/Makefile.master
usr/src/Targetdirs
usr/src/cmd/Makefile
usr/src/cmd/Makefile.cmd
usr/src/cmd/Makefile.targ
usr/src/cmd/auto-install/auto_install.py
usr/src/cmd/auto-install/checkpoints/Makefile
usr/src/cmd/auto-install/config/get_manifest
usr/src/cmd/auto-install/svc/auto-installer
usr/src/cmd/auto-install/svc/manifest-locator
usr/src/lib/Makefile
usr/src/lib/Makefile.targ
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/shadow/Makefile
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf
usr/src/pkg/manifests/system-install-auto-install.mf
usr/src/tools/tests/tests.nose
usr/src/cmd/rbac/Makefile
usr/src/cmd/rbac/prof_attr.system%2Finstall%2Fauto-install
usr/src/cmd/rbac/user_attr.system%2Finstall%2Fauto-install
usr/src/cmd/auto-install/ai_get_manifest.py
Thanks,
Jack
_______________________________________________
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
