Support multiple try masters when sending tries to rietveld.

Support multiple try masters when sending tries to rietveld.

BUG=

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

[Michael Achenbach, 2014-02-27 00:06:21]

PTAL

This is probably not finished yet:
- all code is pretty much c/p inspired by the original code of
GetPreferedTrySlaves - might need some refactoring
- 'git cl try' lacks a unit test (looks like it never had one)
- command-line support for multiple masters is missing

[Paweł Hajdan Jr., 2014-02-27 02:20:24]

Some comments, I mostly focused on design and interfaces.

Thanks for working on this!

https://codereview.chromium.org/178223016/diff/50001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/178223016/diff/50001/git_cl.py#newcode2151
git_cl.py:2151: # multiple try masters yet.
Not only that, but it'll be required to provide some master name. Can we add an
option that would apply the same master name to all the bots specified?

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1104: function_info =
inspect.getargspec(get_preferred_try_masters)
Since this is new, can we standardize on one signature of that function?

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1112: def CheckType(item, t):
It's not obvious to me how much these type checks in python are actually useful.
I'm fine with them, but please consider removing them.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1134: if isinstance(builders, dict):
Similarly, since this is new, can we standardize on specifying both builders and
tests? A dummy value for tests can always be specified.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1208: if isinstance(builders, list):
Why do we need to make a decision based on type here?

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1215: """Merges masters into results.
It's not obvious to me what this does, could you clarify or ideally simplify it?

I could spend more time trying to understand it, but ideally I shouldn't.

https://codereview.chromium.org/178223016/diff/50001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/178223016/diff/50001/rietveld.py#newcode334
rietveld.py:334: self, issue, patchset, reason, clobber, revision, masters):
CQ relies on trigger_try_jobs working the way it worked before.

If you're changing semantics, please add a new method and deprecate this one.

[Michael Achenbach, 2014-02-27 18:35:22]

All done (git cl try test case still missing).

https://codereview.chromium.org/178223016/diff/50001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/178223016/diff/50001/git_cl.py#newcode2151
git_cl.py:2151: # multiple try masters yet.
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> Not only that, but it'll be required to provide some master name. Can we add
an
> option that would apply the same master name to all the bots specified?

I added a master option. But even better would be to specify the whole JSON
masters->builders->tests on the command line.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1104: function_info =
inspect.getargspec(get_preferred_try_masters)
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> Since this is new, can we standardize on one signature of that function?

Done.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1112: def CheckType(item, t):
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> It's not obvious to me how much these type checks in python are actually
useful.
> I'm fine with them, but please consider removing them.

If presubmit hooks return the wrong formats, it'd first be detected deeper down
the code and not be so obvious anymore. Would be nice if there were a more
declarative way to express the expected format.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1134: if isinstance(builders, dict):
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> Similarly, since this is new, can we standardize on specifying both builders
and
> tests? A dummy value for tests can always be specified.

Done.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1208: if isinstance(builders, list):
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> Why do we need to make a decision based on type here?

This is removed now, since now only dicts are allowed.

https://codereview.chromium.org/178223016/diff/50001/presubmit_support.py#new...
presubmit_support.py:1215: """Merges masters into results.
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> It's not obvious to me what this does, could you clarify or ideally simplify
it?
> 
> I could spend more time trying to understand it, but ideally I shouldn't.

I tried to simplify it and made it a function. The best would be to look at the
tests and check if they're complete.

https://codereview.chromium.org/178223016/diff/50001/rietveld.py
File rietveld.py (right):

https://codereview.chromium.org/178223016/diff/50001/rietveld.py#newcode334
rietveld.py:334: self, issue, patchset, reason, clobber, revision, masters):
On 2014/02/27 02:20:24, Paweł Hajdan Jr. wrote:
> CQ relies on trigger_try_jobs working the way it worked before.
> 
> If you're changing semantics, please add a new method and deprecate this one.

