Jack,
Thanks very much for the review.
On 05/13/11 03:54 PM, Jack Schwartz wrote:
> 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.
Not having an image path is an error condition - will revise.
> 238: Logging message is inaccurate.
ok
> usr/src/cmd/installadm/create_service.py
> ---------------------------------------
>
> 164: s/error is service/error if service/
ok
> 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)
Turns out that the passing of image was for the pkg version which we aren't
implementing now, so will simplify this function and save your version for later.
> 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.
Thanks for the reminder, I had meant to do this.
> 311: A clearer name may be should_be_default_for_arch()
changed
> 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.
ok
> 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)
yes
> 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.
ok
> 340, 342, 358, 360, other places?: I believe pep8 will complain about no
spaces
> before and after the +.
OK to above
> 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.
Keith will respond to these comments.
> usr/src/cmd/installadm/rename_service.py: OK
> ---------------------------------------
>
> usr/src/cmd/installadm/service_config.py
> ---------------------------------------
>
> 114: Nit: Put this line after 117
N/A due to code changes
> 281: How come a missing property name is not an error? (Not sure if this is an
> error or not, so please explain.)
It is, see line 302.
> 294: Doesn't port need to be checked even if new PROP_TXT_RECORD is added?
It's checked on 296
> 795: Looks like takes defaults if config file for service name doesn't exist.
> Update comment?
ok
> 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
ok
> 1108: Fix imports later?
This will be done sometime after Ethan's project goes in as it will involve some
code restructuring which. if done now, will cause merging to be more difficult.
> 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.
changed back.
Thanks,
Sue
> 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
