git cl try: support multiple bots from different masters without specifying master.

git cl try: support multiple bots from different masters without specifying master.

It might be good to make this change after the refactoring CL http://crrev.com/2442153002.

BUG=640740
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/123a468d14c58e78685f4d2bf166b4c285870f2d

[qyearsley, 2016-10-22 20:14:21]

Description was changed from

==========
Change GetBuilderMaster to return a dict of masters to builders.

BUG=
==========

to

==========
Change GetBuilderMaster to return a dict of masters to builders.

The purpose of this CL is to make it so that `git cl try` can be invoked with
multiple bots on different masters, without specifying master or bucket name.

It might be good to commit this after the refactoring CL
http://crbug.com/2442153002

BUG=640740
==========

[qyearsley, 2016-10-22 20:14:21]

qyearsley@chromium.org changed reviewers:
+ tandrii@chromium.org

[qyearsley, 2016-10-22 20:14:50]

Description was changed from

==========
Change GetBuilderMaster to return a dict of masters to builders.

The purpose of this CL is to make it so that `git cl try` can be invoked with
multiple bots on different masters, without specifying master or bucket name.

It might be good to commit this after the refactoring CL
http://crbug.com/2442153002

BUG=640740
==========

to

==========
Change GetBuilderMaster to return a dict of masters to builders.

The purpose of this CL is to make it so that `git cl try` can be invoked with
multiple bots on different masters, without specifying master or bucket name.

It might be good to commit this after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

[qyearsley, 2016-10-22 20:15:59]

Description was changed from

==========
Change GetBuilderMaster to return a dict of masters to builders.

The purpose of this CL is to make it so that `git cl try` can be invoked with
multiple bots on different masters, without specifying master or bucket name.

It might be good to commit this after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

to

==========
git cl try: support multiple bots from different masters without specifying
master.

It might be good to apply this after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

[qyearsley, 2016-10-22 20:16:16]

Description was changed from

==========
git cl try: support multiple bots from different masters without specifying
master.

It might be good to apply this after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

to

==========
git cl try: support multiple bots from different masters without specifying
master.

It might be good to make this change after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

[tandrii(chromium), 2016-10-23 17:00:02]

I already approved your refactoring change, so I expect you'd be rebasing this
one, but approach so far SGTM.

That said, consider adding a test mocking out urlfetch to make sure codepath of
2+ masters is regression tested.

https://codereview.chromium.org/2439293002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/20001/git_cl.py#newcode4695
git_cl.py:4695: return None, ('Invalid json string from %s. Error: %s.' %
(map_url, e))
this Go-style error reporting makes me sad in Python every time i look :(

[qyearsley, 2016-10-24 21:33:39]

Now updated, although this CL still doesn't contain a unit test.

https://codereview.chromium.org/2439293002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/20001/git_cl.py#newcode4695
git_cl.py:4695: return None, ('Invalid json string from %s. Error: %s.' %
(map_url, e))
On 2016/10/23 at 17:00:02, tandrii(chromium) wrote:
> this Go-style error reporting makes me sad in Python every time i look :(

Yeah -- I guess another way to do it is to raise an Exception and return a
single value; although that's not necessarily much easier to read, and it's not
clear what Exception should be raised.

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py#newcode399
git_cl.py:399: new_style = filter(lambda x: isinstance(x, tuple), options.bot)
One thing I'm not sure about -- are the elements in options.bot ever tuples? How
is one supposed to specify specify new-style bots with tests?

[tandrii(chromium), 2016-10-25 17:59:12]

tandrii@chromium.org changed reviewers:
+ machenbach@chromium.org

[tandrii(chromium), 2016-10-25 17:59:12]

Michael, PTAL

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py#newcode399
git_cl.py:399: new_style = filter(lambda x: isinstance(x, tuple), options.bot)
On 2016/10/24 21:33:39, qyearsley wrote:
> One thing I'm not sure about -- are the elements in options.bot ever tuples?
How
> is one supposed to specify specify new-style bots with tests?

Very good question. I don't see how either.
+machenbach@

[tandrii(chromium), 2016-10-25 18:01:38]

as for this CL itself -> SGTM % please add regression test

[qyearsley, 2016-10-25 21:04:47]

On 2016/10/25 at 18:01:38, tandrii wrote:
> as for this CL itself -> SGTM % please add regression test

Alright -- added a test, could you take a look?

Also, if bots/tests are not in fact specified in new style and options.bot is
never a list with tuples, then this could perhaps be simplified further,
although perhaps that could be done in a separate CL.

[Michael Achenbach, 2016-10-26 08:36:40]

lgtm with suggestions:

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py#newcode391
git_cl.py:391: buckets[_prefix_master(master)] = {b: [] for b in builders}
How about inlining this in the _get_master_map_for_builders as the raw
master_map value isn't used for anything else?

Below you could use _prefix_master(master) as key in master_map and e.g.
directly write:

master_map.setdefault(_prefix_master(masters[0]), {})[builder] = []

Then you could just return master_map here.

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py#newcode439
git_cl.py:439: else:
nit: No else after returns required.

The code below could be written (or see other suggestion above):

master_map.setdefault(masters[0], []).append(builder)

[tandrii(chromium), 2016-10-26 09:55:01]

lgtm + thanks for tests % consider Michael's suggestions.

https://codereview.chromium.org/2439293002/diff/60001/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/2439293002/diff/60001/tests/git_cl_test.py#ne...
tests/git_cl_test.py:2054: json.dumps({"builder1": ["master1"], "builder2":
["master2"]})))
nit: single quotes.

