Service account module

Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a locally stored secret.

R=nodir,mlliu
BUG=479187
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300478

[RobertoCN, 2016-05-05 19:47:08]

Description was changed from

==========
Service account module

Splitting this from the still pending buildbucket module.

R=nodir
==========

to

==========
Service account module

Splitting this from the still pending buildbucket module.

R=nodir
==========

[RobertoCN, 2016-05-05 19:47:08]

robertocn@chromium.org changed reviewers:
+ mlliu@chromium.org

[RobertoCN, 2016-05-05 19:48:15]

Description was changed from

==========
Service account module

Splitting this from the still pending buildbucket module.

R=nodir
==========

to

==========
Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a
locally stored secret.

R=nodir,mlliu
==========

[RobertoCN, 2016-05-06 18:38:36]

Description was changed from

==========
Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a
locally stored secret.

R=nodir,mlliu
==========

to

==========
Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a
locally stored secret.

R=nodir,mlliu
BUG=479187
==========

[RobertoCN, 2016-05-06 18:38:45]

PTAL

[nodir, 2016-05-06 19:41:56]

On 2016/05/06 18:38:45, RobertoCN wrote:
> PTAL

Is code same?

[RobertoCN, 2016-05-06 19:43:28]

On 2016/05/06 19:41:56, nodir wrote:
> On 2016/05/06 18:38:45, RobertoCN wrote:
> > PTAL
> 
> Is code same?

Yes, I just added example.py for expectations coverage.

[nodir, 2016-05-06 20:32:09]

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/api.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/api.py:36: print('The authutil
binary was not found at the default locations, '
recipe modules and recipes should no print directly to stdout, but use
StepPresentation.
ex.result.presentation.logs['blahblah'] = [...lines...]
where ex is StepFailure
docs: https://github.com/luci/recipes-py/blob/master/doc/user_guide.md#logging

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/config.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
why AUTHUTILPATH is not read on windows?

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/example.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/example.py:15: 
two blank lines between top level declarations

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/example.py:25: 
this is too many blank lines here

[nodir, 2016-05-06 20:38:01]

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/config.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
On 2016/05/06 20:32:09, nodir wrote:
> why AUTHUTILPATH is not read on windows?

apparently we should not use env in recipes because it makes recipes
non-hermetic, so please remove dep on env below

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
also please add a TODO that authutil should be installed via cipd

[RobertoCN, 2016-05-06 21:38:11]

Addressed all comments, please take a look.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/api.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/api.py:36: print('The authutil
binary was not found at the default locations, '
On 2016/05/06 20:32:09, nodir wrote:
> recipe modules and recipes should no print directly to stdout, but use
> StepPresentation.
> ex.result.presentation.logs['blahblah'] = [...lines...]
> where ex is StepFailure
> docs: https://github.com/luci/recipes-py/blob/master/doc/user_guide.md#logging

Done.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/config.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
Done.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
On 2016/05/06 20:32:09, nodir wrote:
> why AUTHUTILPATH is not read on windows?

Removed the reading of the env variable completely.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/config.py:22: c.authutil_path =
'C:\\infra-tools\\authutil.exe'
On 2016/05/06 20:38:01, nodir wrote:
> On 2016/05/06 20:32:09, nodir wrote:
> > why AUTHUTILPATH is not read on windows?
> 
> apparently we should not use env in recipes because it makes recipes
> non-hermetic, so please remove dep on env below

Done.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/service_account/example.py (right):

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/example.py:15: 
On 2016/05/06 20:32:09, nodir wrote:
> two blank lines between top level declarations

Done.

https://codereview.chromium.org/1954753003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/service_account/example.py:25: 
On 2016/05/06 20:32:09, nodir wrote:
> this is too many blank lines here

Done.

[nodir, 2016-05-06 21:49:36]

lgtm

https://codereview.chromium.org/1954753003/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/service_account/__init__.py (right):

https://codereview.chromium.org/1954753003/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/service_account/__init__.py:3:
'recipe_engine/path',
sort

[RobertoCN, 2016-05-06 22:12:14]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-05-06 22:12:15]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1954753003/#ps60001
(title: "Sorting deps.")

[commit-bot: I haz the power, 2016-05-06 22:12:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954753003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954753003/60001

[RobertoCN, 2016-05-06 22:12:53]

On 2016/05/06 21:49:36, nodir wrote:
> lgtm
> 
>
https://codereview.chromium.org/1954753003/diff/40001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/service_account/__init__.py (right):
> 
>
https://codereview.chromium.org/1954753003/diff/40001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/service_account/__init__.py:3:
> 'recipe_engine/path',
> sort

done.

[commit-bot: I haz the power, 2016-05-06 22:17:37]

Description was changed from

==========
Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a
locally stored secret.

R=nodir,mlliu
BUG=479187
==========

to

==========
Service account module

Splitting this from the still pending buildbucket module.

This module encapsulates the call to generate an authentication token from a
locally stored secret.

R=nodir,mlliu
BUG=479187

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300478
==========

[commit-bot: I haz the power, 2016-05-06 22:17:38]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=300478

[nodir, 2016-05-07 00:22:51]

https://codereview.chromium.org/1954753003/diff/60001/scripts/slave/recipe_mo...
File
scripts/slave/recipe_modules/service_account/example.expected/service_account_win.json
(right):

https://codereview.chromium.org/1954753003/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/service_account/example.expected/service_account_win.json:9:
"stdout": "/path/to/tmp/"
why does it say "/path/to/tmp" in stdout?
are you sure logs won't contain the token?