[Findit] Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

[Findit] Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356
patch from issue 1914653003 at patchset 80001 (http://crrev.com/1914653003#ps80001)

Committed: https://chromium.googlesource.com/infra/infra/+/059e75397048f79d6bc4de3d399d7a7d3f213071

[Sharu Jiang, 2016-05-05 16:02:33]

Description was changed from

==========
Fix nits.


Fix nits.


Fix nits.


Fix nits.


Fix pylints.


fetch DEPS from buildspec/


intermediate


Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356

patch from issue 1914653003 at patchset 80001
(http://crrev.com/1914653003#ps80001)

Merge branch 'master' of https://chromium.googlesource.com/infra/infra into
buildspec


change to googleplex app.


Fix nits.
==========

to

==========
[Findit] Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356

patch from issue 1914653003 at patchset 80001
(http://crrev.com/1914653003#ps80001)
==========

[Sharu Jiang, 2016-05-05 16:02:33]

katesonia@chromium.org changed reviewers:
+ mbarbella@google.com, stgao@chromium.org

[Sharu Jiang, 2016-05-05 16:04:42]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-05-05 16:04:51]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-05-05 16:05:27]

PTAL :)

[Martin Barbella, 2016-05-05 16:37:37]

mbarbella@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Martin Barbella, 2016-05-05 16:37:38]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:68: def Load(self, _, version,
deps_file):
Is there any way to refactor DEPSLoader such that the unused argument isn't
needed? For example adding the URL in the DEPSDownloader constructor (not sure
if this would be clean since I didn't check the implementation, just throwing
the idea out there).

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:77: 
Additional blank line here.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:91: root_dep = dependency.Dependency(
Would it be possible or cleaner to use the converted revision when creating
this? It feels a bit hacky to replace revision after the fact.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:74: good_version, bad_version =
regression_versions
How about last_good_version, first_bad_version? Makes it a bit easier to parse
for someone who isn't as familiar with the algorithm.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:88: 'found': True,
Is this correct?

[stgao, 2016-05-05 17:32:29]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/app.yaml
File appengine/findit/app.yaml (left):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/app.ya...
appengine/findit/app.yaml:1: application: findit-for-me-dev
Why this change is needed?

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:66: class
DEPSDownloaderForChromeVersion(deps_parser.DEPSLoader):
Can we just reuse DEPSDownloader above with some slight change to it?

The code #69-#76 seems duplicate of the implementation in GitRepository.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:68: def Load(self, _, version,
deps_file):
This breaks the API expected by deps_parser.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:92: _CHROMIUM_ROOT_DIR,
_CHROMIUM_REPO_MASTER, revision, 'DEPS')
When reusing DEPSDownloader above, this might need some change.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:107: # Convert chrome version revision
to githash revision to be consistent.
"version revision" seems a bit confusing.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:108: root_dep.revision =
repo.GetRevisionForChromeVersion(root_dep.revision)
If we convert the version to git-hash here, will that have side-effect on
pulling changelogs in the regression range?

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:23: historic_metadata (dict): Mapping from
Chrome version to historic metadata.
Is it still a dict?

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:96: 'solution': 'core_algorithm',
Is it always 'core_algorithm'? What if there is no regression range detected --
is git blame used instead?

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas_crash_pipeline.py:82: 'crash_identifiers':
crash_identifiers,
Should we include the ``client_id`` here too?
This is not urgent although.

[Sharu Jiang, 2016-05-05 20:26:08]

Patchset #2 (id:60001) has been deleted

[Sharu Jiang, 2016-05-05 20:26:45]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/app.yaml
File appengine/findit/app.yaml (left):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/app.ya...
appengine/findit/app.yaml:1: application: findit-for-me-dev
On 2016/05/05 17:32:28, stgao wrote:
> Why this change is needed?

