Keith,
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
ok to all of above
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.
Will make this change (this turned out to be comparing 133 vs. 150).
publish_manifest.py:
89: Nit: Typo: argument(s:) -> argument(s):
103: Nit: % -> comma
151: Should be gettext'ed?
ok to above
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
ok to these
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 ..."
ok
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.
yes, seems so, will revise this
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?
yes
188-9, 204: Please raise an exception. Ditto for the disable_, enable_, and
restore_instance functions.
ok
396: Nit: The error message should state which property is missing
will fix
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?
Maybe a separate thread?
create_client.py:
37, 40: import arrangement looks off. (Double check the other files as well, I
guess)
In discussing with you, you were ok leaving it as it is.
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
yes to both
Should 106-109 come before 102-103?
I chose this ordering because the script (102-103) was previously being called
from installadm.c, before create_client was called so this order seemed the
most consistent to existing functionality.
282: 'prog' looks to be unused
yes
delete_client.py:
58: Nit: Change to "Too many arguments" (and it's definitely plural, since
len(args) > 1)
71, 72: Update logging statements
ok
delete_service.py:
75-76: Should this be capitalized?
ok
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?
ok for above.
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.
will do
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
good idea.
1296: Should a different message be raised for "blank" service name?
No, don't think it's necessary.
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"...
Changed to the ever-popular "Too many arguments"
125: Does this need to get logged, since we're dumping the output to stdout
anyway?
(Above three also apply to do_disable_service)
ok to all
283, 342: Change from "sub_cmds.get(entry, None)" to "sub_cmds[entry][1]"
ok
304: I think it should be sys.exit(2) for CLI errors?
ok
343-344: Wasn't this previously checked on 302?
Will remove.
Updated webrev at:
full: http://cr.opensolaris.org/~sohn/7015429.v2
diffs: http://cr.opensolaris.org/~sohn/7015429.v2.diffs
Thank you very much for the review,
Sue
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
