On 11/15/2012 03:12 AM, Senthil Kumaran S wrote:
Hi Andy,
On Monday 12 November 2012 11:41 PM, Andy Doan wrote:
I just spent a little time looking at the branch and have a few comments:
I just made sure all your comments are addressed in revision 448. Please
have a look at it. The new job file which I used to test this is attached.
<snip>
The following changes are incorporated as per Andy's review comments:
1) Remove testdefX
2) Remove specifying 'files'
3) Do not guess git/bzr repos
4) Get rid of .get() syntax for accessing dictionary keys whereever
possible
5) Split code in _configure_target
6) Copy all the files required for test to target directory
(mwhudson) - no symlinks
</snip>
This is starting to look pretty good. I'll make a couple of nit-picks:
1) has_key
You have a couple of things like:
if testdef_repo.has_key('bzr-repo')
I think has_key is being deprecated and its more accepted to instead do:
if 'bzr-repo' in testdef_repo:
2) the counter in _get_test_definition_from_repo and
_get_test_definition_from_url
I see the point and I think I created the madness. However - I think we
can avoid having to pass the counter between the two functions. Its
basically there to prevent collisions. However, I think you can avoid
passing the counter to the functions by prefixing the directories you
create with something unique per-function like:
_get_test_definition_from_url: url-<dir>_<local counter>
_get_test_definition_from_repo: repo-<dir>_<local counter>
Then the functions can just return the array of directories and not the
tuple.
3) _copy_test (not sure about this comment)
This function has logic to copy the repo to the target with
shutil.copy2. I wonder if this is needed. When you look at the logic of
_configure_target, the repo cloning logic will be called while the
target's filesystem is mounted. So could we just do the bzr/git clone
straight to that directory and then avoid the copy operation later?
-andy
_______________________________________________
linaro-validation mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/linaro-validation
