Hi Karen,
Many thanks for your review.
An updated webrev which includes fixes for the issues you raised is at:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_04/
Responses below.
On 10/11/10 19:08, Karen Tung wrote:
Hi Dermot,
Here are my code review comments. I didn't look through Keith's comment
on the code. So, if I mention anything that's already been covered,
please
just point me to the response.
All files:
* please alphabetize the imports
fixed
usr/src/lib/install_manifest/common/Makefile:
usr/src/lib/install_manifest/parser/Makefile:
usr/src/lib/install_manifest/writer/Makefile:
* In all 3 Makefiles, line 53, and line 65-68: since the SUBDIRS is
not defined or used. I think any mention of it can be removed.
fixed
/usr/src/lib/install_manifest/common/__init__.py:
* line 71, s/Wwhere/where/
fixed
* line 83, do we need to validate and make sure that "dtd_file" is valid
before making this call? If it is assumed that the dtd_file is valid,
I think
it would be a good idea to say so in the parameters section of the
comment.
This should not be necessary. The call to etree.DTD(dtd_file) raises
DTDParseError
for both syntax errors in the file or if the file is not present. If
the file is not present
the returned ManifestError from validate_manifest() will look like:
ManifestError: Unable to parse DTD file
[blah/lib/install_manifest/test/manifests/dc.dtd]: : DTDParseError
- failed to load external entity
"blah/lib/install_manifest/test/manifests/dc.dtd"
which I think is a fairly descriptive message?
usr/src/lib/install_manifest/parser/parser.py:
* line 100: perhaps also mention that the name parameter is not used
by the checkpoint at all.
fixed (here and in writer.py also)
* line 249: Should "cancel_requested" be checked here also?
If you say so!
fixed.
* line 338-339, 344-345: If you call LOGGER.exception(), you can just
pass in
the "msg" as an argument to the LOGGER.exception() call and you will
get the full stack trace printed in the log.
Keith also made this point.
fixed
usr/src/lib/install_manifest/writer/writer.py:
* line 220-221, 226-227, 260-262: combined into LOGGER.exception() as
mentioned above.
fixed (I missed these the first time around)
* line 142: Should the directory path for the "manifest" argument be
check to ensure
that it is valid?
It's much easier to try to write to the manifest file later on and return
an error if that fails, rather than check up front if it would be
possible to
write it.
I think it would be being overly defensive to fail in the initializer
method if a file to be created moments later cannot be created at that
time. Unless there's a particular reason you think this check would be
useful?
* line 233: check cancel_requested?
fixed
usr/src/lib/install_manifest/test/common.py
* lines 29-34: instead of hard coding
"lib/install_manifest/test/manifests"
in every line, I think it is easier to just define that in one variable
and use that to build the other variables. Also makes it easier if
we want to change the path in the future.
already fixed (as part of the:
TEST_BASE_DIR = os.path.dirname(os.path.abspath(__file__))
code you suggested to me in a different thread).
usr/src/lib/install_manifest/test/test_mp_with_engine.py, lines 49-61
usr/src/lib/install_manifest/test/test_mw_dry_run.py, lines 51-63
usr/src/lib/install_manifest/test/test_mw_with_engine.py, lines 51-63
* The engine tests has 2 convenience functions that
other tests can use for doing the engine setup and tear down.
Darren requested them for the DOC tests, so, I added them to
the engine tests recently.
I think you can just use them. The functions are in:
usr/src/lib/install_engine/test/engine_test_utils.py
already fixed (I'm calling get_new_engine_instance() and reset_engine())
usr/src/lib/install_manifest/test/dc_*.xml:
* These are all DC manifests that are modified slightly for testing.
Instead of keeping these, I wonder whether it makes sense to get a
copy of
the most up to date DC manifest from the DC directory, and then, generate
a file inserting/deleting things as appropriate for the tests. This way,
we don't need to worry about keeping these files up-to-date as the DC
manifests
evolve.
It's only a coincidence that these files are based off the DC manifests.
These files are for unit testing very specific XML and DTD features in MP/MW
and so it's best to keep them separate from the "real" DC files - it
would add
unnecessary complication if the unit test input files were in some way
linked
to the real DC files.
There is no need to keep these files in-sync with the DC files.
Thanks,
- Dermot
Thanks,
--Karen
On 09/30/10 07:40, Dermot McCluskey wrote:
Hi All,
I'd like a code review for the ManifestParser/ManifestWriter
checkpoints for the CUD project. The changes are here:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_01/
Please sent comments by 10/12/2010.
Note: this webrev just includes the new files under
usr/src/lib/install_manifest.
There are also a very small number of changes to higher-level
Makefiles, etc
which have been omitted here for convenience.
Thanks,
- Dermot
_______________________________________________
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
