On Fri, Sep 26, 2008 at 02:48:58PM -0500, Shawn Walker wrote:
> Brad Hall wrote:
> >Hi,
> >
> >Link to webrev: http://cr.opensolaris.org/~bhall/bug-3588/
> >Link to bug: http://defect.opensolaris.org/bz/show_bug.cgi?id=3588
> >
> >The changes include:
> >
> >- A warning to the user if they are attempting to create an image where one
> > already exists
> >- A force flag (-f) to allow them to really do that if they want to
> >- A test case for this bug
>
> client.py:
> line 563: added whitespace?
Oops, gone.
> line 1909: wording suggestion: "If this operation should be performed
> anyway, use the -f (force) option." -- or put a comma after "this" in
> your current message.
Sounds better, changed.
> Also, I'd like to see this logic put into set_attrs instead of
> client.py and a force parameter added to set_attrs instead...
Why? A number of other safety checks are being done in
client.py::image_create, but set_attrs for the most part is just setting
attributes of the image. (Though I do see the comment above set_attrs that it
should be merged with mkdirs into a create function) It seems like if this is
going to be moved into image.py it should be moved to some sort of "check
validity" method. Anyhow, I'm not opposed to moving it, just wanted to
understand why you want to see it moved.
> pkg.1.txt:
> line 94ff: The wording you have is inconsistent with the pattern we
> have for other options. I suggest something like: "With -f, force the
> creation of an image over an existing image. This option should be used
> with care."
Fixed, that sounds better as well.
> t_image_create.py:
> I would add a test to ensure that force works.
Ok, will do.
> testutils.py:
> Do we really want to always force the creation of an image? I take
> it we were creating new images over old images before and it was
> allowing this?
Yup.
> Cheers,
> --
> Shawn Walker
Thanks for reviewing.
-Brad
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
