Make output placeholders like json.output index-able by name.

Make output placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a json.output, while we wanted to dump some info from ninja into another json.output. (Please refer to patchset 2 in https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional list. However, this is not explicit, error-prone and might cause complexity when many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198
Committed: https://github.com/luci/recipes-py/commit/13a5e85f41041a66bf717521312d63f6bfc899e4

[stgao, 2016-03-10 00:55:30]

Patchset #1 (id:1) has been deleted

[stgao, 2016-03-10 00:59:50]

Description was changed from

==========
Make json outputs indexable by name.

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

to

==========
Make json outputs indexable by name.

BUG=593198
==========

[stgao, 2016-03-10 01:06:03]

Description was changed from

==========
Make json outputs indexable by name.

BUG=593198
==========

to

==========
Make json outputs indexable by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach to refer to the results in a positional list.
However, this is not  explicit and might cause complexity when many json.outputs
are added in the same step.
Alternative, the approach here is to make json.outputs index-able by a name,
which is more convenient and less error-prone.

BUG=593198
==========

[stgao, 2016-03-10 01:06:27]

Description was changed from

==========
Make json outputs indexable by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach to refer to the results in a positional list.
However, this is not  explicit and might cause complexity when many json.outputs
are added in the same step.
Alternative, the approach here is to make json.outputs index-able by a name,
which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make json.outputs indexable by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach to refer to the results in a positional list.
However, this is not  explicit and might cause complexity when many json.outputs
are added in the same step.
Alternative, the approach here is to make json.outputs index-able by a name,
which is more convenient and less error-prone.

BUG=593198
==========

[stgao, 2016-03-10 01:07:45]

Description was changed from

==========
Make json.outputs indexable by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach to refer to the results in a positional list.
However, this is not  explicit and might cause complexity when many json.outputs
are added in the same step.
Alternative, the approach here is to make json.outputs index-able by a name,
which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make json.outputs index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not  explicit and might cause complexity when many
json.outputs are added in the same step.
Alternative, the approach in this CL is to make json.outputs index-able by
names, which is more convenient and less error-prone.

BUG=593198
==========

[stgao, 2016-03-10 01:08:43]

stgao@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org

[stgao, 2016-03-10 01:08:44]

Hi forks, would you mind a review?

[martiniss, 2016-03-10 02:02:30]

Robbie are you familiar with this code? I haven't really looked at it, but can
dig into it.

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
File recipe_modules/json/example.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
recipe_modules/json/example.py:42: assert result.json.output_all == [[1, 2, 3],
['x', 'y', FULLWIDTH_Z]]
Add a TODO here to delete when the bug is closed?

[iannucci, 2016-03-10 03:17:42]

really really cool :) I like it a lot. Added some comments.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:20: overwrite - whether to over write the value
from a with the same key.
So I think that now they're all named, there's no longer a need for override vs.
non-override. The behavior should always be override-by-name

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:511: setattr(o, placeholder_name + "_all", lst)
so, actually, because of the way the recipe pinning works, you don't need to
support both:
  * prepare this CL to be the new world (e.g. foos instead of foo_all)
  * simulate it's roll into build and build_internal (I can show you how this is
done, I don't have a great document to point you at right now)
  * prepare CLs for build and build_internal as the result of that simulation
(i.e. rewrite them to use foos instead of foo_all), we'll +1 those
  * we'll +1 this
  * land this
  * update the downstream CLs to include the committed version of this in their
recipe.cfg pins
  * land the downstream CLs

We have a bug on file to automate the majority of this process, but we've done
this manually 1-2 times now and it works pretty well. It allows you to see the
full effect of the change downstream, and doesn't leave you in a weird
intermediate state where both interfaces are supported.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py#n...
recipe_engine/util.py:40: def __init__(self, id=None):
I think you can do

  id=DEFAULT_ID

or maybe
  
  id=Placeholder.DEFAULT_ID

