Pass mb_mastername and mb_buildername as parameters to override the default.

Pass mb_mastername and mb_buildername as parameters to override the default mastername and buildername to run MB.

BUG=

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

[stgao, 2015-11-24 21:15:43]

Description was changed from

==========
Override the master and builder used to run MB.

Recipes used by Findit will have match the configuration based on two given
parameters "target_mastername" and "target_buildername" that identify the
continuous builder where compile failures occur.

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

to

==========
Override the master and builder used to run MB.

Recipes used by Findit will have to match the configuration based on two build
properties "target_mastername" and "target_buildername" that identify the
continuous builder where compile failures occur.

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

[stgao, 2015-11-24 23:20:36]

stgao@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[stgao, 2015-11-24 23:20:37]

Hi Pawel,

As you suggested, I separated this out from the original CL
https://codereview.chromium.org/1416763007/.

Do you mind a review?

Thanks,
Shuotao Gao

[Paweł Hajdan Jr., 2015-11-25 16:29:15]

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

https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:479: target_mastername =
(self.m.properties.get('target_mastername') or
Why not make these arguments of this method like I suggested?

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

https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/filter/api.py:163: # We need to use the actual
mastername and buildername we're running on,
I think it'd make sense to also plumb mastername and buildername here.

[Dirk Pranke, 2015-11-25 17:26:33]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2015-11-25 17:26:34]

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

