Hi Joe,
General: I think it'd be worth considering laying out the files
differently, rather than using
{osol/solaris}_install.auto_install.utils. "utils" is too generic in my
eyes - if a generic location is needed, dropping directly under
auto_install would be preferred.
For example, osol_install.auto_install.utils.ai_image could simply be
"osol_install.auto_install.image" (note that the "ai_" prefix is
removed, as it's implied by the "auto_install" part).
create_service.py:
42: (Alternative style option) - when importing this many items from a
module, I find it cleaner to simply import the module and reference the
functions/constants via the module, e.g. "import foo.bar.baz as baz;
baz.MY_CONSTANT; baz.my_function()"
113/117/122: Is the "is_iso" really in a position to fatally exit here?
(As much as possible, I try to view functions in isolation from their
context - as an arbitrary consumer of is_iso(), I wouldn't expect those
failures to kill my program). The first OSError will only occur if
"/usr/bin/file" has an issue - quite frankly, it's not worth worrying
about, and if a sysadmin has mucked around with that, it's probably
better if things exit uncleanly.
126-9: Nit: Use the syntax "if 'x' in 'abcxyz'" for checking for substrings.
131/4: Parens not needed.
165: Re-wrapping this exception doesn't seem to add significant value.
285: This line should probably be run from installadm itself, rather
than individually in each subcommand.
330: Just have create_target raise an appropriate error in the first
place - no need to re-wrap the exception.
399-414: Nits: Trailing '\'s not needed (due to being inside brackets
{}), and should align all lines with the opening brace, e.g.:
line_1 = {hello : "goodbye"
line_2: "foobar"}
487-492: Another good candidate for not-try-excepting.
delete_service.py:
86-87: Use "parser.error(<msg>)" to print_help and exit in one step.
installadm.py:
61: ESCDELAY is a curses-related environment variable and shouldn't be
needed here
129-130: Use parser.error()
ai_image.py:
59: I think the default name should be different.
88: As discussed on the phone, it'd be best to avoid adding yet another
'run_cmd' style function.
160: This appears to be duplicated code from create_service.py
228: Why are root privileges required to *instantiate* an object? Let
the pkg API complain when insufficient privileges are found.
297: Don't raise a new OSError - let the old one propagate!
299-334: Might be worth investigating defining this as an 'abstract base
class': http://docs.python.org/library/abc.html
517: This should be in a 'finally' clause to ensure that CWD is restored
even if a failure occurs.
736: Looks like "self._unmount_iso_image()" should be done just once, in
a 'finally' clause
ai_smf_service.py:
165-169: By setting "props[prop] =" to something, you're modifying the
dictionary that was passed in - a caller may not be expecting that. I
think these lines could be simply removed, and line 170 changed to:
smf.AIservice(smf.AISCF(), pg_name)[prop] = str(props[prop]) # blindly cast to
a str()
On a separate note, is it possible to instantiate the smf.AIservice()
object once, prior to entering the 'for' loop, rather than instantiating
a new one for each property?
339-406: These 3 functions are identical except for the string passed to
smf.AISCF(FMRI=AI_SVC_FMRI).state and the value checked for it. Suggest
consolidating to a single function. (If you're feeling adventurous, I
can show you how to use functools.partial() to achieve this end;
otherwise, defining "MAINTENANCE", "ONLINE", etc. as constants and using
them as inputs to the function will work.
dbbase.py:
The code in this file looks familiar... was this file copied/moved from
elsewhere? If so, before pushing, can you tell 'hg' that it was moved or
copied from another file? (Did not review this since I can't tell what,
if anything, was modified in this file)
grub.py:
327: Rather than _SPACE and _TAB, could you use "\s", or "[ \t]" (a
literal space followed by \t to represent TAB)? That would make these RE
constructs more readable, or at least more consistent with other regex
statements.
339/345: Changing the 'for' loop on line 325 to "for line_num, line in
enumerate(menu_lst_content):" would give you the line number without
requiring a call to menu_lst_content.index().
mount_service.py:
127-130: To prevent errors from the Popen call from bypassing the
null_handle.close() call, use the "with" syntax:
with open("/dev/null", "w") as null_handle:
cmdout, cmderr = Popen(cmd, stdout=null_handle,
stderr=null_handle).communicate()
The null_handle will be closed automatically when leaving the 'with' block.
installadm_common.py:
142: As discussed, even though this is temporary, it can easily be
replaced with: subprocess.check_call()
targetdir.py:
usr/src/lib/install_target/zfs.py now has a ZFS dataset class that may
be leveraged in many of the ZFS functions here.
169: I think the check won't catch correctly if you the cwd is a subdir
of target path. Something like:
if os.getcwd().startswith(target_path):
would catch that case. (There's also a slim chance of symlinks making
the target path look different from the cwd; if that's a concern than
some manipulation from os.path may be in order).
199, 215: There's a lot of raising, catching, re-raising here - is it
all truly necessary, or could some of this be trimmed, in favor of
letting the Python exceptions bubble?
Test code:
Not specifically looked at, though I can take a look if you have
specific areas you'd like reviewed, questions about approaches /
coverage, etc.
On 12/ 2/10 01:08 PM, [email protected] wrote:
This is preliminary code review request for a selection of code for
the Install Service Image Management project.
The ISIM project design document can be found here:
http://hub.opensolaris.org/bin/view/Project+caiman/AI+Image+Management
The webrev can be found here:
http://cr.opensolaris.org/~joev/ISIM_2_DEC_2010/
Please provide feedback by the two weeks from today, Thursday
16.Dec.2010.
For this round of review input please focus on only the following bits:
ai_image:
---------
This code provides a single interface for managing either ISO or
pkg(5) based AI images.
usr/src/cmd/installadm/utils/ai_image.py
usr/src/cmd/installadm/utils/test/test_ai_image.py
Code coverage is over 80%
dbbase:
-------
This functionality is currently provided in installadm_common.py It
has been moved to a separate module, simplified a little and tests a
have been added.
dbbase provides dictionary style access to both system configuration
files and data in string format. I realize it might be better to
implement this as a Mutable Mapping but it had already been
implemented as derived from dict.
usr/src/cmd/installadm/utils/dbbase.py
usr/src/cmd/installadm/utils/test/test_dbbase.py
Code coverage is over 90%
host_service:
-------------
Currently only a skeleton this module will be used to provide a
centralized access point to resources available on the host server.
usr/src/cmd/installadm/utils/host_server.py
usr/src/cmd/installadm/utils/test/test_host_server.py
Test currently only run on a system with the Install SMF service is
available. I have considered having setUp install a dummy SMF service
so tests can be run on build systems. Any suggestions for running
these tests on the build system are appreciated.
Thank you!
Joe
_______________________________________________
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