?

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py#n...
recipe_engine/util.py:76: if self.id == self.DEFAULT_ID:
use the `is` keyword for comparison (compares pointer)

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/api.py
File recipe_modules/json/api.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/api...
recipe_modules/json/api.py:30: def __init__(self, api, add_json_log, id=id):
we should use something other than 'id', which is a python builtin (yeah I know,
it's a really crummy builtin function name).  Maybe just 'name'?

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
File recipe_modules/json/example.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
recipe_modules/json/example.py:44: assert result.json.outputs['2'] == ['x', 'y',
FULLWIDTH_Z]
awesome :)

[stgao, 2016-03-10 20:29:26]

Description was changed from

==========
Make json.outputs index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not  explicit and might cause complexity when many
json.outputs are added in the same step.
Alternative, the approach in this CL is to make json.outputs index-able by
names, which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not  explicit and might cause complexity when many
json.outputs are added in the same step.
Alternatively, the approach in this CL is to make placeholders like json.outputs
index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

[stgao, 2016-03-10 20:34:23]

Comments are addressed.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:20: overwrite - whether to over write the value
from a with the same key.
On 2016/03/10 03:17:42, iannucci wrote:
> So I think that now they're all named, there's no longer a need for override
vs.
> non-override. The behavior should always be override-by-name

Unfortunately, we still have to do it this way, because of `mod_data`,
'step_data`, and `depend_on_data` in TestData, see line #174-176.

We could only overwrite `placeholder_data`.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:511: setattr(o, placeholder_name + "_all", lst)
On 2016/03/10 03:17:42, iannucci wrote:
> so, actually, because of the way the recipe pinning works, you don't need to
> support both:
>   * prepare this CL to be the new world (e.g. foos instead of foo_all)
>   * simulate it's roll into build and build_internal (I can show you how this
is
> done, I don't have a great document to point you at right now)
>   * prepare CLs for build and build_internal as the result of that simulation
> (i.e. rewrite them to use foos instead of foo_all), we'll +1 those
>   * we'll +1 this
>   * land this
>   * update the downstream CLs to include the committed version of this in
their
> recipe.cfg pins
>   * land the downstream CLs
> 
> We have a bug on file to automate the majority of this process, but we've done
> this manually 1-2 times now and it works pretty well. It allows you to see the
> full effect of the change downstream, and doesn't leave you in a weird
> intermediate state where both interfaces are supported.

Sounds good. Let's go with this approach then.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py#n...
recipe_engine/util.py:40: def __init__(self, id=None):
On 2016/03/10 03:17:42, iannucci wrote:
> I think you can do
> 
>   id=DEFAULT_ID
> 
> or maybe
>   
>   id=Placeholder.DEFAULT_ID
> 
> ?

No. We can't do that, because subclasses will call this with id=None and
id=DEFAULT_ID here won't work in that case.

https://codereview.chromium.org/1773273003/diff/20001/recipe_engine/util.py#n...
recipe_engine/util.py:76: if self.id == self.DEFAULT_ID:
On 2016/03/10 03:17:42, iannucci wrote:
> use the `is` keyword for comparison (compares pointer)

