Add decorations to bot names for modifying parts of try sets.

Add decorations to bot names for modifying parts of try sets.

BUG=None
TEST=New jobs supported.

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

[Peter Mayo, 2011-12-07 16:38:00]

Self-review.

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
File masters/master.tryserver.chromium/master.cfg (right):

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
masters/master.tryserver.chromium/master.cfg:29: LISTEN_TO_SVN = True
Testing: remove

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base.py
File scripts/master/try_job_base.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base....
scripts/master/try_job_base.py:81: 'Chose %s for job %s' % (','.join(bot_names),
options['reason']))
Only the tense change is necessary.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base....
scripts/master/try_job_base.py:88: keys = ('clobber', 'issue', 'patchset',
'rietveld', 'testfilter', 'builds')
This can go away now.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb8.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:45: builderNames=parsed_job['bot'],
We may need to break this down to do separately per bot.
BB8 testing still required, I suppose.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_decor...
File scripts/master/try_job_decoration.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_decor...
scripts/master/try_job_decoration.py:14: props.updateFromProperties(properties)
Should pull the props to be modified, and [ut them back.

[Peter Mayo, 2011-12-09 21:29:08]

Some rework still retesting, but I think this is good to look at now.

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
File masters/master.tryserver.chromium/master.cfg (right):

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
masters/master.tryserver.chromium/master.cfg:29: LISTEN_TO_SVN = True
On 2011/12/07 16:38:00, Peter Mayo wrote:
> Testing: remove

Done.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base.py
File scripts/master/try_job_base.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base....
scripts/master/try_job_base.py:81: 'Chose %s for job %s' % (','.join(bot_names),
options['reason']))
On 2011/12/07 16:38:00, Peter Mayo wrote:
> Only the tense change is necessary.

Done.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base....
scripts/master/try_job_base.py:88: keys = ('clobber', 'issue', 'patchset',
'rietveld', 'testfilter', 'builds')
On 2011/12/07 16:38:00, Peter Mayo wrote:
> This can go away now.

Done.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb8.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:45: builderNames=parsed_job['bot'],
On 2011/12/07 16:38:00, Peter Mayo wrote:
> We may need to break this down to do separately per bot.
> BB8 testing still required, I suppose.

Done.

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_decor...
File scripts/master/try_job_decoration.py (right):

http://codereview.chromium.org/8847005/diff/2001/scripts/master/try_job_decor...
scripts/master/try_job_decoration.py:14: props.updateFromProperties(properties)
On 2011/12/07 16:38:00, Peter Mayo wrote:
> Should pull the props to be modified, and [ut them back.

Done.

[M-A Ruel, 2011-12-09 21:50:29]

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
File masters/master.tryserver.chromium/master.cfg (right):

http://codereview.chromium.org/8847005/diff/2001/masters/master.tryserver.chr...
masters/master.tryserver.chromium/master.cfg:29: LISTEN_TO_SVN = True
On 2011/12/07 16:38:00, Peter Mayo wrote:
> Testing: remove

Trick:
Create a git branch "testing"
Create a new branch "my_cl"
git branch --set-upstream my_cl testing
git pull
git cl upload

then you won't have these test things showing up as diff.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base.py
File scripts/master/try_job_base.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base....
scripts/master/try_job_base.py:75: build_names = []
Why build_names? It's confusing. It's not the name of a build but of a builder.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base....
scripts/master/try_job_base.py:79: bot_names = [b.split(':')[0] for b in
options['bot']]
I'd keep the log intact, it's more informative.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb7.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb7.py:110: builder_name.split(':')[0:1],
builder_name.split(':', 1)[0:1],

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb7.py:115: ':'.join(builder_name.split(':')[1:])))
''.join(builder_name.split(':', 1)[1:])

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb8.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:37: for build in parsed_job['bot']:
builder

build is confusing.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:38: bot = build.split(':')[0]
bot = build.split(':', 1)[0]

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:44: ':'.join(build.split(':')[1:])))
''.join(build.split(':', 1)[1:])
will work even if no ':' is present in the string.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
File scripts/master/try_job_decoration.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
scripts/master/try_job_decoration.py:8: def parse_decoration(properties,
decorations):
I think I'd prefer that in try_job_base.py.

[Peter Mayo, 2011-12-10 01:08:26]

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base.py
File scripts/master/try_job_base.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base....
scripts/master/try_job_base.py:75: build_names = []
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> Why build_names? It's confusing. It's not the name of a build but of a
builder.

