On 10/25/10 3:23 PM, Dave Miner wrote:
Consider this a light review of the non-test code. Intend to review
the manifests as well.
Dave
distro_const/__init__.py: Much of this seems like stuff that would be
shared across apps using the DOC; is there a better way to implement
so that it can be shared?
ai_publish_pkg.py
-----------------
97: I'm surprised we need to use tmp files to pass this around; why
shouldn't this be in the DOC if not provided in the arguments?
This has been fixed in both pre-pkg-img-mod (where the regex is done to
find the version of AI is done) and ai_publish_pkg. We now add to the
DC_LABEL DataObjectDict dictionary.
114,125,129: Shouldn't we be capturing error messages to the log? Or
am I misunderstanding the effect of _NULL here?
If a failure occurs in a checkpoint, we allow the engine to handle it.
For most of the CLI calls made within the checkpoints, we use
subprocess.check_call() to run the command. check_call() raises a
CalledProcessError exception if the exit status of the command is
non-zero. I added a small check to the main dc.py application to check
what's logged in the errsvc module against CalledProcessError. If
that's what occurred, we print the strerror() of the exit code to the
debug log:
09:28:59 === Executing Create ISO Checkpoint ==
09:28:59 Uncaught exception from 'create-iso' checkpoint
09:28:59 Snapshotting DOC to /rpool/dc_ai/build_data/.data_cache.latest
09:28:59 Taking zfs snapshot: .step_latest
09:29:00 Failed Checkpoints:
09:29:00 create-iso
09:29:00 Command '['ls', '/nothere']' returned non-zero exit status 2
09:29:00 No such file or directory <<---- the output added with the
check for CalledProcessError
Also, unrelated to this, the CLI commands are slated to be replaced with
pkg5 API calls in the very near term which will change how this code
looks entirely.
boot_archive_archive.py
-----------------------
300: No Fat Albert in the logs, please :-)
Fixed.
341, others: Stylistically, I think the logs would be more palatable
if SHOUTING was reserved for errors (if anything), not info messages.
Fixed.
boot_archive_configure.py
-------------------------
446: Primary Administrator is going away; this needs to be adjusted to
pick up the changes made in slim_source changeset 861:ccd399d2c6f7
Fixed.
create_iso.py
-------------
97: Need comment here
Fixed.
171: can we either use a symlink or pass a reference to the correct
file via the DOC to create_usb so that we don't have an extra several
hundred MB of copied ISO around (and a simpler create_usb.py that
doesn't have to grope around trying to find its ISO)?
create_usb.py, 125: this really should use some tmpification to a
per-process name so that multiple create_usb's on the same machine
don't collide with each other. Probably something that usbgen should
just handle itself, really...
Fixed to use a tempfile.mkdtemp call to create a unique directory to
build the usb file in.
custom_script.py, 74: /dev/null for stdout and stderr means we don't
capture any output from the scripts in the logs. that seems bad.
Fixed to log both stdout and stderr.
grub_setup.py, 287, 340: this is probably no longer desirable, as
recent CR 6984779 has embedded this in the image
Fixed.
pkg_img_mod.py, 173: sure we don't want to capture output to at least
the detailed log?
Fixed.
pre_pkg_img_mod.py, 254: I'm a little confused here. If the vi->vim
symlink exists and vim is there, why replace it? I would assume the
reason to alter the link is when vim is not present (in which case, it
might make sense that it was run always, not just for AI). Note that
implementation of the mediated links proposal
http://mail.opensolaris.org/pipermail/pkg-discuss/2010-October/024604.html
would probably obsolete this problem, anyway.
Fixed.
The code now uses this logic:
If the vi -> vim symlink exists, do nothing.
elif the vi -> /usr/has/bin/vi symlink exists, do nothing
else: # no symlink
if vim exists, set /usr/bin/vi -> /usr/bin/vim
elif /usr/has/bin/vi exists, set /usr/bin/vi -> /usr/has/bin/vi
else: log an error in the log file.
distro_const.py
---------------
159 + others: perhaps put the log files into private per-run subdir of
/var/tmp so that collisions don't happen with multiple users on same
machine
The logging has been completely redone to take advantage of Ginnie's
logging module and use the log file she was already creating (with DEBUG
set). The default log file uses the pid of the process in its name so
we're protected there.
236: commensurate, perhaps?
Fixed.
477: since we're only accepting datasets now, seems like we can close
bug 3947?
_______________________________________________
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