Done.

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/api.py
File recipe_modules/json/api.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/api...
recipe_modules/json/api.py:30: def __init__(self, api, add_json_log, id=id):
On 2016/03/10 03:17:42, iannucci wrote:
> we should use something other than 'id', which is a python builtin (yeah I
know,
> it's a really crummy builtin function name).  Maybe just 'name'?

Sounds good.

Note: PlaceHolder already has a property "name". Will add this as a prefix to
it.

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
File recipe_modules/json/example.py (right):

https://codereview.chromium.org/1773273003/diff/20001/recipe_modules/json/exa...
recipe_modules/json/example.py:42: assert result.json.output_all == [[1, 2, 3],
['x', 'y', FULLWIDTH_Z]]
On 2016/03/10 02:02:30, martiniss wrote:
> Add a TODO here to delete when the bug is closed?

Remove in this CL directly.

[stgao, 2016-03-11 23:38:33]

Patchset #3 (id:60001) has been deleted

[stgao, 2016-03-11 23:40:25]

This CL is now rebased upon the split of Placeholder into input and output ones
(https://codereview.chromium.org/1785543004/).

[iannucci, 2016-03-12 03:43:55]

https://chromiumcodereview.appspot.com/1773273003/diff/80001/recipe_engine/re...
File recipe_engine/recipe_test_api.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/80001/recipe_engine/re...
recipe_engine/recipe_test_api.py:26: assert allow_merge, '2+ test data has the
same key %r' % k
I'm not sure if this error will be very clear?

https://chromiumcodereview.appspot.com/1773273003/diff/80001/recipe_engine/re...
recipe_engine/recipe_test_api.py:91: combineify('output_placeholder_data', ret,
self, other, allow_merge=False)
We should make sure that the following case still works:

  # in module, provides default mocked output with name 'main_output'
  api.step("step", ..., step_test_data=lambda: self.m.raw_io.test_api.output(
     'something', name="main_output"))

  # in GenTests, can override that mocked output
  yield (api.test("name") + 
         api.step_data("step",
           api.raw_io.output("other", name="main_output"))

I'm assuming this assertion here just prevents you from doing

  + api.step_data("step", api.raw_io.output("foo"), api.raw_io.output("bar"))

OR

  + api.step_data("step", api.raw_io.output("foo"))
  + api.step_data("step", api.raw_io.output("bar"))

?

https://chromiumcodereview.appspot.com/1773273003/diff/80001/recipe_engine/ut...
File recipe_engine/util.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/80001/recipe_engine/ut...
recipe_engine/util.py:67: return "%s.%s" % self.namespaces
should we consider a format like "json.output[label]", where if label is
default, it's omitted? I'm thinking about things like step links for json.output
right now; ideally we could preserve the current appearance of json.output links
on the waterfall when there's only out output, but nicely distinguish them if
there are multiple.

[stgao, 2016-03-22 05:30:26]

Patchset #4 (id:100001) has been deleted

[stgao, 2016-03-22 05:31:12]

Patchset #4 (id:120001) has been deleted

[stgao, 2016-03-22 05:58:04]

Comments are addressed. PTAL :)

The corresponding change to recipes/recipe_modules are in:
build/ : https://codereview.chromium.org/1820073003/
build_internal/ : https://chromereviews.googleplex.com/380557013/

I checked depot_tools too, but no change is needed there.

https://codereview.chromium.org/1773273003/diff/80001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1773273003/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:26: assert allow_merge, '2+ test data has the
same key %r' % k
On 2016/03/12 03:43:55, iannucci wrote:
> I'm not sure if this error will be very clear?

Removed, as this assert will make GenTests fail to override the default mocked
output in modules.

https://codereview.chromium.org/1773273003/diff/80001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:91: combineify('output_placeholder_data', ret,
self, other, allow_merge=False)
On 2016/03/12 03:43:55, iannucci wrote:
> We should make sure that the following case still works:
> 
>   # in module, provides default mocked output with name 'main_output'
>   api.step("step", ..., step_test_data=lambda: self.m.raw_io.test_api.output(
>      'something', name="main_output"))
> 
>   # in GenTests, can override that mocked output
>   yield (api.test("name") + 
>          api.step_data("step",
>            api.raw_io.output("other", name="main_output"))

Added a testcase in recipe_modules/raw_io/example.py
And the assertion above was fired, thus I removed the assertion.

> 
> I'm assuming this assertion here just prevents you from doing
> 
>   + api.step_data("step", api.raw_io.output("foo"), api.raw_io.output("bar"))
> 
> OR
> 
>   + api.step_data("step", api.raw_io.output("foo"))
>   + api.step_data("step", api.raw_io.output("bar"))
> 
> ?

Yes, they are the cases the assert was intended for. But it seems we can't do it
now for the first case you mentioned.

https://codereview.chromium.org/1773273003/diff/80001/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/1773273003/diff/80001/recipe_engine/util.py#n...
recipe_engine/util.py:67: return "%s.%s" % self.namespaces
On 2016/03/12 03:43:55, iannucci wrote:
> should we consider a format like "json.output[label]", where if label is
> default, it's omitted? I'm thinking about things like step links for
json.output
> right now; ideally we could preserve the current appearance of json.output
links
> on the waterfall when there's only out output, but nicely distinguish them if
> there are multiple.

This is handled by the full_name below. Moved it up here instead and changed log
name to json.output[name].

Currently, if there is more than one output, only the last one will show up on
the buildbot UI. That's a bug which motivated my idea for this CL :)

[stgao, 2016-03-22 18:03:32]

Description was changed from

==========
Make placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not  explicit and might cause complexity when many
json.outputs are added in the same step.
Alternatively, the approach in this CL is to make placeholders like json.outputs
index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not explicit, error-prone and might cause complexity when
many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like
json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

[stgao, 2016-03-22 18:03:51]

Description was changed from

==========
Make placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not explicit, error-prone and might cause complexity when
many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like
json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make output placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not explicit, error-prone and might cause complexity when
many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like
json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

[iannucci, 2016-03-22 22:54:25]

lgtm

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
File recipe_engine/recipe_test_api.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:30: dest_dict[k] = v
I don't think this needs distinction any more: all output placeholders should
overwrite, right?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:51: return "InputPlaceholderTestData(%r)" %
(self.data,)
we don't need this class at all

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:62: return "OutputPlaceholderTestData(%r)" %
(self.data,)
maybe `OutputPlaceholderTestData(DEFAULT, %r)` ?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
File recipe_engine/step_runner.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
recipe_engine/step_runner.py:466: # This assert also ensures at most one
placeholder has the default name.
well, it also asserts (correctly) that you don't have two placeholders with the
name 'a', too, right?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
recipe_engine/step_runner.py:535: setattr(o, placeholder_name, default_result)
what if there's two placeholders with names, and no 'None' names.

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/u...
File recipe_engine/util.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/u...
recipe_engine/util.py:70: return "%s.%s" % self.namespaces
%s.%s[DEFAULT]

maybe?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_modules/...
File recipe_modules/raw_io/example.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_modules/...
recipe_modules/raw_io/example.py:82: #     AND the default step_test_data ALSO
provides another bad value,
s/default step_test_data/default step_test_data in RunSteps above/

[stgao, 2016-03-22 23:40:25]

Comments are addressed.

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
File recipe_engine/recipe_test_api.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:30: dest_dict[k] = v
On 2016/03/22 22:54:24, iannucci wrote:
> I don't think this needs distinction any more: all output placeholders should
> overwrite, right?

We still need this distinction, as we discussed offline for the following cases:

combineify('mod_data', ret, self, other)
combineify('step_data', ret, self, other)
combineify('depend_on_data', ret, self, other)

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:51: return "InputPlaceholderTestData(%r)" %
(self.data,)
On 2016/03/22 22:54:24, iannucci wrote:
> we don't need this class at all

We need this one for the stdin below at line #139. It's strange to return an
OutputPlaceholderTestData there.

Or should we return None at #139?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/r...
recipe_engine/recipe_test_api.py:62: return "OutputPlaceholderTestData(%r)" %
(self.data,)
On 2016/03/22 22:54:24, iannucci wrote:
> maybe `OutputPlaceholderTestData(DEFAULT, %r)` ?

Done.

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
File recipe_engine/step_runner.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
recipe_engine/step_runner.py:466: # This assert also ensures at most one
placeholder has the default name.
On 2016/03/22 22:54:24, iannucci wrote:
> well, it also asserts (correctly) that you don't have two placeholders with
the
> name 'a', too, right?

Yes. Updated.

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/s...
recipe_engine/step_runner.py:535: setattr(o, placeholder_name, default_result)
On 2016/03/22 22:54:24, iannucci wrote:
> what if there's two placeholders with names, and no 'None' names.

Then the default output is set to None, per our discussion earlier.
Or should we not set it at all in this case?

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/u...
File recipe_engine/util.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_engine/u...
recipe_engine/util.py:70: return "%s.%s" % self.namespaces
On 2016/03/22 22:54:24, iannucci wrote:
> %s.%s[DEFAULT]
> 
> maybe?

Maybe not, this will change the current behavior in links.
I'd keep it as to avoid possible break and avoid updating lots of expectation
files for this.

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_modules/...
File recipe_modules/raw_io/example.py (right):

https://chromiumcodereview.appspot.com/1773273003/diff/130001/recipe_modules/...
recipe_modules/raw_io/example.py:82: #     AND the default step_test_data ALSO
provides another bad value,
On 2016/03/22 22:54:24, iannucci wrote:
> s/default step_test_data/default step_test_data in RunSteps above/

Done.

[stgao, 2016-03-23 05:35:06]

Patchset #6 (id:170001) has been deleted

[stgao, 2016-03-23 05:35:48]

Patchset #5 (id:150001) has been deleted

[stgao, 2016-03-23 17:58:33]

Patchset #6 (id:210001) has been deleted

[stgao, 2016-03-23 17:58:40]

Patchset #5 (id:190001) has been deleted

[iannucci, 2016-03-23 18:50:13]

lgtm

https://codereview.chromium.org/1773273003/diff/230001/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/1773273003/diff/230001/recipe_engine/util.py#...
recipe_engine/util.py:60: return "%s.%s" % self.namespaces
"%s.%s[DEFAULT]" ?

[stgao, 2016-03-23 19:01:44]

https://codereview.chromium.org/1773273003/diff/230001/recipe_engine/util.py
File recipe_engine/util.py (right):

https://codereview.chromium.org/1773273003/diff/230001/recipe_engine/util.py#...
recipe_engine/util.py:60: return "%s.%s" % self.namespaces
On 2016/03/23 18:50:13, iannucci wrote:
> "%s.%s[DEFAULT]" ?

This is used as the log name in buildbots.
For backward-compatibility, we don't want to add the "[DEFAULT]" in case any
service relies on the log name to be "json.output" instead of
"json.output[DEFAULT]".

[stgao, 2016-03-23 19:01:50]

The CQ bit was checked by stgao@chromium.org

[stgao, 2016-03-23 19:01:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/1773273003/#ps250001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-23 19:01:54]

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

[commit-bot: I haz the power, 2016-03-23 19:03:36]

Description was changed from

==========
Make output placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not explicit, error-prone and might cause complexity when
many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like
json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198
==========

to

==========
Make output placeholders like json.output index-able by name.

Use case: in compile step, currently goma dumps its server status to a
json.output, while we wanted to dump some info from ninja into another
json.output. (Please refer to patchset 2 in
https://codereview.chromium.org/1766873002/)

We could take the current approach of referring to the results in a positional
list. However, this is not explicit, error-prone and might cause complexity when
many json.outputs are added in the same step.
Alternatively, the approach in this CL is to make output placeholders like
json.outputs index-able by names, which is more convenient and less error-prone.

BUG=593198

Committed:
https://github.com/luci/recipes-py/commit/13a5e85f41041a66bf717521312d63f6bfc...
==========

[commit-bot: I haz the power, 2016-03-23 19:03:38]

Committed patchset #6 (id:250001) as
https://github.com/luci/recipes-py/commit/13a5e85f41041a66bf717521312d63f6bfc...