Hi Darren and Matt.
Here is part of of my AI-CUD review. It does not include tests. Also
it does not include target_selection.py as I saved the best for last :)
I plan on doing target_selection.py tomorrow...
I hope this isn't too late. I got stuck last week with last minute
urgent Derived Manifest stuff...
General question: Where is add_drivers tag handled? I couldn't find it
in my child of the AI-CUD gate, and I cannot get to the gate referenced
in the code review. Is it saved for later?
publish_manifest.py:
--------------------
OK sans DMM changes*
Makefile.targ:
--------------
OK sans DMM changes*
*AI-server changes I made for Derived Manifests project went back to
slim_source later and so aren't reflected here.
auto-install/Makefile:
----------------------
Is there a .po file which gets built? Building of the .po was removed
from this Makefile. Is it elsewhere?
auto_install/__init__.py: OK
-------------------------
ai_get_manifest.py:
-------------------
49-50: This kind of thing is good to have defined in a common file as it
is used all over. (I moved this into install_common in my push. You
may consider using it from there.)
73, 828, 829: While on the subject of common stuff, I suggest moving a
definition of /var/run to install_common as well so it is not
duplicated, then reference that definition from everywhere. That way if
it changes again, you need to change it only in one place.
ai_instance.py: OK
---------------
ai_sd.py:
---------
161: same comments about /var/run as for
ai_get_manifest lines 73, 828, 829
auto-install.py: OK
----------------
auto_install.py:
---------------
188, 225: It may be clearer to call "profile" as in options.profile
something other than "profile" as one may confuse that with SC
profiles. What do you think?
236-238 and 243: this can be replaced with
fl = linecache.getline(profile, 1)
See dmm.py for an example. Thanks to Drew for pointing this out during
DM code review.
490: Nit: successful is spelled with one ell.
527, 545: I don't understand the logic here. Why are these messages
printed only when there are no checkpoints?
850, 855, 864, 867, 870, 878, others?: set up another shared definition
in install_common for /var/sadm/system. Use the one proposed earlier
for /var/run.
956: What do 10000 and 30000 represent? Please add a comment.
1011, 1012: What does 524288 represent? Please add a comment.
checkpoints/Makefile: OK
---------------------
config/Makefile:
----------------
Not sure it's worth including this file in the review when the only
change other than copyright is removing one blank line... ...but OK.
auto-install/default.xml: OK
-------------------------
svc/Makefile:
svc/auto-installer.xml:
----------------------
No changes other than copyright. Suggest deleting from review and push.
manifest-locator:
-----------------
Continuing with common definitions implementation, if /var/run/ai.xml is
defined in in install_common like this:
AI_XML="/var/run/ai.xml"
you could replace in manifest-locator
AI_MANIFEST=/var/run/ai.xml
with
AI_MANIFEST=`python -c "import solaris_install.install_common;
print AI_XML"`
66: similarly for AI_SERVICE_LIST
Other defs too?
utmpx.py:
---------
58: Too bad there isn't a way of passing pid_t to something which
magically translates it to a uint. Else if it changes and breaks
something, it will be a nightmare to figure it out...
users_on_console() and the main look remarkably similar. Can they be
combined?
distro_const/checkpoints/pre_pkg_img_mod.py: OK
--------------------------------------------
installadm/__init__.py:
installadm/Makefile:
-----------------------
Just curious: why is __init__.py needed?
install_common/__init__.py:
---------------------------
370: Defer to previous comments about shared paths and other definitions.
ai.dtd:
-------
This appears to be an artifact of previous gate state (pre-SC-profiles)
and is no longer relevant. Is this true?
system-install-auto-install.mf: which package do the DTDS go back with
now? They have been removed from here, and not added to the other pkg
manifest of this code review.
Thanks,
Jack
On 04/22/11 10:25 AM, Darren Kenny wrote:
Hi,
I'd like to ask for help in reviewing the code for the moving of the Auto
Installer client to the CUD architecture.
Some light reading for the weekend ;)
As Drew pointed out, we're adding less code than we're removing - that has to be
positive, doesn't it??
I'm going to schedule a code review for 8am PDT/4pm IST on Tuesday 26th, I'll
send out details closer to the date.
The webrev is at:
http://cr.opensolaris.org/~dkenny/cud_ai-to-slim/
If at all possible, I would like to get he bulk of the feedback by COB Friday
May 29.
Thanks,
Darren.
_______________________________________________
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
