[Findit] Re-org code.

[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder" = Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* add some example of file moving

And the final step is to do a one-off moving after the split is done: appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as https://codereview.chromium.org/2447253002/.

Committed: https://chromium.googlesource.com/infra/infra/+/76710217389bfb35e38b83a142bb443064e5a859

[stgao, 2016-11-10 18:43:10]

Description was changed from

==========
[Findit] Refatoring code.

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

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step: split Predator code into a sub directory
appengine/findit/predator.
And the next step is to do a one-off move after the split is done.
==========

[stgao, 2016-11-10 18:45:51]

Description was changed from

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step: split Predator code into a sub directory
appengine/findit/predator.
And the next step is to do a one-off move after the split is done.
==========

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving
And the next step is to do a one-off moving after the split is done.
==========

[stgao, 2016-11-10 18:46:51]

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

[stgao, 2016-11-10 18:46:52]

ptal

[stgao, 2016-11-10 18:51:49]

Description was changed from

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving
And the next step is to do a one-off moving after the split is done.
==========

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving

And the final step is to do a one-off moving after the split is done.
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

[wrengr, 2016-11-10 21:02:17]

On 2016/11/10 18:46:52, stgao (slow on Monday) wrote:
> ptal

Could you paste the output of `git log --stat` to make it a bit clearer which
files are moved (vs actually added/deleted/modified), since Rietveldt loses that
info?

[wrengr, 2016-11-10 21:03:52]

On 2016/11/10 21:02:17, wrengr wrote:
> On 2016/11/10 18:46:52, stgao (slow on Monday) wrote:
> > ptal
> 
> Could you paste the output of `git log --stat` to make it a bit clearer which
> files are moved (vs actually added/deleted/modified), since Rietveldt loses
that
> info?

And/or the output of `find . -name '*.py'`, just so we can see the directory
structure itself.

[wrengr, 2016-11-10 21:20:35]

Very +1 to having all the README.md files. Here's a few comments on those files.
Still looking through the rest of the changes.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/infra_clie...
File appengine/findit/infra_clients/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/infra_clie...
appengine/findit/infra_clients/README.md:4: with APIs of various infra services
like bulidbucket, Swarming/Isolate, Reitveld
-> "buildbucket" (I assume?)

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/libs/READM...
File appengine/findit/libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/libs/READM...
appengine/findit/libs/README.md:5: AppEngine-compatible -- could run on
AppEngine.
Would be worth specifically mentioning the `os` modules and/or writing to the
file system as specific examples of things which are excluded

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
File appengine/findit/predator/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
appengine/findit/predator/README.md:3: * [analysis](analysis): Core of analysis
for Chrome crashes.
-> "Core library for analyzing Chrome crashes."

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
appengine/findit/predator/README.md:4: * [app](app): The App Engine modules to
provide core services of Predator.
-> "The AppEngine application for invoking the library to analyze Chrome
crashes."

Or something to that effect. The code under ./app isn't just "modules" but the
whole AE application/client. Also, I think using "services" here is misleading,
since that word has a technical meaning within AE. Plus, there isn't really a
"core" set of analyses distinct from peripheral analyses.

[stgao, 2016-11-10 22:07:03]

Description was changed from

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving

And the final step is to do a one-off moving after the split is done.
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving

And the final step is to do a one-off moving after the split is done:
appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

[stgao, 2016-11-10 22:13:41]

On 2016/11/10 21:02:17, wrengr wrote:
> On 2016/11/10 18:46:52, stgao (slow on Monday) wrote:
> > ptal
> 
> Could you paste the output of `git log --stat` to make it a bit clearer which
> files are moved (vs actually added/deleted/modified), since Rietveldt loses
that
> info?

appengine/findit/common/http_client_appengine.py                               |
 2 +-
 appengine/findit/common/test/chrome_dependency_fetcher_test.py                
|  2 +-
 appengine/findit/gae_libs/README.md                                           
| 17 +++++++++++++++++
 appengine/findit/gae_libs/__init__.py                                         
|  1 +
 appengine/findit/gae_libs/model/__init__.py                                   
|  1 +
 appengine/findit/gae_libs/model/test/__init__.py                              
|  1 +
 appengine/findit/{ => gae_libs}/model/test/versioned_config_test.py           
|  2 +-
 appengine/findit/{ => gae_libs}/model/test/versioned_model_test.py            
|  2 +-
 appengine/findit/{ => gae_libs}/model/versioned_config.py                     
|  2 +-
 appengine/findit/{ => gae_libs}/model/versioned_model.py                      
|  0
 appengine/findit/infra_clients/README.md                                      
| 15 +++++++++++++++
 appengine/findit/infra_clients/__init__.py                                    
|  1 +
 appengine/findit/infra_clients/codereview/__init__.py                         
|  1 +
 appengine/findit/{common => infra_clients/codereview}/codereview.py           
|  0
 appengine/findit/{common => infra_clients/codereview}/rietveld.py             
|  4 ++--
 appengine/findit/infra_clients/codereview/test/__init__.py                    
|  1 +
 appengine/findit/{common => infra_clients/codereview}/test/rietveld_test.py   
|  4 ++--
 appengine/findit/lib/gitiles/test/gitiles_repository_test.py                  
|  4 +---
 appengine/findit/libs/README.md                                               
| 12 ++++++++++++
 appengine/findit/libs/__init__.py                                             
|  1 +
 appengine/findit/libs/http/__init__.py                                        
|  1 +
 appengine/findit/{common => libs/http}/retry_http_client.py                   
|  0
 appengine/findit/libs/http/test/__init__.py                                   
|  1 +
 appengine/findit/{common => libs/http}/test/retry_http_client_test.py         
|  2 +-
 appengine/findit/model/crash/crash_config.py                                  
|  2 +-
 appengine/findit/model/flake/flake_analysis_request.py                        
|  2 +-
 appengine/findit/model/flake/master_flake_analysis.py                         
|  2 +-
 appengine/findit/model/suspected_cl_confidence.py                             
|  2 +-
 appengine/findit/model/wf_config.py                                           
|  2 +-
 appengine/findit/predator/README.md                                           
| 16 ++++++++++++++++
 appengine/findit/predator/__init__.py                                         
|  1 +
 appengine/findit/predator/analysis/__init__.py                                
|  1 +
 appengine/findit/predator/app/__init__.py                                     
|  1 +
 appengine/findit/predator/app/backend/__init__.py                             
|  1 +
 appengine/findit/predator/app/common/__init__.py                              
|  1 +
 appengine/findit/predator/app/frontend/__init__.py                            
|  1 +
 appengine/findit/waterfall/send_notification_for_culprit_pipeline.py          
|  2 +-
 appengine/findit/waterfall/test/buildbot_test.py                              
|  2 +-
 appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
|  2 +-
 appengine/findit/waterfall/test/swarming_util_test.py                         
|  4 ++--
 40 files changed, 96 insertions(+), 23 deletions(-)

[stgao, 2016-11-10 22:17:24]

On 2016/11/10 21:03:52, wrengr wrote:
> On 2016/11/10 21:02:17, wrengr wrote:
> > On 2016/11/10 18:46:52, stgao (slow on Monday) wrote:
> > > ptal
> > 
> > Could you paste the output of `git log --stat` to make it a bit clearer
which
> > files are moved (vs actually added/deleted/modified), since Rietveldt loses
> that
> > info?
> 
> And/or the output of `find . -name '*.py'`, just so we can see the directory
> structure itself.

This won't be that helpful as it is too verbose.
If you'd like to look at the directory structure, you may check
https://codereview.chromium.org/2447253002/ instead.

[chanli, 2016-11-10 23:04:04]

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
appengine/findit/common/http_client_appengine.py:23: class
HttpClientAppengine(RetryHttpClient):  # pragma: no cover
Why does this http_client_appengine stay with Findit? Predator doesn't need it?

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/model/cras...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/model/cras...
appengine/findit/model/crash/crash_config.py:15: class
CrashConfig(VersionedConfig):
This module should be moved to predator, right?

[wrengr, 2016-11-10 23:49:04]

On 2016/11/10 22:13:41, stgao (slow on Monday) wrote:
> On 2016/11/10 21:02:17, wrengr wrote:
> > On 2016/11/10 18:46:52, stgao (slow on Monday) wrote:
> > > ptal
> > 
> > Could you paste the output of `git log --stat` to make it a bit clearer
which
> > files are moved (vs actually added/deleted/modified), since Rietveldt loses
> that
> > info?
> 
> appengine/findit/common/http_client_appengine.py                              
|
>  2 +-
>  appengine/findit/common/test/chrome_dependency_fetcher_test.py               

> |  2 +-
>  appengine/findit/gae_libs/README.md                                          

> | 17 +++++++++++++++++
>  appengine/findit/gae_libs/__init__.py                                        

> |  1 +
>  appengine/findit/gae_libs/model/__init__.py                                  

> |  1 +
>  appengine/findit/gae_libs/model/test/__init__.py                             

> |  1 +
>  appengine/findit/{ => gae_libs}/model/test/versioned_config_test.py          

> |  2 +-
>  appengine/findit/{ => gae_libs}/model/test/versioned_model_test.py           

> |  2 +-
>  appengine/findit/{ => gae_libs}/model/versioned_config.py                    

> |  2 +-
>  appengine/findit/{ => gae_libs}/model/versioned_model.py                     

> |  0
>  appengine/findit/infra_clients/README.md                                     

> | 15 +++++++++++++++
>  appengine/findit/infra_clients/__init__.py                                   

> |  1 +
>  appengine/findit/infra_clients/codereview/__init__.py                        

> |  1 +
>  appengine/findit/{common => infra_clients/codereview}/codereview.py          

> |  0
>  appengine/findit/{common => infra_clients/codereview}/rietveld.py            

> |  4 ++--
>  appengine/findit/infra_clients/codereview/test/__init__.py                   

> |  1 +
>  appengine/findit/{common => infra_clients/codereview}/test/rietveld_test.py  

> |  4 ++--
>  appengine/findit/lib/gitiles/test/gitiles_repository_test.py                 

> |  4 +---
>  appengine/findit/libs/README.md                                              

> | 12 ++++++++++++
>  appengine/findit/libs/__init__.py                                            

> |  1 +
>  appengine/findit/libs/http/__init__.py                                       

> |  1 +
>  appengine/findit/{common => libs/http}/retry_http_client.py                  

> |  0
>  appengine/findit/libs/http/test/__init__.py                                  

> |  1 +
>  appengine/findit/{common => libs/http}/test/retry_http_client_test.py        

> |  2 +-
>  appengine/findit/model/crash/crash_config.py                                 

> |  2 +-
>  appengine/findit/model/flake/flake_analysis_request.py                       

> |  2 +-
>  appengine/findit/model/flake/master_flake_analysis.py                        

> |  2 +-
>  appengine/findit/model/suspected_cl_confidence.py                            

> |  2 +-
>  appengine/findit/model/wf_config.py                                          

> |  2 +-
>  appengine/findit/predator/README.md                                          

> | 16 ++++++++++++++++
>  appengine/findit/predator/__init__.py                                        

> |  1 +
>  appengine/findit/predator/analysis/__init__.py                               

> |  1 +
>  appengine/findit/predator/app/__init__.py                                    

> |  1 +
>  appengine/findit/predator/app/backend/__init__.py                            

> |  1 +
>  appengine/findit/predator/app/common/__init__.py                             

> |  1 +
>  appengine/findit/predator/app/frontend/__init__.py                           

> |  1 +
>  appengine/findit/waterfall/send_notification_for_culprit_pipeline.py         

> |  2 +-
>  appengine/findit/waterfall/test/buildbot_test.py                             

> |  2 +-
> 
appengine/findit/waterfall/test/send_notification_for_culprit_pipeline_test.py
> |  2 +-
>  appengine/findit/waterfall/test/swarming_util_test.py                        

> |  4 ++--
>  40 files changed, 96 insertions(+), 23 deletions(-)

Thanks :)

