I have a few more comments below..
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 destination is not required, then of course img_action isn't
required, because the user doesn't have to specify an image.
I think this is also ok.. This is how I see the possible scenarios:
Assuming default is:
'create image at default location'
-User wants to use default action(create):
No destination specified. Default action is to create an image at the
default location.
-User wants to specify the image location. Use default action(create):
Destination is specified and action is to create a new image at this
location.
-user wants to use an existing image. Must specify img_action:
<image img_root="/export/home/foobar/image" img_action="use_existing">
This will result in clearing the image of all installed packages and
reinstalling the defined list. Does not re-create image.
-User wants to update an already created image, must specify img_action:
<img img_root="/export/home/foobar/image" img_action="update">
This will result in the addition of specified packages to this image.
this should work right, which makes destination and img_action optional.
sarah
***
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
