On Fri, 2010-04-16 at 15:51 +0200, Josef Skladanka wrote:
Hi James,
thanks for the comments, replies inside your mail:
On 04/16/2010 03:36 PM, James Laska wrote:
> Nice patches Josef. Just some comments below.
>
> On Fri, 2010-04-16 at 12:45 +0200, Josef Skladanka wrote:
>> +parser.add_option('--autotest-server', action='store',
default=None,
>> help='Sets the autotest-server hostname. Used for creating URLs
>> to
>> results.\
>
> The value passed in through --autotest-server doesn't appear to be used
> when scheduling the autotest job using atest. Shouldn't the value
> provided also be used to provide the correct parameter to atest?
>
> # atest job create -h
> -w WEB_SERVER, --web=WEB_SERVER
> Specify the autotest server to talk to
>
> If so, I would recommend changing the wording of the --help to reflect
> this.
>
No, that value is not passed to the atest command. My intent was to
provide a way to set the value, if socket.gethostname() would not return
the right value (e.g. gethostname() returns
dhcp-lab-241.englab.brq.redhat.com, while the correct hostname could be
jskladan.brq.redhat.com).
I don't know, what exactly does the -w parameter of atest do, and i we
want to use it or not - maybe it would be for the best, if you added
this to todays resultsDB meeting agenda?
If we are providing a way to tell autoqa where the autotest-server is, I
think it would make sense to use that information for job scheduling and
for linking back to job results.
Would adding the following to your patchset achieve this?
diff --git a/autoqa b/autoqa
index efb94e3..97e4b28 100755
--- a/autoqa
+++ b/autoqa
@@ -96,2 +96,4 @@ def schedule_job(controlfile, required_arch=None,
email=None, name=None, dryrun=
cmd += ['-m', '*%s' % required_arch]
+ if opts.autotest_server
+ cmd += ['-w', opts.autotest_server]
# NOTE this doesn't work without -m, so we need to pick something..
>> @@ -38,6 +38,7 @@ conf = {
>> 'testdir': '/usr/share/autotest/client/site_tests',
>> 'hookdir': '/usr/share/autoqa',
>> 'notification_email': '',
>> + 'autotest_server': socket.gethostname(),
>> }
>> cfg_parser = SafeConfigParser() # used by prep_controlfile
>> try:
>
> Should establishing default config options be moved into
> autoqa/config.py instead?
I do not know, what config.py does, but I'm almost certain, that it is
not imported in the autoqa (the file autoqa).
These hardcoded config options make more sense to me at the place they
currently are (since once knows on first sight, what they are, and that
they are set. But this is, of course, only my opinion.
I asked wwoods about the use of autoqa/config.py. He notes this is
currently used for test-specific configuration, not system-wide
configuration. While it would be good to make autoqa/config.py work for
both, so we can move all config handling into that file, this can be a
future exercise outside the scope of your changes.
Thanks,
James