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:
- Remove testdefX
- Remove specifying 'files'
- Do not guess git/bzr repos
- 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