refactor: reduce duplicate code in run actions #603
Conversation
Signed-off-by: Joe Shimkus <[email protected]>
Signed-off-by: Joe Shimkus <[email protected]>
Signed-off-by: Joe Shimkus <[email protected]>
Makes the core of all run actions' __call__ a sequence of pre-process, job start, run and post-process. Signed-off-by: Joe Shimkus <[email protected]>
Re-establishes a single log message of "Calling Ansible runner" rather than one per iteration. Signed-off-by: Joe Shimkus <[email protected]>
|
You may want to incorporate the change in #606 |
Thanks. I'd categorize this change as an additional refinement of the run* actions' changes in that PR. |
| from typing import Callable | ||
| from urllib.parse import urljoin | ||
|
|
||
| import drools |
There was a problem hiding this comment.
@jshimkus-rh Do we need to import specific things here maybe?
from drools import ruleset
There was a problem hiding this comment.
@jshimkus-rh The earlier implementation tried using a hierarchical model which @benthomasson didn't want to use and wanted to use composition instead of inheritance, this PR seems to be going using inheritance. So we would need approval from @benthomasson if we wants to use inheritance here.
There was a problem hiding this comment.
Assuming such an objection from @benthomasson I'd like to hear what he considers to be compositional as there's nothing in the previous code (being changed with this PR) that adheres to the conventional definition of composition; there's no facility being re-used across the various classes, rather there's largely duplicated code implemented separately.
There was a problem hiding this comment.
@jshimkus-rh Do we need to import specific things here maybe?
from drools import ruleset
We could do that. I chose not to in order to limit possible confusion between drools and EDA's concept of rulesets.
It was motivated by consistency with eliminating the transitive overriding in tests of the drools apis which would have to be specified against run_base.py file rather than the run_job_template file, as an example.
There was a problem hiding this comment.
My two cents: From what I understand, composition, whether in contrast to inheritance or not, is a pattern used to decouple pieces of logic or distinct responsibilities. In this scenario, where we have shared logic across similar actions, inheritance seems like a fitting approach to me.
ansible_rulebook/action/run_base.py
Outdated
| ) | ||
| await super().__call__() | ||
|
|
||
| async def _do_run(self) -> bool: |
There was a problem hiding this comment.
We don't actually need a boolean as result here and I think it is not a good pattern. Error handling should be done through exceptions.
There was a problem hiding this comment.
We don't have a binary situation; we have success, retriable errors and non-retriable errors. I find I can live with the idea of using StopAsyncIteration and have pushed a change with that. If people would rather have a specific exception for the hierarchy let me know.
Signed-off-by: Joe Shimkus <[email protected]>
|
Actions will become plugins in a future release. My main concern here is that the actions have a minimal interface with few dependencies and are easy to write by third-party developers. Our style for these plugins is to have a single file that defines the plugin where the interface resides. Here run_base breaks that convention by hiding the interface in another file. run_job_template.py and run_workflow_template.py do have a lot of shared code. |
|
I prefer to use inheritance only for is-a relationships and not for code reuse. Library functions are good for code reuse. Here we only have two examples run_job_template.py and run_workflow_template.py which is not enough examples to make good abstractions yet. I prefer a minimum of 3 examples. We should resist the temptation to make abstractions too early. I am fine with leaving the duplicate code as is until we have more examples of how we could abstract it. |
Utilize OO inheritance to reduce code duplicated across the various run actions.