[wrengr, 2016-11-11 00:06:45]

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
appengine/findit/common/http_client_appengine.py:23: class
HttpClientAppengine(RetryHttpClient):  # pragma: no cover
On 2016/11/10 23:04:04, chanli wrote:
> Why does this http_client_appengine stay with Findit? Predator doesn't need
it? 
> 

If you're referring to the "findit" string that occurs in the path
"appengine/findit/blah", that's a misnomer. That occurrence of the string should
(a) really be "cl_culprit_finder" as per the renaming, and (b) will be removed
in a subsequent CL when Predator and Findit(waterfall) are moved to be
top-level.

If you're referring to the stuff in "appengine/findit/libs", part of the problem
is how the proposed directory changes affect shared libraries. That is: certain
libraries (e.g., lib.http.*) are shared by both Findit(waterfall) and Predator.
For both projects to have access to the code, we use symlinks; however, the code
does have to physically reside somewhere. In stgao's current proposal that
physical location is inside Findit(waterfall); i.e., Predator's "lib" will be a
symlink to Findit's "lib", whereas Findit's "lib" will be actual files. In the
most recent meeting, me and mbarbella brought this up as being suboptimal since
it suggests that the lib code is "really part of Findit" rather than making it
clear that it is shared/independent code. Though I think this is safe to address
in a separate CL.

