Lucas, Thanks for your comments. I'll get a good svn diff patch out soon.
Before I do though, just one question. I kind of patterned my get_value off of the same thing that is done in <AT_ROOT>/tko/db.py, so that is where I got the idea for that. I wanted to kind of shorten the path to this function as it would really make the code look ugly since the constant is quite long to begin with. Also, is svn diff ok to use or would you prefer git? Kelly On Thu, Feb 11, 2010 at 6:31 PM, Lucas Meneghel Rodrigues <[email protected]>wrote: > On Thu, 2010-02-11 at 17:44 -0800, K.D. Lucas wrote: > > I'm currently working with a class of machines with very limited disk > > space; consequently, I'd like to move the following constant to a > > place where it is a little more convenient to configure. > > Hi, some comments below: > > > I'd like to move AUTOTEST_GB_DISKSPACE_REQUIRED = 20 to a value > > derived from global_config.ini file. > > > > > > --- global_config.ini (revision 4230) > > +++ global_config.ini (working copy) > > Your mailer chewed up your patch, you'll have to find a way to make it > not auto-wrap the patch contents. > > > [SERVER] > > +# Minimum amount of disk space required for AutoTest in GB > > +gb_diskspace_required: 5 > > ===================================================================== > > and then implement the following change in > > <AUTOTEST_ROOT>/server/hosts/abstract_ssh.py > > Point out manually where the change is is also not a good practice, you > should generate the contents of the patch all at once, p1 level, using > an appropriate tool such as svn diff, or better, git. > > If you are interested I can send you a quick start guide of how to > re-create this patch using git and send it with git send-email. It will > resolve all the formatting problems on your current patch. > > > ====================================================================== > > --- abstract_ssh.py (revision 4230) > > +++ abstract_ssh.py (working copy) > > @@ -5,11 +5,12 @@ > > from autotest_lib.client.common_lib.global_config import > > global_config > > > > > > -enable_master_ssh = global_config.get_config_value('AUTOSERV', > > - > > 'enable_master_ssh', > > - type=bool, > > default=False) > > +get_value = global_config.get_config_value > > It'd be preferable to not do the above, as we keep this import style to > make it easier for unit tests, and also, get_value is a very generic > name, which is a bad idea in terms of namespace. > > > +enable_master_ssh = get_value('AUTOSERV', 'enable_master_ssh', > > type=bool, > > + default=False) > > > > + > > def make_ssh_command(user="root", port=22, opts='', > > hosts_file='/dev/null', > > connect_timeout=30, alive_interval=300): > > base_command = ("/usr/bin/ssh -a -x %s -o > > StrictHostKeyChecking=no " > > @@ -487,9 +488,11 @@ > > > > > > # tunable constants for the verify & repair code > > - AUTOTEST_GB_DISKSPACE_REQUIRED = 20 > > + AUTOTEST_GB_DISKSPACE_REQUIRED = get_value("SERVER", > > + > > "gb_diskspace_required", > > + type=int, > > + default=20) > > > > - > > def verify_connectivity(self): > > super(AbstractSSHHost, self).verify_connectivity() > > > > The general approach looks good to me, but you have to fix the issues > found and re-send so we can consider your patch for inclusion. > > Lucas > > > -- K.D. Lucas [email protected]
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
