add GCS support to docs server

Add Google Cloud Storage support to the documentation AppEngine server using Cloud Storage official Python client library.

With this CL, URL paths can be directly mapped to Cloud Storage buckets (http://developer.chrome.com/path/... -> gs://bucket/...) using content_storage.json configuration file.

open-source-thrid-party-reviews@ team: this CL adds a third-party library to Chromium repo. The library will be used only in the documentation server and it is the official Google library to access Google Cloud Storage from a Google AppEngine application (docs server).

BUG=338007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250663

[Renato Mangini (chromium), 2014-01-25 00:24:11]

Google Cloud Storage support. Ready for review.

[not at google - send to devlin, 2014-01-27 17:59:30]

thanks Jeffrey for agreeing to be the reviewer for this one :)

[Jeffrey Yasskin, 2014-01-29 01:08:08]

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/content_providers.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:38: # will read the
content_provider configuration from there instead
Instead of what? Also, end comments with punctuation.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:50:
self._using_debug_conf = True
This variable is never used. Do you need it?

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:114: bucket =
bucket.lstrip('gs:/')
lstrip will drop a 'g' or 's' from the start of the bucket name in addition to
the 'gs://' prefix: http://docs.python.org/2/library/stdtypes.html#str.lstrip

Use `bucket = bucket[len('gs://'):]` or similar instead.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/content_providers_test.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers_test.py:107: #
TODO(mangini): create tests for GCS
I'm nervous about checking things in without tests.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:25: from
gcs_file_system_provider import CloudStorageFileSystemProvider
Please keep imports alphabetical.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/extensions_paths.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/extensions_paths.py:34: LOCAL_DEBUG_DIR =
'local_debug'
Why isn't this joined with SERVER2 like the other files?

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:4: 
Please comment that this is mostly documented in gcs_file_system_provider.py.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:20: raise
FileNotFoundError('Read failed for %s: %s' % (filename, e))
Usually traceback.format_exc() is a better context string than str(e).

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:29: for os_path in
files:
Maybe just "return [os_path.filename for os_path in files]"?

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:38: full_path =
full_path.rstrip('/');
"In this mode, the "path_prefix" argument should end in the delimiter specified"
implies you shouldn't strip the trailing '/'.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:41: for _file in
cloudstorage_api.listbucket(full_path, delimiter="/"):
Be consistent about ' vs ". In this file, it looks like ' is more common.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:47: version =
max(version, child_stat)
Please comment whether you want the maximum st_ctime for any file directly in
the |path| or the max st_ctime for any file recursively under it. If you want
the recursive one, you need to do use listbucket without |delimiter="/"|.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:75: logging.debug('gcs:
requested path %s, reading %s' % (path, full_path))
We have an 80-column limit for Chrome Python, right?

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:85: return
Future(delegate=Gettable(resolve))
There's no benefit to returning a delayed Future if you don't start some
processing outside of the delegate. In the case of _ReadFile, it looks like
open() does start some work, so if you call that outside resolve() and read()
inside, you'll avoid blocking in some cases. listbucket() also seems to start
some work, which is waited for when __iter__ is called, so you can profitably
split those too.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:95: raise authError
You should just use "raise" here in order to keep the original stack trace.
http://docs.python.org/2/reference/simple_stmts.html#the-raise-statement

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system_provider.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:31: If running
in Preview mode or in Development mode with use_local_fs set to
Can you add some piece of this to chrome/common/extensions/docs/README so people
can find how to preview the github docs?

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:78: from
gcs_file_system import CloudStorageFileSystem
Comment why this is imported late.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:80:
debug_access_token, debug_bucket_prefix)
Check the indentation.

[Renato Mangini (chromium), 2014-01-31 02:29:03]

Comments addressed. PTAL.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/content_providers.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:38: # will read the
content_provider configuration from there instead
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Instead of what? Also, end comments with punctuation.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:50:
self._using_debug_conf = True
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> This variable is never used. Do you need it?

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers.py:114: bucket =
bucket.lstrip('gs:/')
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> lstrip will drop a 'g' or 's' from the start of the bucket name in addition to
> the 'gs://' prefix: http://docs.python.org/2/library/stdtypes.html#str.lstrip
> 
> Use `bucket = bucket[len('gs://'):]` or similar instead.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/content_providers_test.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers_test.py:107: #
TODO(mangini): create tests for GCS
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> I'm nervous about checking things in without tests.

I agree with you. If you don't mind, I commit myself to submit these tests next
week. It's summit+offsite week for my team and I won't have time to write them
now, but the tech writers need to start testing this change on the staging
server without the _patch URL asap.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/cron_servlet.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/cron_servlet.py:25: from
gcs_file_system_provider import CloudStorageFileSystemProvider
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Please keep imports alphabetical.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/extensions_paths.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/extensions_paths.py:34: LOCAL_DEBUG_DIR =
'local_debug'
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Why isn't this joined with SERVER2 like the other files?

