Make the recipe roller tryjob accept tryjobs.

Make the recipe roller tryjob accept tryjobs.

R=estaab@chromium.org,iannucci@chromium.org,phajdan.jr@chromium.org
BUG=594265
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299456

[martiniss, 2016-03-18 23:43:13]

PTAL

Turns out there's a `patch` argument to bot_update which mostly does what I
want, so don't need the whole sentinel thing I was talking to you guys about.

https://codereview.chromium.org/1812123002/diff/1/scripts/slave/recipes/infra...
File scripts/slave/recipes/infra/recipe_roll_tryjob.expected/deps.json (left):

https://codereview.chromium.org/1812123002/diff/1/scripts/slave/recipes/infra...
scripts/slave/recipes/infra/recipe_roll_tryjob.expected/deps.json:188: "-O",
This was wrong before... whoops. Yay expectations :/

[Paweł Hajdan Jr., 2016-03-19 00:22:26]

LGTM

[iannucci, 2016-03-23 19:08:40]

lgtm

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
File scripts/slave/recipes/infra/recipe_roll_tryjob.py (right):

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:187: def parse_patches(api,
patches_raw, rietveld, issue, patchset, patch_project):
hm.. this seems a bit awkward of an API to me. What if you had a different
function 'extract_patches_from_rietveld', and then fed the result of that into
parse_patches?

And I still maintain that 'patches_raw' should be a real json dictionary instead
of this string thing... what was the reason for not doing that again?

I would also consider having the rietveld syntax look like:

DOWNSTREAM_PATCH=recipes:<project_name>:<url>
DOWNSTREAM_PATCH=recipes:<project_name2>:<url>

Instead of cramming it all into one line. I'd also make sure that rietveld's
url-finder regex can correctly link the url portions as well.

The 'recipes' literal would allow us to use this syntax and roller mechanism for
applying other sorts of patches too (e.g. be able to have a roller which tries
to roll generic upstream changes downstream). Something like
`DOWNSTREAM_PATCH=DEPS:chromium_src:<url>`. The rollers would need to
disambiguate the type of dependency (recipes.cfg, DEPS, submodules, repo, etc.)
in order to know how to update the downstream patch with the roll information
(bumping the pinned version).

Just a thought.

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:335: help="The code review
site the tryjob patch isfrom"),
This is very rietveld specific: "The Rietveld instance the issue is from"

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:337: help="The issue number
this patch is from"),
The Rietveld issue number to pull data from.

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:339: help="The patchset to
apply from this patch"),
The patchset number for the supplied issue.

Though honestly why can't we just have it be one url?

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:342: help="The luci-config
name of the project this patch belongs to"),
Isn't this retrievable from rietveld directly?

[martiniss, 2016-03-23 20:38:43]

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
File scripts/slave/recipes/infra/recipe_roll_tryjob.py (right):

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:187: def parse_patches(api,
patches_raw, rietveld, issue, patchset, patch_project):
On 2016/03/23 at 19:08:40, iannucci wrote:
> hm.. this seems a bit awkward of an API to me. What if you had a different
function 'extract_patches_from_rietveld', and then fed the result of that into
parse_patches?
> 
> And I still maintain that 'patches_raw' should be a real json dictionary
instead of this string thing... what was the reason for not doing that again?
> 
> I would also consider having the rietveld syntax look like:
> 
> DOWNSTREAM_PATCH=recipes:<project_name>:<url>
> DOWNSTREAM_PATCH=recipes:<project_name2>:<url>
Looks great!

This syntax is only for command line invocations. I figured that people wouldn't
want to do something like patches='[{\"project\": \"build\", ......' on the
command line. It's a bit ugly but it does work. I think eventually it'd be
mostly replaced by just passing in one patch on the command line, and having
this look at the downstream patches and do smart things™ from there.
> 
> Instead of cramming it all into one line. I'd also make sure that rietveld's
url-finder regex can correctly link the url portions as well.
> 
> The 'recipes' literal would allow us to use this syntax and roller mechanism
for applying other sorts of patches too (e.g. be able to have a roller which
tries to roll generic upstream changes downstream). Something like
`DOWNSTREAM_PATCH=DEPS:chromium_src:<url>`. The rollers would need to
disambiguate the type of dependency (recipes.cfg, DEPS, submodules, repo, etc.)
in order to know how to update the downstream patch with the roll information
(bumping the pinned version).
> 
> Just a thought.

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:335: help="The code review
site the tryjob patch isfrom"),
On 2016/03/23 at 19:08:40, iannucci wrote:
> This is very rietveld specific: "The Rietveld instance the issue is from"

Done.

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:337: help="The issue number
this patch is from"),
On 2016/03/23 at 19:08:40, iannucci wrote:
> The Rietveld issue number to pull data from.

Done.

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:339: help="The patchset to
apply from this patch"),
On 2016/03/23 at 19:08:40, iannucci wrote:
> The patchset number for the supplied issue.
> 
> Though honestly why can't we just have it be one url?

Done, and because CQ sets these properties when they send a tryjob, so we need
to accept them... AFAIK

https://codereview.chromium.org/1812123002/diff/60001/scripts/slave/recipes/i...
scripts/slave/recipes/infra/recipe_roll_tryjob.py:342: help="The luci-config
name of the project this patch belongs to"),
On 2016/03/23 at 19:08:40, iannucci wrote:
> Isn't this retrievable from rietveld directly?

Probably, yeah. It's set by CQ though, so I figure we might as well use it.

[martiniss, 2016-03-23 20:38:44]

The CQ bit was checked by martiniss@chromium.org

[martiniss, 2016-03-23 20:38:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1812123002/#ps60001
(title: "Documentation.")

[commit-bot: I haz the power, 2016-03-23 20:38:50]

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

[commit-bot: I haz the power, 2016-03-23 20:46:00]

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

[commit-bot: I haz the power, 2016-03-23 20:46:01]

Try jobs failed on following builders:
  Build Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)

[martiniss, 2016-03-23 23:49:59]

The CQ bit was checked by martiniss@chromium.org

[martiniss, 2016-03-23 23:50:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1812123002/#ps80001
(title: "Whoops forgot files, and expectations.")

[commit-bot: I haz the power, 2016-03-23 23:50:03]

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

[commit-bot: I haz the power, 2016-03-23 23:54:21]

Description was changed from

==========
Make the recipe roller tryjob accept tryjobs.

R=estaab@chromium.org,iannucci@chromium.org,phajdan.jr@chromium.org
BUG=594265
==========

to

==========
Make the recipe roller tryjob accept tryjobs.

R=estaab@chromium.org,iannucci@chromium.org,phajdan.jr@chromium.org
BUG=594265

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

[commit-bot: I haz the power, 2016-03-23 23:54:22]

Committed patchset #5 (id:80001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299456