TestStep variables proof of concept

[rihter007]

PoC for #25

Introduced a new interface for step plugins:

type StepsVariables interface {
	Add(tgtID string, name string, v interface{}) error
	Get(tgtID string, name string, v interface{}) error
}

that can add/read variables from current/previous steps.

A mapping between external->internal variables is represented as StepVariablesMapping type which was added to step bundles.

In order to keep PR short, will add parameters template + better testing including e2e in the next PR

[xaionaro]

Get(tgtID string, name string, v interface{}) error

It puts the value into v?

[rihter007]

Get(tgtID string, name string, v interface{}) error

It puts the value into v?

yes, probably I should rename method/variable for more clearity. v -> out

[codecov-commenter]

Codecov Report

Merging #83 (8196bfa) into main (927ea7f) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   63.85%   63.84%   -0.02%     
==========================================
  Files         163      164       +1     
  Lines       10170    10291     +121     
==========================================
+ Hits         6494     6570      +76     
- Misses       2980     3006      +26     
- Partials      696      715      +19     

Flag	Coverage Δ
e2e	49.93% <37.95%> (-0.32%)	⬇️
integration	55.41% <46.20%> (-0.25%)	⬇️
unittests	49.36% <62.50%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/jobmanager/bundles.go	70.83% <ø> (ø)
pkg/pluginregistry/errors.go	33.33% <0.00%> (-66.67%)	⬇️
pkg/storage/limits/limits.go	100.00% <ø> (ø)
plugins/teststeps/echo/echo.go	8.00% <0.00%> (ø)
plugins/teststeps/randecho/randecho.go	6.66% <0.00%> (ø)
plugins/teststeps/sshcmd/sshcmd.go	1.20% <0.00%> (ø)
tests/integ/jobmanager/jobdescriptors.go	71.42% <ø> (ø)
tests/plugins/teststeps/channels/channels.go	0.00% <0.00%> (ø)
tests/plugins/teststeps/hanging/hanging.go	0.00% <0.00%> (ø)
tests/plugins/teststeps/noreturn/noreturn.go	33.33% <0.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[xaionaro]

Get(tgtID string, name string, v interface{}) error

It puts the value into v?

yes, probably I should rename method/variable for more clearity. v -> out

Yeah, let's rename the variable v in the interface to in and out accordingly?

[xaionaro]

I have questions, request, but they can go in another PR. But the review wasn't careful, and I assume @mimir-d also wanted to go through, so I would recommend to wait :)

[xaionaro]

My main concern here is that the PR contributes into tech debt we have in TestRunner. Let me quote myself from another chat:

Basically it is a mess of different ideas intertwined in muddy implicit fuss.

But I guess solving this issue is a whole another epic story.

[rihter007]

My main concern here is that the PR contributes into tech debt we have in TestRunner. Let me quote myself from another chat:

Basically it is a mess of different ideas intertwined in muddy implicit fuss.

But I guess solving this issue is a whole another epic story.

I would say the main tech debt is in convoluted state machine implementation. If it will be moved into a separate logic, everything will be straightforward. Saving additional info at the end of the TestRunner.Run is straightforward

[mimir-d]

returning for the v2 breakage and comments

[mimir-d]

would also be useful to have a "permitted outputs" type of array in the plugin itself, and the framework checks if that's valid when emitting; like the Events one. this would also be able to be extracted as docs, otherwise there's no way of knowing what output a plugin produces without reading the code

[rihter007]

would also be useful to have a "permitted outputs" type of array in the plugin itself, and the framework checks if that's valid when emitting; like the Events one. this would also be able to be extracted as docs, otherwise there's no way of knowing what output a plugin produces without reading the code

I don't know, on the other hand it makes you each time to specify all the variables when using the plugin. We can add this later as improvement. And to get a list of that permitted outputs one still needs to read either a doc or code :)

[mimir-d]

makes you each time to specify all the variables when using the plugin

yes, but it can also act as a filter; if youre only interested in some outputs, you only write those for that specific plugin. The main point is to make things obvious for the reader, and adds some safety on top.
eg.

- name: cmd
  label: cmd0
  parameters:
    - ...
  outputs:
    - stdout

- name: echo
  label: echo0
  parameters:
    - message: "cmd said {{step_output `cmd0.stdout`}}"

# this would be an error
- name: echo
  label: echo1
  parameters:
    - message: "cmd said {{step_output `cmd0.stderr`}}"

And to get a list of that permitted outputs one still needs to read either a doc or code

only valid for the first writer, these descriptors are usually a write once, read many kind of thing so it would have value for the readers

[rihter007]

only valid for the first writer, these descriptors are usually a write once, read many kind of thing so it would have value for the readers
If you mean copy-paste of test descriptors :)

Anyway we can add it later if we start to feel the need of that kind of documentation

[mimir-d]

no longer needed, checked by regex

[rihter007]

Unfortunately it can't be omitted as FindString is used with further length comparison.
(MatchString doesn't finds any match and doesn't suit our needs)

[rihter007]

Rebased the PR. @mimir-d could you please restamp it?
Added https://github.com/linuxboot/contest/releases/tag/v1.0.0 release tag

[rihter007]

Found a bug introduced by my previous PR, the fix is in #121