[Findit] Add settings for actions after culprits or suspects are identified.

[Findit] Add settings for actions after culprits or suspects are identified.

BUG=621140
Committed: https://chromium.googlesource.com/infra/infra/+/c1f42a4eba90432bd63ea4c07f168ddb8c2ce0a0

[stgao, 2016-06-28 20:48:09]

stgao@chromium.org changed reviewers:
+ chanli@chromium.org, lijeffrey@chromium.org

[stgao, 2016-06-28 20:48:09]

ptal

[lijeffrey, 2016-06-30 01:02:48]

lgtm with a nit

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:84: def
_PassNotificationTimeLimit(commit_time, latency_limit_seconds):
nit: I would rename this _NotificationTimeLimitPassed(...) or something similar,
and the variable name 'pass_time_limit' (which without more context can sound
like it's meant to ignore a timeout) to 'time_limit_passed' or something to make
it more clear that this is indeed for timeout.

[stgao, 2016-06-30 17:35:33]

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:84: def
_PassNotificationTimeLimit(commit_time, latency_limit_seconds):
On 2016/06/30 01:02:48, lijeffrey wrote:
> nit: I would rename this _NotificationTimeLimitPassed(...) or something
similar,
> and the variable name 'pass_time_limit' (which without more context can sound
> like it's meant to ignore a timeout) to 'time_limit_passed' or something to
make
> it more clear that this is indeed for timeout.

hm, this is actually not a timeout.
But the rename makes sense.

[chanli, 2016-06-30 20:23:42]

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:87: return
latency_seconds > latency_limit_seconds
Though I think this is just an implementation detail, but why do you want to use
seconds instead of minutes here? IMO, minutes are more reasonable here.

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:98:
'cr_notification_latency_limit_seconds', 1)
are 100000 and 1 default values or some impossible values to prevent
notification to be sent?

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py:40:
def _MockDatetimeUtcNow(self, mocked_utcnow):
This should be trivial: I noticed that for all _MockDatetimeUtcNow you used
below you are passing _MOCKED_DATETIME_UTCNOW in, so this could be changed to:
def _MockDatetimeUtcNow(self):
    class Mocked_Datetime(object):
      @staticmethod
      def utcnow():
        return _MOCKED_DATETIME_UTCNOW
    self.mock(send_notification_for_culprit_pipeline,
              'datetime', Mocked_Datetime)

[stgao, 2016-06-30 22:36:57]

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:87: return
latency_seconds > latency_limit_seconds
On 2016/06/30 20:23:41, chanli wrote:
> Though I think this is just an implementation detail, but why do you want to
use
> seconds instead of minutes here? IMO, minutes are more reasonable here.

We could do minutes. Done.

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:98:
'cr_notification_latency_limit_seconds', 1)
On 2016/06/30 20:23:41, chanli wrote:
> are 100000 and 1 default values or some impossible values to prevent
> notification to be sent?

Yes, added a comment on this.

[stgao, 2016-06-30 22:37:45]

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
File
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py:40:
def _MockDatetimeUtcNow(self, mocked_utcnow):
On 2016/06/30 20:23:42, chanli wrote:
> This should be trivial: I noticed that for all _MockDatetimeUtcNow you used
> below you are passing _MOCKED_DATETIME_UTCNOW in, so this could be changed to:
> def _MockDatetimeUtcNow(self):
>     class Mocked_Datetime(object):
>       @staticmethod
>       def utcnow():
>         return _MOCKED_DATETIME_UTCNOW
>     self.mock(send_notification_for_culprit_pipeline,
>               'datetime', Mocked_Datetime)

Done.

[chanli, 2016-06-30 22:43:54]

On 2016/06/30 22:37:45, stgao wrote:
>
https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
> File
> appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
> (right):
> 
>
https://codereview.chromium.org/2104113002/diff/1/appengine/findit/waterfall/...
>
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py:40:
> def _MockDatetimeUtcNow(self, mocked_utcnow):
> On 2016/06/30 20:23:42, chanli wrote:
> > This should be trivial: I noticed that for all _MockDatetimeUtcNow you used
> > below you are passing _MOCKED_DATETIME_UTCNOW in, so this could be changed
to:
> > def _MockDatetimeUtcNow(self):
> >     class Mocked_Datetime(object):
> >       @staticmethod
> >       def utcnow():
> >         return _MOCKED_DATETIME_UTCNOW
> >     self.mock(send_notification_for_culprit_pipeline,
> >               'datetime', Mocked_Datetime)
> 
> Done.

lgtm

[stgao, 2016-06-30 22:48:52]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-06-30 22:48:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
lijeffrey@chromium.org
Link to the patchset: https://codereview.chromium.org/2104113002/#ps40001
(title: "Address chan's comments.")

[commit-bot: I haz the power, 2016-06-30 22:48:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 23:31:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 23:31:49]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Precise...)

[stgao, 2016-07-01 15:19:09]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2016-07-01 15:19:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 15:33:29]

Description was changed from

==========
[Findit] Add settings for actions after culprits or suspects are identified.

BUG=621140
==========

to

==========
[Findit] Add settings for actions after culprits or suspects are identified.

BUG=621140

Committed:
https://chromium.googlesource.com/infra/infra/+/c1f42a4eba90432bd63ea4c07f168...
==========

[commit-bot: I haz the power, 2016-07-01 15:33:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/c1f42a4eba90432bd63ea4c07f168...

[commit-bot: I haz the power, 2016-07-01 15:33:31]

CQ bit was unchecked.