Sorry, this was accidentally removed, will change it back.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:66: class
DEPSDownloaderForChromeVersion(deps_parser.DEPSLoader):
On 2016/05/05 17:32:28, stgao wrote:
> Can we just reuse DEPSDownloader above with some slight change to it?
> 
> The code #69-#76 seems duplicate of the implementation in GitRepository.

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:68: def Load(self, _, version,
deps_file):
On 2016/05/05 16:37:37, Martin Barbella wrote:
> Is there any way to refactor DEPSLoader such that the unused argument isn't
> needed? For example adding the URL in the DEPSDownloader constructor (not sure
> if this would be clean since I didn't check the implementation, just throwing
> the idea out there).

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:77: 
On 2016/05/05 16:37:37, Martin Barbella wrote:
> Additional blank line here.

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:91: root_dep = dependency.Dependency(
On 2016/05/05 16:37:37, Martin Barbella wrote:
> Would it be possible or cleaner to use the converted revision when creating
> this? It feels a bit hacky to replace revision after the fact.

I have to keep it as a version until we call
deps_parser.UpdateDependencyTree(root_dep, [platform], deps_loader), since we
need the version to pull DEPS from buildspec/ within it.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:92: _CHROMIUM_ROOT_DIR,
_CHROMIUM_REPO_MASTER, revision, 'DEPS')
On 2016/05/05 17:32:28, stgao wrote:
> When reusing DEPSDownloader above, this might need some change.

It seems no change is needed.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:107: # Convert chrome version revision
to githash revision to be consistent.
On 2016/05/05 17:32:28, stgao wrote:
> "version revision" seems a bit confusing.

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:108: root_dep.revision =
repo.GetRevisionForChromeVersion(root_dep.revision)
On 2016/05/05 17:32:28, stgao wrote:
> If we convert the version to git-hash here, will that have side-effect on
> pulling changelogs in the regression range?

No, after this, pulling changelogs would be exactly the same.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:23: historic_metadata (dict): Mapping from
Chrome version to historic metadata.
On 2016/05/05 17:32:28, stgao wrote:
> Is it still a dict?

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:74: good_version, bad_version =
regression_versions
On 2016/05/05 16:37:37, Martin Barbella wrote:
> How about last_good_version, first_bad_version? Makes it a bit easier to parse
> for someone who isn't as familiar with the algorithm.

Done.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:88: 'found': True,
On 2016/05/05 16:37:37, Martin Barbella wrote:
> Is this correct?

No, updated it.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:96: 'solution': 'core_algorithm',
On 2016/05/05 17:32:28, stgao wrote:
> Is it always 'core_algorithm'? What if there is no regression range detected
--
> is git blame used instead?

Right now, we don't support the git blame yet because people has concerns about
the git blame ranking. Once we enable the git blame, I will modify this.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas_crash_pipeline.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas_crash_pipeline.py:82: 'crash_identifiers':
crash_identifiers,
On 2016/05/05 17:32:28, stgao wrote:
> Should we include the ``client_id`` here too?
> This is not urgent although.

Done.

[stgao, 2016-05-05 21:02:47]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:108: root_dep.revision =
repo.GetRevisionForChromeVersion(root_dep.revision)
On 2016/05/05 20:26:45, sharu jiang wrote:
> On 2016/05/05 17:32:28, stgao wrote:
> > If we convert the version to git-hash here, will that have side-effect on
> > pulling changelogs in the regression range?
> 
> No, after this, pulling changelogs would be exactly the same.

What if there are patch cherrypick in the Chrome branches?

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:96: 'solution': 'core_algorithm',
On 2016/05/05 20:26:45, sharu jiang wrote:
> On 2016/05/05 17:32:28, stgao wrote:
> > Is it always 'core_algorithm'? What if there is no regression range detected
> --
> > is git blame used instead?
> 
> Right now, we don't support the git blame yet because people has concerns
about
> the git blame ranking. Once we enable the git blame, I will modify this.

In that case, if we don't have regression range, should we still run the
analysis above in #79?

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/app.yaml
File appengine/findit/app.yaml (left):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/app.ya...
appengine/findit/app.yaml:1: application: findit-for-me-dev
?

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:46: repo =
git_repository.GitRepository(_BUILDSPEC_REPO, http_client)
Can we handled both in the same way?

