On 11/ 4/10 04:41 AM, Dermot McCluskey wrote:
Alok, Keith,
Just one small follow-on comment below.
On 11/03/10 18:59, Keith Mitchell wrote:
Hi Alok & Drew,
Here comes my review for section [2]!
One thing I forgot to check in the prior review was the copyright
notices, but I'll assume that you've run 'hg pbchk' and fixed any
issues from the copyright section.
configuration.py:
In class methods, instead of using
"Configuration.CONFIGURATION_LABEL" it's slightly better to use
"cls.CONFIGURATION_LABEL". This is a minor point in this class, but
becomes more relevant in the next file...
144, 146, 148: Not strictly needed (no harm in resetting to None);
perhaps the __init__ function should accept these as parameters?
Similar comment for the Distro and GrubMods classes from distro_spec,
and Checkpoint from execution_checkpoint.
distro_spec.py:
All the classes like MediaIM, that are "basic" classes (no frills)
could have a common parent that is the following:
class BasicParentClass(DataObject):
LABEL = None
# __init__ not needed; DataObject.__init__ will be used
def to_xml(self):
return etree.Element(self.LABEL)
@classmethod
def can_handle(cls, element):
return element.tag == cls.LABEL # Note the use of "cls"
@classmethod
def from_xml(cls, element):
return cls(cls.LABEL) # Again, note the use of class
The subclasses then all just need to be of the form:
class MediaIM(ParentClass):
LABEL = "media_im"
This could even be used as the parent class for the non-basic classes
in here, with the more complex ones overriding the relevant methods
on an as needed basis.
boot_archive_contents_*.xml:
I'm very glad that we'll have this solution to the boot archive
contents problem!
profile/*.xml - these aren't brand new files are they? When prepping
for the final push, please mark them as moved versions of the
originals, to ensure proper tracking in the mercurial logs. And,
since they're existing files, the copyright date needs to start with
the earliest date of their existence.
I think the same should apply to the default manifests.
dc_ai_sparc.xml:
(Many of these comments will apply to all manifests; please update
accordingly)
35: Nit: Is there a better name for "incremental_media_name" that is
more descriptive? Maybe just "add_timestamp"?
Please be careful if making any adjustments to the already agreed DTD
schemata. Although the particular file that this attribute name comes
from,
dc.dtd, is not yet a published interface, most of the other files such
as ai.dtd, target.dtd, etc are already in use and are, to some extent,
supported
interfaces for Install. And so we should be careful to minimize breaking
backwards compatibility.
Also, in this case, does the "increment" in question need to be a
timestamp?
Or could it also be some other suffix to avoid the ISO file being
overwritten,
eg ....iso.1, ....iso.2, or ....iso.<pid>, etc?
- Dermot
Good point, a change shouldn't be taken lightly (however, if there's a
time for change, it's now, while there's more flexibility).
I like the idea of iso.1, iso.2. The timestamp aspect can be achieved by
looking at the creation date of the iso.
- Keith
I don't know if this happens or not, but it would be convenient to
symlink <distro> to <distro>.<latest timestamp> if this flag is set
to true.
55: See comment from review of section [1] regarding the value of
anything other than "use_existing" in a DC context. Additionally,
since it's an "advanced" feature, I think it's better to remove the
comments on line 45-51 and document it elsewhere. I imagine the vast
majority of users will just be confused at best by the possibility of
using the 'create' actions for zpools/datasets with DC.
57: Would a better default be something like: "rpool/dc/ai"? The
"sparc" doesn't seem needed, and having an extra "level" probably
sets up the default a little more cleanly for most users.
69: Nit: Is the leading slash needed? Might look cleaner without.
94-95: I'm not sure it's good to have anything other than strictly
"example.com" as an example URL. Since the mirrors should be
different, perhaps "mirror.example.com" or "example.com/mirror" would
be appropriate?
112 vs 113: Inconsistent indentation for lines 110-112 compared to
the remainder of the pkg list.
Please prepend all pkg names with "pkg:/"
Please change the package list to match the current AI manifest's
list of "pkg:/entire", "pkg:/system/install/media/internal" and
"pkg:/auto_install"
308: Use example.com
327: This is a relative include, which means when the end user copies
this file out of the installed location to make modifications, the
include will break. Please use an absolute path.
351: The execution sections indentation is rather inconsistent.
403: Is this list guaranteed to be stable, or will we run into
end-user maintenance issues similar to the boot archive files list
down the road?
431: Can we try moving the ai-publish-package stage to be the final
step? I don't think that will break anything, and it makes it
slightly easier for someone to skip that step if they want to save
time (they can just use the -p command at the CLI rather than editing
the manifest).
467-469: Looks like it needs an update
Other default manifest notes:
- Glad to see that we're finally losing the other, unused live CD
manifest.
dc_livecd.xml:
106: Set to pkg.oracle.com before putback (double-check other
publishers to be sure, as well)
139-150: Please update to use the following four packages:
pkg:/entire
pkg:/slim_install
pkg:/system/install/gui-install
pkg:/system/install/media/internal
dc_text_sparc:
Package list should be:
pkg:/entire
pkg:/server_install
pkg:/system/install/text-install
pkg:/system/install/media/internal
585-589: I'm not sure anyone who doesn't already know what size_pad
does will understand what this means. Perhaps just state that
"size_pad increases the amount of free space in the boot archive, at
the expense of memory on the booted system" or something along those
lines.
596-605: Adjust indentation
- Keith
On 10/19/10 03:46 PM, Alok Aggarwal wrote:
This is a code review request for the rewrite of DC
to make use of CUD. The implementation here closely
follows the design document that can be found here:
http://cvs.opensolaris.org/source/xref/caiman/caiman-docs/distro_constructor/dc_cud_design.pdf
Note that this wad does not include the support for
VMC. The support for VMC will be a separate, follow on
project.
Since the changes are fairly substantial we are breaking
up the review into the following sections:
DC app, RBAC, Makefiles and packaging [1]
Manifests and XML serialization/de-serialization [2]
Pre pkg image mod, boot archive configuration and archive archive
checkpoints [3]
All the other remaining checkpoints [4]
Please sign up to review one or more of the sections
and let us know exactly which ones you will be reviewing.
Webrev location is:
http://cr.opensolaris.org/~aalok/cud_dc
Please have your comments in by COB November 5th.
Thanks,
Drew and Alok
_______________________________________________
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
