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.
916 - Need a period at the end of the error message.
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