In abstract, both are to retrieve the DEPS file at the given deps_file path from
the given Gitiles repo at the given revision.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:47: url = '%s/+/master/releases/%s/%s'
% (_BUILDSPEC_REPO, revision,
Why can't we use the repo_url passed in here?

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:52: content =
repo._SendRequestForTextResponse(url, headers=headers)
Class-internal functions like this are not supposed to be used outside the
class.

Is there other cleaner solution?

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/git_repository.py:47: def
_SendRequestForJsonResponse(self, url, params=None, headers=None):
It seems not clean for the GitRepository to know about http headers which should
be handled by HttpClient.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:89: 'found': (bool(suspected_dep) or
bool(suspected_components) or
Should we not consider `suspected_dep` and `suspected_components` for now as
they are not implemented yet? Another question is how we define the meaning of
"found" here, should it be culprit only or broader scope?

[Sharu Jiang, 2016-05-05 23:03:03]

Patchset #3 (id:100001) has been deleted

[Sharu Jiang, 2016-05-05 23:03:45]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:108: root_dep.revision =
repo.GetRevisionForChromeVersion(root_dep.revision)
On 2016/05/05 21:02:46, stgao wrote:
> On 2016/05/05 20:26:45, sharu jiang wrote:
> > On 2016/05/05 17:32:28, stgao wrote:
> > > If we convert the version to git-hash here, will that have side-effect on
> > > pulling changelogs in the regression range?
> > 
> > No, after this, pulling changelogs would be exactly the same.
> 
> What if there are patch cherrypick in the Chrome branches?

Hm... I see, in this case, we shouldn't do the conversion at all. We can treat
the chrome version the same as revision, since for pulling changelogs, they are
the same.

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:96: 'solution': 'core_algorithm',
On 2016/05/05 21:02:46, stgao wrote:
> On 2016/05/05 20:26:45, sharu jiang wrote:
> > On 2016/05/05 17:32:28, stgao wrote:
> > > Is it always 'core_algorithm'? What if there is no regression range
detected
> > --
> > > is git blame used instead?
> > 
> > Right now, we don't support the git blame yet because people has concerns
> about
> > the git blame ranking. Once we enable the git blame, I will modify this.
> 
> In that case, if we don't have regression range, should we still run the
> analysis above in #79?

In FindItForCrash, we checkout if the regression_deps_rolls is empty or not,
besides, I plan to add the git blame code inside FindItForCrash, so I still want
to run this function.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/app.yaml
File appengine/findit/app.yaml (left):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/app.ya...
appengine/findit/app.yaml:1: application: findit-for-me-dev
On 2016/05/05 21:02:46, stgao wrote:
> ?

Oops...

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:46: repo =
git_repository.GitRepository(_BUILDSPEC_REPO, http_client)
On 2016/05/05 21:02:47, stgao wrote:
> Can we handled both in the same way?
> 
> In abstract, both are to retrieve the DEPS file at the given deps_file path
from
> the given Gitiles repo at the given revision.

Done.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:47: url = '%s/+/master/releases/%s/%s'
% (_BUILDSPEC_REPO, revision,
On 2016/05/05 21:02:46, stgao wrote:
> Why can't we use the repo_url passed in here?

Done.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:52: content =
repo._SendRequestForTextResponse(url, headers=headers)
On 2016/05/05 21:02:47, stgao wrote:
> Class-internal functions like this are not supposed to be used outside the
> class.
> 
> Is there other cleaner solution?

Done.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/common...
appengine/findit/common/git_repository.py:47: def
_SendRequestForJsonResponse(self, url, params=None, headers=None):
On 2016/05/05 21:02:47, stgao wrote:
> It seems not clean for the GitRepository to know about http headers which
should
> be handled by HttpClient.

Done.

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/80001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:89: 'found': (bool(suspected_dep) or
bool(suspected_components) or
On 2016/05/05 21:02:47, stgao wrote:
> Should we not consider `suspected_dep` and `suspected_components` for now as
> they are not implemented yet? Another question is how we define the meaning of
> "found" here, should it be culprit only or broader scope?
I think the 'found' here means a broader scope, so we can at least have some
results, even it's just suspected components, or suspected projects. So I think
it's ok to leave them here?

The 'found_suspects' in tag dict should indicate whether we found
culprit_results or not.

[stgao, 2016-05-06 00:15:53]

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:50: http_client.Get =
functools.partial(http_client.Get, headers=headers)
This seems a little hacky to me.

An alternative is to make HttpClientAppengine attach the needed header when a
flag is set or something.

By the hostname in the url, we know when the header should be attached.

Will discuss with you on the details for this offline.

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:52: repo =
git_repository.GitRepository(_BUILDSPEC_REPO, http_client)
Can we unify this with the code below?

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/git_repository.py:261: def
GetRevisionForChromeVersion(self, version):
Is this still needed?

[Sharu Jiang, 2016-05-06 07:44:22]

Patchset #4 (id:140001) has been deleted

[Sharu Jiang, 2016-05-06 07:44:33]

Patchset #4 (id:160001) has been deleted

[Sharu Jiang, 2016-05-06 07:47:50]

Patchset #4 (id:180001) has been deleted

[Sharu Jiang, 2016-05-06 18:32:06]

Patchset #4 (id:200001) has been deleted

[Sharu Jiang, 2016-05-06 18:35:20]

Patchset #4 (id:220001) has been deleted

[Sharu Jiang, 2016-05-06 18:35:28]

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:50: http_client.Get =
functools.partial(http_client.Get, headers=headers)
On 2016/05/06 00:15:53, stgao wrote:
> This seems a little hacky to me.
> 
> An alternative is to make HttpClientAppengine attach the needed header when a
> flag is set or something.
> 
> By the hostname in the url, we know when the header should be attached.
> 
> Will discuss with you on the details for this offline.

Done.

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:52: repo =
git_repository.GitRepository(_BUILDSPEC_REPO, http_client)
On 2016/05/06 00:15:53, stgao wrote:
> Can we unify this with the code below?

Done.

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
File appengine/findit/common/git_repository.py (right):

https://codereview.chromium.org/1950123003/diff/120001/appengine/findit/commo...
appengine/findit/common/git_repository.py:261: def
GetRevisionForChromeVersion(self, version):
On 2016/05/06 00:15:53, stgao wrote:
> Is this still needed?

Noop.

[stgao, 2016-05-06 19:11:24]

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/fracas.py (right):

https://codereview.chromium.org/1950123003/diff/40001/appengine/findit/crash/...
appengine/findit/crash/fracas.py:96: 'solution': 'core_algorithm',
On 2016/05/05 23:03:45, sharu jiang wrote:
> On 2016/05/05 21:02:46, stgao wrote:
> > On 2016/05/05 20:26:45, sharu jiang wrote:
> > > On 2016/05/05 17:32:28, stgao wrote:
> > > > Is it always 'core_algorithm'? What if there is no regression range
> detected
> > > --
> > > > is git blame used instead?
> > > 
> > > Right now, we don't support the git blame yet because people has concerns
> > about
> > > the git blame ranking. Once we enable the git blame, I will modify this.
> > 
> > In that case, if we don't have regression range, should we still run the
> > analysis above in #79?
> 
> In FindItForCrash, we checkout if the regression_deps_rolls is empty or not,
> besides, I plan to add the git blame code inside FindItForCrash, so I still
want
> to run this function.

As long as analysis is not run and empty result is returned for crash without
regression range, we are good.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:12: _CHROMIUM_ROOT_DIR = 'src/'
nit: one more line above.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:80: deps_repo_info['deps_repo_url'] =
_BUILDSPEC_REPO
Mind a comment on why this updating is needed?

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:13: _CHROME_INTERNAL_HOST =
'https://chrome-internal.googlesource.com'
Is it better for this to be a list?

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:19: def
_GetExpandedHeaders(self, url, headers):
nit: this function is only for adding Oauth2.0 token, right?

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:20: #TODO(katesonia): Move this
to config.
nit: The host above needs to be moved to config, not this function.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:23: 'Authorization': 'Bearer '
+ auth_util.GetAuthToken()}
This might involve an api request, and is only useful for a subset of all http
requests. We should do it on demand.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/test/dependency_test.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/test/dependency_test.py:53:
self.assertNotEqual(dep.deps_repo_url, dep.repo_url)
How about asserting against the expected values?

[Sharu Jiang, 2016-05-06 20:27:28]

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/chromium_deps.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:12: _CHROMIUM_ROOT_DIR = 'src/'
On 2016/05/06 19:11:24, stgao wrote:
> nit: one more line above.

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/chromium_deps.py:80: deps_repo_info['deps_repo_url'] =
_BUILDSPEC_REPO
On 2016/05/06 19:11:24, stgao wrote:
> Mind a comment on why this updating is needed?

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:13: _CHROME_INTERNAL_HOST =
'https://chrome-internal.googlesource.com'
On 2016/05/06 19:11:24, stgao wrote:
> Is it better for this to be a list?

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:19: def
_GetExpandedHeaders(self, url, headers):
On 2016/05/06 19:11:24, stgao wrote:
> nit: this function is only for adding Oauth2.0 token, right?

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:20: #TODO(katesonia): Move this
to config.
On 2016/05/06 19:11:24, stgao wrote:
> nit: The host above needs to be moved to config, not this function.

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:23: 'Authorization': 'Bearer '
+ auth_util.GetAuthToken()}
On 2016/05/06 19:11:24, stgao wrote:
> This might involve an api request, and is only useful for a subset of all http
> requests. We should do it on demand.

Done.

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
File appengine/findit/common/test/dependency_test.py (right):

https://codereview.chromium.org/1950123003/diff/240001/appengine/findit/commo...
appengine/findit/common/test/dependency_test.py:53:
self.assertNotEqual(dep.deps_repo_url, dep.repo_url)
On 2016/05/06 19:11:24, stgao wrote:
> How about asserting against the expected values?

Done.

[stgao, 2016-05-06 20:35:44]

lgtm with a nit.

https://codereview.chromium.org/1950123003/diff/270016/appengine/findit/commo...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/1950123003/diff/270016/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:15: _INTERNAL_HOSTS =
['https://chrome-internal.googlesource.com']
To be safe, we may want to add a tailing "/", "....com/"

[Sharu Jiang, 2016-05-06 20:53:48]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-05-06 20:53:48]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/1950123003/#ps290001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-05-06 20:53:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950123003/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950123003/290001

[commit-bot: I haz the power, 2016-05-06 20:58:06]

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

[commit-bot: I haz the power, 2016-05-06 20:58:08]

Try jobs failed on following builders:
  Infra Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Presubmit/build...)

