Hi Sue and Keith.
Code seems pretty solid! Here's group 2, plus service.py:
usr/src/cmd/installadm/create_client.py
---------------------------------------
130: Error message requests that an image path be provided, but the
parser and usage for create-client does not accommodate one.
238: Logging message is inaccurate.
usr/src/cmd/installadm/create_service.py
---------------------------------------
164: s/error is service/error if service/
Optimization (mainly removing duplicate code):
if image:
count = 0
basename = None
try:
basename = image.get_basename()
com.validate_service_name(basename)
svc_name = basename
except (ValueError, AttributeError):
pass
if basename is None:
basename = BASE_DEF_SVC_NAME
count = 1
svc_name = basename + "_" + str(count)
248: Suggest genericizing default_image_path_ok to a general yes/no
function. Arguments can be a prompt string, and possibly a string to
display if "yes" or "no" is not entered. It can return True or False.
311: A clearer name may be should_be_default_for_arch()
415-416: nit: may be better not to say which links are made here, as the
comment could go out of date and forget to be updated if
do_default_sparc_symlinks() gets updated in the future. One can look up
do_default_sparc_symlinks() to see what those links are.
usr/src/cmd/installadm/delete_client.py: OK
---------------------------------------
usr/src/cmd/installadm/delete_service.py
---------------------------------------
132: suggest using generic yes/no function (see comment for
create_service.py/line 248)
usr/src/cmd/installadm/installadm.py
---------------------------------------
216: It can also return 4 on line 363. What does that mean? It can
also return 1 if an exception is encountered. Please document these things.
340, 342, 358, 360, other places?: I believe pep8 will complain about no
spaces before and after the +.
usr/src/cmd/installadm/list.py
---------------------------------------
188, 494: Just curious as to why
print ' '.ljust(width)
was changed to
print ' ' * width
Seems more consistent with other print statements in this function to
leave it the "ljust" way.
234: strip of the -> strip off the
365: '-' is length 1, but aliasofwidth is still set to 0 here. Move 363
to below the if/else, to 365+?
384: Making "No services configured" as an error again would be
consistent with other commands, for example ls when it finds no files.
518: same comment as for 384.
607-614: Idea: since errors will muddy up the IO formatting on the
screen, you can maintain a list of errors and dump them to the screen at
the end of the "normal" output (but through stderr). This will keep
formatting readable and still display errors as they need to be displayed.
743: General question: how come some calls to AIService are in
try/except (e.g. 560, 343, 827, 974) and others are not (229, 743) ?
777: Please either add an "else: raise Exception" for the case where the
dbtable is not a profile or manifest table (preferred), or else change
the elif on 764 to an else.
usr/src/cmd/installadm/rename_service.py: OK
---------------------------------------
usr/src/cmd/installadm/service_config.py
---------------------------------------
114: Nit: Put this line after 117
281: How come a missing property name is not an error? (Not sure if
this is an error or not, so please explain.)
294: Doesn't port need to be checked even if new PROP_TXT_RECORD is added?
795: Looks like takes defaults if config file for service name doesn't
exist. Update comment?
611: filesy -> filesystem
usr/src/cmd/installadm/set_service.py: OK
---------------------------------------
Extra:
usr/src/cmd/installadm/service.py
---------------------------------------
344: should ONLY -> should be used ONLY
1108: Fix imports later?
1122: Unless a service's manifests can now be in any directory (which
doesn't look to be the case), please change the os.path.join back to
"/".join(self.manifest_dir, manifest_name). This in effect causes any
absolute manifest_names to err out instead of making use of their
absolute pathnames.
Thanks,
Jack
On 05/ 2/11 03:36 PM, Sue Sohn wrote:
We are requesting a code review of the ISIM project (minus the DHCP
related
modifications which will be sent out for review separately).
Webrev location:
http://cr.opensolaris.org/~kemitche/webrev.ISIM.1/
Please provide your comments by May 16.
The code can be divided into the following sections:
1) ai-webserver subdir
usr/src/cmd/ai-webserver/AI_database.py
usr/src/cmd/ai-webserver/cgi_get_manifest.py
usr/src/cmd/ai-webserver/common_profile.py
usr/src/cmd/ai-webserver/create_profile.py
usr/src/cmd/ai-webserver/delete_manifest.py
usr/src/cmd/ai-webserver/delete_profile.py
usr/src/cmd/ai-webserver/export.py
usr/src/cmd/ai-webserver/export_profile.py
usr/src/cmd/ai-webserver/publish_manifest.py
usr/src/cmd/ai-webserver/set_criteria.py
usr/src/cmd/ai-webserver/test/test_set_criteria.py
usr/src/cmd/ai-webserver/validate_profile.py
2) installadm subdir - commands plus service_config.py:
usr/src/cmd/installadm/create_client.py
usr/src/cmd/installadm/create_service.py
usr/src/cmd/installadm/delete_client.py
usr/src/cmd/installadm/delete_service.py
usr/src/cmd/installadm/installadm.py
usr/src/cmd/installadm/list.py
usr/src/cmd/installadm/rename_service.py
usr/src/cmd/installadm/service_config.py
usr/src/cmd/installadm/set_service.py
3) Rest of installadm subdir, including service.py and image.py
usr/src/cmd/installadm/ai_smf_service.py
usr/src/cmd/installadm/aimdns.py
usr/src/cmd/installadm/aimdns_mod.py
usr/src/cmd/installadm/aimdnsd.py
usr/src/cmd/installadm/grub.py
usr/src/cmd/installadm/image.py
usr/src/cmd/installadm/installadm-common.sh
usr/src/cmd/installadm/installadm_common.py
usr/src/cmd/installadm/properties.py
usr/src/cmd/installadm/service.py
usr/src/cmd/installadm/setup-image.sh
usr/src/cmd/installadm/setup-service.sh
usr/src/cmd/installadm/setup-sparc.sh
usr/src/cmd/installadm/setup-tftp-links.sh
usr/src/cmd/installadm/svc-install-server
4) Other (Makefiles, packaging changes, lib changes, man page)
usr/src/cmd/auto-install/svc/manifest-locator
usr/src/cmd/auto-install/version
usr/src/cmd/installadm/Makefile
usr/src/cmd/installadm/test/test_aimdns.py
usr/src/cmd/installadm/test/test_create_service.py
usr/src/cmd/installadm/test/test_rename_service.py
usr/src/cmd/installadm/test/test_service_config.py
usr/src/cmd/slim-install/svc/net-assembly
usr/src/lib/install_common/__init__.py
usr/src/lib/netif/Makefile
usr/src/lib/netif/_netif.c
usr/src/lib/netif/_netif.h
usr/src/lib/netif/mapfile-vers
usr/src/lib/netif/netif.py
usr/src/lib/netif/test/test_netif.py
usr/src/man/installadm.1m.txt
usr/src/pkg/manifests/install-installadm.mf
usr/src/pkg/manifests/system-install-auto-install-auto-install-common.mf
Thanks,
Sue, Keith, Jesse, and Harold
_______________________________________________
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
