[Findit] Factoring the gitiles (etc) stuff out into its own directory

[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:646189
Committed: https://chromium.googlesource.com/infra/infra/+/83d1a174d74106cb37b020cd17843b95f01937b9

[wrengr, 2016-09-14 23:30:27]

Description was changed from

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=
==========

to

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:644372
==========

[wrengr, 2016-09-14 23:30:27]

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

[wrengr, 2016-09-14 23:30:59]

Description was changed from

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:644372
==========

to

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:646189
==========

[wrengr, 2016-09-14 23:33:37]

Here's an initial stab at separating out all the gitiles stuff. There's still
some work to be done cleaning up, but all the unit tests and coverage pass. PTAL

[Martin Barbella, 2016-09-19 20:36:27]

I'm going to defer to Shuotao here, but at a glance this looks much cleaner to
me and I'm very much in favor of making a change like this.

[stgao, 2016-09-21 23:03:25]

Posted some comments.

BTW, I'd suggest not adding comments to the code for simple expected changes. We
could make a quick CL for them directly.

Regarding this CL, we could scope it to moving files around and not changing
functional code.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/cache_decorator.py (left):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/cache_decorator.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
As this file is not related to gitiles, I'd keep as is until we find a better
place for it (common/utils as in the new code layout I shared with you).

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

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:14: # responses of doing SSL stuff).
This sgtm in general. But it is possible to mock and test in even we import
GitRepository directly here.

What we need to change to the test is:

self.mock(git_repository, 'GitRepository', DummyGitRepository)
      |
self.mock(chromium_deps, 'GitRepository', DummyGitRepository)

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:14: # responses of doing SSL stuff).
BTW, can we remove this comment? For functional code, we keep comments for the
functionality instead of for testing.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:15: from gitiles import git_repository
nit: import order.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:43: # DEPSDownloader class (via RAII)?
Because a GitRepository is defined by the repo_url here.

I'm fine with moving it with RAII and creating a new DEPSDownloader each time
one is needed, although I don't see much benefit in that way.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/codereview.py (left):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/codereview.py:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
Please keep this interface. We are migrating to Gerrit Code Review. We need an
implementation for that later and swap out the Rietveld. This class is to
abstraction for both codereview.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/deps_parser.py (right):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/deps_parser.py:169: # root_dep which returns the
deps_content? What's the point in threading
We want root_dep class to be a plain object, and not to deal with http request
or the like.

What's the benefit of moving this over?

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/deps_parser.py:179: # not just call SetParent ourselves?
It's good to move that over here. Let's make the change directly in a CL instead
of adding a comment here.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/deps_parser.py:200: # on the floor. To what end?
You meant chromium_deps.py turns the result back to a dict?

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
Please keep this interface. See my explanation in the doc of new code layout.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/rietveld.py (right):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/rietveld.py:15: # of it?
This should go to waterfall/infra/ as in the new code layout I sent you.

I'm against passing a callable object. Currently only Post method happens to be
in use, because the two APIs we interact with is with post. There are other APIs
with get. Thus, I'd pass the http_client instance over.

Currently, this only works on findit-for-me appengine because of authentication
using app identity.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/gitile...
File appengine/findit/gitiles/__init__.py (left):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/gitile...
appengine/findit/gitiles/__init__.py:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
We don't want gitiles in the root directory, as the code here  serves more of a
gitiles client to pull changelogs like other infra clients to deal with
Buildbucket/etc. It is not at the same level of modulation or layer compared to
other stuff in the root directory.

As described in
https://docs.google.com/document/d/1JT1GanWYEDxVnClIBAdz1Ysb6KubdgDFj9pQFc4yt...,
we'd put it in common/vcs/gitiles with shared stuff in common/vcs.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/gitile...
File appengine/findit/gitiles/diff.py (right):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/gitile...
appengine/findit/gitiles/diff.py:1: # Copyright 2014 The Chromium Authors. All
rights reserved.
Mind renaming it to change_type.py?

[wrengr, 2016-09-28 00:59:49]

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/cache_decorator.py (left):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/cache_decorator.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2016/09/21 23:03:24, stgao (slow) wrote:
> As this file is not related to gitiles, I'd keep as is until we find a better
> place for it (common/utils as in the new code layout I shared with you).

I have to move it. Leaving it in ./common causes a "cyclic import" problem.
There's no actual cycle, but Python gets confused since it doesn't have a proper
module system. More concretely:

1. Every file that lives in $DIR depends on $DIR/__init__.py
2. For example, ./gitiles/git_repository.py depends on ./gitiles/__init__.py
3. Some files in ./common use git_repository.py, and thus transitively depend on
./gitiles/__init__.py
4. But git_repository.py uses cache_decorator.py, and thus transitively depends
on the __init__.py for where cache_decorator.py lives.
5. Thus, if cache_decorator.py lives in ./common, then Python crashes at
load-time due to the cyclic dependency between ./gitiles/__init__.py and
./common/__init__.py. To avoid this, cache_decorator.py must live in ./gitiles
or in some directory that's topologically "before" ./gitiles.

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

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:14: # responses of doing SSL stuff).
On 2016/09/21 23:03:24, stgao (slow) wrote:
> BTW, can we remove this comment? For functional code, we keep comments for the
> functionality instead of for testing.

I appreciate the desire to remove the comment, but it took a long time to track
down the bug and it's the sort of bug that's liable to show up again if not
noted. Personally I think the mock tests should be done differently so that it's
not so counterintuitive and hard to debug; they should be properly passing
objects around rather than monkey patching classes out from under the functional
code, imo.

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:43: # DEPSDownloader class (via RAII)?
On 2016/09/21 23:03:24, stgao (slow) wrote:
> Because a GitRepository is defined by the repo_url here.

We could just as well (a) pass repo_url to the DEPSDownloader constructor and
drop the argument to the Load method, or (b) pass the GitRepository object to
the Load method in lieu of passing the string. The choice depends on who
conceptually "owns" the GitRepository object: either the DEPSDownloader itself,
or someone else who wants to share the same object with other classes as well.

> I'm fine with moving it with RAII and creating a new DEPSDownloader each time
> one is needed, although I don't see much benefit in that way.

The benefit is code clarity. Right now the DEPSDownloader class isn't doing
anything at all; so having it there just makes things more complicated. I don't
know what sort of state HttpClientAppengine is carrying, but if the
GitRepository and its HttpClientAppengine can be reused by all the calls to Load
for that repo_url, then it makes sense to cache those objects when constructing
DEPSDownloader. If such reuse doesn't make sense (due to whatever
HttpClientAppengine is up to), then we should get rid of the DEPSDownloader
class and make Load a standalone function (or perhaps a method on GitRepository
or some subclass of GitRepository).

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
File appengine/findit/common/deps_parser.py (right):

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/deps_parser.py:169: # root_dep which returns the
deps_content? What's the point in threading
On 2016/09/21 23:03:25, stgao (slow) wrote:
> We want root_dep class to be a plain object, and not to deal with http request
> or the like.