Done. I used dev_appserver.py directly, which required to have it non-prefixed.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:4: 
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Please comment that this is mostly documented in gcs_file_system_provider.py.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:20: raise
FileNotFoundError('Read failed for %s: %s' % (filename, e))
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Usually traceback.format_exc() is a better context string than str(e).

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:29: for os_path in
files:
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Maybe just "return [os_path.filename for os_path in files]"?

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:38: full_path =
full_path.rstrip('/');
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> "In this mode, the "path_prefix" argument should end in the delimiter
specified"
> implies you shouldn't strip the trailing '/'.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:41: for _file in
cloudstorage_api.listbucket(full_path, delimiter="/"):
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Be consistent about ' vs ". In this file, it looks like ' is more common.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:47: version =
max(version, child_stat)
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Please comment whether you want the maximum st_ctime for any file directly in
> the |path| or the max st_ctime for any file recursively under it. If you want
> the recursive one, you need to do use listbucket without |delimiter="/"|.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:75: logging.debug('gcs:
requested path %s, reading %s' % (path, full_path))
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> We have an 80-column limit for Chrome Python, right?

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:85: return
Future(delegate=Gettable(resolve))
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> There's no benefit to returning a delayed Future if you don't start some
> processing outside of the delegate. In the case of _ReadFile, it looks like
> open() does start some work, so if you call that outside resolve() and read()
> inside, you'll avoid blocking in some cases. listbucket() also seems to start
> some work, which is waited for when __iter__ is called, so you can profitably
> split those too.

But by splitting open and read I will loose the benefit of the constrained
"with" context with auto-closing and would need to keep a reference to the open
connection for an unforeseeable time, until the Future delegate is executed. Is
it worth?

BTW: I used the same approach of local_file_system, since I wasn't familiar with
this implementation of Futures.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:95: raise authError
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> You should just use "raise" here in order to keep the original stack trace.
> http://docs.python.org/2/reference/simple_stmts.html#the-raise-statement

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system_provider.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:31: If running
in Preview mode or in Development mode with use_local_fs set to
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Can you add some piece of this to chrome/common/extensions/docs/README so
people
> can find how to preview the github docs?

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:78: from
gcs_file_system import CloudStorageFileSystem
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Comment why this is imported late.

Done.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:80:
debug_access_token, debug_bucket_prefix)
On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> Check the indentation.

Done.

[Jeffrey Yasskin, 2014-02-03 22:12:56]

Sorry for the delay. LGTM after these changes.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/content_providers_test.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/content_providers_test.py:107: #
TODO(mangini): create tests for GCS
On 2014/01/31 02:29:04, Renato Mangini (chromium) wrote:
> On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> > I'm nervous about checking things in without tests.
> 
> I agree with you. If you don't mind, I commit myself to submit these tests
next
> week. It's summit+offsite week for my team and I won't have time to write them
> now, but the tech writers need to start testing this change on the staging
> server without the _patch URL asap.

SG

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system.py (right):

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:20: raise
FileNotFoundError('Read failed for %s: %s' % (filename, e))
On 2014/01/31 02:29:04, Renato Mangini (chromium) wrote:
> On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> > Usually traceback.format_exc() is a better context string than str(e).
> 
> Done.

Please do this everywhere you're currently using %s on an exception object.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:85: return
Future(delegate=Gettable(resolve))
On 2014/01/31 02:29:04, Renato Mangini (chromium) wrote:
> On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> > There's no benefit to returning a delayed Future if you don't start some
> > processing outside of the delegate. In the case of _ReadFile, it looks like
> > open() does start some work, so if you call that outside resolve() and
read()
> > inside, you'll avoid blocking in some cases. listbucket() also seems to
start
> > some work, which is waited for when __iter__ is called, so you can
profitably
> > split those too.
> 
> But by splitting open and read I will loose the benefit of the constrained
> "with" context with auto-closing and would need to keep a reference to the
open
> connection for an unforeseeable time, until the Future delegate is executed.
Is
> it worth?

I think so, given all the fixes Ben had to make to get the docserver to respond
quickly. However, there's also a risk that the Future will never have get()
called, in which case the connection might never get closed (unless it's
automatically closed by GC, which is possible). It's up to you which risk you
want to take.

> BTW: I used the same approach of local_file_system, since I wasn't familiar
with
> this implementation of Futures.

Yeah, they're a little weird. It'd be good eventually to replace them with the
appengine ndb futures so we can use 'yield' syntax to use them, and so they'll
have more than 2 phases of execution.