But it's not always the name of a builder.  win is the name of a builder, but
win:compile is not.  I think builder would be misleading.  Build has too many
meanings in this context, but was the best I could think of - another alternate?

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base....
scripts/master/try_job_base.py:79: bot_names = [b.split(':')[0] for b in
options['bot']]
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> I'd keep the log intact, it's more informative.

Interesting, then chose is often the wrong verb.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb7.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb7.py:110: builder_name.split(':')[0:1],
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> builder_name.split(':', 1)[0:1],

Done.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb7.py:115: ':'.join(builder_name.split(':')[1:])))
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> ''.join(builder_name.split(':', 1)[1:])

Done.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
File scripts/master/try_job_base_bb8.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:37: for build in parsed_job['bot']:
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> builder
> 
> build is confusing.

Same as other instance.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:38: bot = build.split(':')[0]
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> bot = build.split(':', 1)[0]

Done.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base_...
scripts/master/try_job_base_bb8.py:44: ':'.join(build.split(':')[1:])))
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> ''.join(build.split(':', 1)[1:])
> will work even if no ':' is present in the string.

FWIW, so does the existing syntax, by experiment at least.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
File scripts/master/try_job_decoration.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
scripts/master/try_job_decoration.py:8: def parse_decoration(properties,
decorations):
On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> I think I'd prefer that in try_job_base.py.

Causes a circular dependency base-> bb7 -> base
and bb8


As well as the implementation of a language for describing a set of
modifications being separable from a try job.

[M-A Ruel, 2011-12-12 20:46:58]

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base.py
File scripts/master/try_job_base.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_base....
scripts/master/try_job_base.py:75: build_names = []
Personally, I wouldn't touch this file at all in this CL.

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
File scripts/master/try_job_decoration.py (right):

http://codereview.chromium.org/8847005/diff/5009/scripts/master/try_job_decor...
scripts/master/try_job_decoration.py:8: def parse_decoration(properties,
decorations):
On 2011/12/10 01:08:27, Peter Mayo wrote:
> On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> > I think I'd prefer that in try_job_base.py.
> 
> Causes a circular dependency base-> bb7 -> base
> and bb8
> 
> 
> As well as the implementation of a language for describing a set of
> modifications being separable from a try job.

No because since it's a mixin, you can just call the superclass member;

self.parse_decoration(foo)

so no it's not a "circular" dependency. See how self.last_good_urls is used in
the mixins.

[Peter Mayo, 2011-12-12 21:55:56]

On 2011/12/12 20:46:58, Marc-Antoine Ruel wrote:
> File scripts/master/try_job_base.py (right):
> ...
> Personally, I wouldn't touch this file at all in this CL.
> 
> File scripts/master/try_job_decoration.py
> > On 2011/12/09 21:50:29, Marc-Antoine Ruel wrote:
> > > I think I'd prefer that in try_job_base.py.


I presume the previous comment is scoped to what has simplified down to a name
change.


Moving the parsing into the base... not completely convinced, but not
sufficiently opposed.

[M-A Ruel, 2011-12-12 22:32:44]

On 2011/12/12 21:55:56, Peter Mayo wrote:
> I presume the previous comment is scoped to what has simplified down to a name
> change.
> 
> 
> Moving the parsing into the base... not completely convinced, but not
> sufficiently opposed.

Ah right, I missed that.
Blah, ok, but I still don't like build_names. :)

lgtm

[Peter Mayo, 2011-12-13 00:17:00]

Retested:  Started submitted jobs with and without decoration.

[commit-bot: I haz the power, 2011-12-13 00:17:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petermayo@chromium.org/8847005/14001

[commit-bot: I haz the power, 2011-12-13 00:19:12]

Presubmit check for 8847005-14001 failed and returned exit status 1.

Running presubmit commit checks ...
************* Module slave.slave_utils
W0311:384,0: Bad indentation. Found 8 spaces, expected 6
W0622:378,0:GSUtilCopyDir: Redefining built-in 'file'

** Presubmit ERRORS **
Fix pylint errors first.

Presubmit checks took 115.2s to calculate.

[commit-bot: I haz the power, 2011-12-13 01:40:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petermayo@chromium.org/8847005/14001

[commit-bot: I haz the power, 2011-12-13 01:42:49]

Change committed as 114145