On 18/08/2011 20:28, Ethan Quach wrote:
> Darren,
>
> I just have a couple more nits ...
>
> data_files.py
>
> 884 - I know I made the suggestion to add this but this looks
> unnecessary to me now. Would it be better if 884 just raises the error
> from 891, and then we remove the except clause at 890.
I changed as you described, I had thought of this, but needed to be sure that
there wasn't some other possible exception to be thrown - but after some testing
I couldn't get anything else to happen...
>
> 916 - Need a period at the end of the error message.
Done.
Update webrev at:
https://cr.opensolaris.org/action/browse/caiman/dkenny/7037014-3/webrev/
Thanks,
Darren.
>
>
> thanks,
> -ethan
>
>
> On 08/18/11 06:07, Darren Kenny wrote:
>> Hi Ethan,
>>
>> Thanks for the review...
>>
>> On 18/08/2011 10:17, Ethan Quach wrote:
>>> Darren,
>>>
>>> data_files.py
>>> ----------------
>>> 48 - doesn't look like this is used anymore in this file, so it can be
>>> removed.
>>>
>> Removed.
>>
>>> 866 - place holder for a comment?
>> Removed.
>>
>>> 890 - Should this 'if' be paired with an else in case system_url is
>>> None? (The else should perhaps just raise some exception that can be
>>> caught at 892)
>> Makes sense, I've added code to raise an exception if the system_url is not
>> found in the manifest.
>>
>>> 892 - We should probably catch explicit parsing exceptions and output
>>> the error message(s) accordingly, in case the manifest simply didn't parse.
>> I added a new message for this.
>>
>>> 916 - This final else clause now catches the exact case where the
>>> manifest instance the user is trying to use specifies an ai.dtd.X that
>>> is not equal to the the ai.dtd.Y that is in the imagepath of the install
>>> service, so let's enhance this error message to say precisely that.
>> Done.
>>
>>>
>>> aimanifest.py
>>> -----------------
>>> 42 - Doesn't look like this variable definition is used anywhere in this
>>> file.
>> Yep, been removed.
>>
>> I've placed an updated webrev at:
>>
>> https://cr.opensolaris.org/action/browse/caiman/dkenny/7037014-2/webrev/
>>
>> Let me know if this is ok,
>>
>> Thanks,
>>
>> Darren.
>>
>>>
>>> -ethan
>>>
>>>
>>> On 08/12/11 10:09, Darren Kenny wrote:
>>>> Hi,
>>>>
>>>> I'd like to request a code review for the bug:
>>>>
>>>> 7037014 Install DTDs should be versioned
>>>>
>>>> The webrev is at:
>>>>
>>>> https://cr.opensolaris.org/action/browse/caiman/dkenny/7037014/webrev/
>>>>
>>>> Summary:
>>>>
>>>> The basic changes that have been made are as follows:
>>>>
>>>> - Rename all .dtd files to .dtd.1
>>>>
>>>> - Add references in install_common to get to these file using
>>>> constants.
>>>>
>>>> - Add a method to check if a give string looks like a .dtd or .dtd.1
>>>> file.
>>>>
>>>> - Add constants to Makefile.master for the DTD versions, one for each
>>>> file.
>>>>
>>>> - Generate the .dtd.<Version> file at build time, and similarly, any
>>>> file that needs a reference to this versioned name.
>>>>
>>>> For this to work I've added an entry in Makefile.master to do a sed
>>>> on any file with a .src suffix.
>>>>
>>>> The sed replaces any @DTD_VERSION_XX@ type values in the text.
>>>>
>>>> All .xml files are now named .xml.src.
>>>>
>>>> - The packaging refers to variables $(DTD_VERSION_XX), which are
>>>> replaced as as part of the pkgmogrify.
>>>>
>>>> - Symlinks exist now for ai.dtd and dc.dtd to their versioned
>>>> counterpart. This is to aid migration at this last stage.
>>>>
>>>> - Attempting to add an XML manifest with a reference to ai.dtd will
>>>> generate an error if there is an ai.dtd.1 file also in the install
>>>> image.
>>>>
>>>> Testing done:
>>>>
>>>> In all my generation of ISOs I've been using the DC in the build I'm
>>>> doing, so DC has been well tested.
>>>>
>>>> Jack did some Derived Manifest testing (Thanks)
>>>>
>>>> AI installs have been done using various AI servers:
>>>>
>>>> - snv_151a
>>>> - snv_171
>>>> - snv_171 + dtd_versioned bits
>>>>
>>>> Both SPARC and x86 clients were tested - with the AI clients installing
>>>> the ISOs generated from the dtd_versioning workspace and snv_171 ISOs.
>>>>
>>>> I also verified that an zones install worked 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
