Ed,

Here's an updated webrev.
    http://coupe/sw83825/ips-ipkgAIinstall/webrev.ipkg.2

Thanks,
Susan


On 06/ 1/11 09:11 AM, Susan Kamm-Worrell wrote:
 On 06/ 1/11 08:49 AM, Edward Pilatowicz wrote:
On Wed, Jun 01, 2011 at 08:20:18AM -0600, Susan Kamm-Worrell wrote:
  On 06/ 1/11 02:33 AM, Edward Pilatowicz wrote:
On Tue, May 31, 2011 at 10:55:03PM -0600, Susan Kamm-Worrell wrote:
Ed,

Thanks for the review.

Responses below.

Susan


On 05/31/11 07:08 PM, Edward Pilatowicz wrote:
[ resend after dropping lots of quoted command output ]

- in clone, it seems weird that we're adding arguments after the
zonename, it seems like all arguments should come before the zonename.
For the rest of the commands the brand-specific-options come after all the
common command arguments.  I arc'd the extension of clone with that
can you provide some examples?  i thought that brand options can be
intermixed with zoneadm options, and zoneadm will just pass unknown
options onto the brand callback.

thought in mind:
(ipkg)
clone [-m copy] [-s zfs_snapshot] source_zone [-c sc_profile.xml | dir]

(s10)
clone [-m copy] [-s zfs_snapshot] source_zone [-c sysidcfg]


well, cli processing in general looks like:

<command> <options> <arguments>

i really really really don't like the idea of supporting:

<command> <options> <arguments> <brand options>

aside from being a bad idea in general, doing this will prevent us from
ever being able to add another argument to the clone subcommand.

wrt arc, i'm sorry i missed this.  but we can fix this with a quick
email to the case alias.  also, if you think that fixing this will put
you at risk of missing this build then i'd say we just drop the clone
changes from this wad, update the arc case, and deliver the clone
changes next build.  (i doubt AI depends on these clone changes.)
I guess this was just my interpretation from the zoneadm manapage:
   zoneadm -z zonename [-u uuid-match] subcommand [subcommand_options]
where I understood the subcommand of clone to be 'clone clone_source'.

np.

really, the format for zoneadm is:

zoneadm -z<zonename> [<options> ] subcommand [<subcommand_options>] [<subcommand_args>]

and putting<brand_subcommand_options>  after<subcommand_args>  just
doesn't seem right to me.  afaik, no subcommands distinguish between
<subcommand_options>  and<brand_subcommand_options>.

It's no problem to make this change to clone, but I was under the impression
that all changes associated with an ARC case for a consolidation had to
be pushed together.

this is generally true, so if this wasn't done it would clearly be a bug
that would need to be fixed asap.  :)

The s10 changes already went back with this change. Would it be possible to push this change, then fix it with a bug fix and updated fasttrack next build?
Then I could fix both s10 and ipkg consolidations in the same build.

grumble, grumble, grumble, i guess so...
please get bugs filed on the issue.

As soon as this is pushed I'll file the bug and get started
on the ARC change.


but i thought the -c option allows the user to specify a new sysconfig
profile.  i didn't think we were going to support installing old style
sysidconfig profiles with this same option.  do we really need to do
that? why not just generate an error if -c or -m are specified and the new sysconfig mechanism aren't active? (it seems like we're delivering new features for the old framework that we're about to rip out anyway.)
The plan is for the zones test suites to start using the -c option and to mimic this code passing in the needed sysidcfg file if the old sysconfig framework was in use and to use a profile for the new sysconfig framework. This would allow
the test suites to be used by both the old and new frameworks.

Once the new SC framework is in place and is working, the tests can also be modified
to remove the old sysidcfg code.

sigh.  supporting two frameworks is kinda confusing so i was hoping to
keep the code for the old one to a minimum.  but i understand what
you're saying, so ok.  could we get a bug filed on ripping out the
support for the old framework?

Absolutely. Hopefully the old framework will be removed in the build after
the new SC framework integrates.  Their current target is 169.


ed


_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss

Reply via email to