Hi Sue,
Before I dive in, if any of my comments aren't worth addressing due to
upcoming ISIM changes, just say so. But most of what I've got is a pile
of messaging nits.
- Keith
delete_manifest.py:
64: It would be more user friendly to have a specific, separate failure
messages for missing manifest name and missing service name
83, 84: Nit: Use "logging.debug("options = %s", options)" (comma instead
of percent)
86: The return value here seems awkward. Could instead this be changed to:
options.svcdir_path = svcdir_path
return options
119 vs 133: I know this isn't from your changes, but I'm concerned that
the man_name is sanitized in one place and not the other.
publish_manifest.py:
89: Nit: Typo: argument(s:) -> argument(s):
103: Nit: % -> comma
151: Should be gettext'ed?
set_criteria.py:
66: Nit: If my recollection of OptionParser is correct, "CRITERIA" might
be a better metavar. Or "criteria=value|range" (See output of
set-criteria -h)
98: Nit: % -> comma
ai_smf_service.py:
(I'll try to be concise in this file; I expect it to evolve a bit with
ISIM - if any of my comments here aren't worth addressing as a result,
just state so)
46: Nit: Update comment to "Max wait time in seconds ..."
102-103: Three separate smf.AIservice objects are instantiated in this
code path (actually, 2 * # properties + 1). Perhaps that's how AISCF is
designed to work, but that seems undesirable.
132-135: Modifying the props dictionary passed in seems bad - can you
just figure out what the value should be and set the value on the
smf.AIservice?
188-9, 204: Please raise an exception. Ditto for the disable_, enable_,
and restore_instance functions.
396: Nit: The error message should state which property is missing
417: Is now a good time to bring up how strange I find it that the AI
SMF service could go into maintenance by a command an administrator runs?
create_client.py:
37, 40: import arrangement looks off. (Double check the other files as
well, I guess)
93-94: As with delete manifest, it would be slightly more user-friendly
to indicate what option is missing.
96-97, 318: Nit: logging: % -> comma
Should 106-109 come before 102-103?
282: 'prog' looks to be unused
delete_client.py:
58: Nit: Change to "Too many arguments" (and it's definitely plural,
since len(args) > 1)
71, 72: Update logging statements
delete_service.py:
75-76: Should this be capitalized?
83: Same nit comment as for delete_client
93, 94: Update logging statements
785: I think this line should be removed? It's taken care of in
installadm.py, correct?
list.py:
76-84: pep-8: remove spaces around the equal signs in keyword args
1067-1102: If you have time, change the variable names to lower case please.
installadm_common.py:
1293: Nit - use single quotes to enclose the Python string, and there's
no need to escape the internal double quotes, which will be more readable
1296: Should a different message be raised for "blank" service name?
installadm.py:
110: nit: no spaces around usage=usage
124: Nit: len(args) is > 1 in this case, so no parens needed around the
"s"...
125: Does this need to get logged, since we're dumping the output to
stdout anyway?
(Above three also apply to do_disable_service)
283, 342: Change from "sub_cmds.get(entry, None)" to "sub_cmds[entry][1]"
304: I think it should be sys.exit(2) for CLI errors?
343-344: Wasn't this previously checked on 302?
On 01/28/11 10:57 AM, Sue Sohn wrote:
Could I please get a review of the changes for:
7015429 Convert installadm.c to python
Webrev:
http://cr.opensolaris.org/~sohn/7015429
We can thank Joe for some of this code, since he began the conversion
effort as
part of the ISIM project.
Thanks,
Sue
_______________________________________________
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