Done. As soon as CQ is going to call the new method, we might need to collect
the return values. I don't know if the CQ actually reads those.

[ghost stip (do not use), 2014-03-01 00:56:16]

some comments

https://chromiumcodereview.appspot.com/178223016/diff/70001/git_cl.py
File git_cl.py (right):

https://chromiumcodereview.appspot.com/178223016/diff/70001/git_cl.py#newcode...
git_cl.py:2170: # Return a master map with an empty master to be backwards
compatible.
make note that by default options.master is the empty string

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support...
presubmit_support.py:1095: exec script_text in context
if possible, try to refactor this out to share code with getprefeeredtryslaves.
ok for a separate cl

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support...
presubmit_support.py:1109: def CheckType(item, t):
I'm ok removing lines 1109 to 1141, since any code which doesn't have this
format will break

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support...
presubmit_support.py:1202: """
nit: keep """ on same line

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support...
presubmit_support.py:1234: output_stream.write("Warning, no presubmit.py
found.\n")
nit: PRESUBMIT.py vs presubmit.py

https://chromiumcodereview.appspot.com/178223016/diff/70001/presubmit_support...
presubmit_support.py:1254: for (_, builders) in results.iteritems():
for builders in results.itervalues()

https://chromiumcodereview.appspot.com/178223016/diff/70001/rietveld.py
File rietveld.py (right):

https://chromiumcodereview.appspot.com/178223016/diff/70001/rietveld.py#newco...
rietveld.py:334: self, issue, patchset, reason, clobber, revision,
builders_and_tests):
add optional master= argument, and have trigger_distributed_try_jobs loop over
this method

[Michael Achenbach, 2014-03-01 01:30:39]

All done. PTAL.

https://codereview.chromium.org/178223016/diff/70001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/178223016/diff/70001/git_cl.py#newcode2170
git_cl.py:2170: # Return a master map with an empty master to be backwards
compatible.
On 2014/03/01 00:56:16, stip wrote:
> make note that by default options.master is the empty string

Done.

https://codereview.chromium.org/178223016/diff/70001/presubmit_support.py
File presubmit_support.py (right):

https://codereview.chromium.org/178223016/diff/70001/presubmit_support.py#new...
presubmit_support.py:1095: exec script_text in context
On 2014/03/01 00:56:16, stip wrote:
> if possible, try to refactor this out to share code with
getprefeeredtryslaves.
> ok for a separate cl

Follow up CL.

https://codereview.chromium.org/178223016/diff/70001/presubmit_support.py#new...
presubmit_support.py:1109: def CheckType(item, t):
On 2014/03/01 00:56:16, stip wrote:
> I'm ok removing lines 1109 to 1141, since any code which doesn't have this
> format will break

Done.

https://codereview.chromium.org/178223016/diff/70001/presubmit_support.py#new...
presubmit_support.py:1234: output_stream.write("Warning, no presubmit.py
found.\n")
On 2014/03/01 00:56:16, stip wrote:
> nit: PRESUBMIT.py vs presubmit.py

Done.

https://codereview.chromium.org/178223016/diff/70001/presubmit_support.py#new...
presubmit_support.py:1254: for (_, builders) in results.iteritems():
On 2014/03/01 00:56:16, stip wrote:
> for builders in results.itervalues()

Done.

https://codereview.chromium.org/178223016/diff/90001/tests/presubmit_unittest.py
File tests/presubmit_unittest.py (right):

https://codereview.chromium.org/178223016/diff/90001/tests/presubmit_unittest...
tests/presubmit_unittest.py:1051: 
After removing the checks, there are no bad results anymore.

[ghost stip (do not use), 2014-03-01 01:36:52]

lgtm

[Paweł Hajdan Jr., 2014-03-01 01:50:02]

LGTM

[Michael Achenbach, 2014-03-01 02:06:09]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2014-03-01 02:06:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/machenbach@chromium.org/178223016/90001

[commit-bot: I haz the power, 2014-03-01 02:08:33]

Change committed as 254321