On 12/21/10 08:32 AM, William Schumann wrote:
Requesting code review for enhancements for service configuration profiles in
the Automated Installer.
http://cr.opensolaris.org/~wmsch/profile/
The code can be divided into sections:
UI - installadm invokes UI, modifications in database routines, since profiles
are now in a separate table.
CGI - runs under AI image-server under Apache, locate_profile, ai-httpd.conf
AI client - modifications to ai_get_manifest, manifest-locator,
auto-install.c/h, ict.py
package modifications for new PY modules, CGI script: install-installadm.mf
automated tests
Modified AI_database.py to allow many functions to specify database table.
Should default to 'manifest'. Changed AI_database.py message output to stderr,
which becomes significant for CGIs, where stderr goes to the error log and
stdout should be only for intended output so that message header output is
properly sequenced. Tried to fix a problem that appears when there is no
criteria in the manifest database in build_query_str().
Design document:
http://hub.opensolaris.org/bin/download/Project+caiman/System+Configuration+Project/SCProfileProposal.odt
Authentication and user-defined scripting not supported in this release.
Decided to back out support for XInclude for the time being, problem being that
our best attempt at specifying when and if XInclusions are to occur depends on
modifications to service_bundle(4) DTD, and it still needs approval by SMF. Some
artifacts of this support remain in the source, since approval is expected.
Have done unit testing on AI microroot, but have not built or tested
auto-install image. Will do so next, in concurrence with code review.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi William,
For all files, please update copyright to 2011.
These are comments on the files I have reviewed so far.
AI_database.py
343 update comment
354 Is there a reason for adding the extra space at the end of query_str?
423 Why have you removed lines 422-425 (old line numbers)?
431 Can getProfileCriteria be combined with getManifestCriteria? A lot of this
code is duplicated.
ai-webserver/Makefile
34-35 Please alphabetize
47-48 alphabetize and perhaps place in list ala PYMODULES. Or, at least
split into 3 lines so you aren't over 80 cols.
publish_manifest.py
230, 304, 306 In find_colliding_criteria, "table" is passed in 3 calls to
AIdb.getCriteria, but it doesn't appear to be defined anywhere.
614-622 please update comment and arg list with the argument you added
679 ditto
list.py
34-37 please alphabetize each of the two groups of imports
783-841 This new function, get_profile_names, is very similar to existing
get_manifest_names. Is there a way to combine them?
785-797 Update comments to reflect profiles, rather than manifests
823 remove commented out code
1000-1062 Can get_service_profiles be combined with get_service_manifests to
avoid duplicate (or very similar) code?
1014 line is longer than 80 chars. Have you run pylint on these files?
1025 manifest->profile
1114 Update comments to reflect use for both manifests and profiles
1271-1272 Don't need the "is True"
1272 The case of OPTIONS.client and OPTIONS.profile (but no OPTIONS.manifest) is
not printing the blank line between the listings.
installadm.c
136 create-> create-profile (new subcommand, no need for alias)
141 delete-> delete-profile (new subcommand, no need for alias)
1579 No option parsing for set-criteria should be done here. It should all be
handled in set_criteria.py. In fact, I was expecting that the changes to support
profiles in the set-criteria subcommand would also be in set_criteria.py, rather
than a new set_profile.py module.
Thanks,
Sue
set_profile.py
See my previous email about set-profile. Didn't review this file.
installadm.c
135 create-> create-profile (new subcommand, no need for alias)
140 delete-> delete-profile (new subcommand, no need for alias)
151 and 156 Same thing for validate and export
149 and 154 These subcommands shoudl be simply "validate" and "export"??
1378 Same question as above about new set-profile subcommand.
common_profile.py
General:
o For all functions, please have a consistent block comment that
includes Args, Returns, Raises.
o For output to stderr, please use "_" notation for translation
26-33 group imports together, then froms
45,46,164 (etc, please address all of them) add space after '#'. Also add
a couple of spaces before the # to distance the comment a bit.
58 checkIPv4 seems very similar to __checkIPv4 in verifyXML. Any way not to
duplicate this code?
96 Will verify -> Verifies that
116 show-off :)
136 Same comment/question for MACaddress vs. __checkMAC
138 checks an MAC address string, that it has six hex
->
Checks that a MAC address string has six hex
152 Isn't this almost exactly the same as format_value in set_criteria? Can that
be used instead?
193, 200, 205, 251 Use err instead of 'e'. Pylint should complain about this,
have you run it?
201 making -> creating
207, 253 don't need parens
210 line up 0644 under 'logfile'
215 making -> initializing
273 only -> The only
299 Remove spaces before/after = per pep8 guidelines
471, 473 add spaces before and after '+' inside []
477 Do you want to leave this debug statement here? If so, do you want to also
log the results?
756 validate_criteria_from_user seems very similar to find_colliding_manifests
in publish_manifest.py. Can we avoid this duplicate code?
create-profile.py
37 add blank line between external and solaris modules. Also, group imports and
from's together and alphabetize
71 line up "name" under "If"
93, 98, 138 (and other output to stderr) Please use "_" notation for translation
101 add space after '#' (lots of these throughout file)
87-124 a few blank lines would improve readability
166-270 same
203 "Name for profile" -> "Name of profile"
203 Since you are looping through possible multiple profiles, it would be good
to tell the user which name already exists
212 Why use .format here? Would it be more consistent with our existing code to
have:
"I/O error(%s) opening profile %s: %s" % (errno, profile_file, strerror)
270 Since you give errors for profiles with "issues", you also might want to
give an "adding profile xxx" message for successfully added profiles so that the
user who has tried to add multiple profiles with varying success will clearly
know what has been added and what hasn't.
275 Slight reword suggestion: "Error:\tRoot privileges are required for "
"this command."
export_profile.py
100 Slight reword suggestion: "Error:\tRoot privileges are required for "
"this command."
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
