Be sure to read past what looks like the end of my comments below.
Sorry. I closed this email out and then added stuff to the end.
sarah
****
On 08/ 9/10 06:20 AM, Sarah Jelinek wrote:
Hi Jean and Dermot,
Some comments inline..
On 08/ 9/10 04:32 AM, Dermot McCluskey wrote:
Hi Jean,
Thanks for reviewing.
On 08/06/10 21:34, [email protected] wrote:
I looked at the following files:
ai_manifest.xml:
line 152, 158: Sarah's design calls for type to be "ips" not "IPS"
(My assumption being that Sarah's spec meant this, if we're going to
make this case insensitive it is fine by me, I do need to know
though and it should be called out in her spec)
Sarah changed this in her spec recently to all upper-case values at my
request. The client calls the DDU library, passing this value as one of
the params and the lib expects them to be upper-case, so it was more
convenient to have them in uppercase in the manifest.
line 176: "p5i" not "P5I"
I would expect a <name>"location of p5i file</name> after
this.
Or is that something that is expected will be filled in by
the application?
The actual location of the P5I file is stored in the corresponding
source/publisher/origin/name, ie line 173.
line 184-186: How come you used publisher here and not dir? I think
of publisher and origin as more IPS related and the dir path="" as a
directory like you have here.
The comments in software.dtd say:
Origin can be an uri, path to a file, archive, directory.
Origin is also being used to store details of SVR4 pkgs as well
of IPS. The <dir> element is not for directories containing
SVR4 pkgs, it's for something else (I can't remember what, off hand.)
directories to be used for things like cpio'ing.
line 188: "svr4" not "SVR4"
line 198-200: same as 184-186
line 202: "du" not "DU"
line 222: This is an ending software with no beginning. There is a
searchall above that looks like it goes with it but...
You are correct. Will fix.
Thanks for fixing this. It is in a comment block so wouldn't have been
caught by validating it. We also need to fix:
<searchall/> to </searchall>
thanks,
sarah
- Dermot
ai_parse_manifest.py:
Looks fine to me.
cmd/auto-install/default.xml:
line 34 & 40: Same capitalization comment for type.
software.dtd.html:
line 42: Capitalization comment from above.
lines 56-59: Sarah and I discussed just 2 days ago adding
<!ATTLIST image img_action (create|use_existing|update)>
here. You should ask her about this. This would make the destination
required in the schema for an image so it might have bigger impact
than you think.
This is something we discussed but I hadn't added it to this code yet.
This discussion happened after we froze the code for this review.
I am not sure that destination will be required with these changes. I
thought about this some more after we talked.
The img_action attribute is required in the new schema. If a user
specifies a create with no destination, then we can assume the default
destination. If a user specifies a use_existing or update wtih no
destination element then this would result in a failure. This is part
of the semantic validation we must do, either in the client or in the
import of the manifest.
If the user specifies a destination with a 'create' action then we
just create the image at the location specified.
I will have to update the software.dtd to reflect the inclusion of the
img_action attribute.
sarah
****
Jean
On 08/ 5/10 04:07 PM, 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 <http://defect.opensolaris.org/bz/show_bug.cgi?id=16423>
Updates to AI schema should be made
15449 <http://defect.opensolaris.org/bz/show_bug.cgi?id=15449>
installadm add validates combined manifest against image-specific
schema as well as schema in /usr/share/auto_install/
6975043 <http://bugs.opensolaris.org/view_bug.do?bug_id=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
_______________________________________________
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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
