New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 816652 link

Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature

Blocked on:
issue 843453



Sign in to add a comment

Email notifications specified in the build

Project Member Reported by no...@chromium.org, Feb 26 2018

Issue description

CrOS infra has a use case: a build should specify email addresses that should be notified when build ends. The email should include at least status and a link to the build. CrOS needs it only for manually scheduled tryjobs, and NOT builds created by CrOS CQ.

proposal: 

- add protocol for in-build email notification config. Add "email_notify" key to the parameters_json field which defines who to notify over email. The value is a JSON array of objects, where each has key "email", which is a string email address.

- extend cbuildbot to specify that

- extend luci_notify to send an email to all recipients if the build is a luci build.
Code: https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/luci_notify/notify/pubsub.go?q=luci_notify/notify/pubsu&sq=package:chromium&dr&rcl=bee896decafdfda281a007920c61bc10e2427a41&l=65

 
Cc: jinjingl@chromium.org
Owner: dgarr...@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/a1c983f35be152ce63138a79182ca9c8c9e3b030

commit a1c983f35be152ce63138a79182ca9c8c9e3b030
Author: Don Garrett <dgarrett@google.com>
Date: Tue Mar 06 22:29:23 2018

luci-notify: Allow emails to chromium.org.

Tryjob notifications are usually sent to the accounts generating CLs,
which are usually chromium.org based.

Allow email to be sent to @chromium.org accounts, in addition to
@google.com accounts. This is a loosening of restrictions, and so
a potential loosening of security (but only if people can edit the
luci-notify configurations).

This affects luci-notify configurations, as well as any buildbucket
property based notifications when they are implemented.

BUG= chromium:816652 

Change-Id: I9a05ff9b7eebdeb99581faad9d0ec1947b032591
Reviewed-on: https://chromium-review.googlesource.com/947709
Commit-Queue: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/a1c983f35be152ce63138a79182ca9c8c9e3b030/luci_notify/notify/notify.go

Labels: Swarming
cros tryjob needs to be updated to set email_notify appropriately, and to have a way to NOT set it for automated usage (like the PreCQ launcher).
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/2e38b60d6943bf5d393d601b6a5719bdc8a3b95a

commit 2e38b60d6943bf5d393d601b6a5719bdc8a3b95a
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 09 08:16:26 2018

cros tryjob: Cleanup tag/properties.

Make a few cleanups before a real change:
  All tags should also be properties,
  There is no point in including tags with empty values.
  Sort tags/json keys for better output consistency.
  Remove obsolete build_type tag.

BUG= chromium:816652 
TEST=Unittests + manual tryjobs.

Change-Id: I474e02f2e61219cb56d1fb8c4ab67cfedcf6ea5f
Reviewed-on: https://chromium-review.googlesource.com/956671
Commit-Ready: Don Garrett <dgarrett@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/2e38b60d6943bf5d393d601b6a5719bdc8a3b95a/lib/remote_try.py
[modify] https://crrev.com/2e38b60d6943bf5d393d601b6a5719bdc8a3b95a/lib/remote_try_unittest.py

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/82b33841fb1617306dd91687dc3cbe6a158637ad

commit 82b33841fb1617306dd91687dc3cbe6a158637ad
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 09 08:16:27 2018

cros tryjob: Set email_notify property.

In order to leverage luci_notify for tryjobs, we need to set the
'email_notify' parameter in our tryjob requests.

Start doing so for swarming tryjobs. This won't have any effect until
CL:947711 is in production.

BUG= chromium:816652 
TEST=run_tests

Change-Id: Id510c5fb017ddedef2e4456a2c98bd9130f4ae09
Reviewed-on: https://chromium-review.googlesource.com/956672
Commit-Ready: Don Garrett <dgarrett@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Jacob Kopczynski <jkop@chromium.org>

[modify] https://crrev.com/82b33841fb1617306dd91687dc3cbe6a158637ad/lib/remote_try.py
[modify] https://crrev.com/82b33841fb1617306dd91687dc3cbe6a158637ad/lib/remote_try_unittest.py

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/aa6a315f08b9f8c3fefc54cca91f7b921543b853

commit aa6a315f08b9f8c3fefc54cca91f7b921543b853
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 09 21:23:50 2018

luci-notify: Generate build completion emails for "email_notify".

