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"?
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
