Protocol, UI and smoke test for Primary <-> Replica linking process.

Protocol, UI and smoke test for Primary <-> Replica linking process.

The workflow to add a new auth db replica:
1. Admin of Primary goes to 'Services' page, enters app_id of the replica
   and clicks 'Generate linking URL'.
2. Auth service generates a link that looks like
   https://<replica-id>.appspot.com/auth/link?t=<base64>.
3. Admin (in GAE sense) of replica clicks this link and sees confirmation page.
4. Upon confirmation, replica contacts primary (via urlfetch), and if
   everything looks correct, they are now linked (know about each other and
   have a shared secret).

R=maruel@chromium.org
BUG=

Committed: https://code.google.com/p/swarming/source/detail?repo=default&r=01004cb

[vadimsh, 2014-06-29 21:53:17]

There're 3 chunks here:
1. Actual CL logic.
2. Smoke test.
3. PRESUBMIT.py modification to ignore *_pb2.py files.

Suggested review order:
1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
ticket is generated).
2. auth/ui/ui.py (it's where ticket is handled).
3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
calls to register itself).
4. Everything else.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
File services/auth_service/tests/replica_app/app.yaml (right):

https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
dependencies of components/auth.

app.yaml includes can't specify libraries :(

[vadimsh, 2014-06-29 22:06:25]

btw, runs here:
https://818-2bc3ef0-tainted-vadimsh-dot-chrome-infra-auth-dev.appspot.com/aut...

But there's no second service running this code as a default version to attach
as a replica. So it's a limited demo.

[M-A, 2014-06-30 01:18:31]

https://codereview.appspot.com/106310043/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.appspot.com/106310043/diff/1/PRESUBMIT.py#newcode32
PRESUBMIT.py:32: excluded_paths=[r'.*_pb2\.py'],
in theory, you'd want $

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
File services/auth_service/common/replication.py (right):

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:35: shared_secret =
ndb.BlobProperty(indexed=False)
Why not use asymmetric encryption?

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:39: """Create a new AuthReplicaState
or reset the state of existing one."""
Creates
resets

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:44: auth_db_rev=0,
Why not use default=0?

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:45: rev_modified_ts=None,
Not really necessary?

https://codereview.appspot.com/106310043/diff/1/services/auth_service/fronten...
File services/auth_service/frontend/handlers.py (right):

https://codereview.appspot.com/106310043/diff/1/services/auth_service/fronten...
services/auth_service/frontend/handlers.py:115: host = 'https://%s.appspot.com'
% app_id
What about non-appspot domains? No need to handle this right away, just
wondering if you have an idea about these.

https://codereview.appspot.com/106310043/diff/1/services/components/component...
File services/components/components/auth/replication.py (right):

https://codereview.appspot.com/106310043/diff/1/services/components/component...
services/components/components/auth/replication.py:33: (True, None) on success.
Why not exception? Especially that urlfetch raises in case of deadline so you'll
have to manage exceptions anyway.

[vadimsh, 2014-06-30 22:58:52]

https://codereview.appspot.com/106310043/diff/1/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.appspot.com/106310043/diff/1/PRESUBMIT.py#newcode32
PRESUBMIT.py:32: excluded_paths=[r'.*_pb2\.py'],
On 2014/06/30 01:18:31, M-A wrote:
> in theory, you'd want $

Done.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
File services/auth_service/common/replication.py (right):

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:35: shared_secret =
ndb.BlobProperty(indexed=False)
On 2014/06/30 01:18:31, M-A wrote:
> Why not use asymmetric encryption?

Good idea. I initially (~several months ago) wanted to do so, and forgot since
then. I'll use built-in APIs
(https://developers.google.com/appengine/docs/python/appidentity/functions):
sign_blob and get_public_certificates.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:39: """Create a new AuthReplicaState
or reset the state of existing one."""
On 2014/06/30 01:18:31, M-A wrote:
> Creates
> resets

Done.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:44: auth_db_rev=0,
On 2014/06/30 01:18:31, M-A wrote:
> Why not use default=0?

Done.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/common/...
services/auth_service/common/replication.py:45: rev_modified_ts=None,
On 2014/06/30 01:18:31, M-A wrote:
> Not really necessary?

Done.

https://codereview.appspot.com/106310043/diff/1/services/auth_service/fronten...
File services/auth_service/frontend/handlers.py (right):

https://codereview.appspot.com/106310043/diff/1/services/auth_service/fronten...
services/auth_service/frontend/handlers.py:115: host = 'https://%s.appspot.com'
% app_id
On 2014/06/30 01:18:31, M-A wrote:
> What about non-appspot domains? No need to handle this right away, just
> wondering if you have an idea about these.

Well, the mechanism I use on dev_appserver (i.e. using ids like
'isolateserver-dev@localhost:9090') can totally work on real GAE too. The model
doesn't encode '*.appspot.com' anywhere (since I test that model on localhost
with 'custom' urls).

