895721 - Harness changes in bug 894980 broke Talos for older revisions on Try

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Matthew N. [:MattN]]

Any queued or future talos runs on Try seem to break because of this change. This makes it harder to bisect performance regressions using Try server.

18:29:55    FATAL - Uncaught exception: Traceback (most recent call last):
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\base\script.py", line 1048, in run
18:29:55    FATAL -     self.run_action(action)
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\base\script.py", line 990, in run_action
18:29:55    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\base\script.py", line 931, in _possibly_run_method
18:29:55    FATAL -     return getattr(self, method_name)()
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\mozilla\testing\talos.py", line 449, in download_and_extract
18:29:55    FATAL -     self._populate_webroot()
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\mozilla\testing\talos.py", line 390, in _populate_webroot
18:29:55    FATAL -     talos_repo = self.query_talos_repo()
18:29:55    FATAL -   File "C:\slave\test\scripts\mozharness\mozilla\testing\talos.py", line 256, in query_talos_repo
18:29:55    FATAL -     return self.talos_json_config['global']['talos_repo']
18:29:55    FATAL - KeyError: 'talos_repo'
18:29:55    FATAL - Exiting -1

Comment 1

[Aki Sasaki (not active)]

I think we can work around this by adding a default of http://hg.mozilla.org/build/talos .

Comment 2

[Armen [:armenzg]]

Attachment: talos.diff

Comment 3

[Jason Yeo[:jyeo]]

Comment on attachment 778435 [details] [diff] [review]
talos.diff

Review of attachment 778435 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozharness/mozilla/testing/talos.py
@@ +254,5 @@
>          """
> +        default_repo = "http://hg.mozilla.org/build/talos"
> +        try:
> +            if self.query_talos_json_config():
> +                return self.talos_json_config['global']['talos_repo']

Not sure about the usage of "try...except" here. I would prefer

return self.talos_json_config['global'].get('talos_repo', default_repo)

@@ +256,5 @@
> +        try:
> +            if self.query_talos_json_config():
> +                return self.talos_json_config['global']['talos_repo']
> +            else:
> +                return self.config.get('talos_repo')

Same here. self.config.get('talos_repo', default_repo)

Comment 4

[Armen [:armenzg]]

Attachment: talos.diff

I used the try/except clause to avoid having to write this (which I think we should) and saw as ugly:
self.talos_json_config.get('global', {}).get('talos_repo', default_repo)

I am getting worried about the case where the talos.json's would not contain "global" in it.

Comment 5

[Aki Sasaki (not active)]

Comment on attachment 778453 [details] [diff] [review]
talos.diff

If you're worried about global going away, your comment string is fine, too.

Comment 6

[Armen [:armenzg]]

Comment on attachment 778453 [details] [diff] [review]
talos.diff

http://hg.mozilla.org/build/mozharness/rev/01eac888fc98

Comment 7

[Armen [:armenzg]]

This is live now.

Comment 8

[Matthew N. [:MattN]]

Thanks for the quick fix!