[Sharu Jiang, 2016-05-06 22:05:38]

Patchset #8 (id:310001) has been deleted

[Sharu Jiang, 2016-05-06 22:08:26]

Patchset #8 (id:320001) has been deleted

[Sharu Jiang, 2016-05-06 22:23:03]

Hi, 

I added the filter stacktrace based signature function in a new patch, I was
supposed to include this in another patch but we cannot pass the signature to
fracas.py but do nothing.

Please review the filtering part.

Thanks :)

https://codereview.chromium.org/1950123003/diff/270016/appengine/findit/commo...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/1950123003/diff/270016/appengine/findit/commo...
appengine/findit/common/http_client_appengine.py:15: _INTERNAL_HOSTS =
['https://chrome-internal.googlesource.com']
On 2016/05/06 20:35:44, stgao wrote:
> To be safe, we may want to add a tailing "/", "....com/"

Done.

[Martin Barbella, 2016-05-06 22:50:55]

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/callstack_filters.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:9: """Filter all the stack frames
before the signature frame."""
This class name could be improved to better reflect what it actually does,
though off the top of my head I can't think of a concise one. As-is, you really
need to read this to know.

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:14: # Filter out the types of
signature, for exampel [Out of Memery] signature.
Nit: example and Memory, also no need for the last "signature".

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/fracas_parser.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/fracas_parser.py:22: def Parse(self, stacktrace_string,
deps, signature):
Should signature be optional? In the tests, it seems counter-intuitive to use
empty signatures since what that behavior should do isn't immediately obvious.

