Removing the mutation in the factories for getting dep repositories

Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG=677224
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2605943002
Committed: https://chromium.googlesource.com/infra/infra/+/49521996b621b7cdb4c070f15b171c38630f6f34

[wrengr, 2016-12-28 20:57:43]

Description was changed from

==========
Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG=677224
TBR=stgao@chromium.org
==========

to

==========
Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG=677224
TBR=stgao@chromium.org
==========

[wrengr, 2016-12-28 20:57:43]

wrengr@chromium.org changed reviewers:
+ inferno@chromium.org, katesonia@chromium.org, mbarbella@chromium.org

[wrengr, 2016-12-28 20:58:11]

This CL solves the other half of the issue about FindSuspects mutating the
Repository object. PTAL

[Sharu Jiang, 2016-12-28 23:05:13]

There is another place to clean. In run-predator.py under delta_test/, the
LocalGitRepository() should be replaced with factory.

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/crash/cras...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:90: def
_CachedGitilesRepositoryFactory(repo_url): # pragma: no cover
what's the reason for ``pragma: no cover`` here?

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/handlers/c...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/handlers/c...
appengine/findit/handlers/crash/crash_handler.py:132: def
get_repository(repo_url): # pragma: no cover
This one is duplicate to the one in crash_pipeline.py with different names. We'd
better avoid the duplication.

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

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extract_deps_info_pipeline.py:46: def
_CachedGitilesRepositoryFactory(repo_url): # pragma: no cover
Ditto, we have this in 3 different places, how about move this to a module in
gae_libs/?

[wrengr, 2016-12-29 20:57:31]

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/crash/cras...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/crash/cras...
appengine/findit/crash/crash_pipeline.py:90: def
_CachedGitilesRepositoryFactory(repo_url): # pragma: no cover
On 2016/12/28 23:05:12, Sharu Jiang wrote:
> what's the reason for ``pragma: no cover`` here?

Apparently it's never called by the unit tests ¯\_(ツ)_/¯

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/handlers/c...
File appengine/findit/handlers/crash/crash_handler.py (right):

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/handlers/c...
appengine/findit/handlers/crash/crash_handler.py:132: def
get_repository(repo_url): # pragma: no cover
On 2016/12/28 23:05:12, Sharu Jiang wrote:
> This one is duplicate to the one in crash_pipeline.py with different names.
We'd
> better avoid the duplication.

Done.

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

https://codereview.chromium.org/2605943002/diff/1/appengine/findit/waterfall/...
appengine/findit/waterfall/extract_deps_info_pipeline.py:46: def
_CachedGitilesRepositoryFactory(repo_url): # pragma: no cover
On 2016/12/28 23:05:12, Sharu Jiang wrote:
> Ditto, we have this in 3 different places, how about move this to a module in
> gae_libs/?

Done.

[Martin Barbella, 2016-12-29 21:52:33]

lgtm

[Sharu Jiang, 2016-12-30 00:00:46]

lgtm with comments.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:95:
CachedGitilesRepository.Factory(HttpClientAppengine()))
This indentation is incorrect:

https://google.github.io/styleguide/pyguide.html#Indentation

       # Aligned with opening delimiter
       foo = long_function_name(var_one, var_two,
                                var_three, var_four)