https://codereview.chromium.org/139303023/diff/810001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:95: raise authError
On 2014/01/31 02:29:04, Renato Mangini (chromium) wrote:
> On 2014/01/29 01:08:08, Jeffrey Yasskin wrote:
> > You should just use "raise" here in order to keep the original stack trace.
> > http://docs.python.org/2/reference/simple_stmts.html#the-raise-statement
> 
> Done.

Not done.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system.py (right):

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:34: for os_path in
files:
I think you can remove these 3 lines now.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system_provider.py (right):

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:11: from
empty_dir_file_system import EmptyDirFileSystem
Keep these alphabetized too. It's fine to put "import" statements in a separate
block from "from" statements since that lets you just run your editor's 'sort'
command.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:79: so we only
import it when we are sure we are not on preview.py or tests
Use # comments everywhere except docstrings. And end comments with punctuation
so it's clear that you finished your thought.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:83:
debug_access_token, debug_bucket_prefix)
Not quite right: http://www.python.org/dev/peps/pep-0008/#indentation (see the
rules on "hanging indent".)

[Renato Mangini (chromium), 2014-02-04 02:07:08]

PTAL. Other comments also addressed, although rietveld are not showing them in
the drafts messages below. 

Re the discussion on Futures, I prefer having a predictable slower read time
than a race condition of too many files open. Ideally, when we fix the bug with
CachedFileSystem, read operations in the GCSFileSystem will only occur when a
file's stat is newer than the one cached in memcache or database.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system.py (right):

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system.py:34: for os_path in
files:
On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> I think you can remove these 3 lines now.

ugh, I thought I had removed them. Bad connection in hotel + byobu not properly
configured = frequent disconnect + vim buffer lost. Sorry, done now.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
File chrome/common/extensions/docs/server2/gcs_file_system_provider.py (right):

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:11: from
empty_dir_file_system import EmptyDirFileSystem
On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> Keep these alphabetized too. It's fine to put "import" statements in a
separate
> block from "from" statements since that lets you just run your editor's 'sort'
> command.

Done.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:79: so we only
import it when we are sure we are not on preview.py or tests
On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> Use # comments everywhere except docstrings. And end comments with punctuation
> so it's clear that you finished your thought.

Done.

https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
chrome/common/extensions/docs/server2/gcs_file_system_provider.py:83:
debug_access_token, debug_bucket_prefix)
On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> Not quite right: http://www.python.org/dev/peps/pep-0008/#indentation (see the
> rules on "hanging indent".)

Done.

[Renato Mangini (chromium), 2014-02-04 02:13:37]

Actually, Jeffrey, you don't need to take another look if you don't want, since
you have already LGTM'ed if the comments were addressed.

I will send to CQ tomorrow morning.

Thanks!

On 2014/02/04 02:07:08, Renato Mangini (chromium) wrote:
> PTAL. Other comments also addressed, although rietveld are not showing them in
> the drafts messages below. 
> 
> Re the discussion on Futures, I prefer having a predictable slower read time
> than a race condition of too many files open. Ideally, when we fix the bug
with
> CachedFileSystem, read operations in the GCSFileSystem will only occur when a
> file's stat is newer than the one cached in memcache or database.
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> File chrome/common/extensions/docs/server2/gcs_file_system.py (right):
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> chrome/common/extensions/docs/server2/gcs_file_system.py:34: for os_path in
> files:
> On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> > I think you can remove these 3 lines now.
> 
> ugh, I thought I had removed them. Bad connection in hotel + byobu not
properly
> configured = frequent disconnect + vim buffer lost. Sorry, done now.
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> File chrome/common/extensions/docs/server2/gcs_file_system_provider.py
(right):
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> chrome/common/extensions/docs/server2/gcs_file_system_provider.py:11: from
> empty_dir_file_system import EmptyDirFileSystem
> On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> > Keep these alphabetized too. It's fine to put "import" statements in a
> separate
> > block from "from" statements since that lets you just run your editor's
'sort'
> > command.
> 
> Done.
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> chrome/common/extensions/docs/server2/gcs_file_system_provider.py:79: so we
only
> import it when we are sure we are not on preview.py or tests
> On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> > Use # comments everywhere except docstrings. And end comments with
punctuation
> > so it's clear that you finished your thought.
> 
> Done.
> 
>
https://codereview.chromium.org/139303023/diff/990001/chrome/common/extension...
> chrome/common/extensions/docs/server2/gcs_file_system_provider.py:83:
> debug_access_token, debug_bucket_prefix)
> On 2014/02/03 22:12:56, Jeffrey Yasskin wrote:
> > Not quite right: http://www.python.org/dev/peps/pep-0008/#indentation (see
the
> > rules on "hanging indent".)
> 
> Done.

[Jeffrey Yasskin, 2014-02-04 03:06:03]