[Michael Achenbach, 2016-10-26 10:17:47]

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/40001/git_cl.py#newcode399
git_cl.py:399: new_style = filter(lambda x: isinstance(x, tuple), options.bot)
On 2016/10/25 17:59:12, tandrii(chromium) wrote:
> On 2016/10/24 21:33:39, qyearsley wrote:
> > One thing I'm not sure about -- are the elements in options.bot ever tuples?
> How
> > is one supposed to specify specify new-style bots with tests?
> 
> Very good question. I don't see how either.
> +machenbach@

I have no idea. I just got involved here at some point on demand. By now I
forgot all about it. I'd need to dig in the code and find out.

I think a bunch of old assumptions and requirements might not hold anymore.

I'm happy as long as this still supports passing -p properties to buildbucket.

[qyearsley, 2016-10-26 16:08:42]

On 2016/10/26 at 10:17:47, machenbach wrote:
> https://codereview.chromium.org/2439293002/diff/40001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/2439293002/diff/40001/git_cl.py#newcode399
> git_cl.py:399: new_style = filter(lambda x: isinstance(x, tuple), options.bot)
> On 2016/10/25 17:59:12, tandrii(chromium) wrote:
> > On 2016/10/24 21:33:39, qyearsley wrote:
> > > One thing I'm not sure about -- are the elements in options.bot ever
tuples?
> > How
> > > is one supposed to specify specify new-style bots with tests?
> > 
> > Very good question. I don't see how either.
> > +machenbach@
> 
> I have no idea. I just got involved here at some point on demand. By now I
forgot all about it. I'd need to dig in the code and find out.
> 
> I think a bunch of old assumptions and requirements might not hold anymore.
> 
> I'm happy as long as this still supports passing -p properties to buildbucket.

Makes sense; I added a TODO to look into this and possibly simplify it.

[qyearsley, 2016-10-26 16:08:47]

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py#newcode391
git_cl.py:391: buckets[_prefix_master(master)] = {b: [] for b in builders}
On 2016/10/26 at 08:36:40, machenbach (slow) wrote:
> How about inlining this in the _get_master_map_for_builders as the raw
master_map value isn't used for anything else?
> 
> Below you could use _prefix_master(master) as key in master_map and e.g.
directly write:
> 
> master_map.setdefault(_prefix_master(masters[0]), {})[builder] = []
> 
> Then you could just return master_map here.

Makes sense; done

https://codereview.chromium.org/2439293002/diff/60001/git_cl.py#newcode439
git_cl.py:439: else:
On 2016/10/26 at 08:36:40, machenbach (slow) wrote:
> nit: No else after returns required.
> 
> The code below could be written (or see other suggestion above):
> 
> master_map.setdefault(masters[0], []).append(builder)

Done; also, renamed "master_map" to "bucket_map" to reflect that the keys are
now bucket names (with the "master." prefix).

https://codereview.chromium.org/2439293002/diff/60001/tests/git_cl_test.py
File tests/git_cl_test.py (right):

https://codereview.chromium.org/2439293002/diff/60001/tests/git_cl_test.py#ne...
tests/git_cl_test.py:2054: json.dumps({"builder1": ["master1"], "builder2":
["master2"]})))
On 2016/10/26 at 09:55:01, tandrii(chromium) wrote:
> nit: single quotes.

Done

[qyearsley, 2016-10-26 16:08:50]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-10-26 16:08:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
tandrii@chromium.org, machenbach@chromium.org
Link to the patchset: https://codereview.chromium.org/2439293002/#ps80001
(title: "Respond to suggestions, add TODO")

[commit-bot: I haz the power, 2016-10-26 16:09:03]

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-26 16:12:21]

Description was changed from

==========
git cl try: support multiple bots from different masters without specifying
master.

It might be good to make this change after the refactoring CL
http://crrev.com/2442153002.

BUG=640740
==========

to

==========
git cl try: support multiple bots from different masters without specifying
master.

It might be good to make this change after the refactoring CL
http://crrev.com/2442153002.

BUG=640740

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/123a468d14c58e...
==========

[commit-bot: I haz the power, 2016-10-26 16:12:22]

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