https://codereview.appspot.com/106310043/diff/1/services/components/component...
File services/components/components/auth/replication.py (right):

https://codereview.appspot.com/106310043/diff/1/services/components/component...
services/components/components/auth/replication.py:33: (True, None) on success.
On 2014/06/30 01:18:31, M-A wrote:
> Why not exception? Especially that urlfetch raises in case of deadline so
you'll
> have to manage exceptions anyway.

Done.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
File services/auth_service/frontend/handlers.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/handlers.py:86:
self.send_response(get_public_certificates())
This stuff looks like this:
https://822-cd37527-tainted-vadimsh-dot-chrome-infra-auth-dev.appspot.com/aut...

sign_blob returns tuple (key_name, <signature>). Primary will send both to
Replica. Replicas will fetch public certs via auth_service/api/v1/certificates
API (thus relying on SSL certs for authenticity), and would verify signature of
the blob.

To be honest, I can't think of a scenario when X-Appengine-Inbound-Appid
wouldn't be enough to confirm authenticity of Primary. But it's relatively easy
to add the signature, and the code may be useful for other stuff too.

[M-A, 2014-07-01 01:06:35]

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
File services/auth_service/frontend/handlers.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/handlers.py:162: self.response.set_status(200)
200 is always implied, so it's not really needed.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/handlers.py:225:
r'/auth_service/api/v1/internal/link_replica',
I'd prefer the routes to stay sorted.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
File services/auth_service/frontend/templates/services.html (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:35: <strong>Linking
URL:</strong> <a target="_blank" href="{{url}}">link</a>.
s/link/{{url}}/ ?

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:47: <!-- List of
registered services. !-->
Replace <!-- with {#  #} everywhere.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:57: class
DevServerApplication(object):
A lot of this code is functionally similar to
services/swarming/tests/server_smoke_test.py. I think it'd be good to dedupe it
a little. That said I don't mind for this to be done later.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:204: def request(self,
url, body=None, headers=None):
s/url/resource/ ?

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:239: self.auth_service =
DevServerApplication(SERVICE_APP_DIR, 9555, 9560)
Use find_free_port() instead of hard coding them.

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
File services/components/components/auth/proto/replication.proto (right):

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
services/components/components/auth/proto/replication.proto:34: required string
primary_id = 1;
https://developers.google.com/protocol-buffers/docs/proto#simple
Section "Specifying Field Rules"
Yellow box "Required Is Forever".

Are you really sure you want to mark everything as required?

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
File services/components/components/auth/ui/ui.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
services/components/components/auth/ui/ui.py:204:
self.request.GET['t'].encode('ascii'))
Why enforce t to be in the URL even for post?

[vadimsh, 2014-07-01 03:07:34]

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
File services/auth_service/frontend/handlers.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/handlers.py:162: self.response.set_status(200)
On 2014/07/01 01:06:34, M-A wrote:
> 200 is always implied, so it's not really needed.

Done.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/handlers.py:225:
r'/auth_service/api/v1/internal/link_replica',
On 2014/07/01 01:06:34, M-A wrote:
> I'd prefer the routes to stay sorted.

Done.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
File services/auth_service/frontend/templates/services.html (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:35: <strong>Linking
URL:</strong> <a target="_blank" href="{{url}}">link</a>.
On 2014/07/01 01:06:34, M-A wrote:
> s/link/{{url}}/ ?
It's freaking huge :) And I don't know enough CSS to neatly display it so it is
easily copy-pastable. 

For the reference, here's how the link looks:
https://isolateserver-dev.appspot.com/auth/link?t=ChVjaHJvbWUtaW5mcmEtYXV0aC1...

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:47: <!-- List of
registered services. !-->
On 2014/07/01 01:06:34, M-A wrote:
> Replace <!-- with {#  #} everywhere.

Done.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:57: class
DevServerApplication(object):
On 2014/07/01 01:06:35, M-A wrote:
> A lot of this code is functionally similar to
> services/swarming/tests/server_smoke_test.py. I think it'd be good to dedupe
it
> a little. That said I don't mind for this to be done later.
Yeah, that was my plan, I'll do it in a follow up CL, that one is already huge.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:204: def request(self,
url, body=None, headers=None):
On 2014/07/01 01:06:34, M-A wrote:
> s/url/resource/ ?

Done.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:239: self.auth_service =
DevServerApplication(SERVICE_APP_DIR, 9555, 9560)
On 2014/07/01 01:06:34, M-A wrote:
> Use find_free_port() instead of hard coding them.

It doesn't really work for multi-module apps (like isolate and this
auth_service). If I pass --port=8080, default module will run on 8080, and
backend on 8081, and so on.

