Jack,
You've done a whole lot of great work here. Thanks for cleaning up the
XML verification parameters
and doing the pylint stuff.
Comments are below.
Sorry for the delay,
John
================================================================================
usr/src/cmd/ai-webserver/AI_database.py
================================================================================
Looks fine.
================================================================================
usr/src/cmd/ai-webserver/cgi_get_manifest.py
================================================================================
1. Should the following be changed?
194 servs_port =
serv[PROP_TXT_RECORD].partition(':')[-1]
To be like lines at:
186 if PROP_TXT_RECORD in serv.keys():
187 port =
serv[PROP_TXT_RECORD].partition(':')[-1]
Or should the check be removed from line 186? If it should be
removed then
things need to change at line 213 and 214 of the new code.
2. I would change line 224 from:
224 if not servicename:
To:
if found_servicename:
3. I think you should probably add found_servicename to the exception
output at
lines 249-259.
================================================================================
usr/src/cmd/ai-webserver/delete_manifest.py
================================================================================
1. You might consider putting the "%s" in parentheses:
117 raise SystemExit(_("Error:\tCannot delete default
manifest %s.") %
118 man_name)
Otherwise we might end up with a default manifest default.xml as the
output
which to me looks a little odd.
2. Also the webrev complains that the file is executable. That should
probably
be changed.
executable file: mode 755
================================================================================
usr/src/cmd/ai-webserver/publish_manifest.py
================================================================================
1. Please put the imports in alphabetic order:
30 import gettext
31 import logging
32 import lxml.etree
33 import os.path
34 import StringIO
35 import errno
36 import sys
37 import pwd
38 from stat import S_IRUSR, S_IWUSR
39 from optparse import OptionParser
40
41 import osol_install.auto_install.AI_database as AIdb
42 import osol_install.auto_install.verifyXML as verifyXML
43 import osol_install.libaiscf as smf
44 from osol_install.auto_install.installadm_common import
validate_service_name
45 from osol_install.auto_install.properties import set_default,
get_service_dir
46 from solaris_install import Popen, CalledProcessError, _,
AI_DATA, \
47 KSH93_SHEBANG, PYTHON_SHEBANG
According to former members of the team this is a pep8 requirement.
2. Parentheses are unnecessary:
151 if (do_add):
3. Should there be an PROP_IMAGE_PATH defined in installadm_common.py like
there is for PROP_TXT_RECORD? This would then be used from lines like:
181 image_path = svc['image_path']
4. Since criteria_file and set_as_default are not changing you should really
just use the thing passed into the application so that it is easier
to tell
that these were passed in rather then generated within the code:
196 files = DataFiles(service_dir=service_dir,
image_path=image_path,
197 database_path=os.path.join(service_dir,
"AI.db"),
198 manifest_file=options.manifest_path,
199 manifest_name=options.manifest_name,
200 criteria_dict=criteria_dict,
201 criteria_file=criteria_file,
202 service_name=options.service_name,
203 set_as_default=set_as_default)
5. Please use shutil.copyfile() instead of the Popen method you are
using within
the _copy_file() function (shutil.copyfile('/tmp/original',
'/tmp/copy')).
6. This:
646 if root is not None:
can be changed to simply:
if root:
7. Also the _copy_file() is not needed if you use shutil.copyfile() as the
only other thing within _copy_file() is the chown/chmod which could
be done
for all cases after the script copy else case around line 667 or 668.
8. This:
717 if errors is not None:
Can be changed to simply:
if errors:
9. The same for line 788. These are then consistent with lines 1393 and
1404.
10. The same for line 1461.
================================================================================
usr/src/cmd/ai-webserver/set_criteria.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/ai-webserver/verifyXML.py
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/Makefile
================================================================================
Looks good.
================================================================================
usr/src/cmd/installadm/delete_service.py
================================================================================
1. line 640 you disable pylint message W0612 for the unused files
variable but
never re-enable the warning. This will cause pylint to ignore unused
variables the rest of the file. So add:
# pylint: enable-msg=W0612
after the for loop.
================================================================================
usr/src/cmd/installadm/installadm.py
================================================================================
1. Seems kind of arbitrary at this point to have multiple from/import
statements from the same file and then have a single from/import for
other
things:
35 from osol_install.auto_install import create_client
36 from osol_install.auto_install import create_service
37 from osol_install.auto_install import delete_client
38 from osol_install.auto_install import delete_manifest
39 from osol_install.auto_install import delete_service
40 from osol_install.auto_install import list as ai_list
41 from osol_install.auto_install import publish_manifest
42 from osol_install.auto_install import set_criteria
43 from osol_install.auto_install import set_service
Verses:
44 from osol_install.auto_install.ai_smf_service import \
45 PROP_STATUS, STATUS_OFF, InstalladmAISmfServicesError, \
46 check_for_enabled_services, enable_install_service, \
47 get_pg_props, is_pg, set_pg_props
48 from osol_install.auto_install.installadm_common import \
49 CHECK_SETUP_SCRIPT, SERVICE_DISABLE, SETUP_SERVICE_SCRIPT, \
50 validate_service_name
I know you're just following what's already there.
2. why use Pipe(/usr/bin/cat manifest) instead of:
with open(manifest) as man_fp:
manifest_data = man_fp.read()
print manifest_data
3. Could use the not version for:
269 if (options.service_name is None):
Becomes:
if not options.service_name:
In either case loose the parentheses.
4. Similar comment for:
274 if ((options.mname is None) and (options.pname is None)):
becomes:
if not options.mname and not options.pname:
5. Similar comment for:
279 elif (options.mname is not None):
becomes:
elif options.mname:
6. Also loose the parentheses around:
286 if (options.mname == "default"):
7. Could just check for value for:
295 if options.output_name is not None:
becomes
if options.output_name:
8. Why not simply use shutil.copyfile() instead of Popen(/usr/bin/cat
manifest)
at lines 295-312?
================================================================================
usr/src/cmd/installadm/list.py
================================================================================
1. Change to a simple check:
965 if criteria is not None:
becomes:
if criteria:
2. You can loose the parentheses:
1031 if (cwidth > 0):
3. Change to a simple check:
1050 if default_mfest is not None:
becomes:
if default_mfest:
================================================================================
usr/src/cmd/installadm/properties.py
================================================================================
1. I think we go back and forth in how we build paths within our group.
Sometimes we use os.path.join and other times we do what you are
doing below:
86 manifest_path = service_dir + "/" + AI_DATA + "/" +
manifest_name
You should probably change it as you use os.path.abspath() at lines
52 and
73.
2. You should probably use the DEFAULT_MANIFEST_PROP for the following
raise:
108 raise KeyError(_("default-manifest property is missing "
109 "from service %s" % service_name))
================================================================================
usr/src/cmd/installadm/set_service.py
================================================================================
1. You should import DEFAULT_MANIFEST_PROP from properties then use it
in the
list assignment at line 37:
37 SERVICE_PROPS = ["default-manifest"]
2. Please change:
79 if options.propval is None:
to:
if not options.propval:
3. The parsing of the propval option does not seem consistent with the
usage statement:
44 'set-service\t-o|--option <prop>=<value> ... <svcname>\n'
45 '\t\tprop=value can be:\n'
46 '\t\t\tglobal-menu=true|false\n'
47 '\t\t\tname=<new_svcname>\n'
48 '\t\t\talias=<alias_name>\n'
49 '\t\t\tdefault-manifest=<manifest/script>'
The usage statement seems to indicate that the --option can be multiple
options (i.e., -o global-menu=true -o alias=somethingelse).
4. Also I am not quite following the logic here. When and/or how does
SERVICE_PROPS get updated?
================================================================================
usr/src/cmd/installadm/setup-service.sh
================================================================================
Looks good.
================================================================================
usr/src/lib/install_common/__init__.py
================================================================================
1. Please turn pylint message W0212 back on after line 302.
299 # pylint: disable-msg=W0212
300 popen.stdout, popen.stderr = popen._log(logger,
log_bufsize,
301
stdout_loglevel,
302
stderr_loglevel)
================================================================================
usr/src/pkg/manifests/install-installadm.mf
================================================================================
1. Not real keen on the name space management within /usr/sbin for AI stuff:
112 link path=/usr/sbin/publish_manifest
target=/usr/lib/python2.6/vendor-packages/osol_install/auto_install/publish_manifest.py
perhaps something like ai_publish_manifest. publish_manifest seems too
generic and that it should work for other manifest containing stuff.
On 03/17/11 04:52 PM, Jack Schwartz wrote:
Hi everyone.
Here is the AI server part of the Derived Manifests project.
http://cr.opensolaris.org/~schwartz/110317.1/webrev/
Please review by Weds 3/23 COB, and please let me know you plan on
reviewing.
John F., I would like you in particular to review this stuff as the
changes of default manifest management are right up your alley.
If anyone wants a 1-1 to go over this stuff please let me know.
There will be one more small code review forthcoming regarding unit
tests for this stuff, but I didn't want to hold this up for it. That
said, I have good results having tested by running many installadm
commands manually.
Read on if you're curious about what this wad brings to our product:
1) Gets rid of a hard-wired default file called default.xml. It now
allows any manifest in a service's manifest pool to be designated as
the default. Any criteria belonging to a manifest designated as a
default are ignored. Any manifest not containing criteria and not
designated the default is noted as inactive.
2) There is a new installadm export command
3) There is a new installadm update-manifest command
4) Refactored duplicate code which got the correct service directory,
into a single routine.
5) Removed cruft from publish_manifest
6) All files I touched are mostly if not completely pep8 compliant.
(There are some temporary XXX which mark placeholders.)
Thanks for your time.
Jack
_______________________________________________
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