Why do we want root_dep to not do http stuff? Even if we do, that doesn't change
the question.

> What's the benefit of moving this over?

It cleans up the code. Right now, UpdateDependencyTree needs to know all about
the guts of both root_dep and deps_loader, which makes it fragile against
changes to both of them. But UpdateDependencyTree doesn't actually care about
the deps_loader object, we could just as well pass in the deps_content instead.
Or if we really don't want to do that for some reason we could instead say
`deps_content = root_dep.LoadDEPS(deps_loader)`. Either of these changes would
help insulate UpdateDependencyTree against changes to the properties that
root_dep supports. If you're concerned about root_dep not doing any http stuff,
the method can simply delegate back to the deps_loader (aka, use the visitor
pattern):

    def LoadDEPS(self, deps_loader):
        return deps_loader.Load(
            self.deps_repo_url, self.deps_repo_revision, self.deps_file)

Or even better, get rid of the DEPS(Down)loader classes and inline the logic;
this way only the Dependency class really needs to know about its own innards:

    def DownloadDEPS(self, repo):
      content = None
      # Try .DEPS.git instead of DEPS first, for commits during the Git chaos.
      if self.deps_file == 'DEPS' and self.deps_repo_url ==
_CHROMIUM_REPO_MASTER:
        content = repo.GetSource('.DEPS.git', self.deps_repo_revision)

      if content is None:
        content = repo.GetSource(deps_file, self.deps_repo_revision)

      # Like gclient, fall back to raw 'DEPS' when all else fails.
      if content is None and self.deps_file != 'DEPS':
        content = repo.GetSource('DEPS', self.deps_repo_revision)

      if content is None:
        raise Exception(
          'Failed to pull %s file from %s, at revision %s.' % (
            self.deps_file, self.deps_repo_url, self.deps_repo_revision))

      return content

