On 08/09/10 18:18, [email protected] wrote:
> 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
Hi Clay,
Ethan wasn't able to get to your comments prior to leaving, so I'm responding to
them.
> 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
Will change to print out add-manifest (and will do similar change to print
set-criteria vs. set_criteria.py)
> line 86/87: -m/-n have their meanings reversed in the comment
Will fix.
> 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.
Will change.
> line 232-234: The None for criteria2 is extraneous and should be omitted.
ok
> line 304: two spaces between "," and "strip"
ok
> 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.
Removing the concept of manifest instances is really outside the scope of this
project.
> lines 403-407: this comment is confusing. I think there's a switch of the
> manifest criteria for criteria from the AI database?
Will reword.
> 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.
Similarly outside the scope.
> 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)
Will change (using '/ai_criteria_manifest/ai_criteria')
> line 604: ioerr seems a bit odd to me; how about msg?
will use err to be consistent with other exceptions
> 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.
This was considered a step forward that, while not solving all issues, didn't
hurt anything. If you feel strongly, I could revert this 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/
This raises a good point, but I'd like to open a bug, wait for Ethan to return,
and then address this as soon as possible in an upcoming build.
> line 721-722: truncated comment
ok
> line 786: indent problem (should be out 4 spaces)
ok
> line 754: I'm not sure where db_file is coming from?
I don't see db_file on line 754. Were you maybe looking at the *old* file? In
that file, there is db_file on 754 - is that what you're looking at? If so, it
was changed to serv as you can see in the webrev.
> 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.
I would prefer to leave this alone as this is outside the scope of this project.
> 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.)
Removed two of the SystemExits per Keith's comments on SystemExits and the
others were changed to ValueErrors. The remaining checking seems pretty much in
line with what was being done before.
> lines 1361-1362: Some comments explaining the interesting newline appends
> would be helpful
ok
> lines 1425-1433: Again this could be simplified to one line or a two line
> for loop
Will make similar change as 594-600.
Thanks for reviewing,
Sue
>
> 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
