build: add recipe code for new autoroller

build: add recipe code for new autoroller

Depends on recipe engine CL https://codereview.chromium.org/1866313002

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

[Paweł Hajdan Jr., 2016-04-07 10:53:02]

phajdan.jr@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org, nodir@chromium.org

[martiniss, 2016-04-07 18:28:00]

Looks good overall.

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/luci_config/api.py:16: fetch_result =
self.m.step('Get project urls',
Please change this to use the fetch module. See
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
for an example of usage; if you could convert the recipe_roll_tryjob to use
this, that would be nice. I can do it later though.

This allows us to pass in auth tokens without leakage.

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/recipe_autoroller/api.py:9: def
prepare_checkout(self):
Why do we need this exactly?

[Paweł Hajdan Jr., 2016-04-07 20:32:16]

Thanks for review. Is there a reason why you didn't approve the CL?

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/luci_config/api.py:16: fetch_result =
self.m.step('Get project urls',
On 2016/04/07 at 18:27:59, martiniss wrote:
> Please change this to use the fetch module. See
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
for an example of usage; if you could convert the recipe_roll_tryjob to use
this, that would be nice. I can do it later though.

Would you be OK with a TODO? This is what recipe_roller currently uses, and it's
known to work.

> This allows us to pass in auth tokens without leakage.

Could you explain more? As far as I can tell, in neither case are we leaking any
auth tokens.

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/recipe_autoroller/api.py:9: def
prepare_checkout(self):
On 2016/04/07 at 18:27:59, martiniss wrote:
> Why do we need this exactly?

So that this logic is not duplicated between public and internal recipes.

If you have specific suggestion how to structure the code differently, feel free
to share.

[martiniss, 2016-04-07 21:27:12]

re: why I didn't lgtm before... I had comments which I wanted to hear your
response for before I was ok with the change landing. You addressed them though,
so lgtm now

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/luci_config/api.py:16: fetch_result =
self.m.step('Get project urls',
On 2016/04/07 at 20:32:15, Paweł Hajdan Jr. wrote:
> On 2016/04/07 at 18:27:59, martiniss wrote:
> > Please change this to use the fetch module. See
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
for an example of usage; if you could convert the recipe_roll_tryjob to use
this, that would be nice. I can do it later though.
> 
> Would you be OK with a TODO? This is what recipe_roller currently uses, and
it's known to work.

A TODO is fine for now.

> 
> > This allows us to pass in auth tokens without leakage.
> 
> Could you explain more? As far as I can tell, in neither case are we leaking
any auth tokens.

Internally we are. See
https://uberchromegw.corp.google.com/i/internal.infra.cron/builders/recipe-ro...
for example; it puts the raw auth token on the command line.

This is fine for here, since we aren't using the auth token.

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/recipe_autoroller/api.py:9: def
prepare_checkout(self):
On 2016/04/07 at 20:32:15, Paweł Hajdan Jr. wrote:
> On 2016/04/07 at 18:27:59, martiniss wrote:
> > Why do we need this exactly?
> 
> So that this logic is not duplicated between public and internal recipes.
> 
> If you have specific suggestion how to structure the code differently, feel
free to share.

I guess my question was more existential; I don't see why we need this checkout
exactly. But we're duplicating what we have already, so it looks fine to me.

[Paweł Hajdan Jr., 2016-04-08 08:47:00]

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

https://codereview.chromium.org/1862383002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/luci_config/api.py:16: fetch_result =
self.m.step('Get project urls',
On 2016/04/07 at 21:27:12, martiniss wrote:
> On 2016/04/07 at 20:32:15, Paweł Hajdan Jr. wrote:
> > On 2016/04/07 at 18:27:59, martiniss wrote:
> > > Please change this to use the fetch module. See
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
for an example of usage; if you could convert the recipe_roll_tryjob to use
this, that would be nice. I can do it later though.
> > 
> > Would you be OK with a TODO? This is what recipe_roller currently uses, and
it's known to work.
> 
> A TODO is fine for now.
> 
> > 
> > > This allows us to pass in auth tokens without leakage.
> > 
> > Could you explain more? As far as I can tell, in neither case are we leaking
any auth tokens.
> 
> Internally we are. See
https://uberchromegw.corp.google.com/i/internal.infra.cron/builders/recipe-ro...
for example; it puts the raw auth token on the command line.
> 
> This is fine for here, since we aren't using the auth token.

Good point. Added a TODO.

[Paweł Hajdan Jr., 2016-04-08 08:47:06]

The CQ bit was checked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2016-04-08 08:47:06]

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

[commit-bot: I haz the power, 2016-04-08 08:47:09]

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

[commit-bot: I haz the power, 2016-04-08 08:51:07]

Description was changed from

==========
build: add recipe code for new autoroller

Depends on recipe engine CL https://codereview.chromium.org/1866313002

BUG=chromium:583303
==========

to

==========
build: add recipe code for new autoroller

Depends on recipe engine CL https://codereview.chromium.org/1866313002

BUG=chromium:583303

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

[commit-bot: I haz the power, 2016-04-08 08:51:08]

Committed patchset #2 (id:20001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299796