With the landing of bug 754073, there is no reason to invoke PerfConfigurator separately from talos (unless e.g. you're mucking with the generated config file or what not). We should switch mozharness to invoke talos directly instead of the intermediary step. Using the '-v' flag will still output the config to stdout, which is probably all we really care about.

Comment on attachment 685398 [details] [diff] [review] remove extraneous call to PerfConfigurator Pylint says: mozharness/mozilla/testing/talos.py:286: [E, Talos.talos_options] method already defined line 55 self.talos_options appears to be a list of commandline options: http://hg.mozilla.org/build/mozharness/file/ad1d9c2a0968/mozharness/mozilla/testing/talos.py#l55 We seem to have a query_talos_options() that should now be named query_talos_extra_options(): http://hg.mozilla.org/build/mozharness/file/ad1d9c2a0968/mozharness/mozilla/testing/talos.py#l237 or maybe we should munge it all into one single query_talos_options() method (or rename the self.talos_options list to something else and munge it all into self.talos_options()). I think the above needs resolving. Did this work as is?

(In reply to Aki Sasaki [:aki] from comment #2) > Comment on attachment 685398 [details] [diff] [review] > remove extraneous call to PerfConfigurator > > Pylint says: > > mozharness/mozilla/testing/talos.py:286: [E, Talos.talos_options] method > already defined line 55 > > self.talos_options appears to be a list of commandline options: > http://hg.mozilla.org/build/mozharness/file/ad1d9c2a0968/mozharness/mozilla/ > testing/talos.py#l55 > > We seem to have a query_talos_options() that should now be named > query_talos_extra_options(): > http://hg.mozilla.org/build/mozharness/file/ad1d9c2a0968/mozharness/mozilla/ > testing/talos.py#l237 > > or maybe we should munge it all into one single query_talos_options() method > (or rename the self.talos_options list to something else and munge it all > into self.talos_options()). > > I think the above needs resolving. Did this work as is? Ah, good catch. It does work as is, evidently because the class-level talos_options is only used one place, to add to config_options at http://hg.mozilla.org/build/mozharness/file/809c4cc296e8/mozharness/mozilla/testing/talos.py#l107 I don't have any strong opinions about the naming of things, except `PerfConfigurator_options` is no longer accurate so shouldn't be used. I would propose talos_options -> talos_config_options PerfConfigurator_options -> talos_options query_talos_options -> query_talos_options (unchanged, though I'm also fine with query_talos_extra_options or query_extra_talos_options) I can't think of a good way to deliminate these things such that it is obvious what apply to the mozharness script and what apply to the wrapped script (i.e. talos). Any opinions?

I don't have strong opinions here; that sounds good. One thing that might help is moving self.talos_options (now talos_config_options, or talos_optparse_options, or talos_commandline_options or w/e) to above the class. If something wants to import that without the class, they can; if we want to use it to specify our optparse options, we can; I'm not sure we need to keep it around as part of the class. By the sound of comment 3 it seems like you'll figure something out that I'll be ok with, now that you're aware of the conflict :)

I found a few other nits that I cleaned up on a second pass

Comment on attachment 685858 [details] [diff] [review] round II Lgtm. We should be able to catch issues on Cedar, if any.

pushed: http://hg.mozilla.org/build/mozharness/rev/9563217e5289