Formally define step config, pass to stream.

Formally define step config, pass to stream.

Formally define the step configuration dictionary members using a
namedtuple. Replace instances of step dictionary and step dictionary
manipulation with the new "recipe_api.StepConfig".

Pass this new StepConfig struct to the "new_step_stream" stream API call
instead of just some select step properties. The StreamEngine
implementation is responsible for deciding how to process and/or use it.

Similarly define a rendered StepConfig as a RenderedStep. Formally pass
that around to components requiring a rendered value.

This allows for greater control over the specification, manipulation,
and consumption of this internal struct.

Change expectation rendering to refrain from rendering empty StepConfig
parameters. This means that if "cmd" is empty, it will just not appear,
which is more consistent with the intent of the step than an empty list.

BUG=chromium:628770
TEST=local

Committed: https://github.com/luci/recipes-py/commit/be22c9a16dd89f9780e193a24edd98346870271d

[martiniss, 2016-09-01 20:50:28]

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

[martiniss, 2016-09-01 20:50:29]

lgtm

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

https://codereview.chromium.org/2253943003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:175: step_config = None # Make sure we use rendered
step config.
what do you mean "Make sure"?

https://codereview.chromium.org/2253943003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:495: #   c (recipe_api.StepConfig): The step
configuration.
"c" is a little too general; I might lean towards something like
"original_config", but if that's too verbose you can leave it.

[dnj, 2016-09-07 16:37:03]

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

https://codereview.chromium.org/2253943003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:175: step_config = None # Make sure we use rendered
step config.
On 2016/09/01 20:50:29, martiniss wrote:
> what do you mean "Make sure"?

We reuse the "step_config" variable in inner scopes, and we have transformed it
into a rendered step. This will ensure that an Exception will be raised if
someone accidentally references the original config.

https://codereview.chromium.org/2253943003/diff/20001/recipe_engine/step_runn...
recipe_engine/step_runner.py:495: #   c (recipe_api.StepConfig): The step
configuration.
On 2016/09/01 20:50:29, martiniss wrote:
> "c" is a little too general; I might lean towards something like
> "original_config", but if that's too verbose you can leave it.

I went with "config".

[dnj, 2016-09-08 18:35:39]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:35:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 18:40:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 18:40:36]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3123e9630c234e10)

[dnj, 2016-09-08 19:00:52]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 19:00:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 19:03:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 19:03:41]

Dry run: This issue passed the CQ dry run.

[dnj, 2016-09-09 02:27:54]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 02:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 02:42:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 02:42:51]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31259e9ff249be10)

[dnj, 2016-09-09 03:01:54]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 03:02:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 03:16:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 03:16:14]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3125bd30464f6810)

[dnj, 2016-09-09 16:06:10]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 16:06:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 16:20:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 16:20:29]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31288ae415661310)

[dnj, 2016-09-09 20:00:29]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 20:00:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 20:14:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 20:14:11]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/31295b43f136fb10)

[dnj, 2016-09-09 21:45:24]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 21:45:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 22:01:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 22:01:44]

Dry run: Try jobs failed on following builders:
  Recipe Roll Downstream Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3129bb52c3e6fa10)

[dnj, 2016-09-10 00:20:31]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 00:20:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 00:27:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 00:27:55]

Dry run: This issue passed the CQ dry run.

[dnj, 2016-09-12 19:07:00]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-09-12 19:07:00]

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

[commit-bot: I haz the power, 2016-09-12 19:07:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 19:14:50]

Description was changed from

==========
Formally define step config, pass to stream.

Formally define the step configuration dictionary members using a
namedtuple. Replace instances of step dictionary and step dictionary
manipulation with the new "recipe_api.StepConfig".

Pass this new StepConfig struct to the "new_step_stream" stream API call
instead of just some select step properties. The StreamEngine
implementation is responsible for deciding how to process and/or use it.

Similarly define a rendered StepConfig as a RenderedStep. Formally pass
that around to components requiring a rendered value.

This allows for greater control over the specification, manipulation,
and consumption of this internal struct.

Change expectation rendering to refrain from rendering empty StepConfig
parameters. This means that if "cmd" is empty, it will just not appear,
which is more consistent with the intent of the step than an empty list.

