Dustin J. Mitchell [:dustin] (he/him) Assignee
	Description • 7 years ago

I see a lot of "if 'signing' in label", and that makes me nervous, as any new task that happens to contain that substring will get matched.

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 1 • 7 years ago

I'm pushing a patch shortly.  Testing shows the only difference to be the addition of some "signed" attributes to beetmover and signing tasks.

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 2 • 7 years ago

hah, looks like I failed to push to mozreview..

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 5 • 7 years ago

Comment on attachment 8843074 [details]
Bug 1340564: make follow-on task names append to the label;

Ah, that's right, flagging mtabara for review causes 500's from mozreview.

	Mihai Tabara [:mtabara]⌚️GMT
	Comment 6 • 7 years ago

@dustin: just wanted to step by and apologize as I didn't have time to check those reviews yet, we're still caught with the migration this week. If these patches can wait until EOW or early next week, that'd be great. Thanks!

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 7 • 7 years ago

There's a minor risk of bitrot, but other than that no rush.

	Justin Wood (:Callek)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8843075 [details]
Bug 1340564: use an attribute to identify signed tasks;

https://reviewboard.mozilla.org/r/113456/#review128068

This has likely bitrotted already, *and* to make matters worse we have a situation where signing tasks are *not* what we'll pass on to beetmover directly for OSX, and a slightly more complicated way for Windows in the future, so `signed` as used may not be great.

Additionally kim is making changes to this very code on `date` to support the needed changes for OSX, I'd rather bitrot this cleanup patch than hers (since this is pretty simple overall)

	Justin Wood (:Callek)
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8843074 [details]
Bug 1340564: make follow-on task names append to the label;

https://reviewboard.mozilla.org/r/113454/#review128070

I'm not really a huge fan of the append variant, I find the prepent better as it groups the tasks of a specific kind together. (I think this is more a preference of how do we group, and in such a case, a better UX for a full graph may be needed, rather than relying on this magic name grouping, where `label` might as well be a uuid for how we actually expect it used in practice, except we *want* a readable string too)

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 10 • 7 years ago

Fair enough.  I do find some of the labels unreadable, but if they make sense to releng, that's OK by me -- I don't think anyone else will be too concerned with them.

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 11 • 7 years ago

Is this ready to be worked on now?

	Justin Wood (:Callek)
	Comment 12 • 7 years ago

Its probably ok to work on this now. I can envision some code cleanup that could make this even easier to work on, but I don't feel anything blocks here.

To say in-bug I've also thought of some very quick pseudocode that can help enforce this "don't compare labels" stuff...

>>> class Label(object):
...    def __str__(self):
...       return self.label
...    def __repr__(self):
...       return str(self)
...    def __cmp__(self, other):
...       raise Exception("Unable to compare")
...    def __init__(self, label):
...       self.label = label
... 
>>> l = Label("foo-signing/opt")
>>> l
foo-signing/opt
>>> l == 'foo'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 7, in __cmp__
Exception: Unable to compare
>>> 'foo' in l
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: argument of type 'Label' is not iterable

	Dustin J. Mitchell [:dustin] (he/him) Assignee
	Comment 17 • 7 years ago

I'm scared to subclass string as in comment 12, for performance and correctness reasons.  We use labels as dict keys a lot!

I've verified that the patch set included here makes no change to a nightly graph except adding a `signed` attribute to lots of tasks (with value "false" in some cases).

	Brian Stack [:bstack]
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8861641 [details]
Bug 1340564: use attributes to identify talos jobs;

https://reviewboard.mozilla.org/r/133636/#review136504

Much nicer!

	Justin Wood (:Callek)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8843075 [details]
Bug 1340564: use an attribute to identify signed tasks;

https://reviewboard.mozilla.org/r/113456/#review136910

	Johan Lorenzo [:jlorenzo]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8861639 [details]
Bug 1340564: filter for android tasks using the build_platform attribute;

https://reviewboard.mozilla.org/r/133632/#review137238

I tested the patch with the protocol detailed in bug 1352477 comment 4. I didn't notice any changes in the taskgraph. Thanks for making the dependencies less label-dependent!

	Gregory Szorc [:gps]
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8861640 [details]
Bug 1340564: specify the target name explicitly for dependent tasks;

https://reviewboard.mozilla.org/r/133634/#review141308

	Pulsebot
	Comment 26 • 7 years ago

Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1aeb99ce3e6c
use an attribute to identify signed tasks; r=Callek
https://hg.mozilla.org/integration/autoland/rev/45d01b30e9d1
filter for android tasks using the build_platform attribute; r=jlorenzo
https://hg.mozilla.org/integration/autoland/rev/516721f50c1e
specify the target name explicitly for dependent tasks; r=gps
https://hg.mozilla.org/integration/autoland/rev/fd1f5c916045
use attributes to identify talos jobs; r=bstack

	Carsten Book [:Tomcat]
	Comment 27 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1aeb99ce3e6c
https://hg.mozilla.org/mozilla-central/rev/45d01b30e9d1
https://hg.mozilla.org/mozilla-central/rev/516721f50c1e
https://hg.mozilla.org/mozilla-central/rev/fd1f5c916045

	Pulsebot
	Comment 28 • 7 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/f0c6a71517c2
Backed out changeset fd1f5c916045 
https://hg.mozilla.org/mozilla-central/rev/2d6baa44eff2
Backed out changeset 516721f50c1e 
https://hg.mozilla.org/mozilla-central/rev/4bb396799a31
Backed out changeset 45d01b30e9d1 
https://hg.mozilla.org/mozilla-central/rev/1ec1d8863720
Backed out changeset 1aeb99ce3e6c for hoping that fix the packageName issue

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 7 years ago

This wasn't at fault. Re-landing.

	Pulsebot
	Comment 30 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/b1e27dba755b
use an attribute to identify signed tasks; r=Callek
https://hg.mozilla.org/mozilla-central/rev/44a0f9f0dff4
filter for android tasks using the build_platform attribute; r=jlorenzo
https://hg.mozilla.org/mozilla-central/rev/41b461000ca6
specify the target name explicitly for dependent tasks; r=gps
https://hg.mozilla.org/mozilla-central/rev/4d481af2abea
use attributes to identify talos jobs; r=bstack