[step] Remove config indirection.

[step] Remove config indirection.

Currently, the path to poplating a recipe engine Step includes the
generation of a "step" module config, a dict, JSON, and finally a
StepConfig named tuple. Shift most of this logic out of the recipe
engine and into the "step" recipe module, and remove the config.

BUG=None
TEST=expectations
  - Simulation passes, no changes to expectations.

Review-Url: https://codereview.chromium.org/2934583003
Committed: https://github.com/luci/recipes-py/commit/264e3cab40de2ee04e33850810188e20d249f9d2

[dnj, 2017-06-10 16:38:18]

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

[dnj, 2017-06-10 16:38:19]

PTAL!

[iannucci, 2017-06-12 18:20:09]

generally lg, but needs tweaks and rebase

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

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:197: return TriggerSpec.create(**trigger_spec)
I don't like this method... the caller should just create TriggerSpec objects
directly.

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:209: return
self._engine.run_step(StepConfig.create(**step_config))
Same here, we should just assert that step_config isa StepConfig

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/run.py#ne...
recipe_engine/run.py:277: if not step_config.infra_step:
newline above

https://codereview.chromium.org/2934583003/diff/20001/recipe_modules/generato...
File recipe_modules/generator_script/api.py (right):

https://codereview.chromium.org/2934583003/diff/20001/recipe_modules/generato...
recipe_modules/generator_script/api.py:98: self.m.step.run_step(**step)
this needs rebasing

[dnj, 2017-06-12 18:59:08]

comments

[dnj, 2017-06-12 19:00:03]

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

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:197: return TriggerSpec.create(**trigger_spec)
On 2017/06/12 18:20:08, iannucci wrote:
> I don't like this method... the caller should just create TriggerSpec objects
> directly.

Done.

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/recipe_ap...
recipe_engine/recipe_api.py:209: return
self._engine.run_step(StepConfig.create(**step_config))
On 2017/06/12 18:20:08, iannucci wrote:
> Same here, we should just assert that step_config isa StepConfig

Done.

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/run.py
File recipe_engine/run.py (right):

https://codereview.chromium.org/2934583003/diff/20001/recipe_engine/run.py#ne...
recipe_engine/run.py:277: if not step_config.infra_step:
On 2017/06/12 18:20:08, iannucci wrote:
> newline above

Done.

[dnj, 2017-06-12 19:05:19]

fix test

[iannucci, 2017-06-12 19:37:58]

lgtm

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api.py
File recipe_modules/step/api.py (right):

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api...
recipe_modules/step/api.py:129: trigger_specs: a list of trigger specifications
do you know enough about this to add a bit more documentation?

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api...
recipe_modules/step/api.py:206: return
self.step_client.run_step(self.step_client.StepConfig(**kwargs))
delete this function

https://codereview.chromium.org/2934583003/diff/60001/unittests/repo_test_uti...
File unittests/repo_test_util.py (right):

https://codereview.chromium.org/2934583003/diff/60001/unittests/repo_test_uti...
unittests/repo_test_util.py:311: '      self.step_client.StepConfig(**%r))' % {
it's is this strictly necessary? This seems like a pretty unfortunate
abstraction violation... Can't this module just depend on recipe_engine/step?

[dnj, 2017-06-13 18:28:38]

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api.py
File recipe_modules/step/api.py (right):

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api...
recipe_modules/step/api.py:129: trigger_specs: a list of trigger specifications
On 2017/06/12 19:37:57, iannucci wrote:
> do you know enough about this to add a bit more documentation?

no :(

https://codereview.chromium.org/2934583003/diff/60001/recipe_modules/step/api...
recipe_modules/step/api.py:206: return
self.step_client.run_step(self.step_client.StepConfig(**kwargs))
On 2017/06/12 19:37:57, iannucci wrote:
> delete this function

Done.

https://codereview.chromium.org/2934583003/diff/60001/unittests/repo_test_uti...
File unittests/repo_test_util.py (right):

https://codereview.chromium.org/2934583003/diff/60001/unittests/repo_test_uti...
unittests/repo_test_util.py:311: '      self.step_client.StepConfig(**%r))' % {
On 2017/06/12 19:37:57, iannucci wrote:
> it's is this strictly necessary? This seems like a pretty unfortunate
> abstraction violation... Can't this module just depend on recipe_engine/step?

Yeah I dunno why it's this way. Changed to call "step".

[dnj, 2017-06-13 18:29:09]

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

[commit-bot: I haz the power, 2017-06-13 18:29:17]

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, 2017-06-13 18:32:27]

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

[commit-bot: I haz the power, 2017-06-13 18:32:28]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-06-13 18:37:26]

(still lgtm)

[dnj, 2017-06-13 18:38:49]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2017-06-13 18:38:58]

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, 2017-06-13 18:41:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497379129031270,
"parent_rev": "cfff93afa16e61285e295d49f6aa2ba64642f330", "commit_rev":
"264e3cab40de2ee04e33850810188e20d249f9d2"}

[commit-bot: I haz the power, 2017-06-13 18:42:01]

Description was changed from

==========
[step] Remove config indirection.

Currently, the path to poplating a recipe engine Step includes the
generation of a "step" module config, a dict, JSON, and finally a
StepConfig named tuple. Shift most of this logic out of the recipe
engine and into the "step" recipe module, and remove the config.

BUG=None
TEST=expectations
  - Simulation passes, no changes to expectations.
==========

to

==========
[step] Remove config indirection.

Currently, the path to poplating a recipe engine Step includes the
generation of a "step" module config, a dict, JSON, and finally a
StepConfig named tuple. Shift most of this logic out of the recipe
engine and into the "step" recipe module, and remove the config.

BUG=None
TEST=expectations
  - Simulation passes, no changes to expectations.

Review-Url: https://codereview.chromium.org/2934583003
Committed:
https://github.com/luci/recipes-py/commit/264e3cab40de2ee04e33850810188e20d24...
==========

[commit-bot: I haz the power, 2017-06-13 18:42:03]

Committed patchset #5 (id:80001) as
https://github.com/luci/recipes-py/commit/264e3cab40de2ee04e33850810188e20d24...