Or

       # 4-space hanging indent; nothing on first line
       foo = long_function_name(
           var_one, var_two, var_three,
           var_four)

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/loglinear/test/changelist_classifier_test.py
(right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/loglinear/test/changelist_classifier_test.py:120: def
GitilesRepositoryFactory(repo_url): # pragma: no cover
nit: MockGitilesRepositoryFactory?

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/test/findit_for_chromecrash_test.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/test/findit_for_chromecrash_test.py:72: def
_GitilesRepositoryFactory(repo_url): # pragma: no cover
We can move this to ``TestCase`` to share.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/gae_li...
File appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:28: def
Factory(cls, http_client): # pragma: no cover
This is general enough that we can move this to ``GitilesRepository``

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/libs/g...
File appengine/findit/libs/gitiles/gitiles_repository.py (left):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/libs/g...
appengine/findit/libs/gitiles/gitiles_repository.py:55: def identifier(self):
This identifier is used in ``_DefaultKeyGenerator`` in cache_decorator.py, we
should keep it.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/util_s...
File appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py
(right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/util_s...
appengine/findit/util_scripts/crash_queries/delta_test/run-predator.py:40:
findit = FinditForClientID(client_id, LocalGitRepository)
I think we should remove ``repo_url`` setter in ``LocalGitRepository``.

[wrengr, 2017-01-03 19:01:02]

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:95:
CachedGitilesRepository.Factory(HttpClientAppengine()))
On 2016/12/30 00:00:46, Sharu Jiang wrote:
> This indentation is incorrect:
> 
> https://google.github.io/styleguide/pyguide.html#Indentation
> 
>        # Aligned with opening delimiter
>        foo = long_function_name(var_one, var_two,
>                                 var_three, var_four)
> 
> Or
> 
>        # 4-space hanging indent; nothing on first line
>        foo = long_function_name(
>            var_one, var_two, var_three,
>            var_four)

Done.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/loglinear/test/changelist_classifier_test.py
(right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/loglinear/test/changelist_classifier_test.py:120: def
GitilesRepositoryFactory(repo_url): # pragma: no cover
On 2016/12/30 00:00:46, Sharu Jiang wrote:
> nit: MockGitilesRepositoryFactory?

Done.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
File appengine/findit/crash/test/findit_for_chromecrash_test.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/crash/...
appengine/findit/crash/test/findit_for_chromecrash_test.py:72: def
_GitilesRepositoryFactory(repo_url): # pragma: no cover
On 2016/12/30 00:00:46, Sharu Jiang wrote:
> We can move this to ``TestCase`` to share.

Done.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/gae_li...
File appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py (right):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/gae_li...
appengine/findit/gae_libs/gitiles/cached_gitiles_repository.py:28: def
Factory(cls, http_client): # pragma: no cover
On 2016/12/30 00:00:46, Sharu Jiang wrote:
> This is general enough that we can move this to ``GitilesRepository``

Done.

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/libs/g...
File appengine/findit/libs/gitiles/gitiles_repository.py (left):

https://codereview.chromium.org/2605943002/diff/20001/appengine/findit/libs/g...
appengine/findit/libs/gitiles/gitiles_repository.py:55: def identifier(self):
On 2016/12/30 00:00:46, Sharu Jiang wrote:
> This identifier is used in ``_DefaultKeyGenerator`` in cache_decorator.py, we
> should keep it.

Done.

[wrengr, 2017-01-03 20:07:14]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2017-01-03 20:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
katesonia@chromium.org, mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/2605943002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-03 20:07:27]

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, 2017-01-03 20:16:13]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483474034426310,
"parent_rev": "a07e0327ff09d5cc120fc9597db5fad399efedba", "commit_rev":
"49521996b621b7cdb4c070f15b171c38630f6f34"}

[commit-bot: I haz the power, 2017-01-03 20:16:17]

Description was changed from

==========
Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG=677224
TBR=stgao@chromium.org
==========

to

==========
Removing the mutation in the factories for getting dep repositories

This CL also removes passing the main repository as an argument, since
it was never actually used for the main repo it was only ever used as
an ad-hoc factory. This CL addresses the first issues described in
https://crrev.com/2608483002

BUG=677224
TBR=stgao@chromium.org

Review-Url: https://codereview.chromium.org/2605943002
Committed:
https://chromium.googlesource.com/infra/infra/+/49521996b621b7cdb4c070f15b171...
==========

[commit-bot: I haz the power, 2017-01-03 20:16:18]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/49521996b621b7cdb4c070f15b171...