567478 - try-as-branch sends email containing urls and text that doesn't fit the results

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

This was brought up in #developers.  It would be good to set where the pusher wants the email notification to be sent. I think this can be added in to the parser that will be created for commit comments to select certain build/platform combos as per bug 473184

Comment 1

[Justin Wood (:Callek)]

To be clear, I think it is FAR more useful to have e-mail go to pusher rather than patch author BY DEFAULT.

For two main reasons:
1) Its how try _used to_ work. And how people expect it.
2) 9 times out of 10, the person who wants the e-mail is the pusher, whether it be from checkin-needed work (Hey, I want to push this for user bob, but I don't want to be breaking the tree on everyone when I didn't write or test the patch).

3) The other common case is someone who wrote the patch is doing the push-to-try for themselves, in this case they are the pusher && patch author, so defaulting to pusher will make sense here.

The far rarer case is when someone that does not have commit [level 1] rights wants to monitor the try build of their own patch[set]. In the try results you also are expected to be able to decipher random orange from patch-caused orange, etc. So even in this case, the chances of someone non-pusher having the best read of results is slim. The person pushing in most cases should be more than willing to forward the e-mail to the person if needed anyway.

Long Story Short, lets make the default case the common [and expected] case and not the other way around [please].

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Sets 'who' to try change pusher, not the changeset author [tested]

Running this through staging right now, have already checked that it sets the who and revision properly for a build - will continue to watch the packaged unittests to make sure there's no issues there.

Putting it in the MozillaBuildFactory's initialSteps allowed me to take out the lines I added to UnittestPackagedBuildFactory last week.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

confirmed that this patch sets who and revision for all packagedUnittests - which will help close bug 567376

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Sets 'who' to try change pusher, not the changeset author [tested]

Same as the last patch, but now with a few extra tweaks to the email text to make more sense based on results - for some reason all the emails said "successfully" even if not so successful.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Sets 'who' to try change pusher, not the changeset author [tested]

Caught one more email text issue - builds completing with warnings were sending the url twice.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: cleans up the try emails output and adds more checks to results

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 447208 [details] [diff] [review]
cleans up the try emails output and adds more checks to results

>diff --git a/status/generators.py b/status/generators.py
>+    elif attrs['result'] == 'exception':
>+        text = """\
>+Your Try Server %(task)s (%(got_revision)s) completed with exception on \
>+%(platform)s on builder: %(builder)s.\n\n""" % locals()

I would've guessed that most builds which hit an exception will have not really completed. For example, all the disconnects we had recently with the Castro link tended to be during a compilation, rather than at the very end. So I'd suggest rewording this to be more like the failure case.

>     else:
>+        text = ""

This is crying out for something to handle other results being added in the future. Even if it's just 'go ask RelEng'.

>+    if attrs['result'] in ('failure', 'exception') and 'who-not-set' not in who:
>         text += "While some of the build steps haven't succeeded, your build package might have been \
>         successfully created and could be available for download at %(url)s\n\n" % locals()

Please remove the spaces before 'successfully' so they don't show up in mails.

r+ with those changes.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 447208 [details] [diff] [review]
cleans up the try emails output and adds more checks to results

http://hg.mozilla.org/build/buildbotcustom/rev/a2c1e641534b

reconfig'd the try-master.