The exact change doesn't matter, what matters is that all these files are
tangled up with one another and consequently hard to maintain. Things like
having the useless DEPSLoader and DEPSDownloader classes, or like
UpdateDependencyTree needing to know all about the guts of root_dep, etc; these
are all just symptoms of the intertwining.

[wrengr, 2016-10-11 22:22:45]

ping

[stgao, 2016-10-13 05:47:32]

Among my comments, I'd like to high-light the one on the gitiles moving. It
won't be in the root directory as we discussed in the meeting.

For now, I would just make it in appengine/findit/common/gitiles.
What do you think?

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

https://codereview.chromium.org/2344443005/diff/20001/appengine/findit/common...
appengine/findit/common/chromium_deps.py:14: # responses of doing SSL stuff).
On 2016/09/28 00:59:49, wrengr wrote:
> On 2016/09/21 23:03:24, stgao (slow) wrote:
> > BTW, can we remove this comment? For functional code, we keep comments for
the
> > functionality instead of for testing.
> 
> I appreciate the desire to remove the comment, but it took a long time to
track
> down the bug and it's the sort of bug that's liable to show up again if not
> noted. Personally I think the mock tests should be done differently so that
it's
> not so counterintuitive and hard to debug; they should be properly passing
> objects around rather than monkey patching classes out from under the
functional
> code, imo.

Then let's keep it. Thanks for your time figuring it out!
I didn't know it cause a big pain on your end.

Passing object sounds a good idea. We will see how the test could be improved.
I'd like to clean up the test with better mocking or other solution too.
A short while ago, I just started to introduce unittest.mock instead of always
depending on the self.mock(module_name, "attribute_name_in_module",
mocked_attribute) provided by the testing framework of the infra repo.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/codereview.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/codereview.py:7: class CodeReview(object):  # pragma: no
cover
Please check my previous comment on why we want to keep this interface.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
I'd keep this interface. Sharu is working on an implementation of it for a local
git repo checkout. The existing git_repository.py is an implementation for the
Gitiles server. Both should conform to this interface.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/rietveld.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/rietveld.py:9: from common.codereview import CodeReview
Please check my previous comment on why we wanted to keep the interface
CodeReview.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/rietveld.py (right):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/rietveld.py:5: # TODO(wrengr): this file is only used by
waterfall. We should either
This should go to an infra/ directory in the new code layout.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/rietveld.py:7: # HttpClientAppengine by abstracting over
the HTTP_CLIENT member. The
Regarding the HTTP_CLIENT member, we could pass it over as a parameter in the
constructor.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
File appengine/findit/gitiles/__init__.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
appengine/findit/gitiles/__init__.py:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
As we discussed in the meeting regarding new code layout, gitiles won't be in
the root dir.
Would you mind make it in appengine/findit/common/gitiles/ for now?

[wrengr, 2016-10-14 18:33:15]

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
On 2016/10/13 05:47:32, stgao wrote:
> I'd keep this interface. Sharu is working on an implementation of it for a
local
> git repo checkout. The existing git_repository.py is an implementation for the
> Gitiles server. Both should conform to this interface.

In that case I'd call the base class GitRepository (since they're both forms of
git; and hopefully they'll share a bunch of code, rather than throwing
NotImplemented everywhere), and call the two subclasses something like
LocalGitRepository and GitilesRepository.