When a build completes, generate emails to all addresses listed in the
"email_notify" property in parameters_json, if present.

BUG= chromium:816652 

Change-Id: I00137a54caf85a10855799fe05434d7b4ed477d7
Reviewed-on: https://chromium-review.googlesource.com/947711
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/aa6a315f08b9f8c3fefc54cca91f7b921543b853/luci_notify/notify/notify.go
[modify] https://crrev.com/aa6a315f08b9f8c3fefc54cca91f7b921543b853/luci_notify/notify/notify_test.go
[modify] https://crrev.com/aa6a315f08b9f8c3fefc54cca91f7b921543b853/luci_notify/notify/pubsub.go
[modify] https://crrev.com/aa6a315f08b9f8c3fefc54cca91f7b921543b853/luci_notify/notify/pubsub_test.go

These two swarming builds were launched after the new luci-notify was pushed.

http://cros-goldeneye/chromeos/healthmonitoring/buildDetails?buildbucketId=8952221644169671216
http://cros-goldeneye/chromeos/healthmonitoring/buildDetails?buildbucketId=8952221642788003312


They both contain parameters_json values that look similar to this:

"parameters_json": "{\"builder_name\": \"Generic\", \"email_notify\": [{\"email\": \"dgarrett@google.com\"}], \"properties\": {\"bot\": [\"success-build\"], \"cbb_branch\": \"master\", \"cbb_config\": \"success-build\", \"cbb_display_label\": \"tryjob\", \"cbb_email\": \"dgarrett@google.com\", \"cbb_extra_args\": [\"--remote-trybot\", \"-b\", \"master\"], \"email\": [\"dgarrett@google.com\"], \"extra_args\": [\"--remote-trybot\", \"-b\", \"master\"], \"name\": \"\", \"owners\": [\"dgarrett@google.com\"], \"user\": \"dgarrett\"}}",

But no notification emails have been generated.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/595ae15cfac7b314a63c4092037e26470faa1d82

commit 595ae15cfac7b314a63c4092037e26470faa1d82
Author: Don Garrett <dgarrett@google.com>
Date: Tue Mar 13 00:40:26 2018

luci-notify: Populate nil referernce to buildbucket.Build.

A pointer to an inner struct wasn't being populated. Test code uses a
mockup helper that does populate it, and so wasn't seeing the
same panic.

This was missed because there is no test coverage for extractBuild,
but adding that coverage requires creating populated http.Request
objects, which is a larger project than I want to address today.

BUG= chromium:816652 

Change-Id: Ib00bd025fa7a829a84425c0f15fdf7ba39e059e6
Reviewed-on: https://chromium-review.googlesource.com/959685
Commit-Queue: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/595ae15cfac7b314a63c4092037e26470faa1d82/luci_notify/notify/notify_test.go
[modify] https://crrev.com/595ae15cfac7b314a63c4092037e26470faa1d82/luci_notify/notify/pubsub.go
[modify] https://crrev.com/595ae15cfac7b314a63c4092037e26470faa1d82/luci_notify/notify/pubsub_test.go

The updated luci-notify is now deployed (and no longer crashing), but also isn't generating email.

The logs look like the email_notify properties aren't being found at all.
Example build:

https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/buildDetails?buildbucketId=8952211839172269888

Has this for parameters_json:

"parameters_json": "{\"builder_name\": \"Generic\", \"email_notify\": [{\"email\": \"dgarrett@google.com\"}], \"properties\": {\"bot\": [\"success-build\"], \"cbb_branch\": \"master\", \"cbb_config\": \"success-build\", \"cbb_display_label\": \"tryjob\", \"cbb_email\": \"dgarrett@google.com\", \"cbb_extra_args\": [\"--remote-trybot\", \"-b\", \"master\"], \"email\": [\"dgarrett@google.com\"], \"extra_args\": [\"--remote-trybot\", \"-b\", \"master\"], \"name\": \"\", \"owners\": [\"dgarrett@google.com\"], \"user\": \"dgarrett\"}}",


I'm tempted to produce local builds with more logging and push them to the dev environment, but that doesn't seem see the builds from our swarming tryjob environment at all.
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4

commit f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 16 15:32:55 2018

luci-notify: Extract email_notify as a parameter.

extract_notify was being extracted as a property, not a parameter, so
move it up a level.