So I decided it's better to fail reliably hitting same hardcoded port (if it is
busy), rather then failing randomly (hitting occupied find_free_port()+1).

Added bonus: if for some reason dev_appserver doesn't die, the test won't start
(it checks that port is not responding to HTTP before starting). That way we can
caught undead app servers earlier, I hope.

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
File services/components/components/auth/proto/replication.proto (right):

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
services/components/components/auth/proto/replication.proto:34: required string
primary_id = 1;
On 2014/07/01 01:06:35, M-A wrote:
> https://developers.google.com/protocol-buffers/docs/proto#simple
> Section "Specifying Field Rules"
> Yellow box "Required Is Forever".
> 
> Are you really sure you want to mark everything as required?

I dunno... I don't feel like writing a lot of 'if HasField(...)' checks now.
Stuff here looks pretty 'required' by my current understanding. And they are
scalar fields, so in the worst case we can send empty strings, etc. :) But I
don't think it will not be required in the future.

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
File services/components/components/auth/ui/ui.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
services/components/components/auth/ui/ui.py:204:
self.request.GET['t'].encode('ascii'))
On 2014/07/01 01:06:35, M-A wrote:
> Why enforce t to be in the URL even for post?

It's just there already, the form is sending POST to page's URL.

I can move it a hidden form field (and thus into POST args), and target form to
some different URL (perhaps just /link, without ?t=...). But it's same shit
basically, just more code. As you prefer, I can make this change.

[M-A, 2014-07-01 13:00:42]

lots of nit picking but nothing super important. lgtm.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:239: self.auth_service =
DevServerApplication(SERVICE_APP_DIR, 9555, 9560)
On 2014/07/01 03:07:34, vadimsh wrote:
> On 2014/07/01 01:06:34, M-A wrote:
> > Use find_free_port() instead of hard coding them.
> 
> It doesn't really work for multi-module apps (like isolate and this
> auth_service). If I pass --port=8080, default module will run on 8080, and
> backend on 8081, and so on.

Bah, it's still feasible, just annoying as the loop has to find 2 consecutive
ports. Not that much a big deal to implement.

Anyhow, I can code it later.

> So I decided it's better to fail reliably hitting same hardcoded port (if it
is
> busy), rather then failing randomly (hitting occupied find_free_port()+1).

It becomes a burden when running the tests in parallel, which PRESUBMIT.py does.


> Added bonus: if for some reason dev_appserver doesn't die, the test won't
start
> (it checks that port is not responding to HTTP before starting). That way we
can
> caught undead app servers earlier, I hope.  

I reboot to kill them. :)

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
File services/components/components/auth/ui/ui.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/components/compo...
services/components/components/auth/ui/ui.py:204:
self.request.GET['t'].encode('ascii'))
On 2014/07/01 03:07:34, vadimsh wrote:
> On 2014/07/01 01:06:35, M-A wrote:
> > Why enforce t to be in the URL even for post?
> 
> It's just there already, the form is sending POST to page's URL.
> 
> I can move it a hidden form field (and thus into POST args), and target form
to
> some different URL (perhaps just /link, without ?t=...). But it's same shit
> basically, just more code. As you prefer, I can make this change.

No, I meant replacing self.request.GET['t'] with self.request.get('t').

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/fro...
File services/auth_service/frontend/templates/services.html (right):

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:89: 
remove one line

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:207: if not
resource.startswith(self.url):
I'm not sure lines 204-233 + 60-62 + 74 belongs to this class. Maybe what you
want is a proper http client class, more something like
https://code.google.com/p/swarming/source/browse/services/swarming/swarm_bot/...
albeit with the json_request in addition. I want to refactor this class in
client/utils/net.py one day once I switch the bot code to use it.

You could set DevServerApplication.client = XsrfClient(self.url) around line 137
so the unit test could do something like:

self.replica.client.request(...)

I tad more verbose but the code would be easier to read IMHO.

*or* even simpler, make 
class DevServerApplication(XsrfClient):

so that self.url, as defined by DevServerApplication, is used by XsrfClient
automatically

