Any takers on a second review? The new webrev is at:
https://cr.opensolaris.org/action/browse/caiman/dkenny/7170813-1/webrev/ Thanks, Darren. On 29/05/2012 17:30, Darren Kenny wrote: > Thanks Jack... > > Everyone else, I still need another review of this, if anyone is able to do > so... > > Thanks, > > Darren. > > On 29/05/2012 17:24, Jack Schwartz wrote: >> Hi Darren. >> >> Thanks for adding the extra comment. >> >> Thanks, >> Jack >> >> On 05/28/12 03:09 AM, Darren Kenny wrote: >>> Hi Jack, >>> >>> On 25/05/2012 18:15, Jack Schwartz wrote: >>>> Hi Darren. >>>> >>>> Pretty tricky navigation in _validate_input(). :) >>>> >>>> Looks like you are unshackling yourself from IPS constraints and doing >>>> the checking yourself instead. Assuming this is OK; not an expert here. >>> Well, doing the checking ourselves allows us to present a more meaningful >>> error to the user rather than a Python stack trace. >>> >>>> Nit: I suggest adding a comment that set_image_args() allows for an >>>> empty _publ. >>> I added some text to the doc string for set_image_args, so that it now >>> looks like: >>> >>> def set_image_args(self): >>> '''Set the image args we need set because the information >>> was passed in via other attributes. These include progtrack, >>> prefix, repo_uri, origins, and mirrors. If we're creating a >>> zone image, we also need to set the use-system-repo property >>> in the props argument. >>> + >>> + For the publisher used to create the image, it is possible to >>> + omit the prefix (_publ) and allow IPS to do auto-discovery from >>> + the repo itself. >>> ''' >>> >>> Thanks, >>> >>> Darren. >>> >>>> Code LGTM. >>>> >>>> Thanks, >>>> Jack >>>> >>>> On 05/25/12 09:02 AM, Darren Kenny wrote: >>>>> Hi, >>>>> >>>>> Could I please get two reviewers for the fix for the following bug: >>>>> >>>>> >>>>> 7170813 AI fails on client hits assert (prefix or refresh_allowed) >>>>> or not repo_uri >>>>> [ http://monaco.us.oracle.com/detail.jsf?cr=7170813 ] >>>>> >>>>> The webrev is at: >>>>> >>>>> https://cr.opensolaris.org/action/browse/caiman/dkenny/7170813/webrev/ >>>>> >>>>> The problem here is that the prefix for the IPS publisher was being >>>>> omitted >>>>> and because I added the refresh_allowed=False in a previous fix, I ended >>>>> up >>>>> causing an assertion to fire for this scenario - which is one that I >>>>> hadn't >>>>> tested for. >>>>> >>>>> It turns out that this omission of the prefix is only permitted for the >>>>> first publisher that is used at creation time of the install image - any >>>>> other publisher must specify the prefix - this is enforced by the IPS >>>>> client API. >>>>> >>>>> So I added some code to also test this scenario, and generate a meaningful >>>>> error should the prefix be omitted from any additional publishers. >>>>> >>>>> I verified that the scenario that caused the bug now works correctly, and >>>>> also that all the scenarios that cause the error to occur are correctly >>>>> caught. >>>>> >>>>> I updated the test suite bug 7163961 to request for addition for tests for >>>>> these scenarios too. >>>>> >>>>> Thanks, >>>>> >>>>> Darren. >>>>> _______________________________________________ >>>>> 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