BUG= chromium:816652 

Change-Id: Id22f72835e91eb8e240e39ec0ed44cd2c03edf03
Reviewed-on: https://chromium-review.googlesource.com/963480
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4/luci_notify/notify/notify.go
[modify] https://crrev.com/f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4/luci_notify/notify/notify_test.go
[modify] https://crrev.com/f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4/luci_notify/notify/pubsub.go
[modify] https://crrev.com/f1de91d8f98a514c280bf6fcfe87b5ed9ab8aab4/luci_notify/notify/pubsub_test.go

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/f427ea699e154985bdee6ac429f667bc4194618e

commit f427ea699e154985bdee6ac429f667bc4194618e
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 16 23:10:46 2018

luci-notify: Address nits from CL:963480

I got in a rush and submitted CL:963480 without addressing the final
nits. Cleaning them up here.

BUG= chromium:816652 

Change-Id: Iab46fdd5e02bda1f619d989e8f897257b7dc52ca
Reviewed-on: https://chromium-review.googlesource.com/967404
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/f427ea699e154985bdee6ac429f667bc4194618e/luci_notify/notify/pubsub.go

This is now working for ChromeOS swarming tryjobs:


Sample Email Text:


luci-notify detected a status change for builder "Generic" at 2018-03-16 23:30:27.90618 +0000 UTC.
New status:	Success
Previous status:	unknown status -1
Bucket:	luci.chromeos.general
Created by:	user:dgarrett@google.com
Created at:	2018-03-16 23:19:01.7003 +0000 UTC
Finished at:	2018-03-16 23:30:27.07968 +0000 UTC
Full details are available here.


The UI linked by the email is:

https://ci.chromium.org/p/chromeos/builds/b8951855506267514288
And the subject was:

[Build Status] Builder Generic on luci.chromeos.general



Three things it would be nice to change:

1) Remove "Previous Status" it will always be Unknown for these notifications.
2) List the cbuildbot build config in the subject/email
3) Link to the Legoland build UI instead.

1) is really trivial.
2) Could be addressed by passing in an additional value or two to the email request.
3) Is really hard. It can't be passed in, since the buildbucket_id can't be known
   until after the buildbucket request is submitted.


However, I believe this is good enough to remove the blocker on "swarming by default" for tryjobs.
Cc: athom@google.com mar...@chromium.org tandrii@chromium.org
 Issue 802923  has been merged into this issue.

Comment 19 by no...@chromium.org, Mar 21 2018

I guess in (2) we could introduce some simple templating mechanism, e.g. ${build_id}. That would address (3).

do you want to keep this bug open and assigned to you or close this one and file another to improve email formatting?

Comment 20 by no...@chromium.org, Mar 21 2018

thinking about this more:

we cannot accept HTML embedded in builds. That would be insecure, and a bit weird, too. If we do accept user-supplied extra content, it will probably be wrapped in <pre> in the email => we cannot insert a link to GE. Templates make this more complicated, too.

I agree that what we have now is good enough, but going forward you'd probably want to customize your emails, add more CrOS-specific content. A GE link is only a beginning? In that case, shouldn't GE be the service that sends the email with content oriented to CrOS devs?

Today, it is possible to create a pubsub topic in your cloud project and specify it when creating a build in chromite. The topic doesn't have to be GE-specific, rather CrOS-specific. Then GE can subscribe to the topic and send nice CrOS-oriented emails, if needed.

I was thinking about more, and wondering creating a template mechanism that could pull from arbitrary tags, but delivering templates via a notification config.

I agree that setting the entire email contents in each build request is a bad idea.

Comment 22 by no...@chromium.org, Mar 21 2018

Good idea. Email templates with ids in project-level config_set SGTM. The email_notify entries (the JSON objects) could specify the template id.
Probably complex enough to be worth a short doc, as well as documentation after the fact.
Cc: jclinton@chromium.org
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/207fa35fd21e35e53046269631839fdb79149427

commit 207fa35fd21e35e53046269631839fdb79149427
Author: Don Garrett <dgarrett@google.com>
Date: Fri Mar 30 22:08:07 2018

Create configuration for luci-notify email templates.

This creates the concept of email templates in the luci-notify
configuration files, but doesn't do anything with them yet.