https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium_tests/api.py:479: target_mastername =
(self.m.properties.get('target_mastername') or
On 2015/11/25 16:29:15, Paweł Hajdan Jr. wrote:
> Why not make these arguments of this method like I suggested?

I replied to that in the other CL. I don't think it makes sense to do so because
findit can be calling multiple different recipes that would all have to be aware
of this. It seems better to have this logic in one place (or two, given the
change to analyze).

[iannucci, 2015-11-25 17:34:36]

On 2015/11/25 at 17:26:34, dpranke wrote:
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> File scripts/slave/recipe_modules/chromium_tests/api.py (right):
> 
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> scripts/slave/recipe_modules/chromium_tests/api.py:479: target_mastername =
(self.m.properties.get('target_mastername') or
> On 2015/11/25 16:29:15, Paweł Hajdan Jr. wrote:
> > Why not make these arguments of this method like I suggested?
> 
> I replied to that in the other CL. I don't think it makes sense to do so
because findit can be calling multiple different recipes that would all have to
be aware of this. It seems better to have this logic in one place (or two, given
the change to analyze).

On cursory examination I agree with Pawel here (even though it will make this
change bigger). This sort of read-a-property-that's-named-such is currently a
large contributor to the SpaghettiFactor (tm) of this code base (action at a
distance based on some specially-named properties). But I'm not maintaining this
code, so I'll leave the final decision to those who are.

[Dirk Pranke, 2015-11-25 17:56:18]

On 2015/11/25 17:34:36, iannucci wrote:
> On 2015/11/25 at 17:26:34, dpranke wrote:
> >
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> > File scripts/slave/recipe_modules/chromium_tests/api.py (right):
> > 
> >
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> > scripts/slave/recipe_modules/chromium_tests/api.py:479: target_mastername =
> (self.m.properties.get('target_mastername') or
> > On 2015/11/25 16:29:15, Paweł Hajdan Jr. wrote:
> > > Why not make these arguments of this method like I suggested?
> > 
> > I replied to that in the other CL. I don't think it makes sense to do so
> because findit can be calling multiple different recipes that would all have
to
> be aware of this. It seems better to have this logic in one place (or two,
given
> the change to analyze).
> 
> On cursory examination I agree with Pawel here (even though it will make this
> change bigger). This sort of read-a-property-that's-named-such is currently a
> large contributor to the SpaghettiFactor (tm) of this code base (action at a
> distance based on some specially-named properties). But I'm not maintaining
this
> code, so I'll leave the final decision to those who are.

It's possible that I'm wrong. I haven't stared at the calling code enough to be
sure
what it'll look like, so I'm fine w/ making the change and we can see which
looks better.

[stgao, 2015-11-25 19:28:41]

On 2015/11/25 17:56:18, Dirk Pranke wrote:
> On 2015/11/25 17:34:36, iannucci wrote:
> > On 2015/11/25 at 17:26:34, dpranke wrote:
> > >
> >
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> > > File scripts/slave/recipe_modules/chromium_tests/api.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1474473004/diff/1/scripts/slave/recipe_module...
> > > scripts/slave/recipe_modules/chromium_tests/api.py:479: target_mastername
=
> > (self.m.properties.get('target_mastername') or
> > > On 2015/11/25 16:29:15, Paweł Hajdan Jr. wrote:
> > > > Why not make these arguments of this method like I suggested?
> > > 
> > > I replied to that in the other CL. I don't think it makes sense to do so
> > because findit can be calling multiple different recipes that would all have
> to
> > be aware of this. It seems better to have this logic in one place (or two,
> given
> > the change to analyze).
> > 
> > On cursory examination I agree with Pawel here (even though it will make
this
> > change bigger). This sort of read-a-property-that's-named-such is currently
a
> > large contributor to the SpaghettiFactor (tm) of this code base (action at a
> > distance based on some specially-named properties). But I'm not maintaining
> this
> > code, so I'll leave the final decision to those who are.
> 
> It's possible that I'm wrong. I haven't stared at the calling code enough to
be
> sure
> what it'll look like, so I'm fine w/ making the change and we can see which
> looks better.

This morning I had a chat with Pawel on this, and he suggested passing them in
as parameters would help to minimize chance of possible breakage or other
issues.
So I will make the change to pass the mb_mastername and mb_buildername from
recipes all the way down to calls to mb modules.
This will be a much bigger change I think, as recipes will have to be updated.

An alternative to consider is: land this small change as is, and then do the
refactoring in a follow-up CL. (I will skip this for now until you guys think
it's a better approach.)

[stgao, 2015-11-25 21:27:52]

Description was changed from

==========
Override the master and builder used to run MB.

Recipes used by Findit will have to match the configuration based on two build
properties "target_mastername" and "target_buildername" that identify the
continuous builder where compile failures occur.

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

to

==========
Refactor to pass mb_mastername and mb_buildername as parameters from recipes or
recipe modules.

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

[stgao, 2015-11-25 22:02:01]

Patchset #2 (id:20001) has been deleted

[stgao, 2015-11-25 22:04:12]

Patchset #2 (id:40001) has been deleted

[stgao, 2015-11-25 22:07:36]

Hi folks,

I've finished the refactoring to pass mb_mastername and mb_buildername as
parameters.
Does this look better?

Thanks,
Shuotao Gao

[Dirk Pranke, 2015-11-25 23:05:52]

I'm not sure that this is an improvement, but if Paweł and Robbie prefer it,
that's fine by me.

[stgao, 2015-11-26 19:25:00]

Hey, Paweł, what do you say about the new patch? Which approach should we go
with?

[Paweł Hajdan Jr., 2015-11-27 10:21:25]

Yup, the approach looks good to me.

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:360: test_spec,
mb_mastername, mb_buildername):
Can we make mb_ args optional and have them default to non-mb ones? Applies to
entire CL.

[stgao, 2015-11-28 07:37:49]

Hey Paweł, would you mind checking my reply to your comment below?

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:360: test_spec,
mb_mastername, mb_buildername):
On 2015/11/27 10:21:24, Paweł Hajdan Jr. wrote:
> Can we make mb_ args optional and have them default to non-mb ones? Applies to
> entire CL.

I think we can not do that, assuming that "non-mb ones" refer to the other two
parameters `mastername` and `buildername` here. (I might be wrong, so please let
me know why we could default to those two.)

Reason: the original code in function `run_mb_and_compile` explicitly said not
to use the mastername and buildername passed in (that code was refactored from
function `compile_specific_targets` at
https://chromium.googlesource.com/chromium/tools/build/+blame/b5a369456d211dc...).


However, we could default to api.properties['mastername'] and
api.properties['buildername'] instead within function`compile_specific_targets`,
if that is preferred.
What do you think?

[Paweł Hajdan Jr., 2015-11-30 12:47:25]

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/api.py (right):

https://codereview.chromium.org/1474473004/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/api.py:360: test_spec,
mb_mastername, mb_buildername):
On 2015/11/28 at 07:37:48, Shuotao wrote:
> On 2015/11/27 10:21:24, Paweł Hajdan Jr. wrote:
> > Can we make mb_ args optional and have them default to non-mb ones? Applies
to
> > entire CL.
> 
> I think we can not do that, assuming that "non-mb ones" refer to the other two
parameters `mastername` and `buildername` here. (I might be wrong, so please let
me know why we could default to those two.)
> 
> Reason: the original code in function `run_mb_and_compile` explicitly said not
to use the mastername and buildername passed in (that code was refactored from
function `compile_specific_targets` at
https://chromium.googlesource.com/chromium/tools/build/+blame/b5a369456d211dc...).
> 
> 
> However, we could default to api.properties['mastername'] and
api.properties['buildername'] instead within function`compile_specific_targets`,
if that is preferred.
> What do you think?

Tentatively sounds good. Maybe that was a bit vague, but I did mean to default
to current behavior.

[stgao, 2015-11-30 17:08:58]

Patchset #3 (id:80001) has been deleted

[stgao, 2015-11-30 17:10:08]

Hi Paweł,

Would you mind another quick review?

Thanks,
Shuotao Gao

[Paweł Hajdan Jr., 2015-11-30 19:35:03]

LGTM

[stgao, 2015-11-30 20:16:42]

Description was changed from

==========
Refactor to pass mb_mastername and mb_buildername as parameters from recipes or
recipe modules.

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

to

==========
Pass mb_mastername and mb_buildername as parameters to override the default
mastername and buildername to run MB.

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

[stgao, 2015-11-30 20:33:35]

Patchset #4 (id:120001) has been deleted

[stgao, 2015-11-30 20:58:42]

Patchset #4 (id:140001) has been deleted

[stgao, 2015-11-30 21:01:34]

The CQ bit was checked by stgao@chromium.org

[stgao, 2015-11-30 21:01:34]

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/1474473004/#ps160001
(title: "Fix nit and docstrings after rebase.")

[commit-bot: I haz the power, 2015-11-30 21:02:37]

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

[commit-bot: I haz the power, 2015-11-30 21:07:31]

Description was changed from

==========
Pass mb_mastername and mb_buildername as parameters to override the default
mastername and buildername to run MB.

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

to

==========
Pass mb_mastername and mb_buildername as parameters to override the default
mastername and buildername to run MB.

BUG=

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

[commit-bot: I haz the power, 2015-11-30 21:07:35]

Committed patchset #4 (id:160001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297744