[lijeffrey, 2016-11-14 07:16:04]

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/gae_libs/R...
File appengine/findit/gae_libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/gae_libs/R...
appengine/findit/gae_libs/README.md:1: # gae_libs
Nit: I'm guessing gae stands for Google Appengine, is it worth mentioning
somewhere?

[stgao, 2016-11-14 23:29:14]

Patchset #2 (id:20001) has been deleted

[stgao, 2016-11-14 23:30:51]

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
File appengine/findit/common/http_client_appengine.py (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/common/htt...
appengine/findit/common/http_client_appengine.py:23: class
HttpClientAppengine(RetryHttpClient):  # pragma: no cover
On 2016/11/10 23:04:04, chanli wrote:
> Why does this http_client_appengine stay with Findit? Predator doesn't need
it? 
> 

We will work on the file moving incrementally, not all in this CL.
This CL is to set up the new code structure and add some example of filing
moving to illustrate the purpose of the new directories.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/gae_libs/R...
File appengine/findit/gae_libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/gae_libs/R...
appengine/findit/gae_libs/README.md:1: # gae_libs
On 2016/11/14 07:16:04, lijeffrey wrote:
> Nit: I'm guessing gae stands for Google Appengine, is it worth mentioning
> somewhere?

Done.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/infra_clie...
File appengine/findit/infra_clients/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/infra_clie...
appengine/findit/infra_clients/README.md:4: with APIs of various infra services
like bulidbucket, Swarming/Isolate, Reitveld
On 2016/11/10 21:20:34, wrengr wrote:
> -> "buildbucket" (I assume?)

Good catch!
Done.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/libs/READM...
File appengine/findit/libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/libs/READM...
appengine/findit/libs/README.md:5: AppEngine-compatible -- could run on
AppEngine.
On 2016/11/10 21:20:35, wrengr wrote:
> Would be worth specifically mentioning the `os` modules and/or writing to the
> file system as specific examples of things which are excluded

Good idea!
Added an example.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/model/cras...
File appengine/findit/model/crash/crash_config.py (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/model/cras...
appengine/findit/model/crash/crash_config.py:15: class
CrashConfig(VersionedConfig):
On 2016/11/10 23:04:04, chanli wrote:
> This module should be moved to predator, right?

We will move in separate CLs later.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
File appengine/findit/predator/README.md (right):

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
appengine/findit/predator/README.md:3: * [analysis](analysis): Core of analysis
for Chrome crashes.
On 2016/11/10 21:20:35, wrengr wrote:
> -> "Core library for analyzing Chrome crashes."

Done.

https://codereview.chromium.org/2488113005/diff/1/appengine/findit/predator/R...
appengine/findit/predator/README.md:4: * [app](app): The App Engine modules to
provide core services of Predator.
On 2016/11/10 21:20:35, wrengr wrote:
> -> "The AppEngine application for invoking the library to analyze Chrome
> crashes."
> 
> Or something to that effect. The code under ./app isn't just "modules" but the
> whole AE application/client. Also, I think using "services" here is
misleading,
> since that word has a technical meaning within AE. Plus, there isn't really a
> "core" set of analyses distinct from peripheral analyses.

Done. Removed "modules" and "services" as they might be confusing especially
both App Engine and Python have the concept "modules" while it has completely
different meanings for them.

[wrengr, 2016-11-15 00:38:01]

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/README.md:4: APIs (gae is short for Google App
Engine), eg: an NDB model that supports
The proper spelling is "e.g.,"

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/README.md:14: containing directory, and ensure that
the containing directory is in sys.path or
"containing directory" is non-idomatic. Should be something like "the root
directory for the project" or "the project's root directory"

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/__init__.py:1: 
Should have the standard copyright preamble

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/model/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/model/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/model/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/model/test/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/README.md:3: This [infra\_clients](infra_clients)
module is for client code that interacts
I don't think "client code" is clear. In programming more generally "client"
could be any code using those various APIs (so calling the code here "client"
code isn't terribly helpful, since we're not looking at things from the
perspective of those libraries). But "client" is also used as a technical name
for AppEngine applications (so it's not clear whether that's what is meant here,
or not).

Also "infra_clients" isn't a good name. In addition to the problems with
"client" mentioned above, there are no actual clients in this directory (as the
name suggests there should be)! Since the only thing in here is the codereview
directory, why not mode that to ./libs/codereview? Or ./gae_libs/codereview (if
it's AppEngine-dependent)? Or if you really want a third directory then you
should use ./${SOMETHING}_libs/codereview for consistency. (I'd suggest against
"infra_libs" as that's too ambiguous. We're in the infra repo, so that could be
*everything*.)

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/README.md:3: This [infra\_clients](infra_clients)
module is for client code that interacts
s/^This /The /

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/codereview/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/codereview/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/codereview/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/codereview/test/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
File appengine/findit/libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:4: on AppEngine but just the standard python
library. However, it should also be
s/the (standard python library)/a $1/

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:5: AppEngine-compatible -- could run on
AppEngine. For example, writing to the file
s/-- could/-- i.e., can be/

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:6: system should be excluded (os.path is OK
though).
-> "For example, you must not write to the file system"

In specifications, "should" indicates a desired goal (which, in practice, may
fail to obtain); for absolute requirements/prohibitions, use "must" (or
"shall").

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/_...
File appengine/findit/libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/_...
appengine/findit/libs/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
File appengine/findit/libs/http/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
appengine/findit/libs/http/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
File appengine/findit/libs/http/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
appengine/findit/libs/http/test/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/README.md:4: * [app](app): The App Engine app to
provide web APIs to analyze Chrome crashes through invoking the core library
above, and web pages to view & monitor analysis results.
nit: line too long

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/analysis/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/analysis/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/backend/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/backend/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/common/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/common/__init__.py:1: 
ditto

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/frontend/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/frontend/__init__.py:1: 
ditto

[stgao, 2016-11-16 18:41:49]

Description was changed from

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* Add some example of file moving

And the final step is to do a one-off moving after the split is done:
appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* add some example of file moving

And the final step is to do a one-off moving after the split is done:
appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

[stgao, 2016-11-16 18:44:11]

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/README.md:4: APIs (gae is short for Google App
Engine), eg: an NDB model that supports
On 2016/11/15 00:38:00, wrengr wrote:
> The proper spelling is "e.g.,"

Done. Thanks for the correction :)

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/README.md:14: containing directory, and ensure that
the containing directory is in sys.path or
On 2016/11/15 00:38:00, wrengr wrote:
> "containing directory" is non-idomatic. Should be something like "the root
> directory for the project" or "the project's root directory"

It doesn't have to be in the root direction of that project. The key part here
is that directory has to be added to the PYTHONPATH or sys.path.

Use a place holder instead of the word "containing directory".

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/__init__.py:1: 
On 2016/11/15 00:38:00, wrengr wrote:
> Should have the standard copyright preamble

An empty space is also acceptable in the __init__.py in the infra repo, but as
this is going to be shared, I added the copyright file header.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/model/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/model/__init__.py:1: 
On 2016/11/15 00:38:00, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
File appengine/findit/gae_libs/model/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/gae_li...
appengine/findit/gae_libs/model/test/__init__.py:1: 
On 2016/11/15 00:38:00, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/README.md:3: This [infra\_clients](infra_clients)
module is for client code that interacts
On 2016/11/15 00:38:01, wrengr wrote:
> s/^This /The /

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/README.md:3: This [infra\_clients](infra_clients)
module is for client code that interacts
On 2016/11/15 00:38:01, wrengr wrote:
> I don't think "client code" is clear. In programming more generally "client"
> could be any code using those various APIs (so calling the code here "client"
> code isn't terribly helpful, since we're not looking at things from the
> perspective of those libraries). But "client" is also used as a technical name
> for AppEngine applications (so it's not clear whether that's what is meant
here,
> or not).

I agree "client code" is misleading here. I changed it to "client libraries" to
match the case here.
https://cloud.google.com/appengine/docs/python/endpoints/gen_clients

"client" is actually good here. With web service APIs (like the infra service
here), the code module encapsulated to talk to those APIs on the user
application side could be conceptually named client of those APIs. And this is
pretty common from my experience.

Basically, there are two types of client "service client" (from perspective of
deployment) and "service api client" (from perspective of code). Here is the
later.

> 
> Also "infra_clients" isn't a good name. In addition to the problems with
> "client" mentioned above, there are no actual clients in this directory (as
the
> name suggests there should be)! Since the only thing in here is the codereview
> directory, why not mode that to ./libs/codereview? Or ./gae_libs/codereview
(if
> it's AppEngine-dependent)? Or if you really want a third directory then you
> should use ./${SOMETHING}_libs/codereview for consistency. (I'd suggest
against
> "infra_libs" as that's too ambiguous. We're in the infra repo, so that could
be
> *everything*.)

I changed it to "infra_api_clients" to be more specific. "infra_libs" is
terrible in this case, and that's the reason I didn't use to match libs/ or
gae_libs/.
This folder is specific for code that talks to infra service, and there are a
few more others like buildbucket, buildbot, swarming, isolate, etc.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/codereview/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/codereview/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
File appengine/findit/infra_clients/codereview/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/infra_...
appengine/findit/infra_clients/codereview/test/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
File appengine/findit/libs/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:4: on AppEngine but just the standard python
library. However, it should also be
On 2016/11/15 00:38:01, wrengr wrote:
> s/the (standard python library)/a $1/

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:5: AppEngine-compatible -- could run on
AppEngine. For example, writing to the file
On 2016/11/15 00:38:01, wrengr wrote:
> s/-- could/-- i.e., can be/

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/R...
appengine/findit/libs/README.md:6: system should be excluded (os.path is OK
though).
On 2016/11/15 00:38:01, wrengr wrote:
> -> "For example, you must not write to the file system"
> 
> In specifications, "should" indicates a desired goal (which, in practice, may
> fail to obtain); for absolute requirements/prohibitions, use "must" (or
> "shall").

Good point.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/_...
File appengine/findit/libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/_...
appengine/findit/libs/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
File appengine/findit/libs/http/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
appengine/findit/libs/http/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
File appengine/findit/libs/http/test/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/libs/h...
appengine/findit/libs/http/test/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/README.md (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/README.md:4: * [app](app): The App Engine app to
provide web APIs to analyze Chrome crashes through invoking the core library
above, and web pages to view & monitor analysis results.
On 2016/11/15 00:38:01, wrengr wrote:
> nit: line too long

This is OK for text file, but not for code file.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/analysis/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/analysis/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/backend/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/backend/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/common/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/common/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
File appengine/findit/predator/app/frontend/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/40001/appengine/findit/predat...
appengine/findit/predator/app/frontend/__init__.py:1: 
On 2016/11/15 00:38:01, wrengr wrote:
> ditto

Done.

[wrengr, 2016-11-16 21:33:09]

lgtm

https://codereview.chromium.org/2488113005/diff/60001/appengine/findit/gae_li...
File appengine/findit/gae_libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/60001/appengine/findit/gae_li...
appengine/findit/gae_libs/__init__.py:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
Should be 2016 for all of these.

[stgao, 2016-11-16 22:27:56]

https://codereview.chromium.org/2488113005/diff/60001/appengine/findit/gae_li...
File appengine/findit/gae_libs/__init__.py (right):

https://codereview.chromium.org/2488113005/diff/60001/appengine/findit/gae_li...
appengine/findit/gae_libs/__init__.py:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
On 2016/11/16 21:33:09, wrengr wrote:
> Should be 2016 for all of these.

Good catch! Done.
I thought I had updated after copy, but actually I didn't...

[stgao, 2016-11-16 22:28:26]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-11-16 22:28:26]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2488113005/#ps80001
(title: "Fix nit!")

[commit-bot: I haz the power, 2016-11-16 22:28:33]

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

[commit-bot: I haz the power, 2016-11-16 22:30:44]

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

[commit-bot: I haz the power, 2016-11-16 22:30:46]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3288134032738810)
  Infra Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3288133e48d54c10)
  Infra Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3288133e5f68fa10)

[stgao, 2016-11-16 23:12:56]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-11-16 23:12:58]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2488113005/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-11-16 23:13:02]

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

[commit-bot: I haz the power, 2016-11-16 23:52:40]

Description was changed from

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* add some example of file moving

And the final step is to do a one-off moving after the split is done:
appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.
==========

to

==========
[Findit] Re-org code.

In order to avoid confusion between "Findit for Waterfall" and "Findit for
Crashes", we plan to split the original Findit project into two parts:
-Findit: the portion of "Findit for Waterfall" in the original Findit.
-Predator: the portion of "Findit for Crashes" in the original Findit.

The containing project is named "Chrome Culprit Finder" ("Chrome Culprit Finder"
= Findit + Predator).

This CL is the first step:
* set up the appropriate directories
* split Predator code into a sub directory appengine/findit/predator
* add some example of file moving

And the final step is to do a one-off moving after the split is done:
appengine/findit/predator + *.yaml -> appengine/predator
The final code layout would be the same as
https://codereview.chromium.org/2447253002/.

Committed:
https://chromium.googlesource.com/infra/infra/+/76710217389bfb35e38b83a142bb4...
==========

[commit-bot: I haz the power, 2016-11-16 23:52:42]

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