wdyt?

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/proto/replication_pb2.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/proto/replication_pb2.py:1: # Generated by
the protocol buffer compiler.  DO NOT EDIT!
FTR, I'm not super happy in leaving the file along non-generated files, I'd
prefer it to be at least in a subdirectory to make the distinction clearer. That
said, I won't block this CL on that and this could be cleaned up later.

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/replication.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/replication.py:83: errors = {
This should be a global.

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/ui/ui.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/ui/ui.py:214: env={
In practice, the code would be slightly more readable and one line shorter if
you used a local variable to hold env. Same for the next function.

That's 2 lines less. We're going to be rich.

https://codereview.appspot.com/106310043/diff/40001/services/components/suppo...
File services/components/support/gae_sdk_utils.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/suppo...
services/components/support/gae_sdk_utils.py:272: Subprocess.
subprocess.Popen instance.

[vadimsh, 2014-07-01 23:11:49]

Replied to all comments. PTAL, esp at replication_smoke_test.py.

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/20001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:239: self.auth_service =
DevServerApplication(SERVICE_APP_DIR, 9555, 9560)
On 2014/07/01 13:00:42, M-A wrote:
> On 2014/07/01 03:07:34, vadimsh wrote:
> > On 2014/07/01 01:06:34, M-A wrote:
> > > Use find_free_port() instead of hard coding them.
> > 
> > It doesn't really work for multi-module apps (like isolate and this
> > auth_service). If I pass --port=8080, default module will run on 8080, and
> > backend on 8081, and so on.
> 
> Bah, it's still feasible, just annoying as the loop has to find 2 consecutive
> ports. Not that much a big deal to implement.
> 
> Anyhow, I can code it later.
> 
> > So I decided it's better to fail reliably hitting same hardcoded port (if it
> is
> > busy), rather then failing randomly (hitting occupied find_free_port()+1).
> 
> It becomes a burden when running the tests in parallel, which PRESUBMIT.py
does.
> 
> 
> > Added bonus: if for some reason dev_appserver doesn't die, the test won't
> start
> > (it checks that port is not responding to HTTP before starting). That way we
> can
> > caught undead app servers earlier, I hope.  
> 
> I reboot to kill them. :)

Ok. Now using find_free_ports.

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/fro...
File services/auth_service/frontend/templates/services.html (right):

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/fro...
services/auth_service/frontend/templates/services.html:89: 
On 2014/07/01 13:00:42, M-A wrote:
> remove one line

Done.

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/tes...
File services/auth_service/tests/replication_smoke_test.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/auth_service/tes...
services/auth_service/tests/replication_smoke_test.py:207: if not
resource.startswith(self.url):
On 2014/07/01 13:00:42, M-A wrote:
> I'm not sure lines 204-233 + 60-62 + 74 belongs to this class. Maybe what you
> want is a proper http client class, more something like
>
https://code.google.com/p/swarming/source/browse/services/swarming/swarm_bot/...
> albeit with the json_request in addition. I want to refactor this class in
> client/utils/net.py one day once I switch the bot code to use it.
> 
> You could set DevServerApplication.client = XsrfClient(self.url) around line
137
> so the unit test could do something like:
> 
> self.replica.client.request(...)
> 
> I tad more verbose but the code would be easier to read IMHO.
> 
> *or* even simpler, make 
> class DevServerApplication(XsrfClient):
> 
> so that self.url, as defined by DevServerApplication, is used by XsrfClient
> automatically
> 
> wdyt?

Made a separate HttpClient class, available as a property of
DevServerApplication class.

I don't think automatic XSRF token insertion is a good idea for code that does
testing. I'd prefer all headers and parameters to be explicitly listed.

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/proto/replication_pb2.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/proto/replication_pb2.py:1: # Generated by
the protocol buffer compiler.  DO NOT EDIT!
On 2014/07/01 13:00:42, M-A wrote:
> FTR, I'm not super happy in leaving the file along non-generated files, I'd
> prefer it to be at least in a subdirectory to make the distinction clearer.
That
> said, I won't block this CL on that and this could be cleaned up later.