[Martin Barbella, 2016-05-06 22:51:59]

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/fracas_parser.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/fracas_parser.py:22: def Parse(self, stacktrace_string,
deps, signature):
On 2016/05/06 22:50:55, Martin Barbella wrote:
> Should signature be optional? In the tests, it seems counter-intuitive to use
> empty signatures since what that behavior should do isn't immediately obvious.

s/empty signatures/empty string signatures/

[stgao, 2016-05-06 23:15:11]

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/callstack_filters.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:21: callstack[:] =
callstack[signature_frame_index:]
How about returning a new list instead of updating in-place?

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/fracas_parser.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/fracas_parser.py:22: def Parse(self, stacktrace_string,
deps, signature):
On 2016/05/06 22:50:55, Martin Barbella wrote:
> Should signature be optional? In the tests, it seems counter-intuitive to use
> empty signatures since what that behavior should do isn't immediately obvious.

+1

[Sharu Jiang, 2016-05-06 23:43:46]

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/callstack_filters.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:9: """Filter all the stack frames
before the signature frame."""
On 2016/05/06 22:50:54, Martin Barbella wrote:
> This class name could be improved to better reflect what it actually does,
> though off the top of my head I can't think of a concise one. As-is, you
really
> need to read this to know.

Change it to FilterFramesBeforeSignature.

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:14: # Filter out the types of
signature, for exampel [Out of Memery] signature.
On 2016/05/06 22:50:55, Martin Barbella wrote:
> Nit: example and Memory, also no need for the last "signature".

