[th/plugin-class] introduce a common baseclass "pluginbase.Plugin" and some refactoring #41
Conversation
- enum_convert() now also accepts the input string as integer and in any letter case. - add enum_convert_list() which parses a list/range of enum values. Use it from TestConfig.parse_test_cases(), which parses the input test_cases configuration. - while at it, also accept "*" for all enum values.
The "if "iperf" in input_ct" checks are unusual. It accepts various bogus strings. Instead, use enum_convert(), which already can convert various strings (e.g. different letter case) to the enum. No need to implement something else.
This is not implemented and does not exist (yet). Remove.
Let's not do multi line imports. It's a style choice that each import should be on a separate line. The benefit is that the lines align (if they are sorted) and it's easier to read. Also, a diff that adds or removes an import, will only add/remove the line that affects the import. Also, group all imports to first do imports of external modules and stdlib, then do "from" imports fro external modules and stdlib, then imports of internal modules and finally "from" imports of internal modules. There is also a flake8-import-order module, but that has additional grouping requirements. That seems too picky and the patch does not follow its suggestions.
This really belongs to "host" module. Also, we should have a defined order in which modules include each other. Currently, "host.py" imports "common.py". But that is not a good order, because host.Host abstracts a way to call programs (on potentially other hosts, even if that is currently not implemented). We may want to implement helper methods based on the ability to call programs, and those may fit well in "common.py". Thus, "host.py" should not have a dependency on "common.py" (but the other way around). Optimally, we also move host.Host.ipa() to "common.py", because this seems a more general helper method, and not something that belongs to host.Host. "host.py" should probably not contain helpers that involve invoking a program. For that, "common.py" would be the better place. That is not done, but it shows that the better order of dependency is that "common" imports "host", and not the other way around.
There should be a clear hierarchy of modules. That is, which modules depend on which. For example, - "logger.py" can only use stdlib and third party modules. - "host.py" can only use the above. - "common.py" can only use the above. Especially the modules "logger.py", "host.py" and "common.py" contain general purpose helpers (aside the fact that they use third party modules). As such, they may be similarly useful in other projects. For example, "cluster-deployment-automation" has similar modules, although with a different implementation. A useful (by maybe not always optimal way) of code reuse is copy-and-paste. It should be possibly to just copy-and-paste these 3 base modules to other projects and use there with no changes. That requires that they don't contain code specific to the project. Move code that is only relevant for this project to a new module "tftbase.py", where "tft" stands for "TrafficFlowTests".
We have some simple data classes that don't have additional dependencies or logic. "PluginResult" is one of them. Move it from "evaluator" to "tftbase" module. That makes more sense, because next we will add a "pluginbase" module, that will require the "PluginResult" and be used by "evaluator". It works bad, if "PluginResult" is inside "evaluator" module. To avoid cycles, move the simple types "PluginResult" to "tftbase".
Give the plugin modules a "plugin" prefix. The benfit is, that we can easier see what plugins we have. Also, note that the plugins are almost never directly imported, used or referred to. The only place that imports these modules is during plugin registration. Hence, the longer name is not a problem, because they are only imported at one place.
We have a "Task" base class, which is used for various purposes. Tasks returned by a "pluginbase.Plugin", should be of a subtype "PluginTask". The benefit is that from this task, we can get the plugin back and in general there is stronger typing where plugins only deal with plugin tasks.
| from typing import Optional | ||
| from typing import Type | ||
| from typing import TypeVar | ||
| from typing import cast |
There was a problem hiding this comment.
OOC, why specify these one per line rather than in a single import? Is this enforced by some linter?
There was a problem hiding this comment.
I try to give the reasoning in the commit message. If that is unclear, then at least the commit message should be improved. But let me explain here:
it's not enforced by a linter. There is https://pypi.org/project/flake8-import-order/ , however that does not enforce single-line imports. flake8-import-order is also not followed, because it has additional style suggestions, that I found cumbersome.
The only reason why I did this, is because IMO this is more readable and nicer with future changes (a diff adds/removes lines for the import that changes, nothing else). I guess, that opinion is not unversally shared, if you see https://stackoverflow.com/questions/15011367/python-module-import-single-line-vs-multi-line .
In short, just my preference. But we can adopt another style, if that is preferred.
There was a problem hiding this comment.
I see, sorry to make you repeat yourself, I missed the commit message. Makes sense to me, I like the multi-line
| ValidateOffload(tc, perf_client, tenant), | ||
| ] | ||
|
|
||
| def eval_log( |
There was a problem hiding this comment.
I like the use of the abstract class to add plugin.enable.
I am not so sure about moving the evaluation logic into the plugin. Originally, the goal was to have this code separated into two parts:
- the flow test tool which takes in a config and creates a raw json output
- the evaluator which takes in that json output, parses out the meaningful data, and determines success/failure.
It seems like these roles would be less differentiated by moving the evaluation logic into the plugin class. WDYT?
There was a problem hiding this comment.
for now, there is only one plugin-specific implementation of eval_log(). It's hard to guess, whether a new plugin will have widely different API, that makes it hard to abstract. In any case, when that plugin gets added, then it's easy to modify the code, so that it can again be good. Maybe something like:
plugin = pluginbase.get_by_name(plugin_output.name)
if plugin.eval_log_is_special_kind():
plugin_result = plugin.eval_log_special_kind(special_args...)
else:
plugin_result = plugin.eval_log(plugin_output, run["flow_test"].tft_metadata)
in any case, for the currently existing code, this abstract well.
the evaluator which takes in that json output, parses out the meaningful data, and determines success/failure.
This didn't change, did it?
All that the branch does at this place is replace
if plugin_output.name == VALIDATE_OFFLOAD_PLUGIN:
self._eval_validate_offload(...with
plugin = pluginbase.get_by_name(plugin_output.name)
plugin_result = plugin.eval_log(
plugin_output, run["flow_test"].tft_metadata
)It really just moves code around. And having code specific to "validate_offload" plugin in evaluate.py is not nice.
The plugins should have a base class, so we can treat them in a generic way.
Add "pluginbase.Plugin" and plugin-specific subclasses.
The users no longer need to know the plugin. They can call
pluginbase.get_by_name(plugin_name)to get a plugin, and then they can get everything they need from the abstract class, without knowing the plugin explicitly.Also, move some code around. For example, move code specific to "ocp-traffic-flow-tests" from "common.py" to a new module "tftbase.py". The benefit is that "common.py" only contains general purpose utilities (and theoretically could also be copy-and-pasted to another python project). Even if that copy-and-pasting never happens, this is a better organization of the code, where modules have a clearer purpose and a clearer order of dependencies.