[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict with the builtin endpoints on appengine.

BUG=656666
Committed: https://chromium.googlesource.com/infra/infra/+/a3a4209bca51591d801794f0c7ae810368610135

[stgao, 2016-10-21 01:19:32]

Description was changed from

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

BUG=656666
==========

to

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to
	appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict
with the builtin endpoints on appengine.

BUG=656666
==========

[stgao, 2016-10-21 01:19:52]

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

[stgao, 2016-10-21 01:19:53]

ptal

[stgao, 2016-10-21 01:20:09]

Description was changed from

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to
	appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict
with the builtin endpoints on appengine.

BUG=656666
==========

to

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to
appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict with
the builtin endpoints on appengine.

BUG=656666
==========

[Sergiy Byelozyorov, 2016-10-21 14:54:59]

LGTM for chromium-try-flakes parts

[lijeffrey, 2016-10-21 17:10:28]

mostly looks good from Findit end, just a few comments/nits

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:46: test, test,
allow_new_analysis=allow_new_analysis, force=force,
nit: Just so this doesn't look like a typo (because test shows up twice), would
it make sense to rename this 

original_test = TestInfo(...)
normalized_test = original_test
analysis = intitialize_flake_pipeline.ScheduleAnalysisIfNeeded(normalized_test,
original_test, ...)?

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:45: def
PopulateInfoToAnalysis(analysis):
nit: How about rename this PopulateAnalysisInfo? Also would it make sense to
make this a member function of master_flake_analysis, that takes TestInfo as
input for the populating? I don't feel particularly strongly about this so
leaving it here is probably fine

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:33: "%s / %s / %s"
by rerun against build artifacts from Waterfall. Please visit
nit: by rerunning

Also do we want to mention swarming was used?

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:35: Automatically
posted by the findit-for-me app.
nit: should we add the go/findit-site link after findit-for-me app?

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

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test_info.py:19: self.step_name, self.test_name)
just to make sure the base64 encoding we use for test_name is only when saving
to the datastore and isn't necessary here, correct?

[stgao, 2016-10-21 22:42:34]

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:46: test, test,
allow_new_analysis=allow_new_analysis, force=force,
On 2016/10/21 17:10:27, lijeffrey wrote:
> nit: Just so this doesn't look like a typo (because test shows up twice),
would
> it make sense to rename this 
> 
> original_test = TestInfo(...)
> normalized_test = original_test
> analysis =
intitialize_flake_pipeline.ScheduleAnalysisIfNeeded(normalized_test,
> original_test, ...)?

Done.

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/initialize_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/initialize_flake_pipeline.py:45: def
PopulateInfoToAnalysis(analysis):
On 2016/10/21 17:10:27, lijeffrey wrote:
> nit: How about rename this PopulateAnalysisInfo? Also would it make sense to
> make this a member function of master_flake_analysis, that takes TestInfo as
> input for the populating? I don't feel particularly strongly about this so
> leaving it here is probably fine

TestInfo is not a NDB model so it is not in model/. Having model/ depend on
waterfall/ seems not a good idea. So I will leave this function here.

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:33: "%s / %s / %s"
by rerun against build artifacts from Waterfall. Please visit
On 2016/10/21 17:10:27, lijeffrey wrote:
> nit: by rerunning
> 
> Also do we want to mention swarming was used?

Done.

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:35: Automatically
posted by the findit-for-me app.
On 2016/10/21 17:10:27, lijeffrey wrote:
> nit: should we add the go/findit-site link after findit-for-me app?

Good idea. But go with a public short link https://goo.gl/YTKnaU

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

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/test_info.py:19: self.step_name, self.test_name)
On 2016/10/21 17:10:27, lijeffrey wrote:
> just to make sure the base64 encoding we use for test_name is only when saving
> to the datastore and isn't necessary here, correct?

Right, here is more for logging.

[chanli, 2016-10-21 23:23:09]

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:46: test, test,
allow_new_analysis=allow_new_analysis, force=force,
So when does the normalization happen? For example when is the 'PRE_' prefix
removed?

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:36: Feedback are
welcome in Tools>Test>FindIt>Flakiness!""") % (
Nit: using component Tools>Test>FindIt>Flakiness?

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

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/post_comment_to_bug_pipeline.py:15: def
_GetIssue(bug_id, issue_tracker):
Any particular reason you want to make it static?

[stgao, 2016-10-22 00:15:09]

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
File appengine/findit/handlers/flake/check_flake.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/handlers/f...
appengine/findit/handlers/flake/check_flake.py:46: test, test,
allow_new_analysis=allow_new_analysis, force=force,
On 2016/10/21 23:23:08, chanli wrote:
> So when does the normalization happen? For example when is the 'PRE_' prefix
> removed?

The "PRE_" is a TODO on your end; it's up to you where it should be done. I just
mentioned it as an example for what's normalization.

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
File appengine/findit/waterfall/flake/recursive_flake_pipeline.py (right):

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/flake/recursive_flake_pipeline.py:36: Feedback are
welcome in Tools>Test>FindIt>Flakiness!""") % (
On 2016/10/21 23:23:08, chanli wrote:
> Nit: using component Tools>Test>FindIt>Flakiness? 

Done.

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

https://codereview.chromium.org/2438673004/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/post_comment_to_bug_pipeline.py:15: def
_GetIssue(bug_id, issue_tracker):
On 2016/10/21 23:23:09, chanli wrote:
> Any particular reason you want to make it static?

No. Moved out of the class into the module instead.

[lijeffrey, 2016-10-25 00:21:44]

Findit side lgtm

[stgao, 2016-11-04 19:27:16]

Patchset #4 (id:60001) has been deleted

[stgao, 2016-11-04 19:53:43]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-11-04 19:53:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergiyb@chromium.org, lijeffrey@chromium.org
Link to the patchset: https://codereview.chromium.org/2438673004/#ps100001
(title: "Add a config flag to enable/disable updating monorail bug.")

[commit-bot: I haz the power, 2016-11-04 19:53:47]

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-11-04 20:42:59]

Description was changed from

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to
appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict with
the builtin endpoints on appengine.

BUG=656666
==========

to

==========
[Findit] Post analysis results of flakes to bug filed by chromium-try-flakes.

Moved appengine/chromium_try_flakes/endpoints/ to
appengine/chromium_try_flakes/endpoints_client/ so that it doesn't conflict with
the builtin endpoints on appengine.

BUG=656666

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

[commit-bot: I haz the power, 2016-11-04 20:43:00]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/a3a4209bca51591d801794f0c7ae8...