Adds config component to Machine Provider

Adds config component to Machine Provider

R=maruel@chromium.org
CC=smut@chromium.org
BUG=636040
Committed: https://github.com/luci/luci-py/commit/9ecf78cae274e26e9ea68b2dca66e60a5f834c6e

[M-A Ruel, 2016-08-09 19:07:03]

Description was changed from

==========
Adds config component to Machine Provider
R=maruel@chromium.org
CC=smut@chromium.org
BUG=
==========

to

==========
Adds config component to Machine Provider
R=maruel@chromium.org
CC=smut@chromium.org
BUG=627558
==========

[M-A Ruel, 2016-08-09 19:22:27]

Description was changed from

==========
Adds config component to Machine Provider
R=maruel@chromium.org
CC=smut@chromium.org
BUG=627558
==========

to

==========
Adds config component to Machine Provider

R=maruel@chromium.org
CC=smut@chromium.org
BUG=636040
==========

[M-A Ruel, 2016-08-09 19:22:41]

looks fine, the only blocker is the commented out lines in cron.yaml.

Added reference to a new bug on crbug.com.

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
File appengine/machine_provider/cron.yaml (right):

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
appengine/machine_provider/cron.yaml:13: #- description: read luci-config
configs
why commend out the description?

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
File appengine/machine_provider/main.py (right):

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
appengine/machine_provider/main.py:18: utils.set_task_queue_module('default')
I'd prefer, while at it, to refactor this file into a proper function call:

def is_enabled_callback():
  return config.settings().enable_ts_monitoring


def main():
  utils.set_task_queue_module('default')
  c = handlers_cron.create_cron_app()
  e = handlers_endpoints.create_endpoints_app()
  f = handlers_frontend.create_frontend_app()
  q = handlers_queues.create_queues_app()
  for app in (c, e, f, q):
    gae_ts_mon.initialize(app=app, is_enabled_fn=is_enabled_callback)
  return c, e, f, q


cron_app, endpoints_app, frontend_app, queue_app = main()


or something like that, but that's optional.

[ryanmartens, 2016-08-09 21:15:39]

On 2016/08/09 19:22:41, M-A Ruel wrote:
> looks fine, the only blocker is the commented out lines in cron.yaml.
> 
> Added reference to a new bug on http://crbug.com.
> 
>
https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
> File appengine/machine_provider/cron.yaml (right):
> 
>
https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
> appengine/machine_provider/cron.yaml:13: #- description: read luci-config
> configs
> why commend out the description?
> 
>
https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
> File appengine/machine_provider/main.py (right):
> 
>
https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
> appengine/machine_provider/main.py:18: utils.set_task_queue_module('default')
> I'd prefer, while at it, to refactor this file into a proper function call:
> 
> def is_enabled_callback():
>   return config.settings().enable_ts_monitoring
> 
> 
> def main():
>   utils.set_task_queue_module('default')
>   c = handlers_cron.create_cron_app()
>   e = handlers_endpoints.create_endpoints_app()
>   f = handlers_frontend.create_frontend_app()
>   q = handlers_queues.create_queues_app()
>   for app in (c, e, f, q):
>     gae_ts_mon.initialize(app=app, is_enabled_fn=is_enabled_callback)
>   return c, e, f, q
> 
> 
> cron_app, endpoints_app, frontend_app, queue_app = main()
> 
> 
> or something like that, but that's optional.

Suggestions implemented. Two notes:
1) I can't call gae_ts_mon.initialize on the endpoints app (which explains the
implementation).
2) I removed the default value of True and plan to manage it through the config
changes (to be submitted separately).

[ryanmartens, 2016-08-09 21:16:31]

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
File appengine/machine_provider/cron.yaml (right):

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
appengine/machine_provider/cron.yaml:13: #- description: read luci-config
configs
On 2016/08/09 19:22:41, M-A Ruel wrote:
> why commend out the description?

Done.

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
File appengine/machine_provider/main.py (right):

https://codereview.chromium.org/2225263004/diff/1/appengine/machine_provider/...
appengine/machine_provider/main.py:18: utils.set_task_queue_module('default')
On 2016/08/09 19:22:41, M-A Ruel wrote:
> I'd prefer, while at it, to refactor this file into a proper function call:
> 
> def is_enabled_callback():
>   return config.settings().enable_ts_monitoring
> 
> 
> def main():
>   utils.set_task_queue_module('default')
>   c = handlers_cron.create_cron_app()
>   e = handlers_endpoints.create_endpoints_app()
>   f = handlers_frontend.create_frontend_app()
>   q = handlers_queues.create_queues_app()
>   for app in (c, e, f, q):
>     gae_ts_mon.initialize(app=app, is_enabled_fn=is_enabled_callback)
>   return c, e, f, q
> 
> 
> cron_app, endpoints_app, frontend_app, queue_app = main()
> 
> 
> or something like that, but that's optional.

Agree that we should refactor this. What are your thoughts on a tuple instead?

[M-A Ruel, 2016-08-09 21:18:52]

lgtm

tuple is fine.

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
File appengine/machine_provider/cron.yaml (right):

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
appengine/machine_provider/cron.yaml:12: ## luci-config
you can remove this line, it's clear from the description.

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
File appengine/machine_provider/handlers_endpoints.py (right):

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
appengine/machine_provider/handlers_endpoints.py:533: api =
endpoints.api_server([
I think it's fine to return directly instead of creating a local named variable.

[ryanmartens, 2016-08-10 12:12:14]

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
File appengine/machine_provider/cron.yaml (right):

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
appengine/machine_provider/cron.yaml:12: ## luci-config
On 2016/08/09 21:18:52, M-A Ruel wrote:
> you can remove this line, it's clear from the description.

Done.

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
File appengine/machine_provider/handlers_endpoints.py (right):

https://codereview.chromium.org/2225263004/diff/20001/appengine/machine_provi...
appengine/machine_provider/handlers_endpoints.py:533: api =
endpoints.api_server([
On 2016/08/09 21:18:52, M-A Ruel wrote:
> I think it's fine to return directly instead of creating a local named
variable.

Done.

[ryanmartens, 2016-08-10 12:40:17]

The CQ bit was checked by ryanmartens@google.com

[ryanmartens, 2016-08-10 12:40:18]

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

[commit-bot: I haz the power, 2016-08-10 12:40:25]

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-08-10 12:42:42]

Description was changed from

==========
Adds config component to Machine Provider

R=maruel@chromium.org
CC=smut@chromium.org
BUG=636040
==========

to

==========
Adds config component to Machine Provider

R=maruel@chromium.org
CC=smut@chromium.org
BUG=636040

Committed:
https://github.com/luci/luci-py/commit/9ecf78cae274e26e9ea68b2dca66e60a5f834c6e
==========

[commit-bot: I haz the power, 2016-08-10 12:42:43]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/9ecf78cae274e26e9ea68b2dca66e60a5f834c6e