Hi John.
I really appreciate your review...
On 03/22/11 03:00 PM, John Fischer wrote:
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.
I'll leave the check in at 186 and add it at 194, for two reasons:
1) The check was there in 186 before; all I did was change 'txt_record'
to PROP_TXT_RECORD as it is elsewhere.
2) if for some reason the PROP_TXT_RECORD key is missing in serv at 194,
the check at 202
( if not path...)
will catch the error as it already does for 186.
2. I would change line 224 from:
224 if not servicename:
To:
if found_servicename:
Yup. That's better... Fixed.
3. I think you should probably add found_servicename to the exception
output at
lines 249-259.
OK. I changed the end of the output from this:
print 'criteria =', criteria, '</ol>'
to
print 'criteria =', criteria
print 'servicename found by port =', str(found_servicename), '</ol>'
================================================================================
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
Changed to 0644 like the rest.
================================================================================
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.
Done.
2. Parentheses are unnecessary:
151 if (do_add):
() removed.
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']
I now import PROP_IMAGE_PATH from ai_smf_service and use it instead.
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)
Done!
5. Please use shutil.copyfile() instead of the Popen method you are
using within
the _copy_file() function (shutil.copyfile('/tmp/original',
'/tmp/copy')).
Great idea. Done!
6. This:
646 if root is not None:
can be changed to simply:
if root:
Yup. Changed.
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.
I'd like to leave _copy_file() in place, as shutil.copyfile still needs
to be enveloped in a try/except, and the chmod and chown together with
the copy are a significant group of statements.
8. This:
717 if errors is not None:
Can be changed to simply:
if errors:
Yup, changed.
9. The same for line 788. These are then consistent with lines 1393
and 1404.
Changed.
10. The same for line 1461.
Changed.
================================================================================
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.
No need: "pylint disable" honors the scope/nesting-level it is in, so it
will affect only the remainder of the "else" clause it is in (which is
the loop itself).
================================================================================
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.
I believe you are the second person to comment on this... It doesn't
look consistent, but what is really happening is that the single line
"from xxx import yyy" statements import files, whereas the "from xxx
import aaa,bbb,ccc" statements import defs from a particular file. In
order to make the difference of importing files and defs within a file
more pronounced, I will change the
from xxx import yyy
to
import xxx.yyy as yyy
example:
from osol_install.auto_install import create_client
becomes
import osol_install.auto_install.create_client as create_client
2. why use Pipe(/usr/bin/cat manifest) instead of:
with open(manifest) as man_fp:
manifest_data = man_fp.read()
print manifest_data
Actually, I heeded your note further below, to use shutil.copyfile
instead....
On a somewhat related note, the CAT definition in installadm.py is
leftover cruft from before I put export in its own file, so I will
remove that definition.
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:
Changed all of these in the whole file!
6. Also loose the parentheses around:
286 if (options.mname == "default"):
Changed.
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?
Thanks for the suggestion. This makes for (IMHO) a *really* elegant
solution, which, except for the try/except, reduces to three lines...
Get a load of this:
def do_export(cmd_options=None):
''' XXX Will merge with William's do_export.'''
options = parse_options(cmd_options)
if not options.output_name:
options.output_name = "/dev/tty"
# Find the file in the directory and cat it out.
try:
shutil.copyfile(options.mname,options.output_name)
except IOError as err:
print >> sys.stderr,
"Error exporting manifest: %s: %s" % (
err.strerror, err.filename)
sys.exit(err.errno)
Isn't it beautiful?
================================================================================
usr/src/cmd/installadm/list.py
================================================================================
1. Change to a simple check:
965 if criteria is not None:
becomes:
if criteria:
Fixed.
2. You can loose the parentheses:
1031 if (cwidth > 0):
Yup. Changed.
3. Change to a simple check:
1050 if default_mfest is not None:
becomes:
if default_mfest:
Done.
================================================================================
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.
The application is slightly different so abspath isn't really correct.
However, os.path.join is used elsewhere and makes for code more elegant
than the original. Changed to:
manifest_path = os.path.join(service_dir, AI_DATA, manifest_name)
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))
Changed to:
raise KeyError(_("\"%(prop)s\" property is missing "
"from service %(serv)s") %
{"prop": DEFAULT_MANIFEST_PROP,
"serv": 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"]
Changed to
SERVICE_PROPS = [DEFAULT_MANIFEST_PROP]
2. Please change:
79 if options.propval is None:
to:
if not options.propval:
Done.
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).
This is true, but the other items are on the way.
For now, I'll leave the other option statements but just comment them out.
4. Also I am not quite following the logic here. When and/or how does
SERVICE_PROPS get updated?
SERVICE_PROPS is a list of valid properties that can be passed to
set-service. As handling for new properties get added, this list gets
updated (by the programmer).
================================================================================
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)
pylint disable statements honor their scope so re-enabling is not
necessary here. (See previous comments.)
================================================================================
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.
OK. Changed here to ai_publish_manifest, and in setup-service.sh which
references it.
Thanks,
Jack
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
