bot_update: understand new Gerrit patch properties.

bot_update: understand new Gerrit patch properties.

This change is backwards compatible, so no expectation changes expected
in downstream rolls.

Depends on https://codereview.chromium.org/2442173003 recipe_engine
change.

BUG=645616
R=martiniss@chromium.org,machenbach@chromium.org

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/51a7b096ca3e9a16e175794de001f9f04e066839

[tandrii(chromium), 2016-10-24 00:44:55]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 00:45:05]

Dry run: 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-10-24 00:51:35]

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

[commit-bot: I haz the power, 2016-10-24 00:51:37]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/320cf79617f7be10)

[tandrii(chromium), 2016-10-24 01:15:37]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 01:15:39]

Dry run: 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-10-24 01:21:34]

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

[commit-bot: I haz the power, 2016-10-24 01:21:35]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/320d1392d4796710)

[tandrii(chromium), 2016-10-24 01:28:55]

The CQ bit was checked by tandrii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 01:29:00]

Dry run: 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-10-24 01:36:45]

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

[commit-bot: I haz the power, 2016-10-24 01:36:45]

Dry run: Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/320d235bf1a22110)

[Michael Achenbach, 2016-10-24 08:18:13]

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/__init__.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/__init__.py:20: 'patchset': Property(default=None),
Maybe leave a comment here about the difference between the two patch*sets

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/__init__.py:25: 'event.patchSet.ref':
Property(default=None, param_name='gerrit_ref'),
nit: how about some alpha or other sorting of keys (e.g. rietveld/gerrit/rest)?

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:13: def __init__(self, issue, patch_issue,
patchset, patch_set, patch_project,
Sure this isn't called anywhere with args? Maybe to be safe, put new keywords in
the end with defaults? And land a new sorting as secondary refactoring CL?
(unless you're sure)

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:18: self._patchset = patchset or patch_set
Are these strings or integers? I assume 0 can't ever be a valid number, right?

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:44: if self.m.properties.get('patch_storage') ==
'gerrit':
Maybe unrelated: Why is bot_update specifying properties in __init__ and
tryserver is not?

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/example.expected/with_gerrit_patch.json (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/example.expected/with_gerrit_patch.json:1: [
Are the two expectation files useful? Not sure what I should look for here.
Thought I'd find traces of patch application.

[tandrii(chromium), 2016-10-24 11:43:37]

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/__init__.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/__init__.py:20: 'patchset': Property(default=None),
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> Maybe leave a comment here about the difference between the two patch*sets

Refactored this code. Yesterday I believed the order is the same as the order in
constructor of API object. Of course not. So, I can re-order at will.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/__init__.py:25: 'event.patchSet.ref':
Property(default=None, param_name='gerrit_ref'),
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> nit: how about some alpha or other sorting of keys (e.g.
rietveld/gerrit/rest)?

Done.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:13: def __init__(self, issue, patch_issue,
patchset, patch_set, patch_project,
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> Sure this isn't called anywhere with args? Maybe to be safe, put new keywords
in
> the end with defaults? And land a new sorting as secondary refactoring CL?
> (unless you're sure)

I wasn't just like you, but I am now after reading the recipe_engine source.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:18: self._patchset = patchset or patch_set
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> Are these strings or integers? I assume 0 can't ever be a valid number, right?

strings for now.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:109: patchset = patchset or self._patchset
same here - strings.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/api.py (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/api.py:44: if self.m.properties.get('patch_storage') ==
'gerrit':
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> Maybe unrelated: Why is bot_update specifying properties in __init__ and
> tryserver is not?

Basically, because nobody refactored it. That recipe module has to be cleaned
up, and maybe gerrit and rietveld infra-specific properties moved there instead
of in recipe_engine.

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/example.expected/with_gerrit_patch.json (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/example.expected/with_gerrit_patch.json:1: [
On 2016/10/24 08:18:13, machenbach (slow) wrote:
> Are the two expectation files useful? Not sure what I should look for here.
> Thought I'd find traces of patch application.

The deprecated one is to be removed once all users migrated to new version.

[Michael Achenbach, 2016-10-24 11:57:50]

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/example.expected/with_gerrit_patch.json (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/example.expected/with_gerrit_patch.json:1: [
On 2016/10/24 11:43:37, tandrii(chromium) wrote:
> On 2016/10/24 08:18:13, machenbach (slow) wrote:
> > Are the two expectation files useful? Not sure what I should look for here.
> > Thought I'd find traces of patch application.
> 
> The deprecated one is to be removed once all users migrated to new version.

This doesn't fully answer my question. I was comparing this file to e.g.:
https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/tryserver/e...
which has a step for applyting a patch. Shouldn't be such a step here?

[tandrii(chromium), 2016-10-24 12:03:54]

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
File recipe_modules/tryserver/example.expected/with_gerrit_patch.json (right):

https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
recipe_modules/tryserver/example.expected/with_gerrit_patch.json:1: [
On 2016/10/24 11:57:49, machenbach (slow) wrote:
> On 2016/10/24 11:43:37, tandrii(chromium) wrote:
> > On 2016/10/24 08:18:13, machenbach (slow) wrote:
> > > Are the two expectation files useful? Not sure what I should look for
here.
> > > Thought I'd find traces of patch application.
> > 
> > The deprecated one is to be removed once all users migrated to new version.
> 
> This doesn't fully answer my question. I was comparing this file to e.g.:
>
https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/tryserver/e...
> which has a step for applyting a patch. Shouldn't be such a step here?

Ah, I see now. Logically, there should be, but apply patch hasn't been
implemented here for Gerrit, and that's what I was trying to indicate with this
expectation.

[Michael Achenbach, 2016-10-24 12:10:25]

On 2016/10/24 12:03:54, tandrii(chromium) wrote:
>
https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
> File recipe_modules/tryserver/example.expected/with_gerrit_patch.json (right):
> 
>
https://codereview.chromium.org/2439373002/diff/40001/recipe_modules/tryserve...
> recipe_modules/tryserver/example.expected/with_gerrit_patch.json:1: [
> On 2016/10/24 11:57:49, machenbach (slow) wrote:
> > On 2016/10/24 11:43:37, tandrii(chromium) wrote:
> > > On 2016/10/24 08:18:13, machenbach (slow) wrote:
> > > > Are the two expectation files useful? Not sure what I should look for
> here.
> > > > Thought I'd find traces of patch application.
> > > 
> > > The deprecated one is to be removed once all users migrated to new
version.
> > 
> > This doesn't fully answer my question. I was comparing this file to e.g.:
> >
>
https://cs.chromium.org/chromium/tools/depot_tools/recipe_modules/tryserver/e...
> > which has a step for applyting a patch. Shouldn't be such a step here?
> 
> Ah, I see now. Logically, there should be, but apply patch hasn't been
> implemented here for Gerrit, and that's what I was trying to indicate with
this
> expectation.

IC lgtm then.

[tandrii(chromium), 2016-10-25 13:10:34]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2016-10-25 13:10:45]

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-10-25 13:16:11]

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

[commit-bot: I haz the power, 2016-10-25 13:16:13]

Try jobs failed on following builders:
  Depot Tools Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3214c89102965910)

[tandrii(chromium), 2016-10-25 13:35:06]

The CQ bit was checked by tandrii@chromium.org

[tandrii(chromium), 2016-10-25 13:35:07]

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

[commit-bot: I haz the power, 2016-10-25 13:35:16]

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-10-25 13:38:30]

Description was changed from

==========
bot_update: understand new Gerrit patch properties.

This change is backwards compatible, so no expectation changes expected
in downstream rolls.

Depends on https://codereview.chromium.org/2442173003 recipe_engine
change.

BUG=645616
R=martiniss@chromium.org,machenbach@chromium.org
==========

to

==========
bot_update: understand new Gerrit patch properties.

This change is backwards compatible, so no expectation changes expected
in downstream rolls.

Depends on https://codereview.chromium.org/2442173003 recipe_engine
change.

BUG=645616
R=martiniss@chromium.org,machenbach@chromium.org

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/51a7b096ca3e9a...
==========

[commit-bot: I haz the power, 2016-10-25 13:38:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/51a7b096ca3e9a...