N.B., because Python does duck typing, it's not technically necessary to have a
class hierarchy in many places; just having the right set of methods is
sufficient. This is part of why I'm advocating for using __call__ etc in various
places (since callable is a standard built-in interface, and so helps make it
easier to create one-off classes (e.g., using lambdas for mocking whatever
callable object)). Of course, the class hierarchy does help to document intent,
which is always good. It's just that, ime, doing Java-style
interfaces-as-specification where nothing is implemented tends not to work very
well for Python; for Python, class hierarchies work best when there's some
shared code.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
File appengine/findit/gitiles/__init__.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
appengine/findit/gitiles/__init__.py:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/10/13 05:47:32, stgao wrote:
> As we discussed in the meeting regarding new code layout, gitiles won't be in
> the root dir.
> Would you mind make it in appengine/findit/common/gitiles/ for now?

I thought it was going to be called ./lib/gitiles? If that's where it'll live
eventually I don't see any reason to put it in ./common/gitiles for now

[stgao, 2016-10-17 17:31:43]

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
On 2016/10/14 18:33:14, wrengr wrote:
> On 2016/10/13 05:47:32, stgao wrote:
> > I'd keep this interface. Sharu is working on an implementation of it for a
> local
> > git repo checkout. The existing git_repository.py is an implementation for
the
> > Gitiles server. Both should conform to this interface.
> 
> In that case I'd call the base class GitRepository (since they're both forms
of
> git; and hopefully they'll share a bunch of code, rather than throwing
> NotImplemented everywhere), and call the two subclasses something like
> LocalGitRepository and GitilesRepository.

I love the new names GitRepository, GitilesRepository, and LocalGitRepository.
Let's go with them.
But I don't anticipate too much code sharing expect parsing CL description for
metadata like commit position, codereview url, etc. That could (and should) be
factored out to be shared, because that part is actually not the responsibility
of a git repo.

> 
> N.B., because Python does duck typing, it's not technically necessary to have
a
> class hierarchy in many places; just having the right set of methods is
> sufficient. This is part of why I'm advocating for using __call__ etc in
various
> places (since callable is a standard built-in interface, and so helps make it
> easier to create one-off classes (e.g., using lambdas for mocking whatever
> callable object)). Of course, the class hierarchy does help to document
intent,
> which is always good. It's just that, ime, doing Java-style
> interfaces-as-specification where nothing is implemented tends not to work
very
> well for Python; for Python, class hierarchies work best when there's some
> shared code.

I agree that a base class like this can't enforce a sublcass to always implement
all the expected methods.

However, this helps a lot to document the APIs, and leave the implementation
details to subclasses:
1. Users of those subclasses don't have to read code of the implementation or
search for public methods.
2. Contributors could refer to this base class for a new  implementation. They
don't have to deal with an existing implementation for which methods are needed
and which are not, and we could avoid missing implementation of some functions.
(e.g, this could help Sharu work on the implementation for a local git
checkout.)

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
File appengine/findit/gitiles/__init__.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/gitil...
appengine/findit/gitiles/__init__.py:1: # Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/10/14 18:33:14, wrengr wrote:
> On 2016/10/13 05:47:32, stgao wrote:
> > As we discussed in the meeting regarding new code layout, gitiles won't be
in
> > the root dir.
> > Would you mind make it in appengine/findit/common/gitiles/ for now?
> 
> I thought it was going to be called ./lib/gitiles? If that's where it'll live
> eventually I don't see any reason to put it in ./common/gitiles for now

sg. But it should be ./libs instead ./lib as discussed :)

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/commo...
File appengine/findit/common/codereview.py (left):

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/commo...
appengine/findit/common/codereview.py:7: class CodeReview(object):  # pragma: no
cover
In the same spirit as I explained in the original
appengine/findit/common/repository.py, let's also keep this base class.

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/handl...
File appengine/findit/handlers/flake/check_flake.py (left):

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/handl...
appengine/findit/handlers/flake/check_flake.py:15: 
style nit: keep this line.
We need two lines to separate top-level definition.

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/handl...
File appengine/findit/handlers/help_triage.py (right):

