1278999 - taskcluster/taskgraph/kind/legacy.py should not assume task.payload.artifacts is a dictionary

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Pete Moore [:pmoore][:pete]]

In docker worker, task.payload.artifacts is a dictionary.

In generic worker, it is a list.

This needs to be cleaned up separately, but for now we should allow for both forms to exist, until generic worker aligns, or until tasks are migrated from generic worker -> taskcluster worker.

Comment 1

[Pete Moore [:pmoore][:pete]]

Attachment: bug1278999_gecko_v1.patch

Hey Wander,

Could you take a look at this (simple) fix, as dustin is on PTO for the next days?

Thanks!
Pete

Comment 2

[Wander Lairson Costa]

Comment on attachment 8761514 [details] [diff] [review]
bug1278999_gecko_v1.patch

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

::: taskcluster/taskgraph/kind/legacy.py
@@ +210,5 @@
> +    except:
> +        # generic worker
> +        values = artifacts
> +
> +    for artifact in values:

How about:

if hasattr(artifacts, "values"):
   artifacts = artifacts.values()

Comment 3

[Rob Thijssen [:grenade (EET/UTC+0300)]]

tested wcosta's suggestion here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36aa7a839c0
seems to work...

Comment 4

[Pete Moore [:pmoore][:pete]]

(In reply to Wander Lairson Costa [:wcosta] from comment #2)
> Comment on attachment 8761514 [details] [diff] [review]
> bug1278999_gecko_v1.patch
> 
> Review of attachment 8761514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/taskgraph/kind/legacy.py
> @@ +210,5 @@
> > +    except:
> > +        # generic worker
> > +        values = artifacts
> > +
> > +    for artifact in values:
> 
> How about:
> 
> if hasattr(artifacts, "values"):
>    artifacts = artifacts.values()

It is an interesting suggestion, many thanks for diving into this.

The only thing I don't like about the alternative suggestion is that it can be misleading at a glance. First artifacts is assigned in line 203:

artifacts = task_def['payload']['artifacts']

and then later, it gets "moved" to artifacts.values() a few lines further down. So a cursory glance at the code you might think that artifacts was task_def['payload']['artifacts'] without spotting it got moved on a couple of lines later. With my version, there is an intermediate variable, so you can't make this mistake. Also you clearly see from the comments the two forks the code can take - 1 route for generic worker, and 1 route for docker worker. It just feels more readable, although I see your version is more concise.

Comment 5

[Wander Lairson Costa]

Comment on attachment 8761514 [details] [diff] [review]
bug1278999_gecko_v1.patch

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

::: taskcluster/taskgraph/kind/legacy.py
@@ +206,5 @@
>  
> +    try:
> +        # docker worker
> +        values = artifacts.values()
> +    except:

"except:" catches everything, even syntax errors, better to use "except AttributeError:"

@@ +207,5 @@
> +    try:
> +        # docker worker
> +        values = artifacts.values()
> +    except:
> +        # generic worker

This also holds for taskcluster-worker

Comment 6

[Pete Moore [:pmoore][:pete]]

Attachment: bug1278999_gecko_v2.patch

Compromise! :-)

Comment 7

[Pete Moore [:pmoore][:pete]]

Attachment: bug1278999_gecko_v2.patch

Same as previous patch, but forgot r=wcosta in commit message, so reattaching with updated commit message. =)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3e2d8ed90a
handle generic worker style artifacts (list not dictionary). r=wcosta

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd3e2d8ed90a