That's why it is in 'proto' directory. In google3 generated *.py files are also
side-by-side with *.proto files (though, they are generated only during the
build).

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/replication.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/replication.py:83: errors = {
On 2014/07/01 13:00:42, M-A wrote:
> This should be a global.

Done.

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
File services/components/components/auth/ui/ui.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/compo...
services/components/components/auth/ui/ui.py:214: env={
On 2014/07/01 13:00:42, M-A wrote:
> In practice, the code would be slightly more readable and one line shorter if
> you used a local variable to hold env. Same for the next function.
> 
> That's 2 lines less. We're going to be rich.

Done.

https://codereview.appspot.com/106310043/diff/40001/services/components/suppo...
File services/components/support/gae_sdk_utils.py (right):

https://codereview.appspot.com/106310043/diff/40001/services/components/suppo...
services/components/support/gae_sdk_utils.py:272: Subprocess.
On 2014/07/01 13:00:42, M-A wrote:
> subprocess.Popen instance.

Done.

[M-A, 2014-07-02 01:27:17]

lgtm

[vadimsh, 2014-07-02 02:42:32]

Committed patchset #4 manually as r01004cb (presubmit successful).

[Omondo1, 2023-12-27 02:52:54]

On 2014/06/29 21:53:17, vadimsh wrote:
> There're 3 chunks here:
> 1. Actual CL logic.
> 2. Smoke test.
> 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> 
> Suggested review order:
> 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
> ticket is generated).
> 2. auth/ui/ui.py (it's where ticket is handled).
> 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
> calls to register itself).
> 4. Everything else.
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> File services/auth_service/tests/replica_app/app.yaml (right):
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> dependencies of components/auth.
> 
> app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:53:22]

On 2014/06/29 21:53:17, vadimsh wrote:
> There're 3 chunks here:
> 1. Actual CL logic.
> 2. Smoke test.
> 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> 
> Suggested review order:
> 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
> ticket is generated).
> 2. auth/ui/ui.py (it's where ticket is handled).
> 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
> calls to register itself).
> 4. Everything else.
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> File services/auth_service/tests/replica_app/app.yaml (right):
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> dependencies of components/auth.
> 
> app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:53:23]

On 2014/06/29 21:53:17, vadimsh wrote:
> There're 3 chunks here:
> 1. Actual CL logic.
> 2. Smoke test.
> 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> 
> Suggested review order:
> 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
> ticket is generated).
> 2. auth/ui/ui.py (it's where ticket is handled).
> 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
> calls to register itself).
> 4. Everything else.
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> File services/auth_service/tests/replica_app/app.yaml (right):
> 
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> dependencies of components/auth.
> 
> app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:53:38]

On 2023/12/27 02:53:23, Omondo1 wrote:
> On 2014/06/29 21:53:17, vadimsh wrote:
> > There're 3 chunks here:
> > 1. Actual CL logic.
> > 2. Smoke test.
> > 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> > 
> > Suggested review order:
> > 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
> > ticket is generated).
> > 2. auth/ui/ui.py (it's where ticket is handled).
> > 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
> > calls to register itself).
> > 4. Everything else.
> > 
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > File services/auth_service/tests/replica_app/app.yaml (right):
> > 
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> > dependencies of components/auth.
> > 
> > app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:53:40]

On 2023/12/27 02:53:23, Omondo1 wrote:
> On 2014/06/29 21:53:17, vadimsh wrote:
> > There're 3 chunks here:
> > 1. Actual CL logic.
> > 2. Smoke test.
> > 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> > 
> > Suggested review order:
> > 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where initial
> > ticket is generated).
> > 2. auth/ui/ui.py (it's where ticket is handled).
> > 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what replica
> > calls to register itself).
> > 4. Everything else.
> > 
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > File services/auth_service/tests/replica_app/app.yaml (right):
> > 
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> > dependencies of components/auth.
> > 
> > app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:53:55]

On 2023/12/27 02:53:40, Omondo1 wrote:
> On 2023/12/27 02:53:23, Omondo1 wrote:
> > On 2014/06/29 21:53:17, vadimsh wrote:
> > > There're 3 chunks here:
> > > 1. Actual CL logic.
> > > 2. Smoke test.
> > > 3. PRESUBMIT.py modification to ignore *_pb2.py files.
> > > 
> > > Suggested review order:
> > > 1. auth_service/frontend/handlers.py, GenerateLinkingURL (it's where
initial
> > > ticket is generated).
> > > 2. auth/ui/ui.py (it's where ticket is handled).
> > > 3. auth_service/frontend/handlers.py, LinkRequestHandler (it's what
replica
> > > calls to register itself).
> > > 4. Everything else.
> > > 
> > >
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > > File services/auth_service/tests/replica_app/app.yaml (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/106310043/diff/1/services/auth_service/tests/r...
> > > services/auth_service/tests/replica_app/app.yaml:14: - name: jinja2
> > > dependencies of components/auth.
> > > 
> > > app.yaml includes can't specify libraries :(

[Omondo1, 2023-12-27 02:59:56]

Publish all my draft

[tricube19842103, 2024-01-18 01:11:22]

|a.com/com.a/f.termin/termin.f/