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?
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.
----------------------
usr/src/Targetdirs
----------------------
line 69: the "derviced_manifest_test" directory is listed here again.
-------------------------------
usr/src/cmd/aimanifest/aimanifest.py
-------------------------------
- alphabetize all the import statements
- line 48-80: is there a reason for not using InstallLogger(), which should
be used for all install code.
- 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.
- 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. 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"?
- line 255: do you really mean to return -1 here? -1 doesn't sound like a
typical return code.
---------------------------------
usr/src/cmd/auto-install/config/get_manifest
---------------------------------
- Why are these changes needed?
---------------------------
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?
----------------------------------
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,
-----------------------------------
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.
----------------------------
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?
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
