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
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.
/usr/src/lib/install_manifest/common/__init__.py:
* line 71, s/Wwhere/where/
* 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.
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.
* line 249: Should "cancel_requested" be checked here also?
* 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.
usr/src/lib/install_manifest/writer/writer.py:
* line 220-221, 226-227, 260-262: combined into LOGGER.exception() as
mentioned above.
* line 142: Should the directory path for the "manifest" argument be
check to ensure
that it is valid?
* line 233: check cancel_requested?
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.
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
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.
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