https://codereview.chromium.org/2344443005/diff/180001/appengine/findit/handl...
appengine/findit/handlers/help_triage.py:5: from lib.gitiles.git_repository
import GitRepository
style nit:order of imports. You may want to check other files too.

[stgao, 2016-10-17 17:48:15]

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
On 2016/10/14 18:33:14, wrengr wrote:
> On 2016/10/13 05:47:32, stgao wrote:
> > I'd keep this interface. Sharu is working on an implementation of it for a
> local
> > git repo checkout. The existing git_repository.py is an implementation for
the
> > Gitiles server. Both should conform to this interface.
> 
> In that case I'd call the base class GitRepository (since they're both forms
of
> git; and hopefully they'll share a bunch of code, rather than throwing
> NotImplemented everywhere), and call the two subclasses something like
> LocalGitRepository and GitilesRepository.
> 
> N.B., because Python does duck typing, it's not technically necessary to have
a
> class hierarchy in many places; just having the right set of methods is
> sufficient. This is part of why I'm advocating for using __call__ etc in
various
> places (since callable is a standard built-in interface, and so helps make it
> easier to create one-off classes (e.g., using lambdas for mocking whatever
> callable object)). Of course, the class hierarchy does help to document
intent,
> which is always good. It's just that, ime, doing Java-style
> interfaces-as-specification where nothing is implemented tends not to work
very
> well for Python; for Python, class hierarchies work best when there's some
> shared code.

Regarding __call__, I like this good idea if the class itself has only one
functionality and it is self-explained by the class name. For a class like this
to deal with Git repo, it seems not feasible or hard for readability.

OTOH, I rarely saw usage of __call__ in the infra appengine codebase, although
I'm not against its appropriate usage :)

/path/to/infra/appengine$ find . -name "*.py" | grep -v "third_party" | xargs
grep "__call__"
./chromium_rietveld/upload.py:  def __call__(self):
./chromium_status/tools/update.py:  def __call__(self, parser, namespace,
values, option_string=None):
./findit/crash/findit_for_chromecrash.py:  # be better IMO to rename it to
__call__ to reduce verbosity.
./findit/crash/scorers/scorer.py:  def __call__(self, result):
./findit/crash/scorers/aggregators.py:  def __call__(self, data_list):
./monorail/framework/framework_helpers.py:  def __call__(self):
./monorail/tools/spam/spam.py:    def __call__(self, fn):

[wrengr, 2016-10-27 17:28:00]

Reintroduced a base class for Git repositories. Renamed the class hierarchy so
GitRepository is the base, with GitilesRepository (and eventually
LocalGitRepository) as subclasses.

I also filed http://crbug.com/660080 re mocking; but that should be addressed in
a separate CL.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/repository.py (left):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/repository.py:7: class Repository(object):  # pragma: no
cover
On 2016/10/17 17:31:43, stgao wrote:
> I love the new names GitRepository, GitilesRepository, and LocalGitRepository.
> Let's go with them.

Done.

> But I don't anticipate too much code sharing expect parsing CL description for
> metadata like commit position, codereview url, etc. That could (and should) be
> factored out to be shared, because that part is actually not the
responsibility
> of a git repo.

Ack.

> I agree that a base class like this can't enforce a sublcass to always
implement
> all the expected methods.
> 
> However, this helps a lot to document the APIs, and leave the implementation
> details to subclasses:
> 1. Users of those subclasses don't have to read code of the implementation or
> search for public methods.
> 2. Contributors could refer to this base class for a new  implementation. They
> don't have to deal with an existing implementation for which methods are
needed
> and which are not, and we could avoid missing implementation of some
functions.
> (e.g, this could help Sharu work on the implementation for a local git
> checkout.)

Ack.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
File appengine/findit/common/rietveld.py (right):

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/rietveld.py:5: # TODO(wrengr): this file is only used by
waterfall. We should either
On 2016/10/13 05:47:32, stgao wrote:
> This should go to an infra/ directory in the new code layout.

