Teach builders.pyl about RepoPoller.

Teach builders.pyl about RepoPoller.

R=pgervais@google.com, stip
BUG=

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

[ghost stip (do not use), 2015-10-12 22:37:59]

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
File scripts/master/master_gen.py (right):

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:171: for values in builders['schedulers'].values():
this is a confusing variable name. can you make it for value in ...

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:173: continue
I'd prefer to unify with the if v['type'] == 'git_poller' above

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:180:
revlinktmpl='https://android.googlesource.com/platform/%s/+/%s'))
whoa what? we should make the revlinktmpl configurable

[danalbert, 2015-10-12 22:58:25]

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
File scripts/master/master_gen.py (right):

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:171: for values in builders['schedulers'].values():
On 2015/10/12 22:37:59, stip wrote:
> this is a confusing variable name. can you make it for value in ...

Agreed that it could be a better name. Perhaps `scheduler_config`. Not sure what
you're suggesting in the second sentence since that's already what I have
here...

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:173: continue
On 2015/10/12 22:37:59, stip wrote:
> I'd prefer to unify with the if v['type'] == 'git_poller' above

Not sure what you mean by this either. Are you just asking for the if to be
merged in to the loop as a list comprehension? I had written it that way first,
but it seemed clunkier than this. Let me know if I misunderstood...

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:176: repo_url=values['manifest_url'],
FYI I'm also going to change the name of this param to just "repo_url". The name
isn't great, but "manifest_url" is misleading because it should actually be
pointing to the parent of the manifest directory.

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:179: pollInterval=300,
Should I pull out this and the `manifest` parameter into something configurable
as well? The treeStableTimer part of the scheduler was hard coded, so wasn't
sure if we were trying to make people use reasonable defaults to keep the load
down or if it just hadn't been made configurable yet.

https://chromiumcodereview.appspot.com/1400073002/diff/1/scripts/master/maste...
scripts/master/master_gen.py:180:
revlinktmpl='https://android.googlesource.com/platform/%s/+/%s'))
On 2015/10/12 22:37:59, stip wrote:
> whoa what? we should make the revlinktmpl configurable

Oops! Will fix.

[ghost stip (do not use), 2015-10-12 23:53:11]

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
File scripts/master/master_gen.py (right):

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
scripts/master/master_gen.py:175: rev_link_template =
scheduler_config.get('rev_link_template', None)
.get() already returns None, so you can just do rev_link_template =
scheduler_config.get('rev_link_template')

I'd raise a ValueError() in that case though, since you don't want to people to
launch a master and then get confused when it doesn't pick up builds

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
scripts/master/master_gen.py:176: branch = scheduler_config.get('branch', None)
same here, you can just do scheduler_config.get('branch')

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
scripts/master/master_gen.py:180: repo_branches=[branch],
what happens if repo_branches = [None] ? should we raise a ValueError if this
isn't set?

[danalbert, 2015-10-13 00:29:52]

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
File scripts/master/master_gen.py (right):

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
scripts/master/master_gen.py:175: rev_link_template =
scheduler_config.get('rev_link_template', None)
On 2015/10/12 23:53:11, stip wrote:
> .get() already returns None, so you can just do rev_link_template =
> scheduler_config.get('rev_link_template')
> 
> I'd raise a ValueError() in that case though, since you don't want to people
to
> launch a master and then get confused when it doesn't pick up builds

Yeah, I just felt writing it was better to be explicit (it is one of Python's
philosophies, after all) and that it helps show why I'm using `.get(key)`
instead of just `[key]`.

As for raising an error, afaict from looking through the RepoPoller source (the
parameter is undocumented :\), this isn't used for finding builds, but just for
printing the URL point to the revision (possibly from gititles) in the build
page. It isn't a required parameter of RepoPoller, and it's entirely possible
(albeit unlikely) that someone has configured a repo that doesn't actually have
a web interface.

https://chromiumcodereview.appspot.com/1400073002/diff/40001/scripts/master/m...
scripts/master/master_gen.py:180: repo_branches=[branch],
On 2015/10/12 23:53:11, stip wrote:
> what happens if repo_branches = [None] ? should we raise a ValueError if this
> isn't set?

It should actually be either `None` or `[]` rather than `[None]`. RepoPoller
handles the default case by using `['master']`. I need to fix this to actually
do that correctly though.

[danalbert, 2015-10-15 17:10:40]

Ping?

[ghost stip (do not use), 2015-10-15 19:48:47]

didn't know it was back up for review :)

our style is .get('...') without the None, so I'd prefer it without the None

lgtm

[danalbert, 2015-10-15 21:00:06]

Okay, fixed now. Thanks for the review.

[danalbert, 2015-10-15 21:00:11]

The CQ bit was checked by danalbert@google.com

[danalbert, 2015-10-15 21:00:11]

The patchset sent to the CQ was uploaded after l-g-t-m from stip@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1400073002/#ps80001
(title: "Follow convention for {}.get()")

[commit-bot: I haz the power, 2015-10-15 21:00:16]

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

[commit-bot: I haz the power, 2015-10-15 21:06:27]

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