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