Is there a bug ticket for doing the new layout? If so, this todo should be added
there. I'll adjust the version in the file as appropriate.

https://codereview.chromium.org/2344443005/diff/160001/appengine/findit/commo...
appengine/findit/common/rietveld.py:7: # HttpClientAppengine by abstracting over
the HTTP_CLIENT member. The
On 2016/10/13 05:47:32, stgao wrote:
> Regarding the HTTP_CLIENT member, we could pass it over as a parameter in the
> constructor.

Acknowledged.

[wrengr, 2016-10-27 17:28:26]

Let me know if there are any outstanding comments I missed.

[Martin Barbella, 2016-10-27 23:27:29]

I do have a general comment, but I'll defer to stgao@ here since I think he has
stronger feelings on this structure than I do in general, and most of the rest
seems straightforward.

I don't really like adding high level TODOs for future cleanup to code. I really
just don't like TODOs in general, but I acknowledge that they do have legitimate
uses. While I'm confident that the work mentioned in the TODOs added here will
be done, they can be difficult to remember to remove or for someone else to
understand all of the context of. Bugs should be preferred.

[wrengr, 2016-10-28 18:13:28]

On 2016/10/27 23:27:29, Martin Barbella wrote:
> I do have a general comment, but I'll defer to stgao@ here since I think he
has
> stronger feelings on this structure than I do in general, and most of the rest
> seems straightforward.
> 
> I don't really like adding high level TODOs for future cleanup to code. I
really
> just don't like TODOs in general, but I acknowledge that they do have
legitimate
> uses. While I'm confident that the work mentioned in the TODOs added here will
> be done, they can be difficult to remember to remove or for someone else to
> understand all of the context of. Bugs should be preferred.

This is a rather old CL, from before that preference was pointed out. I've
updated newer CLs to prefer bug reports, just didn't scan through this one to
make the changeover yet.

[stgao, 2016-10-28 18:21:00]

I did another detailed pass. All looks good.

Most comments are just style nits like the order of import.
One comment though is to keep codereview.py just like we do for
git_repository.py

Regarding TODOs, we usually file bugs first, and then add the TODO in this way:

TODO(http://crbug.com/BUG_ID): a short summary.

In this way, we have bugs to track what need to be done, but also in code we
know what's missing.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/commo...
File appengine/findit/common/codereview.py (left):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/commo...
appengine/findit/common/codereview.py:7: class CodeReview(object):  # pragma: no
cover
Mind reverting this? We wanted to keep it in the same spirit of
git_repository.py or repository.py

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:10: from lib.gitiles.change_log
import ChangeLog
order of import.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
File appengine/findit/crash/test/results_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
appengine/findit/crash/test/results_test.py:7: from lib.gitiles.change_log
import ChangeLog
order of import.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
File appengine/findit/handlers/flake/check_flake.py (left):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
appengine/findit/handlers/flake/check_flake.py:29: 
This line should be kept, as we use two empty lines to separate top-level
definition.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
File appengine/findit/handlers/help_triage.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
appengine/findit/handlers/help_triage.py:5: from lib.gitiles.gitiles_repository
import GitilesRepository
order of import.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/cache_decorator.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/cache_decorator.py:9: # library, with the cached
version as a subclass in some appengine_utils
This is a good idea.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/cache_decorator.py:12: # file isn't a handler.
Hm, I don't quite understand this though. Mind elaborating a little bit?

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/gitiles_repository.py:14: from
lib.gitiles.cache_decorator import Cached, CompressedMemCacher
cache_decorator should be in lib/ instead of lib/gitiles/

Other code is using it besides gitiles.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/test/gitiles_repository_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/gitiles_repository_test.py:13: from
lib.gitiles.change_log import ChangeLog
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:10: from
lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/flake/step_mapper.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/flake/step_mapper.py:10: from lib.gitiles import
cache_decorator
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/pull_changelog_pipeline.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/pull_changelog_pipeline.py:5: from
lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/build_failure_analysis_test.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:7:
from lib.gitiles.gitiles_repository import GitilesRepository
order of import

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py:7:
from lib.gitiles.gitiles_repository import GitilesRepository
order of import

[wrengr, 2016-10-28 19:24:49]

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
File appengine/findit/crash/test/crash_testcase.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
appengine/findit/crash/test/crash_testcase.py:10: from lib.gitiles.change_log
import ChangeLog
On 2016/10/28 18:20:59, stgao wrote:
> order of import.

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
File appengine/findit/crash/test/results_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/crash...
appengine/findit/crash/test/results_test.py:7: from lib.gitiles.change_log
import ChangeLog
On 2016/10/28 18:20:59, stgao wrote:
> order of import.

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
File appengine/findit/handlers/flake/check_flake.py (left):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
appengine/findit/handlers/flake/check_flake.py:29: 
On 2016/10/28 18:20:59, stgao wrote:
> This line should be kept, as we use two empty lines to separate top-level
> definition.

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
File appengine/findit/handlers/help_triage.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/handl...
appengine/findit/handlers/help_triage.py:5: from lib.gitiles.gitiles_repository
import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import.

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/cache_decorator.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/cache_decorator.py:9: # library, with the cached
version as a subclass in some appengine_utils
On 2016/10/28 18:21:00, stgao wrote:
> This is a good idea.

Filed a bug: http://crbug.com/660466

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/cache_decorator.py:12: # file isn't a handler.
On 2016/10/28 18:21:00, stgao wrote:
> Hm, I don't quite understand this though. Mind elaborating a little bit?

This was addressing some previous discussions about the reorg; which I think
shouldn't be in question anymore. At that point, rather than putting all the
stuff into /services (as in the current proposal for code reorg) you'd wanted to
put everything into /handlers. But cache decorators don't handle requests from
the web app frontend, so that doesn't make sense as a place for this file to
live.

I've removed the parenthetical, as it's no longer relevant.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/gitiles_repository.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/gitiles_repository.py:14: from
lib.gitiles.cache_decorator import Cached, CompressedMemCacher
On 2016/10/28 18:21:00, stgao wrote:
> cache_decorator should be in lib/ instead of lib/gitiles/
> 
> Other code is using it besides gitiles.

Since when? Last I looked only gitiles_repository.py used it

(In any case, is done.)

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
File appengine/findit/lib/gitiles/test/gitiles_repository_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/lib/g...
appengine/findit/lib/gitiles/test/gitiles_repository_test.py:13: from
lib.gitiles.change_log import ChangeLog
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/build_failure_analysis.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/build_failure_analysis.py:10: from
lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/flake/step_mapper.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/flake/step_mapper.py:10: from lib.gitiles import
cache_decorator
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/identify_try_job_culprit_pipeline.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/identify_try_job_culprit_pipeline.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/pull_changelog_pipeline.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/pull_changelog_pipeline.py:5: from
lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/send_notification_for_culprit_pipeline.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/test/build_failure_analysis_test.py (right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/build_failure_analysis_test.py:11: from
lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/identify_try_job_culprit_pipeline_test.py:7:
from lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
File
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
(right):

https://codereview.chromium.org/2344443005/diff/240001/appengine/findit/water...
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py:7:
from lib.gitiles.gitiles_repository import GitilesRepository
On 2016/10/28 18:21:00, stgao wrote:
> order of import

Done.

[stgao, 2016-10-28 19:55:10]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-10-28 19:55:11]

lgtm

lgtm, many thanks!

[commit-bot: I haz the power, 2016-10-28 19:55:17]

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-10-28 20:14:33]

Description was changed from

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:646189
==========

to

==========
[Findit] Factoring the gitiles (etc) stuff out into its own directory

BUG=chromium:646189

Committed:
https://chromium.googlesource.com/infra/infra/+/83d1a174d74106cb37b020cd17843...
==========

[commit-bot: I haz the power, 2016-10-28 20:14:35]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/infra/infra/+/83d1a174d74106cb37b020cd17843...