lgtm

[Renato Mangini (chromium), 2014-02-04 17:07:48]

The CQ bit was checked by mangini@chromium.org

[Renato Mangini (chromium), 2014-02-04 17:07:51]

The CQ bit was unchecked by mangini@chromium.org

[Renato Mangini (chromium), 2014-02-04 17:33:20]

The CQ bit was checked by mangini@chromium.org

[commit-bot: I haz the power, 2014-02-04 17:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mangini@chromium.org/139303023/1280001

[commit-bot: I haz the power, 2014-02-04 18:14:26]

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

[commit-bot: I haz the power, 2014-02-04 18:14:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Daniel Berlin, 2014-02-07 21:07:53]

This looks fine for open-source-third-party-reviews

[Renato Mangini (chromium), 2014-02-07 21:37:24]

On 2014/02/07 21:07:53, Daniel Berlin wrote:
> This looks fine for open-source-third-party-reviews

Thank you, Daniel. Could you please send a lgtm, otherwise the commit-bot won't
let me commit it.

[Jeffrey Yasskin, 2014-02-07 21:42:07]

You actually need Pawel or Lei, who own that directory:
https://code.google.com/p/chromium/codesearch/#chromium/src/tools/checklicens...

[Jeffrey Yasskin, 2014-02-07 21:51:32]

Oh, and:
* an OWNERS file
(http://www.chromium.org/developers/adding-3rd-party-libraries#TOC-Add-OWNERS)
*a review from security@, although they'll hopefully be encouraged that this
isn't being used in Chrome itself, just an appengine app
* an lgtm from someone in
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/OWNERS,
perhaps cpu@.

[Paweł Hajdan Jr., 2014-02-07 21:57:12]

https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
File tools/checklicenses/checklicenses.py (right):

https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
tools/checklicenses/checklicenses.py:148: #
https://code.google.com/p/appengine-gcs-client/issues/detail?id=31
This is a Google project. Could you give it a try to get the license headers
before checking it into Chrome?

[Renato Mangini (chromium), 2014-02-07 21:58:18]

On 2014/02/07 21:51:32, Jeffrey Yasskin wrote:
> Oh, and:
> * an OWNERS file

OWNERS added.

[Daniel Berlin, 2014-02-07 22:46:28]

 I will take care of berating the other project :)

On Fri, Feb 7, 2014 at 1:57 PM,  <phajdan.jr@chromium.org> wrote:
>
>
https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
> File tools/checklicenses/checklicenses.py (right):
>
>
https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
> tools/checklicenses/checklicenses.py:148: #
> https://code.google.com/p/appengine-gcs-client/issues/detail?id=31
> This is a Google project. Could you give it a try to get the license
> headers before checking it into Chrome?
>
> https://codereview.chromium.org/139303023/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2014-02-10 20:07:47]

lgtm

Please flesh out more the bug. Like enough for a human from a different team to
grasp what you are doing.

[Renato Mangini (chromium), 2014-02-10 20:26:05]

On 2014/02/10 20:07:47, cpu wrote:
> lgtm
> 
> Please flesh out more the bug. Like enough for a human from a different team
to
> grasp what you are doing.

Done, thanks.

[Renato Mangini (chromium), 2014-02-10 22:21:20]

On 2014/02/07 21:57:12, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
> File tools/checklicenses/checklicenses.py (right):
> 
>
https://codereview.chromium.org/139303023/diff/1510001/tools/checklicenses/ch...
> tools/checklicenses/checklicenses.py:148: #
> https://code.google.com/p/appengine-gcs-client/issues/detail?id=31
> This is a Google project. Could you give it a try to get the license headers
> before checking it into Chrome?


Pawel, I'm working with Ye to have the license headers in the third-party
libraries, but I need my team to start testing this CL on docs staging server
asap, which can only happen when it gets committed. Could you please lgtm while
we get these license headers in the library?

[Paweł Hajdan Jr., 2014-02-11 00:34:52]

On 2014/02/10 22:21:20, Renato Mangini (chromium) wrote:
> Pawel, I'm working with Ye to have the license headers in the third-party
> libraries, but I need my team to start testing this CL on docs staging server
> asap, which can only happen when it gets committed. Could you please lgtm
while
> we get these license headers in the library?

As replied in the other e-mail, I'll still try to be as flexible as reasonably
possible, but if it's really so urgent adding license headers to a Google
project should still be reasonably easy and you should be able to do that.

[Renato Mangini (chromium), 2014-02-12 02:58:53]

The CQ bit was checked by mangini@chromium.org

[commit-bot: I haz the power, 2014-02-12 03:52:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mangini@chromium.org/139303023/1940001

[commit-bot: I haz the power, 2014-02-12 09:34:36]

Change committed as 250663