Hi Ethan,
My apologies for this not being 100% thorough but hopefully this
can reach you before you take your break -- enjoy the time off.
Thank you,
Clay
Looking at publish-manifest.py:
(I'm troubled a bit by extending the functional approach in
find_colliding_manifests and extending the way too complex
DataFiles class -- but thank you for breaking some of the
criteria code off.)
line 59: using %prog with the program name as publish_manifest.py will
cause installadm to print the usage when something goes awry as
# installadm add ...
usage: publish_manifest.py -n service_name -m AI_manifest...
Please hardcode "add" instead of "%prog" or please use a hardlink
from /usr/lib/installadm/add -> /usr/lib/installadm/publish_manifest.py
so that error output isn't yet more confusing to the user
line 86/87: -m/-n have their meanings reversed in the comment
lines 108, 116, 126, 135: these should probably use the parser.error()
function since these are errors related to commandline
processing. Further, the exceptions from criteria_to_dict which
are due to mal-formed options should probably be caught and
passed to parser.error() as well.
line 232-234: The None for criteria2 is extraneous and should be omitted.
line 304: two spaces between "," and "strip"
line 374: The call to getManifestCriteria() here only gets the first
instance of the criteria manifest; why check only the first? If
the concept of manifest instances are going away with this fix,
please strip that code and database column out of AIdb, et al.
lines 403-407: this comment is confusing. I think there's a switch of the
manifest criteria for criteria from the AI database?
lines 577-591: Again, if we are getting rid of the idea of manifest
instances please get rid of the code supporting them; this is
code which happens when publishing a manifest with a new criteria
set.
line 594-600: This can be simplified to:
[tag.getparent().remove(tag) for tag in root.xpath('/ai_criteria')]
or:
for tag in root.xpath('/ai_criteria'):
tag.getparent().remove(tag)
line 604: ioerr seems a bit odd to me; how about msg?
line 609-611: this is a false sense of security as one can still go to
http://localhost:46501 and read these. Further, we should not be
running the AI webserver of today or tomorrow as root which would
make these unreadable in such a change.
line 648-677: Unfortunately this can cause line numbers to be off, since
we are manually hacking up the XML DOM. I.e. if I have a legacy
manifest it can be akin to:
<ai_criteria_manifest>
<ai_embedded_manifest>
<ai_manifest name="foobar">
[...]
</ai_manifest>
</ai_embedded_manifest>
<ai_criteria name="MEM">
<range>512 1024</range>
</ai_criteria>
<sc_embedded_manifest name="foobar_config">
[...]
</sc_embedded_manifest>
<ai_criteria name="IPv4">
<value>192.168.1.breakme</value>
</ai_criteria>
</ai_criteria_manifest>
Which would result in an error on line 6 while the admin. will see
the error on a line likely greater than 130 -- and won't be able
to use xmllint(1) to figure out what's up since the schema
doesn't validate for what the admin has anymore.
Please use a wildcard definition and add optional
ai_embedded_manifest and sc_embedded manifest tags in place so
the admin. has a hope of understanding where an error occured --
also then you don't have to strip anything out of the DOM either.
This will allow the validator to ignore anything between those
tags.
To see how to implement a wildcard for the sc_manifest tags (and
the ai_manfiest will be the exact same) please see criteria.rng
in the following webrev:
http://cr.opensolaris.org/~clayb/15998/
line 721-722: truncated comment
line 786: indent problem (should be out 4 spaces)
line 754: I'm not sure where db_file is coming from?
lines 1089-1097: Are we supporting multiple SC manifests any more (did
the client ever)? If not, please remove that functionality from
publish_manifest with these changes.
lines 1152-1177: Can't all the SystemExits here be entrusted to the
criteria schema to check this? (There's a lot of "by hand" XML
validation going on here which seems to defeat using XML.)
lines 1361-1362: Some comments explaining the interesting newline appends
would be helpful
lines 1425-1433: Again this could be simplified to one line or a two line
for loop
On Thu, 5 Aug 2010, Ethan Quach wrote:
Hi all,
The following is the code review for the AI manifest schema changes,
and the installadm criteria changes. It is a rather large review, so
partial/piecewise review would be also be fine, just let us know what
you're reviewing. We've pre-requested reviews from some of you
already, but all comments welcomed by Aug 16th.
Webrev:
-------------
http://cr.opensolaris.org/~equach/webrev.ai-schema/
Bugs:
--------
16423 Updates to AI schema should be made
15449 installadm add validates combined manifest against image-specific schema
as well as
schema in /usr/share/auto_install/
6975043 separate criteria and ai manifest
thanks,
-ethan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