BUG=chromium:628770
TEST=local
==========

to

==========
Formally define step config, pass to stream.

Formally define the step configuration dictionary members using a
namedtuple. Replace instances of step dictionary and step dictionary
manipulation with the new "recipe_api.StepConfig".

Pass this new StepConfig struct to the "new_step_stream" stream API call
instead of just some select step properties. The StreamEngine
implementation is responsible for deciding how to process and/or use it.

Similarly define a rendered StepConfig as a RenderedStep. Formally pass
that around to components requiring a rendered value.

This allows for greater control over the specification, manipulation,
and consumption of this internal struct.

Change expectation rendering to refrain from rendering empty StepConfig
parameters. This means that if "cmd" is empty, it will just not appear,
which is more consistent with the intent of the step than an empty list.

BUG=chromium:628770
TEST=local

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

[commit-bot: I haz the power, 2016-09-12 19:14:51]

Committed patchset #4 (id:60001) as
https://github.com/luci/recipes-py/commit/be22c9a16dd89f9780e193a24edd9834687...

[iannucci, 2016-09-12 19:41:05]

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

[iannucci, 2016-09-12 19:41:08]

lgtm

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
File recipe_engine/recipe_api.py (right):

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:22: _StepConfig =
collections.namedtuple('StepConfig',
I would name this inner class _StepConfig too. Otherwise type errors may be hard
to debug.

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:28: """
StepConfig is the representation of a raw step as the recipe_engine sees it. You
should use the standard 'step' recipe module, which will construct and pass this
data to the engine for you, instead. The only reason why you would need to worry
about this object is if you're modifying the step module itself.

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:30: name: name of the step, will appear in buildbots
waterfall
let's add types to these, e.g.

name (str): ...

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:31: cmd: command to run, list of one or more strings
cmd (list): command to run. Acceptable types: str, Path, Placeholder

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:32: cwd: absolute path to working directory for the
command
cwd (Path): ...

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:33: env: dict with overrides for environment
variables
env (dict): overrides for environment variables. Each value is % formatted with
the entire existing os.environ. A value of `None` will remove that envvar from
the environ. e.g.
  {"envvar": "%(envvar)s;%(envvar2)s;extra",
   "delete_this": None,
   "static_value": "something" }

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:34: allow_subannotations: if True, lets the step
emit its own annotations
NOTE: Enabling this can cause some buggy behavior. Please strongly consider
using step_result.presentation instead. If you have questions, please contact
infra-dev@chromium.org.

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:35: trigger_specs: a list of trigger specifications,
see also _trigger_builds.
example schema? Actually... maybe another named tuple?

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:37: infra_step: if True, this is an infrastructure
step.
...... will raise InfraFailure instead of StepFailure.

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:40: ignored).
I think this (and stdin+stderr) actually must be Placeholders

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:44: ok_ret: Allowed return code list.
set of return codes allowed. If the step process returns something not on this
list, it will raise a StepFailure (or InfraFailure if infra_step==True).

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:45: step_test_data: Possible associated step test
data.
step_test_data (func -> recipe_test_api.StepTestData): A factory which returns a
StepTestData object that will be used as the default test data for this step.
The recipe author can override/augment this object in the GenTests function.

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:46: nest_level: the step's nesting level.
int?

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

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/step_runn...
recipe_engine/step_runner.py:398: _STEP_CONFIG_RENDER_BLACKLIST = set((
frozenset

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/step_runn...
recipe_engine/step_runner.py:442: buf =
self._annotator.step_buffer(rendered_step.config.name)
why still use rendered_step here instead of rs?

https://codereview.chromium.org/2253943003/diff/60001/recipe_engine/step_runn...
recipe_engine/step_runner.py:450: self._step_history[rendered_step.config.name]
= rs
here too?

https://codereview.chromium.org/2253943003/diff/60001/recipe_modules/platform...
File recipe_modules/platform/example.expected/linux64.json (left):

https://codereview.chromium.org/2253943003/diff/60001/recipe_modules/platform...
recipe_modules/platform/example.expected/linux64.json:3: "cmd": [],
with the addition of step whitelisting, I think we should render this even when
it's empty. That will also make your recipe-roller experience much easier.