Allow setting arbitrary 'git cl try' properties with buildbucket.

Allow setting arbitrary 'git cl try' properties with buildbucket.

This allows calling 'git cl try' with properties. Each
property is set with the option -p key=value, where value
will be treated as json if possible or as string.

Examples for -p and pitfalls:

key=string -> 'string'
key=1 -> 1
key=[1,2,3] -> [1,2,3]
key=["one","two","three"] -> '[one,two,three]'
'key=["one","two","three"]' -> ['one','two','three']

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

[Michael Achenbach, 2015-09-14 14:16:14]

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

[Michael Achenbach, 2015-09-14 14:18:23]

PTAL - this orients on how run_recipe works. Parameters-only in this CL. The
functionality for a json properties file or stdin might come with a follow up.

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py
File git_cl.py (left):

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#oldcode232
git_cl.py:232: override_properties=None):
This was added by me a while ago in order to hook into the trigger_try_jobs
method from a client. It was never used and is obsolete with the new option.

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

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode217
git_cl.py:217: def _get_properties_from_options(options):
This is partially copied from
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/tools/ru...
but using json instead of python.

[tandrii(chromium), 2015-09-14 14:52:22]

lg-tm, but let's have nodir or someone else review this.

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py
File git_cl.py (left):

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#oldcode232
git_cl.py:232: override_properties=None):
On 2015/09/14 14:18:23, Michael Achenbach wrote:
> This was added by me a while ago in order to hook into the trigger_try_jobs
> method from a client. It was never used and is obsolete with the new option.

Acknowledged.

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

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode223
git_cl.py:223: pass  # If a value couldn't be evaluated, silently ignore it.
how about:
s/, silently ignore it./, treat it as a string

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode287
git_cl.py:287: parameters['properties'].update(properties)
I'm not sure if overriding all properties is a good thing in terms on the scale
of tradeoffs between customizability and ease of debug.

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode3088
git_cl.py:3088: "key2=value2 etc (buildbucket only). The value will treated as"
nit: missing "be" : will BE treated

[nodir, 2015-09-14 16:12:27]

lgtm as long as it is tested % crbug.com/488235 is still blocked on
crbug.com/487822. If people start to use it, CQ interpret these builds as full
builds.

[Michael Achenbach, 2015-09-14 19:03:06]

On 2015/09/14 16:12:27, nodir wrote:
> lgtm as long as it is tested % crbug.com/488235 is still blocked on
> crbug.com/487822. If people start to use it, CQ interpret these builds as full
> builds.

Tested manually, e.g. send try jobs like:
https://uberchromegw.corp.google.com/i/internal.client.v8/builders/v8_linux64...

...which incorporate a few properties, like testfilter and extra_args. I also
tested with wrong inputs.

Do you also refer to any automated testing? I looked a bit through the git-cl
tests - but didn't really find anything about git-cl try, where to continue
from...

[nodir, 2015-09-14 20:25:25]

On 2015/09/14 19:03:06, Michael Achenbach wrote:
> On 2015/09/14 16:12:27, nodir wrote:
> > lgtm as long as it is tested % crbug.com/488235 is still blocked on
> > crbug.com/487822. If people start to use it, CQ interpret these builds as
full
> > builds.
> 
> Tested manually, e.g. send try jobs like:
>
https://uberchromegw.corp.google.com/i/internal.client.v8/builders/v8_linux64...
> 
> ...which incorporate a few properties, like testfilter and extra_args. I also
> tested with wrong inputs.
> 
> Do you also refer to any automated testing? I looked a bit through the git-cl
> tests - but didn't really find anything about git-cl try, where to continue
> from...

I meant manual testing, that you did.

[Michael Achenbach, 2015-09-15 08:32:57]

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

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode223
git_cl.py:223: pass  # If a value couldn't be evaluated, silently ignore it.
On 2015/09/14 14:52:22, tandrii(chromium) wrote:
> how about:
> s/, silently ignore it./, treat it as a string

Done.

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode287
git_cl.py:287: parameters['properties'].update(properties)
On 2015/09/14 14:52:22, tandrii(chromium) wrote:
> I'm not sure if overriding all properties is a good thing in terms on the
scale
> of tradeoffs between customizability and ease of debug. 

I'd just like to resemble the buildbucket api, where you can set all these
properties to arbitrary values. Nobody needs to make use of this, but can.

https://codereview.chromium.org/1339073002/diff/20001/git_cl.py#newcode3088
git_cl.py:3088: "key2=value2 etc (buildbucket only). The value will treated as"
On 2015/09/14 14:52:22, tandrii(chromium) wrote:
> nit: missing "be" : will BE treated

Done.

[Michael Achenbach, 2015-09-15 08:42:52]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2015-09-15 08:42:52]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1339073002/#ps40001
(title: "Review")

[commit-bot: I haz the power, 2015-09-15 08:42:57]

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

[commit-bot: I haz the power, 2015-09-15 08:45:24]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296683

[tandrii(chromium), 2015-09-15 08:49:49]

lgtm