An arbitrary number of named templates can be defined and used by name
from both build requests in parameters_json, and from notifier
definitions.

BUG= chromium:816652 
Change-Id: I5f6d1320358b6d4c6ec97f1576c9b4f8529fb60a
Reviewed-on: https://chromium-review.googlesource.com/982675
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/Makefile
[add] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/api/config/generate.go
[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/api/config/notify.pb.go
[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/api/config/notify.proto
[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/api/config/settings.pb.go
[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/api/config/testdata/basic.cfg
[modify] https://crrev.com/207fa35fd21e35e53046269631839fdb79149427/luci_notify/config/notifier.go

Status: Available (was: Started)
The design doc for this is at:
  go/luci-luci-notify-email-templates

A partial CL for the next step of implementation is at:
  https://crrev.com/c/989335
Components: Infra>Client>ChromeOS>CI
Owner: ----
Passing to jclinton for reprioritization/assignment.

This is a general feature, but has been somewhat loudly requested for ChromeOS tryjobs.
Cc: akes...@chromium.org
Owner: mikenichols@chromium.org
Status: Assigned (was: Available)
Mike, what do you think about taking this one?
Just to make it more fun.

This is in Chops code in Go, which is a different set of tools from ChromeOS/chromite.

Sure thing.  I'll see what I can do.

-- Mike
Cc: -tandrii@chromium.org
Project Member

Comment 34 by bugdroid1@chromium.org, Apr 18 2018

Labels: merge-merged-config
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/5a3e82702b4b431fac34d2553cdd7404d7fd7b76

commit 5a3e82702b4b431fac34d2553cdd7404d7fd7b76
Author: Mike Nichols <mikenichols@chromium.org>
Date: Wed Apr 18 23:13:00 2018

Cc: estaab@chromium.org
Project Member

Comment 36 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/367cada6a888391d7de5f58297bfe2b18a717a71

commit 367cada6a888391d7de5f58297bfe2b18a717a71
Author: Mike Nichols <mikenichols@chromium.org>
Date: Fri Apr 27 19:07:17 2018

[config] Implement ListFiles function to config interface

Provides means to retrieve file list for provided config set.  This functionality is required for the
introduction of email templates to luci-notify, as template names will be based on file names.

BUG= chromium:816652 

Change-Id: Iea335bb50fa98a880c3e46f53eabd862dab3c7b5
Reviewed-on: https://chromium-review.googlesource.com/1026847
Commit-Queue: Mike Nichols <mikenichols@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/erroring/erroring.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/filesystem/fs.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/filesystem/fs_test.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/memory/memory.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/memory/memory_test.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/remote/remote.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/impl/remote/remote_test.go
[modify] https://crrev.com/367cada6a888391d7de5f58297bfe2b18a717a71/config/interface.go

Project Member

Comment 37 by bugdroid1@chromium.org, May 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/64aa358496c614a4e6c8210c2fa89d9451cb3c1b

commit 64aa358496c614a4e6c8210c2fa89d9451cb3c1b
Author: Mike Nichols <mikenichols@chromium.org>
Date: Tue May 15 19:51:19 2018

[notify] Replace backend with config interface for luci_notify.

Replaces the use of backend with a config interface within context.
Additionally fixes an issue with memory implementation in which the
contents were being returned rather than the file names.

BUG= chromium:816652 

Change-Id: I6d9aa8ecf06891bdd26e504e20e71e66a5fc8ef5
Reviewed-on: https://chromium-review.googlesource.com/1058429
Commit-Queue: Mike Nichols <mikenichols@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/64aa358496c614a4e6c8210c2fa89d9451cb3c1b/config/impl/memory/memory.go
[modify] https://crrev.com/64aa358496c614a4e6c8210c2fa89d9451cb3c1b/config/impl/memory/memory_test.go
[modify] https://crrev.com/64aa358496c614a4e6c8210c2fa89d9451cb3c1b/luci_notify/config/config.go
[modify] https://crrev.com/64aa358496c614a4e6c8210c2fa89d9451cb3c1b/luci_notify/config/settings.go
[modify] https://crrev.com/64aa358496c614a4e6c8210c2fa89d9451cb3c1b/luci_notify/frontend/main.go

Project Member

Comment 39 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4

commit 6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4
Author: Mike Nichols <mikenichols@chromium.org>
Date: Thu May 17 22:00:36 2018

[notify] Functionality to allow luci-notify email templates per project.

Email templates are configured per project to allow for
personalization per team/project.

BUG= chromium:816652 

Change-Id: Iccc578acd03a416194b3a79fd5fa5df8270c93a9
Reviewed-on: https://chromium-review.googlesource.com/1044497
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[modify] https://crrev.com/6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4/luci_notify/notify/notify.go
[modify] https://crrev.com/6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4/luci_notify/notify/notify_test.go
[modify] https://crrev.com/6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4/luci_notify/notify/pubsub.go
[modify] https://crrev.com/6d8e1ba9d12a7e3efffd91bddbc39d9652c7bad4/luci_notify/notify/pubsub_test.go

Comment 40 by no...@chromium.org, May 18 2018

Cc: mikenichols@chromium.org
Owner: no...@chromium.org
Status: Stared (was: Assigned)

Comment 41 by no...@chromium.org, May 18 2018

Blockedon: 843718
Project Member

Comment 42 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/36407bce1574f8abbf5edc5d47061972901927fc

commit 36407bce1574f8abbf5edc5d47061972901927fc
Author: Nodir Turakulov <nodir@google.com>
Date: Fri May 18 06:00:36 2018

[luci-notify] Replicate email templates in the datastore

Code that runs frequently must not contact luci-config directly because
luci-config is not designed for this scale and it is slower.

In the existing cron job that fetches project config from luci-config, use
new ListFiles API to fetch names of all email templates of the project,
then fetch them and save in a new EmailTemplate datastore entity.
It happens in the same Datastore transaction as the luci-notify.cfg file.
The root entity is the project itself.

Also add tests for config ingestion, and change notifier name regex to allow
dash and digits.

Bug:  816652 
Change-Id: Ie8e856cd9a2bb67de7a2e7855e4a9bd4c5722c84
Reviewed-on: https://chromium-review.googlesource.com/1064890
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/config.go
[add] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/config_test.go
[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/emailtemplate.go
[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/emailtemplate_test.go
[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/project.go
[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/validate.go
[modify] https://crrev.com/36407bce1574f8abbf5edc5d47061972901927fc/luci_notify/config/validate_test.go

Comment 43 by no...@chromium.org, May 18 2018

Status: Stared (was: stared)
Project Member

Comment 44 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/13f79dadfbe01820f0c71ffd0a8b8b95a3a9812a

commit 13f79dadfbe01820f0c71ffd0a8b8b95a3a9812a
Author: Nodir Turakulov <nodir@google.com>
Date: Fri May 18 17:54:26 2018

[luci-notify] add EmailTemplate.DefinitionURL

If a template is bad, the system should help the user diagnose the problem or
at least find the owner of the template or who made the most recent change.

Store URL of the template config file in EmailTemplate entity.

Bug:  816652 
Change-Id: I0e180406808d7dd8b4e2f5f857dfb0f4ff8c839f
Reviewed-on: https://chromium-review.googlesource.com/1066272
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/13f79dadfbe01820f0c71ffd0a8b8b95a3a9812a/luci_notify/config/config_test.go
[modify] https://crrev.com/13f79dadfbe01820f0c71ffd0a8b8b95a3a9812a/luci_notify/config/emailtemplate.go
[modify] https://crrev.com/13f79dadfbe01820f0c71ffd0a8b8b95a3a9812a/luci_notify/config/emailtemplate_test.go

Project Member

Comment 45 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/b22f1dd8da355ae8a7671b04915a22a994796191

commit b22f1dd8da355ae8a7671b04915a22a994796191
Author: Nodir Turakulov <nodir@chromium.org>
Date: Fri May 18 20:42:41 2018

[luci-notify-dev] Add email templates

Add a notifier and a test email template.

TBR=vadimsh@chromium.org
Bug:  816652 
Change-Id: Iae7c22d63d2b07cc9bd6b6e8800d3b5c08191a2f
Reviewed-on: https://chromium-review.googlesource.com/1066670
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[add] https://crrev.com/b22f1dd8da355ae8a7671b04915a22a994796191/luci-notify-dev/email-templates/test.template
[add] https://crrev.com/b22f1dd8da355ae8a7671b04915a22a994796191/luci-notify-dev.cfg
[add] https://crrev.com/b22f1dd8da355ae8a7671b04915a22a994796191/luci-notify-dev/email-templates/shared.template

Project Member

Comment 46 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/3154e7190f2e9c0aff3ddbe9ed809e6155bc1503

commit 3154e7190f2e9c0aff3ddbe9ed809e6155bc1503
Author: Nodir Turakulov <nodir@chromium.org>
Date: Fri May 18 21:31:18 2018

[luci-notify] Update nodir-spam notifier

Not enough spam.

TBR=vadimsh@chromium.org
Bug:  816652 
Change-Id: Ib8801c97e2a0f34fdfbbf7fe3423d7c7f5a31242
Reviewed-on: https://chromium-review.googlesource.com/1066735
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/3154e7190f2e9c0aff3ddbe9ed809e6155bc1503/luci-notify-dev.cfg

Project Member

Comment 47 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/f7ffc11df8a647e4284dd5e3ed9b3c4295aa234a

commit f7ffc11df8a647e4284dd5e3ed9b3c4295aa234a
Author: Nodir Turakulov <nodir@chromium.org>
Date: Fri May 18 23:07:38 2018

[luci-notify-dev] Update nodir_spam notifier

luci-notify-dev listens to cr-buildbucket-dev, not cr-buildbucket.
Use cr-buildbucket-dev's builders.

TBR=vadimsh@chromium.org

Bug:  816652 
Change-Id: Ib5fe74bbc16725106f154f0d3be75ba7f8745b76
Reviewed-on: https://chromium-review.googlesource.com/1065586
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/f7ffc11df8a647e4284dd5e3ed9b3c4295aa234a/luci-notify-dev.cfg

Project Member

Comment 48 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/83783f66d14cbb65508b89cd7ead355c02ce8727

commit 83783f66d14cbb65508b89cd7ead355c02ce8727
Author: Nodir Turakulov <nodir@google.com>
Date: Fri May 18 23:22:26 2018

[luci-notify] Add email_templates.md

email_templates.md explains the email template feature and provides examples.

Bug:  816652 
Change-Id: I564755fdf91fa7ad6374d3c3a06ef7170d7823d7
Reviewed-on: https://chromium-review.googlesource.com/1066698
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/83783f66d14cbb65508b89cd7ead355c02ce8727/luci_notify/api/config/notify.proto
[add] https://crrev.com/83783f66d14cbb65508b89cd7ead355c02ce8727/luci_notify/doc/email_templates.md

Project Member

Comment 49 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/ff1e57130df6d9a3e97c05699a1e6c4d4d9f1fa9

commit ff1e57130df6d9a3e97c05699a1e6c4d4d9f1fa9
Author: Nodir Turakulov <nodir@google.com>
Date: Fri May 18 23:30:36 2018

[luci-notify] Compare project rev early

Compare config revisions before fetching and parsing all configs.

Bug:  816652 
Change-Id: I16932a0d120502f7866dc9cbb9cd98a29d7f0d9f
Reviewed-on: https://chromium-review.googlesource.com/1066852
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/ff1e57130df6d9a3e97c05699a1e6c4d4d9f1fa9/luci_notify/config/config.go

Project Member

Comment 50 by bugdroid1@chromium.org, May 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/342404c3edd79868efe60d78cb1f665aae8ee2c6

commit 342404c3edd79868efe60d78cb1f665aae8ee2c6
Author: Nodir Turakulov <nodir@chromium.org>
Date: Sat May 19 16:22:50 2018

[luci-notify] Fix test template

s/ViewURL/ViewUrl/

TBR=vadimsh@chromium.org

Bug:  816652 
Change-Id: I57f56ebeeb21c72e1a34ec689fe4f8772a9e61d1
Reviewed-on: https://chromium-review.googlesource.com/1067077
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/342404c3edd79868efe60d78cb1f665aae8ee2c6/luci-notify-dev/email-templates/test.template

Project Member

Comment 51 by bugdroid1@chromium.org, May 22 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/355acbbfaf43449e67bc7ac1556d939e46164810

commit 355acbbfaf43449e67bc7ac1556d939e46164810
Author: Nodir Turakulov <nodir@google.com>
Date: Tue May 22 22:47:27 2018

[luci-noitfy] Improve email generation

Fetch email templates from the datastore, as opposed to luci-config.
luci-config is not designed to be queried at such high rate.

Add support for reusing one template in another. For example, one file
can define a template using {{define "foo"}}, and another file can call it
using {{template "foo" .}}. This is critical because we will probably want
the same presentation of steps, while having multiple build templates.
In this sense, a build template can call a steps template.

Cache parsed templates in process memory using lru package.
There is no point in parsing the same set of templates for each build.

Improve error handling. If a user-defined template fails to execute,
the system must tell the users what went wrong, as opposed to leave that
hidden in the server logs that require manual intervention by service
maintainers. Now an email is guaranteed.

Improve tests. They were hard to follow and weren't unity, e.g. they
tried to test all combinations of inputs without increasing code coverage.

Fix various bugs in the email generation code:
- invalid usage of closures: old notify.go L182-183
- actually handle (retry) transient errors, as opposed just ignore them.
  old notify.go L297 leads to NOT sending emails occasionally
- incorrect directory "email_templates" instead of "email-templates" (dash)
- templateContext used notify.Build instead of buildbucketpb.Build

Remove ParsedEmailTemplate. It turned out to be not useful.

Bug:  816652 
Change-Id: I3206f4058a2c20dac86d5b765b6857eaec531d0f
Reviewed-on: https://chromium-review.googlesource.com/1066177
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/common/data/caching/lru/lru.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/config/emailtemplate.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/config/emailtemplate_test.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/config/validate.go
[add] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/notify/emailgen.go
[add] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/notify/emailgen_test.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/notify/notify.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/notify/notify_test.go
[modify] https://crrev.com/355acbbfaf43449e67bc7ac1556d939e46164810/luci_notify/notify/pubsub_test.go
[delete] https://crrev.com/4fdafa492c30ae66e73b356f7ad3beab4a926407/luci_notify/testutil/data.go

Project Member

Comment 52 by bugdroid1@chromium.org, May 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/e49126baa772c2f55f64ac1bc1863bc41e497969

commit e49126baa772c2f55f64ac1bc1863bc41e497969
Author: Nodir Turakulov <nodir@google.com>
Date: Wed May 23 05:24:07 2018

[luci-notify] Deduplicate emails

Deduplicate per-email push tasks using task name.
Generate a task name based on email, template and build id.
Create tasks always non-transactionally.

Bug:  816652 
Change-Id: If352a4ee4870a4ddcbbcd83ac767493341e52d38
Reviewed-on: https://chromium-review.googlesource.com/1069782
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/e49126baa772c2f55f64ac1bc1863bc41e497969/luci_notify/internal/tq.proto
[modify] https://crrev.com/e49126baa772c2f55f64ac1bc1863bc41e497969/luci_notify/notify/notify.go
[modify] https://crrev.com/e49126baa772c2f55f64ac1bc1863bc41e497969/luci_notify/notify/notify_test.go
[modify] https://crrev.com/e49126baa772c2f55f64ac1bc1863bc41e497969/luci_notify/notify/pubsub_test.go

Comment 53 by no...@chromium.org, May 23 2018

Blockedon: -843718 843453
we will take a little bit different approach for retrieving build steps. Instead of receiving a full build with steps from PubSub, we will only receive a build id in PubSub and use v2 API to retrieve steps. PubSub v1 is sufficient for that.
Project Member

Comment 54 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/8bc2aa83f9ed90af1e183df54067595519f075d8

commit 8bc2aa83f9ed90af1e183df54067595519f075d8
Author: Nodir Turakulov <nodir@chromium.org>
Date: Thu May 24 01:08:30 2018

[luci-notify] Add steps template

TBR=vadimsh@chromium.org

Bug:  816652 
Change-Id: I68c44c2461b1d58de8c6c19d43fdb7d2a7095b03
Reviewed-on: https://chromium-review.googlesource.com/1071110
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[add] https://crrev.com/8bc2aa83f9ed90af1e183df54067595519f075d8/luci-notify-dev/email-templates/steps.template
[delete] https://crrev.com/7e214d09f75ff61e1bdd3960ddfddd8a967a1ed8/luci-notify-dev/email-templates/shared.template

Project Member

Comment 55 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/5ae5ff2125fda3f77f83e9f2dcf1089ccf72eca9

commit 5ae5ff2125fda3f77f83e9f2dcf1089ccf72eca9
Author: Nodir Turakulov <nodir@google.com>
Date: Thu May 24 01:22:28 2018

[luci-notify] Fetch build info via v2 RPC

Fetch build details, include steps, via v2 RPC.
Neither of PubSub v1 or v2 will provide step information.

Bug:  816652 
Change-Id: Ib0e157662d38a1ab67698fa06f6872e1a45c5ce7
Reviewed-on: https://chromium-review.googlesource.com/1070768
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/5ae5ff2125fda3f77f83e9f2dcf1089ccf72eca9/luci_notify/notify/pubsub.go

Project Member

Comment 56 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/2f5df25847c06eab37ed774e13f81bd4dfee0708

commit 2f5df25847c06eab37ed774e13f81bd4dfee0708
Author: Nodir Turakulov <nodir@chromium.org>
Date: Thu May 24 01:27:50 2018

[luci-notify] Switch nodir-spam to infra-continuous-trusty-64

luci-notify doesn't send emails for non-CI builds.

TBR=vadimsh@chromium.org
Bug:  816652 
Change-Id: I4a4e61c0c80488f566f095712a52d485f5e189de
Reviewed-on: https://chromium-review.googlesource.com/1071113
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/2f5df25847c06eab37ed774e13f81bd4dfee0708/luci-notify-dev.cfg

Project Member

Comment 57 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/99d47c1ce459cc339da4b9bb26c80017ff115c02

commit 99d47c1ce459cc339da4b9bb26c80017ff115c02
Author: Nodir Turakulov <nodir@google.com>
Date: Thu May 24 01:45:48 2018

[luci-notify] s/Host/Hostname/ in pubsub

This diff should have been a part of
https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/1070768
but I forgot to upload it.

TBR=mknyszek@google.com

Bug:  816652 
Change-Id: Ibc7b237f8adbbe26427f8a4dddaeb0258844fc4b
Reviewed-on: https://chromium-review.googlesource.com/1071115
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

[modify] https://crrev.com/99d47c1ce459cc339da4b9bb26c80017ff115c02/luci_notify/notify/pubsub.go

Project Member

Comment 58 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/5a7aa04d1693c1df2ac0270591900df4c33ba86b

commit 5a7aa04d1693c1df2ac0270591900df4c33ba86b
Author: Nodir Turakulov <nodir@chromium.org>
Date: Thu May 24 01:50:00 2018

[luci-notify] Fix test template

TBR=vadimsh@chromium.org

Bug:  816652 
Change-Id: I6fdc1b949bc8e978b10aa942907d7a44daf3ed59
Reviewed-on: https://chromium-review.googlesource.com/1071116
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/5a7aa04d1693c1df2ac0270591900df4c33ba86b/luci-notify-dev/email-templates/test.template

Comment 59 by no...@chromium.org, May 24 2018

this is code complete. Verifying. 
Project Member

Comment 60 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/3c7627776aadce4448dd8f466e21c60ce4f6e05f

commit 3c7627776aadce4448dd8f466e21c60ce4f6e05f
Author: Nodir Turakulov <nodir@google.com>
Date: Thu May 24 02:16:18 2018

[luci-notify] Skip not found builds

luci-notify has access to the global pubsub channel, but may not have access
to individual buckets, so GetBuild RPC returns NotFound.
Skip such builds. For now, we will require an explicit permission for
luci-notify to be a reader of a bucket.

TBR=mknyszek@google.com
Bug:  816652 
Change-Id: I69fbe9af833819ebb120724516ab0dea1f148a96
Reviewed-on: https://chromium-review.googlesource.com/1070763
Commit-Queue: Nodir Turakulov <nodir@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/3c7627776aadce4448dd8f466e21c60ce4f6e05f/luci_notify/notify/pubsub.go

Comment 61 by no...@chromium.org, May 24 2018

Status: Fixed (was: stared)
Don could you verify that this WAI?

I recommend starting with test.template and send a tryjob that specifies it. If that works, then rename it to default.template

Docs: https://chromium.googlesource.com/infra/luci/luci-go/+/master/luci_notify/doc/email_templates.md

Comment 62 by no...@chromium.org, May 24 2018

Also please don’t reopen this bug. It is long and its scope is too large. Instead please open new bugs with small scope.

Sign in to add a comment