Looks good. -- John
On Fri, Feb 19, 2010 at 10:50 AM, K.D. Lucas <[email protected]> wrote: > Ok, I've implemented all of changes you suggested, patch is attached. > > Kelly > > > On Wed, Feb 17, 2010 at 12:40 PM, John Admanski <[email protected]>wrote: > >> It would be good to also do an if self.job check, since people do at times >> use the host classes outside of running jobs and it would be good if the >> code can handle that case (the labels would just have to be None). In >> practice it's not a big deal since that code should never be calling >> host.get_platform_label() anyway but since the code is making an effort to >> deal with the fact that host keyvals may not be available it would be nice >> to handle that as well. >> >> Also, you could just do a return keyvals.get('platform', None) instead of >> having to use a big if-else block. >> >> -- John >> >> >> On Wed, Feb 17, 2010 at 10:17 AM, K.D. Lucas <[email protected]> wrote: >> >>> Good idea. I moved it into server/hosts/remote.py. Please take a look, as >>> this did simplify the function. >>> >>> >>> >>> On Tue, Feb 16, 2010 at 7:35 AM, John Admanski <[email protected]>wrote: >>> >>>> You should use the docstring format in CODING_STYLE. >>>> >>>> Also, I do like this better as a host method, not a general function. I >>>> suggest server/hosts/remote.py, there's no reason to tie it to the ssh >>>> classes specifically. Then you can even drop both of the arguments to the >>>> function, since they're implied by self.hostname and self.job.resultdir. >>>> >>>> -- John >>>> >>>> On Fri, Feb 12, 2010 at 1:05 PM, K.D. Lucas <[email protected]> wrote: >>>> >>>>> I wanted some jobs to select the server side component based on a >>>>> platform label, so this patch will add a function that returns the >>>>> platform_label of the specified hostname. >>>>> >>>>> Long term, I think it would be best to expose this as an attribute of >>>>> the AbstractSSHHost class, since it would then be very easy to access >>>>> these >>>>> details and we wouldn't need to run any methods to grab them. I'm not >>>>> familiar enough yet with the code to understand how to best implement >>>>> this, >>>>> so my work around is the attached patch. >>>>> >>>>> Kelly >>>>> >>>>> -- >>>>> K.D. Lucas >>>>> [email protected] >>>>> >>>>> _______________________________________________ >>>>> Autotest mailing list >>>>> [email protected] >>>>> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest >>>>> >>>>> >>>> >>> >>> >>> -- >>> K.D. Lucas >>> [email protected] >>> >> >> > > > -- > K.D. Lucas > [email protected] >
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