Done.

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:21: callstack[:] =
callstack[signature_frame_index:]
On 2016/05/06 23:15:11, stgao wrote:
> How about returning a new list instead of updating in-place?

Can I keep it in-place? Since I want to do:
FilterFramesBeforeSignature(stacktrace.GetCrashStack(), signature)

This way would be simpler.

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/fracas_parser.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/fracas_parser.py:22: def Parse(self, stacktrace_string,
deps, signature):
On 2016/05/06 23:15:11, stgao wrote:
> On 2016/05/06 22:50:55, Martin Barbella wrote:
> > Should signature be optional? In the tests, it seems counter-intuitive to
use
> > empty signatures since what that behavior should do isn't immediately
obvious.
> 
> +1

Done.

[stgao, 2016-05-07 00:02:53]

lgtm with nits.

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
File appengine/findit/crash/callstack_filters.py (right):

https://codereview.chromium.org/1950123003/diff/340001/appengine/findit/crash...
appengine/findit/crash/callstack_filters.py:21: callstack[:] =
callstack[signature_frame_index:]
On 2016/05/06 23:43:46, sharu jiang wrote:
> On 2016/05/06 23:15:11, stgao wrote:
> > How about returning a new list instead of updating in-place?
> 
> Can I keep it in-place? Since I want to do:
> FilterFramesBeforeSignature(stacktrace.GetCrashStack(), signature)
> 
> This way would be simpler.

If it can't be avoided, let's make the docstring clear that the input parameter
is updated.

[Sharu Jiang, 2016-05-07 00:12:51]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-05-07 00:12:51]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/1950123003/#ps380001
(title: "Update doc string.")

[commit-bot: I haz the power, 2016-05-07 00:13:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1950123003/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1950123003/380001

[commit-bot: I haz the power, 2016-05-07 00:16:42]

Description was changed from

==========
[Findit] Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356

patch from issue 1914653003 at patchset 80001
(http://crrev.com/1914653003#ps80001)
==========

to

==========
[Findit] Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356

patch from issue 1914653003 at patchset 80001
(http://crrev.com/1914653003#ps80001)

Committed:
https://chromium.googlesource.com/infra/infra/+/059e75397048f79d6bc4de3d399d7...
==========

[commit-bot: I haz the power, 2016-05-07 00:16:44]

Committed patchset #10 (id:380001) as
https://chromium.googlesource.com/infra/infra/+/059e75397048f79d6bc4de3d399d7...